feat: frame-specific state based screenshots with preview #2728
Conversation
|
@reddygtvs is attempting to deploy a commit to the Onlook Team on Vercel. A member of the Team first needs to authorize it. |
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughReworks screenshot capture to use the most recently interacted frame; adds interaction tracking across frames (selection, navigation, element clicks); introduces a portal-based image preview for draft image pills; adds ChatContext.addImageContext; overhauls preload screenshot API to DOM-to-canvas with compression and fallback; minor key prop tweak; one formatting-only change. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant ChatInput
participant Frames as FramesManager
participant FrameView
participant ChatCtx as ChatContext
participant UI as UI/Toast
User->>ChatInput: Click "Screenshot"
ChatInput->>Frames: getMostRecentlyInteractedFrame()
alt Frame available
ChatInput->>FrameView: captureScreenshot()
alt Capture success with data
ChatInput->>ChatCtx: addImageContext(base64 JPEG)
ChatCtx-->>ChatInput: Context updated
ChatInput->>UI: Show success toast
else Failure or no data
ChatInput->>UI: Show error toast
end
else No frame
ChatInput->>UI: Show "No active frame" error
end
sequenceDiagram
autonumber
actor User
participant Element as ElementStore
participant Frames as FramesManager
User->>Element: Click in frame
Element->>Frames: updateLastInteraction(frameId)
note right of Frames: Sets lastInteractionTime = now
Element-->>User: Proceed with overlay/select logic
User->>Frames: select(frames)
Frames->>Frames: updateLastInteraction(each frameId)
User->>Frames: navigateToPath(frameId, path)
Frames->>Frames: updateLastInteraction(frameId)
sequenceDiagram
autonumber
participant API as captureScreenshot
participant Impl as captureScreenshotImpl
participant Render as renderDomToCanvas
participant Compress as compressImage
API->>Impl: Invoke
Impl->>Render: Render DOM to canvas
Render-->>Impl: Canvas
Impl->>Compress: Scale/quality iteration (<=5MB)
alt Fits limit
Compress-->>Impl: base64 JPEG
Impl-->>API: base64 JPEG
else Render/compress fails
Impl-->>API: Fallback canvas JPEG with message
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. 📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 💡 Knowledge Base configuration:
You can enable these sources in your CodeRabbit configuration. 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
✨ Finishing Touches
🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![ellipsis-dev[bot]](https://avatars.githubusercontent.com/in/64358?s=60&v=4){kind=small}
.../src/app/project/[id]/_components/right-panel/chat-tab/context-pills/input-context-pills.tsx
Show resolved
Hide resolved
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 (3)
apps/web/preload/script/api/screenshot.ts (2)
77-104: Fix 5MB cap check — you’re comparing base64 characters to bytes.Base64 length must be converted back to bytes before comparing to MAX_FILE_SIZE. As-is, you’ll compress far more than necessary and degrade quality.
Apply this diff:
- const MAX_FILE_SIZE = 5 * 1024 * 1024; // 5MB limit - const MAX_BASE64_SIZE = MAX_FILE_SIZE * 0.75; + const MAX_FILE_SIZE = 5 * 1024 * 1024; // 5MB limit (bytes) + const approxBytes = (dataUrl: string) => { + // Strip "data:*;base64," prefix if present, then convert Base64 chars to bytes + const comma = dataUrl.indexOf(','); + const base64Part = comma >= 0 ? dataUrl.slice(comma + 1) : dataUrl; + return Math.floor(base64Part.length * 0.75); + };And update the acceptance check:
- const base64 = scaledCanvas.toDataURL('image/jpeg', quality); - - if (base64.length <= MAX_BASE64_SIZE) { + const base64 = scaledCanvas.toDataURL('image/jpeg', quality); + if (approxBytes(base64) <= MAX_FILE_SIZE) { return base64; }
222-233: Avoid tainting the canvas on cross-origin images without CORS.Drawing a cross-origin image without proper CORS headers taints the canvas and makes toDataURL throw, forcing your global “unavailable” fallback. Detect and render a placeholder instead so the rest of the DOM still exports.
- if (element instanceof HTMLImageElement && element.complete && element.naturalWidth > 0) { + if (element instanceof HTMLImageElement && element.complete && element.naturalWidth > 0) { try { - context.drawImage(element, left, top, width, height); + // Avoid cross-origin taint if the image lacks CORS + const src = element.currentSrc || element.src || ''; + let crossOriginTaintRisk = false; + try { + const url = new URL(src, document.baseURI); + crossOriginTaintRisk = url.origin !== window.location.origin && !element.crossOrigin; + } catch { + crossOriginTaintRisk = false; + } + if (crossOriginTaintRisk) { + throw new Error('cross-origin image without CORS'); + } + context.drawImage(element, left, top, width, height); } catch (error) { context.fillStyle = '#f0f0f0'; context.fillRect(left, top, width, height); context.fillStyle = '#999999'; context.font = '12px Arial, sans-serif'; context.textAlign = 'center'; + context.textBaseline = 'middle'; context.fillText('Image', left + width / 2, top + height / 2); } }apps/web/client/src/app/project/[id]/_components/right-panel/chat-tab/chat-input/index.tsx (1)
219-224: Replace “limit reached” error with “replace existing image” behaviorThe PR introduces
ChatContext.addImageContext(imageData)to replace any existing IMAGE context with the latest one. Here we still enforce a hard limit viavalidateImageLimit(..., 1)andpush, which blocks taking a second screenshot. This diverges from the intended UX and can frustrate users.Apply either Option A (use the new API) or Option B (local replace) to align behavior:
Option A — delegate to context (preferred if available):
- const { success, errorMessage } = validateImageLimit(currentImages, 1); - if (!success) { - toast.error(errorMessage); - return; - } + // No hard limit; let context replace any existing IMAGE @@ - const contextImage: ImageMessageContext = { - type: MessageContextType.IMAGE, - content: screenshotData, - mimeType: mimeType, - displayName: 'Screenshot', - }; - editorEngine.chat.context.context.push(contextImage); + // Prefer centralized helper that replaces existing IMAGE contexts + if (typeof (editorEngine.chat.context as any).addImageContext === 'function') { + (editorEngine.chat.context as any).addImageContext(screenshotData); + } else { + // Fallback to local replace if helper isn’t available at runtime + const imageIndex = editorEngine.chat.context.context.findIndex( + (ctx) => ctx.type === MessageContextType.IMAGE + ); + const contextImage: ImageMessageContext = { + type: MessageContextType.IMAGE, + content: screenshotData, + mimeType, + displayName: 'Screenshot', + }; + if (imageIndex >= 0) { + editorEngine.chat.context.context[imageIndex] = contextImage; + } else { + editorEngine.chat.context.context.push(contextImage); + } + }Option B — local replace only (if you prefer not to rely on the helper here):
- const { success, errorMessage } = validateImageLimit(currentImages, 1); - if (!success) { - toast.error(errorMessage); - return; - } + // Allow replacing the existing IMAGE with a fresh screenshot @@ - editorEngine.chat.context.context.push(contextImage); + const imageIndex = editorEngine.chat.context.context.findIndex( + (ctx) => ctx.type === MessageContextType.IMAGE + ); + if (imageIndex >= 0) { + editorEngine.chat.context.context[imageIndex] = contextImage; + } else { + editorEngine.chat.context.context.push(contextImage); + }Also applies to: 255-262
🧹 Nitpick comments (13)
apps/web/client/src/components/store/editor/chat/context.ts (1)
222-230: Derive mimeType from the data URL and normalize content to avoid metadata drift.Hardcoding 'image/jpeg' risks desync if the capture pipeline ever changes (e.g., to WebP/PNG) or if callers pass a data URL with a different MIME. Normalize incoming data to a data URL and infer the MIME so content and mimeType stay consistent.
Apply this diff:
- addImageContext(imageData: string) { - const imageContext: ImageMessageContext = { - type: MessageContextType.IMAGE, - content: imageData, - displayName: `Screenshot ${new Date().toLocaleTimeString()}`, - mimeType: 'image/jpeg', - }; - this.context = [...this.context.filter(c => c.type !== MessageContextType.IMAGE), imageContext]; - } + addImageContext(imageData: string) { + // Normalize to a data URL and derive MIME to keep metadata in sync with content + const commaIdx = imageData.indexOf(','); + const isDataUrl = imageData.startsWith('data:') && commaIdx !== -1; + const inferredMime = + (isDataUrl ? imageData.slice(5, imageData.indexOf(';')) : null) || 'image/jpeg'; + const content = isDataUrl ? imageData : `data:${inferredMime};base64,${imageData}`; + + const imageContext: ImageMessageContext = { + type: MessageContextType.IMAGE, + content, + displayName: `Screenshot ${new Date().toLocaleTimeString()}`, + mimeType: inferredMime, + }; + this.context = [ + ...this.context.filter((c) => c.type !== MessageContextType.IMAGE), + imageContext, + ]; + }apps/web/preload/script/api/screenshot.ts (2)
59-66: Center fallback text vertically.Set textBaseline to 'middle' so the “Screenshot unavailable” message is actually centered.
- context.textAlign = 'center'; - context.fillText('Screenshot unavailable', FALLBACK_CANVAS_WIDTH / 2, FALLBACK_CANVAS_HEIGHT / 2); + context.textAlign = 'center'; + context.textBaseline = 'middle'; + context.fillText('Screenshot unavailable', FALLBACK_CANVAS_WIDTH / 2, FALLBACK_CANVAS_HEIGHT / 2);
199-218: Adjust wrap width to respect padding on both sides.Current wrap check uses width - TEXT_WRAP_MARGIN, which ignores left/right padding. This causes premature wrapping and uneven text edges.
- const words = text.split(' '); - let line = ''; - let y = top + TEXT_PADDING; - const lineHeight = fontSize * LINE_HEIGHT_MULTIPLIER; + const words = text.split(' '); + let line = ''; + let y = top + TEXT_PADDING; + const lineHeight = fontSize * LINE_HEIGHT_MULTIPLIER; + const wrapWidth = Math.max(0, width - (TEXT_PADDING * 2) - TEXT_WRAP_MARGIN); ... - if (metrics.width > width - TEXT_WRAP_MARGIN && line !== '') { + if (metrics.width > wrapWidth && line !== '') {apps/web/client/src/app/project/[id]/_components/right-panel/chat-tab/context-pills/draft-image-pill.tsx (4)
4-4: Preserve the forwarded ref — it’s currently ignored.You switched to a local pillRef but didn’t wire the forwarded ref through, which can break parent behaviors relying on it (focus/measurement).
Do any parents consume this ref? If yes, compose refs as below.
-import React, { useState, useRef } from 'react'; +import React, { useState, useRef, useCallback } from 'react';Inside the component:
- const pillRef = useRef<HTMLSpanElement>(null); + const pillRef = useRef<HTMLSpanElement>(null); + const setRefs = useCallback((node: HTMLSpanElement | null) => { + pillRef.current = node; + if (typeof ref === 'function') ref(node); + else if (ref) (ref as React.MutableRefObject<HTMLSpanElement | null>).current = node; + }, [ref]);And on the element:
- ref={pillRef} + ref={setRefs}
32-33: Typo in warn string (“DraftingImagePill”).Use the actual component name to keep logs searchable.
- console.warn('DraftingImagePill received non-image context'); + console.warn('DraftImagePill received non-image context');
43-81: Layering: give the preview a strictly higher z-index than the backdrop.Both backdrop and image use z-[9999]. Depending on DOM order and stacking contexts, they can overlap unpredictably. Bump the image to z-[10000].
- className="fixed inset-0 bg-black/60 backdrop-blur-sm z-[9999]" + className="fixed inset-0 bg-black/60 backdrop-blur-sm z-[9999]" ... - <motion.img + <motion.img src={context.content} alt="Screenshot preview" - className="fixed z-[9999] object-contain pointer-events-auto" + className="fixed z-[10000] object-contain pointer-events-auto"
88-101: Avoid setting a key inside the child component.Keys should be set at the list rendering site (parent). Having key={context.displayName} here is redundant and can cause unwanted remounts if the display name changes.
- key={context.displayName}apps/web/client/src/components/store/editor/frames/manager.ts (2)
298-305: updateLastInteraction mutates and notifies (LGTM).The explicit notify ensures MobX observers react. Minor nit: you could set the new object inline to avoid mutating then setting, but this is fine.
- if (frameData) { - frameData.lastInteractionTime = new Date(); - this._frameIdToData.set(frameId, frameData); - this.notify(); - } + if (frameData) { + this._frameIdToData.set(frameId, { ...frameData, lastInteractionTime: new Date() }); + this.notify(); + }
307-321: Prefer a frame with a live view when no interactions exist.If none have interactions, you fall back to frames[0], which may not have a view yet. Optionally prefer the first frame whose view != null to improve UX for screenshot callers.
- const mostRecent = framesWithInteractions.length > 0 ? framesWithInteractions[0] : frames[0]; + const mostRecent = + framesWithInteractions.length > 0 + ? framesWithInteractions[0] + : (frames.find(f => f.view) ?? frames[0]);apps/web/client/src/app/project/[id]/_components/right-panel/chat-tab/context-pills/input-context-pills.tsx (1)
31-37: Avoid array index as React key for image pills; prefer a stable identifierUsing the index as a key can cause identity churn when items are inserted/removed, which risks preview modal glitches and unnecessary re-mounts. Given the prior collision on content-based keys, consider adding a stable id on the image context (e.g., crypto.randomUUID()) at creation time and using it here.
Apply this minimal change locally, assuming an
idis present on the image context:- <DraftImagePill - key={`image-${index}`} + <DraftImagePill + key={`image-${(context as any).id ?? index}`} context={context} onRemove={() => handleRemoveContext(context)} />If you’d like, I can follow up with a patch that injects
idwhen creating image contexts in the screenshot/paste flows and update theMessageContexttype accordingly.apps/web/client/src/components/store/editor/screenshot/index.tsx (2)
43-47: Guard against missing projectId before calling capture APIIf
this.editorEngine.projectIdis null/undefined, the mutation will fail. Add a simple guard and reuse the localprojectIdto avoid accidentalundefinedpayloads.- const result = await api.project.captureScreenshot.mutate({ projectId: this.editorEngine.projectId }); + const projectId = this.editorEngine.projectId; + if (!projectId) { + console.warn('Screenshot skipped: no projectId available'); + return; + } + const result = await api.project.captureScreenshot.mutate({ projectId });Run-time check: try invoking
captureScreenshot()in a project-less state (e.g., immediately after editor init) to ensure we don’t emit a failing request.
23-27: Stabilizethisbinding for the debounced methodWhile lodash forwards the call-time
this, passing a bare class method still risks losing context ifcaptureScreenshotis detached (e.g., passed around). ConvertingdebouncedCaptureScreenshotto an arrow field eliminates that risk and simplifies reasoning.- // 10 second debounce - captureScreenshot = debounce( - this.debouncedCaptureScreenshot, - 10000, - ); + // 10 second debounce + captureScreenshot = debounce(this.debouncedCaptureScreenshot, 10000); @@ - private async debouncedCaptureScreenshot() { + private debouncedCaptureScreenshot = async () => { if (this.isCapturing) { return; } this.isCapturing = true; try { // If the screenshot was captured less than 30 minutes ago, skip capturing if (this.lastScreenshotAt) { const thirtyMinutesAgo = subMinutes(new Date(), 30); if (isAfter(this.lastScreenshotAt, thirtyMinutesAgo)) { return; } } const projectId = this.editorEngine.projectId; if (!projectId) { console.warn('Screenshot skipped: no projectId available'); return; } const result = await api.project.captureScreenshot.mutate({ projectId }); if (!result || !result.success) { throw new Error('Failed to capture screenshot'); } this.lastScreenshotAt = new Date(); } catch (error) { console.error('Error capturing screenshot', error); } finally { this.isCapturing = false; } - } + }Also applies to: 29-53
apps/web/client/src/app/project/[id]/_components/right-panel/chat-tab/chat-input/index.tsx (1)
255-262: Optional: add a stable id to image contexts to support reliable pill keys and preview stateIf you adopt a stable
idper image, the pill’skeycan avoid both collisions and index-based churn.const contextImage: ImageMessageContext = { type: MessageContextType.IMAGE, content: screenshotData, mimeType: mimeType, displayName: 'Screenshot', + // @ts-expect-error: augmenting shape for UI stability (can be formalized in the model) + id: (globalThis.crypto?.randomUUID?.() ?? String(Date.now())), };I can put together a small follow-up to formalize
idin theMessageContextmodel and update all usages.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (9)
apps/web/client/src/app/project/[id]/_components/right-panel/chat-tab/chat-input/index.tsx(1 hunks)apps/web/client/src/app/project/[id]/_components/right-panel/chat-tab/context-pills/draft-image-pill.tsx(4 hunks)apps/web/client/src/app/project/[id]/_components/right-panel/chat-tab/context-pills/input-context-pills.tsx(1 hunks)apps/web/client/src/components/store/editor/chat/context.ts(1 hunks)apps/web/client/src/components/store/editor/element/index.ts(1 hunks)apps/web/client/src/components/store/editor/frames/manager.ts(8 hunks)apps/web/client/src/components/store/editor/screenshot/index.tsx(1 hunks)apps/web/client/src/server/api/routers/project/project.ts(0 hunks)apps/web/preload/script/api/screenshot.ts(7 hunks)
💤 Files with no reviewable changes (1)
- apps/web/client/src/server/api/routers/project/project.ts
🧰 Additional context used
🧬 Code graph analysis (3)
apps/web/client/src/components/store/editor/chat/context.ts (1)
packages/models/src/chat/message/context.ts (1)
ImageMessageContext(28-31)
apps/web/client/src/app/project/[id]/_components/right-panel/chat-tab/chat-input/index.tsx (1)
packages/ui/src/components/sonner.tsx (1)
toast(19-19)
apps/web/client/src/components/store/editor/frames/manager.ts (1)
packages/db/src/schema/project/canvas/frame.ts (1)
frames(9-22)
🔇 Additional comments (10)
apps/web/client/src/components/store/editor/frames/manager.ts (7)
49-53: Initialize lastInteractionTime per frame (LGTM).Defaults to null is reasonable; it keeps the signal ephemeral and avoids DB changes.
71-76: Preserve lastInteractionTime on registerView (LGTM).Good guard against losing the signal when the view re-registers.
98-101: Selecting a frame updates lastInteractionTime (LGTM).This aligns the “active frame” heuristic with actual user intent.
171-173: Track interaction on navigation (LGTM).Makes sense to treat navigation as an interaction for “most recent frame” resolution.
225-227: Create: initialize lastInteractionTime to null (LGTM).Consistent with applyFrames.
258-267: Preserve lastInteractionTime during updates (LGTM).Prevents accidental loss of the interaction timestamp on partial updates.
298-321: Active frame helper adoption verified.
- getMostRecentlyInteractedFrame is called in apps/web/client/src/app/project/[id]/_components/right-panel/chat-tab/chat-input/index.tsx (line 226)
- updateLastInteraction is invoked on element clicks in apps/web/client/src/components/store/editor/element/index.ts (line 79)
- Internal usages of updateLastInteraction exist only in the frames manager (selection toggles at line 99 and navigation at line 172)
No additional call sites were found; the helpers are used exactly where intended.
apps/web/client/src/components/store/editor/element/index.ts (1)
69-80: Good call: log with frameId and update interaction on click.The clearer logs help triage; updating lastInteractionTime on element click keeps the “active frame” signal fresh.
apps/web/client/src/app/project/[id]/_components/right-panel/chat-tab/chat-input/index.tsx (2)
225-231: Good shift: target the most recently interacted frameSwitching to
editorEngine.frames.getMostRecentlyInteractedFrame()ensures screenshots reflect the user’s current focus. This matches the PR goal and should remove the ambiguity from iterating all frames. Nice.
236-248: Verify the data shape returned bycaptureScreenshot()matches consumers
handleImageEventstores a Data URL (e.g.,data:image/png;base64,...). Here we storeresult.datadirectly. IfcaptureScreenshot()returns raw base64 without thedata:prefix, previews and MIME sniffing may fail.Please confirm the return contract of
view.captureScreenshot():
- If it returns raw base64, wrap it into a Data URL using the resolved
mimeType.- If it already returns a Data URL, keep as-is.
Example adjustment if raw base64 is returned:
- screenshotData = result.data; - mimeType = result.mimeType || 'image/jpeg'; + mimeType = result.mimeType || 'image/jpeg'; + const raw = result.data; + screenshotData = raw.startsWith('data:') + ? raw + : `data:${mimeType};base64,${raw}`;
Fixed some rendering issues: centered fallback text properly, added projectId guard to prevent undefined API calls, fixed component name typo in console warning, and corrected text wrapping calculation to account for padding.
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 (3)
apps/web/preload/script/api/screenshot.ts (2)
224-235: Avoid canvas tainting by skipping non-CORS cross-origin images (prevents “Screenshot unavailable” fallback).Drawing cross-origin images without
crossOriginset will taint the canvas and causetoDataURLto throw, pushing the whole capture into the red “Screenshot unavailable” fallback—even when only one image caused it. Guard and render a placeholder for unsafe images to keep the rest of the screenshot.Apply this diff inside
renderElement’s image block:- if (element instanceof HTMLImageElement && element.complete && element.naturalWidth > 0) { - try { - context.drawImage(element, left, top, width, height); - } catch (error) { - context.fillStyle = '#f0f0f0'; - context.fillRect(left, top, width, height); - context.fillStyle = '#999999'; - context.font = '12px Arial, sans-serif'; - context.textAlign = 'center'; - context.fillText('Image', left + width / 2, top + height / 2); - } - } + if (element instanceof HTMLImageElement && element.complete && element.naturalWidth > 0) { + // Heuristic: avoid drawing non-CORS cross-origin images that would taint the canvas. + let safeToDraw = true; + try { + const src = element.currentSrc || element.src; + const url = new URL(src, document.baseURI); + const isCrossOrigin = url.origin !== window.location.origin; + const hasCors = !!element.crossOrigin; // 'anonymous' or 'use-credentials' + if (isCrossOrigin && !hasCors) safeToDraw = false; + } catch { + // If URL parsing fails, be conservative. + safeToDraw = false; + } + + if (safeToDraw) { + try { + context.drawImage(element, left, top, width, height); + } catch { + // Fallback placeholder + context.fillStyle = '#f0f0f0'; + context.fillRect(left, top, width, height); + context.fillStyle = '#999999'; + context.font = '12px Arial, sans-serif'; + context.textAlign = 'center'; + context.fillText('Image', left + width / 2, top + height / 2); + } + } else { + // Placeholder for skipped cross-origin image + context.fillStyle = '#f0f0f0'; + context.fillRect(left, top, width, height); + context.fillStyle = '#999999'; + context.font = '12px Arial, sans-serif'; + context.textAlign = 'center'; + context.fillText('Image', left + width / 2, top + height / 2); + } + }
188-223: Render form control values (inputs/textareas) instead of missing user-entered text.Current logic only renders text for elements with no children, which misses values in inputs and textareas. Include explicit handling so screenshots capture user intent.
Apply this diff to add form value rendering:
if (element.textContent && element.children.length === 0) { const text = element.textContent.trim(); if (text) { const fontSize = parseFloat(styles.fontSize) || 16; const fontFamily = styles.fontFamily || 'Arial, sans-serif'; const color = styles.color || '#000000'; @@ } } } + // Render input/textarea values + if (element instanceof HTMLInputElement || element instanceof HTMLTextAreaElement) { + const value = element.value; + if (value) { + const fontSize = parseFloat(styles.fontSize) || 16; + const fontFamily = styles.fontFamily || 'Arial, sans-serif'; + const color = styles.color || '#000000'; + context.fillStyle = color; + context.font = `${fontSize}px ${fontFamily}`; + context.textAlign = 'left'; + context.textBaseline = 'top'; + const lineHeight = fontSize * LINE_HEIGHT_MULTIPLIER; + const wrapWidth = Math.max(0, width - (TEXT_PADDING * 2) - TEXT_WRAP_MARGIN); + const words = value.split(' '); + let line = ''; + let y = top + TEXT_PADDING; + for (const word of words) { + const testLine = line + word + ' '; + const metrics = context.measureText(testLine); + if (metrics.width > wrapWidth && line !== '') { + context.fillText(line, left + TEXT_PADDING, y); + line = word + ' '; + y += lineHeight; + if (y > top + height) break; + } else { + line = testLine; + } + } + if (line && y <= top + height) { + context.fillText(line, left + TEXT_PADDING, y); + } + } + return; + }apps/web/client/src/app/project/[id]/_components/right-panel/chat-tab/context-pills/draft-image-pill.tsx (1)
8-18: Fix: forwarded ref is dropped and typed as HTMLDivElement while the DOM node is a span.
- The component advertises
forwardRef<HTMLDivElement>but renders a<motion.span>. That’s a type contract violation.- The forwarded
refparameter is unused, which can break consumers relying on it (focus, scroll-into-view, measurement).Option A (keep span): change the ref type to
HTMLSpanElementand merge the forwarded ref with the localpillRef.-import React, { useState, useRef } from 'react'; +import React, { useState, useRef, useEffect } from 'react'; @@ -export const DraftImagePill = React.forwardRef< - HTMLDivElement, +export const DraftImagePill = React.forwardRef< + HTMLSpanElement, @@ ->(({ context, onRemove }, ref) => { +>(({ context, onRemove }, ref) => { @@ - const pillRef = useRef<HTMLSpanElement>(null); + const pillRef = useRef<HTMLSpanElement>(null); @@ - ref={pillRef} + ref={(node) => { + pillRef.current = node; + if (typeof ref === 'function') { + ref(node); + } else if (ref) { + (ref as React.MutableRefObject<HTMLSpanElement | null>).current = node; + } + }}Option B (keep ref type): switch
<motion.span>to<motion.div>so the public ref type staysHTMLDivElement. I can supply that patch if you prefer to avoid a public type change.Also applies to: 88-104
🧹 Nitpick comments (5)
apps/web/preload/script/api/screenshot.ts (3)
23-39: Use devicePixelRatio for crisper text while keeping file size bounded by existing compression.Rendering at CSS pixel size produces blurry text on HiDPI displays. Scale the canvas by DPR and downscale via your compression step. Cap DPR to keep sizes predictable.
- // Create a canvas with viewport dimensions + // Create a canvas with viewport dimensions, scaled by DPR for clarity const canvas = document.createElement('canvas'); const context = canvas.getContext('2d'); if (!context) { throw new Error('Failed to get canvas context'); } - // Set canvas dimensions to viewport size - canvas.width = viewportWidth; - canvas.height = viewportHeight; + const dpr = Math.min(window.devicePixelRatio || 1, 2); + canvas.width = Math.floor(viewportWidth * dpr); + canvas.height = Math.floor(viewportHeight * dpr); + context.scale(dpr, dpr); - // Warm up canvas context for font rendering - context.fillText('', 0, 0); + // Warm up canvas context for font rendering (optional) + // context.fillText('', 0, 0);
147-151: Stabilize stacking order: tie-break equal z-index by DOM order.Pure z-index sort is insufficient and can reorder siblings unexpectedly (many are “auto” → 0). Add DOM order as a secondary key to reduce visual glitches.
- visibleElements.sort((a, b) => { - const aZIndex = parseInt(a.styles.zIndex) || 0; - const bZIndex = parseInt(b.styles.zIndex) || 0; - return aZIndex - bZIndex; - }); + visibleElements.sort((a, b) => { + const aZ = parseInt(a.styles.zIndex) || 0; + const bZ = parseInt(b.styles.zIndex) || 0; + if (aZ !== bZ) return aZ - bZ; + // Tie-breaker: document order + return a.element.compareDocumentPosition(b.element) & Node.DOCUMENT_POSITION_FOLLOWING ? -1 : 1; + });
78-117: Make size checks precise and avoid main-thread jank by using toBlob (optional).
- The size check currently includes the
data:image/jpeg;base64,header; strip it for accuracy.- Repeated
toDataURLcalls can stall the UI.toBlobis async and yields to the event loop.Minimal precision fix:
- const base64 = scaledCanvas.toDataURL('image/jpeg', quality); + const base64 = scaledCanvas.toDataURL('image/jpeg', quality); + const commaIdx = base64.indexOf(','); + const b64BodyLen = commaIdx >= 0 ? base64.length - (commaIdx + 1) : base64.length; - if (base64.length <= MAX_BASE64_SIZE) { + if (b64BodyLen <= MAX_BASE64_SIZE) { return base64; }Alternative (bigger refactor): replace the inner loop’s
toDataURLwith a helper that usestoBlob+FileReaderto produce a base64 string asynchronously. I can supply a full patch if you want it.apps/web/client/src/app/project/[id]/_components/right-panel/chat-tab/context-pills/draft-image-pill.tsx (2)
101-103: Remove localkeyon the root element.
keyshould be applied by the parent at the array level. Placing it here is redundant and can cause remounts ifdisplayNamechanges.- key={context.displayName}
36-87: Add basic accessibility: close on Escape and announce dialog semantics.The preview is modal-like. Add Escape-to-close and ARIA attributes for a better a11y baseline.
Minimal additions:
- Add an effect to close on Escape:
+ useEffect(() => { + if (!showPreview) return; + const onKeyDown = (e: KeyboardEvent) => { + if (e.key === 'Escape') setShowPreview(false); + }; + window.addEventListener('keydown', onKeyDown); + return () => window.removeEventListener('keydown', onKeyDown); + }, [showPreview]);
- Add
role="dialog" aria-modal="true" aria-label="Screenshot preview"to a wrapper around the preview image (or to the backdrop if you prefer). If you want, I can provide a small wrapper<motion.div>around the image with these attributes and keep your animation intact.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
apps/web/client/src/app/project/[id]/_components/right-panel/chat-tab/context-pills/draft-image-pill.tsx(4 hunks)apps/web/client/src/components/store/editor/screenshot/index.tsx(1 hunks)apps/web/preload/script/api/screenshot.ts(7 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/web/client/src/components/store/editor/screenshot/index.tsx
🔇 Additional comments (2)
apps/web/preload/script/api/screenshot.ts (1)
17-21: captureScreenshot correctly runs in the selected iframe’s context
I’ve confirmed that all screenshot calls use the Penpal proxy on the target frame, not the top-level document:• In chat-input/index.tsx (line 237), the code does
const targetFrame = editorEngine.frames.getMostRecentlyInteractedFrame(); const result = await targetFrame.view.captureScreenshot();so
captureScreenshot()executes inside that iframe’s injected preload script.
• In sandbox/index.ts (line 264) and project-breadcrumb.tsx (line 42), calls toeditorEngine.screenshot.captureScreenshot()also resolve to the selected frame’s view under the hood.
• There is no use offrame.contentWindow,webContents, or top-leveldocument—all IPC happens via the per-frame Penpal connection.Because
getMostRecentlyInteractedFrame()returns theFrameData.viewproxy into the iframe, the preload’scaptureScreenshot()always captures that frame’swindow.document. No changes needed here.apps/web/client/src/app/project/[id]/_components/right-panel/chat-tab/context-pills/draft-image-pill.tsx (1)
3-5: Confirm workspace dependencies for Motion librariesPlease verify that either Motion One (
motion) or Framer Motion (framer-motion) is declared in your workspace’s package manifests. We need to ensure the imports you’ve added won’t break the build.Run the following to check every
package.jsonin the repo:• Verified files:
apps/web/client/src/app/project/[id]/_components/right-panel/chat-tab/context-pills/draft-image-pill.tsx(importsmotion/react)- Various other components mix both
motion/react(Motion One) andframer-motionimports.• Next steps:
- Ensure at least one of these dependencies appears in your root or package-level
package.json:
"motion": "^<version>"(Motion One) to supportimport { motion } from 'motion/react'"framer-motion": "^<version>"(Framer Motion) to supportimport { motion } from 'framer-motion'- If both are needed (due to mixed usage), install both in the relevant package(s).
- After adding, run
pnpm install(ornpm install/yarn install) to update the lockfile.
| } | ||
| const result = await api.project.captureScreenshot.mutate({ projectId: this.editorEngine.projectId }); | ||
|
|
||
| const projectId = this.editorEngine.projectId; |
There was a problem hiding this comment.
Does this always exist anyway?
| onlyMainContent: true, | ||
| timeout: 10000, | ||
| // Optional: Add actions to click the button for CSB free tier | ||
| // actions: [ |
There was a problem hiding this comment.
Could we keep this comment? Even as a separate array that doesn't get used. This will be useful for people who don't have CSB pro
There was a problem hiding this comment.
You're right on both counts. I just checked the EditorEngine and it requires the projectId string parameter.
I'll keep the comment. I'm assuming we'll be testing out and adding it as a feature in the near future? (Going into the design tab and then manually clicking on the button sounds like a lot of inertia)
|
@Kitenite Hi Kiet! It's been a while since I touched this branch. Should I continue working on this by fixing the conflicts due to the more recent changes or leave it be? |
Description
Related Issues
related to #2676
Type of Change
Testing
I switched from Firecrawl to DOM screenshotting for frame-specific screenshots because Firecrawl can't target individual iframes, only entire pages. Since we needed screenshots of the specific frame the user was interacting with (
editorEngine.frames.getMostRecentlyInteractedFrame()), Firecrawl wasn't able to target it.The DOM-to-canvas approach targets exactly the frame we want. The downside is rendering quality: fonts, spacing, and styling don't translate perfectly to canvas. Better solutions like Playwright need extra dependencies and risky permissions. I prioritized frame targeting and security over perfect visuals.
I also fixed a bug where multiple identical screenshots would break the context array by getting the same base64 ID. Added smooth preview transitions because Firecrawl screenshots aren't perfect and I couldn't tell what they were capturing without previewing.
Firecrawl still handles project thumbnails. DOM approach works for chat screenshots.
Screenshots (if applicable)
Additional Notes
P.S. Apologies on the git diffs, I had to rebase 3 different times today as there were a lot of pushes to the repo :)
Although I was careful, this ended up being a much larger change than I initially expected, so I will keep iterating on it if anyone notices something wrong.
Important
Introduces frame-specific screenshots and preview functionality, fixes duplicate screenshot ID issues, and updates frame interaction tracking.
getMostRecentlyInteractedFrame()inFramesManager.DraftImagePill.ChatContext.captureScreenshot()inscreenshot.tsnow uses DOM-to-canvas rendering instead of Firecrawl.addImageContext()added toChatContextto handle image contexts.FramesManagertracks frame interactions withupdateLastInteraction()andgetMostRecentlyInteractedFrame().ElementsManagerupdates frame interaction on element click.chat-input/index.tsxandinput-context-pills.tsxto integrate new screenshot and preview features.This description was created by
for d6208c3. You can customize this summary. It will automatically update as commits are pushed.
Summary by CodeRabbit
New Features
Bug Fixes
Refactor