Ade Browser#243
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
|
@copilot review but do not make fixes |
📝 WalkthroughWalkthroughAdds a built-in browser service (Electron CDP-backed) with IPC/preload/renderer bindings, CLI browser commands, chat integration for browser context and screenshots, and replaces the right-edge floating pane with a persistent work sidebar; also threads laneId through App Control and iOS simulator sessions. ChangesBuilt-In Browser Service
Work Sidebar Reorganization
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested labels
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
Here's the review. No fixes made. OverallSolid feature — the layered security model (URL filtering at both renderer and main, Security / Correctness1. 2. 3. Debugger state is shared across all tabs 4. Architecture / Maintainability5. Massive single-file components 6. Renderer redeclares types that already exist in 7. 8. Minor9. 10. Double 11. |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
apps/desktop/src/renderer/components/files/FilesPage.tsx (1)
1279-1308:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon't seed
workspaceIdfrom a stale lane selection.This effect only runs once, so if
selectedLaneIdFromStorehydrates or changes beforelistWorkspaces()resolves, the callback still uses the lane from the first render. That can lock the page onto the wrong workspace, and the fallback at Lines 1318-1322 will not repair it onceworkspaceIdis non-empty.Possible fix
+ const selectedLaneIdRef = useRef(selectedLaneId); + useEffect(() => { + selectedLaneIdRef.current = selectedLaneId; + }, [selectedLaneId]); + useEffect(() => { const startedAt = performance.now(); logRendererDebugEvent("renderer.files.list_workspaces.begin"); window.ade.files.listWorkspaces() .then((items) => { @@ setWorkspaceId((current) => { if (current && items.some((workspace) => workspace.id === current)) return current; if (current) { setOpenTabs([]); setActiveTabPath(null); setSelectedNodePath(null); } - return defaultFilesWorkspaceId(items, selectedLaneId); + return defaultFilesWorkspaceId(items, selectedLaneIdRef.current); }); })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/components/files/FilesPage.tsx` around lines 1279 - 1308, The then-handler for window.ade.files.listWorkspaces() captures a stale selectedLaneId from the initial render which can cause setWorkspaceId to pick the wrong workspace; change the handler to read the latest lane selection (e.g., keep selectedLaneId in a ref like selectedLaneIdRef that you update when selectedLaneId changes) and use that ref inside the listWorkspaces().then(...) callback when calling defaultFilesWorkspaceId and deciding whether to clear tabs; update references to selectedLaneId in the setWorkspaceId callback to use the ref (or otherwise obtain the current selectedLaneId at resolution time) so workspace selection is based on the most recent lane.apps/desktop/src/main/services/appControl/appControlService.ts (1)
1360-1368:⚠️ Potential issue | 🟠 Major | ⚡ Quick winLaunch lane resolution ignores explicit
launchArgs.laneIdwhen no resolver is provided.Line 1360 uses only
args.resolveLaneId?.(...); since that callback is optional, launch can fail even when the caller already supplied a validlaneId. Mirror the connect-path fallback and normalize before storing.💡 Suggested fix
- const laneId = await args.resolveLaneId?.({ + const resolvedLaneId = await args.resolveLaneId?.({ projectRoot, cwd: resolved.cwd, laneId: launchArgs.laneId ?? null, chatSessionId: launchArgs.chatSessionId ?? null, }); - if (!laneId) { + const laneId = (resolvedLaneId ?? launchArgs.laneId ?? null)?.trim() || null; + if (!laneId) { throw new Error("App Control could not resolve a lane for the terminal. Select a lane or pass laneId."); }Also applies to: 1374-1374
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/main/services/appControl/appControlService.ts` around lines 1360 - 1368, The current launch path only calls the optional resolver (args.resolveLaneId?.(...)) and throws if it returns falsy, ignoring an explicit launchArgs.laneId; change the logic around the laneId variable so you first normalize/accept launchArgs.laneId when present, then call args.resolveLaneId as a fallback (or use its result only if launchArgs.laneId is null/undefined), normalize the final laneId before storing, and apply the same pattern where you resolve/store laneId elsewhere (the other resolve usage). Update the code using the resolveLaneId symbol and the laneId local variable to mirror the connect-path fallback behavior.apps/desktop/src/main/services/ipc/registerIpc.ts (1)
1763-1784:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRedact built-in browser data from IPC trace logs.
These new browser channels now flow through
ipc.invoke.begin/ipc.invoke.done, but the redaction map still only covers app-control and terminal payloads. In dev or withADE_TRACE_IPC=1, navigated URLs and browser status/screenshot data can land in logs, including OAuth codes or embedded credentials.🛡️ Minimal redaction patch
const ipcChannelRedactionMap: Record<string, ReadonlySet<string>> = { [IPC.appControlLaunch]: new Set(["command", "env"]), [IPC.appControlLaunchInTerminal]: new Set(["command", "env"]), [IPC.appControlTypeText]: new Set(["text"]), [IPC.appControlDispatchKey]: new Set(["text", "unmodifiedText", "key", "code"]), [IPC.terminalWrite]: new Set(["data"]), [IPC.ptyWrite]: new Set(["data"]), + [IPC.builtInBrowserNavigate]: new Set(["url"]), + [IPC.builtInBrowserCreateTab]: new Set(["url"]), };You’ll likely also want a small result-redaction hook for
IPC.builtInBrowserGetStatusandIPC.builtInBrowserCaptureScreenshot, otherwise the same data can still leak onipc.invoke.done. As per coding guidelines,apps/desktop/src/**: "Electron desktop app — check for IPC security, proper main/renderer process separation, and React best practices."Also applies to: 1988-2021
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/main/services/ipc/registerIpc.ts` around lines 1763 - 1784, The IPC redaction currently only covers app-control and terminal channels; update ipcChannelRedactionMap to include the built-in browser channels (e.g., IPC.builtInBrowserGetStatus and IPC.builtInBrowserCaptureScreenshot) and list sensitive keys to redact such as "url", "href", "location", "code", "token", "accessToken", "cookies", "screenshot", "image", "data", "html", "title", and "status" so redactIpcArgsForChannel masks those values; additionally add a result-redaction hook for invoke responses (the ipc.invoke.done handling path) to ensure returned payloads from built-in browser calls are scrubbed before logging/trace output.
🟡 Minor comments (9)
apps/desktop/src/shared/adeCliGuidance.ts-24-24 (1)
24-24:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winInline browser guidance omits
clear-selectionand drifts from full guidance.Line 24’s inline browser command list should include
clear-selectionto match Line 11 and avoid partial command guidance.Suggested patch
- ... `switch --tab <id>`, `screenshot --text`, and `inspect-start` / `select-current`; ... + ... `switch --tab <id>`, `screenshot --text`, and `inspect-start` / `select-current` / `clear-selection`; ...🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/shared/adeCliGuidance.ts` at line 24, The inline browser command list in the ADE guidance string is missing the clear-selection command; update the string literal that contains "inspect-start / select-current" to include "clear-selection" in that same inline command list (e.g., change "inspect-start / select-current" to "inspect-start / select-current / clear-selection" or equivalent with backticks) so the guidance on browser commands matches the full guidance elsewhere.apps/desktop/src/renderer/components/terminals/TerminalView.tsx-102-103 (1)
102-103:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winNormalize scheme-less localhost links before opening.
Line 183 opens raw matched text. Since the matcher also captures values like
localhost:3000, those can fail or be treated inconsistently unless a scheme is added first.Suggested patch
function cleanTerminalLinkText(raw: string): string { return raw.replace(/[),.;:!?]+$/g, ""); } +function normalizeTerminalLinkTarget(url: string): string { + return /^(?:localhost|127\.0\.0\.1|\[::1\])(?::\d+)?(?:\/|$)/i.test(url) + ? `http://${url}` + : url; +} + function createTerminalLinkProvider(term: Terminal): ILinkProvider { return { @@ decorations: { underline: true, pointerCursor: true }, activate(event: MouseEvent) { event.preventDefault(); - openUrlInAdeBrowser(text); + openUrlInAdeBrowser(normalizeTerminalLinkTarget(text)); }, }); }Also applies to: 181-184
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/components/terminals/TerminalView.tsx` around lines 102 - 103, Normalize scheme-less localhost/127.0.0.1/[::1] matches before opening by detecting if a matched URL (matched by TERMINAL_LINK_PATTERN) lacks a scheme (e.g., "localhost:3000" or "127.0.0.1:5000") and prepending "http://" (or "http://[::1]" for IPv6) before passing it to the opener; update the link-open handler in TerminalView (the code that currently opens the raw matched text) to perform this normalization so localhost-style matches are consistently valid URLs when opened.apps/ade-cli/src/cli.ts-2627-2637 (1)
2627-2637:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winParse only one positional URL here.
Line 2631 and Line 2641 currently join the whole remaining argv into the URL. That makes trailing flags or escape-hatch args part of the navigation target before
collectGenericObjectArgs()can parse them.Suggested fix
if (sub === "open" || sub === "navigate" || sub === "go") { const explicitUrl = readValue(args, ["--url"]); const tabId = readValue(args, ["--tab", "--tab-id"]); const newTab = readFlag(args, ["--new-tab"]); - const url = explicitUrl ?? args.filter((arg) => arg !== "--active-tab").join(" "); + const url = explicitUrl ?? firstPositional(args); if (!url.trim()) throw new CliUsageError("browser open requires a URL."); return { kind: "execute", label: "browser open", steps: [actionStep("result", "built_in_browser", "navigate", collectGenericObjectArgs(args, { url, tabId, newTab: newTab ? true : undefined, @@ if (sub === "new-tab" || sub === "tab" || sub === "new") { const background = readFlag(args, ["--background"]); - const url = readValue(args, ["--url"]) ?? (args.length ? args.join(" ") : undefined); + const url = readValue(args, ["--url"]) ?? firstPositional(args) ?? undefined; return { kind: "execute", label: "browser new tab", steps: [actionStep("result", "built_in_browser", "createTab", collectGenericObjectArgs(args, { url, activate: background ? false : undefined, }))] }; }Also applies to: 2639-2645
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ade-cli/src/cli.ts` around lines 2627 - 2637, The URL is built by joining all remaining argv tokens which can accidentally include flags; instead derive url as the first positional token (excluding tokens that start with "--" and excluding "--active-tab") when explicitUrl is not provided. Replace the current url = explicitUrl ?? args.filter((arg) => arg !== "--active-tab").join(" ") logic with something like: use explicitUrl if present, otherwise find the first arg that does not startWith("--") and is not "--active-tab" and use that as the URL; keep passing the original args into collectGenericObjectArgs(args, ...) so flag parsing remains intact (references: readValue, readFlag, args, url, collectGenericObjectArgs, newTab, tabId).apps/ade-cli/src/cli.ts-3238-3238 (1)
3238-3238:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winMap the new browser aliases into help resolution too.
Line 3238 adds executable aliases, but
ade help built-in-browserandade built-in-browser --helpstill fall back to top-level help because the alias table above does not normalize those names back tobrowser.Suggested fix
const aliases: Record<string, string> = { lane: "lanes", + "ade-browser": "browser", + "built-in-browser": "browser", + "builtin-browser": "browser", diff: "diff", diffs: "diff",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ade-cli/src/cli.ts` at line 3238, The new executable aliases ("ade-browser", "built-in-browser", "builtin-browser") are checked when dispatching to buildBrowserPlan but are not normalized for the help-resolution logic, so "ade built-in-browser --help" falls back to top-level help; update the help alias normalization to map those aliases back to "browser" (the same mapping used around the primary variable) — e.g., add the aliases into the alias table or the normalization step that resolves the primary command so that when primary === "ade-browser" | "built-in-browser" | "builtin-browser" it is rewritten to "browser" before help lookups and before calling buildBrowserPlan.apps/desktop/src/renderer/components/terminals/TerminalsPage.tsx-45-46 (1)
45-46:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winClean up resize listeners when the drag is interrupted.
mousemove/mouseuphandlers and the body cursor/user-select styles are only cleared fromonUp. If this component unmounts, the sidebar closes, or the window loses focus mid-drag, those globals can stick around and leave the app in a broken resize state.🧹 Suggested fix
const workContentPaneRef = useRef<HTMLDivElement | null>(null); const workSidebarPaneRef = useRef<HTMLDivElement | null>(null); + const activeResizeCleanupRef = useRef<(() => void) | null>(null); + + useEffect(() => () => activeResizeCleanupRef.current?.(), []); ... const handleWorkSidebarResizeMouseDown = useCallback((event: React.MouseEvent<HTMLDivElement>) => { + activeResizeCleanupRef.current?.(); event.preventDefault(); const container = event.currentTarget.parentElement; if (!container) return; ... const onUp = () => { - if (animationFrame != null) { - window.cancelAnimationFrame(animationFrame); - animationFrame = null; - } - applyWidth(pendingWidthPct); - document.removeEventListener("mousemove", onMove); - document.removeEventListener("mouseup", onUp); - document.body.style.cursor = ""; - document.body.style.userSelect = ""; + cleanup(); + applyWidth(pendingWidthPct); work.setWorkSidebarWidthPct(pendingWidthPct); }; + const cleanup = () => { + if (animationFrame != null) { + window.cancelAnimationFrame(animationFrame); + animationFrame = null; + } + document.removeEventListener("mousemove", onMove); + document.removeEventListener("mouseup", onUp); + window.removeEventListener("blur", onUp); + document.body.style.cursor = ""; + document.body.style.userSelect = ""; + activeResizeCleanupRef.current = null; + }; ... + activeResizeCleanupRef.current = cleanup; document.addEventListener("mousemove", onMove); document.addEventListener("mouseup", onUp); + window.addEventListener("blur", onUp); }, [work]);Also applies to: 440-487
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/components/terminals/TerminalsPage.tsx` around lines 45 - 46, The mousemove/mouseup handlers and document body styles set during dragging are only cleared in onUp and can persist if the component unmounts, sidebar closes, or the window blurs; add a centralized cleanup that removes the document 'mousemove' and 'mouseup' listeners and resets document.body.style.cursor and document.body.style.userSelect, and invoke that cleanup from component unmount (useEffect cleanup), window 'blur' handler, and any sidebar-close path. Locate the drag setup/teardown code around workContentPaneRef and workSidebarPaneRef (and functions like onMove/onUp/startDrag) and ensure the same removal/reset logic is called from those lifecycle/blur handlers so no global listeners or styles remain if a drag is interrupted.apps/desktop/src/renderer/components/chat/ChatIosSimulatorPanel.tsx-2223-2224 (1)
2223-2224:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winShow a user-visible message when context capture is unavailable
Line 2223 and Line 2396 return early when
onAddContextis missing, so capture actions can appear to do nothing. Mirror theselectElementAtbehavior and set a message before returning.Proposed fix
const addSimulatorCaptureContext = useCallback(async (frame: PreviewCrop["frame"]) => { if (!snapshot?.screenshot.dataUrl) return; - if (!onAddContext) return; + if (!onAddContext) { + setMessage("Chat attachments are not available in this panel."); + return; + } const capture = await attachSimulatorCapture(frame).catch(() => null); // ... }, [attachSimulatorCapture, onAddContext, snapshot]); const addPreviewCaptureContext = useCallback(async (frame: PreviewCrop["frame"]) => { if (!previewImage || !selectedPreviewTarget || !previewResult?.dataUrl) return; - if (!onAddContext) return; + if (!onAddContext) { + setMessage("Chat attachments are not available in this panel."); + return; + } const capture = await attachPreviewCapture(frame).catch(() => null); // ... }, [attachPreviewCapture, onAddContext, previewImage, previewResult, selectedPreviewTarget]);Also applies to: 2396-2397
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/components/chat/ChatIosSimulatorPanel.tsx` around lines 2223 - 2224, The early returns when onAddContext is falsy (and when attachSimulatorCapture fails) silently do nothing; mirror selectElementAt's behavior by showing a user-visible message before returning. Specifically, in the branches that check onAddContext and after attachSimulatorCapture.catch(() => null) where the code currently returns, call the same UI message/notification helper used by selectElementAt (e.g., the setMessage/showToast function that selectElementAt uses) with a clear text like "Context capture is unavailable" and then return; reference onAddContext, attachSimulatorCapture, and selectElementAt to locate where to add the message.apps/desktop/src/renderer/lib/openExternal.ts-39-47 (1)
39-47:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winOnly emit the built-in-browser event when ADE can actually use it.
Right now the sidebar/browser event fires before the
window.ade?.builtInBrowsercheck, so unsupported environments still switch the UI toward the built-in browser even though the URL falls back toopenExternalUrl().Suggested fix
- window.dispatchEvent(new CustomEvent<OpenBuiltInBrowserDetail>(ADE_OPEN_BUILT_IN_BROWSER_EVENT, { - detail: { url: normalized }, - })); const browser = window.ade?.builtInBrowser; if (browser) { + window.dispatchEvent(new CustomEvent<OpenBuiltInBrowserDetail>(ADE_OPEN_BUILT_IN_BROWSER_EVENT, { + detail: { url: normalized }, + })); void browser.navigate({ url: normalized, newTab: true }).catch(() => openExternalUrl(normalized)); return; }As per coding guidelines, "
apps/desktop/src/**: Electron desktop app — check for IPC security, proper main/renderer process separation, and React best practices."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/lib/openExternal.ts` around lines 39 - 47, The event ADE_OPEN_BUILT_IN_BROWSER_EVENT is dispatched before verifying the built-in browser exists, causing the UI to switch even when we fallback to openExternalUrl; modify openExternal.ts so you first check window.ade?.builtInBrowser (and the browser variable) and only dispatch the CustomEvent when builtInBrowser is present, then call browser.navigate(...).catch(() => openExternalUrl(normalized)); otherwise call openExternalUrl(normalized) without emitting the event; use the existing symbols ADE_OPEN_BUILT_IN_BROWSER_EVENT, window.ade, builtInBrowser, browser.navigate, openExternalUrl and normalized to locate and change the logic.apps/desktop/src/renderer/components/chat/AgentChatComposer.tsx-146-178 (1)
146-178:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUse the normalized browser fields before falling back to
metadata.
ChatBuiltInBrowserPanel.normalizeContextItem()writestext,selector,role, andtagNameonto the top-level item, but these helpers and the preview mostly readmetadata.*. That means browser chips from the new panel will often degrade to"Browser element"and lose their selector/role hints unless the backend duplicates the same data into metadata.Proposed fix
function builtInBrowserContextDisplayLabel(item: BuiltInBrowserContextItem): string { const metadata = item.metadata ?? {}; - for (const value of [metadata.label, metadata.text, metadata.value, item.componentId, metadata.selector, metadata.role, metadata.tagName]) { + for (const value of [ + item.text, + item.selector, + item.title, + metadata.label, + metadata.text, + metadata.value, + item.componentId, + item.role, + item.tagName, + metadata.selector, + metadata.role, + metadata.tagName, + ]) { if (typeof value === "string" && value.trim()) return value.trim(); } return "Browser element"; } function builtInBrowserContextRoleHint(item: BuiltInBrowserContextItem): string | null { const metadata = item.metadata ?? {}; - const role = typeof metadata.role === "string" ? metadata.role.trim() : null; + const role = typeof item.role === "string" && item.role.trim() + ? item.role.trim() + : typeof metadata.role === "string" ? metadata.role.trim() : null; if (role) return role; - const tag = typeof metadata.tagName === "string" ? metadata.tagName.trim() : null; + const tag = typeof item.tagName === "string" && item.tagName.trim() + ? item.tagName.trim() + : typeof metadata.tagName === "string" ? metadata.tagName.trim() : null; return tag ? tag.toLowerCase() : null; }- {typeof selectedBuiltInBrowserContext.metadata.selector === "string" ? ( + {typeof (selectedBuiltInBrowserContext.selector ?? selectedBuiltInBrowserContext.metadata.selector) === "string" ? ( <div className="truncate font-mono text-[length:calc(var(--chat-font-size)*9/14)] text-teal-50/55"> - {selectedBuiltInBrowserContext.metadata.selector} + {selectedBuiltInBrowserContext.selector ?? String(selectedBuiltInBrowserContext.metadata.selector)} </div> ) : null}Also applies to: 2215-2259
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/components/chat/AgentChatComposer.tsx` around lines 146 - 178, The helpers (builtInBrowserContextDisplayLabel, builtInBrowserContextSourceDescription, builtInBrowserContextRoleHint, builtInBrowserContextFrameHint) should prefer normalized top-level fields written by ChatBuiltInBrowserPanel.normalizeContextItem (e.g., item.text, item.selector, item.role, item.tagName, item.url) before falling back to metadata.*, so update builtInBrowserContextDisplayLabel to check item.text, item.selector, item.value, item.componentId, item.role, item.tagName (and item.metadata.* only after those), update builtInBrowserContextRoleHint to check item.role then item.tagName then metadata, and ensure builtInBrowserContextSourceDescription keeps checking item.url first then metadata.url; apply the same ordering change to any duplicate helper code in the 2215-2259 region so chips use the normalized fields.apps/desktop/src/preload/preload.ts-888-891 (1)
888-891:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winInvalidate the browser status cache after
selectPoint().
selectPoint()mutateshasSelectionin main, but unlike the other state-changing methods it doesn't clearbuiltInBrowserStatusCache. A caller that doesawait selectPoint(); await getStatus();can still read the pre-selection snapshot until the async event arrives or the 500ms TTL expires.Proposed fix
- selectPoint: async (args: BuiltInBrowserSelectPointArgs): Promise<BuiltInBrowserSelectResult> => - ipcRenderer.invoke(IPC.builtInBrowserSelectPoint, args), + selectPoint: async (args: BuiltInBrowserSelectPointArgs): Promise<BuiltInBrowserSelectResult> => + clearAround( + () => builtInBrowserStatusCache.clear(), + () => ipcRenderer.invoke(IPC.builtInBrowserSelectPoint, args), + ),Also applies to: 2488-2489
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/preload/preload.ts` around lines 888 - 891, selectPoint() mutates hasSelection in main but doesn't clear the builtInBrowserStatusCache, causing callers doing await selectPoint(); await getStatus(); to receive stale status; after the selectPoint implementation (and the other occurrence at the similar block around the second instance), call the cache invalidation method on builtInBrowserStatusCache (e.g., builtInBrowserStatusCache.invalidate() or the cache's clear() API) immediately after the IPC/selectPoint call so the next getStatus() fetches fresh state.
🧹 Nitpick comments (1)
apps/desktop/src/main/services/builtInBrowser/builtInBrowserService.test.ts (1)
89-96: ⚡ Quick win
disposetest title and assertions are misaligned.The test says it “stops further events” but only checks post-dispose status fields. Add an explicit no-more-emits assertion (or rename the test to match current assertions).
Proposed test tightening
-it("dispose clears emitted state and stops further events", () => { +it("dispose clears emitted state and stops further events", async () => { const service = createBuiltInBrowserService({ onEvent: collector.onEvent }); + await service.setBounds({ x: 1, y: 1, width: 0, height: 0, visible: false }); + const emitsBeforeDispose = collector.events.length; service.dispose(); // dispose itself must not throw; subsequent getStatus reflects an empty service. const status = service.getStatus(); expect(status.tabs).toEqual([]); expect(status.attached).toBe(false); + + await service.setBounds({ x: 2, y: 2, width: 0, height: 0, visible: false }); + expect(collector.events.length).toBe(emitsBeforeDispose); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/main/services/builtInBrowser/builtInBrowserService.test.ts` around lines 89 - 96, The test title and checks are misaligned: either rename the test to reflect only status assertions or add an explicit assertion that no further events are emitted after calling dispose; update the test that uses createBuiltInBrowserService, collector.onEvent, service.dispose and service.getStatus to also attempt to trigger or simulate an event after dispose (or call whatever emit/trigger method exists on the service) and assert the collector did not receive any new events (or that collector.events length is unchanged), or alternatively change the test name to something like "dispose clears emitted state and resets status" if you prefer not to add the no-more-emits assertion.
🤖 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/ade-cli/src/cli.ts`:
- Around line 2620-2677: buildBrowserPlan currently builds plans that target the
built_in_browser domain without checking for a desktop socket; add a guard at
the start of buildBrowserPlan that verifies a desktop socket is available and
fail fast (throw CliUsageError with a clear message mentioning --socket) when a
browser subcommand is requested and the socket is missing; implement this by
calling an existing helper like
checkDesktopSocketAvailable()/desktopSocketAvailable() or add a small helper
(e.g. ensureDesktopSocketAvailable) and use it before any branch that returns an
execute plan for the built_in_browser domain so execution will not attempt to
run when the runtime lacks the desktop socket.
In `@apps/desktop/src/main/main.ts`:
- Around line 3450-3451: The AppContext shape is inconsistent: dormant contexts
include builtInBrowserService but initContextForProjectRoot() returns live
contexts without it, causing getActiveContext().builtInBrowserService to be
undefined for open projects; fix by adding builtInBrowserService to the object
returned by initContextForProjectRoot() (or ensure it is injected/assigned on
the same code path that constructs the live context) and update the AppContext
type if needed so both dormant and live contexts expose the same
builtInBrowserService property (also mirror this change where getActiveContext()
/ orchestratorService are composed).
In `@apps/desktop/src/main/services/builtInBrowser/builtInBrowserService.ts`:
- Around line 713-731: The debuggerDetachListener currently nulls
debuggerMessageListener and debuggerDetachListener without removing them from
wc.debugger, leaving stale callbacks registered; modify the detach branch inside
attachDebuggerListeners so it calls the existing detachDebuggerListeners(wc)
helper to invoke wc.debugger.off(...) and remove the listeners before clearing
the refs and updating inspecting/debuggerAttachedForInspect and emitting status,
ensuring debuggerMessageListener and debuggerDetachListener are deregistered via
detachDebuggerListeners rather than just being set to null.
- Around line 393-399: The call to
electronWebContents.fromId(input.webContentsId) must be hardened: after
obtaining nextWebContents, validate its origin and type before calling
configureBrowserSession()/configureBrowserWebContents(). Specifically, ensure
nextWebContents exists and is not destroyed, then verify
nextWebContents.getType() indicates a guest/webview (e.g., "guest-webview" or
"webview") and that it belongs to the dedicated browser partition/session
(compare nextWebContents.session or its partition name against the expected
browser partition constant used by configureBrowserSession). If any check fails,
throw and refuse to bind the webContents; only call
configureBrowserWebContents(nextWebContents) when these validations pass.
In `@apps/desktop/src/main/services/ipc/registerIpc.ts`:
- Around line 2230-2235: The parsed webContentsId is used directly in
attachWebview without proving it belongs to the requesting renderer; update the
handlers in builtInBrowserService.ts (the code path that calls
electronWebContents.fromId(...) and the call sites of attachWebview()) to verify
ownership before proceeding: after resolving with
electronWebContents.fromId(input.webContentsId) confirm the resolved WebContents
is owned by the same renderer that sent the IPC (compare to event.sender or the
sender's BrowserWindow via BrowserWindow.fromWebContents(event.sender)), or
consult/maintain an allowlist of valid webContents IDs tied to the requesting
renderer, and reject the request (return error) if the ownership check fails;
keep guardBuiltInBrowserIpc() for origin checks but add this additional
per-request ownership validation wherever attachWebview() is invoked.
In `@apps/desktop/src/renderer/components/chat/AgentChatPane.tsx`:
- Around line 4243-4252: When isParallelLaunch is true the parallel-send branch
forwards only draft and attachments and therefore loses the visual context;
update the parallel launch forwarding logic in AgentChatPane (the code path that
handles sending to child lanes when isParallelLaunch is true) to include the
same visual context fields you build here—use builtInBrowserContextSnapshot (and
the ios/appControl snapshots if applicable) and pass visualContextPrefix and
visualContextDisplayChips along with draft and attachments so child lanes
receive the browser/context chips just like the normal send path; ensure the
forwarded payload keys match what child lanes expect (the same names used by the
non-parallel send branch).
- Around line 3624-3632: The browser temp screenshot save block currently calls
addAttachment(...) which updates latestAttachmentRef and causes subsequent
addIosElementContext/addAppControlContext calls to reuse the browser PNG; change
the flow so saving the temp attachment via
window.ade.agentChat.saveTempAttachment(...) does NOT call addAttachment.
Instead store saved.path in a local variable (e.g., browserTempPath) and update
any UI/state for attachments without touching latestAttachmentRef, or if you
must call addAttachment, capture the previous latestAttachmentRef and
immediately restore it after the call; reference the item.screenshotDataUrl save
block, window.ade.agentChat.saveTempAttachment, addAttachment,
latestAttachmentRef, addIosElementContext and addAppControlContext when making
the change.
In `@apps/desktop/src/renderer/components/chat/ChatBuiltInBrowserPanel.tsx`:
- Line 177: The component ChatBuiltInBrowserPanel currently auto-creates a
browser tab pointing to DEFAULT_BROWSER_URL ("https://www.google.com/") when no
tabs exist, causing unsolicited external requests and preventing the empty-state
UI from showing; change this to either (a) use a local blank/start URL constant
(e.g., "about:blank" or an internal route) instead of DEFAULT_BROWSER_URL, or
(b) delay creating a tab until the user explicitly opens a new tab or navigates
(lazy creation). Update the tab-creation logic in ChatBuiltInBrowserPanel and
any related handlers that reference DEFAULT_BROWSER_URL so the empty-state UI is
rendered by default and external navigation only happens on user action.
In `@apps/desktop/src/renderer/components/terminals/TerminalsPage.tsx`:
- Around line 421-429: The ADE_OPEN_BUILT_IN_BROWSER_EVENT handler currently
only calls setViewMode("tabs") and setWorkSidebarTab("browser") but does not
open the sidebar when work.workSidebarOpen is false; update the
openBrowserSidebar function to also ensure the sidebar is shown (e.g. call the
appropriate setter such as setWorkSidebarOpen(true) or
setWorkSidebarVisible(true) / setWorkSidebarOpen to true depending on the
available API) so that the browser tab becomes visible; keep the other calls to
setViewMode and setWorkSidebarTab as-is.
In `@apps/desktop/src/renderer/components/terminals/useWorkSessions.ts`:
- Around line 343-350: selectLaneForActiveTab currently only returns early in an
effect but is also invoked from setSelectedSessionId, setActiveItemId, and
openSessionTab, so those callers can change the selected lane while the user is
on non-Work routes; move the route guard into selectLaneForActiveTab itself: at
the top of the selectLaneForActiveTab callback add a check for the current
route/view (e.g. viewMode !== "grid" or your app's Work-route predicate) and
return early if not on the Work route, then keep the existing sessionId/session
lookup and selectLane(session.laneId) behavior so all callers
(setSelectedSessionId, setActiveItemId, openSessionTab) automatically honor the
route gate.
In `@apps/desktop/src/renderer/lib/openExternal.ts`:
- Around line 27-31: The branch currently passes the original raw url to
openExternalUrl even when canOpenInAdeBrowser(normalized) rejects unsafe
schemes; update openUrlInAdeBrowser so it uses the normalized value and never
forwards a rejected scheme to the external opener: call
openExternalUrl(normalized) only when normalized is truthy and
canOpenInAdeBrowser(normalized) returns true, and otherwise drop/return without
calling openExternalUrl. Reference: openUrlInAdeBrowser,
normalizeBrowserUrlInput, canOpenInAdeBrowser, openExternalUrl.
In `@apps/ios/ADE/Views/Work/WorkRootScreen.swift`:
- Around line 46-47: Persisted selectedLaneId (AppStorage in WorkRootScreen) can
be stale and hide sessions; validate and reconcile it against the current lanes
whenever lane data changes. Update the code that receives/loads lane data (e.g.,
after fetching in WorkRootScreen or in the view model backing
WorkFiltersSection) to check if selectedLaneId exists in the current lanes' id
set and if not set selectedLaneId = "all" (or the WorkFiltersSection fallback)
so the UI won’t be stranded on an invalid lane id; you can implement this in the
lanes state observer, onAppear, or a validateSelectedLane() helper invoked after
lanes update.
---
Outside diff comments:
In `@apps/desktop/src/main/services/appControl/appControlService.ts`:
- Around line 1360-1368: The current launch path only calls the optional
resolver (args.resolveLaneId?.(...)) and throws if it returns falsy, ignoring an
explicit launchArgs.laneId; change the logic around the laneId variable so you
first normalize/accept launchArgs.laneId when present, then call
args.resolveLaneId as a fallback (or use its result only if launchArgs.laneId is
null/undefined), normalize the final laneId before storing, and apply the same
pattern where you resolve/store laneId elsewhere (the other resolve usage).
Update the code using the resolveLaneId symbol and the laneId local variable to
mirror the connect-path fallback behavior.
In `@apps/desktop/src/main/services/ipc/registerIpc.ts`:
- Around line 1763-1784: The IPC redaction currently only covers app-control and
terminal channels; update ipcChannelRedactionMap to include the built-in browser
channels (e.g., IPC.builtInBrowserGetStatus and
IPC.builtInBrowserCaptureScreenshot) and list sensitive keys to redact such as
"url", "href", "location", "code", "token", "accessToken", "cookies",
"screenshot", "image", "data", "html", "title", and "status" so
redactIpcArgsForChannel masks those values; additionally add a result-redaction
hook for invoke responses (the ipc.invoke.done handling path) to ensure returned
payloads from built-in browser calls are scrubbed before logging/trace output.
In `@apps/desktop/src/renderer/components/files/FilesPage.tsx`:
- Around line 1279-1308: The then-handler for window.ade.files.listWorkspaces()
captures a stale selectedLaneId from the initial render which can cause
setWorkspaceId to pick the wrong workspace; change the handler to read the
latest lane selection (e.g., keep selectedLaneId in a ref like selectedLaneIdRef
that you update when selectedLaneId changes) and use that ref inside the
listWorkspaces().then(...) callback when calling defaultFilesWorkspaceId and
deciding whether to clear tabs; update references to selectedLaneId in the
setWorkspaceId callback to use the ref (or otherwise obtain the current
selectedLaneId at resolution time) so workspace selection is based on the most
recent lane.
---
Minor comments:
In `@apps/ade-cli/src/cli.ts`:
- Around line 2627-2637: The URL is built by joining all remaining argv tokens
which can accidentally include flags; instead derive url as the first positional
token (excluding tokens that start with "--" and excluding "--active-tab") when
explicitUrl is not provided. Replace the current url = explicitUrl ??
args.filter((arg) => arg !== "--active-tab").join(" ") logic with something
like: use explicitUrl if present, otherwise find the first arg that does not
startWith("--") and is not "--active-tab" and use that as the URL; keep passing
the original args into collectGenericObjectArgs(args, ...) so flag parsing
remains intact (references: readValue, readFlag, args, url,
collectGenericObjectArgs, newTab, tabId).
- Line 3238: The new executable aliases ("ade-browser", "built-in-browser",
"builtin-browser") are checked when dispatching to buildBrowserPlan but are not
normalized for the help-resolution logic, so "ade built-in-browser --help" falls
back to top-level help; update the help alias normalization to map those aliases
back to "browser" (the same mapping used around the primary variable) — e.g.,
add the aliases into the alias table or the normalization step that resolves the
primary command so that when primary === "ade-browser" | "built-in-browser" |
"builtin-browser" it is rewritten to "browser" before help lookups and before
calling buildBrowserPlan.
In `@apps/desktop/src/preload/preload.ts`:
- Around line 888-891: selectPoint() mutates hasSelection in main but doesn't
clear the builtInBrowserStatusCache, causing callers doing await selectPoint();
await getStatus(); to receive stale status; after the selectPoint implementation
(and the other occurrence at the similar block around the second instance), call
the cache invalidation method on builtInBrowserStatusCache (e.g.,
builtInBrowserStatusCache.invalidate() or the cache's clear() API) immediately
after the IPC/selectPoint call so the next getStatus() fetches fresh state.
In `@apps/desktop/src/renderer/components/chat/AgentChatComposer.tsx`:
- Around line 146-178: The helpers (builtInBrowserContextDisplayLabel,
builtInBrowserContextSourceDescription, builtInBrowserContextRoleHint,
builtInBrowserContextFrameHint) should prefer normalized top-level fields
written by ChatBuiltInBrowserPanel.normalizeContextItem (e.g., item.text,
item.selector, item.role, item.tagName, item.url) before falling back to
metadata.*, so update builtInBrowserContextDisplayLabel to check item.text,
item.selector, item.value, item.componentId, item.role, item.tagName (and
item.metadata.* only after those), update builtInBrowserContextRoleHint to check
item.role then item.tagName then metadata, and ensure
builtInBrowserContextSourceDescription keeps checking item.url first then
metadata.url; apply the same ordering change to any duplicate helper code in the
2215-2259 region so chips use the normalized fields.
In `@apps/desktop/src/renderer/components/chat/ChatIosSimulatorPanel.tsx`:
- Around line 2223-2224: The early returns when onAddContext is falsy (and when
attachSimulatorCapture fails) silently do nothing; mirror selectElementAt's
behavior by showing a user-visible message before returning. Specifically, in
the branches that check onAddContext and after attachSimulatorCapture.catch(()
=> null) where the code currently returns, call the same UI message/notification
helper used by selectElementAt (e.g., the setMessage/showToast function that
selectElementAt uses) with a clear text like "Context capture is unavailable"
and then return; reference onAddContext, attachSimulatorCapture, and
selectElementAt to locate where to add the message.
In `@apps/desktop/src/renderer/components/terminals/TerminalsPage.tsx`:
- Around line 45-46: The mousemove/mouseup handlers and document body styles set
during dragging are only cleared in onUp and can persist if the component
unmounts, sidebar closes, or the window blurs; add a centralized cleanup that
removes the document 'mousemove' and 'mouseup' listeners and resets
document.body.style.cursor and document.body.style.userSelect, and invoke that
cleanup from component unmount (useEffect cleanup), window 'blur' handler, and
any sidebar-close path. Locate the drag setup/teardown code around
workContentPaneRef and workSidebarPaneRef (and functions like
onMove/onUp/startDrag) and ensure the same removal/reset logic is called from
those lifecycle/blur handlers so no global listeners or styles remain if a drag
is interrupted.
In `@apps/desktop/src/renderer/components/terminals/TerminalView.tsx`:
- Around line 102-103: Normalize scheme-less localhost/127.0.0.1/[::1] matches
before opening by detecting if a matched URL (matched by TERMINAL_LINK_PATTERN)
lacks a scheme (e.g., "localhost:3000" or "127.0.0.1:5000") and prepending
"http://" (or "http://[::1]" for IPv6) before passing it to the opener; update
the link-open handler in TerminalView (the code that currently opens the raw
matched text) to perform this normalization so localhost-style matches are
consistently valid URLs when opened.
In `@apps/desktop/src/renderer/lib/openExternal.ts`:
- Around line 39-47: The event ADE_OPEN_BUILT_IN_BROWSER_EVENT is dispatched
before verifying the built-in browser exists, causing the UI to switch even when
we fallback to openExternalUrl; modify openExternal.ts so you first check
window.ade?.builtInBrowser (and the browser variable) and only dispatch the
CustomEvent when builtInBrowser is present, then call
browser.navigate(...).catch(() => openExternalUrl(normalized)); otherwise call
openExternalUrl(normalized) without emitting the event; use the existing symbols
ADE_OPEN_BUILT_IN_BROWSER_EVENT, window.ade, builtInBrowser, browser.navigate,
openExternalUrl and normalized to locate and change the logic.
In `@apps/desktop/src/shared/adeCliGuidance.ts`:
- Line 24: The inline browser command list in the ADE guidance string is missing
the clear-selection command; update the string literal that contains
"inspect-start / select-current" to include "clear-selection" in that same
inline command list (e.g., change "inspect-start / select-current" to
"inspect-start / select-current / clear-selection" or equivalent with backticks)
so the guidance on browser commands matches the full guidance elsewhere.
---
Nitpick comments:
In `@apps/desktop/src/main/services/builtInBrowser/builtInBrowserService.test.ts`:
- Around line 89-96: The test title and checks are misaligned: either rename the
test to reflect only status assertions or add an explicit assertion that no
further events are emitted after calling dispose; update the test that uses
createBuiltInBrowserService, collector.onEvent, service.dispose and
service.getStatus to also attempt to trigger or simulate an event after dispose
(or call whatever emit/trigger method exists on the service) and assert the
collector did not receive any new events (or that collector.events length is
unchanged), or alternatively change the test name to something like "dispose
clears emitted state and resets status" if you prefer not to add the
no-more-emits assertion.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: fcf2ce3a-e6a6-4e66-8dbe-653b8418f08a
⛔ Files ignored due to path filters (11)
.claude/scheduled_tasks.lockis excluded by!**/*.lockdocs/ARCHITECTURE.mdis excluded by!docs/**docs/features/chat/README.mdis excluded by!docs/**docs/features/chat/composer-and-ui.mdis excluded by!docs/**docs/features/computer-use/app-control.mdis excluded by!docs/**docs/features/files-and-editor/README.mdis excluded by!docs/**docs/features/files-and-editor/editor-surfaces.mdis excluded by!docs/**docs/features/ios-simulator/README.mdis excluded by!docs/**docs/features/terminals-and-sessions/README.mdis excluded by!docs/**docs/features/terminals-and-sessions/ui-surfaces.mdis excluded by!docs/**goal.mdis excluded by!*.md
📒 Files selected for processing (48)
apps/ade-cli/src/bootstrap.tsapps/ade-cli/src/cli.test.tsapps/ade-cli/src/cli.tsapps/desktop/src/main/main.tsapps/desktop/src/main/services/adeActions/registry.tsapps/desktop/src/main/services/appControl/appControlService.tsapps/desktop/src/main/services/builtInBrowser/builtInBrowserService.test.tsapps/desktop/src/main/services/builtInBrowser/builtInBrowserService.tsapps/desktop/src/main/services/ios/iosSimulatorService.test.tsapps/desktop/src/main/services/ios/iosSimulatorService.tsapps/desktop/src/main/services/ipc/registerIpc.tsapps/desktop/src/preload/global.d.tsapps/desktop/src/preload/preload.tsapps/desktop/src/renderer/components/app/AppShell.tsxapps/desktop/src/renderer/components/app/FloatingFilesWorkspace.tsxapps/desktop/src/renderer/components/app/RightEdgeFloatingPane.tsxapps/desktop/src/renderer/components/chat/AgentChatComposer.tsxapps/desktop/src/renderer/components/chat/AgentChatMessageList.tsxapps/desktop/src/renderer/components/chat/AgentChatPane.tsxapps/desktop/src/renderer/components/chat/ChatAppControlPanel.tsxapps/desktop/src/renderer/components/chat/ChatBuiltInBrowserPanel.tsxapps/desktop/src/renderer/components/chat/ChatIosSimulatorPanel.test.tsxapps/desktop/src/renderer/components/chat/ChatIosSimulatorPanel.tsxapps/desktop/src/renderer/components/chat/chatMarkdown.tsxapps/desktop/src/renderer/components/files/FilesPage.test.tsxapps/desktop/src/renderer/components/files/FilesPage.tsxapps/desktop/src/renderer/components/lanes/useLaneWorkSessions.tsapps/desktop/src/renderer/components/run/LaneRuntimeBar.tsxapps/desktop/src/renderer/components/terminals/SessionListPane.test.tsxapps/desktop/src/renderer/components/terminals/SessionListPane.tsxapps/desktop/src/renderer/components/terminals/TerminalView.tsxapps/desktop/src/renderer/components/terminals/TerminalsPage.tsxapps/desktop/src/renderer/components/terminals/WorkSidebar.tsxapps/desktop/src/renderer/components/terminals/WorkStartSurface.tsxapps/desktop/src/renderer/components/terminals/WorkViewArea.tsxapps/desktop/src/renderer/components/terminals/useWorkSessions.test.tsapps/desktop/src/renderer/components/terminals/useWorkSessions.tsapps/desktop/src/renderer/index.cssapps/desktop/src/renderer/lib/openExternal.tsapps/desktop/src/renderer/state/appStore.test.tsapps/desktop/src/renderer/state/appStore.tsapps/desktop/src/shared/adeCliGuidance.tsapps/desktop/src/shared/ipc.tsapps/desktop/src/shared/types/appControl.tsapps/desktop/src/shared/types/builtInBrowser.tsapps/desktop/src/shared/types/index.tsapps/desktop/src/shared/types/iosSimulator.tsapps/ios/ADE/Views/Work/WorkRootScreen.swift
💤 Files with no reviewable changes (4)
- apps/desktop/src/renderer/components/app/AppShell.tsx
- apps/desktop/src/renderer/components/app/RightEdgeFloatingPane.tsx
- apps/desktop/src/renderer/components/app/FloatingFilesWorkspace.tsx
- apps/desktop/src/renderer/index.css
| function buildBrowserPlan(args: string[]): CliPlan { | ||
| const sub = firstPositional(args) ?? "status"; | ||
| if (sub === "help") return { kind: "help", text: HELP_BY_COMMAND.browser }; | ||
| if (sub === "actions") return { kind: "execute", label: "browser actions", steps: [listActionsStep("actions", "built_in_browser")] }; | ||
| if (sub === "status" || sub === "tabs" || sub === "list") { | ||
| return { kind: "execute", label: "browser status", steps: [actionStep("result", "built_in_browser", "getStatus", collectGenericObjectArgs(args))] }; | ||
| } | ||
| if (sub === "open" || sub === "navigate" || sub === "go") { | ||
| const explicitUrl = readValue(args, ["--url"]); | ||
| const tabId = readValue(args, ["--tab", "--tab-id"]); | ||
| const newTab = readFlag(args, ["--new-tab"]); | ||
| const url = explicitUrl ?? args.filter((arg) => arg !== "--active-tab").join(" "); | ||
| if (!url.trim()) throw new CliUsageError("browser open requires a URL."); | ||
| return { kind: "execute", label: "browser open", steps: [actionStep("result", "built_in_browser", "navigate", collectGenericObjectArgs(args, { | ||
| url, | ||
| tabId, | ||
| newTab: newTab ? true : undefined, | ||
| }))] }; | ||
| } | ||
| if (sub === "new-tab" || sub === "tab" || sub === "new") { | ||
| const background = readFlag(args, ["--background"]); | ||
| const url = readValue(args, ["--url"]) ?? (args.length ? args.join(" ") : undefined); | ||
| return { kind: "execute", label: "browser new tab", steps: [actionStep("result", "built_in_browser", "createTab", collectGenericObjectArgs(args, { | ||
| url, | ||
| activate: background ? false : undefined, | ||
| }))] }; | ||
| } | ||
| if (sub === "switch" || sub === "activate") { | ||
| return { kind: "execute", label: "browser switch", steps: [actionStep("result", "built_in_browser", "switchTab", collectGenericObjectArgs(args, { | ||
| tabId: requireValue(readValue(args, ["--tab", "--tab-id"]) ?? firstPositional(args), "tabId"), | ||
| }))] }; | ||
| } | ||
| if (sub === "close" || sub === "close-tab") { | ||
| return { kind: "execute", label: "browser close", steps: [actionStep("result", "built_in_browser", "closeTab", collectGenericObjectArgs(args, { | ||
| tabId: requireValue(readValue(args, ["--tab", "--tab-id"]) ?? firstPositional(args), "tabId"), | ||
| }))] }; | ||
| } | ||
| if (sub === "reload" || sub === "refresh") return { kind: "execute", label: "browser reload", steps: [actionStep("result", "built_in_browser", "reload", collectGenericObjectArgs(args))] }; | ||
| if (sub === "back") return { kind: "execute", label: "browser back", steps: [actionStep("result", "built_in_browser", "goBack", collectGenericObjectArgs(args))] }; | ||
| if (sub === "forward") return { kind: "execute", label: "browser forward", steps: [actionStep("result", "built_in_browser", "goForward", collectGenericObjectArgs(args))] }; | ||
| if (sub === "stop") return { kind: "execute", label: "browser stop", steps: [actionStep("result", "built_in_browser", "stop", collectGenericObjectArgs(args))] }; | ||
| if (sub === "screenshot" || sub === "capture") return { kind: "execute", label: "browser screenshot", steps: [actionStep("result", "built_in_browser", "captureScreenshot", collectGenericObjectArgs(args))] }; | ||
| if (sub === "select" || sub === "select-point" || sub === "point") { | ||
| const x = readNumberOption(args, ["--x"]); | ||
| const y = readNumberOption(args, ["--y"]); | ||
| if (x == null || y == null) throw new CliUsageError("browser select requires --x and --y."); | ||
| return { kind: "execute", label: "browser selection", steps: [actionStep("result", "built_in_browser", "selectPoint", collectGenericObjectArgs(args, { | ||
| x, | ||
| y, | ||
| includeScreenshot: readFlag(args, ["--no-screenshot"]) ? false : undefined, | ||
| }))] }; | ||
| } | ||
| if (sub === "inspect-start" || sub === "start-inspect" || sub === "inspect") return { kind: "execute", label: "browser inspect start", steps: [actionStep("result", "built_in_browser", "startInspect", collectGenericObjectArgs(args))] }; | ||
| if (sub === "inspect-stop" || sub === "stop-inspect") return { kind: "execute", label: "browser inspect stop", steps: [actionStep("result", "built_in_browser", "stopInspect", collectGenericObjectArgs(args))] }; | ||
| if (sub === "select-current" || sub === "selection" || sub === "selected") return { kind: "execute", label: "browser selection", steps: [actionStep("result", "built_in_browser", "selectCurrent", collectGenericObjectArgs(args))] }; | ||
| if (sub === "clear-selection" || sub === "clear") return { kind: "execute", label: "browser clear selection", steps: [actionStep("result", "built_in_browser", "clearSelection", collectGenericObjectArgs(args))] }; | ||
| return { kind: "execute", label: `browser ${sub}`, steps: [actionStep("result", "built_in_browser", sub, collectGenericObjectArgs(args))] }; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== browser action wiring =="
rg -n --type=ts 'built_in_browser|createTab|switchTab|closeTab|startInspect|stopInspect|captureScreenshot' apps/ade-cli/src apps/desktop/src
echo
echo "== headless runtime entry points =="
rg -n --type=ts 'createAdeRuntime|createAdeRpcRequestHandler|run_ade_action|list_ade_actions' apps/ade-cli/src apps/desktop/src
echo
echo "== built_in_browser references inside the CLI/headless side only =="
rg -n --type=ts 'built_in_browser' apps/ade-cli/srcRepository: arul28/ADE
Length of output: 50037
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== AdeRuntime type definition =="
rg -n --type=ts 'interface AdeRuntime|type AdeRuntime' apps/ade-cli/src
echo
echo "== builtInBrowserService in bootstrap/runtime setup =="
rg -n --type=ts 'builtInBrowserService' apps/ade-cli/src apps/desktop/src
echo
echo "== Runtime creation and service initialization =="
rg -A 20 'export async function createAdeRuntime' apps/ade-cli/src/bootstrap.ts | head -60
echo
echo "== Check for fallback or error handling for missing domains =="
rg -n --type=ts 'domain.*not.*found|socket.*required|desktop.*required' apps/ade-cli/srcRepository: arul28/ADE
Length of output: 3549
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== toService function implementation =="
rg -A 10 'function toService|const toService' apps/desktop/src/main/services/adeActions/registry.ts | head -30
echo
echo "== How runtime validates/enforces domains =="
rg -n --type=ts 'getService|getServiceForDomain|domain.*available' apps/ade-cli/src apps/desktop/src/main/services/adeActions/
echo
echo "== Full AdeRuntime type definition =="
sed -n '116,200p' apps/ade-cli/src/bootstrap.ts
echo
echo "== How createAdeRuntime initializes builtInBrowserService =="
rg -A 100 'export async function createAdeRuntime' apps/ade-cli/src/bootstrap.ts | grep -i 'builtin\|browser\|return' | head -20Repository: arul28/ADE
Length of output: 7363
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== getService implementation in bootstrap =="
sed -n '640,700p' apps/ade-cli/src/bootstrap.ts
echo
echo "== adeRpcServer domain validation and error handling =="
rg -B 5 -A 5 "Domain.*is unavailable in this runtime" apps/ade-cli/src/adeRpcServer.ts
echo
echo "== Check if built_in_browser is initialized in headless bootstrap =="
rg -n 'builtInBrowserService.*=' apps/ade-cli/src/bootstrap.tsRepository: arul28/ADE
Length of output: 1339
Browser commands require a desktop socket connection.
The built_in_browser domain is not initialized in headless CLI mode. These plans will fail at execution time with "Domain 'built_in_browser' is unavailable in this runtime" rather than failing fast. Add validation to buildBrowserPlan() to require the desktop socket, or remove browser subcommands from the headless CLI surface to match the help text that already documents --socket as required.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/ade-cli/src/cli.ts` around lines 2620 - 2677, buildBrowserPlan currently
builds plans that target the built_in_browser domain without checking for a
desktop socket; add a guard at the start of buildBrowserPlan that verifies a
desktop socket is available and fail fast (throw CliUsageError with a clear
message mentioning --socket) when a browser subcommand is requested and the
socket is missing; implement this by calling an existing helper like
checkDesktopSocketAvailable()/desktopSocketAvailable() or add a small helper
(e.g. ensureDesktopSocketAvailable) and use it before any branch that returns an
execute plan for the built_in_browser domain so execution will not attempt to
run when the runtime lacks the desktop socket.
| const nextWebContents = electronWebContents.fromId(input.webContentsId); | ||
| if (!nextWebContents || nextWebContents.isDestroyed()) { | ||
| throw new Error("Browser webview is not available."); | ||
| } | ||
|
|
||
| configureBrowserSession(); | ||
| configureBrowserWebContents(nextWebContents); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n apps/desktop/src/main/services/builtInBrowser/builtInBrowserService.ts | sed -n '390,410p'Repository: arul28/ADE
Length of output: 920
🏁 Script executed:
# Search for BROWSER_PARTITION definition
rg "BROWSER_PARTITION" apps/desktop/src/main/services/builtInBrowser/Repository: arul28/ADE
Length of output: 565
🏁 Script executed:
# Check the full context and function signature
cat -n apps/desktop/src/main/services/builtInBrowser/builtInBrowserService.ts | sed -n '370,420p'Repository: arul28/ADE
Length of output: 2102
🏁 Script executed:
# Check imports at the top of the file
head -50 apps/desktop/src/main/services/builtInBrowser/builtInBrowserService.tsRepository: arul28/ADE
Length of output: 1525
🏁 Script executed:
# Check how attachWebview is exposed via IPC
rg "attachWebview" apps/desktop/src/main/services/builtInBrowser/ -A 2 -B 2Repository: arul28/ADE
Length of output: 1057
🏁 Script executed:
# Search for getType() usage in the codebase to understand the API
rg "\.getType\(\)" apps/desktop/src/ --type tsRepository: arul28/ADE
Length of output: 36
🏁 Script executed:
# Check for similar webContents validation patterns
rg "session\.fromPartition" apps/desktop/src/ --type ts -B 2 -A 2Repository: arul28/ADE
Length of output: 731
🏁 Script executed:
# Look at more of the function to understand the full flow
cat -n apps/desktop/src/main/services/builtInBrowser/builtInBrowserService.ts | sed -n '420,480p'Repository: arul28/ADE
Length of output: 2588
🏁 Script executed:
# Find where these methods are registered as IPC handlers
rg "ipcMain\|handle\|on" apps/desktop/src/main/services/builtInBrowser/ --type ts -B 2 -A 2Repository: arul28/ADE
Length of output: 36
🏁 Script executed:
# Search the whole main process for IPC handler registration of builtInBrowser
rg "builtInBrowser" apps/desktop/src/main/ --type ts -lRepository: arul28/ADE
Length of output: 264
🏁 Script executed:
# Check if there's an IPC setup in the main file
find apps/desktop/src/main -name "*.ts" -type f | head -20Repository: arul28/ADE
Length of output: 1140
🏁 Script executed:
# Check registerIpc.ts for IPC handler setup
cat -n apps/desktop/src/main/services/ipc/registerIpc.ts | head -100Repository: arul28/ADE
Length of output: 4055
🏁 Script executed:
# Search for attachWebview in registerIpc
rg "attachWebview" apps/desktop/src/main/services/ipc/registerIpc.ts -B 3 -A 3Repository: arul28/ADE
Length of output: 430
🏁 Script executed:
# Check the main.ts file
head -100 apps/desktop/src/main/main.tsRepository: arul28/ADE
Length of output: 6473
Validate webContentsId origin and type before using in renderer-supplied fromId().
The electronWebContents.fromId(input.webContentsId) call accepts any ID the renderer passes without verifying it belongs to an actual browser guest on the dedicated partition. An XSS or compromised renderer can exploit this to bind privileged app content views to this service, then navigate, inspect, or screenshot them via the IPC surface.
Add validation to ensure the webContents is a guest webview on the browser partition:
Required hardening
const nextWebContents = electronWebContents.fromId(input.webContentsId);
if (!nextWebContents || nextWebContents.isDestroyed()) {
throw new Error("Browser webview is not available.");
}
+if (nextWebContents.getType() !== "webview") {
+ throw new Error("Only browser webview guests can be attached.");
+}
+if (nextWebContents.session !== session.fromPartition(BROWSER_PARTITION)) {
+ throw new Error("Browser webview must use the ADE browser partition.");
+}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/desktop/src/main/services/builtInBrowser/builtInBrowserService.ts`
around lines 393 - 399, The call to
electronWebContents.fromId(input.webContentsId) must be hardened: after
obtaining nextWebContents, validate its origin and type before calling
configureBrowserSession()/configureBrowserWebContents(). Specifically, ensure
nextWebContents exists and is not destroyed, then verify
nextWebContents.getType() indicates a guest/webview (e.g., "guest-webview" or
"webview") and that it belongs to the dedicated browser partition/session
(compare nextWebContents.session or its partition name against the expected
browser partition constant used by configureBrowserSession). If any check fails,
throw and refuse to bind the webContents; only call
configureBrowserWebContents(nextWebContents) when these validations pass.
| const attachDebuggerListeners = (wc: WebContents): void => { | ||
| if (debuggerMessageListener || debuggerDetachListener) return; | ||
| debuggerMessageListener = (_event, method, params) => { | ||
| if (method !== "Overlay.inspectNodeRequested") return; | ||
| const backendNodeId = isRecord(params) ? params.backendNodeId : null; | ||
| if (typeof backendNodeId !== "number" || !Number.isFinite(backendNodeId)) return; | ||
| void handleInspectNodeRequested(wc, backendNodeId).catch(emitError); | ||
| }; | ||
| debuggerDetachListener = (_event, reason) => { | ||
| logger()?.debug("built_in_browser.debugger_detached", { reason }); | ||
| inspecting = false; | ||
| debuggerAttachedForInspect = false; | ||
| debuggerMessageListener = null; | ||
| debuggerDetachListener = null; | ||
| emitStatus(); | ||
| }; | ||
| wc.debugger.on("message", debuggerMessageListener); | ||
| wc.debugger.on("detach", debuggerDetachListener); | ||
| }; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
wc -l apps/desktop/src/main/services/builtInBrowser/builtInBrowserService.tsRepository: arul28/ADE
Length of output: 130
🏁 Script executed:
cat -n apps/desktop/src/main/services/builtInBrowser/builtInBrowserService.ts | sed -n '700,750p'Repository: arul28/ADE
Length of output: 2147
🏁 Script executed:
rg "detachDebuggerListeners" apps/desktop/src/main/services/builtInBrowser/builtInBrowserService.ts -B 5 -A 10Repository: arul28/ADE
Length of output: 1368
Detach debugger listeners before clearing the listener refs.
When the debugger detaches at line 721–728, the handler nulls debuggerMessageListener and debuggerDetachListener without calling off(). This leaves the callbacks registered on wc.debugger, causing stale listeners to fire or double-deliver events in subsequent inspect sessions on the same WebContents.
Call the existing detachDebuggerListeners(wc) function (defined at lines 733–742) to properly deregister listeners via .off() before nulling the refs.
Proposed fix
debuggerDetachListener = (_event, reason) => {
logger()?.debug("built_in_browser.debugger_detached", { reason });
+ detachDebuggerListeners(wc);
inspecting = false;
debuggerAttachedForInspect = false;
- debuggerMessageListener = null;
- debuggerDetachListener = null;
emitStatus();
};🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/desktop/src/main/services/builtInBrowser/builtInBrowserService.ts`
around lines 713 - 731, The debuggerDetachListener currently nulls
debuggerMessageListener and debuggerDetachListener without removing them from
wc.debugger, leaving stale callbacks registered; modify the detach branch inside
attachDebuggerListeners so it calls the existing detachDebuggerListeners(wc)
helper to invoke wc.debugger.off(...) and remove the listeners before clearing
the refs and updating inspecting/debuggerAttachedForInspect and emitting status,
ensuring debuggerMessageListener and debuggerDetachListener are deregistered via
detachDebuggerListeners rather than just being set to null.
| if (item.screenshotDataUrl && !attachmentPath) { | ||
| try { | ||
| const saved = await window.ade.agentChat.saveTempAttachment({ | ||
| data: stripDataUrlPrefix(item.screenshotDataUrl), | ||
| filename: "built-in-browser-selection.png", | ||
| }); | ||
| attachmentPath = saved.path; | ||
| addAttachment({ path: saved.path, type: inferAttachmentType(saved.path, "image/png") }); | ||
| } catch (error) { |
There was a problem hiding this comment.
Keep browser temp attachments out of the cross-tool pairing ref.
addAttachment() updates latestAttachmentRef, and addIosElementContext / addAppControlContext use that ref as the candidate screenshot for their next context packet. After Line 3631, the next iOS/App Control selection made within the 10s window can incorrectly reuse this browser PNG instead of its own capture.
Suggested fix
- addAttachment({ path: saved.path, type: inferAttachmentType(saved.path, "image/png") });
+ setAttachments((current) => (
+ current.some((entry) => entry.path === saved.path)
+ ? current
+ : [...current, { path: saved.path, type: inferAttachmentType(saved.path, "image/png") }]
+ ));📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (item.screenshotDataUrl && !attachmentPath) { | |
| try { | |
| const saved = await window.ade.agentChat.saveTempAttachment({ | |
| data: stripDataUrlPrefix(item.screenshotDataUrl), | |
| filename: "built-in-browser-selection.png", | |
| }); | |
| attachmentPath = saved.path; | |
| addAttachment({ path: saved.path, type: inferAttachmentType(saved.path, "image/png") }); | |
| } catch (error) { | |
| if (item.screenshotDataUrl && !attachmentPath) { | |
| try { | |
| const saved = await window.ade.agentChat.saveTempAttachment({ | |
| data: stripDataUrlPrefix(item.screenshotDataUrl), | |
| filename: "built-in-browser-selection.png", | |
| }); | |
| attachmentPath = saved.path; | |
| setAttachments((current) => ( | |
| current.some((entry) => entry.path === saved.path) | |
| ? current | |
| : [...current, { path: saved.path, type: inferAttachmentType(saved.path, "image/png") }] | |
| )); | |
| } catch (error) { |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/desktop/src/renderer/components/chat/AgentChatPane.tsx` around lines
3624 - 3632, The browser temp screenshot save block currently calls
addAttachment(...) which updates latestAttachmentRef and causes subsequent
addIosElementContext/addAppControlContext calls to reuse the browser PNG; change
the flow so saving the temp attachment via
window.ade.agentChat.saveTempAttachment(...) does NOT call addAttachment.
Instead store saved.path in a local variable (e.g., browserTempPath) and update
any UI/state for attachments without touching latestAttachmentRef, or if you
must call addAttachment, capture the previous latestAttachmentRef and
immediately restore it after the call; reference the item.screenshotDataUrl save
block, window.ade.agentChat.saveTempAttachment, addAttachment,
latestAttachmentRef, addIosElementContext and addAppControlContext when making
the change.
|
|
||
| const BOUNDS_SETTLE_MS = 1_200; | ||
| const BOUNDS_SETTLE_MIN_FRAME_MS = 32; | ||
| const DEFAULT_BROWSER_URL = "https://www.google.com/"; |
There was a problem hiding this comment.
Don't hit Google just by opening the panel.
This effect creates a tab to https://www.google.com/ as soon as the panel sees no tabs. That sends unsolicited third-party traffic on mount and also makes the empty-state UI unreachable. Prefer lazy tab creation or a local blank/start page instead.
Also applies to: 1027-1052
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/desktop/src/renderer/components/chat/ChatBuiltInBrowserPanel.tsx` at
line 177, The component ChatBuiltInBrowserPanel currently auto-creates a browser
tab pointing to DEFAULT_BROWSER_URL ("https://www.google.com/") when no tabs
exist, causing unsolicited external requests and preventing the empty-state UI
from showing; change this to either (a) use a local blank/start URL constant
(e.g., "about:blank" or an internal route) instead of DEFAULT_BROWSER_URL, or
(b) delay creating a tab until the user explicitly opens a new tab or navigates
(lazy creation). Update the tab-creation logic in ChatBuiltInBrowserPanel and
any related handlers that reference DEFAULT_BROWSER_URL so the empty-state UI is
rendered by default and external navigation only happens on user action.
| const { setViewMode, setWorkSidebarTab } = work; | ||
| useEffect(() => { | ||
| const openBrowserSidebar = () => { | ||
| setViewMode("tabs"); | ||
| setWorkSidebarTab("browser"); | ||
| }; | ||
| window.addEventListener(ADE_OPEN_BUILT_IN_BROWSER_EVENT, openBrowserSidebar); | ||
| return () => window.removeEventListener(ADE_OPEN_BUILT_IN_BROWSER_EVENT, openBrowserSidebar); | ||
| }, [setViewMode, setWorkSidebarTab]); |
There was a problem hiding this comment.
Actually open the sidebar on ADE_OPEN_BUILT_IN_BROWSER_EVENT.
This handler only switches the view mode and selects the "browser" tab. When work.workSidebarOpen is false, workSidebarVisible stays false, so the event named “open built-in browser” still leaves the browser hidden.
🐛 Suggested fix
- const { setViewMode, setWorkSidebarTab } = work;
+ const { setViewMode, setWorkSidebarOpen, setWorkSidebarTab } = work;
useEffect(() => {
const openBrowserSidebar = () => {
setViewMode("tabs");
+ setWorkSidebarOpen(true);
setWorkSidebarTab("browser");
};
window.addEventListener(ADE_OPEN_BUILT_IN_BROWSER_EVENT, openBrowserSidebar);
return () => window.removeEventListener(ADE_OPEN_BUILT_IN_BROWSER_EVENT, openBrowserSidebar);
- }, [setViewMode, setWorkSidebarTab]);
+ }, [setViewMode, setWorkSidebarOpen, setWorkSidebarTab]);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const { setViewMode, setWorkSidebarTab } = work; | |
| useEffect(() => { | |
| const openBrowserSidebar = () => { | |
| setViewMode("tabs"); | |
| setWorkSidebarTab("browser"); | |
| }; | |
| window.addEventListener(ADE_OPEN_BUILT_IN_BROWSER_EVENT, openBrowserSidebar); | |
| return () => window.removeEventListener(ADE_OPEN_BUILT_IN_BROWSER_EVENT, openBrowserSidebar); | |
| }, [setViewMode, setWorkSidebarTab]); | |
| const { setViewMode, setWorkSidebarOpen, setWorkSidebarTab } = work; | |
| useEffect(() => { | |
| const openBrowserSidebar = () => { | |
| setViewMode("tabs"); | |
| setWorkSidebarOpen(true); | |
| setWorkSidebarTab("browser"); | |
| }; | |
| window.addEventListener(ADE_OPEN_BUILT_IN_BROWSER_EVENT, openBrowserSidebar); | |
| return () => window.removeEventListener(ADE_OPEN_BUILT_IN_BROWSER_EVENT, openBrowserSidebar); | |
| }, [setViewMode, setWorkSidebarOpen, setWorkSidebarTab]); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/desktop/src/renderer/components/terminals/TerminalsPage.tsx` around
lines 421 - 429, The ADE_OPEN_BUILT_IN_BROWSER_EVENT handler currently only
calls setViewMode("tabs") and setWorkSidebarTab("browser") but does not open the
sidebar when work.workSidebarOpen is false; update the openBrowserSidebar
function to also ensure the sidebar is shown (e.g. call the appropriate setter
such as setWorkSidebarOpen(true) or setWorkSidebarVisible(true) /
setWorkSidebarOpen to true depending on the available API) so that the browser
tab becomes visible; keep the other calls to setViewMode and setWorkSidebarTab
as-is.
| const selectLaneForActiveTab = useCallback( | ||
| (sessionId: string | null) => { | ||
| if (!sessionId || viewMode === "grid") return; | ||
| const session = sessionsById.get(sessionId); | ||
| if (!session) return; | ||
| selectLane(session.laneId); | ||
| }, | ||
| [selectLane, sessionsById, viewMode], |
There was a problem hiding this comment.
Route-gate the shared lane-sync helper, not just the effect.
selectLaneForActiveTab() is also called from setSelectedSessionId, setActiveItemId, and openSessionTab, so tab activation can still change the selected lane while the user is on /files or another non-Work route. The new useEffect guard only fixes the restore-after-load path.
Suggested fix
const selectLaneForActiveTab = useCallback(
(sessionId: string | null) => {
- if (!sessionId || viewMode === "grid") return;
+ if (!isWorkRoute || !sessionId || viewMode === "grid") return;
const session = sessionsById.get(sessionId);
if (!session) return;
selectLane(session.laneId);
},
- [selectLane, sessionsById, viewMode],
+ [isWorkRoute, selectLane, sessionsById, viewMode],
);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const selectLaneForActiveTab = useCallback( | |
| (sessionId: string | null) => { | |
| if (!sessionId || viewMode === "grid") return; | |
| const session = sessionsById.get(sessionId); | |
| if (!session) return; | |
| selectLane(session.laneId); | |
| }, | |
| [selectLane, sessionsById, viewMode], | |
| const selectLaneForActiveTab = useCallback( | |
| (sessionId: string | null) => { | |
| if (!isWorkRoute || !sessionId || viewMode === "grid") return; | |
| const session = sessionsById.get(sessionId); | |
| if (!session) return; | |
| selectLane(session.laneId); | |
| }, | |
| [isWorkRoute, selectLane, sessionsById, viewMode], | |
| ); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/desktop/src/renderer/components/terminals/useWorkSessions.ts` around
lines 343 - 350, selectLaneForActiveTab currently only returns early in an
effect but is also invoked from setSelectedSessionId, setActiveItemId, and
openSessionTab, so those callers can change the selected lane while the user is
on non-Work routes; move the route guard into selectLaneForActiveTab itself: at
the top of the selectLaneForActiveTab callback add a check for the current
route/view (e.g. viewMode !== "grid" or your app's Work-route predicate) and
return early if not on the Work route, then keep the existing sessionId/session
lookup and selectLane(session.laneId) behavior so all callers
(setSelectedSessionId, setActiveItemId, openSessionTab) automatically honor the
route gate.
| export function openUrlInAdeBrowser(url: string | undefined | null): void { | ||
| const normalized = normalizeBrowserUrlInput(url); | ||
| if (!normalized || !canOpenInAdeBrowser(normalized)) { | ||
| openExternalUrl(url); | ||
| return; |
There was a problem hiding this comment.
Don't forward rejected schemes to the external opener.
canOpenInAdeBrowser() correctly rejects non-HTTP(S) input, but this branch still passes the original value to openExternalUrl(). That means inputs like javascript:... or data:... still reach window.open/the bridge instead of being dropped or explicitly allowlisted.
Suggested fix
export function openUrlInAdeBrowser(url: string | undefined | null): void {
const normalized = normalizeBrowserUrlInput(url);
- if (!normalized || !canOpenInAdeBrowser(normalized)) {
- openExternalUrl(url);
+ if (!normalized) return;
+ if (!canOpenInAdeBrowser(normalized)) {
+ if (/^(mailto|tel):/i.test(normalized)) {
+ openExternalUrl(normalized);
+ }
return;
}As per coding guidelines, "apps/desktop/src/**: Electron desktop app — check for IPC security, proper main/renderer process separation, and React best practices."
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export function openUrlInAdeBrowser(url: string | undefined | null): void { | |
| const normalized = normalizeBrowserUrlInput(url); | |
| if (!normalized || !canOpenInAdeBrowser(normalized)) { | |
| openExternalUrl(url); | |
| return; | |
| export function openUrlInAdeBrowser(url: string | undefined | null): void { | |
| const normalized = normalizeBrowserUrlInput(url); | |
| if (!normalized) return; | |
| if (!canOpenInAdeBrowser(normalized)) { | |
| if (/^(mailto|tel):/i.test(normalized)) { | |
| openExternalUrl(normalized); | |
| } | |
| return; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/desktop/src/renderer/lib/openExternal.ts` around lines 27 - 31, The
branch currently passes the original raw url to openExternalUrl even when
canOpenInAdeBrowser(normalized) rejects unsafe schemes; update
openUrlInAdeBrowser so it uses the normalized value and never forwards a
rejected scheme to the external opener: call openExternalUrl(normalized) only
when normalized is truthy and canOpenInAdeBrowser(normalized) returns true, and
otherwise drop/return without calling openExternalUrl. Reference:
openUrlInAdeBrowser, normalizeBrowserUrlInput, canOpenInAdeBrowser,
openExternalUrl.
| @AppStorage("ade.work.laneFilter") var selectedLaneId = "all" | ||
| @AppStorage("ade.work.statusFilter") private var selectedStatusRawValue = WorkSessionStatusFilter.all.rawValue |
There was a problem hiding this comment.
Persisted lane filter can strand the list behind a stale lane id.
Line 46 persists selectedLaneId globally, but invalid/stale lane ids are not reconciled when lane data changes. With current WorkFiltersSection fallback behavior (apps/ios/ADE/Views/Work/WorkRootComponents.swift:9-31), this can hide all sessions while showing “All lanes.”
Suggested patch
+ private func sanitizePersistedLaneFilter() {
+ if selectedLaneId != "all" && !lanes.contains(where: { $0.id == selectedLaneId }) {
+ selectedLaneId = "all"
+ }
+ }
+
var body: some View {
NavigationStack(path: $path) {
ScrollViewReader { proxy in
List {
...
}
+ .onAppear {
+ sanitizePersistedLaneFilter()
+ }
+ .onChange(of: lanes.map(\.id)) { _, _ in
+ sanitizePersistedLaneFilter()
+ }
...🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/ios/ADE/Views/Work/WorkRootScreen.swift` around lines 46 - 47, Persisted
selectedLaneId (AppStorage in WorkRootScreen) can be stale and hide sessions;
validate and reconcile it against the current lanes whenever lane data changes.
Update the code that receives/loads lane data (e.g., after fetching in
WorkRootScreen or in the view model backing WorkFiltersSection) to check if
selectedLaneId exists in the current lanes' id set and if not set selectedLaneId
= "all" (or the WorkFiltersSection fallback) so the UI won’t be stranded on an
invalid lane id; you can implement this in the lanes state observer, onAppear,
or a validateSelectedLane() helper invoked after lanes update.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
arul28
force-pushed
the
ade/ade-browser-d5ca6ac8
branch
from
3d31a5b to
f781138
Compare
|
@copilot review but do not make fixes |
Here's the updated review, incorporating the Greptile P1/P2 findings alongside the issues from my previous pass. Still no fixes. P1 Bugs (state corruption confirmed)1. 2. Previously Identified Issues (still open)3. 4. Password field value leaked via CDP ( 5. Debugger state is a singleton across all tabs 6. CSP 7. 8. 9. Double 10. Renderer type declarations diverge from shared types |
- Browser service: captureScreenshot/selectPoint throw on no tab instead of auto-creating a blank one - navigate(newTab) and createTab throw the tab-limit error before clearing inspect/selection state - NODE_METADATA_FUNCTION drops value for <input type=password> to prevent leaking it through agent prompts - Debugger inspect listeners are tracked per-WebContents and detached when switching tabs mid-inspect - handleInspectNodeRequested no longer double-calls stopInspect (finally-only) - main.ts CSP: drop https: from frame-src; <webview>/WebContentsView don't need it - ChatBuiltInBrowserPanel: reject file:/blob:/data:/devtools: in normalizeUrlForNavigation with a clear toast - ChatBuiltInBrowserPanel: import shared BuiltInBrowserTab; collapse loading vs isLoading drift - ChatBuiltInBrowserPanel: clear autoAttachedContextIdsRef on URL/active-tab change - WorkSidebar: null-guard window.ade.builtInBrowser; only hide when previous tab was browser Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
@copilot review but do not make fixes |
Here's the updated review for this round's Greptile findings. Still no fixes. Greptile P1 — Already Fixed
Greptile P2 — Confirmed Open
Previously Identified Issues (still open)P1 × 2 — navigate() / createTab() premature side-effects ( CSP
Password field value in CDP metadata ( Debugger state singleton across tabs (closure-level
|
…b race - Browser service: reload/goBack/goForward/stop/startInspect now use currentWebContents() + early throw, matching the iteration-1 fix for captureScreenshot/selectPoint (Greptile P1) - dispose() clears inspecting/debuggerAttachedForInspect and detaches debugger listeners synchronously before the async teardown so an in-flight CDP callback can't run against torn-down state (Copilot P2) - openExternal.ts marks a pendingBuiltInBrowserNavigation flag before dispatching ADE_OPEN_BUILT_IN_BROWSER_EVENT; ChatBuiltInBrowserPanel consumes the flag in the default-tab effect to suppress the empty-state Google tab when an external link triggered the open, eliminating the double-tab race (Greptile P1) Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
@copilot review but do not make fixes |
Here's the updated review for this round's Greptile findings. Still no fixes. Greptile P1 — Already Fixed: phantom blank tabs in page-control methods
Greptile P1 — Partially Addressed: default-tab race on link click
If the Fix: the flag should always be consumed (and cleared) if it was set, regardless of whether the default tab was actually suppressed. One way: check and consume at the top of the effect unconditionally when Previously Identified Issues (still open)
Password field value in CDP metadata — Debugger state singleton across tabs —
CSP |
Consume consumePendingBuiltInBrowserNavigation() as soon as the panel has an API and status, before the browserTabs.length early return. Without this, a fast status update with tabs already present on first render leaves the flag set, which then suppresses the default tab on a later panel reopen with zero tabs. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
@copilot review but do not make fixes |
Here's the updated review for this round. Still no fixes. Previously Identified Issues — Now Fixed
Password field value in CDP metadata — Fixed. Greptile P2 — Confirmed Open
Previously Identified Issues (still open)
Debugger state singleton across tabs —
CSP |
There was a problem hiding this comment.
Actionable comments posted: 7
♻️ Duplicate comments (9)
apps/desktop/src/renderer/components/chat/AgentChatPane.tsx (2)
3624-3632:⚠️ Potential issue | 🟠 Major | ⚡ Quick winKeep browser temp screenshots out of the shared attachment pairing ref.
This save path still goes through
addAttachment(), which updateslatestAttachmentRef. The next iOS/App Control selection inside the 10s reuse window can therefore pick up this browser PNG instead of its own screenshot.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/components/chat/AgentChatPane.tsx` around lines 3624 - 3632, The browser temp screenshot save is still calling addAttachment(...) which updates latestAttachmentRef and causes subsequent iOS/App selections to reuse the browser PNG; change the flow in the AgentChatPane logic that handles item.screenshotDataUrl (the block calling window.ade.agentChat.saveTempAttachment and addAttachment) so that after saving via agentChat.saveTempAttachment(...) you only set attachmentPath = saved.path (and use inferAttachmentType(saved.path, "image/png") locally if needed) but do NOT call addAttachment(...) or otherwise update latestAttachmentRef for this temp browser screenshot; ensure addAttachment remains used for real user attachments only.
4252-4260:⚠️ Potential issue | 🟠 Major | ⚡ Quick winParallel launch still drops visual context.
These snapshots/prefixes are only built on the normal send path. When
isParallelLaunchis true, the early branch above still forwards only the draft and attachments, so the built-in browser/iOS/App Control chips shown in the composer never reach the child lanes.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/components/chat/AgentChatPane.tsx` around lines 4252 - 4260, When isParallelLaunch is true the early branch still only forwards draft and attachments and never includes the visual context computed by builtInBrowserContextSnapshot, iosContextSnapshot, appControlContextSnapshot or their derived values (visualContextPrefix, visualContextDisplayChips). Update the parallel-launch branch that forwards to child lanes to also compute or reuse the same iosContextPrefix, appControlContextPrefix, builtInBrowserContextPrefix and corresponding display chips and include visualContextPrefix and visualContextDisplayChips in the payload sent to the child lanes (the same fields used on the normal send path), so the built-in browser/iOS/App Control chips present in the composer are forwarded in parallel launches.apps/desktop/src/renderer/lib/openExternal.ts (1)
45-49:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon't forward rejected schemes to the external opener.
canOpenInAdeBrowser()rejects non-HTTP(S) input, but this branch still passes the originalurltoopenExternalUrl(). Inputs likejavascript:ordata:are therefore not actually blocked here.As per coding guidelines, "
apps/desktop/src/**: Electron desktop app — check for IPC security, proper main/renderer process separation, and React best practices."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/lib/openExternal.ts` around lines 45 - 49, openUrlInAdeBrowser currently forwards the original unnormalized url to openExternalUrl even when canOpenInAdeBrowser(normalized) rejects unsafe schemes; update openUrlInAdeBrowser so it first computes normalized = normalizeBrowserUrlInput(url) and then only calls openExternalUrl with the normalized value for allowed schemes (i.e., call openExternalUrl(normalized) only when normalized is truthy and canOpenInAdeBrowser(normalized) is true); if normalized is falsy or canOpenInAdeBrowser(normalized) is false, do not call openExternalUrl (simply return) to ensure rejected schemes like "javascript:" or "data:" are never forwarded.apps/desktop/src/main/services/builtInBrowser/builtInBrowserService.ts (2)
748-769:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRemove debugger listeners in the detach callback instead of just nulling the refs.
Lines 759-765 clear the listener references without calling
detachDebuggerListeners(wc). If the debugger detaches unexpectedly, the old callbacks stay registered onwc.debugger, so the next inspect session on the sameWebContentscan double-handle events.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/main/services/builtInBrowser/builtInBrowserService.ts` around lines 748 - 769, In debuggerDetachListener (created inside attachDebuggerListeners), replace the manual nulling of listener references with a call to detachDebuggerListeners(wc) so the handlers are actually removed from wc.debugger; ensure debuggerDetachListener still logs and updates inspecting/debuggerAttachedForInspect state (and that detachDebuggerListeners clears debuggerMessageListener, debuggerDetachListener, inspectListenerWebContents and calls wc.debugger.off/.removeListener for the stored callbacks) to prevent duplicated handlers on subsequent inspect sessions.
394-400:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winReject non-browser
webContentsIdvalues inattachWebview().Lines 394-400 still trust any renderer-supplied
webContentsIdafter the null check. A compromised renderer can bind arbitrary appWebContentsinto this service and then usenavigate,startInspect, orcaptureScreenshotagainst privileged content. This needs a type/partition check before the rebinding step.As per coding guidelines, "Electron desktop app — check for IPC security, proper main/renderer process separation, and React best practices."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/main/services/builtInBrowser/builtInBrowserService.ts` around lines 394 - 400, Reject non-browser webContentsId values by verifying the WebContents type and partition before rebinding: after obtaining nextWebContents via electronWebContents.fromId(input.webContentsId) (and the existing null/isDestroyed checks) assert nextWebContents.getType() === 'webview' and also validate nextWebContents.getWebPreferences().partition matches the expected browser partition pattern (or a configured allowed partition) and throw an Error if not; perform these checks in attachWebview (the block that calls configureBrowserSession() and configureBrowserWebContents(nextWebContents)) so only genuine webview/guest contents are accepted.apps/ade-cli/src/cli.ts (1)
2620-2677:⚠️ Potential issue | 🟠 MajorFail fast when browser subcommands run without a desktop socket.
These plans still target the
built_in_browserdomain after the CLI falls back to headless mode, so they fail late withDomain 'built_in_browser' is unavailable in this runtimeinstead of surfacing a clear usage error up front. The help text already documents--socket, so this still looks unresolved.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ade-cli/src/cli.ts` around lines 2620 - 2677, buildBrowserPlan currently builds plans targeting the built_in_browser domain even when the CLI is running in headless mode (no desktop socket), causing late runtime errors; fix by validating desktop-socket availability up-front: add/consume a helper like ensureDesktopSocketAvailable() (or implement a small check) and call it at the start of buildBrowserPlan to throw a CliUsageError with a clear message (mentioning --socket and HELP_BY_COMMAND.browser if helpful) whenever the desktop socket is not available, so plans targeting "built_in_browser" fail fast instead of producing "Domain 'built_in_browser' is unavailable in this runtime" at execution time.apps/desktop/src/main/services/ipc/registerIpc.ts (1)
6447-6450:⚠️ Potential issue | 🔴 Critical | ⚡ Quick win
webContentsIdis still not bound to the requesting renderer.
guardBuiltInBrowserIpc()only verifies the caller is an ADE renderer. This handler still forwards an arbitrarywebContentsIdintoattachWebview(), so a compromised renderer can target anotherWebContentsunless ownership is checked againstevent.senderor itsBrowserWindowfirst.As per coding guidelines, "Electron desktop app — check for IPC security, proper main/renderer process separation, and React best practices."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/main/services/ipc/registerIpc.ts` around lines 6447 - 6450, The handler for IPC.builtInBrowserAttachWebview currently trusts the client-supplied webContentsId; after guardBuiltInBrowserIpc(...) and before calling ensureBuiltInBrowser().attachWebview(...), verify that the webContentsId in parseBuiltInBrowserAttachWebviewArgs(arg, IPC.builtInBrowserAttachWebview) belongs to the requesting renderer (compare it to event.sender.id or to BrowserWindow.fromWebContents(event.sender).webContents.id/owner window id) and reject/throw if it does not match; keep the existing guard call and only forward the parsed args to attachWebview when ownership is confirmed.apps/desktop/src/main/main.ts (1)
3450-3450:⚠️ Potential issue | 🟠 Major | ⚡ Quick winKeep the live
AppContextshape consistent with the dormant one.Line 3450 wires
builtInBrowserServiceintorpcRuntime, and Line 3773 adds it to the dormant context, but the live object returned frominitContextForProjectRoot()still omitsbuiltInBrowserService.getActiveContext().builtInBrowserServicewill therefore beundefinedfor open projects and onlynullwhen dormant.♻️ Suggested fix
computerUseArtifactBrokerService, iosSimulatorService, appControlService, + builtInBrowserService, queueLandingService, issueInventoryService,Also applies to: 3773-3773
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/main/main.ts` at line 3450, The live AppContext returned by initContextForProjectRoot() is missing the builtInBrowserService property (rpcRuntime wires it in and the dormant context includes it), causing getActiveContext().builtInBrowserService to be undefined for active projects; update initContextForProjectRoot() to include a builtInBrowserService field (initialized the same way or to null/placeholder consistent with the dormant context) so the live and dormant context shapes match, and ensure any callers (e.g., code expecting getActiveContext().builtInBrowserService) handle the same null/instance semantics.apps/desktop/src/renderer/components/terminals/TerminalsPage.tsx (1)
420-429:⚠️ Potential issue | 🟠 Major | ⚡ Quick winActually open the sidebar on
ADE_OPEN_BUILT_IN_BROWSER_EVENT.Line 420 makes visibility depend on
work.workSidebarOpen, but Lines 423-425 only switch the view mode and active tab. If the sidebar is collapsed, the “open built-in browser” event still leaves the browser hidden.♻️ Suggested fix
- const { setViewMode, setWorkSidebarTab } = work; + const { setViewMode, setWorkSidebarOpen, setWorkSidebarTab } = work; useEffect(() => { const openBrowserSidebar = () => { setViewMode("tabs"); + setWorkSidebarOpen(true); setWorkSidebarTab("browser"); }; window.addEventListener(ADE_OPEN_BUILT_IN_BROWSER_EVENT, openBrowserSidebar); return () => window.removeEventListener(ADE_OPEN_BUILT_IN_BROWSER_EVENT, openBrowserSidebar); - }, [setViewMode, setWorkSidebarTab]); + }, [setViewMode, setWorkSidebarOpen, setWorkSidebarTab]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/components/terminals/TerminalsPage.tsx` around lines 420 - 429, The handler for ADE_OPEN_BUILT_IN_BROWSER_EVENT changes only view/tab but doesn't actually open the sidebar when it's collapsed; update the openBrowserSidebar function to also set the sidebar open (call the work sidebar open setter, e.g. setWorkSidebarOpen(true) or the appropriate method on the work object) so the browser becomes visible, and add that setter (setWorkSidebarOpen) to the useEffect dependency array along with setViewMode and setWorkSidebarTab.
🧹 Nitpick comments (3)
apps/desktop/src/renderer/components/chat/ChatBuiltInBrowserPanel.tsx (2)
52-67: 💤 Low valueLocal type duplicates and diverges from shared type.
The local
BuiltInBrowserContextItemtype differs from the shared type inapps/desktop/src/shared/types/builtInBrowser.ts. The shared type requiresprovider,componentId,pixelFrame,sourceFile,sourceLinewhile this local type addsselector,text,role,tagName,sessionIdwith an open index signature.Consider importing and extending the shared type to maintain consistency:
import type { BuiltInBrowserContextItem as SharedContextItem } from "../../../shared/types/builtInBrowser"; type BuiltInBrowserContextItem = SharedContextItem & { selector?: string | null; text?: string | null; // ... additional fields };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/components/chat/ChatBuiltInBrowserPanel.tsx` around lines 52 - 67, Replace the locally defined BuiltInBrowserContextItem with an extension of the shared type: import the shared BuiltInBrowserContextItem (refer to it here as SharedContextItem) and define the local type as SharedContextItem & { selector?: string | null; text?: string | null; role?: string | null; tagName?: string | null; sessionId?: string | null; screenshotDataUrl?: string | null; selectedAt?: string }; ensure you remove the open index signature ([key: string]: unknown) and any divergent required fields so the local type stays consistent with the shared BuiltInBrowserContextItem while adding only the extra optional properties used by ChatBuiltInBrowserPanel.tsx.
957-957: The dependency array optimization is safe but could be clearer.The effect correctly depends on
tabIdsSignaturerather than the fullbrowserTabsarray because tab URL changes are driven by the main process, which directly controls the webview via Electron's webContents API (not the renderer'ssrcattribute). The renderer's webviewsrcis only set during initial load (whenpendingUrlexists or the tab isn't yet attached).When the main process navigates a tab, the URL is updated in the status object and
statusRefis refreshed immediately. For the active tab's fallback URL,statusRef.current?.urlwill have the fresh value, so the logic remains sound.However, the dependency array could be more maintainable if documented, since future developers might not immediately understand why
tab.urlis accessed without a corresponding dependency on URL changes. If you want to add a safety margin:+const tabUrlsSignature = useMemo(() => browserTabs.map(t => `${t.id}:${t.url ?? ''}`).join('|'), [browserTabs]); -}, [activeTabId, applyStatus, tabIdsSignature, captureBase, webviewNavigationNonce]); +}, [activeTabId, applyStatus, tabUrlsSignature, captureBase, webviewNavigationNonce]);This ensures the effect re-runs if tab URLs change externally, improving clarity at minimal performance cost.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/components/chat/ChatBuiltInBrowserPanel.tsx` at line 957, The effect currently depends on activeTabId, applyStatus, tabIdsSignature, captureBase, and webviewNavigationNonce but accesses tab.url via browserTabs/statusRef; add a short comment above the useEffect explaining why tab URLs are not included (main process drives navigation and statusRef is updated) and to be safer/clearer add the active tab's URL to the dependency array (e.g. use activeTab?.url or a derived tabUrlsSignature) so the effect re-runs when the active tab's URL changes externally; update the dependency list in the useEffect that references activeTabId, applyStatus, tabIdsSignature, captureBase, webviewNavigationNonce and keep statusRef/pendingUrl logic as-is.apps/desktop/src/renderer/components/files/FilesPage.tsx (1)
169-176: 💤 Low valueSecond
findfallback indefaultFilesWorkspaceIdis effectively unreachablePer the
FilesWorkspacetype (laneId: string | null), primary workspaces havelaneId = null. The secondfind(withoutkind !== "primary") can only produce a different result than the first if a primary workspace has a non-nulllaneId— which appears never to occur. The fallback can be simplified:♻️ Suggested simplification
function defaultFilesWorkspaceId(workspaces: FilesWorkspace[], preferredLaneId: string | null): string { if (preferredLaneId) { - const laneWorkspace = workspaces.find((workspace) => workspace.kind !== "primary" && workspace.laneId === preferredLaneId) - ?? workspaces.find((workspace) => workspace.laneId === preferredLaneId); + const laneWorkspace = workspaces.find((workspace) => workspace.laneId === preferredLaneId); if (laneWorkspace) return laneWorkspace.id; } return workspaces[0]?.id ?? ""; }If primary workspaces can have a non-null
laneIdin some future scenario, the current double-find order (prefer non-primary, fall back to primary) would need to be preserved.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/components/files/FilesPage.tsx` around lines 169 - 176, The fallback second find in function defaultFilesWorkspaceId is redundant given FilesWorkspace.laneId is null for primary workspaces; simplify by removing the second find and just search once for a non-primary workspace matching preferredLaneId (using workspaces.find workspace => workspace.kind !== "primary" && workspace.laneId === preferredLaneId), returning its id if found, otherwise fall back to workspaces[0]?.id ?? ""; if you expect primary workspaces might later carry a laneId, preserve the current two-step preference logic (first prefer non-primary then primary) when making 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/ade-cli/src/cli.ts`:
- Line 3238: The help-resolution alias map is missing the new browser names, so
help commands like "ade built-in-browser --help" still fall back to top-level
help; update the same alias set used by the help path to include "browser",
"ade-browser", "built-in-browser", and "builtin-browser" and point them to the
browser command so help lookup resolves the same command that the execution
dispatch (where primary is compared and buildBrowserPlan is called) uses. Locate
the alias mapping used by the help/resolution code (the same map that the help
path consults) and add entries for those four aliases that map to the existing
browser command key so help and execution are consistent.
In `@apps/desktop/src/main/services/builtInBrowser/builtInBrowserService.ts`:
- Around line 443-468: In navigate(), don't clear inspect/selection state or
commit tab changes until the target tab is validated and navigation succeeds:
first resolve which tab will be used (validate input.tabId against tabs or use
ensureActiveTab) before calling stopInspectQuietly() or
clearSelectionInternal(); for new tabs created by createTabState(), treat them
as provisional (do not push to tabs or set activeTabId) until
wc.loadURL(targetUrl) completes successfully; perform wc.loadURL inside
try/catch and on failure rollback any provisional tab or avoid mutating
tabs/activeTabId, then only call emitStatus() and return getStatus() after
successful load; ensure references to navigate, stopInspectQuietly,
clearSelectionInternal, createTabState, tabs, activeTabId, wc.loadURL,
ensureActiveTab, emitStatus, and getStatus are updated accordingly.
In `@apps/desktop/src/main/services/ipc/registerIpc.ts`:
- Around line 2230-2235: The parser functions
parseBuiltInBrowserAttachWebviewArgs and parseBuiltInBrowserTabArgs currently
reject missing/blank tabId but the shared IPC types declare tabId?: string |
null; update these parsers to align with the shared contract by removing the
unconditional invalidBuiltInBrowserArg call and instead: use
optionalBuiltInBrowserString(record, "tabId", channel, 128) to allow
undefined/null, only call invalidBuiltInBrowserArg if tabId is present but
equals the empty string, and ensure the returned type allows tabId to be
undefined|null; reference the existing helpers builtInBrowserRecord,
optionalBuiltInBrowserString, and invalidBuiltInBrowserArg when making the
change.
In `@apps/desktop/src/renderer/components/chat/AgentChatComposer.tsx`:
- Around line 2146-2148: The component introduced context-only sends via
hasBuiltInBrowserContext and singleReady, but submitComposerDraft() (Cursor
Cloud branch) and the active-turn steer button still require draft text, causing
inconsistent behavior; add a shared predicate (e.g., hasContextSelection)
computed from hasBuiltInBrowserContext || hasIosElementContext ||
hasAppControlContext and use that instead of ad-hoc checks so all submit/steer
paths (submitComposerDraft(), the steer button render logic, sendEnabled
calculation, and any Cursor Cloud branches) consistently allow context-only
sends, or alternatively revert those branches to require draft.trim().length > 0
to keep text-only behavior—update references to parallelChatMode, modelId,
draft, singleReady, parallelReady, composerInputLocked, parallelLaunchBusy and
busy to use the unified predicate.
In `@apps/desktop/src/renderer/components/chat/ChatIosSimulatorPanel.tsx`:
- Around line 2221-2224: addSimulatorCaptureContext and addPreviewCaptureContext
currently return silently when onAddContext is missing; mirror selectElementAt's
behavior by showing the same user-facing message ("Chat attachments are not
available in this panel.") instead of silently returning. Locate
addSimulatorCaptureContext and addPreviewCaptureContext and add the same
early-check/how-to-notify logic used in selectElementAt (use the same
toast/notification call or helper) so the UI gives consistent feedback when
onAddContext is undefined.
In `@apps/desktop/src/renderer/components/files/FilesPage.tsx`:
- Around line 1311-1316: The reconciliation effect that sets workspaceId (using
defaultFilesWorkspaceId and setWorkspaceId) can change the workspace without
clearing per-workspace state (openTabs, activeTabPath, selectedNodePath),
causing stale tabs to operate against the wrong workspace; modify that effect to
either call the existing switchWorkspace(nextWorkspaceId) callback (preferred)
or, if calling it would cause cycles, replicate its behavior: when
nextWorkspaceId differs from current, clear openTabs, reset activeTabPath and
selectedNodePath to null (or their initial values), then
setWorkspaceId(nextWorkspaceId) so that tab state is reset consistently with the
switchWorkspace logic.
In `@apps/desktop/src/renderer/components/terminals/useWorkSessions.ts`:
- Around line 465-468: The setter setWorkSidebarWidthPct currently uses
Math.max/Math.min which produces NaN for non-finite inputs; change
setWorkSidebarWidthPct to first validate widthPct with Number.isFinite (or
isFinite) and, if not finite, substitute the same default used in
normalizeWorkSidebarWidthPct (36) before clamping to the allowed range (26–55)
and calling setProjectViewState; reference the setWorkSidebarWidthPct callback
and normalizeWorkSidebarWidthPct in appStore when implementing this guard.
---
Duplicate comments:
In `@apps/ade-cli/src/cli.ts`:
- Around line 2620-2677: buildBrowserPlan currently builds plans targeting the
built_in_browser domain even when the CLI is running in headless mode (no
desktop socket), causing late runtime errors; fix by validating desktop-socket
availability up-front: add/consume a helper like ensureDesktopSocketAvailable()
(or implement a small check) and call it at the start of buildBrowserPlan to
throw a CliUsageError with a clear message (mentioning --socket and
HELP_BY_COMMAND.browser if helpful) whenever the desktop socket is not
available, so plans targeting "built_in_browser" fail fast instead of producing
"Domain 'built_in_browser' is unavailable in this runtime" at execution time.
In `@apps/desktop/src/main/main.ts`:
- Line 3450: The live AppContext returned by initContextForProjectRoot() is
missing the builtInBrowserService property (rpcRuntime wires it in and the
dormant context includes it), causing getActiveContext().builtInBrowserService
to be undefined for active projects; update initContextForProjectRoot() to
include a builtInBrowserService field (initialized the same way or to
null/placeholder consistent with the dormant context) so the live and dormant
context shapes match, and ensure any callers (e.g., code expecting
getActiveContext().builtInBrowserService) handle the same null/instance
semantics.
In `@apps/desktop/src/main/services/builtInBrowser/builtInBrowserService.ts`:
- Around line 748-769: In debuggerDetachListener (created inside
attachDebuggerListeners), replace the manual nulling of listener references with
a call to detachDebuggerListeners(wc) so the handlers are actually removed from
wc.debugger; ensure debuggerDetachListener still logs and updates
inspecting/debuggerAttachedForInspect state (and that detachDebuggerListeners
clears debuggerMessageListener, debuggerDetachListener,
inspectListenerWebContents and calls wc.debugger.off/.removeListener for the
stored callbacks) to prevent duplicated handlers on subsequent inspect sessions.
- Around line 394-400: Reject non-browser webContentsId values by verifying the
WebContents type and partition before rebinding: after obtaining nextWebContents
via electronWebContents.fromId(input.webContentsId) (and the existing
null/isDestroyed checks) assert nextWebContents.getType() === 'webview' and also
validate nextWebContents.getWebPreferences().partition matches the expected
browser partition pattern (or a configured allowed partition) and throw an Error
if not; perform these checks in attachWebview (the block that calls
configureBrowserSession() and configureBrowserWebContents(nextWebContents)) so
only genuine webview/guest contents are accepted.
In `@apps/desktop/src/main/services/ipc/registerIpc.ts`:
- Around line 6447-6450: The handler for IPC.builtInBrowserAttachWebview
currently trusts the client-supplied webContentsId; after
guardBuiltInBrowserIpc(...) and before calling
ensureBuiltInBrowser().attachWebview(...), verify that the webContentsId in
parseBuiltInBrowserAttachWebviewArgs(arg, IPC.builtInBrowserAttachWebview)
belongs to the requesting renderer (compare it to event.sender.id or to
BrowserWindow.fromWebContents(event.sender).webContents.id/owner window id) and
reject/throw if it does not match; keep the existing guard call and only forward
the parsed args to attachWebview when ownership is confirmed.
In `@apps/desktop/src/renderer/components/chat/AgentChatPane.tsx`:
- Around line 3624-3632: The browser temp screenshot save is still calling
addAttachment(...) which updates latestAttachmentRef and causes subsequent
iOS/App selections to reuse the browser PNG; change the flow in the
AgentChatPane logic that handles item.screenshotDataUrl (the block calling
window.ade.agentChat.saveTempAttachment and addAttachment) so that after saving
via agentChat.saveTempAttachment(...) you only set attachmentPath = saved.path
(and use inferAttachmentType(saved.path, "image/png") locally if needed) but do
NOT call addAttachment(...) or otherwise update latestAttachmentRef for this
temp browser screenshot; ensure addAttachment remains used for real user
attachments only.
- Around line 4252-4260: When isParallelLaunch is true the early branch still
only forwards draft and attachments and never includes the visual context
computed by builtInBrowserContextSnapshot, iosContextSnapshot,
appControlContextSnapshot or their derived values (visualContextPrefix,
visualContextDisplayChips). Update the parallel-launch branch that forwards to
child lanes to also compute or reuse the same iosContextPrefix,
appControlContextPrefix, builtInBrowserContextPrefix and corresponding display
chips and include visualContextPrefix and visualContextDisplayChips in the
payload sent to the child lanes (the same fields used on the normal send path),
so the built-in browser/iOS/App Control chips present in the composer are
forwarded in parallel launches.
In `@apps/desktop/src/renderer/components/terminals/TerminalsPage.tsx`:
- Around line 420-429: The handler for ADE_OPEN_BUILT_IN_BROWSER_EVENT changes
only view/tab but doesn't actually open the sidebar when it's collapsed; update
the openBrowserSidebar function to also set the sidebar open (call the work
sidebar open setter, e.g. setWorkSidebarOpen(true) or the appropriate method on
the work object) so the browser becomes visible, and add that setter
(setWorkSidebarOpen) to the useEffect dependency array along with setViewMode
and setWorkSidebarTab.
In `@apps/desktop/src/renderer/lib/openExternal.ts`:
- Around line 45-49: openUrlInAdeBrowser currently forwards the original
unnormalized url to openExternalUrl even when canOpenInAdeBrowser(normalized)
rejects unsafe schemes; update openUrlInAdeBrowser so it first computes
normalized = normalizeBrowserUrlInput(url) and then only calls openExternalUrl
with the normalized value for allowed schemes (i.e., call
openExternalUrl(normalized) only when normalized is truthy and
canOpenInAdeBrowser(normalized) is true); if normalized is falsy or
canOpenInAdeBrowser(normalized) is false, do not call openExternalUrl (simply
return) to ensure rejected schemes like "javascript:" or "data:" are never
forwarded.
---
Nitpick comments:
In `@apps/desktop/src/renderer/components/chat/ChatBuiltInBrowserPanel.tsx`:
- Around line 52-67: Replace the locally defined BuiltInBrowserContextItem with
an extension of the shared type: import the shared BuiltInBrowserContextItem
(refer to it here as SharedContextItem) and define the local type as
SharedContextItem & { selector?: string | null; text?: string | null; role?:
string | null; tagName?: string | null; sessionId?: string | null;
screenshotDataUrl?: string | null; selectedAt?: string }; ensure you remove the
open index signature ([key: string]: unknown) and any divergent required fields
so the local type stays consistent with the shared BuiltInBrowserContextItem
while adding only the extra optional properties used by
ChatBuiltInBrowserPanel.tsx.
- Line 957: The effect currently depends on activeTabId, applyStatus,
tabIdsSignature, captureBase, and webviewNavigationNonce but accesses tab.url
via browserTabs/statusRef; add a short comment above the useEffect explaining
why tab URLs are not included (main process drives navigation and statusRef is
updated) and to be safer/clearer add the active tab's URL to the dependency
array (e.g. use activeTab?.url or a derived tabUrlsSignature) so the effect
re-runs when the active tab's URL changes externally; update the dependency list
in the useEffect that references activeTabId, applyStatus, tabIdsSignature,
captureBase, webviewNavigationNonce and keep statusRef/pendingUrl logic as-is.
In `@apps/desktop/src/renderer/components/files/FilesPage.tsx`:
- Around line 169-176: The fallback second find in function
defaultFilesWorkspaceId is redundant given FilesWorkspace.laneId is null for
primary workspaces; simplify by removing the second find and just search once
for a non-primary workspace matching preferredLaneId (using workspaces.find
workspace => workspace.kind !== "primary" && workspace.laneId ===
preferredLaneId), returning its id if found, otherwise fall back to
workspaces[0]?.id ?? ""; if you expect primary workspaces might later carry a
laneId, preserve the current two-step preference logic (first prefer non-primary
then primary) when making 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 643a9d7d-0237-4b91-bc61-8fba413537f8
⛔ Files ignored due to path filters (11)
.claude/scheduled_tasks.lockis excluded by!**/*.lockdocs/ARCHITECTURE.mdis excluded by!docs/**docs/features/chat/README.mdis excluded by!docs/**docs/features/chat/composer-and-ui.mdis excluded by!docs/**docs/features/computer-use/app-control.mdis excluded by!docs/**docs/features/files-and-editor/README.mdis excluded by!docs/**docs/features/files-and-editor/editor-surfaces.mdis excluded by!docs/**docs/features/ios-simulator/README.mdis excluded by!docs/**docs/features/terminals-and-sessions/README.mdis excluded by!docs/**docs/features/terminals-and-sessions/ui-surfaces.mdis excluded by!docs/**goal.mdis excluded by!*.md
📒 Files selected for processing (48)
apps/ade-cli/src/bootstrap.tsapps/ade-cli/src/cli.test.tsapps/ade-cli/src/cli.tsapps/desktop/src/main/main.tsapps/desktop/src/main/services/adeActions/registry.tsapps/desktop/src/main/services/appControl/appControlService.tsapps/desktop/src/main/services/builtInBrowser/builtInBrowserService.test.tsapps/desktop/src/main/services/builtInBrowser/builtInBrowserService.tsapps/desktop/src/main/services/ios/iosSimulatorService.test.tsapps/desktop/src/main/services/ios/iosSimulatorService.tsapps/desktop/src/main/services/ipc/registerIpc.tsapps/desktop/src/preload/global.d.tsapps/desktop/src/preload/preload.tsapps/desktop/src/renderer/components/app/AppShell.tsxapps/desktop/src/renderer/components/app/FloatingFilesWorkspace.tsxapps/desktop/src/renderer/components/app/RightEdgeFloatingPane.tsxapps/desktop/src/renderer/components/chat/AgentChatComposer.tsxapps/desktop/src/renderer/components/chat/AgentChatMessageList.tsxapps/desktop/src/renderer/components/chat/AgentChatPane.tsxapps/desktop/src/renderer/components/chat/ChatAppControlPanel.tsxapps/desktop/src/renderer/components/chat/ChatBuiltInBrowserPanel.tsxapps/desktop/src/renderer/components/chat/ChatIosSimulatorPanel.test.tsxapps/desktop/src/renderer/components/chat/ChatIosSimulatorPanel.tsxapps/desktop/src/renderer/components/chat/chatMarkdown.tsxapps/desktop/src/renderer/components/files/FilesPage.test.tsxapps/desktop/src/renderer/components/files/FilesPage.tsxapps/desktop/src/renderer/components/lanes/useLaneWorkSessions.tsapps/desktop/src/renderer/components/run/LaneRuntimeBar.tsxapps/desktop/src/renderer/components/terminals/SessionListPane.test.tsxapps/desktop/src/renderer/components/terminals/SessionListPane.tsxapps/desktop/src/renderer/components/terminals/TerminalView.tsxapps/desktop/src/renderer/components/terminals/TerminalsPage.tsxapps/desktop/src/renderer/components/terminals/WorkSidebar.tsxapps/desktop/src/renderer/components/terminals/WorkStartSurface.tsxapps/desktop/src/renderer/components/terminals/WorkViewArea.tsxapps/desktop/src/renderer/components/terminals/useWorkSessions.test.tsapps/desktop/src/renderer/components/terminals/useWorkSessions.tsapps/desktop/src/renderer/index.cssapps/desktop/src/renderer/lib/openExternal.tsapps/desktop/src/renderer/state/appStore.test.tsapps/desktop/src/renderer/state/appStore.tsapps/desktop/src/shared/adeCliGuidance.tsapps/desktop/src/shared/ipc.tsapps/desktop/src/shared/types/appControl.tsapps/desktop/src/shared/types/builtInBrowser.tsapps/desktop/src/shared/types/index.tsapps/desktop/src/shared/types/iosSimulator.tsapps/ios/ADE/Views/Work/WorkRootScreen.swift
💤 Files with no reviewable changes (4)
- apps/desktop/src/renderer/components/app/AppShell.tsx
- apps/desktop/src/renderer/components/app/FloatingFilesWorkspace.tsx
- apps/desktop/src/renderer/index.css
- apps/desktop/src/renderer/components/app/RightEdgeFloatingPane.tsx
✅ Files skipped from review due to trivial changes (19)
- apps/desktop/src/shared/types/index.ts
- apps/desktop/src/renderer/components/terminals/SessionListPane.test.tsx
- apps/desktop/src/renderer/components/run/LaneRuntimeBar.tsx
- apps/desktop/src/renderer/components/chat/ChatIosSimulatorPanel.test.tsx
- apps/desktop/src/shared/types/iosSimulator.ts
- apps/desktop/src/renderer/state/appStore.test.ts
- apps/ade-cli/src/bootstrap.ts
- apps/desktop/src/renderer/components/lanes/useLaneWorkSessions.ts
- apps/desktop/src/renderer/components/files/FilesPage.test.tsx
- apps/desktop/src/renderer/components/terminals/WorkViewArea.tsx
- apps/desktop/src/shared/types/appControl.ts
- apps/desktop/src/main/services/builtInBrowser/builtInBrowserService.test.ts
- apps/desktop/src/shared/adeCliGuidance.ts
- apps/desktop/src/shared/ipc.ts
- apps/desktop/src/main/services/ios/iosSimulatorService.test.ts
- apps/desktop/src/shared/types/builtInBrowser.ts
- apps/ade-cli/src/cli.test.ts
- apps/desktop/src/renderer/components/terminals/WorkSidebar.tsx
- apps/desktop/src/renderer/components/chat/ChatAppControlPanel.tsx
🚧 Files skipped from review as they are similar to previous changes (9)
- apps/desktop/src/renderer/components/terminals/WorkStartSurface.tsx
- apps/desktop/src/main/services/appControl/appControlService.ts
- apps/desktop/src/main/services/ios/iosSimulatorService.ts
- apps/desktop/src/renderer/components/terminals/SessionListPane.tsx
- apps/desktop/src/renderer/components/chat/AgentChatMessageList.tsx
- apps/desktop/src/preload/preload.ts
- apps/desktop/src/renderer/components/chat/chatMarkdown.tsx
- apps/ios/ADE/Views/Work/WorkRootScreen.swift
- apps/desktop/src/renderer/components/terminals/useWorkSessions.test.ts
| const parseBuiltInBrowserAttachWebviewArgs = (value: unknown, channel: string): BuiltInBrowserAttachWebviewArgs => { | ||
| const record = builtInBrowserRecord(value, channel, true); | ||
| const webContentsId = builtInBrowserNumber(record, "webContentsId", channel, { min: 1, max: Number.MAX_SAFE_INTEGER }); | ||
| const tabId = optionalBuiltInBrowserString(record, "tabId", channel, 128); | ||
| if (!tabId) invalidBuiltInBrowserArg(channel, "tabId must be a non-empty string"); | ||
| return { tabId, webContentsId }; |
There was a problem hiding this comment.
Shared IPC types no longer match these parsers.
parseBuiltInBrowserAttachWebviewArgs() and parseBuiltInBrowserTabArgs() now reject missing/blank tabId, but the shared contracts still declare tabId?: string | null. That means preload/renderer code can type-check and still fail at runtime on these handlers.
As per coding guidelines, "Keep IPC contracts, preload types, shared types, and renderer usage in sync whenever an interface changes".
Also applies to: 2268-2272
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/desktop/src/main/services/ipc/registerIpc.ts` around lines 2230 - 2235,
The parser functions parseBuiltInBrowserAttachWebviewArgs and
parseBuiltInBrowserTabArgs currently reject missing/blank tabId but the shared
IPC types declare tabId?: string | null; update these parsers to align with the
shared contract by removing the unconditional invalidBuiltInBrowserArg call and
instead: use optionalBuiltInBrowserString(record, "tabId", channel, 128) to
allow undefined/null, only call invalidBuiltInBrowserArg if tabId is present but
equals the empty string, and ensure the returned type allows tabId to be
undefined|null; reference the existing helpers builtInBrowserRecord,
optionalBuiltInBrowserString, and invalidBuiltInBrowserArg when making the
change.
- builtInBrowserService.createTab: normalizeBrowserUrl runs before any tab state mutation, so an invalid URL throws without leaving an orphan tab - ade-cli: add ade-browser/built-in-browser/builtin-browser aliases to the help-resolution map so all four names route to the browser command - IPC: tighten BuiltInBrowserAttachWebviewArgs.tabId and BuiltInBrowserTabArgs.tabId to required strings to match parser runtime; switch the two parsers to invalidBuiltInBrowserArg(...) so TS narrows - AgentChatComposer: extract hasContextSelection so the cloud submit branch and active-turn steer button accept context-only sends, matching singleReady - ChatIosSimulatorPanel: addSimulatorCaptureContext and addPreviewCaptureContext show the same "Chat attachments are not available in this panel." toast as selectElementAt instead of failing silently - FilesPage: when the lane-driven workspace reconciliation changes workspaceId, clear openTabs/activeTabPath/selectedNodePath so save/read targets the right workspace - useWorkSessions.setWorkSidebarWidthPct: fall back to 36 on non-finite input before clamping, mirroring appStore.normalizeWorkSidebarWidthPct - TerminalView: drop the off-by-one fallback that re-called getLine with the 1-based bufferLineNumber Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
@copilot review but do not make fixes |
Here's the updated review for the new Greptile round. Still no fixes. Greptile P1 — Already Fixed
Greptile P2 (security) — Confirmed Open
Greptile P2 — Confirmed Open
Greptile P2 — Confirmed Open (residual)
The flag is single-use and cannot guard rapid sequential clicks. Previously Identified Issues (still open)
Debugger state singleton across tabs — closure-level
|
- builtInBrowserService.navigate: validate tabId before clearing inspect/
selection state, so navigate() with an invalid id no longer leaves the
service in a degraded state (Greptile P1)
- main.ts will-attach-webview hardening: did-attach-webview listener installs
setWindowOpenHandler('deny') and a stricter will-navigate filter
(isAllowedAdeBrowserWebviewNavigation, http(s) only) so a compromised
renderer can't attach an empty-src webview and then loadURL elsewhere
(Copilot P2)
- useWorkSessions stripUrlFilterParams: compare via
URLSearchParams.toString() so the effect is self-stabilizing — navigate()
only fires when stripping yields a different query string, regardless of
effect-dep churn (Copilot P2)
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
@copilot review but do not make fixes |
Here's the updated review for this Greptile round. Still no fixes. Greptile P1 — Already Fixed
Greptile Issue 2 — Confirmed OpenModule-level regex with Greptile Issue 3 — Confirmed Open
Previously Identified Issues (still open)
URL bar scheme validation ( Debugger state singleton across tabs — |
| return getStatus(); | ||
| } | ||
|
|
||
| async function closeTab(input: BuiltInBrowserTabArgs): Promise<BuiltInBrowserStatus> { | ||
| const tabId = input.tabId?.trim(); | ||
| if (!tabId) throw new Error("Browser tab id is required."); |
There was a problem hiding this comment.
switchTab clears selection unconditionally, even for same-tab switches
clearSelectionInternal() is called on every switchTab invocation, regardless of whether tab.id === activeTabId. If a caller (e.g., the UI re-rendering) calls switchTab with the already-active tab, the user's selected element context is silently discarded. The stopInspectQuietly call already has the correct guard (tab.id !== activeTabId); clearSelectionInternal should use the same guard.
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/desktop/src/main/services/builtInBrowser/builtInBrowserService.ts
Line: 511-516
Comment:
**`switchTab` clears selection unconditionally, even for same-tab switches**
`clearSelectionInternal()` is called on every `switchTab` invocation, regardless of whether `tab.id === activeTabId`. If a caller (e.g., the UI re-rendering) calls `switchTab` with the already-active tab, the user's selected element context is silently discarded. The `stopInspectQuietly` call already has the correct guard (`tab.id !== activeTabId`); `clearSelectionInternal` should use the same guard.
How can I resolve this? If you propose a fix, please make it concise.
| // doesn't leave the service with cleared inspect/selection state. | ||
| let existingTab: BrowserTabState | null = null; | ||
| if (!input.newTab && input.tabId) { | ||
| existingTab = tabs.find((entry) => entry.id === input.tabId) ?? null; | ||
| if (!existingTab) throw new Error(`Browser tab not found: ${input.tabId}`); | ||
| } | ||
| const switchingTabs = input.newTab || (input.tabId && input.tabId !== activeTabId); | ||
| if (switchingTabs) { | ||
| await stopInspectQuietly("built_in_browser.navigate_stop_inspect_failed"); | ||
| clearSelectionInternal(); | ||
| } | ||
| let tab = input.newTab ? createTabState() : null; | ||
| if (tab) { | ||
| tabs = [...tabs, tab]; | ||
| activeTabId = tab.id; | ||
| } else if (existingTab) { | ||
| tab = existingTab; | ||
| activeTabId = tab.id; | ||
| } else { | ||
| tab = ensureActiveTab(); | ||
| } | ||
| const wc = tab.webContents; | ||
| attachViewsToCurrentWindow(); | ||
| await wc.loadURL(targetUrl); | ||
| emitStatus(); | ||
| return getStatus(); | ||
| } | ||
|
|
||
| async function createTab(input: BuiltInBrowserCreateTabArgs = {}): Promise<BuiltInBrowserStatus> { | ||
| if (tabs.length >= MAX_BROWSER_TABS) { | ||
| throw new Error(`ADE browser is limited to ${MAX_BROWSER_TABS} tabs. Close a tab before opening another.`); | ||
| } |
There was a problem hiding this comment.
When navigate() is called without newTab and without a different tabId, switchingTabs evaluates to false, so stopInspectQuietly() is never called. However, Electron's CDP Overlay.setInspectMode command is page-level and is lost when the WebContents navigates. After the load completes, inspecting in service state remains true and getStatus().isInspecting returns true, but the CDP overlay is gone — the user sees the "Inspecting" badge in the UI but clicking elements has no effect. The fix is to always call stopInspectQuietly() when loading a URL into the current tab, not only when switching tabs.
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/desktop/src/main/services/builtInBrowser/builtInBrowserService.ts
Line: 449-480
Comment:
**Inspect mode desynchronizes with CDP overlay after same-tab navigation**
When `navigate()` is called without `newTab` and without a different `tabId`, `switchingTabs` evaluates to `false`, so `stopInspectQuietly()` is never called. However, Electron's CDP `Overlay.setInspectMode` command is page-level and is lost when the WebContents navigates. After the load completes, `inspecting` in service state remains `true` and `getStatus().isInspecting` returns `true`, but the CDP overlay is gone — the user sees the "Inspecting" badge in the UI but clicking elements has no effect. The fix is to always call `stopInspectQuietly()` when loading a URL into the current tab, not only when switching tabs.
How can I resolve this? If you propose a fix, please make it concise.
Summary
Describe the change.
What Changed
Key files and behaviors.
Validation
How you tested.
Risks
Anything to watch.
Summary by CodeRabbit
New Features
Improvements
Removed
Tests
Greptile Summary
This PR introduces a full built-in browser for the ADE desktop app: a new
BuiltInBrowserService(multi-tab, CDP-backed inspection, screenshots), a large React panel (ChatBuiltInBrowserPanel) using<webview>tags, a resizableWorkSidebarreplacing the removed floating panes, and terminal URL auto-detection that opens links in the ADE browser.Three P1 issues remain:
switchTabcallsclearSelectionInternal()unconditionally, silently discarding the user's selected element even when switching to the already-active tab.navigate()leavesinspecting = truein service state while the CDPOverlay.setInspectModeis lost on page reload, desynchronizing the UI badge from actual inspect capability.frame-srcCSP now includesfile:andapp:origins in production (viacspSources), permittingfile://iframe loading in the renderer whereframe-src 'none'was previously enforced.Confidence Score: 3/5
Not safe to merge without addressing the CSP frame-src regression and the inspect-state desync bug.
Three P1 findings: inspect state desynchronization after same-tab navigation, unconditional selection clear in switchTab, and a security-relevant CSP broadening (file: in frame-src). Multiple P1s on a core-path feature pull the score to 3.
apps/desktop/src/main/services/builtInBrowser/builtInBrowserService.ts (inspect desync + switchTab), apps/desktop/src/main/main.ts (frame-src CSP)
Security Review
frame-srcallowsfile:in production (main.ts): The newframe-srcdirective includescspSourceswhich in production mode is'self' file: app:. This permitsfile://URLs inside renderer<iframe>elements, enabling local filesystem access if any XSS or injected iframe is possible. The previous value wasframe-src 'none'.webviewTag: trueenabled (main.ts): Expands attack surface; mitigated bywill-attach-webviewanddid-attach-webviewhandlers that enforcehttp/https-only navigation and strip preload scripts.Important Files Changed
Sequence Diagram
sequenceDiagram participant T as Terminal / Markdown Link participant OE as openExternal.ts participant TP as TerminalsPage participant BP as ChatBuiltInBrowserPanel participant IPC as Electron IPC participant SVC as BuiltInBrowserService T->>OE: openUrlInAdeBrowser(url) OE->>OE: markPendingBuiltInBrowserNavigation() OE->>TP: dispatch ADE_OPEN_BUILT_IN_BROWSER_EVENT TP->>BP: mount (sidebar switches to browser tab) OE->>IPC: browser.navigate({url, newTab:true}) IPC->>SVC: navigate() SVC->>SVC: normalizeBrowserUrl, tab-limit check SVC->>SVC: createTabState(), push to tabs SVC-->>IPC: BuiltInBrowserStatus IPC-->>BP: status update via onEvent BP->>BP: useEffect: consumePendingBuiltInBrowserNavigation() note over BP: flag was true → skip Google tab creation BP->>IPC: attachWebview({tabId, webContentsId}) IPC->>SVC: attachWebview() SVC-->>BP: webview attached BP->>SVC: setBounds({visible:true, ...}) SVC->>SVC: attachViewsToCurrentWindow()Comments Outside Diff (1)
apps/desktop/src/main/main.ts, line 471-483 (link)frame-srcnow allowsfile:andapp:origins in productionIn production,
cspSources = "'self' file: app:", so the newframe-srcdirective becomesframe-src 'self' file: app: http://localhost:* http://127.0.0.1:* about:. This permits anyfile://URL to load inside an<iframe>in the renderer. Since the built-in browser uses<webview>tags (not iframes), consider tighteningframe-srcto'self' about:in production, mirroring howwebviewaccess is separately gated bywill-attach-webview.Prompt To Fix With AI
Prompt To Fix All With AI
Reviews (7): Last reviewed commit: "ship: iteration 5 — final review fixes" | Re-trigger Greptile