Conversation
Greptile SummaryThis PR adds a Screenshot (PNG) export option to the chat export dropdown. It introduces a hidden off-screen Confidence Score: 4/5Safe to merge after fixing the missing The core capture pipeline (snapdom integration, object-URL management, off-screen portal rendering, stable effect deps) is well-implemented and the prior thread concerns around URL leaking and effect instability have been addressed. One P1 accessibility gap remains (no
Important Files Changed
Sequence DiagramsequenceDiagram
participant User
participant ChatHeader
participant useScreenshotExport
participant ScreenshotRenderer
participant snapdom
participant ScreenshotPreviewModal
User->>ChatHeader: Click "Screenshot (PNG)"
ChatHeader->>useScreenshotExport: startCapture()
useScreenshotExport-->>ChatHeader: isCapturing=true
ChatHeader->>ScreenshotRenderer: mount (portal to document.body)
Note over ScreenshotRenderer: rAF + 500ms delay
ScreenshotRenderer->>snapdom: captureElementAsBlob(el)
snapdom-->>ScreenshotRenderer: Blob (PNG)
ScreenshotRenderer->>useScreenshotExport: onCaptureComplete(blob)
useScreenshotExport->>useScreenshotExport: revoke old objectURL (if any)
useScreenshotExport->>useScreenshotExport: URL.createObjectURL(blob)
useScreenshotExport-->>ChatHeader: isCapturing=false, screenshot={blob,url}
ChatHeader->>ScreenshotRenderer: unmount
ChatHeader->>ScreenshotPreviewModal: mount with imageUrl + blob
User->>ScreenshotPreviewModal: Click "Download"
ScreenshotPreviewModal->>ScreenshotPreviewModal: downloadBlob(blob, filename)
User->>ScreenshotPreviewModal: Click "Copy to clipboard"
ScreenshotPreviewModal->>ScreenshotPreviewModal: navigator.clipboard.write(ClipboardItem)
User->>ScreenshotPreviewModal: Close
ScreenshotPreviewModal->>useScreenshotExport: dismissPreview()
useScreenshotExport->>useScreenshotExport: revoke objectURL, screenshot=null
Prompt To Fix All With AIThis is a comment left during a code review.
Path: ui/src/components/ScreenshotRenderer/ScreenshotRenderer.tsx
Line: 91-101
Comment:
**Off-screen container is readable by screen readers**
The container is positioned off-screen with `left: -99999px` but is still present in the DOM and fully accessible to assistive technologies. Screen readers will traverse this element and read out every conversation message a second time, right after reading the real chat. This duplicates the entire conversation for any user on a screen reader.
Add `aria-hidden="true"` to suppress it from the accessibility tree:
```suggestion
<div
ref={containerRef}
aria-hidden="true"
className={`${themeClass} bg-background text-foreground`}
style={{
position: "fixed",
left: "-99999px",
top: 0,
width: 800,
padding: 32,
}}
>
```
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: ui/src/hooks/useScreenshotExport.ts
Line: 16-19
Comment:
**Re-capture while preview is open leaves the stale modal visible**
When the user clicks "Screenshot (PNG)" while an existing preview is already showing, `startCapture` sets `isCapturing = true` but does not clear `screenshot`. This means:
1. `ScreenshotPreviewModal` stays open showing the previous capture.
2. `ScreenshotRenderer` simultaneously mounts and begins a new capture.
The two components are rendered at the same time until `onCaptureComplete` fires. Clearing the previous result immediately on re-capture would give clearer feedback that a new capture is underway:
```suggestion
const startCapture = useCallback(() => {
if (objectUrlRef.current) {
URL.revokeObjectURL(objectUrlRef.current);
objectUrlRef.current = null;
}
setScreenshot(null);
setIsCapturing(true);
toast.info("Capturing screenshot…", "Rendering all messages");
}, [toast]);
```
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: ui/src/components/ScreenshotRenderer/ScreenshotPreviewModal.tsx
Line: 38-46
Comment:
**`setTimeout` handle not cleared on unmount**
If the user copies the image and then immediately closes the modal (within 2 seconds), the `setTimeout(() => setCopied(false), 2000)` callback still fires on the unmounted component. In React 18 this is silently ignored, but it's better practice to store the timer ID and cancel it in a cleanup to avoid dangling callbacks.
```suggestion
const handleCopy = useCallback(async () => {
try {
await navigator.clipboard.write([new ClipboardItem({ [blob.type]: blob })]);
setCopied(true);
const timer = setTimeout(() => setCopied(false), 2000);
return () => clearTimeout(timer);
} catch {
toast.error("Copy failed", "Your browser may not support copying images");
}
}, [blob, toast]);
```
Alternatively, manage the timer with a `useRef` + `useEffect` cleanup if the callback needs to be reset on each invocation.
How can I resolve this? If you propose a fix, please make it concise.Reviews (2): Last reviewed commit: "Review fixes" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| const startCapture = useCallback(() => { | ||
| setIsCapturing(true); | ||
| toast.info("Capturing screenshot…", "Rendering all messages"); | ||
| }, [toast]); |
There was a problem hiding this comment.
When the user clicks "Screenshot (PNG)" while an existing preview is already showing, startCapture sets isCapturing = true but does not clear screenshot. This means:
ScreenshotPreviewModalstays open showing the previous capture.ScreenshotRenderersimultaneously mounts and begins a new capture.
The two components are rendered at the same time until onCaptureComplete fires. Clearing the previous result immediately on re-capture would give clearer feedback that a new capture is underway:
| const startCapture = useCallback(() => { | |
| setIsCapturing(true); | |
| toast.info("Capturing screenshot…", "Rendering all messages"); | |
| }, [toast]); | |
| const startCapture = useCallback(() => { | |
| if (objectUrlRef.current) { | |
| URL.revokeObjectURL(objectUrlRef.current); | |
| objectUrlRef.current = null; | |
| } | |
| setScreenshot(null); | |
| setIsCapturing(true); | |
| toast.info("Capturing screenshot…", "Rendering all messages"); | |
| }, [toast]); |
Prompt To Fix With AI
This is a comment left during a code review.
Path: ui/src/hooks/useScreenshotExport.ts
Line: 16-19
Comment:
**Re-capture while preview is open leaves the stale modal visible**
When the user clicks "Screenshot (PNG)" while an existing preview is already showing, `startCapture` sets `isCapturing = true` but does not clear `screenshot`. This means:
1. `ScreenshotPreviewModal` stays open showing the previous capture.
2. `ScreenshotRenderer` simultaneously mounts and begins a new capture.
The two components are rendered at the same time until `onCaptureComplete` fires. Clearing the previous result immediately on re-capture would give clearer feedback that a new capture is underway:
```suggestion
const startCapture = useCallback(() => {
if (objectUrlRef.current) {
URL.revokeObjectURL(objectUrlRef.current);
objectUrlRef.current = null;
}
setScreenshot(null);
setIsCapturing(true);
toast.info("Capturing screenshot…", "Rendering all messages");
}, [toast]);
```
How can I resolve this? If you propose a fix, please make it concise.
1 participant
No description provided.