Add file/screenshot upload to QA feedback interface #1018
Conversation
…seTaskDetail - Add feedbackImages state as ImageAttachment[] for storing feedback images - Add setFeedbackImages setter for direct state updates - Add addFeedbackImage handler for adding a single image - Add addFeedbackImages handler for adding multiple images at once - Add removeFeedbackImage handler for removing an image by ID - Add clearFeedbackImages handler for clearing all images - Import ImageAttachment type from shared/types 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
…submitReview - Add ImageAttachment import from ./task types - Update submitReview signature to include optional images parameter 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
… to accept and pass images
…l displa - Add paste event handler for screenshot/image clipboard support - Add drag-over and drag-leave handlers for visual feedback during drag - Add drop handler for image file drops - Add image thumbnail display (64x64) with remove button on hover - Import image utilities from ImageUpload.tsx (generateImageId, blobToBase64, etc.) - Add i18n support for all new UI text - Make new props optional for backward compatibility during incremental rollout - Allow submission with either text feedback or images (not both required) - Add visual drag feedback with border/background color change
…te and pass to TaskReview - Pass feedbackImages and setFeedbackImages from useTaskDetail hook to TaskReview - Update handleReject to include images in submitReview call - Allow submission with images only (no text required) - Clear images after successful submission
📝 WalkthroughWalkthroughAdds image-attachment support for QA feedback: UI paste/drag-and-drop, client-side validation and thumbnails, feedbackImages state in the task-detail hook, props wired through TaskDetailModal → TaskReview → QAFeedbackSection, and submitReview extended through preload, IPC, and main handlers to persist images with rejections. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant QAFeedbackSection
participant useTaskDetail
participant TaskDetailModal
participant TaskStore
participant MainProcess
User->>QAFeedbackSection: Paste or drop image(s)
QAFeedbackSection->>QAFeedbackSection: Validate, thumbnail, base64 encode
QAFeedbackSection->>useTaskDetail: onImagesChange(images)
useTaskDetail->>useTaskDetail: Update feedbackImages state
useTaskDetail-->>QAFeedbackSection: Render thumbnails
User->>TaskDetailModal: Submit review (approve/reject)
TaskDetailModal->>TaskStore: submitReview(taskId, approved, feedback, feedbackImages)
TaskStore->>MainProcess: IPC submitReview(..., images)
MainProcess-->>TaskStore: IPCResult
TaskStore->>useTaskDetail: Clear feedbackImages
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Suggested Labels
Suggested Reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Summary of ChangesHello @AndyMik90, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the human review process by introducing image and screenshot attachment capabilities to the QA feedback section. This allows users to provide visual evidence and context alongside their textual feedback, streamlining communication and improving the clarity of change requests for completed tasks. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request successfully adds file and screenshot upload functionality to the QA feedback section, reusing existing capabilities for a consistent user experience. The implementation is comprehensive, covering paste and drag-and-drop, image validation, and thumbnail generation. My review includes suggestions to improve maintainability by refactoring duplicated code, enhancing error handling and internationalization, and better encapsulating state management logic within the useTaskDetail hook.
| // 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([]); | ||
| }, []); |
There was a problem hiding this comment.
These new helper functions (addFeedbackImage, addFeedbackImages, removeFeedbackImage, clearFeedbackImages) for manipulating the feedbackImages state are a good addition. However, they are not currently used in this pull request. The QAFeedbackSection component receives the raw setFeedbackImages setter and implements its own logic for adding and removing images.
To improve encapsulation and make the useTaskDetail hook the single source of truth for this state logic, you should refactor the code to use these functions. Pass them as props to QAFeedbackSection instead of setFeedbackImages. This would make the code cleaner and avoid re-implementing state update logic in the component.
For example, QAFeedbackSection could receive onAddImages={addFeedbackImages} and onRemoveImage={removeFeedbackImage}.
| const handlePaste = useCallback(async (e: ClipboardEvent<HTMLTextAreaElement>) => { | ||
| // Skip image handling if feature is not enabled | ||
| if (!onImagesChange) return; | ||
|
|
||
| const clipboardItems = e.clipboardData?.items; | ||
| if (!clipboardItems) return; | ||
|
|
||
| // Find image items in clipboard | ||
| const imageItems: DataTransferItem[] = []; | ||
| for (let i = 0; i < clipboardItems.length; i++) { | ||
| const item = clipboardItems[i]; | ||
| if (item.type.startsWith('image/')) { | ||
| imageItems.push(item); | ||
| } | ||
| } | ||
|
|
||
| // If no images, allow normal paste behavior | ||
| if (imageItems.length === 0) return; | ||
|
|
||
| // Prevent default paste when we have images | ||
| e.preventDefault(); | ||
|
|
||
| // Check if we can add more images | ||
| const remainingSlots = MAX_IMAGES_PER_TASK - images.length; | ||
| if (remainingSlots <= 0) { | ||
| setError(t('feedback.maxImagesError', { count: MAX_IMAGES_PER_TASK })); | ||
| return; | ||
| } | ||
|
|
||
| setError(null); | ||
|
|
||
| // Process image items | ||
| const newImages: ImageAttachment[] = []; | ||
| const existingFilenames = images.map(img => img.filename); | ||
|
|
||
| for (const item of imageItems.slice(0, remainingSlots)) { | ||
| const file = item.getAsFile(); | ||
| if (!file) continue; | ||
|
|
||
| // Validate image type | ||
| if (!isValidImageMimeType(file.type)) { | ||
| setError(t('feedback.invalidTypeError', { types: ALLOWED_IMAGE_TYPES_DISPLAY })); | ||
| continue; | ||
| } | ||
|
|
||
| try { | ||
| const dataUrl = await blobToBase64(file); | ||
| const thumbnail = await createThumbnail(dataUrl); | ||
|
|
||
| // Generate filename for pasted images (screenshot-timestamp.ext) | ||
| const extension = file.type.split('/')[1] || 'png'; | ||
| const baseFilename = `screenshot-${Date.now()}.${extension}`; | ||
| const resolvedFilename = resolveFilename(baseFilename, [ | ||
| ...existingFilenames, | ||
| ...newImages.map(img => img.filename) | ||
| ]); | ||
|
|
||
| newImages.push({ | ||
| id: generateImageId(), | ||
| filename: resolvedFilename, | ||
| mimeType: file.type, | ||
| size: file.size, | ||
| data: dataUrl.split(',')[1], // Store base64 without data URL prefix | ||
| thumbnail | ||
| }); | ||
| } catch { | ||
| setError(t('feedback.processingError', 'Failed to process pasted image')); | ||
| } | ||
| } | ||
|
|
||
| if (newImages.length > 0) { | ||
| onImagesChange([...images, ...newImages]); | ||
| // Show success feedback | ||
| setPasteSuccess(true); | ||
| setTimeout(() => setPasteSuccess(false), 2000); | ||
| } | ||
| }, [images, onImagesChange, t]); | ||
|
|
||
| /** | ||
| * Handle drag over textarea for image drops | ||
| */ | ||
| const handleTextareaDragOver = useCallback((e: DragEvent<HTMLTextAreaElement>) => { | ||
| e.preventDefault(); | ||
| e.stopPropagation(); | ||
| setIsDragOverTextarea(true); | ||
| }, []); | ||
|
|
||
| /** | ||
| * Handle drag leave from textarea | ||
| */ | ||
| const handleTextareaDragLeave = useCallback((e: DragEvent<HTMLTextAreaElement>) => { | ||
| e.preventDefault(); | ||
| e.stopPropagation(); | ||
| setIsDragOverTextarea(false); | ||
| }, []); | ||
|
|
||
| /** | ||
| * Handle drop on textarea for images | ||
| */ | ||
| const handleTextareaDrop = useCallback( | ||
| async (e: DragEvent<HTMLTextAreaElement>) => { | ||
| e.preventDefault(); | ||
| e.stopPropagation(); | ||
| setIsDragOverTextarea(false); | ||
|
|
||
| // Skip image handling if feature is not enabled | ||
| if (!onImagesChange) return; | ||
| if (isSubmitting) return; | ||
|
|
||
| const files = e.dataTransfer?.files; | ||
| if (!files || files.length === 0) return; | ||
|
|
||
| // Filter for image files | ||
| const imageFiles: File[] = []; | ||
| for (let i = 0; i < files.length; i++) { | ||
| const file = files[i]; | ||
| if (file.type.startsWith('image/')) { | ||
| imageFiles.push(file); | ||
| } | ||
| } | ||
|
|
||
| if (imageFiles.length === 0) return; | ||
|
|
||
| // Check if we can add more images | ||
| const remainingSlots = MAX_IMAGES_PER_TASK - images.length; | ||
| if (remainingSlots <= 0) { | ||
| setError(t('feedback.maxImagesError', { count: MAX_IMAGES_PER_TASK })); | ||
| return; | ||
| } | ||
|
|
||
| setError(null); | ||
|
|
||
| // Process image files | ||
| const newImages: ImageAttachment[] = []; | ||
| const existingFilenames = images.map(img => img.filename); | ||
|
|
||
| for (const file of imageFiles.slice(0, remainingSlots)) { | ||
| // Validate image type | ||
| if (!isValidImageMimeType(file.type)) { | ||
| setError(t('feedback.invalidTypeError', { types: ALLOWED_IMAGE_TYPES_DISPLAY })); | ||
| continue; | ||
| } | ||
|
|
||
| try { | ||
| const dataUrl = await blobToBase64(file); | ||
| const thumbnail = await createThumbnail(dataUrl); | ||
|
|
||
| // Use original filename or generate one | ||
| const baseFilename = file.name || `dropped-image-${Date.now()}.${file.type.split('/')[1] || 'png'}`; | ||
| const resolvedFilename = resolveFilename(baseFilename, [ | ||
| ...existingFilenames, | ||
| ...newImages.map(img => img.filename) | ||
| ]); | ||
|
|
||
| newImages.push({ | ||
| id: generateImageId(), | ||
| filename: resolvedFilename, | ||
| mimeType: file.type, | ||
| size: file.size, | ||
| data: dataUrl.split(',')[1], // Store base64 without data URL prefix | ||
| thumbnail | ||
| }); | ||
| } catch { | ||
| setError(t('feedback.processingError', 'Failed to process dropped image')); | ||
| } | ||
| } | ||
|
|
||
| if (newImages.length > 0) { | ||
| onImagesChange([...images, ...newImages]); | ||
| // Show success feedback | ||
| setPasteSuccess(true); | ||
| setTimeout(() => setPasteSuccess(false), 2000); | ||
| } | ||
| }, | ||
| [images, isSubmitting, onImagesChange, t] | ||
| ); |
There was a problem hiding this comment.
There is significant code duplication between the handlePaste and handleTextareaDrop functions. Both perform very similar logic for processing image files, including checking capacity, validating files, converting to base64, creating thumbnails, and updating state.
To improve maintainability and adhere to the DRY (Don't Repeat Yourself) principle, I recommend refactoring this logic into a single, shared helper function. This function could accept an array of File objects and handle the entire processing pipeline.
handlePaste and handleTextareaDrop would then become simpler, responsible only for extracting the files from the event and passing them to this new shared function.
| } catch { | ||
| setError(t('feedback.processingError', 'Failed to process pasted image')); | ||
| } |
There was a problem hiding this comment.
This error handling can be improved in two ways:
-
Missing Translation Key: The translation key
'feedback.processingError'is used here and inhandleTextareaDrop, but it hasn't been added to the i18n JSON files (en/tasks.jsonandfr/tasks.json). Please add this key to provide proper internationalization for the error message. -
Error Logging: The actual error that is caught is suppressed. For better debugging, it's good practice to log the caught error to the console.
I suggest updating this catch block (and the one in handleTextareaDrop) to address these points.
} catch (err) {
console.error('Failed to process pasted image:', err);
setError(t('feedback.processingError', 'Failed to process pasted image'));
}
![sentry[bot]](https://avatars.githubusercontent.com/u/1396951?s=60&v=4){kind=small}
|
|
||
| try { | ||
| const result = await window.electronAPI.submitReview(taskId, approved, feedback); | ||
| const result = await window.electronAPI.submitReview(taskId, approved, feedback, images); |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In @apps/frontend/src/renderer/components/task-detail/hooks/useTaskDetail.ts:
- Line 12: The feedbackImages state in useTaskDetail is never cleared when the
task prop changes, causing stale images to persist; add a useEffect inside the
useTaskDetail hook that watches the task identifier (e.g., task?.id or taskId)
and calls setFeedbackImages([]) whenever it changes so the modal resets images
when a new task is loaded. Ensure the effect runs only on task id changes (not
on every render) and reference feedbackImages and setFeedbackImages within the
useTaskDetail hook to implement the reset.
In
@apps/frontend/src/renderer/components/task-detail/task-review/QAFeedbackSection.tsx:
- Around line 59-135: handlePaste and handleTextareaDrop duplicate image
validation/processing; extract a shared async helper (e.g., processImageFiles)
used by both to validate mime types (isValidImageMimeType /
ALLOWED_IMAGE_TYPES_DISPLAY), convert blobs to base64 (blobToBase64), create
thumbnails (createThumbnail), resolve filenames (resolveFilename) and build
ImageAttachment objects with generateImageId; have the helper accept File[] and
a source flag ('paste'|'drop'), respect remainingSlots (MAX_IMAGES_PER_TASK) and
existing filenames (images.map), set errors via setError on per-file failure,
return ImageAttachment[] and then update handlers to call
onImagesChange([...images, ...newImages]), handle paste success UI
(setPasteSuccess) in handlePaste, and keep hooks/deps aligned (useCallback deps:
images, t, etc.).
- Around line 124-126: The catch block in QAFeedbackSection.tsx swallows
exceptions; change it to catch the error (e.g., catch (err)) and record it
before calling setError: log the error with console.error or your app logger (or
call Sentry.captureException(err) if available) and include a short contextual
message that mentions the operation (processing pasted image) so you can trace
failures tied to the setError call in setError(t('feedback.processingError',
'Failed to process pasted image')).
- Around line 221-223: The catch block in QAFeedbackSection silently swallows
errors — update the catch to accept the error (e.g., catch (err)) and handle it:
log the error (console.error or the app logger) and include useful error info
when setting state or reporting (call setError with the existing
t('feedback.processingError', ...) message but also log err/message and
optionally send to monitoring), referencing the catch block around the image
processing logic and the setError call so you add error handling without
changing the user-facing message.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (8)
apps/frontend/src/renderer/components/task-detail/TaskDetailModal.tsxapps/frontend/src/renderer/components/task-detail/TaskReview.tsxapps/frontend/src/renderer/components/task-detail/hooks/useTaskDetail.tsapps/frontend/src/renderer/components/task-detail/task-review/QAFeedbackSection.tsxapps/frontend/src/renderer/stores/task-store.tsapps/frontend/src/shared/i18n/locales/en/tasks.jsonapps/frontend/src/shared/i18n/locales/fr/tasks.jsonapps/frontend/src/shared/types/ipc.ts
🧰 Additional context used
📓 Path-based instructions (3)
apps/frontend/src/**/*.{tsx,ts}
📄 CodeRabbit inference engine (CLAUDE.md)
apps/frontend/src/**/*.{tsx,ts}: Use i18n translation keys for all user-facing text in the frontend. All labels, buttons, messages must use translation keys from react-i18next with namespace:section.key format (e.g., 'navigation:items.githubPRs').
Never use hardcoded strings for UI text in JSX/TSX files. Always use translation keys via useTranslation() hook.
Files:
apps/frontend/src/renderer/components/task-detail/TaskReview.tsxapps/frontend/src/renderer/stores/task-store.tsapps/frontend/src/renderer/components/task-detail/TaskDetailModal.tsxapps/frontend/src/renderer/components/task-detail/hooks/useTaskDetail.tsapps/frontend/src/renderer/components/task-detail/task-review/QAFeedbackSection.tsxapps/frontend/src/shared/types/ipc.ts
apps/frontend/**/*.{ts,tsx}
⚙️ CodeRabbit configuration file
apps/frontend/**/*.{ts,tsx}: Review React patterns and TypeScript type safety.
Check for proper state management and component composition.
Files:
apps/frontend/src/renderer/components/task-detail/TaskReview.tsxapps/frontend/src/renderer/stores/task-store.tsapps/frontend/src/renderer/components/task-detail/TaskDetailModal.tsxapps/frontend/src/renderer/components/task-detail/hooks/useTaskDetail.tsapps/frontend/src/renderer/components/task-detail/task-review/QAFeedbackSection.tsxapps/frontend/src/shared/types/ipc.ts
apps/frontend/src/shared/i18n/locales/**/*.json
📄 CodeRabbit inference engine (CLAUDE.md)
Add new translation keys to ALL language files (at minimum: en/.json and fr/.json) in apps/frontend/src/shared/i18n/locales/ when adding new UI text.
Files:
apps/frontend/src/shared/i18n/locales/en/tasks.jsonapps/frontend/src/shared/i18n/locales/fr/tasks.json
🧠 Learnings (5)
📓 Common learnings
Learnt from: MikeeBuilds
Repo: AndyMik90/Auto-Claude PR: 661
File: apps/frontend/src/renderer/components/onboarding/OllamaModelSelector.tsx:176-189
Timestamp: 2026-01-04T23:59:48.743Z
Learning: In the AndyMik90/Auto-Claude repository, pre-existing i18n issues (hardcoded user-facing strings that should be localized) can be deferred to future i18n cleanup passes rather than requiring immediate fixes in PRs that don't introduce new i18n violations.
📚 Learning: 2026-01-04T23:59:45.209Z
Learnt from: MikeeBuilds
Repo: AndyMik90/Auto-Claude PR: 661
File: apps/frontend/src/renderer/components/onboarding/OllamaModelSelector.tsx:176-189
Timestamp: 2026-01-04T23:59:45.209Z
Learning: In the AndyMik90/Auto-Claude repository, pre-existing i18n issues (hardcoded user-facing strings) can be deferred for future i18n cleanup passes. Do not fix such issues in PRs that do not introduce new i18n violations, especially in frontend TSX components (e.g., apps/frontend/**/*.tsx). If a PR adds new i18n violations, address them in that PR.
Applied to files:
apps/frontend/src/renderer/components/task-detail/TaskReview.tsxapps/frontend/src/renderer/components/task-detail/TaskDetailModal.tsxapps/frontend/src/renderer/components/task-detail/task-review/QAFeedbackSection.tsx
📚 Learning: 2026-01-12T13:47:32.539Z
Learnt from: CR
Repo: AndyMik90/Auto-Claude PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-12T13:47:32.539Z
Learning: Applies to apps/frontend/src/shared/i18n/locales/**/*.json : Add new translation keys to ALL language files (at minimum: en/*.json and fr/*.json) in apps/frontend/src/shared/i18n/locales/ when adding new UI text.
Applied to files:
apps/frontend/src/shared/i18n/locales/en/tasks.jsonapps/frontend/src/shared/i18n/locales/fr/tasks.json
📚 Learning: 2026-01-12T13:47:32.539Z
Learnt from: CR
Repo: AndyMik90/Auto-Claude PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-12T13:47:32.539Z
Learning: Applies to apps/frontend/src/**/*.{tsx,ts} : Use i18n translation keys for all user-facing text in the frontend. All labels, buttons, messages must use translation keys from react-i18next with namespace:section.key format (e.g., 'navigation:items.githubPRs').
Applied to files:
apps/frontend/src/shared/i18n/locales/en/tasks.jsonapps/frontend/src/shared/i18n/locales/fr/tasks.json
📚 Learning: 2026-01-10T15:30:12.808Z
Learnt from: taniar88
Repo: AndyMik90/Auto-Claude PR: 883
File: apps/frontend/src/main/agent/agent-process.ts:21-21
Timestamp: 2026-01-10T15:30:12.808Z
Learning: Before adding a new language to AVAILABLE_LANGUAGES, ensure UI translations exist and are complete in apps/frontend/src/shared/i18n/locales. Do not advertise official support for a language until its locale files are fully translated and validated, to avoid user-facing issues.
Applied to files:
apps/frontend/src/shared/i18n/locales/en/tasks.jsonapps/frontend/src/shared/i18n/locales/fr/tasks.json
🧬 Code graph analysis (6)
apps/frontend/src/renderer/components/task-detail/TaskReview.tsx (1)
apps/frontend/src/shared/types/task.ts (1)
ImageAttachment(113-121)
apps/frontend/src/renderer/stores/task-store.ts (1)
apps/frontend/src/shared/types/task.ts (1)
ImageAttachment(113-121)
apps/frontend/src/renderer/components/task-detail/TaskDetailModal.tsx (1)
apps/frontend/src/renderer/stores/task-store.ts (1)
submitReview(308-326)
apps/frontend/src/renderer/components/task-detail/hooks/useTaskDetail.ts (1)
apps/frontend/src/shared/types/task.ts (2)
Task(238-258)ImageAttachment(113-121)
apps/frontend/src/renderer/components/task-detail/task-review/QAFeedbackSection.tsx (4)
apps/frontend/src/shared/types/task.ts (1)
ImageAttachment(113-121)apps/frontend/src/shared/constants/task.ts (2)
MAX_IMAGES_PER_TASK(188-188)ALLOWED_IMAGE_TYPES_DISPLAY(207-207).design-system/src/lib/icons.ts (2)
X(37-37)RotateCcw(31-31).design-system/src/components/Button.tsx (1)
Button(10-44)
apps/frontend/src/shared/types/ipc.ts (1)
apps/frontend/src/shared/types/task.ts (1)
ImageAttachment(113-121)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Seer Code Review
- GitHub Check: Cursor Bugbot
🔇 Additional comments (15)
apps/frontend/src/shared/i18n/locales/en/tasks.json (1)
129-135: LGTM!The new
feedbacktranslation keys are well-structured with proper interpolation placeholders ({{count}},{{types}}). The keys appropriately cover all user-facing text for the image attachment feature.apps/frontend/src/shared/i18n/locales/fr/tasks.json (1)
129-135: LGTM!French translations are well-written and match the English keys. The interpolation placeholders are correctly preserved. As per coding guidelines, both English and French locale files now contain the required
feedbacktranslation keys.apps/frontend/src/shared/types/ipc.ts (2)
43-44: LGTM!The
ImageAttachmenttype import is correctly added to support the new image parameter in the API.
160-160: LGTM!The
submitReviewsignature is extended with an optionalimagesparameter, maintaining backward compatibility. The parameter ordering is logical.apps/frontend/src/renderer/stores/task-store.ts (2)
2-2: LGTM!The
ImageAttachmenttype import is correctly added to support the updatedsubmitReviewfunction.
308-326: LGTM!The
submitReviewfunction correctly:
- Adds an optional
imagesparameter maintaining backward compatibility- Forwards images to the IPC layer
- Preserves existing error handling and state update logic
apps/frontend/src/renderer/components/task-detail/hooks/useTaskDetail.ts (3)
4-4: LGTM!The
ImageAttachmenttype import is correctly added to support the new feedback images state.
215-233: LGTM!The feedback image management callbacks are well-implemented:
- All use
useCallbackwith correct empty dependency arrays (since they only use functional state updates)addFeedbackImageandaddFeedbackImagescorrectly append to existing stateremoveFeedbackImagecorrectly filters by IDclearFeedbackImagesproperly resets to empty array
269-269: LGTM!The new feedback image state and handlers are properly exposed in the hook's return object, following the existing patterns.
Also applies to: 310-310, 344-347
apps/frontend/src/renderer/components/task-detail/TaskReview.tsx (1)
35-38: LGTM! Clean prop threading for image attachment support.The optional props maintain backward compatibility while enabling the image attachment feature when provided. The prop flow from interface → destructuring → child component is correctly implemented.
Also applies to: 81-82, 146-147
apps/frontend/src/renderer/components/task-detail/task-review/QAFeedbackSection.tsx (3)
1-29: LGTM! Proper imports and well-typed interface extension.Optional props with JSDoc comments maintain backward compatibility and provide clear documentation.
246-358: LGTM! Well-implemented UI with proper i18n and accessibility.Key positives:
canSubmitcorrectly allows submission with either text or images- All new user-facing text uses translation keys (lines 252, 255, 261, 280, 288, 330, 349, 354)
- Accessible remove button with
aria-label- Visual feedback for drag state, paste success, and errors
- Feature gracefully degraded when
onImagesChangenot provided
49-49: Unused ref declaration.
textareaRefis declared but never used in the component.Proposed fix
Remove the unused ref if not needed:
- // Ref for the textarea - const textareaRef = useRef<HTMLTextAreaElement>(null);Or if intended for future use (e.g., programmatic focus), ensure it's connected:
<Textarea - ref={textareaRef} + ref={textareaRef} // Already present at line 260The ref is passed to Textarea at line 260, so this may just need the comment removed or clarified for its purpose.
Likely an incorrect or invalid review comment.
apps/frontend/src/renderer/components/task-detail/TaskDetailModal.tsx (2)
106-116: LGTM! Proper integration of image submission flow.The
handleRejectfunction correctly:
- Guards for either text feedback or images (line 108)
- Passes
feedbackImagestosubmitReview(line 112)- Clears images after successful submission (line 115)
This aligns with the
submitReviewsignature intask-store.ts.
437-438: LGTM! Props correctly wired to TaskReview.The image state and setter are properly passed to enable the feature in the review flow.
|
|
||
| export function useTaskDetail({ task }: UseTaskDetailOptions) { | ||
| const [feedback, setFeedback] = useState(''); | ||
| const [feedbackImages, setFeedbackImages] = useState<ImageAttachment[]>([]); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider resetting feedbackImages when the task changes.
The feedbackImages state is not reset when the task prop changes. If the modal is reused across different tasks without unmounting, stale images from a previous review could persist.
♻️ Suggested defensive reset
Add an effect to clear images when the task ID changes:
+ // Reset feedback images when task changes
+ useEffect(() => {
+ setFeedbackImages([]);
+ }, [task.id]);🤖 Prompt for AI Agents
In @apps/frontend/src/renderer/components/task-detail/hooks/useTaskDetail.ts at
line 12, The feedbackImages state in useTaskDetail is never cleared when the
task prop changes, causing stale images to persist; add a useEffect inside the
useTaskDetail hook that watches the task identifier (e.g., task?.id or taskId)
and calls setFeedbackImages([]) whenever it changes so the modal resets images
when a new task is loaded. Ensure the effect runs only on task id changes (not
on every render) and reference feedbackImages and setFeedbackImages within the
useTaskDetail hook to implement the reset.
| } catch { | ||
| setError(t('feedback.processingError', 'Failed to process pasted image')); | ||
| } |
There was a problem hiding this comment.
Silent error swallowing loses debugging context.
The empty catch {} block sets a user-facing error but doesn't log the actual exception, making debugging difficult in production.
Proposed fix
- } catch {
- setError(t('feedback.processingError', 'Failed to process pasted image'));
+ } catch (err) {
+ console.error('Failed to process pasted image:', err);
+ setError(t('feedback.processingError', 'Failed to process pasted image'));
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| } catch { | |
| setError(t('feedback.processingError', 'Failed to process pasted image')); | |
| } | |
| } catch (err) { | |
| console.error('Failed to process pasted image:', err); | |
| setError(t('feedback.processingError', 'Failed to process pasted image')); | |
| } |
🤖 Prompt for AI Agents
In
@apps/frontend/src/renderer/components/task-detail/task-review/QAFeedbackSection.tsx
around lines 124 - 126, The catch block in QAFeedbackSection.tsx swallows
exceptions; change it to catch the error (e.g., catch (err)) and record it
before calling setError: log the error with console.error or your app logger (or
call Sentry.captureException(err) if available) and include a short contextual
message that mentions the operation (processing pasted image) so you can trace
failures tied to the setError call in setError(t('feedback.processingError',
'Failed to process pasted image')).
| } catch { | ||
| setError(t('feedback.processingError', 'Failed to process dropped image')); | ||
| } |
There was a problem hiding this comment.
Same silent error swallowing as paste handler.
Proposed fix
- } catch {
- setError(t('feedback.processingError', 'Failed to process dropped image'));
+ } catch (err) {
+ console.error('Failed to process dropped image:', err);
+ setError(t('feedback.processingError', 'Failed to process dropped image'));
}🤖 Prompt for AI Agents
In
@apps/frontend/src/renderer/components/task-detail/task-review/QAFeedbackSection.tsx
around lines 221 - 223, The catch block in QAFeedbackSection silently swallows
errors — update the catch to accept the error (e.g., catch (err)) and handle it:
log the error (console.error or the app logger) and include useful error info
when setting state or reporting (call setError with the existing
t('feedback.processingError', ...) message but also log err/message and
optionally send to monitoring), referencing the catch block around the image
processing logic and the setError call so you add error handling without
changing the user-facing message.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
apps/frontend/src/renderer/components/task-detail/task-review/QAFeedbackSection.tsx
Show resolved
Hide resolved
AndyMik90
left a comment
AndyMik90
left a comment
There was a problem hiding this comment.
🤖 Auto Claude PR Review
Merge Verdict: 🔴 BLOCKED
🔴 Blocked - Merge conflicts must be resolved before merge.
Blocked: PR has merge conflicts with base branch. Resolve conflicts before merge.
Risk Assessment
| Factor | Level | Notes |
|---|---|---|
| Complexity | Medium | Based on lines changed |
| Security Impact | None | Based on security findings |
| Scope Coherence | Good | Based on structural review |
🚨 Blocking Issues (Must Fix)
- Merge Conflicts: PR has conflicts with base branch that must be resolved
- Critical: Images parameter not forwarded through preload API - data loss bug (apps/frontend/src/preload/api/task-api.ts:112)
- Critical: Main process IPC handler does not receive or process images (apps/frontend/src/main/ipc-handlers/task/execution-handlers.ts:315)
Findings Summary
- Critical: 2 issue(s)
- Medium: 4 issue(s)
- Low: 1 issue(s)
Generated by Auto Claude PR Review
Findings (7 selected of 7 total)
🔴 [bf4ebc138af1] [CRITICAL] Images parameter not forwarded through preload API - data loss bug
📁 apps/frontend/src/preload/api/task-api.ts:112
The PR updates task-store.ts to pass images as a 4th parameter and updates ipc.ts types to accept images, but the preload API in task-api.ts is NOT updated. The existing implementation only passes 3 parameters (taskId, approved, feedback) to ipcRenderer.invoke(). This means any images attached by users will be silently dropped and never reach the backend. The preload/api/task-api.ts file is NOT included in the PR's changed files list.
Suggested fix:
Add task-api.ts to the PR. Update submitReview to accept and forward images: `submitReview: (taskId: string, approved: boolean, feedback?: string, images?: ImageAttachment[]) => ipcRenderer.invoke(IPC_CHANNELS.TASK_REVIEW, taskId, approved, feedback, images)`
🔴 [7d42336bef41] [CRITICAL] Main process IPC handler does not receive or process images
📁 apps/frontend/src/main/ipc-handlers/task/execution-handlers.ts:315
Even if the preload API is fixed, the main process TASK_REVIEW handler also needs to be updated to receive the images parameter and implement image storage logic. Currently it only destructures 3 parameters and writes text feedback to QA_FIX_REQUEST.md with no image handling.
Suggested fix:
Add images parameter to handler signature and implement image storage: save images to disk in the spec directory and reference them in QA_FIX_REQUEST.md
🟡 [56196cab4d47] [MEDIUM] Missing translation key 'feedback.processingError'
📁 apps/frontend/src/shared/i18n/locales/en/tasks.json:131
The QAFeedbackSection.tsx uses translation key 'feedback.processingError' in catch blocks (e.g., line ~97: setError(t('feedback.processingError', 'Failed to process pasted image'))), but this key is not present in the translation files. The en/tasks.json diff shows keys for dragDropHint, imageAdded, maxImagesError, invalidTypeError, and removeImage - but not processingError. This will cause the fallback text to display but may cause issues in stricter i18n setups.
Suggested fix:
Add to en/tasks.json: `"processingError": "Failed to process image"` and to fr/tasks.json: `"processingError": "Échec du traitement de l'image"`
🟡 [ee6c23f67c69] [MEDIUM] feedbackImages state not reset when task changes
📁 apps/frontend/src/renderer/components/task-detail/hooks/useTaskDetail.ts:214
The useTaskDetail hook adds feedbackImages state but does not reset it when the task prop changes. If the modal is reused across different tasks (task.id changes), images attached to one task's feedback could persist and accidentally be submitted with a different task's feedback.
Suggested fix:
Add a useEffect to reset feedbackImages when task.id changes: `useEffect(() => { setFeedbackImages([]); }, [task.id]);` or include it in an existing task-change effect.
🟡 [c9df10e5b0f2] [MEDIUM] Silent error swallowing in paste handler loses debugging context
📁 apps/frontend/src/renderer/components/task-detail/task-review/QAFeedbackSection.tsx:97
The catch block in handlePaste only sets user-facing error state but doesn't log the actual exception: catch { setError(t('feedback.processingError', ...)) }. This makes debugging image processing failures very difficult as the actual error (e.g., CORS issues, canvas errors) is lost.
Suggested fix:
Log the error before setting user-facing message: `catch (error) { console.error('Failed to process pasted image:', error); setError(t('feedback.processingError', ...)); }`
🟡 [705519601427] [MEDIUM] Silent error swallowing in drop handler loses debugging context
📁 apps/frontend/src/renderer/components/task-detail/task-review/QAFeedbackSection.tsx:167
Same issue as paste handler - the catch block in handleTextareaDrop doesn't log the actual exception, only sets user-facing error. This is a duplicate of the pattern in handlePaste.
Suggested fix:
Log the error before setting user-facing message: `catch (error) { console.error('Failed to process dropped image:', error); setError(t('feedback.processingError', ...)); }`
🔵 [1ddc61c7e321] [LOW] Code duplication between handlePaste and handleTextareaDrop handlers
📁 apps/frontend/src/renderer/components/task-detail/task-review/QAFeedbackSection.tsx:47
Both handlers contain nearly identical logic for: validating file types, checking remaining slots against MAX_IMAGES_PER_TASK, processing files with blobToBase64/createThumbnail, generating unique filenames with resolveFilename, and creating ImageAttachment objects. This ~60 lines of duplicated logic violates DRY principles.
Suggested fix:
Extract shared logic to a helper function: `async function processImageFiles(files: File[], existingImages: ImageAttachment[], onImagesChange: Function, t: TFunction): Promise<void>` that both handlers can call.
This review was generated by Auto Claude.
Fixes: - CRITICAL: Forward images parameter through preload API (task-api.ts) - CRITICAL: Process images in IPC handler and save to spec dir (execution-handlers.ts) - MEDIUM: Add processingError translation key (en/fr tasks.json) - MEDIUM: Reset feedbackImages state when task changes (useTaskDetail.ts) - MEDIUM: Add console.error logging in paste/drop error handlers (QAFeedbackSection.tsx) Co-Authored-By: Claude Opus 4.5 <[email protected]>
|
Test User seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account. You have signed the CLA already but the status is still pending? Let us recheck it. |
github-actions
bot
added
area/frontend
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In @apps/frontend/src/main/ipc-handlers/task/execution-handlers.ts:
- Around line 411-444: Sanitize and constrain image filenames before writing to
disk to prevent path traversal: replace usages of image.filename when building
imagePath by first validating and normalizing the name (e.g., use
path.basename(image.filename) and reject if it differs from the original or
contains '..' or path.isAbsolute returns true), strip any path separators and
non-whitelisted characters (allow only alphanumerics, dashes, underscores and a
single dot for extension), and generate a safe unique filename (prefix with a
timestamp or UUID) before calling writeFileSync; update savedImages to use the
sanitized/unique filename and add error handling for rejected filenames (skip
and log a warning) around the existing imagesDir/writeFileSync logic.
- Line 430: Replace the blocking calls writeFileSync and mkdirSync inside the
async IPC handler with their async fs.promises equivalents: use await
fs.promises.mkdir(imagesDir, { recursive: true }) (replacing mkdirSync) and
await fs.promises.writeFile(imagePath, imageBuffer) (replacing writeFileSync);
ensure the handler function is async (it already is) and import/alias
fs.promises (or fsPromises) at the top so you await these operations and
handle/rethrow errors as appropriate around the imagePath/imageBuffer/imagesDir
logic.
In
@apps/frontend/src/renderer/components/task-detail/task-review/QAFeedbackSection.tsx:
- Around line 104-128: The extension extraction in QAFeedbackSection uses
file.type.split('/')[1] which yields compound strings like "svg+xml"; change the
logic that derives extension (used when building baseFilename and in the
dropped-file handler around line 208) to strip any suffix after a '+' (e.g.,
take file.type.split('/')[1].split('+')[0] or equivalent) and fall back to 'png'
if empty, then use that cleaned extension when calling resolveFilename and when
constructing the newImages entries (same for the dropped files code path).
- Around line 130-136: In QAFeedbackSection, the inline setTimeout used after
setPasteSuccess can update state after unmount; store the timeout ID (e.g., in a
ref like pasteTimeoutRef) when calling setTimeout in the paste handling logic
and clear it on unmount or when a new timeout is scheduled (useEffect cleanup or
a cleanup function) so that setPasteSuccess(false) is not called on an unmounted
component; ensure you clear the timeout using
clearTimeout(pasteTimeoutRef.current) and reset the ref when appropriate.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (10)
apps/frontend/src/main/ipc-handlers/task/execution-handlers.tsapps/frontend/src/preload/api/task-api.tsapps/frontend/src/renderer/components/task-detail/TaskDetailModal.tsxapps/frontend/src/renderer/components/task-detail/TaskReview.tsxapps/frontend/src/renderer/components/task-detail/hooks/useTaskDetail.tsapps/frontend/src/renderer/components/task-detail/task-review/QAFeedbackSection.tsxapps/frontend/src/renderer/stores/task-store.tsapps/frontend/src/shared/i18n/locales/en/tasks.jsonapps/frontend/src/shared/i18n/locales/fr/tasks.jsonapps/frontend/src/shared/types/ipc.ts
🧰 Additional context used
📓 Path-based instructions (3)
apps/frontend/src/shared/i18n/locales/**/*.json
📄 CodeRabbit inference engine (CLAUDE.md)
Add new translation keys to ALL language files (at minimum: en/.json and fr/.json) in apps/frontend/src/shared/i18n/locales/ when adding new UI text.
Files:
apps/frontend/src/shared/i18n/locales/en/tasks.jsonapps/frontend/src/shared/i18n/locales/fr/tasks.json
apps/frontend/src/**/*.{tsx,ts}
📄 CodeRabbit inference engine (CLAUDE.md)
apps/frontend/src/**/*.{tsx,ts}: Use i18n translation keys for all user-facing text in the frontend. All labels, buttons, messages must use translation keys from react-i18next with namespace:section.key format (e.g., 'navigation:items.githubPRs').
Never use hardcoded strings for UI text in JSX/TSX files. Always use translation keys via useTranslation() hook.
Files:
apps/frontend/src/renderer/components/task-detail/TaskReview.tsxapps/frontend/src/renderer/components/task-detail/TaskDetailModal.tsxapps/frontend/src/renderer/components/task-detail/hooks/useTaskDetail.tsapps/frontend/src/renderer/stores/task-store.tsapps/frontend/src/preload/api/task-api.tsapps/frontend/src/shared/types/ipc.tsapps/frontend/src/main/ipc-handlers/task/execution-handlers.tsapps/frontend/src/renderer/components/task-detail/task-review/QAFeedbackSection.tsx
apps/frontend/**/*.{ts,tsx}
⚙️ CodeRabbit configuration file
apps/frontend/**/*.{ts,tsx}: Review React patterns and TypeScript type safety.
Check for proper state management and component composition.
Files:
apps/frontend/src/renderer/components/task-detail/TaskReview.tsxapps/frontend/src/renderer/components/task-detail/TaskDetailModal.tsxapps/frontend/src/renderer/components/task-detail/hooks/useTaskDetail.tsapps/frontend/src/renderer/stores/task-store.tsapps/frontend/src/preload/api/task-api.tsapps/frontend/src/shared/types/ipc.tsapps/frontend/src/main/ipc-handlers/task/execution-handlers.tsapps/frontend/src/renderer/components/task-detail/task-review/QAFeedbackSection.tsx
🧠 Learnings (4)
📓 Common learnings
Learnt from: CR
Repo: AndyMik90/Auto-Claude PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-12T13:47:32.539Z
Learning: When submitting PRs to AndyMik90/Auto-Claude upstream, always target the develop branch, NOT main. Commits must include sign-off (git commit -s).
Learnt from: MikeeBuilds
Repo: AndyMik90/Auto-Claude PR: 661
File: apps/frontend/src/renderer/components/onboarding/OllamaModelSelector.tsx:176-189
Timestamp: 2026-01-04T23:59:48.743Z
Learning: In the AndyMik90/Auto-Claude repository, pre-existing i18n issues (hardcoded user-facing strings that should be localized) can be deferred to future i18n cleanup passes rather than requiring immediate fixes in PRs that don't introduce new i18n violations.
📚 Learning: 2026-01-12T13:47:32.539Z
Learnt from: CR
Repo: AndyMik90/Auto-Claude PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-12T13:47:32.539Z
Learning: Applies to apps/frontend/src/shared/i18n/locales/**/*.json : Add new translation keys to ALL language files (at minimum: en/*.json and fr/*.json) in apps/frontend/src/shared/i18n/locales/ when adding new UI text.
Applied to files:
apps/frontend/src/shared/i18n/locales/en/tasks.jsonapps/frontend/src/shared/i18n/locales/fr/tasks.json
📚 Learning: 2026-01-10T15:30:12.808Z
Learnt from: taniar88
Repo: AndyMik90/Auto-Claude PR: 883
File: apps/frontend/src/main/agent/agent-process.ts:21-21
Timestamp: 2026-01-10T15:30:12.808Z
Learning: Before adding a new language to AVAILABLE_LANGUAGES, ensure UI translations exist and are complete in apps/frontend/src/shared/i18n/locales. Do not advertise official support for a language until its locale files are fully translated and validated, to avoid user-facing issues.
Applied to files:
apps/frontend/src/shared/i18n/locales/en/tasks.jsonapps/frontend/src/shared/i18n/locales/fr/tasks.json
📚 Learning: 2026-01-04T23:59:45.209Z
Learnt from: MikeeBuilds
Repo: AndyMik90/Auto-Claude PR: 661
File: apps/frontend/src/renderer/components/onboarding/OllamaModelSelector.tsx:176-189
Timestamp: 2026-01-04T23:59:45.209Z
Learning: In the AndyMik90/Auto-Claude repository, pre-existing i18n issues (hardcoded user-facing strings) can be deferred for future i18n cleanup passes. Do not fix such issues in PRs that do not introduce new i18n violations, especially in frontend TSX components (e.g., apps/frontend/**/*.tsx). If a PR adds new i18n violations, address them in that PR.
Applied to files:
apps/frontend/src/renderer/components/task-detail/TaskReview.tsxapps/frontend/src/renderer/components/task-detail/TaskDetailModal.tsxapps/frontend/src/renderer/components/task-detail/task-review/QAFeedbackSection.tsx
🧬 Code graph analysis (7)
apps/frontend/src/renderer/components/task-detail/TaskReview.tsx (1)
apps/frontend/src/shared/types/task.ts (1)
ImageAttachment(117-125)
apps/frontend/src/renderer/components/task-detail/hooks/useTaskDetail.ts (1)
apps/frontend/src/shared/types/task.ts (1)
ImageAttachment(117-125)
apps/frontend/src/renderer/stores/task-store.ts (1)
apps/frontend/src/shared/types/task.ts (1)
ImageAttachment(117-125)
apps/frontend/src/preload/api/task-api.ts (2)
apps/frontend/src/shared/types/task.ts (1)
ImageAttachment(117-125)apps/frontend/src/__mocks__/electron.ts (1)
ipcRenderer(53-61)
apps/frontend/src/shared/types/ipc.ts (1)
apps/frontend/src/shared/types/task.ts (1)
ImageAttachment(117-125)
apps/frontend/src/main/ipc-handlers/task/execution-handlers.ts (1)
apps/frontend/src/shared/types/task.ts (1)
ImageAttachment(117-125)
apps/frontend/src/renderer/components/task-detail/task-review/QAFeedbackSection.tsx (2)
apps/frontend/src/shared/types/task.ts (1)
ImageAttachment(117-125)apps/frontend/src/shared/constants/task.ts (2)
MAX_IMAGES_PER_TASK(197-197)ALLOWED_IMAGE_TYPES_DISPLAY(216-216)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Seer Code Review
- GitHub Check: Cursor Bugbot
- GitHub Check: CodeQL (javascript-typescript)
- GitHub Check: CodeQL (python)
🔇 Additional comments (29)
apps/frontend/src/shared/i18n/locales/en/tasks.json (1)
161-168: LGTM! Well-structured i18n keys for the new feedback image feature.The new
feedbacksection follows the existing naming conventions and properly uses interpolation variables ({{count}},{{types}}). The keys cover the essential user-facing messages for the image upload functionality.apps/frontend/src/shared/i18n/locales/fr/tasks.json (1)
161-168: LGTM! French translations are complete and properly localized.The French translations mirror the English structure with appropriate grammatical constructions. Interpolation variables are correctly preserved. As per coding guidelines, both en and fr locale files have been updated with the new feedback keys.
apps/frontend/src/shared/types/ipc.ts (2)
45-46: LGTM! Import addition for ImageAttachment type.The import correctly brings in
ImageAttachmentfrom the shared task types, enabling type-safe image handling in the IPC layer.
162-162: LGTM! Backward-compatible API extension.The
submitReviewsignature is extended with an optionalimagesparameter, maintaining backward compatibility with existing callers while enabling the new image attachment feature.apps/frontend/src/main/ipc-handlers/task/execution-handlers.ts (4)
3-3: LGTM! Type import for ImageAttachment.
5-5: LGTM! Added mkdirSync import for directory creation.
321-322: LGTM! Handler signature extended to accept images.The optional
imagesparameter correctly typed withImageAttachment[]maintains backward compatibility.
449-449: LGTM! Image references correctly appended to QA fix request content.The markdown format
is appropriate for including image references in the QA fix request document.apps/frontend/src/renderer/stores/task-store.ts (2)
2-2: LGTM! ImageAttachment type imported for use in submitReview.
496-502: LGTM! submitReview function correctly extended to support images.The function signature and IPC call properly forward the optional
imagesparameter, maintaining backward compatibility while enabling the new image attachment feature.apps/frontend/src/renderer/components/task-detail/hooks/useTaskDetail.ts (5)
4-4: LGTM! ImageAttachment type imported for state typing.
53-53: LGTM! Feedback images state properly initialized.
165-168: Good defensive coding - reset images when task changes.This prevents image leakage between tasks, ensuring users don't accidentally submit images from a previous task's feedback session.
264-282: LGTM! Well-structured image management callbacks.All four callbacks (
addFeedbackImage,addFeedbackImages,removeFeedbackImage,clearFeedbackImages) are properly memoized withuseCallbackand have stable empty dependency arrays, preventing unnecessary re-renders.
433-433: LGTM! State and callbacks properly exposed in hook return.The
feedbackImagesstate,setFeedbackImagessetter, and the four image management callbacks are all correctly exposed for consumption by components.Also applies to: 477-477, 513-516
apps/frontend/src/renderer/components/task-detail/TaskReview.tsx (4)
1-1: LGTM! ImageAttachment type imported for props typing.
36-39: LGTM! Well-documented optional props for image handling.The JSDoc comments clearly describe the purpose of each prop, and making them optional maintains backward compatibility with existing usages.
88-89: LGTM! Props correctly destructured.
166-167: LGTM! Image props correctly passed to QAFeedbackSection.The
imagesandonImagesChangeprops are properly threaded through to the child component, completing the prop drilling for the image attachment feature.apps/frontend/src/renderer/components/task-detail/TaskDetailModal.tsx (2)
120-130: LGTM! Correctly updated rejection flow to support image attachments.The logic properly allows submission when either text feedback or images are present, passes
feedbackImagestosubmitReview, and clears the image state after successful submission.
521-522: LGTM! Proper wiring of image state to TaskReview.The
imagesandonImagesChangeprops are correctly passed toTaskReview, enabling the image attachment flow through toQAFeedbackSection.apps/frontend/src/preload/api/task-api.ts (3)
16-18: LGTM! Correctly added ImageAttachment type import.
36-41: LGTM! Interface signature properly extended for image attachments.The optional
imagesparameter is correctly typed and positioned after the existing optionalfeedbackparameter.
114-120: The handler inexecution-handlers.tsalready correctly accepts and processes theimagesparameter. The implementation at lines 316-323 registers the TASK_REVIEW handler with the proper signature, includingimages?: ImageAttachment[]as the 5th parameter. The handler actively processes these images at lines 411-444, saving them to afeedback_imagesdirectory and including image references in the generated QA_FIX_REQUEST.md file.apps/frontend/src/renderer/components/task-detail/task-review/QAFeedbackSection.tsx (5)
1-18: LGTM! Well-organized imports.All necessary utilities, types, and constants are properly imported to support the image attachment feature.
20-29: LGTM! Good backward-compatible props extension.The optional
imagesandonImagesChangeprops maintain backward compatibility with existing usages while enabling the new image attachment feature.
141-154: LGTM! Proper drag event handling.The drag over/leave handlers correctly manage event propagation and update the visual feedback state.
241-248: LGTM! Clean image removal and submission logic.The
handleRemoveImagefunction properly filters by ID, andcanSubmitcorrectly allows submission with either text feedback or attached images.
250-361: LGTM! Good i18n compliance and accessible UI.All new user-facing strings use the
t()translation function with appropriate keys and fallbacks. The image thumbnails include properalttext andaria-labelfor accessibility.
| const base64Data = image.data.replace(/^data:image\/\w+;base64,/, ''); | ||
| const imageBuffer = Buffer.from(base64Data, 'base64'); | ||
| const imagePath = path.join(imagesDir, image.filename); | ||
| writeFileSync(imagePath, imageBuffer); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider using async file operations to avoid blocking the event loop.
writeFileSync is used inside an async IPC handler. For potentially large image files, this could block the main process 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, mkdirSync at line 417 could use await fsPromises.mkdir(imagesDir, { recursive: true }).
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| writeFileSync(imagePath, imageBuffer); | |
| await fsPromises.writeFile(imagePath, imageBuffer); |
🤖 Prompt for AI Agents
In @apps/frontend/src/main/ipc-handlers/task/execution-handlers.ts at line 430,
Replace the blocking calls writeFileSync and mkdirSync inside the async IPC
handler with their async fs.promises equivalents: use await
fs.promises.mkdir(imagesDir, { recursive: true }) (replacing mkdirSync) and
await fs.promises.writeFile(imagePath, imageBuffer) (replacing writeFileSync);
ensure the handler function is async (it already is) and import/alias
fs.promises (or fsPromises) at the top so you await these operations and
handle/rethrow errors as appropriate around the imagePath/imageBuffer/imagesDir
logic.
apps/frontend/src/renderer/components/task-detail/task-review/QAFeedbackSection.tsx
Show resolved
Hide resolved
| if (newImages.length > 0) { | ||
| onImagesChange([...images, ...newImages]); | ||
| // Show success feedback | ||
| setPasteSuccess(true); | ||
| setTimeout(() => setPasteSuccess(false), 2000); | ||
| } | ||
| }, [images, onImagesChange, t]); |
There was a problem hiding this comment.
Minor: setTimeout without cleanup may cause state update on unmounted component.
If the component unmounts before the 2-second timeout completes, setPasteSuccess(false) will attempt to update state on an unmounted component.
Proposed fix using useEffect cleanup pattern
Consider storing the timeout ID in a ref and clearing it on unmount:
+ const pasteTimeoutRef = useRef<ReturnType<typeof setTimeout> | null>(null);
+
+ // Cleanup timeout on unmount
+ useEffect(() => {
+ return () => {
+ if (pasteTimeoutRef.current) {
+ clearTimeout(pasteTimeoutRef.current);
+ }
+ };
+ }, []);
// In handlePaste and handleTextareaDrop:
if (newImages.length > 0) {
onImagesChange([...images, ...newImages]);
// Show success feedback
setPasteSuccess(true);
- setTimeout(() => setPasteSuccess(false), 2000);
+ if (pasteTimeoutRef.current) clearTimeout(pasteTimeoutRef.current);
+ pasteTimeoutRef.current = setTimeout(() => setPasteSuccess(false), 2000);
}🤖 Prompt for AI Agents
In
@apps/frontend/src/renderer/components/task-detail/task-review/QAFeedbackSection.tsx
around lines 130 - 136, In QAFeedbackSection, the inline setTimeout used after
setPasteSuccess can update state after unmount; store the timeout ID (e.g., in a
ref like pasteTimeoutRef) when calling setTimeout in the paste handling logic
and clear it on unmount or when a new timeout is scheduled (useEffect cleanup or
a cleanup function) so that setPasteSuccess(false) is not called on an unmounted
component; ensure you clear the timeout using
clearTimeout(pasteTimeoutRef.current) and reset the ref when appropriate.
| } catch (imgError) { | ||
| console.error('[TASK_REVIEW] Failed to save image:', image.filename, imgError); | ||
| } | ||
| } |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
| await submitReview(task.id, false, state.feedback, state.feedbackImages); | ||
| state.setIsSubmitting(false); | ||
| state.setFeedback(''); | ||
| state.setFeedbackImages([]); |
There was a problem hiding this comment.
Form state cleared regardless of submission success
Medium Severity
The handleReject function clears feedbackImages (and feedback) unconditionally after calling submitReview, without checking whether submission succeeded. The submitReview function returns false on 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 new setFeedbackImages([]) call extends this data loss to images, which can be harder to recreate (e.g., screenshots). Compare with handleDelete in the same file which correctly checks result.success before cleanup.
apps/frontend/src/renderer/components/task-detail/hooks/useTaskDetail.ts
Show resolved
Hide resolved
…param - Resolve merge conflicts in tasks.json (en/fr) - include both feedback and review sections - Update ipc-bridge.test.ts to expect 5th parameter (images) in submitReview call Co-Authored-By: Claude Opus 4.5 <[email protected]>
AndyMik90
left a comment
AndyMik90
left a comment
There was a problem hiding this comment.
🤖 Auto Claude PR Review
🟠 Follow-up Review: Needs Revision
🟠 Needs revision - 2 new issue(s) found in recent changes.
Resolution Status
- ✅ Resolved: 2 previous findings addressed
- ❌ Unresolved: 0 previous findings remain
- 🆕 New Issues: 2 new findings in recent changes
Finding Validation
- 🔍 Dismissed as False Positives: 1 findings were re-investigated and found to be incorrect
- ✓ Confirmed Valid: 2 findings verified as genuine issues
- 👤 Needs Human Review: 0 findings require manual verification
Verdict
CI Status: ⏳ 8 checks pending - Cannot approve until CI completes.
Previous Findings: Both HIGH severity (path traversal) and MEDIUM severity (base64 regex) findings are now RESOLVED with proper defense-in-depth implementation.
New Issues: 2 LOW severity issues remain after validation (1 was dismissed as false positive):
- Missing server-side MIME validation (low practical risk)
- SVG file extension cosmetic issue
Recommendation: Once CI passes, this PR is ready to merge. The remaining issues are low severity and can be addressed in follow-up work. The security fix for path traversal is well-implemented with multiple layers of protection.
Review Process
Agents invoked: resolution-verifier, new-code-reviewer, finding-validator
This is an AI-generated follow-up review using parallel specialist analysis with finding validation.
Findings (2 selected of 2 total)
🔵 [REVIEW-NEW-001] [LOW] Missing server-side MIME type validation
📁 apps/frontend/src/main/ipc-handlers/task/execution-handlers.ts:432
The IPC handler writes image data without validating the MIME type. While frontend validates types, a compromised renderer could send arbitrary data. Practical risk is LOW since files are not executed and path traversal is prevented.
Suggested fix:
Add server-side validation: if (!ALLOWED_IMAGE_TYPES.includes(image.mimeType)) { continue; }
🔵 [REVIEW-NEW-003] [LOW] SVG file extension includes '+xml' suffix
📁 apps/frontend/src/renderer/components/task-detail/task-review/QAFeedbackSection.tsx:109
When pasting SVG images, file.type.split('/')[1] produces 'svg+xml' instead of 'svg', resulting in non-standard file extensions like '.svg+xml'.
Suggested fix:
Use MIME type to extension mapping: const mimeToExt = {'image/svg+xml': 'svg', ...}
This review was generated by Auto Claude.
- Add server-side MIME type validation for image uploads (defense in depth) - Fix SVG file extension: map 'image/svg+xml' to '.svg' instead of '.svg+xml' - Add MIME-to-extension mapping for all allowed image types Co-Authored-By: Claude Opus 4.5 <[email protected]>
github-actions
bot
added
🔄 Checking
| const ALLOWED_MIME_TYPES = ['image/png', 'image/jpeg', 'image/gif', 'image/webp', 'image/svg+xml']; | ||
| if (image.mimeType && !ALLOWED_MIME_TYPES.includes(image.mimeType)) { | ||
| console.warn('[TASK_REVIEW] Skipping image with disallowed MIME type:', image.mimeType); | ||
| continue; | ||
| } |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
AndyMik90
left a comment
AndyMik90
left a comment
There was a problem hiding this comment.
🤖 Auto Claude PR Review
🟠 Follow-up Review: Needs Revision
🟠 Needs revision - 2 blocking issue(s) require fixes.
Resolution Status
- ✅ Resolved: 2 previous findings addressed
- ❌ Unresolved: 0 previous findings remain
- 🆕 New Issues: 4 new findings in recent changes
Finding Validation
- 🔍 Dismissed as False Positives: 0 findings were re-investigated and found to be incorrect
- ✓ Confirmed Valid: 2 findings verified as genuine issues
- 👤 Needs Human Review: 0 findings require manual verification
🚨 Blocking Issues
- security: MIME validation bypass when mimeType is undefined
- quality: Drop handler missing SVG extension mapping (incomplete fix)
Verdict
Two factors require revision before merge: (1) CI Status: 2 checks still pending - cannot approve until CI completes. (2) Two MEDIUM severity issues confirmed valid: NEW-002 (MIME validation bypass when mimeType is undefined) and NEW-003 (SVG extension fix incomplete - drop handler not updated). Both previous LOW findings (REVIEW-NEW-001 and REVIEW-NEW-003) are now fully resolved. The fixes are straightforward: change the MIME validation condition to reject missing mimeType, and apply the mimeToExtension mapping to the drop handler.
Review Process
Agents invoked: resolution-verifier, new-code-reviewer, finding-validator
This is an AI-generated follow-up review using parallel specialist analysis with finding validation.
Findings (4 selected of 4 total)
🟡 [NEW-002] [MEDIUM] MIME validation bypass when mimeType is undefined
📁 apps/frontend/src/main/ipc-handlers/task/execution-handlers.ts:428
The server-side MIME type validation condition 'if (image.mimeType && ...)' allows images with undefined/null/empty mimeType to bypass validation entirely. A malformed image object without a mimeType field would skip the MIME type check and proceed to file writing.
Suggested fix:
Change to: if (!image.mimeType || !ALLOWED_MIME_TYPES.includes(image.mimeType)) { console.warn('[TASK_REVIEW] Skipping image with missing or disallowed MIME type:', image.mimeType); continue; }
🟡 [NEW-003] [MEDIUM] Drop handler missing SVG extension mapping (incomplete fix)
📁 apps/frontend/src/renderer/components/task-detail/task-review/QAFeedbackSection.tsx:216
The SVG extension fix was only applied to the paste handler. The drop handler still uses file.type.split('/')[1] which produces 'svg+xml' for SVG files without filenames. This is an inconsistency - pasted SVGs get correct '.svg' extension but dropped SVGs get '.svg+xml'.
Suggested fix:
Apply the same mimeToExtension mapping used in handlePaste: const extension = mimeToExtension[file.type] || file.type.split('/')[1] || 'png'; const baseFilename = file.name || `dropped-image-${Date.now()}.${extension}`;
🔵 [NEW-001] [LOW] MIME allowlist mismatch with shared constants
📁 apps/frontend/src/main/ipc-handlers/task/execution-handlers.ts:427
The server-side ALLOWED_MIME_TYPES array is hardcoded and does not include 'image/jpg' which exists in the shared ALLOWED_IMAGE_TYPES constant. This creates inconsistent behavior where 'image/jpg' passes frontend validation but fails server-side validation.
Suggested fix:
Add 'image/jpg' to the list or import from shared constants for consistency.
🔵 [NEW-004] [LOW] Inconsistent data format between image data and thumbnail
📁 apps/frontend/src/renderer/components/task-detail/task-review/QAFeedbackSection.tsx:129
Image data is stored as raw base64 (without data URL prefix) while thumbnail retains the full data URL prefix. This inconsistency could cause confusion when consuming the data. Consider documenting this design decision.
Suggested fix:
Add documentation to ImageAttachment interface explaining that 'data' is raw base64 and 'thumbnail' is a display-ready data URL.
This review was generated by Auto Claude.
- Change MIME validation to reject missing mimeType (prevents bypass) - Add 'image/jpg' to server-side allowlist for consistency - Apply mimeToExtension mapping to drop handler (was only in paste handler) Co-Authored-By: Claude Opus 4.5 <[email protected]>
github-actions
bot
added
size/L
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In
@apps/frontend/src/renderer/components/task-detail/task-review/QAFeedbackSection.tsx:
- Around line 140-143: The setTimeout calls in QAFeedbackSection (where
setPasteSuccess(true) is followed by setTimeout(() => setPasteSuccess(false),
2000)) may fire after unmount; add a cleanup using a ref to store the timeout ID
(e.g., timeoutRef) and clear it in a useEffect cleanup so pending timeouts are
cancelled on unmount; update the handlers to assign the timeout ID to
timeoutRef.current instead of calling setTimeout directly and call
clearTimeout(timeoutRef.current) in the effect cleanup.
- Around line 110-116: The mimeToExtension map is duplicated in handlePaste and
handleTextareaDrop; extract it to a single shared constant (e.g.,
MIME_TO_EXTENSION) at module scope or import from a common util and replace the
inline maps in both functions with that constant; update any references to use
MIME_TO_EXTENSION and ensure exports/imports are added if moving to a shared
utility.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
apps/frontend/src/main/ipc-handlers/task/execution-handlers.tsapps/frontend/src/renderer/components/task-detail/task-review/QAFeedbackSection.tsx
🧰 Additional context used
📓 Path-based instructions (2)
apps/frontend/src/**/*.{tsx,ts}
📄 CodeRabbit inference engine (CLAUDE.md)
apps/frontend/src/**/*.{tsx,ts}: Use i18n translation keys for all user-facing text in the frontend. All labels, buttons, messages must use translation keys from react-i18next with namespace:section.key format (e.g., 'navigation:items.githubPRs').
Never use hardcoded strings for UI text in JSX/TSX files. Always use translation keys via useTranslation() hook.
Files:
apps/frontend/src/renderer/components/task-detail/task-review/QAFeedbackSection.tsxapps/frontend/src/main/ipc-handlers/task/execution-handlers.ts
apps/frontend/**/*.{ts,tsx}
⚙️ CodeRabbit configuration file
apps/frontend/**/*.{ts,tsx}: Review React patterns and TypeScript type safety.
Check for proper state management and component composition.
Files:
apps/frontend/src/renderer/components/task-detail/task-review/QAFeedbackSection.tsxapps/frontend/src/main/ipc-handlers/task/execution-handlers.ts
🧠 Learnings (1)
📚 Learning: 2026-01-04T23:59:45.209Z
Learnt from: MikeeBuilds
Repo: AndyMik90/Auto-Claude PR: 661
File: apps/frontend/src/renderer/components/onboarding/OllamaModelSelector.tsx:176-189
Timestamp: 2026-01-04T23:59:45.209Z
Learning: In the AndyMik90/Auto-Claude repository, pre-existing i18n issues (hardcoded user-facing strings) can be deferred for future i18n cleanup passes. Do not fix such issues in PRs that do not introduce new i18n violations, especially in frontend TSX components (e.g., apps/frontend/**/*.tsx). If a PR adds new i18n violations, address them in that PR.
Applied to files:
apps/frontend/src/renderer/components/task-detail/task-review/QAFeedbackSection.tsx
🧬 Code graph analysis (2)
apps/frontend/src/renderer/components/task-detail/task-review/QAFeedbackSection.tsx (3)
apps/frontend/src/shared/types/task.ts (1)
ImageAttachment(117-125)apps/frontend/src/shared/constants/task.ts (2)
MAX_IMAGES_PER_TASK(197-197)ALLOWED_IMAGE_TYPES_DISPLAY(216-216).design-system/src/lib/icons.ts (2)
X(37-37)RotateCcw(31-31)
apps/frontend/src/main/ipc-handlers/task/execution-handlers.ts (1)
apps/frontend/src/shared/types/task.ts (1)
ImageAttachment(117-125)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Cursor Bugbot
- GitHub Check: Seer Code Review
- GitHub Check: CodeQL (python)
- GitHub Check: CodeQL (javascript-typescript)
🔇 Additional comments (4)
apps/frontend/src/main/ipc-handlers/task/execution-handlers.ts (2)
411-464: LGTM - Well-designed defense-in-depth security measures.The server-side image processing includes proper security controls:
- MIME type allowlist validation
- Filename sanitization via
path.basename()- Path traversal prevention with resolved path verification
- Graceful per-image error handling
One minor note:
'image/jpg'on line 428 is technically non-standard (browsers use'image/jpeg'), but including it as a fallback is reasonable for defense-in-depth since the frontend might pass it in edge cases.
315-323: LGTM - Clean optional parameter addition.The optional
images?: ImageAttachment[]parameter maintains backward compatibility with existing callers while enabling the new image attachment feature.apps/frontend/src/renderer/components/task-detail/task-review/QAFeedbackSection.tsx (2)
35-46: LGTM - Clean component design with proper i18n.The component correctly:
- Uses
useTranslation('tasks')with namespace-prefixed keys- Provides English fallbacks for all translation calls
- Maintains backward compatibility via optional
imagesandonImagesChangeprops- Gates image upload features on
onImagesChangepresence
264-266: LGTM - Correct submission logic.The
canSubmitlogic properly allows submission with either text feedback or attached images, supporting the use case of visual-only feedback.
| const handlePaste = useCallback(async (e: ClipboardEvent<HTMLTextAreaElement>) => { | ||
| // Skip image handling if feature is not enabled | ||
| if (!onImagesChange) return; | ||
|
|
||
| const clipboardItems = e.clipboardData?.items; | ||
| if (!clipboardItems) return; | ||
|
|
||
| // Find image items in clipboard | ||
| const imageItems: DataTransferItem[] = []; | ||
| for (let i = 0; i < clipboardItems.length; i++) { | ||
| const item = clipboardItems[i]; | ||
| if (item.type.startsWith('image/')) { | ||
| imageItems.push(item); | ||
| } | ||
| } | ||
|
|
||
| // If no images, allow normal paste behavior | ||
| if (imageItems.length === 0) return; | ||
|
|
||
| // Prevent default paste when we have images | ||
| e.preventDefault(); | ||
|
|
||
| // Check if we can add more images | ||
| const remainingSlots = MAX_IMAGES_PER_TASK - images.length; | ||
| if (remainingSlots <= 0) { | ||
| setError(t('feedback.maxImagesError', { count: MAX_IMAGES_PER_TASK })); | ||
| return; | ||
| } | ||
|
|
||
| setError(null); | ||
|
|
||
| // Process image items | ||
| const newImages: ImageAttachment[] = []; | ||
| const existingFilenames = images.map(img => img.filename); | ||
|
|
||
| for (const item of imageItems.slice(0, remainingSlots)) { | ||
| const file = item.getAsFile(); | ||
| if (!file) continue; | ||
|
|
||
| // Validate image type | ||
| if (!isValidImageMimeType(file.type)) { | ||
| setError(t('feedback.invalidTypeError', { types: ALLOWED_IMAGE_TYPES_DISPLAY })); | ||
| continue; | ||
| } | ||
|
|
||
| try { | ||
| const dataUrl = await blobToBase64(file); | ||
| const thumbnail = await createThumbnail(dataUrl); | ||
|
|
||
| // Generate filename for pasted images (screenshot-timestamp.ext) | ||
| // Map MIME types to proper file extensions (handles svg+xml -> svg, etc.) | ||
| const mimeToExtension: Record<string, string> = { | ||
| 'image/svg+xml': 'svg', | ||
| 'image/jpeg': 'jpg', | ||
| 'image/png': 'png', | ||
| 'image/gif': 'gif', | ||
| 'image/webp': 'webp', | ||
| }; | ||
| const extension = mimeToExtension[file.type] || file.type.split('/')[1] || 'png'; | ||
| const baseFilename = `screenshot-${Date.now()}.${extension}`; | ||
| const resolvedFilename = resolveFilename(baseFilename, [ | ||
| ...existingFilenames, | ||
| ...newImages.map(img => img.filename) | ||
| ]); | ||
|
|
||
| newImages.push({ | ||
| id: generateImageId(), | ||
| filename: resolvedFilename, | ||
| mimeType: file.type, | ||
| size: file.size, | ||
| data: dataUrl.split(',')[1], // Store base64 without data URL prefix | ||
| thumbnail | ||
| }); | ||
| } catch (error) { | ||
| console.error('[QAFeedbackSection] Failed to process pasted image:', error); | ||
| setError(t('feedback.processingError', 'Failed to process pasted image')); | ||
| } | ||
| } | ||
|
|
||
| if (newImages.length > 0) { | ||
| onImagesChange([...images, ...newImages]); | ||
| // Show success feedback | ||
| setPasteSuccess(true); | ||
| setTimeout(() => setPasteSuccess(false), 2000); | ||
| } | ||
| }, [images, onImagesChange, t]); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for file size validation patterns
rg -n "MAX.*SIZE|maxSize|size.*limit|MAX.*FILE" --type=ts --type=tsx apps/frontend/src/Repository: AndyMik90/Auto-Claude
Length of output: 92
🏁 Script executed:
#!/bin/bash
# Find the image validation utility function
fd ".*\.ts$|.*\.tsx$" apps/frontend/src/ | xargs rg -l "isValidImageMimeType" | head -5Repository: AndyMik90/Auto-Claude
Length of output: 340
🏁 Script executed:
#!/bin/bash
# Look for image-related constants and limits
rg -n "MAX_IMAGES|ALLOWED_IMAGE|IMAGE.*LIMIT|FILE.*LIMIT" --type=ts --type=tsx apps/frontend/src/Repository: AndyMik90/Auto-Claude
Length of output: 92
🏁 Script executed:
#!/bin/bash
# Check the server-side handler for image validation
find . -name "execution-handlers.ts" -o -name "*execution*handler*" -type f | head -10Repository: AndyMik90/Auto-Claude
Length of output: 129
🏁 Script executed:
#!/bin/bash
# Search for size-related constants and validation with correct syntax
rg -n "MAX.*SIZE|maxSize|size.*limit|MAX.*FILE|MAX_IMAGES" -t ts apps/frontend/src/Repository: AndyMik90/Auto-Claude
Length of output: 6251
🏁 Script executed:
#!/bin/bash
# Find and display the isValidImageMimeType function
fd "useImageUpload|ImageUpload" -t f apps/frontend/src/ | while read file; do
if rg -q "isValidImageMimeType" "$file"; then
echo "=== File: $file ==="
rg -B2 -A10 "isValidImageMimeType" "$file" | head -30
fi
doneRepository: AndyMik90/Auto-Claude
Length of output: 2522
🏁 Script executed:
#!/bin/bash
# Check execution-handlers.ts for image size validation
cat -n ./apps/frontend/src/main/ipc-handlers/task/execution-handlers.ts | grep -A20 -B5 -i "image\|size\|file.*size" | head -60Repository: AndyMik90/Auto-Claude
Length of output: 2902
🏁 Script executed:
#!/bin/bash
# Search for MAX_IMAGES_PER_TASK constant location
rg -n "MAX_IMAGES_PER_TASK" -t ts apps/frontend/src/ -B2 -A2Repository: AndyMik90/Auto-Claude
Length of output: 6458
🏁 Script executed:
#!/bin/bash
# Check QAFeedbackSection.tsx for the drop handler and any size validation
cat -n apps/frontend/src/renderer/components/task-detail/task-review/QAFeedbackSection.tsx | sed -n '1,300p' | tail -100Repository: AndyMik90/Auto-Claude
Length of output: 4606
🏁 Script executed:
#!/bin/bash
# Specifically look for file size validation in QAFeedbackSection
rg -n "MAX_IMAGE_SIZE|file\.size" apps/frontend/src/renderer/components/task-detail/task-review/QAFeedbackSection.tsxRepository: AndyMik90/Auto-Claude
Length of output: 128
🏁 Script executed:
#!/bin/bash
# Check ImageUpload.tsx to see where MAX_IMAGE_SIZE is actually validated
cat -n apps/frontend/src/renderer/components/ImageUpload.tsx | sed -n '160,180p'Repository: AndyMik90/Auto-Claude
Length of output: 1018
🏁 Script executed:
#!/bin/bash
# Confirm QAFeedbackSection does not import MAX_IMAGE_SIZE
head -30 apps/frontend/src/renderer/components/task-detail/task-review/QAFeedbackSection.tsx | grep -E "import|MAX_IMAGE"Repository: AndyMik90/Auto-Claude
Length of output: 526
🏁 Script executed:
#!/bin/bash
# Check if there's any file size validation in the entire QAFeedbackSection file
rg -n "MAX_IMAGE_SIZE|file.*size|size.*limit" apps/frontend/src/renderer/components/task-detail/task-review/QAFeedbackSection.tsxRepository: AndyMik90/Auto-Claude
Length of output: 128
Add MAX_IMAGE_SIZE validation to paste and drop handlers.
The handlePaste and drop handlers in this file capture file.size but don't validate against MAX_IMAGE_SIZE (10 MB), unlike the ImageUpload component (line 170) which validates and warns users. Import MAX_IMAGE_SIZE from constants and add size validation before processing images to maintain consistency and prevent excessively large images from being processed.
| const mimeToExtension: Record<string, string> = { | ||
| 'image/svg+xml': 'svg', | ||
| 'image/jpeg': 'jpg', | ||
| 'image/png': 'png', | ||
| 'image/gif': 'gif', | ||
| 'image/webp': 'webp', | ||
| }; |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Extract duplicate mimeToExtension map to a shared constant.
The mimeToExtension map is duplicated between handlePaste (lines 110-116) and handleTextareaDrop (lines 217-223). Consider extracting this to a constant at module scope or importing from a shared utility.
♻️ Suggested refactor
+// At module scope or import from shared utility
+const MIME_TO_EXTENSION: Record<string, string> = {
+ 'image/svg+xml': 'svg',
+ 'image/jpeg': 'jpg',
+ 'image/png': 'png',
+ 'image/gif': 'gif',
+ 'image/webp': 'webp',
+};
// Then in handlePaste and handleTextareaDrop:
- const mimeToExtension: Record<string, string> = {
- 'image/svg+xml': 'svg',
- 'image/jpeg': 'jpg',
- 'image/png': 'png',
- 'image/gif': 'gif',
- 'image/webp': 'webp',
- };
- const extension = mimeToExtension[file.type] || file.type.split('/')[1] || 'png';
+ const extension = MIME_TO_EXTENSION[file.type] || file.type.split('/')[1] || 'png';🤖 Prompt for AI Agents
In
@apps/frontend/src/renderer/components/task-detail/task-review/QAFeedbackSection.tsx
around lines 110 - 116, The mimeToExtension map is duplicated in handlePaste and
handleTextareaDrop; extract it to a single shared constant (e.g.,
MIME_TO_EXTENSION) at module scope or import from a common util and replace the
inline maps in both functions with that constant; update any references to use
MIME_TO_EXTENSION and ensure exports/imports are added if moving to a shared
utility.
| // Show success feedback | ||
| setPasteSuccess(true); | ||
| setTimeout(() => setPasteSuccess(false), 2000); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider cleaning up timeout on unmount.
The setTimeout on lines 141-142 (and 248-249) could attempt a state update after component unmount. While React handles this gracefully, adding cleanup would be cleaner.
♻️ Optional: Use ref-based cleanup
// At component scope
const timeoutRef = useRef<ReturnType<typeof setTimeout>>();
// In useEffect cleanup
useEffect(() => {
return () => {
if (timeoutRef.current) clearTimeout(timeoutRef.current);
};
}, []);
// Then in handlers:
timeoutRef.current = setTimeout(() => setPasteSuccess(false), 2000);🤖 Prompt for AI Agents
In
@apps/frontend/src/renderer/components/task-detail/task-review/QAFeedbackSection.tsx
around lines 140 - 143, The setTimeout calls in QAFeedbackSection (where
setPasteSuccess(true) is followed by setTimeout(() => setPasteSuccess(false),
2000)) may fire after unmount; add a cleanup using a ref to store the timeout ID
(e.g., timeoutRef) and clear it in a useEffect cleanup so pending timeouts are
cancelled on unmount; update the handlers to assign the timeout ID to
timeoutRef.current instead of calling setTimeout directly and call
clearTimeout(timeoutRef.current) in the effect cleanup.
| } | ||
|
|
||
| if (newImages.length > 0) { | ||
| onImagesChange([...images, ...newImages]); |
There was a problem hiding this comment.
Race condition in async image handlers causes data loss
Medium Severity
The handlePaste and handleTextareaDrop callbacks capture the images array from the closure at creation time, then use it after async operations (blobToBase64, createThumbnail) complete. If a user pastes or drops images rapidly before React re-renders, multiple callbacks execute with the same stale images value. When each calls onImagesChange([...images, ...newImages]), they overwrite each other's additions instead of accumulating. Using functional state updates (onImagesChange(prev => [...prev, ...newImages])) would ensure operations always build on the latest state.
Additional Locations (1)
AndyMik90
left a comment
AndyMik90
left a comment
There was a problem hiding this comment.
🤖 Auto Claude PR Review
🟠 Follow-up Review: Needs Revision
🟠 Needs revision - 2 new issue(s) found in recent changes.
Resolution Status
- ✅ Resolved: 4 previous findings addressed
- ❌ Unresolved: 0 previous findings remain
- 🆕 New Issues: 2 new findings in recent changes
Finding Validation
- 🔍 Dismissed as False Positives: 1 findings were re-investigated and found to be incorrect
- ✓ Confirmed Valid: 0 findings verified as genuine issues
- 👤 Needs Human Review: 0 findings require manual verification
Verdict
CI Status: 1 check pending - cannot approve until CI completes.
Code Quality: Excellent progress on this commit:
- ✅ 3/4 previous findings fully resolved (NEW-001, NEW-002, NEW-003)
- ✅ 1/4 previous finding dismissed as false positive (NEW-004 - intentional design)
⚠️ 2 new LOW severity findings (both defense-in-depth suggestions with existing mitigations)
The commit successfully addresses the MIME validation bypass (NEW-002) and SVG extension mapping (NEW-003) as claimed. The 2 new findings are minor defense-in-depth suggestions - the server-side already has proper path traversal protection.
What's blocking: Only waiting for CI to complete. Once CI passes and no failures are reported, this PR should be ready to merge. The LOW severity findings are optional improvements that don't block merge.
Review Process
Agents invoked: resolution-verifier, new-code-reviewer, finding-validator
This is an AI-generated follow-up review using parallel specialist analysis with finding validation.
Findings (2 selected of 2 total)
🔵 [NEWCODE-001] [LOW] Path traversal check edge case (defense-in-depth)
📁 apps/frontend/src/main/ipc-handlers/task/execution-handlers.ts:446
The path traversal protection uses startsWith(resolvedImagesDir + path.sep) which theoretically wouldn't catch a path exactly equal to resolvedImagesDir. However, this is already mitigated by path.basename() sanitization and checks for '.' and '..' filenames. The layered defenses make this unexploitable.
Suggested fix:
Optional enhancement: Add check `resolvedPath === resolvedImagesDir` for completeness, though current implementation is safe.
🔵 [NEWCODE-002] [LOW] Frontend could sanitize filename (defense-in-depth)
📁 apps/frontend/src/renderer/components/task-detail/task-review/QAFeedbackSection.tsx:225
The drop handler uses file.name directly without sanitization before transport. While the server properly sanitizes with path.basename(), defense-in-depth suggests sanitizing on both sides.
Suggested fix:
Consider: const sanitizedName = file.name.split(/[\\/]/).pop() || `dropped-image-${Date.now()}.${extension}`;
This review was generated by Auto Claude.
…eedback-interface
AndyMik90
deleted the
auto-claude/107-add-file-upload-to-task-feedback-interface
branch
* auto-claude: subtask-1-1 - Add feedbackImages state and handlers to useTaskDetail - Add feedbackImages state as ImageAttachment[] for storing feedback images - Add setFeedbackImages setter for direct state updates - Add addFeedbackImage handler for adding a single image - Add addFeedbackImages handler for adding multiple images at once - Add removeFeedbackImage handler for removing an image by ID - Add clearFeedbackImages handler for clearing all images - Import ImageAttachment type from shared/types 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]> * auto-claude: subtask-1-2 - Update IPC interface to support images in submitReview - Add ImageAttachment import from ./task types - Update submitReview signature to include optional images parameter 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]> * auto-claude: subtask-1-3 - Update submitReview function in task-store to accept and pass images * auto-claude: subtask-2-1 - Add paste/drop handlers and image thumbnail displa - Add paste event handler for screenshot/image clipboard support - Add drag-over and drag-leave handlers for visual feedback during drag - Add drop handler for image file drops - Add image thumbnail display (64x64) with remove button on hover - Import image utilities from ImageUpload.tsx (generateImageId, blobToBase64, etc.) - Add i18n support for all new UI text - Make new props optional for backward compatibility during incremental rollout - Allow submission with either text feedback or images (not both required) - Add visual drag feedback with border/background color change * auto-claude: subtask-2-2 - Update TaskReview to pass image props to QAFeedbackSection * auto-claude: subtask-2-3 - Update TaskDetailModal to manage image state and pass to TaskReview - Pass feedbackImages and setFeedbackImages from useTaskDetail hook to TaskReview - Update handleReject to include images in submitReview call - Allow submission with images only (no text required) - Clear images after successful submission * auto-claude: subtask-3-1 - Add English translations for feedback image UI * auto-claude: subtask-3-2 - Add French translations for feedback image UI * fix(security): sanitize image filename to prevent path traversal - Use path.basename() to strip directory components from filenames - Validate sanitized filename is not empty, '.', or '..' - Add defense-in-depth check verifying resolved path stays within target directory - Fix base64 data URL regex to handle complex MIME types (e.g., svg+xml) Co-Authored-By: Claude Opus 4.5 <[email protected]> * fix: add MIME type validation and fix SVG file extension - Add server-side MIME type validation for image uploads (defense in depth) - Fix SVG file extension: map 'image/svg+xml' to '.svg' instead of '.svg+xml' - Add MIME-to-extension mapping for all allowed image types Co-Authored-By: Claude Opus 4.5 <[email protected]> * fix: require mimeType and apply SVG extension fix to drop handler - Change MIME validation to reject missing mimeType (prevents bypass) - Add 'image/jpg' to server-side allowlist for consistency - Apply mimeToExtension mapping to drop handler (was only in paste handler) Co-Authored-By: Claude Opus 4.5 <[email protected]> --------- Co-authored-by: Claude Opus 4.5 <[email protected]> Co-authored-by: Test User <[email protected]>
* ci: add Azure auth test workflow * fix(worktree): handle "already up to date" case correctly (ACS-226) (#961) * fix(worktree): handle "already up to date" case correctly (ACS-226) When git merge returns non-zero for "Already up to date", the merge code incorrectly treated this as a conflict and aborted. Now checks git output to distinguish between: - "Already up to date" - treat as success (nothing to merge) - Actual conflicts - abort as before - Other errors - show actual error message Also added comprehensive tests for edge cases: - Already up to date with no_commit=True - Already up to date with delete_after=True - Actual merge conflict detection - Merge conflict with no_commit=True * test: strengthen merge conflict abort verification Improve assertions in conflict detection tests to explicitly verify: - MERGE_HEAD does not exist after merge abort - git status returns clean (no staged/unstaged changes) This is more robust than just checking for absence of "CONFLICT" string, as git status --porcelain uses status codes, not literal words. * test: add git command success assertions and branch deletion verification - Add explicit returncode assertions for all subprocess.run git add/commit calls - Add branch deletion verification in test_merge_worktree_already_up_to_date_with_delete_after - Ensures tests fail early if git commands fail rather than continuing silently --------- Co-authored-by: StillKnotKnown <[email protected]> * fix(terminal): add collision detection for terminal drag and drop reordering (#985) * fix(terminal): add collision detection for terminal drag and drop reordering Add closestCenter collision detection to DndContext to fix terminal drag and drop swapping not detecting valid drop targets. The default rectIntersection algorithm required too much overlap for grid layouts. Co-Authored-By: Claude Opus 4.5 <[email protected]> * fix(terminal): handle file drops when closestCenter returns sortable ID Address PR review feedback: - Fix file drop handling to work when closestCenter collision detection returns the sortable ID instead of the droppable ID - Add terminals to useCallback dependency array to prevent stale state Co-Authored-By: Claude Opus 4.5 <[email protected]> --------- Co-authored-by: Claude Opus 4.5 <[email protected]> * fix(ACS-181): enable auto-switch on 401 auth errors & OAuth-only profiles (#900) * fix(ACS-181): enable auto-switch for OAuth-only profiles Add OAuth token check at the start of isProfileAuthenticated() so that profiles with only an oauthToken (no configDir) are recognized as authenticated. This allows the profile scorer to consider OAuth-only profiles as valid alternatives for proactive auto-switching. Previously, isProfileAuthenticated() immediately returned false if configDir was missing, causing OAuth-only profiles to receive a -500 penalty in the scorer and never be selected for auto-switch. Fixes: ACS-181 Co-Authored-By: Claude Opus 4.5 <[email protected]> Signed-off-by: Black Circle Sentinel <[email protected]> * fix(ACS-181): detect 'out of extra usage' rate limit messages The previous patterns only matched "Limit reached · resets ..." but Claude Code also shows "You're out of extra usage · resets ..." which wasn't being detected. This prevented auto-switch from triggering. Added new patterns to both output-parser.ts (terminal) and rate-limit-detector.ts (agent processes) to detect: - "out of extra usage · resets ..." - "You're out of extra usage · resets ..." Co-Authored-By: Claude Opus 4.5 <[email protected]> * fix(ACS-181): add real-time rate limit detection and debug logging - Add real-time rate limit detection in agent-process.ts processLog() so rate limits are detected immediately as output appears, not just when the process exits - Add clear warning message when auto-switch is disabled in settings - Add debug logging to profile-scorer.ts to trace profile evaluation - Add debug logging to rate-limit-detector.ts to trace pattern matching This enables immediate detection and auto-switch when rate limits occur during task execution. Co-Authored-By: Claude Opus 4.5 <[email protected]> * fix(frontend): enable auto-switch on 401 auth errors - Propagate 401/403 errors from fetchUsageViaAPI to checkUsageAndSwap in UsageMonitor to trigger proactive profile swapping. - Fix usage monitor race condition by ensuring it waits for ClaudeProfileManager initialization. - Fix isProfileAuthenticated to correctly validate OAuth-only profiles. * fix(ACS-181): address PR review feedback - Revert unrelated files (rate-limit-detector, output-parser, agent-process) to upstream state - Gate profile-scorer logging behind DEBUG flag - Fix usage-monitor type safety and correct catch block syntax - Fix misleading indentation in index.ts app updater block * fix(frontend): enforce eslint compliance for logs in profile-scorer - Replace all console.log with console.warn (per linter rules) - Strictly gate all debug logs behind isDebug check to prevent production noise * fix(ACS-181): add swap loop protection for auth failures - Add authFailedProfiles Map to track profiles with recent auth failures - Implement 5-minute cooldown before retrying failed profiles - Exclude failed profiles from swap candidates to prevent infinite loops - Gate TRACE logs behind DEBUG flag to reduce production noise - Change console.log to console.warn for ESLint compliance --------- Signed-off-by: Black Circle Sentinel <[email protected]> Co-authored-by: Claude Opus 4.5 <[email protected]> * feat(frontend): add Claude Code version rollback feature (#983) * feat(frontend): add Claude Code version rollback feature Add ability for users to switch to any of the last 20 Claude Code CLI versions directly from the Claude Code popup in the sidebar. Changes: - Add IPC channels for fetching available versions and installing specific version - Add backend handlers to fetch versions from npm registry (with 1-hour cache) - Add version selector dropdown in ClaudeCodeStatusBadge component - Add warning dialog before switching versions (warns about closing sessions) - Add i18n support for English and French translations Co-Authored-By: Claude Opus 4.5 <[email protected]> * fix: address PR review feedback for Claude Code version rollback - Add validation after semver filtering to handle empty version list - Add error state and UI feedback for installation/version switch failures - Extract magic number 5000ms to VERSION_RECHECK_DELAY_MS constant - Bind Select value prop to selectedVersion state - Normalize version comparison to handle 'v' prefix consistently - Use normalized version comparison in SelectItem disabled check Co-Authored-By: Claude Opus 4.5 <[email protected]> --------- Co-authored-by: Claude Opus 4.5 <[email protected]> * fix(security): inherit security profiles in worktrees and validate shell -c commands (#971) * fix(security): inherit security profiles in worktrees and validate shell -c commands - Add inherited_from field to SecurityProfile to mark profiles copied from parent projects - Skip hash-based re-analysis for inherited profiles (fixes worktrees losing npm/npx etc.) - Add shell_validators.py to validate commands inside bash/sh/zsh -c strings - Register shell validators to close security bypass via bash -c "arbitrary_command" - Add 13 new tests for inherited profiles and shell -c validation Fixes worktree security config not being inherited, which caused agents to be blocked from running npm/npx commands in isolated workspaces. * docs: update README download links to v2.7.3 (#976) - Update all stable download links from 2.7.2 to 2.7.3 - Add Flatpak download link (new in 2.7.3) * fix(security): close shell -c bypass vectors and validate inherited profiles - Fix combined shell flags bypass (-xc, -ec, -ic) in _extract_c_argument() Shell allows combining flags like `bash -xc 'cmd'` which bypassed -c detection - Add recursive validation for nested shell invocations Prevents bypass via `bash -c "bash -c 'evil_cmd'"` - Validate inherited_from path in should_reanalyze() with defense-in-depth - Must exist and be a directory - Must be an ancestor of current project - Must contain valid security profile - Add comprehensive test coverage for all security fixes Co-Authored-By: Claude Opus 4.5 <[email protected]> * style: fix import ordering in test_security.py Co-Authored-By: Claude Opus 4.5 <[email protected]> * style: format shell_validators.py Co-Authored-By: Claude Opus 4.5 <[email protected]> --------- Co-authored-by: Claude Opus 4.5 <[email protected]> * feat(frontend): add searchable branch combobox to worktree creation dialog (#979) * feat(frontend): add searchable branch combobox to worktree creation dialog - Replace limited Select dropdown with searchable Combobox for branch selection - Add new Combobox UI component with search filtering and scroll support - Remove 15-branch limit - now shows all branches with search - Improve worktree name validation to allow dots and underscores - Better sanitization: spaces become hyphens, preserve valid characters - Add i18n keys for branch search UI in English and French Co-Authored-By: Claude Opus 4.5 <[email protected]> * fix(frontend): address PR review feedback for worktree dialog - Extract sanitizeWorktreeName utility function to avoid duplication - Replace invalid chars with hyphens instead of removing them (feat/new → feat-new) - Trim trailing hyphens and dots from sanitized names - Add validation to forbid '..' in names (invalid for Git branch names) - Refactor branchOptions to use map/spread instead of forEach/push - Add ARIA accessibility: listboxId, aria-controls, role="listbox" Co-Authored-By: Claude Opus 4.5 <[email protected]> * fix(frontend): align worktree name validation with backend regex - Fix frontend validation to match backend WORKTREE_NAME_REGEX (no dots, must end with alphanumeric) - Update sanitizeWorktreeName to exclude dots from allowed characters - Update i18n messages (en/fr) to remove mention of dots - Add displayName to Combobox component for React DevTools - Export Combobox from UI component index.ts - Add aria-label to Combobox listbox for accessibility Co-Authored-By: Claude Opus 4.5 <[email protected]> * fix(frontend): address PR review accessibility and cleanup issues - Add forwardRef pattern to Combobox for consistency with other UI components - Add keyboard navigation (ArrowUp/Down, Enter, Escape, Home, End) - Add aria-activedescendant for screen reader focus tracking - Add unique option IDs for ARIA compliance - Add cleanup for async branch fetching to prevent state updates on unmounted component Co-Authored-By: Claude Opus 4.5 <[email protected]> --------- Co-authored-by: Claude Opus 4.5 <[email protected]> * fix(frontend): sync worktree config to renderer on terminal restoration (#982) * fix(frontend): sync worktree config to renderer on terminal restoration When terminals are restored after app restart, the worktree config was not being synced to the renderer, causing the worktree label to not appear. This adds a new IPC channel to send worktree config during restoration and a listener in useTerminalEvents to update the terminal store. Co-Authored-By: Claude Opus 4.5 <[email protected]> * fix(frontend): always sync worktreeConfig to handle deleted worktrees Addresses PR review feedback: send worktreeConfig IPC message unconditionally so the renderer can clear stale worktree labels when a worktree is deleted while the app is closed. Co-Authored-By: Claude Opus 4.5 <[email protected]> --------- Co-authored-by: Claude Opus 4.5 <[email protected]> * fix(merge): include files with content changes even when semantic analysis is empty (#986) * fix(merge): include files with content changes even when semantic analysis is empty The merge system was discarding files that had real code changes but no detected semantic changes. This happened because: 1. The semantic analyzer only detects imports and function additions/removals 2. Files with only function body modifications returned semantic_changes=[] 3. The filter used Python truthiness (empty list = False), excluding these files 4. This caused merges to fail with "0 files to merge" despite real changes The fix uses content hash comparison as a fallback check. If the file content actually changed (hash_before != hash_after), include it for merge regardless of whether the semantic analyzer could parse the specific change types. This fixes merging for: - Files with function body modifications (most common case) - Unsupported file types (Rust, Go, etc.) where semantic analysis returns empty - Any file where the analyzer fails to detect the specific change pattern Co-Authored-By: Claude Opus 4.5 <[email protected]> * refactor(merge): add TaskSnapshot.has_modifications property and handle DIRECT_COPY Address PR review feedback: 1. DRY improvement: Add `has_modifications` property to TaskSnapshot - Centralizes the modification detection logic - Checks semantic_changes first, falls back to content hash comparison - Handles both complete tasks and in-progress tasks safely 2. Fix for files with empty semantic_changes (Cursor issue #2): - Add DIRECT_COPY MergeDecision for files that were modified but couldn't be semantically analyzed (body changes, unsupported languages) - MergePipeline returns DIRECT_COPY when has_modifications=True but semantic_changes=[] (single task case) - Orchestrator handles DIRECT_COPY by reading file directly from worktree - This prevents silent data loss where apply_single_task_changes would return baseline content unchanged 3. Update _update_stats to count DIRECT_COPY as auto-merged The combination ensures: - Files ARE detected for merge (has_modifications check) - Files ARE properly merged (DIRECT_COPY reads from worktree) - No silent data loss (worktree content used instead of baseline) Co-Authored-By: Claude Opus 4.5 <[email protected]> * fix(merge): handle DIRECT_COPY in merge_tasks() and log missing files - Add DIRECT_COPY handling to merge_tasks() for multi-task merges (was only handled in merge_task() for single-task merges) - Add warning logging when worktree file doesn't exist during DIRECT_COPY in both merge_task() and merge_tasks() Co-Authored-By: Claude Opus 4.5 <[email protected]> * fix(merge): remove unnecessary f-string prefixes Co-Authored-By: Claude Opus 4.5 <[email protected]> * fix(merge): properly fail DIRECT_COPY when worktree file missing - Extract _read_worktree_file_for_direct_copy() helper to DRY up logic - Set decision to FAILED when worktree file not found (was silent success) - Add warning when worktree_path is None in merge_tasks - Use `is not None` check for merged_content to allow empty files - Fix has_modifications for new files with empty hash_before - Add debug_error() to merge_tasks exception handling for consistency Co-Authored-By: Claude Opus 4.5 <[email protected]> * style(merge): fix ruff formatting for long line Co-Authored-By: Claude Opus 4.5 <[email protected]> --------- Co-authored-by: Claude Opus 4.5 <[email protected]> * fix(terminal): detect Claude exit and reset label when user closes Claude (#990) * fix(terminal): detect Claude exit and reset label when user closes Claude Previously, the "Claude" label on terminals would persist even after the user closed Claude (via /exit, Ctrl+D, etc.) because the system only reset isClaudeMode when the entire terminal process exited. This change adds robust Claude exit detection by: - Adding shell prompt patterns to detect when Claude exits and returns to shell (output-parser.ts) - Adding new IPC channel TERMINAL_CLAUDE_EXIT for exit notifications - Adding handleClaudeExit() to reset terminal state in main process - Adding onClaudeExit callback in terminal event handler - Adding onTerminalClaudeExit listener in preload API - Handling exit event in renderer to update terminal store Now when a user closes Claude within a terminal, the label is removed immediately while the terminal continues running. Co-Authored-By: Claude Opus 4.5 <[email protected]> * fix(terminal): add line-start anchors to exit detection regex patterns Address PR review findings: - Add ^ anchors to CLAUDE_EXIT_PATTERNS to prevent false positive exit detection when Claude outputs paths, array access, or Unicode arrows - Add comprehensive unit tests for detectClaudeExit and related functions - Remove duplicate debugLog call in handleClaudeExit (keep console.warn) Co-Authored-By: Claude Opus 4.5 <[email protected]> * fix(terminal): prevent false exit detection for emails and race condition - Update user@host regex to require path indicator after colon, preventing emails like [email protected]: from triggering exit detection - Add test cases for emails at line start to ensure they don't match - Add guard in onTerminalClaudeExit to prevent setting status to 'running' if terminal has already exited (fixes potential race condition) Co-Authored-By: Claude Opus 4.5 <[email protected]> --------- Co-authored-by: Claude Opus 4.5 <[email protected]> * fix(app-update): persist downloaded update state for Install button visibility (#992) * fix(app-update): persist downloaded update state for Install button visibility When updates auto-download in background, users miss the update-downloaded event if not on Settings page. This causes "Install and Restart" button to never appear. Changes: - Add downloadedUpdateInfo state in app-updater.ts to persist downloaded info - Add APP_UPDATE_GET_DOWNLOADED IPC handler to query downloaded state - Add getDownloadedAppUpdate API method in preload - Update AdvancedSettings to check for already-downloaded updates on mount Now when user opens Settings after background download, the component queries persisted state and shows "Install and Restart" correctly. Co-Authored-By: Claude Opus 4.5 <[email protected]> * fix(app-update): resolve race condition and type safety issues - Fix race condition where checkForAppUpdates() could overwrite downloaded update info with null, causing 'Unknown' version display - Add proper type guard for releaseNotes (can be string | array | null) instead of unsafe type assertion Co-Authored-By: Claude Opus 4.5 <[email protected]> * fix(app-update): clear downloaded update state on channel change and add useEffect cleanup - Clear downloadedUpdateInfo when update channel changes to prevent showing Install button for updates from a different channel (e.g., beta update showing after switching to stable channel) - Add isCancelled flag to useEffect async operations in AdvancedSettings to prevent React state updates on unmounted components Addresses CodeRabbit review findings. Co-Authored-By: Claude Opus 4.5 <[email protected]> --------- Co-authored-by: Claude Opus 4.5 <[email protected]> * fix(backend): add Sentry integration and fix broken pipe errors (#991) * fix(backend): add Sentry integration and fix broken pipe errors - Add sentry-sdk to Python backend for error tracking - Create safe_print() utility to handle BrokenPipeError gracefully - Initialize Sentry in CLI, GitHub runner, and spec runner entry points - Use same SENTRY_DSN environment variable as Electron frontend - Apply privacy path masking (usernames removed from stack traces) Fixes "Review Failed: [Errno 32] Broken pipe" error in PR review Co-Authored-By: Claude Opus 4.5 <[email protected]> * fix(backend): address PR review findings for Sentry integration - Fix ruff linting errors (unused imports, import sorting) - Add path masking to set_context() and set_tag() for privacy - Add defensive path masking to capture_exception() kwargs - Add debug logging for bare except clauses in sentry.py - Add top-level error handler in cli/main.py with Sentry capture - Add error handling with Sentry capture in spec_runner.py - Move safe_print to core/io_utils.py for broader reuse - Migrate GitLab runner files to use safe_print() - Add fallback import pattern in sdk_utils.py Co-Authored-By: Claude Opus 4.5 <[email protected]> * style: apply ruff formatting Co-Authored-By: Claude Opus 4.5 <[email protected]> * fix(backend): address CodeRabbit review findings for Sentry and io_utils - Add path masking to capture_message() kwargs for privacy consistency - Add recursion depth limit (50) to _mask_object_paths() to prevent stack overflow - Add WSL path masking support (/mnt/[a-z]/Users/...) - Add consistent ImportError debug logging across Sentry wrapper functions - Add ValueError handling in safe_print() for closed stdout scenarios - Improve reset_pipe_state() documentation with usage warnings Co-Authored-By: Claude Opus 4.5 <[email protected]> --------- Co-authored-by: Claude Opus 4.5 <[email protected]> * fix: improve Claude CLI detection and add installation selector (#1004) * fix: improve Claude CLI detection and add installation selector This PR addresses the "Claude Code not found" error when starting tasks by improving CLI path detection across all platforms. Backend changes: - Add cross-platform `find_claude_cli()` function in `client.py` that checks: - CLAUDE_CLI_PATH environment variable for user override - System PATH via shutil.which() - Homebrew paths on macOS - NVM paths for Node.js version manager installations - Platform-specific standard locations (Windows: AppData, Program Files; Unix: .local/bin) - Pass detected `cli_path` to ClaudeAgentOptions in both `create_client()` and `create_simple_client()` - Improve Windows .cmd/.bat file execution using proper cmd.exe flags (/d, /s, /c) and correct quoting for paths with spaces Frontend changes: - Add IPC handlers for scanning all Claude CLI installations and switching active path - Update ClaudeCodeStatusBadge to show current CLI path and allow selection when multiple installations are detected - Add `writeSettingsFile()` to settings-utils for persisting CLI path selection - Add translation keys for new UI elements (English and French) Closes #1001 * fix: address PR review findings for Claude CLI detection Addresses all 8 findings from Auto Claude PR Review: Security improvements: - Add path sanitization (_is_secure_path) to backend CLI validation to prevent command injection via malicious paths - Add isSecurePath validation in frontend IPC handler before CLI execution - Normalize paths with path.resolve() before execution Architecture improvements: - Refactor scanClaudeInstallations to use getClaudeDetectionPaths() from cli-tool-manager.ts as single source of truth (addresses code duplication) - Add cross-reference comments between backend _get_claude_detection_paths() and frontend getClaudeDetectionPaths() to keep them in sync Bug fixes: - Fix path display truncation to use regex /[/\\]/ for cross-platform compatibility (Windows uses backslashes) - Add null check for version in UI rendering (shows "version unknown" instead of "vnull") - Use DEFAULT_APP_SETTINGS merge pattern for settings persistence Debugging improvements: - Add error logging in validateClaudeCliAsync catch block for better debugging of CLI detection issues Translation additions: - Add "versionUnknown" key to English and French navigation.json * ci(release): move VirusTotal scan to separate post-release workflow (#980) * ci(release): move VirusTotal scan to separate post-release workflow VirusTotal scans were blocking release creation, taking 5+ minutes per file. This change moves the scan to a separate workflow that triggers after the release is published, allowing releases to be available immediately. - Create virustotal-scan.yml workflow triggered on release:published - Remove blocking VirusTotal step from release.yml - Scan results are appended to release notes after completion - Add manual trigger option for rescanning old releases * fix(ci): address PR review issues in VirusTotal scan workflow - Add error checking on gh release view to prevent wiping release notes - Replace || true with proper error handling to distinguish "no assets" from real errors - Use file-based approach for release notes to avoid shell expansion issues - Use env var pattern consistently for secret handling - Remove placeholder text before appending VT results - Document 32MB threshold with named constant - Add HTTP status code validation on all curl requests Co-Authored-By: Claude Opus 4.5 <[email protected]> * fix(ci): add concurrency control and remove dead code in VirusTotal workflow - Add concurrency group to prevent TOCTOU race condition when multiple workflow_dispatch runs target the same release tag - Remove unused analysis_failed variable declaration Co-Authored-By: Claude Opus 4.5 <[email protected]> * fix(ci): improve error handling in VirusTotal workflow - Fail workflow when download errors occur but scannable assets exist - Add explicit timeout handling for analysis polling loop - Use portable sed approach (works on both GNU and BSD sed) Co-Authored-By: Claude Opus 4.5 <[email protected]> --------- Co-authored-by: Claude Opus 4.5 <[email protected]> * fix(ui): display actual base branch name instead of hardcoded main (#969) * fix(ui): display actual base branch name instead of hardcoded "main" The merge conflict UI was showing "Main branch has X new commits" regardless of the actual base branch. Now it correctly displays the dynamic branch name (e.g., "develop branch has 40 new commits") using the baseBranch value from gitConflicts. * docs: update README download links to v2.7.3 (#976) - Update all stable download links from 2.7.2 to 2.7.3 - Add Flatpak download link (new in 2.7.3) * fix(i18n): add translation keys for branch divergence messages - Add merge section to taskReview.json with pluralized translations - Update WorkspaceStatus.tsx to use i18n for branch behind message - Update MergePreviewSummary.tsx to use i18n for branch divergence text - Add French translations for all new keys Co-Authored-By: Claude Opus 4.5 <[email protected]> * fix(i18n): add missing translation keys for branch behind details - Add branchHasNewCommitsSinceBuild for build started message - Add filesNeedAIMergeDueToRenames for path-mapped files - Add fileRenamesDetected for rename detection message - Add filesRenamedOrMoved for generic rename/move message - Update WorkspaceStatus.tsx to use all new i18n keys Co-Authored-By: Claude Opus 4.5 <[email protected]> * fix(i18n): correct pluralization for rename count in AI merge message The filesNeedAIMergeDueToRenames translation has two values that need independent pluralization (fileCount and renameCount). Since i18next only supports one count parameter, added separate translation keys for singular/plural renames and select the correct key based on renameCount value. Co-Authored-By: Claude Opus 4.5 <[email protected]> * fix(i18n): use translation keys for merge button labels with dynamic branch Replace hardcoded 'Stage to Main' and 'Merge to Main' button labels with i18n translation keys that interpolate the actual target branch name. Also adds translations for loading states (Resolving, Staging, Merging). Co-Authored-By: Claude Opus 4.5 <[email protected]> --------- Co-authored-by: Claude Opus 4.5 <[email protected]> * fix(github-prs): prevent preloading of PRs currently under review (#1006) - Updated logic to skip PRs that are currently being reviewed when determining which PRs need preloading. - Enhanced condition to only fetch existing review data from disk if no review is in progress, ensuring that ongoing reviews are not overwritten by stale data. * chore: bump version to 2.7.4 * hotfix/sentry-backend-build * fix(github): resolve circular import issues in context_gatherer and services (#1026) - Updated import statements in context_gatherer.py to import safe_print from core.io_utils to avoid circular dependencies with the services package. - Introduced lazy imports in services/__init__.py to prevent circular import issues, detailing the import chain in comments for clarity. - Added a lazy import handler to load classes on first access, improving module loading efficiency. * feat(sentry): embed Sentry DSN at build time for packaged apps (#1025) * feat(sentry): integrate Sentry configuration into Electron build - Added build-time constants for Sentry DSN and sampling rates in electron.vite.config.ts. - Enhanced environment variable handling in env-utils.ts to include Sentry settings for subprocesses. - Implemented getSentryEnvForSubprocess function in sentry.ts to provide Sentry environment variables for Python backends. - Updated Sentry-related functions to prioritize build-time constants over runtime environment variables for improved reliability. This integration ensures that Sentry is properly configured for both local development and CI environments. * fix(sentry): add typeof guards for build-time constants in tests The __SENTRY_*__ constants are only defined when Vite's define plugin runs during build. In test environments (vitest), these constants are undefined and cause ReferenceError. Added typeof guards to safely handle both cases. Co-Authored-By: Claude Opus 4.5 <[email protected]> --------- Co-authored-by: Claude Opus 4.5 <[email protected]> * Fix Duplicate Kanban Task Creation on Rapid Button Clicks (#1021) * auto-claude: subtask-1-1 - Add convertingIdeas state and guard logic to useIdeation hook * auto-claude: subtask-1-2 - Update IdeaDetailPanel to accept isConverting prop * auto-claude: subtask-2-1 - Add idempotency check for linked_task_id in task-c * auto-claude: subtask-3-1 - Manual testing: Verify rapid clicking creates only one task - Fixed missing convertingIdeas prop connection in Ideation.tsx - Added convertingIdeas to destructured hook values - Added isConverting prop to IdeaDetailPanel component - Created detailed manual-test-report.md with code review and E2E testing instructions - All code implementation verified via TypeScript checks (no errors) - Multi-layer protection confirmed: UI disabled, guard check, backend idempotency - Manual E2E testing required for final verification Co-Authored-By: Claude Sonnet 4.5 <[email protected]> * fix: address PR review findings for duplicate task prevention - Fix TOCTOU race condition by moving idempotency check inside lock - Fix React state closure by using ref for synchronous tracking - Add i18n translations for ideation UI (EN + FR) - Add error handling with toast notifications for conversion failures Co-Authored-By: Claude Opus 4.5 <[email protected]> --------- Co-authored-by: Claude Sonnet 4.5 <[email protected]> * feat(terminal): add YOLO mode to invoke Claude with --dangerously-skip-permissions (#1016) * feat(terminal): add YOLO mode to invoke Claude with --dangerously-skip-permissions Add a toggle in Developer Tools settings that enables "YOLO Mode" which starts Claude with the --dangerously-skip-permissions flag, bypassing all safety prompts. Changes: - Add dangerouslySkipPermissions setting to AppSettings interface - Add translation keys for YOLO mode (en/fr) - Modify claude-integration-handler to accept and append extra flags - Update terminal-manager and terminal-handlers to read and forward the setting - Add Switch toggle with warning styling in DevToolsSettings UI The toggle includes visual warnings (amber colors, AlertTriangle icon) to clearly indicate this is a dangerous option that bypasses Claude's permission system. Co-Authored-By: Claude Opus 4.5 <[email protected]> * fix(terminal): address PR review issues for YOLO mode implementation - Add async readSettingsFileAsync to avoid blocking main process during settings read - Extract YOLO_MODE_FLAG constant to eliminate duplicate flag strings - Store dangerouslySkipPermissions on terminal object to persist YOLO mode across profile switches - Update switchClaudeProfile callback to pass stored YOLO mode setting These fixes address: - LOW: Synchronous file I/O in IPC handler - LOW: Flag string duplicated in invokeClaude and invokeClaudeAsync - MEDIUM: YOLO mode not persisting when switching Claude profiles Co-Authored-By: Claude Opus 4.5 <[email protected]> --------- Co-authored-by: Claude Opus 4.5 <[email protected]> * Make worktree isolation prominent in UI (#1020) * auto-claude: subtask-1-1 - Add i18n translation keys for worktree notice banner and merge tooltip - Added wizard.worktreeNotice.title and wizard.worktreeNotice.description for task creation banner - Added review.mergeTooltip for merge button explanation - Translations added to both en/tasks.json and fr/tasks.json * auto-claude: subtask-1-2 - Add visible info banner to TaskCreationWizard expl * auto-claude: subtask-1-3 - Add tooltip to 'Merge with AI' button in WorkspaceStatus - Import Tooltip components from ui/tooltip - Wrap merge button with Tooltip, TooltipTrigger, TooltipContent - Add contextual tooltip text explaining merge operation: * With AI: explains worktree merge, removal, and AI conflict resolution * Without AI: explains worktree merge and removal - Follows Radix UI tooltip pattern from reference file * fix: use i18n key for merge button tooltip in WorkspaceStatus * fix: clarify merge tooltip - worktree removal is optional (qa-requested) Fixes misleading tooltip text that implied worktree is automatically removed during merge. In reality, after merge, users are shown a dialog where they can choose to keep or remove the worktree. Updated tooltip to reflect this flow. Changes: - Updated en/tasks.json: Changed tooltip to clarify worktree removal is optional - Updated fr/tasks.json: Updated French translation to match QA Feedback: "Its currently saying on the tooltip that it will 'remove the worktree' Please validate if this is the actual logic. As per my understanding, there will be an extra button afterwards that will make sure that the user has access to the work tree if they want to revert anything. The user has to manually accept to remove the work tree." Co-Authored-By: Claude Sonnet 4.5 <[email protected]> * fix: use theme-aware colors for worktree info banner Replace hardcoded blue colors with semantic theme classes to support dark mode properly. Uses the same pattern as other info banners in the codebase (bg-info/10, border-info/30, text-info). Co-Authored-By: Claude Opus 4.5 <[email protected]> --------- Co-authored-by: Claude Sonnet 4.5 <[email protected]> * fix(terminal): improve worktree name input UX (#1012) * fix(terminal): improve worktree name input to not strip trailing characters while typing - Allow trailing hyphens/underscores during input (only trim on submit) - Add preview name that shows the final sanitized value for branch preview - Remove invalid characters instead of replacing with hyphens - Collapse consecutive underscores in addition to hyphens - Final sanitization happens on submit to match backend WORKTREE_NAME_REGEX Co-Authored-By: Claude Opus 4.5 <[email protected]> * fix(terminal): address PR review findings for worktree name validation - Fix submit button disabled check to use sanitized name instead of raw input - Simplify trailing trim logic (apply once after all transformations) - Apply lowercase in handleNameChange to reduce input/preview gap - Internationalize 'name' fallback using existing translation key Co-Authored-By: Claude Opus 4.5 <[email protected]> * fix(terminal): improve header responsiveness for multiple terminals - Hide text labels (Claude, Open in IDE) when ≥4 terminals, show icon only - Add dynamic max-width to worktree name badge with truncation - Add tooltips to all icon-only elements for accessibility - Maintain full functionality while reducing header width requirements Co-Authored-By: Claude Opus 4.5 <[email protected]> --------- Co-authored-by: Claude Opus 4.5 <[email protected]> Co-authored-by: Test User <[email protected]> * fix(terminal): enhance terminal recreation logic with retry mechanism (#1013) * fix(terminal): enhance terminal recreation logic with retry mechanism - Introduced a maximum retry limit and delay for terminal recreation when dimensions are not ready. - Added cleanup for retry timers on component unmount to prevent memory leaks. - Improved error handling to report failures after exceeding retry attempts, ensuring better user feedback during terminal setup. * fix(terminal): address PR review feedback for retry mechanism - Fix race condition: clear pending retry timer at START of effect to prevent multiple timers when dependencies change mid-retry - Fix isCreatingRef: keep it true during retry window to prevent duplicate creation attempts from concurrent effect runs - Extract duplicated retry logic into scheduleRetryOrFail helper (consolidated 5 duplicate instances into 1 reusable function) - Add handleSuccess/handleError helpers to reduce code duplication - Reduce file from 295 to 237 lines (~20% reduction) Addresses review feedback from CodeRabbit, Gemini, and Auto Claude. Co-Authored-By: Claude Opus 4.5 <[email protected]> --------- Co-authored-by: Test User <[email protected]> Co-authored-by: Claude Opus 4.5 <[email protected]> * feat(terminal): add task worktrees section and remove terminal limit (#1033) * feat(terminal): add task worktrees section and remove terminal limit - Remove 12 terminal worktree limit (now unlimited) - Add "Task Worktrees" section in worktree dropdown below terminal worktrees - Task worktrees (created by kanban) now accessible for manual work - Update translations for new section labels (EN + FR) Co-Authored-By: Claude Opus 4.5 <[email protected]> * fix(terminal): address PR review feedback - Clear taskWorktrees state when project is null or changes - Parallelize API calls with Promise.all for better performance - Use consistent path-based filtering for both worktree types - Add clarifying comment for createdAt timestamp Co-Authored-By: Claude Opus 4.5 <[email protected]> --------- Co-authored-by: Claude Opus 4.5 <[email protected]> Co-authored-by: Test User <[email protected]> * Add file/screenshot upload to QA feedback interface (#1018) * auto-claude: subtask-1-1 - Add feedbackImages state and handlers to useTaskDetail - Add feedbackImages state as ImageAttachment[] for storing feedback images - Add setFeedbackImages setter for direct state updates - Add addFeedbackImage handler for adding a single image - Add addFeedbackImages handler for adding multiple images at once - Add removeFeedbackImage handler for removing an image by ID - Add clearFeedbackImages handler for clearing all images - Import ImageAttachment type from shared/types 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]> * auto-claude: subtask-1-2 - Update IPC interface to support images in submitReview - Add ImageAttachment import from ./task types - Update submitReview signature to include optional images parameter 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]> * auto-claude: subtask-1-3 - Update submitReview function in task-store to accept and pass images * auto-claude: subtask-2-1 - Add paste/drop handlers and image thumbnail displa - Add paste event handler for screenshot/image clipboard support - Add drag-over and drag-leave handlers for visual feedback during drag - Add drop handler for image file drops - Add image thumbnail display (64x64) with remove button on hover - Import image utilities from ImageUpload.tsx (generateImageId, blobToBase64, etc.) - Add i18n support for all new UI text - Make new props optional for backward compatibility during incremental rollout - Allow submission with either text feedback or images (not both required) - Add visual drag feedback with border/background color change * auto-claude: subtask-2-2 - Update TaskReview to pass image props to QAFeedbackSection * auto-claude: subtask-2-3 - Update TaskDetailModal to manage image state and pass to TaskReview - Pass feedbackImages and setFeedbackImages from useTaskDetail hook to TaskReview - Update handleReject to include images in submitReview call - Allow submission with images only (no text required) - Clear images after successful submission * auto-claude: subtask-3-1 - Add English translations for feedback image UI * auto-claude: subtask-3-2 - Add French translations for feedback image UI * fix(security): sanitize image filename to prevent path traversal - Use path.basename() to strip directory components from filenames - Validate sanitized filename is not empty, '.', or '..' - Add defense-in-depth check verifying resolved path stays within target directory - Fix base64 data URL regex to handle complex MIME types (e.g., svg+xml) Co-Authored-By: Claude Opus 4.5 <[email protected]> * fix: add MIME type validation and fix SVG file extension - Add server-side MIME type validation for image uploads (defense in depth) - Fix SVG file extension: map 'image/svg+xml' to '.svg' instead of '.svg+xml' - Add MIME-to-extension mapping for all allowed image types Co-Authored-By: Claude Opus 4.5 <[email protected]> * fix: require mimeType and apply SVG extension fix to drop handler - Change MIME validation to reject missing mimeType (prevents bypass) - Add 'image/jpg' to server-side allowlist for consistency - Apply mimeToExtension mapping to drop handler (was only in paste handler) Co-Authored-By: Claude Opus 4.5 <[email protected]> --------- Co-authored-by: Claude Opus 4.5 <[email protected]> Co-authored-by: Test User <[email protected]> * fix(auth): await profile manager initialization before auth check (#1010) * fix(auth): await profile manager initialization before auth check Fixes race condition where hasValidAuth() was called before the ClaudeProfileManager finished async initialization from disk. The getClaudeProfileManager() returns a singleton immediately with default profile data (no OAuth token). When hasValidAuth() runs before initialization completes, it returns false even when valid credentials exist. Changed all pre-flight auth checks to use await initializeClaudeProfileManager() which ensures initialization completes via promise caching. Signed-off-by: StillKnotKnown <[email protected]> * fix(auth): add error handling for profile manager initialization Prevents unhandled promise rejections when initializeClaudeProfileManager() throws due to filesystem errors (permissions, disk full, corrupt JSON). The ipcMain.on handler for TASK_START doesn't await promises, so unhandled rejections could crash the main process. Wrapped all await initializeClaudeProfileManager() calls in try-catch blocks. Found via automated code review. Signed-off-by: StillKnotKnown <[email protected]> * test: mock initializeClaudeProfileManager in subprocess tests The test mock was only mocking getClaudeProfileManager, but now we also use initializeClaudeProfileManager which wasn't mocked, causing test failures. Signed-off-by: StillKnotKnown <[email protected]> * fix(auth): add try-catch for initializeClaudeProfileManager in remaining handlers Addresses PR review feedback - TASK_UPDATE_STATUS and TASK_RECOVER_STUCK handlers were missing try-catch blocks for initializeClaudeProfileManager(), inconsistent with TASK_START handler. If initialization fails, users now get specific file permissions guidance instead of generic error messages. Signed-off-by: StillKnotKnown <[email protected]> * refactor(auth): extract profile manager initialization into helper Extract the repeated initializeClaudeProfileManager() + try/catch pattern into a helper function ensureProfileManagerInitialized() that returns a discriminated union for type-safe error handling. This reduces code duplication across TASK_START, TASK_UPDATE_STATUS, and TASK_RECOVER_STUCK handlers while preserving context-specific error handling behavior. The helper returns: - { success: true, profileManager } on success - { success: false, error } on failure Signed-off-by: StillKnotKnown <[email protected]> * fix(auth): improve error details and allow retry after transient failures Two improvements to profile manager initialization: 1. Include actual error details in failure response for better debugging. Previously, only a generic message was returned to users, making it hard to diagnose the root cause. Now the error message is appended. 2. Reset cached promise on failure to allow retries after transient errors. Previously, if initialize() failed (e.g., EACCES, ENOSPC), the rejected promise was cached forever, requiring app restart to recover. Now the cached promise is reset on failure, allowing subsequent calls to retry. Signed-off-by: StillKnotKnown <[email protected]> --------- Signed-off-by: StillKnotKnown <[email protected]> Co-authored-by: StillKnotKnown <[email protected]> Co-authored-by: Andy <[email protected]> * fix(frontend): validate Windows claude.cmd reliably in GUI (#1023) * fix: use absolute cmd.exe for Claude CLI validation * fix: make cmd.exe validation type-safe for tests * fix: satisfy frontend typecheck for cli tool tests Signed-off-by: Umaru <[email protected]> * test: mock windows-paths exports for isSecurePath Signed-off-by: Umaru <[email protected]> * test: make cli env tests platform-aware Signed-off-by: Umaru <[email protected]> * test: cover isSecurePath guard in claude detection Signed-off-by: Umaru <[email protected]> * test: align env-utils mocks with shouldUseShell Signed-off-by: Umaru <[email protected]> * test: assert isSecurePath for cmd path * fix(frontend): handle quoted claude.cmd paths in validation --------- Signed-off-by: Umaru <[email protected]> Co-authored-by: Andy <[email protected]> * 2.7.4 release * changelog 2.7.4 --------- Signed-off-by: Black Circle Sentinel <[email protected]> Signed-off-by: StillKnotKnown <[email protected]> Signed-off-by: Umaru <[email protected]> Co-authored-by: StillKnotKnown <[email protected]> Co-authored-by: StillKnotKnown <[email protected]> Co-authored-by: Claude Opus 4.5 <[email protected]> Co-authored-by: Michael Ludlow <[email protected]> Co-authored-by: Test User <[email protected]> Co-authored-by: Umaru <[email protected]>
Add file and screenshot paste/upload functionality to the QAFeedbackSection component, which is the feedback interface users see during human review of completed tasks. This feature enables users to provide visual context (screenshots, images) when requesting changes on completed tasks. The implementation will reuse the existing paste/drop capabilities already implemented in TaskCreationWizard ("Create New Task" modal), ensuring consistency in behavior and code quality.
Summary by CodeRabbit
New Features
Bug Fixes
✏️ Tip: You can customize this high-level summary in your review settings.