feat(app): embedded browser tab (WebContentsView) - #1204
Merged
Conversation
Add a desktop-only "Browser" tab to the session right panel: one app-owned WebContentsView on a persistent partition, driven by a navigation toolbar (back / forward / reload-or-stop, an editable two-tone address bar, and an overflow menu for open-in-system-browser / copy-link / clear-data) plus the prototype's empty, loading, loaded, and clear-data-confirm states. The page lives in the main process and survives tab switches; the renderer owns the DOM chrome and reports the content rect each animation frame while visible, so main sizes/shows the native overlay and hides it on tab switch, panel collapse, or when an app modal / the tab's own menu would paint over it. Security: the view loads only http/https (validated in main), runs on its own partition with no preload, sandboxed and context-isolated, denies all permission requests, and hands non-web schemes to the system browser. The feature is gated on the Electron platform via canUseBrowser(platform) and is absent on web. Part of #1186. Automation (a narrow CDP bridge) and HTML-artifact preview are deliberately out of scope for this PR.
Contributor
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (11)
📝 WalkthroughWalkthroughAdds a desktop-only embedded browser: platform contracts, Electron controller and options, IPC + preload bridge, renderer BrowserPanel UI, URL/view helpers and tests, right-panel tab integration, command palette inclusion, and i18n strings. ChangesEmbedded Browser Integration
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint install failed. For unrecoverable errors, disable the tool in CodeRabbit configuration. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
github-actions
Bot
added
app
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Suggested priority: P2 (includes user-path files (packages/app/src/components/command-palette/command-palette-default-items.test.ts, packages/app/src/components/command-palette/command-palette-default-items.ts, packages/app/src/components/command-palette/command-palette-search-items.ts, packages/app/src/context/platform.tsx, packages/app/src/desktop-api.ts, packages/app/src/i18n/en.ts, packages/app/src/i18n/zh.ts, packages/app/src/pages/session/browser/browser-panel.tsx, packages/app/src/pages/session/browser/url.test.ts, packages/app/src/pages/session/browser/url.ts, packages/app/src/pages/session/browser/view-state.test.ts, packages/app/src/pages/session/browser/view-state.ts, packages/app/src/pages/session/right-panel-tab-strip.tsx, packages/app/src/pages/session/right-panel-tabs.ts, packages/app/src/pages/session/session-side-panel.tsx, packages/app/src/pages/session/use-session-commands.tsx, packages/desktop-electron/src/main/browser/controller.ts, packages/desktop-electron/src/main/browser/logic.test.ts, packages/desktop-electron/src/main/browser/logic.ts, packages/desktop-electron/src/main/browser/options.test.ts, packages/desktop-electron/src/main/browser/options.ts, packages/desktop-electron/src/main/index.ts, packages/desktop-electron/src/main/ipc/browser.ts, packages/desktop-electron/src/preload/index.ts, packages/desktop-electron/src/preload/types.ts, packages/desktop-electron/src/renderer/index.tsx)).
P1/P0 are reserved for maintainer confirmation. Please relabel manually if this is a release blocker, security issue, data-loss risk, or updater/runtime failure.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces an embedded browser feature for the desktop application, utilizing Electron's WebContentsView to overlay a secure, sandboxed browser panel. It includes the frontend UI components, URL normalization utilities, and the necessary IPC wiring between the main and renderer processes. Feedback on the implementation highlights two critical issues in the main process controller: a security vulnerability in openExternal that could allow dangerous URL schemes to execute on the host system, and a runtime crash risk in the destroy method caused by calling a non-existent close method on WebContents.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
…ling Codex review (#1204): - [P1] openExternal now hands only an allow-list (mailto:/tel:) to the OS via the new safeExternalUrl helper; file:, javascript:, and custom app protocols from page-provided links/window.open are dropped so a hostile page can't launch local files or registered handlers. - [P2] clear-data wipes the persistent partition session directly instead of needing a live controller, so it works when the tab is opened fresh after a restart; any live views then reload to reflect the signed-out state.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/app/src/pages/session/session-side-panel.tsx`:
- Around line 140-145: The active tab state may remain "browser" even when the
computed shellTabs (from createMemo using view().sidePanel.openTabs() and
canUseBrowser(platform)) filters that value out, leaving the panel blank; after
computing shellTabs ensure the persisted active tab (sidePanelTab()) is
normalized—if sidePanelTab() === "browser" and !canUseBrowser(platform) (or if
sidePanelTab() is not present in shellTabs) call the setter (setSidePanelTab or
equivalent) to pick a valid fallback (e.g., shellTabs[0] or a default like
"search"); update the logic near createMemo(shellTabs) and wherever sidePanelTab
is read (the getter used at line ~368) so the mounted Tabs.Content values always
match the active tab.
In `@packages/desktop-electron/src/main/browser/controller.ts`:
- Around line 74-76: The openExternal method currently uses NON_WEB_SCHEME to
forward many non-about: schemes to shell.openExternal; change this to parse the
url (e.g., with the URL constructor or equivalent) and compare its protocol
against a strict allowlist of exact protocols you intend to support (reference
openExternal and NON_WEB_SCHEME to find the call site), explicitly permit
"about:" handling if needed, and only call shell.openExternal(url).catch(...)
when the parsed protocol is in that allowlist; also ensure malformed URLs are
rejected safely and logged rather than forwarded.
🪄 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 Plus
Run ID: 8c9f7df4-4feb-4160-ab08-6532af3317cb
📒 Files selected for processing (26)
packages/app/src/components/command-palette/command-palette-default-items.test.tspackages/app/src/components/command-palette/command-palette-default-items.tspackages/app/src/components/command-palette/command-palette-search-items.tspackages/app/src/context/platform.tsxpackages/app/src/desktop-api.tspackages/app/src/i18n/en.tspackages/app/src/i18n/zh.tspackages/app/src/pages/session/browser/browser-panel.tsxpackages/app/src/pages/session/browser/url.test.tspackages/app/src/pages/session/browser/url.tspackages/app/src/pages/session/browser/view-state.test.tspackages/app/src/pages/session/browser/view-state.tspackages/app/src/pages/session/right-panel-tab-strip.tsxpackages/app/src/pages/session/right-panel-tabs.tspackages/app/src/pages/session/session-side-panel.tsxpackages/app/src/pages/session/use-session-commands.tsxpackages/desktop-electron/src/main/browser/controller.tspackages/desktop-electron/src/main/browser/logic.test.tspackages/desktop-electron/src/main/browser/logic.tspackages/desktop-electron/src/main/browser/options.test.tspackages/desktop-electron/src/main/browser/options.tspackages/desktop-electron/src/main/index.tspackages/desktop-electron/src/main/ipc/browser.tspackages/desktop-electron/src/preload/index.tspackages/desktop-electron/src/preload/types.tspackages/desktop-electron/src/renderer/index.tsx
CodeRabbit (#1204): a session persisted with the browser tab active can be restored where canUseBrowser is false (the feature flag-disabled or rolled back). Its chip was already filtered out, but the active "browser" selection left the panel on a tab whose <Tabs.Content> never mounts (blank body). Mirror the existing dangling-terminal guard: close the unsupported persisted tab so the selection falls back to a real one.
…vers The native WebContentsView is a window-level layer that DOM CSS can't cover. A settings/automations/skills takeover keeps the session mounted (CSS-hidden via `invisible`, not unmounted, to preserve its state), so the overlay kept painting through the takeover. Feed `mainSurfaceOpen` into the visibility gate as a first-class `coveredBySurface` input so the overlay hides whenever a surface covers the session. Found during manual QA of the browser tab; deeper surface-architecture rework tracked in #1209.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/desktop-electron/src/main/browser/controller.ts`:
- Around line 142-146: reloadIfLoaded() currently only reloads when
state().hasPage is true, missing the case where the first navigation is
in-flight; update reloadIfLoaded() to treat webContents that are loading
(wc.isLoading()) as a live view: if wc.isLoading(), add a one-shot listener
(e.g., 'did-stop-loading' or equivalent) that calls wc.reload() once the
navigation settles (ensure listener is removed and check
this.destroyed/wc.isDestroyed() before reloading), else if state().hasPage then
call wc.reload() immediately; keep existing early return guards (this.destroyed
|| this.wc.isDestroyed()) and reference reloadIfLoaded(), state().hasPage,
wc.isLoading(), wc.reload(), and wc.isDestroyed() 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 Plus
Run ID: b93bbd10-d3ba-4e15-9f6f-dfb007da7b69
📒 Files selected for processing (8)
packages/app/src/pages/session/browser/browser-panel.tsxpackages/app/src/pages/session/browser/view-state.test.tspackages/app/src/pages/session/browser/view-state.tspackages/app/src/pages/session/session-side-panel.tsxpackages/desktop-electron/src/main/browser/controller.tspackages/desktop-electron/src/main/browser/logic.test.tspackages/desktop-electron/src/main/browser/logic.tspackages/desktop-electron/src/main/ipc/browser.ts
🚧 Files skipped from review as they are similar to previous changes (7)
- packages/desktop-electron/src/main/browser/logic.test.ts
- packages/desktop-electron/src/main/browser/logic.ts
- packages/app/src/pages/session/browser/view-state.ts
- packages/app/src/pages/session/session-side-panel.tsx
- packages/app/src/pages/session/browser/view-state.test.ts
- packages/desktop-electron/src/main/ipc/browser.ts
- packages/app/src/pages/session/browser/browser-panel.tsx
…state PR1's embedded browser is human-driven only; agent-driven navigation is PR2. The empty-state copy promised an ability that does not exist yet and misled users into asking the agent to open pages. Updated zh and en.
The titlebar add-tab "+" menu opens downward over the embedded browser's content region. The browser paints a native WebContentsView there, and a native layer ignores DOM stacking, so the menu's lower items were occluded by the loaded page once a tab was active. Lift the menu's open state into SessionSidePanel and thread it to BrowserPanel so the native overlay is suppressed while the menu is up, mirroring how the browser's own overflow menu and app modals already suppress it.
A bare address with no scheme always got https, so an HTTP-only local dev server typed as "localhost:3000" failed to load on first navigation. Pick the scheme from the host: loopback hosts (localhost, *.localhost, 127.0.0.0/8, ::1, 0.0.0.0) default to http since they only ever route to the local machine; everything else — including private LAN IPs, whose network may be hostile — keeps the https default, and the user can type http:// to opt in.
reloadIfLoaded() only reloaded when hasPage was true, but hasPage stays false until the first navigation commits. If browser:clear-data ran while that first load was in flight, the request already sent with pre-clear cookies could land as stale signed-in content and never get the reload this path promises. Treat an in-flight load as live: reload now when a page is loaded, otherwise defer one reload until did-stop-loading when the first navigation is still in flight. The decision is extracted as the pure clearDataReloadAction for tests.
Owner
Author
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
This was referenced Jun 9, 2026
Astro-Han
added a commit
that referenced
this pull request
Jun 10, 2026
First of three flat PRs for embedded-browser agent control (#1186): the self-owned, security-sensitive layer. PawWork's agent needs to drive the embedded browser (PR #1204 shipped only the visible WebContentsView panel) with no Chrome, no extension, no second process; the transport is webContents.debugger <-> ws <-> CDP client. Landing the sealed boundary on its own kept the contract under undiluted review. PR2 (BrowserSession + generic tools) and PR3 (opencli adapters) build on top. Change boundary (packages/desktop-electron only): - cdp-bridge.ts: CdpBridge relays ws CDP JSON-RPC to/from webContents.debugger over ws://127.0.0.1:<random>/<secret>. Security contract, each rule test-covered: loopback + OS-random port; constant-time secret match at the HTTP upgrade (wrong secret never consumes the single connection slot; pathname-only compare so query strings are ignored); Host pinned to the loopback authority + any browser Origin rejected; single connection; secret in main-process memory only (redactedEndpoint is the only loggable form); only the passed WebContents attached, never a global --remote-debugging-port; endpoint/secret returned as a same-process value, never over renderer IPC/preload. - controller-automation.ts: shared main-process registry of per-window controllers; attachForWindow returns the sealed endpoint same-process (consumed by PR2). - controller.ts: attachAutomation/detachAutomation; destroy() tears the bridge down. - ipc/browser.ts: lifts the controllers map out of the registerBrowserIpc() closure into the registry; no new renderer channel. - options.ts: CDP_BRIDGE_SECRET_LENGTH, BRIDGE_START_TIMEOUT_MS. Adds ws + @types/ws (pinned to the already-resolved transitives). - scripts/smoke-cdp-bridge-ws.ts + cdp-bridge-ws-harness.ts: real-Node + real-ws smoke chained into test:ci. Review trail (four independent passes; every finding verified before fixing, each fix red/green tested): - Codex: P1 teardown did not fail in-flight CDP commands (client hung ~30s) -> pending tracking + CDP error frames on stop, secret rotation per start. - Fresh-eye model review: P1 missing ws socket 'error' handler let one malformed client frame kill the whole main process (reproduced under real Node + ws; invisible to bun test, whose "ws" resolves to Bun's shim) -> terminate on error. P1 bridge-lifetime pending Set let a reconnecting client reusing command ids receive the dead connection's stale result -> pending keyed by issuing connection, swept on close. P2 concurrent start() misreported target-busy -> memoized in-flight start. - Human review: P2 teardown error frames now carry sessionId so flattened-session clients can route them; P3 pathname-based authorization. - Gemini: high win.id read inside the 'closed' handler throws after destroy (pre-existing, lifted verbatim) -> id captured up front; medium WebContents destroyed directly is not guaranteed to emit debugger 'detach' -> 'destroyed' listener routes through the same teardown. - Final self-audit: stop() racing a start() in flight waited out the full listen timeout or surfaced a bare TypeError -> stop() aborts a pending listen with a typed error (target-destroyed/target-busy). Verification: 34 bun tests under packages/desktop-electron/src/main/browser (20 on the bridge), typecheck (tsgo -b) clean, eslint clean, and smoke:cdp-bridge-ws under real Node + ws@8.20.0 (red-checked: removing the error handler fails the smoke). Full CI green on 7a7de1d including unit-windows-desktop (first Windows run of the smoke) and desktop-smoke (one earlier flake on be50caa was the known report-problem evaluate race, passed on re-run). Residual risk / deferred: no live dev:desktop attach in this PR by design - the bridge has no caller until PR2's BrowserSession lands; the live attach + CDP round-trip is verified there. The bun suite still runs against Bun's ws shim for frame-level semantics; the dedicated smoke covers the real-ws path. Closes #1186? No - #1186 stays open for PR2/PR3.
13 tasks
13 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Adds a desktop-only Browser tab to the session right panel: one app-owned
WebContentsViewon a persistent partition, reusing Electron's own Chromium.It comes with a navigation toolbar (back / forward / reload-or-stop, an editable
two-tone address bar, and an overflow menu for open in system browser / copy
link / clear browsing data) and the prototype's four states (empty, loading,
loaded, clear-data confirm).
The page lives in the main process and survives tab switches; the renderer
owns the DOM chrome and reports the content rect each animation frame while
visible, so main sizes/shows the native overlay and hides it on tab switch,
panel collapse, or whenever an app modal / the tab's own menu would paint over
it (a native view always paints above the DOM).
The tab is opt-in (opened via
browser.toggle/ the panel+menu, closable —mirroring Review) and gated on the Electron platform via
canUseBrowser(platform),so it never appears on web.
Why
First shippable slice of #1186 (embedded browser). Landing the user-driven
browser surface now keeps the follow-up automation PR focused purely on the
narrow CDP bridge. Design was approved from
docs/design/preview/browser-tab-prototype.html.Related Issue
Part of #1186. Automation (a narrow loopback CDP bridge) and HTML-artifact
preview are deliberately out of scope for this PR.
Human Review Status
Pending
Review Focus
browser-panel.tsx(rAF-while-visible rectreporting +
shouldShowsuppression) andmain/browser/controller.ts(per-window
WebContentsView, show/hide, bounds via the window zoom factor).Does the view stay glued to the content region across resize / panel
drag / open-close, and is it reliably hidden behind menus and modals?
main/browser/options.ts(own partition,no preload, sandbox, context isolation) +
controller.ts(deny all permissionrequests, never spawn child windows) +
logic.ts:parseNavigablemakes onlyhttp/https loadable in-place (file://, javascript: rejected), and
safeExternalUrlhands page-provided links to the OS only for a tightallow-list (
mailto:/tel:) — file://, javascript:, and custom app protocolsare dropped so a hostile page can't launch local files or registered handlers.
canUseBrowser(platform)filters the chip, the+menu, the command, and the tab body off web.
Risk Notes
native layer outside the DOM, so headless/unit tests can't assert its on-screen
position, navigation, clear-data, or dark-mode rendering. Logic, types, and
bundling are fully covered (below); the live overlay behavior should be
eyeballed against the prototype before merge. The conditional UI checklist item
is therefore left unticked.
persist:pawwork-browserpartition (cookies/cache on disk so logins survive restart). Clear browsing
data wipes the partition session directly (
clearStorageData()+clearCache()), so it works even before any view exists (tab opened freshafter restart); live views then reload to show the signed-out state. No
multi-profile.
sites; it is locked down as described in Review Focus and shares no session,
cookies, or IPC bridge with the app renderer.
BrowserWindow.contentView; theoverlay sits below the custom titlebar so it never covers the macOS traffic
lights. No packaging/updater/signing surface touched.
rAF-while-visibleruns a per-framegetBoundingClientRectwhile the browsertab is the visible active tab (stops on switch-away); IPC is diffed so it only
fires on a real device-pixel change. ResizeObserver can't see position-only
moves, hence rAF.
How To Verify
Screenshots or Recordings
Design reference (approved):
docs/design/preview/browser-tab-prototype.html(loaded / empty / loading / clear-data / dark variants). A live capture will be
attached after the manual overlay QA noted above.
Checklist
bug,enhancement,task,documentation. Type labels are author-added; the labeler bot does NOT assign them. Add the label in the GitHub UI, then tick this. (enhancement)app,ui,platform,harness,ci. The labeler bot assigns these on PR open based on changed paths. Confirm the bot's choice (or override if wrong), then tick this. (app,platform, andui— the latter via the labeler's**/*.tsxrule; all correct.)P0,P1,P2,P3. The priority-triage bot suggests one on PR open. Confirm or override, then tick this. (P2— opt-in desktop feature, no urgency.)Pending,Approved by @<reviewer>, orNot required: <reason>(default isPending; "not required" is restricted to bot-authored low-risk PRs).dev, and my PR title and commit messages use Conventional Commits in English.Summary by CodeRabbit
New Features
Tests
Internationalization