feat(ui): add in-app browser inspector pane - #239
Conversation
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
There was a problem hiding this comment.
Note
This report is out of date. Scroll down for Devin Review's latest report on this PR.
Devin Review found 4 potential issues.
3 flags not posted on this PR by your GitHub settings — view them in Devin Review. (Configure)
| useEffect(() => { | ||
| const host = hostRef.current | ||
| if (host === null) return | ||
| const observer = new ResizeObserver(() => syncLayout(showGuest)) | ||
| observer.observe(host) | ||
| syncLayout(showGuest) | ||
| return () => observer.disconnect() | ||
| }, [syncLayout, showGuest]) |
There was a problem hiding this comment.
🟡 Native browser covers renderer dialogs
When a modal opens over a loaded page, syncLayout keeps the native guest visible. Renderer dialogs become obscured and unclickable wherever they overlap it.
Learn more
A WebContentsView is a native child view, not an element in the renderer's stacking context. CSS z-index cannot place the command palette, content-search overlay, context menus, or other renderer dialogs above it. The current visibility calculation only considers whether the tab has a page, so those overlays leave the guest shown.
Example: Load https://example.com, then press Ctrl+K. The command palette opens in React, but the browser guest remains above the overlapping portion and consumes pointer input.
Recommended fix: Make guest visibility depend on both page state and shell overlay state. Hide it before opening any renderer overlay that can intersect the pane, then restore and resynchronize bounds after the overlay closes.
Was this helpful? React with 👍 or 👎 to provide feedback.
| tab.error = null | ||
| try { | ||
| await tab.guest.loadURL(resolved.url) | ||
| } catch (error: unknown) { | ||
| tab.error = error instanceof Error ? error.message : String(error) | ||
| } | ||
| this.#emit(id) | ||
| return { ok: true, tab: this.snapshot(id) } |
There was a problem hiding this comment.
🟡 Overlapping navigation leaves stale errors
Overlapping navigate calls let an older loadURL rejection overwrite tab.error after the newer call clears it. The successful page retains the stale error.
Learn more
Electron aborts an in-flight loadURL when another navigation supersedes it. Each call currently writes the shared error field without checking whether it is still the newest request. A later successful call emits state but does not clear an error written by the older call after its initial clear.
Example: Submit URL A and immediately submit URL B. B clears the error, A rejects with ERR_ABORTED and stores it, then B succeeds while that error remains visible.
Recommended fix: Track a navigation generation per tab. Only the latest generation can commit success or failure, and clear its error again when that generation succeeds.
Was this helpful? React with 👍 or 👎 to provide feedback.
| if (/^[a-z][a-z0-9+.-]*:\/\//i.test(trimmed)) { | ||
| try { | ||
| const parsed = new URL(trimmed) | ||
| if (!NAVIGABLE.has(parsed.protocol)) { | ||
| return { ok: false, error: `${parsed.protocol} URLs cannot open in the in-app browser` } | ||
| } | ||
| return { ok: true, url: parsed.href } | ||
| } catch { | ||
| return { ok: false, error: 'Enter an http(s) URL' } | ||
| } | ||
| } | ||
| const candidate = `${isLoopbackHost(trimmed) ? 'http' : 'https'}://${trimmed}` |
There was a problem hiding this comment.
🟡 Bare mail links become websites
Entering mailto:hi@example.com makes resolveBrowserUrl prefix HTTPS instead of rejecting the scheme. The browser navigates to example.com with mailto:hi as credentials.
Learn more
The explicit-scheme branch recognizes only schemes followed by ://. Schemes whose normal syntax omits slashes fall through to bare-host normalization. Some then form valid but unrelated HTTPS URLs because URL parsing interprets the original scheme text as credentials.
Example: mailto:hi@example.com becomes https://mailto:hi@example.com/. The expected result is an error stating that mailto: cannot open in the in-app browser.
Recommended fix: Detect unsupported URI schemes before adding a default protocol, while preserving host-and-port inputs such as localhost:5173. Add resolve-level tests for mailto:, javascript:, and data: forms.
Was this helpful? React with 👍 or 👎 to provide feedback.
| function isLoopbackHost(value: string): boolean { | ||
| const host = value.split('/')[0]?.split(':')[0]?.toLowerCase() ?? '' | ||
| return host === 'localhost' || host === '127.0.0.1' || host === '[::1]' || host === '::1' | ||
| } |
There was a problem hiding this comment.
🟡 IPv6 loopback receives HTTPS
Entering [::1]:5173 makes isLoopbackHost read [ and choose HTTPS. HTTP-only IPv6 development servers cannot load.
Learn more
An IPv6 address contains colons inside brackets, so splitting the authority on : cannot isolate its hostname. Although the comparison includes [::1] and ::1, neither value can survive the split used to produce host.
Example: A local server listens at http://[::1]:5173. Entering [::1]:5173 resolves to https://[::1]:5173/, and the TLS connection fails.
Recommended fix: Parse bracketed IPv6 authorities separately or parse a temporary URL and inspect hostname. Add tests for [::1], [::1]:5173, localhost, and 127.0.0.1:5173.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Devin Review found 10 new potential issues.
4 flags not posted on this PR by your GitHub settings — view them in Devin Review. (Configure)
| try { | ||
| await tab.guest.loadURL(resolved.url) | ||
| } catch (error: unknown) { | ||
| tab.error = error instanceof Error ? error.message : String(error) | ||
| } | ||
| this.#emit(id) | ||
| return { ok: true, tab: this.snapshot(id) } |
There was a problem hiding this comment.
🟡 Failed navigation reports success
When loadURL rejects, navigate records the error but returns ok: true. MCP reports success, so agents continue against the previous page.
Learn more
BrowserService.navigate uses a discriminated result so callers can distinguish expected navigation failures from a loaded page. Electron rejects webContents.loadURL for failures such as DNS, TLS, or aborted main-frame loads. The catch records that rejection in tab.error, but the unconditional return still selects the success variant. dispatchBrowserMcpTool consequently emits a normal MCP result rather than isError: true.
Example: Navigating to https://does-not-resolve.invalid rejects loadURL. The tab keeps its prior URL and stores the error, but the tool result reports success with that stale tab.
Recommended fix: Return { ok: false, error: tab.error } from the catch path, emit the updated state, and reserve { ok: true, tab } for a fulfilled loadURL.
Was this helpful? React with 👍 or 👎 to provide feedback.
| const restoreDraft = (): void => { | ||
| if (notes.length > 0) setReviewNotes((prev) => (prev.length === 0 ? notes : prev)) | ||
| if (picks.length > 0) restoreBrowserPicks(picks) | ||
| setComposerSeed((prev) => ({ text, files, nonce: (prev?.nonce ?? 0) + 1 })) |
There was a problem hiding this comment.
🟡 Rejected sends duplicate pick screenshots
When a picked-element send is rejected, restoreDraft restores both the pick and its screenshot as a composer file. Retrying attaches that screenshot twice.
Learn more
files contains both the user's composer images and the screenshots collected from browser picks. On rejection, the pick is restored to the browser-pick store and the same combined array is restored into the composer. handleSend then prepends the restored pick images to those composer files on the retry.
Example: One picked button contributes button.png. A rejected dispatch restores button.png in the pick and composer stores. The retry stages two copies of button.png.
Recommended fix: Keep user-selected composer files separate from pick-derived screenshots through dispatchSend. Restore the pick objects and only the original composer files after rejection. Apply the same separation to the image-staging failure path.
Was this helpful? React with 👍 or 👎 to provide feedback.
| const [reviewNotes, setReviewNotes] = useState< | ||
| { path: string; line: number | null; text: string }[] | ||
| >([]) | ||
| const elementPicks = useSyncExternalStore(subscribeBrowserPicks, browserPicksState) |
There was a problem hiding this comment.
🟡 Picks leak across split sessions
With multiple SessionViews mounted, every composer reads the same process-wide elementPicks store. Sending from any pane consumes all picks for that session.
Learn more
The browser-pick store has one module-level array with no session or pane key. Split layouts mount several SessionView instances simultaneously, and each subscribes to that same snapshot. takeBrowserPicks clears the global array when any composer sends.
Example: Sessions Alpha and Beta are visible side by side. A button picked while Alpha is focused appears under both composers. Sending a message from Beta consumes the button and sends its context to Beta.
Recommended fix: Associate picks with the focused session or pane when they are created. Expose keyed subscribe/read/take/restore operations, and render and consume only the current sessionId's picks.
Was this helpful? React with 👍 or 👎 to provide feedback.
| void stageImages(allFiles).then( | ||
| (attachments) => dispatchSend(text, attachments, allFiles), |
There was a problem hiding this comment.
🟡 Pending sends consume later picks
While image staging runs, dispatchSend takes browser picks only after staging finishes. Picks changed meanwhile alter the earlier message without matching its attachments.
Learn more
handleSend reads the current pick images before starting stageImages, but it leaves the picks in the global store. The asynchronous continuation later calls dispatchSend, which invokes takeBrowserPicks against the then-current store. The element context and staged screenshots can therefore describe different selections.
Example: A user sends pick A, then selects pick B before A's screenshot finishes staging. The first message can consume both A and B's text context while attaching only A's screenshot.
Recommended fix: Atomically take and snapshot the picks when send begins. Pass that snapshot through staging and dispatch, then restore exactly that snapshot on failure without overwriting newer picks.
Was this helpful? React with 👍 or 👎 to provide feedback.
| ' for (let i = 0; i < 4 && cur && cur.nodeType === 1 && cur !== document.documentElement; i++) {', | ||
| ' let part = cur.tagName.toLowerCase()', | ||
| ' if (cur.classList && cur.classList.length) {', | ||
| " part += '.' + Array.from(cur.classList).slice(0, 2).map(function(c) { return CSS.escape(c) }).join('.')", | ||
| ' }', | ||
| ' parts.unshift(part)', | ||
| ' cur = cur.parentElement', | ||
| ' }', | ||
| " return parts.join(' > ')", |
There was a problem hiding this comment.
🟡 Repeated elements share snapshot selectors
cssPath omits sibling indices, so repeated interactive siblings receive identical selectors. browser_click then activates the first sibling instead of the snapshotted element.
Learn more
Browser snapshots give the agent a selector for each interactive element. The picker version of cssPath adds :nth-of-type(...) when siblings share a tag, but the snapshot version does not. browser_click resolves selectors with document.querySelector, which always chooses the first match.
Example: Three <button class="item"> siblings all snapshot as div > button.item. Asking the agent to click the third row clicks the first row.
Recommended fix: Share one selector builder between picker and snapshot scripts, including sibling disambiguation at every path segment. Add a test fixture with repeated same-tag, same-class siblings and verify selectors resolve back to their original nodes.
Was this helpful? React with 👍 or 👎 to provide feedback.
| }), | ||
| 'terminal.kill': z.object({ id: z.string().min(1) }), | ||
| 'browser.open': z.object({ id: z.string().min(1), url: z.string().max(2048).optional() }), | ||
| 'browser.navigate': z.object({ id: z.string().min(1), url: z.string().min(1).max(2048) }), |
There was a problem hiding this comment.
🟡 Empty address cannot open blank page
An empty address reaches browser.navigate, but its schema rejects it before resolveBrowserUrl maps it to about:blank. Clearing the bar leaves the current page open.
Learn more
The URL resolver defines an empty string as the blank tab, and the address form submits its current text unchanged. Parameter validation runs before the main-process handler, so .min(1) makes that supported resolver branch unreachable through RPC.
Example: On https://example.com, the user clears the address and presses Enter. IPC validation rejects the request, and the existing page remains instead of loading about:blank.
Recommended fix: Permit the empty string in this schema, as shown, while retaining the length cap. Keep resolveBrowserUrl responsible for mapping it to about:blank.
| 'browser.navigate': z.object({ id: z.string().min(1), url: z.string().min(1).max(2048) }), | |
| 'browser.navigate': z.object({ id: z.string().min(1), url: z.string().max(2048) }), |
Was this helpful? React with 👍 or 👎 to provide feedback.
| function readBody(req: IncomingMessage): Promise<string> { | ||
| return new Promise((resolve, reject) => { | ||
| const chunks: Buffer[] = [] | ||
| req.on('data', (chunk: Buffer) => chunks.push(chunk)) | ||
| req.on('end', () => resolve(Buffer.concat(chunks).toString('utf8'))) | ||
| req.on('error', reject) | ||
| }) |
There was a problem hiding this comment.
🟨 MCP requests have no size limit
An authorized client can stream an unlimited body into readBody. The Electron main process buffers it entirely, enabling memory exhaustion.
Was this helpful? React with 👍 or 👎 to provide feedback.
| new AcpDriver( | ||
| candidate.kind, | ||
| launch, | ||
| candidate.make(detection.binaryPath), | ||
| (wall) => publishAuthWall(candidate.kind, wall), | ||
| () => browserMcpServers, |
There was a problem hiding this comment.
🟨 All agents receive browser control credentials
Every ACP turn receives the browser MCP token automatically. Any provider process can inspect and control the persistent browser session without user opt-in.
Was this helpful? React with 👍 or 👎 to provide feedback.
| const view = new WebContentsView({ | ||
| webPreferences: { | ||
| sandbox: true, | ||
| contextIsolation: true, | ||
| nodeIntegration: false, | ||
| partition: BROWSER_PARTITION, | ||
| }, |
There was a problem hiding this comment.
🟨 Browser credentials persist across sessions
All guests share one persistent partition, so cookies survive restarts and cross projects and providers. Later agents can inherit earlier authenticated sessions.
Was this helpful? React with 👍 or 👎 to provide feedback.
| const candidate = `${isLoopbackHost(trimmed) ? 'http' : 'https'}://${trimmed}` | ||
| try { | ||
| const parsed = new URL(candidate) | ||
| if (!NAVIGABLE.has(parsed.protocol) || parsed.hostname.length === 0) { | ||
| return { ok: false, error: 'Enter an http(s) URL' } | ||
| } | ||
| return { ok: true, url: parsed.href } |
There was a problem hiding this comment.
🟨 Browser tools reach private network services
Agent-controlled navigation accepts arbitrary HTTP and HTTPS hosts, including loopback and private addresses. Agents can probe and operate local administrative services.
Was this helpful? React with 👍 or 👎 to provide feedback.
b6c2a06 to
0a0bba5
Compare
2 participants
Second slice of in-app browser support: agents can drive the same visible page, and the user can pick elements into the composer.
What changed
browser_navigate/snapshot/click/type/status.Why
ACP has no native browser methods. The client hosts a real Chromium view and hands it to every provider as MCP tools. The previous shell-wide
will-navigatehandler cancelled those loads.How verified
VITEST_MAX_WORKERS=2 pnpm verifygreen (typecheck + lint + tests).Desktop: 1422 passed, 3 skipped.