-
-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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 all 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,8 +1,8 @@ | ||||||
| import { ipcMain, BrowserWindow } from 'electron'; | ||||||
| import { IPC_CHANNELS, AUTO_BUILD_PATHS, getSpecsDir } from '../../../shared/constants'; | ||||||
| import type { IPCResult, TaskStartOptions, TaskStatus } from '../../../shared/types'; | ||||||
| import type { IPCResult, TaskStartOptions, TaskStatus, ImageAttachment } from '../../../shared/types'; | ||||||
| import path from 'path'; | ||||||
| import { existsSync, readFileSync, writeFileSync, renameSync, unlinkSync } from 'fs'; | ||||||
| import { existsSync, readFileSync, writeFileSync, renameSync, unlinkSync, mkdirSync } from 'fs'; | ||||||
| import { spawnSync, execFileSync } from 'child_process'; | ||||||
| import { getToolPath } from '../../cli-tool-manager'; | ||||||
| import { AgentManager } from '../../agent'; | ||||||
|
|
@@ -318,7 +318,8 @@ export function registerTaskExecutionHandlers( | |||||
| _, | ||||||
| taskId: string, | ||||||
| approved: boolean, | ||||||
| feedback?: string | ||||||
| feedback?: string, | ||||||
| images?: ImageAttachment[] | ||||||
| ): Promise<IPCResult> => { | ||||||
| // Find task and project | ||||||
| const { task, project } = findTaskAndProject(taskId); | ||||||
|
|
@@ -407,10 +408,65 @@ export function registerTaskExecutionHandlers( | |||||
| console.warn('[TASK_REVIEW] Writing QA fix request to:', fixRequestPath); | ||||||
| console.warn('[TASK_REVIEW] hasWorktree:', hasWorktree, 'worktreePath:', worktreePath); | ||||||
|
|
||||||
| // Process images if provided | ||||||
| let imageReferences = ''; | ||||||
| if (images && images.length > 0) { | ||||||
| const imagesDir = path.join(targetSpecDir, 'feedback_images'); | ||||||
| try { | ||||||
| if (!existsSync(imagesDir)) { | ||||||
| mkdirSync(imagesDir, { recursive: true }); | ||||||
| } | ||||||
| const savedImages: string[] = []; | ||||||
| for (const image of images) { | ||||||
| try { | ||||||
| if (!image.data) { | ||||||
| console.warn('[TASK_REVIEW] Skipping image with no data:', image.filename); | ||||||
| continue; | ||||||
| } | ||||||
| // Server-side MIME type validation (defense in depth - frontend also validates) | ||||||
| // Reject missing mimeType to prevent bypass attacks | ||||||
| const ALLOWED_MIME_TYPES = ['image/png', 'image/jpeg', 'image/jpg', 'image/gif', 'image/webp', 'image/svg+xml']; | ||||||
| if (!image.mimeType || !ALLOWED_MIME_TYPES.includes(image.mimeType)) { | ||||||
| console.warn('[TASK_REVIEW] Skipping image with missing or disallowed MIME type:', image.mimeType); | ||||||
| continue; | ||||||
| } | ||||||
| // Sanitize filename to prevent path traversal attacks | ||||||
| const sanitizedFilename = path.basename(image.filename); | ||||||
| if (!sanitizedFilename || sanitizedFilename === '.' || sanitizedFilename === '..') { | ||||||
| console.warn('[TASK_REVIEW] Skipping image with invalid filename:', image.filename); | ||||||
| continue; | ||||||
| } | ||||||
| // Remove data URL prefix if present (e.g., "data:image/png;base64," or "data:image/svg+xml;base64,") | ||||||
| const base64Data = image.data.replace(/^data:image\/[^;]+;base64,/, ''); | ||||||
| const imageBuffer = Buffer.from(base64Data, 'base64'); | ||||||
| const imagePath = path.join(imagesDir, sanitizedFilename); | ||||||
| // Verify the resolved path is within the images directory (defense in depth) | ||||||
| const resolvedPath = path.resolve(imagePath); | ||||||
| const resolvedImagesDir = path.resolve(imagesDir); | ||||||
| if (!resolvedPath.startsWith(resolvedImagesDir + path.sep)) { | ||||||
| console.warn('[TASK_REVIEW] Skipping image with path outside target directory:', image.filename); | ||||||
| continue; | ||||||
| } | ||||||
| writeFileSync(imagePath, imageBuffer); | ||||||
|
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 using async file operations to avoid blocking the event loop.
♻️ Suggested async approach+import { promises as fsPromises } from 'fs';
// In the image processing loop:
- writeFileSync(imagePath, imageBuffer);
+ await fsPromises.writeFile(imagePath, imageBuffer);Similarly, 📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||
| savedImages.push(`feedback_images/${sanitizedFilename}`); | ||||||
| console.log('[TASK_REVIEW] Saved image:', sanitizedFilename); | ||||||
| } catch (imgError) { | ||||||
| console.error('[TASK_REVIEW] Failed to save image:', image.filename, imgError); | ||||||
| } | ||||||
| } | ||||||
|
Comment on lines
+453
to
+456
This comment was marked as outdated.
Sorry, something went wrong. |
||||||
| if (savedImages.length > 0) { | ||||||
| imageReferences = '\n\n## Reference Images\n\n' + | ||||||
| savedImages.map(imgPath => ``).join('\n\n'); | ||||||
| } | ||||||
| } catch (dirError) { | ||||||
| console.error('[TASK_REVIEW] Failed to create images directory:', dirError); | ||||||
| } | ||||||
| } | ||||||
|
|
||||||
| try { | ||||||
| writeFileSync( | ||||||
| fixRequestPath, | ||||||
| `# QA Fix Request\n\nStatus: REJECTED\n\n## Feedback\n\n${feedback || 'No feedback provided'}\n\nCreated at: ${new Date().toISOString()}\n` | ||||||
| `# QA Fix Request\n\nStatus: REJECTED\n\n## Feedback\n\n${feedback || 'No feedback provided'}${imageReferences}\n\nCreated at: ${new Date().toISOString()}\n` | ||||||
| ); | ||||||
| } catch (error) { | ||||||
| console.error('[TASK_REVIEW] Failed to write QA fix request:', error); | ||||||
|
|
||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -118,13 +118,15 @@ function TaskDetailModalContent({ open, task, onOpenChange, onSwitchToTerminals, | |
| }; | ||
|
|
||
| const handleReject = async () => { | ||
| if (!state.feedback.trim()) { | ||
| // Allow submission if there's text feedback OR images attached | ||
| if (!state.feedback.trim() && state.feedbackImages.length === 0) { | ||
| return; | ||
| } | ||
| state.setIsSubmitting(true); | ||
| await submitReview(task.id, false, state.feedback); | ||
| await submitReview(task.id, false, state.feedback, state.feedbackImages); | ||
| state.setIsSubmitting(false); | ||
| state.setFeedback(''); | ||
| state.setFeedbackImages([]); | ||
|
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. Form state cleared regardless of submission successMedium Severity The |
||
| }; | ||
|
|
||
| const handleDelete = async () => { | ||
|
|
@@ -516,6 +518,8 @@ function TaskDetailModalContent({ open, task, onOpenChange, onSwitchToTerminals, | |
| showConflictDialog={state.showConflictDialog} | ||
| onFeedbackChange={state.setFeedback} | ||
| onReject={handleReject} | ||
| images={state.feedbackImages} | ||
| onImagesChange={state.setFeedbackImages} | ||
| onMerge={handleMerge} | ||
| onDiscard={handleDiscard} | ||
| onShowDiscardDialog={state.setShowDiscardDialog} | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,7 +1,7 @@ | ||
| import { useState, useRef, useEffect, useCallback } from 'react'; | ||
| import { useProjectStore } from '../../../stores/project-store'; | ||
| import { checkTaskRunning, isIncompleteHumanReview, getTaskProgress, useTaskStore, loadTasks } 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'; | ||
|
|
||
| /** | ||
| * Validates task subtasks structure to prevent infinite loops during resume. | ||
|
|
@@ -50,6 +50,7 @@ export interface UseTaskDetailOptions { | |
|
|
||
| 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); | ||
|
|
@@ -161,6 +162,11 @@ export function useTaskDetail({ task }: UseTaskDetailOptions) { | |
| } | ||
| }, [activeTab]); | ||
|
|
||
| // Reset feedback images when task changes to prevent image leakage between tasks | ||
| useEffect(() => { | ||
| setFeedbackImages([]); | ||
| }, [task.id]); | ||
cursor[bot] marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
| // Load worktree status when task is in human_review | ||
| useEffect(() => { | ||
| if (needsReview) { | ||
|
|
@@ -255,6 +261,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); | ||
|
|
||
|
|
@@ -404,6 +430,7 @@ export function useTaskDetail({ task }: UseTaskDetailOptions) { | |
| return { | ||
| // State | ||
| feedback, | ||
| feedbackImages, | ||
| isSubmitting, | ||
| activeTab, | ||
| isUserScrolledUp, | ||
|
|
@@ -447,6 +474,7 @@ export function useTaskDetail({ task }: UseTaskDetailOptions) { | |
|
|
||
| // Setters | ||
| setFeedback, | ||
| setFeedbackImages, | ||
| setIsSubmitting, | ||
| setActiveTab, | ||
| setIsUserScrolledUp, | ||
|
|
@@ -482,6 +510,10 @@ export function useTaskDetail({ task }: UseTaskDetailOptions) { | |
| handleLogsScroll, | ||
| togglePhase, | ||
| loadMergePreview, | ||
| addFeedbackImage, | ||
| addFeedbackImages, | ||
| removeFeedbackImage, | ||
| clearFeedbackImages, | ||
| handleReviewAgain, | ||
| reloadPlanForIncompleteTask, | ||
| }; | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.