Added a screenshot upload feature in the feedback launcher#3508
Merged
Baalmart merged 20 commits intoMay 21, 2026
Conversation
Contributor
📝 WalkthroughWalkthroughAdds client screenshot capture and annotation to the Feedback launcher with optional Cloudinary upload (sends resulting screenshot_url in SubmitFeedbackRequest); adds server upload route and Cloudinary env getters; logs a non-production warning when NEXTAUTH secret is empty. ChangesFeedback Screenshot Attachment
Auth Secret Initialization with Fallback
Sequence DiagramsequenceDiagram
participant User
participant FeedbackLauncher
participant CloudinaryUploadAPI
participant Cloudinary
participant FeedbackAPI
User->>FeedbackLauncher: Capture & annotate screenshot
FeedbackLauncher->>CloudinaryUploadAPI: POST file (FormData)
CloudinaryUploadAPI->>Cloudinary: Signed upload request (server-side)
Cloudinary-->>CloudinaryUploadAPI: secure_url
CloudinaryUploadAPI-->>FeedbackLauncher: secure_url
FeedbackLauncher->>FeedbackAPI: POST feedback payload (includes screenshot_url)
FeedbackAPI-->>FeedbackLauncher: Response
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsErrors were encountered while retrieving linked issues. Errors (1)
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 |
Contributor
There was a problem hiding this comment.
Pull request overview
Adds optional screenshot attachment support to Vertex’s feedback launcher so users can include visual context when submitting issues/ideas, alongside a small NextAuth secret fallback improvement.
Changes:
- Adds screenshot file selection UI + client-side validation and attempts Cloudinary upload before submitting feedback.
- Extends feedback submission request type to include optional
screenshot_url. - Tweaks NextAuth secret resolution for local development and logs a warning when using a dev fallback secret.
Reviewed changes
Copilot reviewed 3 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
src/vertex/components/features/feedback/feedback-launcher.tsx |
Adds screenshot selection/validation state and wires Cloudinary upload into the feedback submission flow. |
src/vertex/core/apis/feedback.ts |
Adds optional screenshot_url to SubmitFeedbackRequest. |
src/vertex/app/api/auth/[...nextauth]/options.ts |
Improves auth secret fallback behavior and adds a dev warning log. |
src/vertex/package-lock.json |
Updates resolved dependency versions in the Vertex app lockfile. |
package-lock.json |
Introduces a new root lockfile (currently appears to be a stub). |
Files not reviewed (1)
- src/vertex/package-lock.json: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+207
to
+213
| try { | ||
| const uploadedScreenshot = await uploadToCloudinary(screenshotFile, { | ||
| folder: 'feedback', | ||
| tags: ['vertex', 'feedback'], | ||
| }); | ||
|
|
||
| screenshot_url = uploadedScreenshot.secure_url; |
Comment on lines
+301
to
+316
| <div className="rounded-xl border border-dashed border-gray-300 bg-white p-4 transition-colors hover:border-primary/60 dark:border-gray-700 dark:bg-gray-800"> | ||
| <label | ||
| htmlFor="screenshot" | ||
| className="mb-2 block text-xs font-medium uppercase tracking-wide text-gray-500 dark:text-gray-400" | ||
| > | ||
| Capture screenshot | ||
| </label> | ||
|
|
||
| <input | ||
| ref={screenshotInputRef} | ||
| id="screenshot" | ||
| type="file" | ||
| accept={ALLOWED_FEEDBACK_SCREENSHOT_MIME_TYPES.join(',')} | ||
| onChange={handleScreenshotChange} | ||
| className="block w-full cursor-pointer rounded-lg border border-gray-300 bg-white text-sm text-gray-700 shadow-sm file:mr-4 file:cursor-pointer file:rounded-md file:border-0 file:bg-primary file:px-4 file:py-2 file:text-sm file:font-medium file:text-white hover:file:bg-primary/90 focus:outline-none dark:border-gray-700 dark:bg-gray-900 dark:text-gray-200" | ||
| /> |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
src/vertex/components/features/feedback/feedback-launcher.tsx (2)
500-510: 💤 Low valueSilent failure if
toBlobreturns null.If
canvas.toBlobreturnsnull(rare, but possible under memory pressure), the user receives no feedback andscreenshotFileremains unset. Consider adding an error toast for this edge case.🔧 Handle null blob case
canvas.toBlob( (blob) => { if (blob) { setScreenshotDataUrl(dataUrl); setScreenshotFile(new File([blob], 'screenshot.jpg', { type: 'image/jpeg' })); toast.success('Screenshot captured successfully'); + } else { + toast.error('Failed to process screenshot'); } }, 'image/jpeg', 0.9, );🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/vertex/components/features/feedback/feedback-launcher.tsx` around lines 500 - 510, The canvas.toBlob callback currently ignores the null-blob edge case, causing silent failure; update the callback used in feedback-launcher.tsx so that if the blob is null you call toast.error with a helpful message (e.g., "Failed to capture screenshot"), clear or leave setScreenshotFile unset explicitly (or set it to null) and avoid calling setScreenshotDataUrl; keep the existing success branch that calls setScreenshotDataUrl and setScreenshotFile when blob is present.
170-177: 💤 Low valueConsider adding cleanup to prevent state updates on unmounted component.
If the component unmounts before the image finishes loading,
setImgLoaded(true)will fire on an unmounted component. While React 18 is more lenient about this, it's still good practice to add a cleanup flag.🔧 Suggested cleanup pattern
useEffect(() => { + let cancelled = false; const img = new Image(); img.onload = () => { + if (cancelled) return; imgRef.current = img; setImgLoaded(true); }; img.src = baseDataUrl; + return () => { cancelled = true; }; }, [baseDataUrl]);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/vertex/components/features/feedback/feedback-launcher.tsx` around lines 170 - 177, The useEffect that creates an Image and sets imgRef.current and calls setImgLoaded(true) can update state after the component unmounts; modify the effect (the useEffect that depends on baseDataUrl and uses imgRef and setImgLoaded) to use a mounted/aborted flag (or remove the onload handler in cleanup) so the img.onload handler checks the flag before calling setImgLoaded and assigning imgRef.current, and ensure you clean up the Image.onload handler in the return cleanup function to avoid memory leaks.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/vertex/components/features/feedback/feedback-launcher.tsx`:
- Around line 434-440: The await Promise in feedback-launcher.tsx that waits for
track.readyState === 'live' can hang indefinitely; modify the logic in that
block to add a timeout (e.g., 5–10s) so the promise always settles: if
track.readyState === 'live' resolve immediately, otherwise attach the 'unmute'
listener on track and also start a timer that rejects or resolves after the
timeout; ensure you remove the event listener and clear the timeout in both the
listener and timeout handlers to avoid leaks and race conditions—update the
Promise creation around track.readyState / track.addEventListener to include
this timeout and cleanup.
- Around line 838-867: Add a keyboard handler for the preview modal: in the same
component where previewOpen, setPreviewOpen, and screenshotDataUrl are defined,
add a useEffect that, when previewOpen is true, registers a global keydown
listener that closes the modal by calling setPreviewOpen(false) when event.key
=== 'Escape', and cleans up the listener when previewOpen becomes false or the
component unmounts; ensure the effect depends on previewOpen and setPreviewOpen
so the Escape key dismisses the preview the same as clicking the backdrop or the
close button (X).
- Around line 446-448: The code directly instantiates ImageCapture and calls
grabFrame without feature detection; update the capture block to first check for
ImageCapture support (e.g., typeof ImageCapture !== "undefined" ||
window.ImageCapture) and only then call new ImageCapture(track) and
imageCapture.grabFrame(); if ImageCapture is unavailable, implement a
canvas-based fallback that creates an offscreen or hidden canvas, draws the
current video frame via canvas.getContext('2d').drawImage(videoElement,
0,0,width,height) (or use an ImageBitmap if available), and export the image via
canvas.toBlob() or canvas.toDataURL(); in both paths ensure track.stop() is
still called and that errors are handled with a clearer message ("Image capture
not supported; used canvas fallback" or the specific error) and reference the
existing variables imageCapture, grabFrame, bitmap, and track to locate and
replace the block.
---
Nitpick comments:
In `@src/vertex/components/features/feedback/feedback-launcher.tsx`:
- Around line 500-510: The canvas.toBlob callback currently ignores the
null-blob edge case, causing silent failure; update the callback used in
feedback-launcher.tsx so that if the blob is null you call toast.error with a
helpful message (e.g., "Failed to capture screenshot"), clear or leave
setScreenshotFile unset explicitly (or set it to null) and avoid calling
setScreenshotDataUrl; keep the existing success branch that calls
setScreenshotDataUrl and setScreenshotFile when blob is present.
- Around line 170-177: The useEffect that creates an Image and sets
imgRef.current and calls setImgLoaded(true) can update state after the component
unmounts; modify the effect (the useEffect that depends on baseDataUrl and uses
imgRef and setImgLoaded) to use a mounted/aborted flag (or remove the onload
handler in cleanup) so the img.onload handler checks the flag before calling
setImgLoaded and assigning imgRef.current, and ensure you clean up the
Image.onload handler in the return cleanup function to avoid memory leaks.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e6ec1b89-334c-4cc1-bc8e-af97b54f7968
📒 Files selected for processing (2)
src/vertex/app/changelog.mdsrc/vertex/components/features/feedback/feedback-launcher.tsx
✅ Files skipped from review due to trivial changes (1)
- src/vertex/app/changelog.md
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/vertex/components/features/feedback/feedback-launcher.tsx (1)
549-576:⚠️ Potential issue | 🟠 Major | ⚡ Quick winEnforce the 2 MB cap before storing the generated screenshot.
Line 551 and Line 580 both persist a freshly generated JPEG without checking its size first. Full-browser captures can easily exceed 2,097,152 bytes on HiDPI displays, so this pushes a required validation failure to upload time instead of warning immediately in the UI.
💡 Minimal guard
canvas.toBlob( (blob) => { if (blob) { + if (blob.size > 2_097_152) { + showBanner({ + severity: 'warning', + message: 'Screenshots must be 2 MB or smaller.', + scoped: true, + }); + return; + } setScreenshotDataUrl(dataUrl); setScreenshotFile(new File([blob], 'screenshot.jpg', { type: 'image/jpeg' })); showBanner({ severity: 'success', message: 'Screenshot captured successfully', scoped: true }); } }, 'image/jpeg', 0.9, ); return; } const { dataUrl, file } = await flattenAnnotations( rawDataUrl, rects, rawDimensions.w, rawDimensions.h, ); + if (file.size > 2_097_152) { + showBanner({ + severity: 'warning', + message: 'Screenshots must be 2 MB or smaller.', + scoped: true, + }); + return; + } setScreenshotDataUrl(dataUrl); setScreenshotFile(file);Also applies to: 580-588
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/vertex/components/features/feedback/feedback-launcher.tsx` around lines 549 - 576, When converting the canvas to a JPEG and before calling setScreenshotDataUrl/setScreenshotFile (inside the canvas.toBlob callback) enforce a 2,097,152 byte size cap: check blob.size and if it exceeds the cap, call showBanner with a clear error (e.g., "Screenshot exceeds 2MB") and do not set the data URL or file; otherwise proceed to setScreenshotDataUrl(dataUrl) and setScreenshotFile(new File(...)). Apply the same size check in the other canvas.toBlob branch later in this file so both places that persist the generated screenshot validate blob.size before storing or showing success.src/vertex/core/apis/cloudinary.ts (1)
93-99:⚠️ Potential issue | 🟠 Major | ⚡ Quick winLegacy uploader now swallows HTTP failures.
cloudinaryImageUploadno longer checksresponse.ok, so 4xx/5xx responses resolve as normal JSON and can propagate as false “success” to callers.Suggested fix
export const cloudinaryImageUpload = async (formData: FormData) => { const { cloudinaryUrl } = getCloudinaryConfig(); - return await fetch(`${cloudinaryUrl}/image/upload`, { + const response = await fetch(`${cloudinaryUrl}/image/upload`, { method: 'POST', body: formData, - }).then((response) => response.json()); + }); + const result = await response.json(); + if (!response.ok) { + throw new Error(result?.error?.message || 'Cloudinary upload failed'); + } + return result; };🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/vertex/core/apis/cloudinary.ts` around lines 93 - 99, cloudinaryImageUpload currently returns response.json() for any HTTP status, so 4xx/5xx results are treated as success; update cloudinaryImageUpload to await fetch, check response.ok, and if false read the response body (e.g., await response.text() or response.json()) and throw an error containing the HTTP status and the response body/details, otherwise return the parsed JSON; reference the cloudinaryImageUpload function and the response variable when implementing this change.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/vertex/app/api/cloudinary/upload/route.ts`:
- Around line 9-10: The code currently trusts the client-provided folder
variable (folder) when building the Cloudinary upload signature; stop using
client-supplied folders by either hardcoding folder = 'feedback' or validating
against a server-side whitelist before signing. Locate the upload route handler
where folder is read from formData (the folder variable and where signature is
generated) and replace uses of the untrusted folder with a constant ('feedback')
or a validated value derived from a server-side whitelist before creating the
signature and upload params (also update any other spots that read folder around
the tags handling and later signature generation).
- Around line 8-14: Validate the uploaded File server-side before signing or
returning upload data: check the "file" object's presence, its size (using
file.size) against the max allowed bytes (e.g., MAX_UPLOAD_BYTES constant) and
its MIME type (file.type) or extension against an allowlist (e.g.,
ALLOWED_IMAGE_TYPES), and return a 400 JSON error if any check fails; update the
code paths that create a signed upload (the code that reads const file =
formData.get('file') and the later signing/upload block) to perform the same
validation so direct callers cannot obtain a signed URL for
oversized/unsupported files.
- Around line 66-69: The Cloudinary upstream fetch (the call using
fetch(cloudinaryUrl, { method: 'POST', body: cloudinaryFormData })) can
hang—wrap it with an AbortController-based timeout: create an AbortController,
pass controller.signal to the fetch, schedule a setTimeout that calls
controller.abort() after a reasonable limit, and clear the timeout when fetch
completes; update the surrounding handler (in route.ts around the fetch call) to
catch the abort error and return a 504 or appropriate error response. Ensure you
reference cloudinaryUrl, cloudinaryFormData, the fetch call, and the created
AbortController so the timeout logic cleanly aborts the upstream request and
frees route workers.
---
Outside diff comments:
In `@src/vertex/components/features/feedback/feedback-launcher.tsx`:
- Around line 549-576: When converting the canvas to a JPEG and before calling
setScreenshotDataUrl/setScreenshotFile (inside the canvas.toBlob callback)
enforce a 2,097,152 byte size cap: check blob.size and if it exceeds the cap,
call showBanner with a clear error (e.g., "Screenshot exceeds 2MB") and do not
set the data URL or file; otherwise proceed to setScreenshotDataUrl(dataUrl) and
setScreenshotFile(new File(...)). Apply the same size check in the other
canvas.toBlob branch later in this file so both places that persist the
generated screenshot validate blob.size before storing or showing success.
In `@src/vertex/core/apis/cloudinary.ts`:
- Around line 93-99: cloudinaryImageUpload currently returns response.json() for
any HTTP status, so 4xx/5xx results are treated as success; update
cloudinaryImageUpload to await fetch, check response.ok, and if false read the
response body (e.g., await response.text() or response.json()) and throw an
error containing the HTTP status and the response body/details, otherwise return
the parsed JSON; reference the cloudinaryImageUpload function and the response
variable when implementing this change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a624ca40-e384-4b49-8e0a-d989b38ba9d2
📒 Files selected for processing (6)
src/vertex/.env.examplesrc/vertex/app/api/auth/[...nextauth]/options.tssrc/vertex/app/api/cloudinary/upload/route.tssrc/vertex/components/features/feedback/feedback-launcher.tsxsrc/vertex/core/apis/cloudinary.tssrc/vertex/lib/envConstants.ts
✅ Files skipped from review due to trivial changes (1)
- src/vertex/app/api/auth/[...nextauth]/options.ts
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/vertex/components/features/feedback/feedback-launcher.tsx (2)
580-591:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winMissing success banner when annotations are applied.
When
rects.length === 0, a success banner is shown (line 571). However, when annotations are flattened viaflattenAnnotations, no success banner is displayed. This creates an inconsistent user experience.🎨 Proposed fix
const { dataUrl, file } = await flattenAnnotations( rawDataUrl, rects, rawDimensions.w, rawDimensions.h, ); setScreenshotDataUrl(dataUrl); setScreenshotFile(file); + showBanner({ severity: 'success', message: 'Screenshot captured successfully', scoped: true }); } catch {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/vertex/components/features/feedback/feedback-launcher.tsx` around lines 580 - 591, After successful flattening of annotations, add the same success banner call that is shown when rects.length === 0 so users get consistent feedback; specifically, after the await flattenAnnotations(...) and after calling setScreenshotDataUrl(...) and setScreenshotFile(...), invoke showBanner({ severity: 'success', message: 'Screenshot annotations applied', scoped: true }) (or reuse the existing success message string) so that flattenAnnotations, setScreenshotDataUrl, setScreenshotFile and showBanner are updated together to display success when annotations were applied.
496-522:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winPotential track leak if fallback errors before
track.stop().If an exception occurs in the video fallback path (e.g., at
canvas.getContextor earlier), the track won't be stopped, potentially leaving the screen-share indicator active. Consider wrapping the capture logic in a try/finally or ensuringtrack.stop()is called in the outer catch block.🛡️ Proposed fix
} catch (error: unknown) { + // Ensure any lingering tracks are stopped + try { + const tracks = (error as any)?.stream?.getVideoTracks?.() ?? []; + tracks.forEach((t: MediaStreamTrack) => t.stop()); + } catch { /* ignore */ } if (error instanceof Error && error.name === 'NotAllowedError') return; // user cancelled showBanner({ severity: 'error', message: 'Failed to capture screenshot', scoped: true });Alternatively, store the track in a variable at the top of the try block and call
track?.stop()in the catch/finally:const captureScreenshot = async () => { setIsCapturing(true); await new Promise<void>((resolve) => setTimeout(resolve, 50)); + let track: MediaStreamTrack | null = null; try { const stream = await navigator.mediaDevices.getDisplayMedia({ video: { displaySurface: 'browser' }, audio: false, } as DisplayMediaStreamOptions); - const track = stream.getVideoTracks()[0]; + track = stream.getVideoTracks()[0]; // ... rest of capture logic } catch (error: unknown) { + track?.stop(); if (error instanceof Error && error.name === 'NotAllowedError') return; showBanner({ severity: 'error', message: 'Failed to capture screenshot', scoped: true }); } finally { setIsCapturing(false); } };🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/vertex/components/features/feedback/feedback-launcher.tsx` around lines 496 - 522, The video fallback capture path can throw before calling track.stop(), leaking the media track; modify the logic in feedback-launcher.tsx so the MediaStreamTrack (track) is saved to a local variable before the capture block and ensure track?.stop() is invoked in a finally block (or catch + finally) around the video/canvas ops that produce dataUrl, captureWidth and captureHeight; alternatively wrap the inner capture (video creation, await onloadedmetadata, canvas.getContext/drawImage, canvas.toDataURL) in a try/finally and call track?.stop() in the finally to guarantee the track is always stopped even if getContext or drawImage fails.
🧹 Nitpick comments (2)
src/vertex/components/features/feedback/feedback-launcher.tsx (1)
204-211: 💤 Low valueMinor optimization opportunity in canvas dimension effect.
This effect runs whenever
redrawchanges (i.e., on every rect update), but canvas dimensions only need to be set once when the image loads. Consider splitting the dimension-setting logic from the redraw trigger, or guarding with a ref to track if dimensions were already set. Not a functional issue, just a slight inefficiency.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/vertex/components/features/feedback/feedback-launcher.tsx` around lines 204 - 211, The effect currently tied to useEffect with dependencies [imgLoaded, redraw] sets canvas dimensions every time redraw changes; separate the responsibilities by moving dimension-setting into its own effect that only depends on imgLoaded (or use a dimensionsSetRef guard) so canvasRef/current.width and .height are set once when the image loads, while the existing redraw effect continues to run on redraw changes; update references to canvasRef, imgRef, imgLoaded, and redraw accordingly (or add a dimensionsSetRef flag) to avoid resetting dimensions on every rect update.src/vertex/core/apis/cloudinary.ts (1)
93-99: ⚡ Quick winAlign
cloudinaryImageUploaderror handling withuploadToCloudinary(src/vertex/core/apis/cloudinary.ts:93-99)
cloudinaryImageUploadcurrently returnsresponse.json()without checkingresponse.okand withouttry/catch, so callers (if any external/back-compat consumers) may see raw Cloudinary error payloads or unhandled fetch rejections. There are no in-repo call sites found, but it should still mirroruploadToCloudinary’sresponse.okhandling and error normalization for consistency.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/vertex/core/apis/cloudinary.ts` around lines 93 - 99, cloudinaryImageUpload currently returns response.json() without handling non-OK responses or network errors; update cloudinaryImageUpload to mirror uploadToCloudinary by wrapping the fetch in try/catch, await the response, check response.ok and parse the body, and throw a normalized error (including response.status and parsed error payload) when !response.ok so callers receive the same error shape as uploadToCloudinary; reference function names cloudinaryImageUpload and uploadToCloudinary to locate and align behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/vertex/components/features/feedback/feedback-launcher.tsx`:
- Around line 580-591: After successful flattening of annotations, add the same
success banner call that is shown when rects.length === 0 so users get
consistent feedback; specifically, after the await flattenAnnotations(...) and
after calling setScreenshotDataUrl(...) and setScreenshotFile(...), invoke
showBanner({ severity: 'success', message: 'Screenshot annotations applied',
scoped: true }) (or reuse the existing success message string) so that
flattenAnnotations, setScreenshotDataUrl, setScreenshotFile and showBanner are
updated together to display success when annotations were applied.
- Around line 496-522: The video fallback capture path can throw before calling
track.stop(), leaking the media track; modify the logic in feedback-launcher.tsx
so the MediaStreamTrack (track) is saved to a local variable before the capture
block and ensure track?.stop() is invoked in a finally block (or catch +
finally) around the video/canvas ops that produce dataUrl, captureWidth and
captureHeight; alternatively wrap the inner capture (video creation, await
onloadedmetadata, canvas.getContext/drawImage, canvas.toDataURL) in a
try/finally and call track?.stop() in the finally to guarantee the track is
always stopped even if getContext or drawImage fails.
---
Nitpick comments:
In `@src/vertex/components/features/feedback/feedback-launcher.tsx`:
- Around line 204-211: The effect currently tied to useEffect with dependencies
[imgLoaded, redraw] sets canvas dimensions every time redraw changes; separate
the responsibilities by moving dimension-setting into its own effect that only
depends on imgLoaded (or use a dimensionsSetRef guard) so
canvasRef/current.width and .height are set once when the image loads, while the
existing redraw effect continues to run on redraw changes; update references to
canvasRef, imgRef, imgLoaded, and redraw accordingly (or add a dimensionsSetRef
flag) to avoid resetting dimensions on every rect update.
In `@src/vertex/core/apis/cloudinary.ts`:
- Around line 93-99: cloudinaryImageUpload currently returns response.json()
without handling non-OK responses or network errors; update
cloudinaryImageUpload to mirror uploadToCloudinary by wrapping the fetch in
try/catch, await the response, check response.ok and parse the body, and throw a
normalized error (including response.status and parsed error payload) when
!response.ok so callers receive the same error shape as uploadToCloudinary;
reference function names cloudinaryImageUpload and uploadToCloudinary to locate
and align behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b216bcc4-df62-4677-be02-f03eb9bead98
📒 Files selected for processing (3)
src/vertex/app/api/cloudinary/upload/route.tssrc/vertex/components/features/feedback/feedback-launcher.tsxsrc/vertex/core/apis/cloudinary.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Screenshot Upload for Feedback Submission
Closes #3456
Summary
Adds screenshot upload functionality to the feedback submission system in the Vertex application, enabling users to attach visual context when reporting issues or suggesting improvements.
Changes
Screenshot Capture and Upload
Frontend (
feedback-launcher.tsx)screenshotFile,isUploadingScreenshot)handleScreenshotChange()andhandleRemoveScreenshot()handlersAPI (
feedback.ts)screenshot_url?: stringfield toSubmitFeedbackRequestinterfaceAuthentication (
options.ts)Known Issue
Backend Cloudinary Configuration Mismatch
Resolution required: The backend team must configure their Cloudinary account with matching credentials before this feature can be fully utilised in production.
Maturity Checklist
Manual Testing
Setup
Test Cases
1. Screenshot Upload Flow
2. Validation
.txt) : expected error: "Unsupported file format"3. Submission with Screenshot
4. Submission without Screenshot
5. Upload Failure : Graceful Handling
screenshot_urlisundefinedin the backend payloadScreenshot
Summary by CodeRabbit
New Features
Improvements
Documentation
Chores