Add browser inspect prompt and composer image polish#257
Merged
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds per-selection keys and pinned selection state to injected inspect-mode, exposes overlay-visibility and keyed clear APIs, extends host/webview types and BrowserTab handle for rect-based captures and inspect controls, introduces attachScreenshotToComposer, adds a deferred InspectPromptOverlay flow for user text before attaching element-scoped screenshots, and wires a guest toggle shortcut. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant Inject as Injected Inspect JS
participant BrowserPanel
participant BrowserTab
participant Overlay as InspectPromptOverlay
participant Composer
User->>Inject: click element -> captureElement() returns {rect, selectionKey}
Inject-->>BrowserPanel: emit ElementSelectedEvent (rect, selectionKey)
BrowserPanel->>BrowserPanel: store pendingInspection + webviewBounds
BrowserPanel->>Overlay: render prompt anchored to translated rect
User->>Overlay: submit text
Overlay->>BrowserPanel: onSubmit(text)
BrowserPanel->>BrowserTab: setInspectOverlaysVisible(false)
BrowserTab->>Inject: inject hide overlays IIFE
BrowserPanel->>BrowserTab: captureScreenshot(rect)
BrowserTab->>Inject: capturePage(rect) -> dataURL
BrowserPanel->>BrowserTab: setInspectOverlaysVisible(true)
BrowserTab->>Inject: inject show overlays IIFE
BrowserPanel->>Composer: appendDraft + addInspectedElement
BrowserPanel->>Composer: attachScreenshotToComposer(dataURL, label)
BrowserPanel->>BrowserTab: clearInspectSelection(selectionKey)
BrowserTab->>Inject: inject clear-selection IIFE (expectedSelectionKey)
BrowserPanel->>BrowserPanel: clear pendingInspection
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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: 2
🧹 Nitpick comments (1)
apps/web/src/features/browser/ui/InspectPromptOverlay.tsx (1)
137-157: Derive the overlay position instead of syncing it through effects.Line 137 introduces
placementas local state even though it is fully derived fromanchor, and Lines 144-157 immediately synchronize that derived value plus the reset state in effects. That adds an avoidable extra render on every retarget and is the same patternreact-hooks/set-state-in-effectis warning about here.placementcan be computed during render (anchor ? placeOverlay(anchor) : null), and the textarea reset can come from keying the overlay by the inspection id in the parent instead of effect-drivensetState.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/features/browser/ui/InspectPromptOverlay.tsx` around lines 137 - 157, The placement state in InspectPromptOverlay (placement / setPlacement) is derived from anchor via placeOverlay and is unnecessarily synchronized in a useLayoutEffect; compute placement during render using const placement = anchor ? placeOverlay(anchor) : null and remove the placement useState and its useLayoutEffect; also remove the useEffect that resets setText/setIsSubmitting on event and instead have the parent key the overlay by the inspection id (or event identity) so a new key causes React to remount the overlay and reset local textarea state automatically, leaving only anchor, placeOverlay, setText, setIsSubmitting, and event references to guide the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/web/src/features/browser/ui/BrowserPanel.tsx`:
- Around line 74-79: The pendingInspection flow currently snapshots
getWebviewBounds() into PendingInspection.webviewBounds and passes that frozen
rect to InspectPromptOverlay, causing the prompt to drift when layout changes;
change PendingInspection to omit or keep webviewBounds null (store only id,
event, tabId), stop persisting the frozen bounds in pendingInspection, and
instead call getWebviewBounds() at render time (or recompute/update bounds on
layout/resizes) when creating/rendering InspectPromptOverlay so the overlay
always receives live webview bounds; update any code that constructs
pendingInspection (references to pendingInspection and getWebviewBounds) and the
InspectPromptOverlay prop usage to read current bounds rather than the stored
rect.
In `@apps/web/src/features/session/ui/PastedImageCard.tsx`:
- Around line 67-77: The className strings on the remove-button (the m.div
wrapper/button with X) and the DialogContent component contain hardcoded color
utilities like bg-black/72, rgba(...) shadows, and hover:bg-black/5; replace
each hardcoded color or rgba shadow with the corresponding theme CSS
variable/token classes (e.g., overlay background token, theme shadow token,
hover-bg token) so the component is theme-safe, updating the className values on
the remove button and DialogContent accordingly; leave the intentional
image-edge exception border-black/10 and dark:border-white/10 unchanged and
ensure no other hardcoded color literals remain in the m.div/button,
DialogContent, or nearby className strings.
---
Nitpick comments:
In `@apps/web/src/features/browser/ui/InspectPromptOverlay.tsx`:
- Around line 137-157: The placement state in InspectPromptOverlay (placement /
setPlacement) is derived from anchor via placeOverlay and is unnecessarily
synchronized in a useLayoutEffect; compute placement during render using const
placement = anchor ? placeOverlay(anchor) : null and remove the placement
useState and its useLayoutEffect; also remove the useEffect that resets
setText/setIsSubmitting on event and instead have the parent key the overlay by
the inspection id (or event identity) so a new key causes React to remount the
overlay and reset local textarea state automatically, leaving only anchor,
placeOverlay, setText, setIsSubmitting, and event references to guide the
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: defaults
Review profile: CHILL
Plan: Pro
Run ID: d4c7b9b3-890e-4b0c-a1c4-a1fde5af4b80
📒 Files selected for processing (10)
apps/web/src/features/browser/automation/inject/inspect-mode.jsapps/web/src/features/browser/automation/inspect-mode.tsapps/web/src/features/browser/lib/attachScreenshotToComposer.tsapps/web/src/features/browser/types.tsapps/web/src/features/browser/ui/BrowserPanel.tsxapps/web/src/features/browser/ui/BrowserTab.tsxapps/web/src/features/browser/ui/InspectPromptOverlay.tsxapps/web/src/features/browser/webview-manager.tsapps/web/src/features/session/ui/MessageInput.tsxapps/web/src/features/session/ui/PastedImageCard.tsx
Owner
Author
Greenlight — Round 1Fixed (2): live webview bounds are now resolved at render time for the inspect prompt; |
There was a problem hiding this comment.
🧹 Nitpick comments (3)
apps/web/src/features/browser/ui/BrowserPanel.tsx (3)
594-647: Consider adding error logging for silent failures.If
appendDraft,addInspectedElement, or the screenshot capture throws, thefinallyblock clears the inspection but the error is swallowed silently. The user might not realize their inspection data wasn't attached.♻️ Optional: add error logging
try { // 1. User-typed prompt — appended (preserves any in-progress draft). sessionComposerActions.appendDraft(sid, text); // ... rest of try block + } catch (err) { + console.error("[BrowserPanel] Inspect prompt submission failed:", err); } finally { clearPendingInspection(inspection); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/features/browser/ui/BrowserPanel.tsx` around lines 594 - 647, The try/finally in handleInspectPromptSubmit swallows errors from sessionComposerActions.appendDraft, sessionComposerActions.addInspectedElement, handle.captureScreenshot, or attachScreenshotToComposer so failures are silent; update the handler to catch errors, log them (include operation context and inspection ref/tabId) and surface a user-visible error (or rethrow) before the finally clears the inspection. Specifically wrap the body where appendDraft/addInspectedElement/captureScreenshot/attachScreenshotToComposer are called with a catch that calls your logging facility (e.g., console.error or app logger) and triggers a UI error/notification, referencing handleInspectPromptSubmit, sessionComposerActions.appendDraft, sessionComposerActions.addInspectedElement, handle.captureScreenshot, attachScreenshotToComposer, and ensure clearPendingInspection(inspection) still runs in finally.
664-666: Ref access during render — won't react to layout changes.ESLint correctly flags that reading
tabRefs.currentduring render violates the refs-outside-render guideline. While this approach gives you "fresh" bounds on each render, it won't trigger a re-render when the webview resizes (splitter drag, DevTools dock). The overlay could drift if the user resizes while the prompt is open.If you want the overlay to stay anchored through layout changes, consider tracking bounds in state via a
ResizeObserver:♻️ Optional: reactive bounds via ResizeObserver
const [pendingInspectionBounds, setPendingInspectionBounds] = useState<Bounds | null>(null); useEffect(() => { if (!pendingInspection) { setPendingInspectionBounds(null); return; } const handle = tabRefs.current.get(pendingInspection.tabId); if (!handle?.getWebviewBounds) return; // Initial read setPendingInspectionBounds(handle.getWebviewBounds()); // Observe resize of the tab host container const ro = new ResizeObserver(() => { setPendingInspectionBounds(handle.getWebviewBounds()); }); if (tabHostEl) ro.observe(tabHostEl); return () => ro.disconnect(); }, [pendingInspection, tabHostEl]);If layout-change drift is acceptable for this flow (prompt is typically short-lived), you can suppress with
// eslint-disable-next-line react-hooks/refs.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/features/browser/ui/BrowserPanel.tsx` around lines 664 - 666, Reading tabRefs.current.get(...) during render (creating pendingInspectionBounds from pendingInspection and tabRefs) violates refs-outside-render and won't update on layout changes; instead, track bounds in state and update them with a ResizeObserver inside an effect: create state pendingInspectionBounds via useState, in a useEffect keyed on pendingInspection and tabHostEl read the handle = tabRefs.current.get(pendingInspection.tabId), set initial bounds from handle.getWebviewBounds(), and attach a ResizeObserver to tabHostEl that calls setPendingInspectionBounds(handle.getWebviewBounds()) on resize (cleanup disconnect on unmount); alternatively, if drift is acceptable, explicitly suppress the lint rule with // eslint-disable-next-line react-hooks/refs.
657-662: ESLint flagssetStateinside effect — intentional synchronization, but consider restructuring.The linter warns about calling
clearPendingInspection(which callssetState) inside the effect body. While this pattern is technically a "sync effect" (responding to external state changes like tab switches or selector deactivation), it can cause cascading renders.This is acceptable here since the
setStateis conditional and guards against redundant updates. To silence the lint error without changing behavior, you could wrap the call in a microtask:♻️ Optional: defer to avoid lint warning
useEffect(() => { if (!pendingInspection) return; if (pendingInspection.tabId !== activeTabId || !activeSelectorActive) { - clearPendingInspection(pendingInspection); + queueMicrotask(() => clearPendingInspection(pendingInspection)); } }, [pendingInspection, activeTabId, activeSelectorActive, clearPendingInspection]);Alternatively, you could add an eslint-disable comment if the current synchronous behavior is preferred.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/features/browser/ui/BrowserPanel.tsx` around lines 657 - 662, The useEffect is calling clearPendingInspection (which triggers setState) directly causing an ESLint warning; to silence the lint while preserving behavior, defer the call to clearPendingInspection by wrapping it in a microtask (e.g., Promise.resolve().then(() => clearPendingInspection(pendingInspection))) inside the existing useEffect that depends on pendingInspection, activeTabId and activeSelectorActive; keep the existing conditional (pendingInspection.tabId !== activeTabId || !activeSelectorActive) and reference the same symbols (useEffect, pendingInspection, activeTabId, activeSelectorActive, clearPendingInspection) so renders are unchanged but the linter warning is resolved.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@apps/web/src/features/browser/ui/BrowserPanel.tsx`:
- Around line 594-647: The try/finally in handleInspectPromptSubmit swallows
errors from sessionComposerActions.appendDraft,
sessionComposerActions.addInspectedElement, handle.captureScreenshot, or
attachScreenshotToComposer so failures are silent; update the handler to catch
errors, log them (include operation context and inspection ref/tabId) and
surface a user-visible error (or rethrow) before the finally clears the
inspection. Specifically wrap the body where
appendDraft/addInspectedElement/captureScreenshot/attachScreenshotToComposer are
called with a catch that calls your logging facility (e.g., console.error or app
logger) and triggers a UI error/notification, referencing
handleInspectPromptSubmit, sessionComposerActions.appendDraft,
sessionComposerActions.addInspectedElement, handle.captureScreenshot,
attachScreenshotToComposer, and ensure clearPendingInspection(inspection) still
runs in finally.
- Around line 664-666: Reading tabRefs.current.get(...) during render (creating
pendingInspectionBounds from pendingInspection and tabRefs) violates
refs-outside-render and won't update on layout changes; instead, track bounds in
state and update them with a ResizeObserver inside an effect: create state
pendingInspectionBounds via useState, in a useEffect keyed on pendingInspection
and tabHostEl read the handle = tabRefs.current.get(pendingInspection.tabId),
set initial bounds from handle.getWebviewBounds(), and attach a ResizeObserver
to tabHostEl that calls setPendingInspectionBounds(handle.getWebviewBounds()) on
resize (cleanup disconnect on unmount); alternatively, if drift is acceptable,
explicitly suppress the lint rule with // eslint-disable-next-line
react-hooks/refs.
- Around line 657-662: The useEffect is calling clearPendingInspection (which
triggers setState) directly causing an ESLint warning; to silence the lint while
preserving behavior, defer the call to clearPendingInspection by wrapping it in
a microtask (e.g., Promise.resolve().then(() =>
clearPendingInspection(pendingInspection))) inside the existing useEffect that
depends on pendingInspection, activeTabId and activeSelectorActive; keep the
existing conditional (pendingInspection.tabId !== activeTabId ||
!activeSelectorActive) and reference the same symbols (useEffect,
pendingInspection, activeTabId, activeSelectorActive, clearPendingInspection) so
renders are unchanged but the linter warning is resolved.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: cf328277-8c19-4920-8c73-567941fd381c
📒 Files selected for processing (2)
apps/web/src/features/browser/ui/BrowserPanel.tsxapps/web/src/features/session/ui/PastedImageCard.tsx
Owner
Author
Greenlight — Round 2Fixed (2): stale inspect selections are now invalidated when the webview viewport changes, so the prompt cannot submit against a reflowed page; Escape in the inspect prompt now dismisses the prompt without also exiting browser focus mode. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/web/src/features/browser/ui/BrowserPanel.tsx`:
- Around line 574-584: The handleElementSelected callback is referencing
setPendingInspectionBounds before its hook is declared, violating React hooks
ordering; move the useState declaration for pendingInspectionBounds (the
useState<Bounds | null>(null) that returns [pendingInspectionBounds,
setPendingInspectionBounds]) so it appears above the handleElementSelected
function definition, ensuring setPendingInspectionBounds is declared before
being captured by handleElementSelected and keeping existing behavior of
setPendingInspection and nextInspectionIdRef.current increment.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 98aacf76-2bdb-4dc3-885a-d249659dd902
📒 Files selected for processing (2)
apps/web/src/features/browser/ui/BrowserPanel.tsxapps/web/src/features/browser/ui/InspectPromptOverlay.tsx
Owner
Author
Greenlight — Round 3Fixed (0): none. |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/desktop/preload/browser-preload.ts`:
- Around line 52-59: The keydown handler in browser-preload.ts should ignore
repeated keydown events so Cmd/Ctrl+Shift+D (inspect toggle) doesn't flip twice;
add a guard that returns early when e.repeat is true before deciding and routing
the shortcut (the handler block that computes `meta`, `shortcut` and `key`),
ensuring the repeat check runs prior to setting or sending the
`"toggle-inspect-mode"` shortcut.
In `@apps/web/src/features/browser/ui/BrowserPanel.tsx`:
- Line 74: The tooltip always shows a macOS glyph string but the keyboard
handler supports Ctrl+Shift+D on Windows/Linux; change the INSPECT_SHORTCUT
constant to be platform-aware (e.g., detect macOS via navigator.platform or
navigator.userAgent and set INSPECT_SHORTCUT = "⌘⇧D" for macOS else "Ctrl⇧D")
and replace any hard-coded usages with that constant (referenced symbol:
INSPECT_SHORTCUT) so all tooltip locations (including the other places flagged)
display the correct shortcut per OS.
- Around line 76-78: The inspect shortcut is being retriggered on held keys
because repeated keydown events aren't filtered; update isInspectShortcut to
ignore auto-repeats by returning false when the KeyboardEvent.repeat flag is
true (i.e., check e.repeat) in addition to the existing modifiers and key checks
so holding Cmd/Ctrl+Shift+D won't flip inspect mode repeatedly; modify the
isInspectShortcut function to include this repeat check.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2013d7a4-a3f0-4e29-b9f9-fd8928981c73
📒 Files selected for processing (5)
apps/desktop/preload/browser-preload.tsapps/web/src/features/browser/types.tsapps/web/src/features/browser/ui/BrowserPanel.tsxapps/web/src/features/browser/ui/BrowserTab.tsxapps/web/src/features/browser/ui/InspectPromptOverlay.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/web/src/features/browser/types.ts
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/web/src/features/session/lib/parseInspectTags.ts`:
- Around line 65-71: normalizeInspectMarkup is decoding `>` globally which
turns serialized attribute values (e.g. `path="body > div"`) into raw `>`
before tagRegex runs and also mutates plain-text; remove the final
.replace(/>/g, ">") so only the inspect tag markers are unescaped (keep the
.replace(/<(\/?)inspect/g, "<$1inspect") and the specific
.replace(/<\/inspect>/g, "</inspect>")), ensuring `>` remains encoded
in the source string and letting unescapeAttr handle decoding after tagRegex
matches the tag.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 97a0035c-99af-43aa-9987-83e1d3788d57
📒 Files selected for processing (4)
apps/backend/src/services/gh.service.tsapps/backend/test/unit/services/gh.service.test.tsapps/web/src/features/session/lib/parseInspectTags.tsapps/web/src/features/session/ui/blocks/TextBlock.tsx
Owner
Author
Greenlight — Round 2Fixed (4): ignored key-repeat for guest shortcut forwarding, ignored key-repeat for host shortcut handling, made the inspect shortcut label platform-aware, and fixed escaped inspect tag parsing so encoded attribute/text values stay intact until unescape. Already addressed / stale (2): live inspect prompt bounds were already handled via pendingInspectionBounds refresh, and PastedImageCard non-exempt colors are already tokenized. Disagreed (1): setPendingInspectionBounds declaration-order concern is a false positive; the callback only closes over the setter and typecheck passes. Validation: bun run typecheck; focused eslint for parser/preload; raw + escaped inspect parser smoke test. |
This was referenced May 8, 2026
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
1 participant
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.
Adds the inline browser inspect prompt flow, including pinned selection state, prompt anchoring, padded element screenshots, and shared screenshot-to-composer attachment handling. It also hardens inspect cleanup across tab and selector changes while keeping the selected element visible during prompt entry. On the composer side, staged content is split into separate pill, image, and pasted-text rows, and pasted images now open in a cleaner preview dialog with better framing, controls, and image handling. Validation: bunx tsc --noEmit --project tsconfig.web.json and targeted ESLint on the touched files.
Summary by CodeRabbit
New Features
UI Improvements
Bug Fixes