feat: Add screenshot capture to task creation modal #274
Conversation
- Add screenshot capture button in reference image section - Implement Electron IPC handlers for screen/window capture using desktopCapturer - Create ScreenshotCapture modal component with source selection UI - Integrate captured screenshots into image attachments - Support capturing entire screens or specific windows - Auto-generate timestamped filenames for captured screenshots 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Sonnet 4.5 <[email protected]>
- Add collapsible "Reference Images (optional)" section matching edit modal - Move screenshot capture button into Reference Images section - Auto-expand section when images are added via paste/drop/capture - Use ImageUpload component for consistent UI - Show image count badge when images are present 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Sonnet 4.5 <[email protected]>
🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Sonnet 4.5 <[email protected]>
📝 WalkthroughWalkthroughAdds end-to-end screenshot support: main-process IPC handlers, preload ScreenshotAPI, renderer ScreenshotCapture component, IPC channel constants, and integration into TaskCreationWizard including image attachment and thumbnail handling. Changes
Sequence DiagramsequenceDiagram
participant User
participant UI as ScreenshotCapture (Renderer)
participant Preload as Preload API (ScreenshotAPI)
participant Main as Main Process (IPC handlers)
participant Native as desktopCapturer (Native)
User->>UI: open modal
rect rgb(240,248,255)
UI->>Preload: getSources()
Preload->>Main: ipcRenderer.invoke(SCREENSHOT_GET_SOURCES)
Main->>Native: desktopCapturer.getSources(thumbnail small)
Native-->>Main: sources[]
Main-->>Preload: IPCResult<ScreenshotSource[]>
Preload-->>UI: sources (thumbnails)
end
User->>UI: select source, click Capture
rect rgb(255,240,245)
UI->>Preload: captureScreen(sourceId)
Preload->>Main: ipcRenderer.invoke(SCREENSHOT_CAPTURE, sourceId)
Main->>Native: desktopCapturer.getSources(thumbnail large)
Native-->>Main: high-res image
Main-->>Preload: IPCResult<dataURL>
Preload-->>UI: dataURL
end
UI->>User: close modal, call onCapture(dataURL -> TaskCreationWizard)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro 📒 Files selected for processing (2)
🧰 Additional context used📓 Path-based instructions (2)apps/frontend/src/**/*.{ts,tsx}📄 CodeRabbit inference engine (CLAUDE.md)
Files:
apps/frontend/**/*.{ts,tsx}⚙️ CodeRabbit configuration file
Files:
🧬 Code graph analysis (1)apps/frontend/src/preload/api/index.ts (1)
🔇 Additional comments (2)
Comment |
![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
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (7)
apps/frontend/src/main/ipc-handlers/index.tsapps/frontend/src/main/ipc-handlers/screenshot-handlers.tsapps/frontend/src/preload/api/index.tsapps/frontend/src/preload/api/screenshot-api.tsapps/frontend/src/renderer/components/ScreenshotCapture.tsxapps/frontend/src/renderer/components/TaskCreationWizard.tsxapps/frontend/src/shared/constants/ipc.ts
🧰 Additional context used
📓 Path-based instructions (1)
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/main/ipc-handlers/screenshot-handlers.tsapps/frontend/src/main/ipc-handlers/index.tsapps/frontend/src/preload/api/index.tsapps/frontend/src/renderer/components/ScreenshotCapture.tsxapps/frontend/src/preload/api/screenshot-api.tsapps/frontend/src/shared/constants/ipc.tsapps/frontend/src/renderer/components/TaskCreationWizard.tsx
🧬 Code graph analysis (3)
apps/frontend/src/main/ipc-handlers/screenshot-handlers.ts (2)
apps/frontend/src/main/ipc-handlers/index.ts (1)
registerScreenshotHandlers(127-127)apps/frontend/src/preload/api/screenshot-api.ts (1)
ScreenshotSource(5-9)
apps/frontend/src/main/ipc-handlers/index.ts (1)
apps/frontend/src/main/ipc-handlers/screenshot-handlers.ts (1)
registerScreenshotHandlers(9-79)
apps/frontend/src/preload/api/index.ts (1)
apps/frontend/src/preload/api/screenshot-api.ts (2)
ScreenshotAPI(11-14)createScreenshotAPI(16-22)
🪛 Biome (2.1.2)
apps/frontend/src/renderer/components/ScreenshotCapture.tsx
[error] 176-181: Alternative text title element cannot be empty
For accessibility purposes, SVGs should have an alternative text, provided via title element. If the svg element has role="img", you should add the aria-label or aria-labelledby attribute.
(lint/a11y/noSvgWithoutTitle)
[error] 140-150: Provide an explicit type prop for the button element.
The default type of a button is submit, which causes the submission of a form when placed inside a form element. This is likely not the behaviour that you want inside a React application.
Allowed button types are: submit, button or reset
(lint/a11y/useButtonType)
🔇 Additional comments (10)
apps/frontend/src/shared/constants/ipc.ts (1)
302-306: LGTM!The new IPC channel constants follow the established naming convention and are properly added to the
as constobject.apps/frontend/src/main/ipc-handlers/screenshot-handlers.ts (1)
13-40: LGTM!The
getSourceshandler correctly fetches available screens and windows with appropriate thumbnail sizing for preview purposes, and follows the established IPCResult pattern for error handling.apps/frontend/src/renderer/components/ScreenshotCapture.tsx (1)
96-235: LGTM!The component provides good UX with loading states, error handling, source previews with selection indicators, and appropriate button states. The Dialog integration follows the existing UI patterns.
apps/frontend/src/preload/api/index.ts (1)
10-34: LGTM!The ScreenshotAPI is cleanly integrated into the ElectronAPI surface following the established composition pattern used by other APIs.
apps/frontend/src/main/ipc-handlers/index.ts (1)
102-103: LGTM!The screenshot handlers are properly registered following the established pattern and exported for potential custom usage.
apps/frontend/src/preload/api/screenshot-api.ts (1)
1-22: LGTM!The ScreenshotAPI is well-typed with clear interfaces and follows the established preload API patterns. The IPCResult wrapper ensures consistent error handling.
apps/frontend/src/renderer/components/TaskCreationWizard.tsx (4)
611-655: LGTM!The
handleScreenshotCapturecallback is well-implemented with proper limit checking, thumbnail generation, MIME type extraction, and size estimation. The base64-to-bytes conversion formula(base64Length * 3) / 4is correct.
948-973: LGTM!The Reference Images section is well-integrated with the ImageUpload component and provides a clear "Capture" button to open the screenshot modal. The UI follows the established collapsible section pattern.
1229-1235: LGTM!The ScreenshotCapture modal is properly integrated with controlled state and the capture callback is correctly wired.
922-946: Add explicittype="button"to the Reference Images toggle.Similar to other toggle buttons in this file (lines 976, 1119), this button should have an explicit
type="button"to ensure consistent behavior and prevent accidental form submission.🔎 Proposed fix
{/* Reference Images Toggle */} <button + type="button" onClick={() => setShowImages(!showImages)} className={cn(Likely an incorrect or invalid review comment.
| try { | ||
| // Get the source with full resolution | ||
| const sources = await desktopCapturer.getSources({ | ||
| types: ['screen', 'window'], | ||
| thumbnailSize: { width: 1920 * 2, height: 1080 * 2 } // High resolution for retina displays | ||
| }); | ||
|
|
||
| const source = sources.find(s => s.id === sourceId); | ||
| if (!source) { | ||
| return { | ||
| success: false, | ||
| error: 'Screenshot source not found' | ||
| }; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Electron desktopCapturer getSources thumbnailSize documentation
💡 Result:
thumbnailSize is an option passed to desktopCapturer.getSources that sets the scaled size for each source thumbnail. Default is 150×150; you can set width or height to 0 to skip thumbnails (saves processing). Note the returned DesktopCapturerSource.thumbnail may still vary depending on screen/window scale. [1] (electronjs.org)
Sources:
[1] Electron desktopCapturer.getSources docs. (electronjs.org)
[2] DesktopCapturerSource.thumbnail size note. (zeke.github.io)
Citations:
- 1: https://www.electronjs.org/docs/latest/api/desktop-capturer?utm_source=openai
- 2: https://www.electronjs.org/docs/latest/api/desktop-capturer?utm_source=openai
- 3: https://zeke.github.io/electron.atom.io/docs/api/structures/desktop-capturer-source/?utm_source=openai
🏁 Script executed:
# First, let's examine the actual file to understand the complete context
find . -name "screenshot-handlers.ts" -type fRepository: AndyMik90/Auto-Claude
Length of output: 125
🏁 Script executed:
# Read the full screenshot-handlers.ts file to understand the handlers
cat -n ./apps/frontend/src/main/ipc-handlers/screenshot-handlers.tsRepository: AndyMik90/Auto-Claude
Length of output: 2936
Current code uses thumbnails as the final screenshot output, which limits resolution regardless of thumbnailSize value.
The thumbnailSize parameter only controls the preview size of thumbnails—it doesn't enable full-resolution capture. The code retrieves source.thumbnail.toDataURL() as the final image, which is inherently limited to that thumbnail size. For true full-resolution screenshots, use getUserMedia() with the source ID to capture the actual display stream instead of the thumbnail.
The race condition between getSources() and capture() (window closes between selection and capture) is already handled gracefully by the "source not found" error.
🤖 Prompt for AI Agents
In apps/frontend/src/main/ipc-handlers/screenshot-handlers.ts around lines 48 to
61, the code uses source.thumbnail.toDataURL() which only returns a
thumbnail-sized image; to capture full-resolution screenshots replace the
thumbnail flow with obtaining a MediaStream via
navigator.mediaDevices.getUserMedia({ audio: false, video: { mandatory: {
chromeMediaSource: 'desktop', chromeMediaSourceId: sourceId } } }) (or the
Electron equivalent constraint), attach the stream to an offscreen video
element, wait for video metadata/play to ensure actual resolution, draw the
video frame to a canvas sized to videoWidth/videoHeight, call canvas.toDataURL()
to get the full-resolution image, then stop all MediaStreamTracks and release
the video; keep the existing "source not found" guard and proper try/catch
logging.
| useEffect(() => { | ||
| if (open) { | ||
| loadSources(); | ||
| } else { | ||
| // Reset state when closing | ||
| setSources([]); | ||
| setSelectedSource(null); | ||
| setError(null); | ||
| } | ||
| }, [open]); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider adding loadSources to useEffect dependency array or wrapping in useCallback.
The loadSources function is called inside the effect but not listed in the dependency array. While this works because loadSources only depends on stable setters, the linter would typically flag this. Consider wrapping loadSources in useCallback for explicitness.
🤖 Prompt for AI Agents
In apps/frontend/src/renderer/components/ScreenshotCapture.tsx around lines 32
to 41, the useEffect calls loadSources but doesn't include it in the dependency
array; either wrap loadSources in useCallback with its own stable dependencies
(or no deps if it only uses stable setters) and then add the memoized
loadSources to the useEffect dependency array, or add loadSources directly to
the dependency array if you prefer not to memoize; ensure the callback/deps are
consistent so the linter no longer flags the missing dependency.
| <button | ||
| key={source.id} | ||
| onClick={() => setSelectedSource(source.id)} | ||
| className={cn( | ||
| 'relative rounded-lg border-2 transition-all overflow-hidden group', | ||
| 'hover:border-primary/50 hover:shadow-md', | ||
| isSelected | ||
| ? 'border-primary shadow-lg ring-2 ring-primary/20' | ||
| : 'border-border' | ||
| )} | ||
| > |
There was a problem hiding this comment.
Add explicit type="button" to prevent form submission.
The source selection button lacks an explicit type attribute. While not inside a form currently, this is a defensive best practice and is flagged by static analysis.
🔎 Proposed fix
return (
<button
key={source.id}
+ type="button"
onClick={() => setSelectedSource(source.id)}
className={cn(📝 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.
| <button | |
| key={source.id} | |
| onClick={() => setSelectedSource(source.id)} | |
| className={cn( | |
| 'relative rounded-lg border-2 transition-all overflow-hidden group', | |
| 'hover:border-primary/50 hover:shadow-md', | |
| isSelected | |
| ? 'border-primary shadow-lg ring-2 ring-primary/20' | |
| : 'border-border' | |
| )} | |
| > | |
| <button | |
| key={source.id} | |
| type="button" | |
| onClick={() => setSelectedSource(source.id)} | |
| className={cn( | |
| 'relative rounded-lg border-2 transition-all overflow-hidden group', | |
| 'hover:border-primary/50 hover:shadow-md', | |
| isSelected | |
| ? 'border-primary shadow-lg ring-2 ring-primary/20' | |
| : 'border-border' | |
| )} | |
| > |
🧰 Tools
🪛 Biome (2.1.2)
[error] 140-150: Provide an explicit type prop for the button element.
The default type of a button is submit, which causes the submission of a form when placed inside a form element. This is likely not the behaviour that you want inside a React application.
Allowed button types are: submit, button or reset
(lint/a11y/useButtonType)
🤖 Prompt for AI Agents
In apps/frontend/src/renderer/components/ScreenshotCapture.tsx around lines 140
to 150, the source selection <button> is missing an explicit type attribute
which can cause it to act as a submit button when placed inside a form; update
the JSX to add type="button" to the element so it never triggers form
submission, preserving existing onClick behavior and styling.
| <svg | ||
| className="h-4 w-4 text-primary-foreground" | ||
| fill="none" | ||
| viewBox="0 0 24 24" | ||
| stroke="currentColor" | ||
| > | ||
| <path | ||
| strokeLinecap="round" | ||
| strokeLinejoin="round" | ||
| strokeWidth={2} | ||
| d="M5 13l4 4L19 7" | ||
| /> | ||
| </svg> |
There was a problem hiding this comment.
Add accessible title to SVG checkmark icon.
The SVG lacks alternative text, which is an accessibility issue flagged by static analysis. Screen readers won't be able to describe this visual indicator.
🔎 Proposed fix
{isSelected && (
<div className="absolute top-2 right-2 h-6 w-6 rounded-full bg-primary flex items-center justify-center shadow-lg">
<svg
className="h-4 w-4 text-primary-foreground"
fill="none"
viewBox="0 0 24 24"
stroke="currentColor"
+ role="img"
+ aria-label="Selected"
>
+ <title>Selected</title>
<path
strokeLinecap="round"
strokeLinejoin="round"
strokeWidth={2}
d="M5 13l4 4L19 7"
/>
</svg>
</div>
)}📝 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.
| <svg | |
| className="h-4 w-4 text-primary-foreground" | |
| fill="none" | |
| viewBox="0 0 24 24" | |
| stroke="currentColor" | |
| > | |
| <path | |
| strokeLinecap="round" | |
| strokeLinejoin="round" | |
| strokeWidth={2} | |
| d="M5 13l4 4L19 7" | |
| /> | |
| </svg> | |
| <svg | |
| className="h-4 w-4 text-primary-foreground" | |
| fill="none" | |
| viewBox="0 0 24 24" | |
| stroke="currentColor" | |
| role="img" | |
| aria-label="Selected" | |
| > | |
| <title>Selected</title> | |
| <path | |
| strokeLinecap="round" | |
| strokeLinejoin="round" | |
| strokeWidth={2} | |
| d="M5 13l4 4L19 7" | |
| /> | |
| </svg> |
🧰 Tools
🪛 Biome (2.1.2)
[error] 176-181: Alternative text title element cannot be empty
For accessibility purposes, SVGs should have an alternative text, provided via title element. If the svg element has role="img", you should add the aria-label or aria-labelledby attribute.
(lint/a11y/noSvgWithoutTitle)
🤖 Prompt for AI Agents
In apps/frontend/src/renderer/components/ScreenshotCapture.tsx around lines
176–188, the SVG checkmark lacks accessible text for screen readers; add an
accessible title element and associate it with the SVG (e.g., <title
id="some-unique-id">Checked</title> and aria-labelledby="that-id" plus
role="img") so assistive tech can describe the icon, or if the icon is purely
decorative set aria-hidden="true" instead — implement the appropriate change for
this icon's semantics and ensure the title id is unique.
AndyMik90
left a comment
AndyMik90
left a comment
There was a problem hiding this comment.
🤖 Auto Claude PR Review
Merge Verdict: 🔴 BLOCKED
Blocked by 6 critical issues
Risk Assessment
| Factor | Level | Notes |
|---|---|---|
| Complexity | High | Based on lines changed |
| Security Impact | Medium | Based on security findings |
| Scope Coherence | Good | Based on structural review |
🚨 Blocking Issues (Must Fix)
- Critical: VERIFICATION REQUIRED: File existence and IPC registration (apps/frontend/src/main/ipc-handlers/screenshot-handlers.ts:None)
- Critical: VERIFICATION REQUIRED: IPC channel constants (apps/frontend/src/shared/constants/ipc.ts:None)
- Critical: VERIFICATION REQUIRED: Preload API integration (apps/frontend/src/preload/api/screenshot-api.ts:None)
- Critical: VERIFICATION REQUIRED: Component existence (apps/frontend/src/renderer/components/ScreenshotCapture.tsx:None)
- Critical: VERIFICATION REQUIRED: Handler registration (apps/frontend/src/main/ipc-handlers/index.ts:None)
- Critical: VERIFICATION REQUIRED: API composition (apps/frontend/src/preload/api/index.ts:None)
Findings Summary
- Critical: 6 issue(s)
- High: 6 issue(s)
- Medium: 3 issue(s)
Generated by Auto Claude PR Review
Findings (12 selected of 15 total)
🔴 [CRITICAL] VERIFICATION REQUIRED: File existence and IPC registration
📁 apps/frontend/src/main/ipc-handlers/screenshot-handlers.ts:null
PR claims to add screenshot-handlers.ts but this file does not exist on current branch. Must verify: 1) File is properly created, 2) Handler is imported and registered in ipc-handlers/index.ts, 3) Uses registerScreenshotHandlers() function signature pattern.
Suggested fix:
Verify file exists and is registered in setupIpcHandlers() following pattern: import { registerScreenshotHandlers } from './screenshot-handlers'; then call registerScreenshotHandlers(getMainWindow);
🟠 [HIGH] Missing input validation for sourceId parameter
📁 apps/frontend/src/main/ipc-handlers/screenshot-handlers.ts:null
desktopCapturer.getSources() returns source IDs in format 'screen:N:N' or 'window:N:N'. The SCREENSHOT_CAPTURE handler must validate sourceId format before use to prevent injection or manipulation attacks.
Suggested fix:
Add validation: if (!/^(screen|window):\d+:\d+$/.test(sourceId)) { return { success: false, error: 'Invalid source ID format' }; }
🟠 [HIGH] No rate limiting on screenshot capture API
📁 apps/frontend/src/main/ipc-handlers/screenshot-handlers.ts:null
Without rate limiting, renderer could spam screenshot requests causing DoS or data exfiltration. Screen capture is a privileged operation that should be throttled.
Suggested fix:
Implement rate limiter: max 5 getSources requests/minute and max 3 capture requests/minute using in-memory counter with time window.
🟠 [HIGH] Potential XSS via unsanitized window titles
📁 apps/frontend/src/main/ipc-handlers/screenshot-handlers.ts:null
Window titles from desktopCapturer sources could contain malicious HTML/scripts. If these are rendered in the UI without sanitization, XSS is possible.
Suggested fix:
Sanitize all source.name values before sending to renderer: source.name = DOMPurify.sanitize(source.name, { ALLOWED_TAGS: [] })
🔴 [CRITICAL] VERIFICATION REQUIRED: IPC channel constants
📁 apps/frontend/src/shared/constants/ipc.ts:null
PR claims to add SCREENSHOT_GET_SOURCES and SCREENSHOT_CAPTURE channels but these do not exist in current ipc.ts. Must verify channels are added with correct naming convention.
Suggested fix:
Add to IPC_CHANNELS: SCREENSHOT_GET_SOURCES: 'screenshot:getSources', SCREENSHOT_CAPTURE: 'screenshot:capture'
🔴 [CRITICAL] VERIFICATION REQUIRED: Preload API integration
📁 apps/frontend/src/preload/api/screenshot-api.ts:null
PR claims to add screenshot-api.ts but file doesn't exist. Must verify: 1) File creates ScreenshotAPI interface, 2) Uses ipcRenderer.invoke pattern, 3) Is integrated into ElectronAPI in index.ts.
Suggested fix:
Verify file exists with createScreenshotAPI() factory, integrated into createElectronAPI() in preload/api/index.ts
🔴 [CRITICAL] VERIFICATION REQUIRED: Component existence
📁 apps/frontend/src/renderer/components/ScreenshotCapture.tsx:null
PR claims to add ScreenshotCapture.tsx (236 lines) but file doesn't exist on current branch. Must verify component is properly implemented with loading states, error handling, and cleanup.
Suggested fix:
Verify file exists and follows React component patterns from codebase.
🟠 [HIGH] Missing canvas cleanup causing memory leaks
📁 apps/frontend/src/renderer/components/ScreenshotCapture.tsx:null
Based on existing ImageUpload.tsx patterns, thumbnail generation creates canvas elements that are never cleaned up. Multiple screenshot captures could accumulate canvases in memory.
Suggested fix:
After canvas.toDataURL(), add cleanup: canvas.width = 0; canvas.height = 0;
🟠 [HIGH] Missing Image object cleanup
📁 apps/frontend/src/renderer/components/ScreenshotCapture.tsx:null
Image objects created for processing should have their event handlers and src cleared after use to prevent memory leaks.
Suggested fix:
After image processing, add: img.onload = null; img.onerror = null; img.src = '';
🟠 [HIGH] VERIFICATION REQUIRED: Integration with existing component
📁 apps/frontend/src/renderer/components/TaskCreationWizard.tsx:null
PR claims to add 'Reference Images (optional)' section with Capture button, but current TaskCreationWizard.tsx doesn't have this. Must verify integration follows existing patterns.
Suggested fix:
Verify TaskCreationWizard imports and renders ScreenshotCapture, handles captured images via existing ImageAttachment pattern.
🔴 [CRITICAL] VERIFICATION REQUIRED: Handler registration
📁 apps/frontend/src/main/ipc-handlers/index.ts:null
Current index.ts doesn't import or register screenshot handlers. PR must add: import and registerScreenshotHandlers() call in setupIpcHandlers().
Suggested fix:
Add import and registration following existing pattern in setupIpcHandlers().
🔴 [CRITICAL] VERIFICATION REQUIRED: API composition
📁 apps/frontend/src/preload/api/index.ts:null
Current index.ts doesn't include ScreenshotAPI. PR must integrate createScreenshotAPI() into createElectronAPI() composition.
Suggested fix:
Add to ElectronAPI interface and createElectronAPI() function following existing pattern.
This review was generated by Auto Claude.
Security improvements: - Add input validation for sourceId to prevent injection attacks - Sanitize window/screen names to prevent XSS vulnerabilities - Implement rate limiting (5 getSources/min, 10 captures/min) Performance improvements: - Fix memory leaks in thumbnail generation - Clean up canvas elements after use (set width/height to 0) - Clean up Image object event handlers and src after processing - Prevent canvas and Image accumulation in memory Addresses PR review feedback from Auto Claude reviewer. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Sonnet 4.5 <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/frontend/src/renderer/components/ImageUpload.tsx (1)
165-336: Replace hardcoded strings with translation keys.This component contains multiple user-facing strings that must be converted to use
useTranslation()per coding guidelines. All strings inapps/frontend/src/renderer/components/ImageUpload.tsxmust reference translation keys rather than hardcoded text.Hardcoded strings to replace:
- Lines 165, 171, 172: Error/validation messages and file limit notifications
- Lines 182, 188, 209: Error messages for file validation, size warnings, and processing failures
- Lines 331, 335–336: UI labels and helper text for the drop zone
Import
useTranslationfromreact-i18nextand use the formatt('namespace:section.key'). Create a new namespace for this component (e.g.,imageUpload) in bothapps/frontend/src/shared/i18n/locales/en/andapps/frontend/src/shared/i18n/locales/fr/with appropriate keys for all user-facing text.
♻️ Duplicate comments (1)
apps/frontend/src/main/ipc-handlers/screenshot-handlers.ts (1)
112-127: Thumbnail API still limits resolution; use getUserMedia for full-resolution capture.The code continues to use
source.thumbnail.toDataURL()which only returns a thumbnail-sized image regardless of thethumbnailSizeparameter. This was flagged in the previous review and remains unresolved.To capture true full-resolution screenshots, replace the thumbnail approach with
navigator.mediaDevices.getUserMedia()using the source ID to obtain the actual display stream.The previous review comment provides detailed implementation guidance for switching to the MediaStream-based approach.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
apps/frontend/src/main/ipc-handlers/screenshot-handlers.tsapps/frontend/src/renderer/components/ImageUpload.tsx
🧰 Additional context used
📓 Path-based instructions (2)
apps/frontend/src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
apps/frontend/src/**/*.{ts,tsx}: Always use translation keys withuseTranslation()for all user-facing text in React/TypeScript frontend components - use formatnamespace:section.key(e.g.,navigation:items.githubPRs)
Never use hardcoded strings in JSX/TSX files for user-facing text - always reference translation keys fromapps/frontend/src/shared/i18n/locales/
Files:
apps/frontend/src/renderer/components/ImageUpload.tsxapps/frontend/src/main/ipc-handlers/screenshot-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/ImageUpload.tsxapps/frontend/src/main/ipc-handlers/screenshot-handlers.ts
🔇 Additional comments (4)
apps/frontend/src/renderer/components/ImageUpload.tsx (1)
102-120: LGTM! Proper cleanup prevents memory leaks.The explicit cleanup of canvas dimensions and Image object handlers/src after thumbnail generation correctly prevents memory accumulation in long-running sessions.
apps/frontend/src/main/ipc-handlers/screenshot-handlers.ts (3)
15-42: LGTM! Rate limiting and sanitization strengthen security posture.The rate limiting implementation correctly tracks requests per handler within a sliding time window, and the sanitization function removes common XSS vectors from source names.
103-110: LGTM! Regex validation prevents injection attacks.The sourceId format validation using
/^(screen|window):\d+:\d+$/correctly guards against malformed or malicious input.
59-137: This is a main process IPC handler, not renderer code. The coding guideline foruseTranslation()applies to React/TypeScript components in the renderer process (apps/frontend/src/renderer/**or component files with.tsxextension). Main process IPC handlers cannot use React hooks and are correctly returning plain error strings—the renderer process will receive and localize these strings as needed. All other IPC handlers in this directory follow the same pattern.Likely an incorrect or invalid review comment.
Resolved merge conflict in src/preload/api/index.ts by including both ScreenshotAPI and GitHubAPI in the ElectronAPI interface.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
| mimeType, | ||
| size, | ||
| data: base64Data, | ||
| thumbnail |
There was a problem hiding this comment.
Missing null check on base64 data split result
The code splits dataUrl and accesses index [1] without checking if the split produced at least 2 elements. If the data URL is malformed and doesn't contain a comma, base64Data would be undefined, causing a crash when accessing .length on line 632, or storing undefined in the data field on line 639 which expects a string. While Electron's toDataURL() should normally produce properly formatted URLs, the code lacks defensive checks to handle edge cases gracefully.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| return name | ||
| .replace(/<[^>]*>/g, '') // Remove HTML tags | ||
| .replace(/javascript:/gi, '') // Remove javascript: protocol | ||
| .replace(/on\w+=/gi, '') // Remove event handlers like onclick= |
Check failure
Code scanning / CodeQL
Incomplete URL scheme check High
| return name | ||
| .replace(/<[^>]*>/g, '') // Remove HTML tags | ||
| .replace(/javascript:/gi, '') // Remove javascript: protocol | ||
| .replace(/on\w+=/gi, '') // Remove event handlers like onclick= |
Check failure
Code scanning / CodeQL
Incomplete multi-character sanitization High
| return name | ||
| .replace(/<[^>]*>/g, '') // Remove HTML tags |
Check failure
Code scanning / CodeQL
Incomplete multi-character sanitization High
MikeeBuilds
added
feature
AndyMik90
left a comment
AndyMik90
left a comment
There was a problem hiding this comment.
🤖 Auto Claude PR Review
Follow-up Review
Reviewed 14 new commit(s) since last review.
Verdict: 🔴 BLOCKED
Still blocked by 3 critical issues (3 unresolved, 0 new)
Progress Since Last Review
- Resolved: 6 finding(s) addressed
- Still Open: 9 finding(s) remaining
- New Issues: 5 new finding(s) in recent commits
Generated by Auto Claude Follow-up Review
Findings (11 selected of 14 total)
🔴 [CRITICAL] VERIFICATION REQUIRED: Preload API integration
📁 apps/frontend/src/preload/api/screenshot-api.ts:null
PR claims to add screenshot-api.ts but file doesn't exist. Must verify: 1) File creates ScreenshotAPI interface, 2) Uses ipcRenderer.invoke pattern, 3) Is integrated into ElectronAPI in index.ts.
Suggested fix:
Verify file exists with createScreenshotAPI() factory, integrated into createElectronAPI() in preload/api/index.ts
🔴 [CRITICAL] VERIFICATION REQUIRED: Component existence
📁 apps/frontend/src/renderer/components/ScreenshotCapture.tsx:null
PR claims to add ScreenshotCapture.tsx (236 lines) but file doesn't exist on current branch. Must verify component is properly implemented with loading states, error handling, and cleanup.
Suggested fix:
Verify file exists and follows React component patterns from codebase.
🟠 [HIGH] Missing canvas cleanup causing memory leaks
📁 apps/frontend/src/renderer/components/ScreenshotCapture.tsx:null
Based on existing ImageUpload.tsx patterns, thumbnail generation creates canvas elements that are never cleaned up. Multiple screenshot captures could accumulate canvases in memory.
Suggested fix:
After canvas.toDataURL(), add cleanup: canvas.width = 0; canvas.height = 0;
🟠 [HIGH] Missing Image object cleanup
📁 apps/frontend/src/renderer/components/ScreenshotCapture.tsx:null
Image objects created for processing should have their event handlers and src cleared after use to prevent memory leaks.
Suggested fix:
After image processing, add: img.onload = null; img.onerror = null; img.src = '';
🟠 [HIGH] VERIFICATION REQUIRED: Integration with existing component
📁 apps/frontend/src/renderer/components/TaskCreationWizard.tsx:null
PR claims to add 'Reference Images (optional)' section with Capture button, but current TaskCreationWizard.tsx doesn't have this. Must verify integration follows existing patterns.
Suggested fix:
Verify TaskCreationWizard imports and renders ScreenshotCapture, handles captured images via existing ImageAttachment pattern.
🔴 [CRITICAL] VERIFICATION REQUIRED: Handler registration
📁 apps/frontend/src/main/ipc-handlers/index.ts:null
Current index.ts doesn't import or register screenshot handlers. PR must add: import and registerScreenshotHandlers() call in setupIpcHandlers().
Suggested fix:
Add import and registration following existing pattern in setupIpcHandlers().
🟠 [HIGH] Hardcoded password detected
📁 (in diff):0
Potential security issue in new code: hardcoded password detected
🟠 [HIGH] Hardcoded API key detected
📁 (in diff):0
Potential security issue in new code: hardcoded api key detected
🟠 [HIGH] Hardcoded secret detected
📁 (in diff):0
Potential security issue in new code: hardcoded secret detected
🟠 [HIGH] Use of eval() detected
📁 (in diff):0
Potential security issue in new code: use of eval() detected
🟠 [HIGH] dangerouslySetInnerHTML usage detected
📁 (in diff):0
Potential security issue in new code: dangerouslysetinnerhtml usage detected
This review was generated by Auto Claude.
AlexMadera
left a comment
AlexMadera
left a comment
There was a problem hiding this comment.
🤖 Auto Claude PR Review
Merge Verdict: 🟡 MERGE WITH CHANGES
1 high-priority issues to address
Risk Assessment
| Factor | Level | Notes |
|---|---|---|
| Complexity | High | Based on lines changed |
| Security Impact | Low | Based on security findings |
| Scope Coherence | Good | Based on structural review |
Findings Summary
- High: 1 issue(s)
- Medium: 4 issue(s)
- Low: 3 issue(s)
Generated by Auto Claude PR Review
Findings (8 selected of 8 total)
🟠 [HIGH] Missing unit tests for new IPC handlers
📁 apps/frontend/src/main/ipc-handlers/screenshot-handlers.ts:0
New IPC handlers (SCREENSHOT_GET_SOURCES, SCREENSHOT_CAPTURE) have no accompanying test file. The existing pattern in the codebase includes tests for IPC handlers in __tests__/ipc-handlers.test.ts. Screenshot handlers involve Electron's desktopCapturer API which should be mocked and tested.
Suggested fix:
Create `apps/frontend/src/main/__tests__/screenshot-handlers.test.ts` with tests covering: rate limiting behavior, sourceId validation, error handling, and mocked desktopCapturer responses.
🟡 [MEDIUM] Race condition in useEffect async operation
📁 apps/frontend/src/renderer/components/ScreenshotCapture.tsx:32
The loadSources() async function is called in useEffect when open changes to true, but there's no cleanup or AbortController to cancel in-flight requests. If the dialog closes while loadSources() is executing, it will attempt to call setSources/setSelectedSource on an unmounted or reset component.
Suggested fix:
Add a mounted flag or AbortController to the useEffect: `useEffect(() => { let mounted = true; if (open) { loadSources().then(() => { if (!mounted) return; /* setState calls */ }); } return () => { mounted = false; }; }, [open]);`
🟡 [MEDIUM] XSS sanitization can be bypassed
📁 apps/frontend/src/main/ipc-handlers/screenshot-handlers.ts:22
The sanitizeName function uses a blocklist approach that can be bypassed: (1) HTML entities like <script> are not handled, (2) data: and vbscript: protocols are not blocked, (3) The regex on\w+= can be bypassed with whitespace like onclick\n=. Window names from external applications could contain malicious content.
Suggested fix:
Replace blocklist with HTML entity encoding: `return name.replace(/&/g, '&').replace(/</g, '<').replace(/>/g, '>').replace(/"/g, '"').replace(/'/g, ''').trim().substring(0, 200);` Also ensure renderer uses textContent not innerHTML when displaying names.
🟡 [MEDIUM] Large screenshot memory consumption
📁 apps/frontend/src/main/ipc-handlers/screenshot-handlers.ts:43
SCREENSHOT_CAPTURE requests thumbnails at 3840x2160 (4K) resolution. Each screenshot as base64 can be 5-15MB. Rate limit of 10 captures/minute still allows 150MB+ of image data to accumulate in memory, which could cause memory pressure on systems with limited RAM.
Suggested fix:
Consider: (1) reducing default resolution to 1920x1080, (2) adding a total memory budget check, (3) compressing to JPEG in the handler, or (4) documenting the memory impact for users with many monitors.
🟡 [MEDIUM] Missing cleanup for async operations in useEffect
📁 apps/frontend/src/renderer/components/TaskCreationWizard.tsx:178
The useEffect at line 141+ that handles draft restoration calls async functions like fetchBranches() and fetchProjectDefaultBranch() without cleanup. The eslint-disable comment masks this issue. If the dialog unmounts during these async operations, state updates will be attempted on unmounted components.
Suggested fix:
Add a mounted flag or move the async calls to a separate effect with proper cleanup. Consider using AbortController for cancellable fetch operations.
🔵 [LOW] Missing component tests
📁 apps/frontend/src/renderer/components/ScreenshotCapture.tsx:0
ScreenshotCapture.tsx is a new React component with complex async behavior, loading states, and error handling. No test file was added. The project has established testing patterns in __tests__ directories.
Suggested fix:
Create `apps/frontend/src/renderer/components/__tests__/ScreenshotCapture.test.tsx` covering: loading state, error display, source selection, capture flow, and dialog close behavior.
🔵 [LOW] Missing aria-label on remove button
📁 apps/frontend/src/renderer/components/TaskCreationWizard.tsx:949
The remove button in the image thumbnail section (around line 849 in original, 949 after changes) only has an X icon without aria-label. Screen reader users won't understand the button's purpose.
Suggested fix:
Add aria-label to the remove button: `<button aria-label={`Remove image ${image.filename}`} ...>`
🔵 [LOW] Error messages may leak system information
📁 apps/frontend/src/main/ipc-handlers/screenshot-handlers.ts:58
The catch blocks return the raw error message to the renderer: error instanceof Error ? error.message : 'Failed to...'. Error messages from Electron's desktopCapturer could contain file paths, usernames, or internal state information.
Suggested fix:
Log detailed errors server-side but return generic messages to renderer: `return { success: false, error: 'Failed to get screenshot sources' };`
This review was generated by Auto Claude.
|
|
MikeeBuilds
added
❌ MERGE CONFLICTS
stability-hold
5 participants
Summary
Adds a built-in screenshot capture feature to the task creation modal, allowing users to capture screens or windows directly without leaving the app.
What This Does
This PR adds a screenshot capture system integrated into the Reference Images section of the new task modal. Users can now capture screenshots of their entire screen or individual windows and add them as reference images for their tasks.
Changes Made
1. Screenshot Capture Modal (
ScreenshotCapture.tsx)2. Electron IPC Layer
screenshot-handlers.ts): Uses Electron'sdesktopCapturerAPISCREENSHOT_GET_SOURCES: Returns list of available screens/windows with thumbnailsSCREENSHOT_CAPTURE: Captures full-resolution screenshot from selected sourcescreenshot-api.ts): Exposes screenshot functionality to rendereripc.ts): Added new IPC channel definitions3. Task Creation Modal Integration (
TaskCreationWizard.tsx)ImageUploadcomponent for consistent UIscreenshot-1234567890.png)User Flow
Technical Details
desktopCapturer.getSources()for screenshot captureWhy This Feature?
Previously, users had to:
Now they can capture directly from the app in one click, streamlining the workflow for adding visual references to tasks.
Testing
Tested on macOS with:
Screenshots
The feature adds a "Capture" button in the Reference Images section that opens a source selection modal showing all available screens and windows.
Files Changed
apps/frontend/src/main/ipc-handlers/screenshot-handlers.ts(new)apps/frontend/src/main/ipc-handlers/index.ts(register handlers)apps/frontend/src/preload/api/screenshot-api.ts(new)apps/frontend/src/preload/api/index.ts(integrate API)apps/frontend/src/renderer/components/ScreenshotCapture.tsx(new)apps/frontend/src/renderer/components/TaskCreationWizard.tsx(integration)apps/frontend/src/shared/constants/ipc.ts(IPC channels)🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes / Improvements
✏️ Tip: You can customize this high-level summary in your review settings.