-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
Add file/screenshot upload to QA feedback interface #1018
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 8 commits
e694aee
915a733
9150597
32b6e7a
7174864
0968189
93ab00b
4ea7a39
34950de
13b6f5f
6f46661
4e7a642
3725973
973c352
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,14 +1,15 @@ | ||
| import { useState, useRef, useEffect, useCallback } from 'react'; | ||
| import { useProjectStore } from '../../../stores/project-store'; | ||
| import { checkTaskRunning, isIncompleteHumanReview, getTaskProgress } from '../../../stores/task-store'; | ||
| import type { Task, TaskLogs, TaskLogPhase, WorktreeStatus, WorktreeDiff, MergeConflict, MergeStats, GitConflictInfo } from '../../../../shared/types'; | ||
| import type { Task, TaskLogs, TaskLogPhase, WorktreeStatus, WorktreeDiff, MergeConflict, MergeStats, GitConflictInfo, ImageAttachment } from '../../../../shared/types'; | ||
|
|
||
| export interface UseTaskDetailOptions { | ||
| task: Task; | ||
| } | ||
|
|
||
| export function useTaskDetail({ task }: UseTaskDetailOptions) { | ||
| const [feedback, setFeedback] = useState(''); | ||
| const [feedbackImages, setFeedbackImages] = useState<ImageAttachment[]>([]); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧹 Nitpick | 🔵 Trivial Consider resetting The ♻️ Suggested defensive resetAdd an effect to clear images when the task ID changes: + // Reset feedback images when task changes
+ useEffect(() => {
+ setFeedbackImages([]);
+ }, [task.id]);🤖 Prompt for AI Agents |
||
| const [isSubmitting, setIsSubmitting] = useState(false); | ||
| const [activeTab, setActiveTab] = useState('overview'); | ||
| const [isUserScrolledUp, setIsUserScrolledUp] = useState(false); | ||
|
|
@@ -211,6 +212,26 @@ export function useTaskDetail({ task }: UseTaskDetailOptions) { | |
| }); | ||
| }, []); | ||
|
|
||
| // Add a feedback image | ||
| const addFeedbackImage = useCallback((image: ImageAttachment) => { | ||
| setFeedbackImages(prev => [...prev, image]); | ||
| }, []); | ||
|
|
||
| // Add multiple feedback images at once | ||
| const addFeedbackImages = useCallback((images: ImageAttachment[]) => { | ||
| setFeedbackImages(prev => [...prev, ...images]); | ||
| }, []); | ||
|
|
||
| // Remove a feedback image by ID | ||
| const removeFeedbackImage = useCallback((imageId: string) => { | ||
| setFeedbackImages(prev => prev.filter(img => img.id !== imageId)); | ||
| }, []); | ||
|
|
||
| // Clear all feedback images | ||
| const clearFeedbackImages = useCallback(() => { | ||
| setFeedbackImages([]); | ||
| }, []); | ||
|
Comment on lines
+264
to
+282
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. These new helper functions ( To improve encapsulation and make the For example, |
||
|
|
||
| // Track if we've already loaded preview for this task to prevent infinite loops | ||
| const hasLoadedPreviewRef = useRef<string | null>(null); | ||
|
|
||
|
|
@@ -245,6 +266,7 @@ export function useTaskDetail({ task }: UseTaskDetailOptions) { | |
| return { | ||
| // State | ||
| feedback, | ||
| feedbackImages, | ||
| isSubmitting, | ||
| activeTab, | ||
| isUserScrolledUp, | ||
|
|
@@ -285,6 +307,7 @@ export function useTaskDetail({ task }: UseTaskDetailOptions) { | |
|
|
||
| // Setters | ||
| setFeedback, | ||
| setFeedbackImages, | ||
| setIsSubmitting, | ||
| setActiveTab, | ||
| setIsUserScrolledUp, | ||
|
|
@@ -318,5 +341,9 @@ export function useTaskDetail({ task }: UseTaskDetailOptions) { | |
| handleLogsScroll, | ||
| togglePhase, | ||
| loadMergePreview, | ||
| addFeedbackImage, | ||
| addFeedbackImages, | ||
| removeFeedbackImage, | ||
| clearFeedbackImages, | ||
| }; | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Form state cleared regardless of submission success
Medium Severity
The
handleRejectfunction clearsfeedbackImages(andfeedback) unconditionally after callingsubmitReview, without checking whether submission succeeded. ThesubmitReviewfunction returnsfalseon failure, but this return value is ignored. If submission fails, users lose their feedback text and attached images with no error indication. While text clearing was pre-existing, the newsetFeedbackImages([])call extends this data loss to images, which can be harder to recreate (e.g., screenshots). Compare withhandleDeletein the same file which correctly checksresult.successbefore cleanup.