Skip to content

fix(desktop): suppress native browser view for every dialog overlay (Fixes #3980) - #3984

Open
waterWang wants to merge 1 commit into
omnigent-ai:mainfrom
waterWang:fix/overlay-suppress-all-dialogs
Open

fix(desktop): suppress native browser view for every dialog overlay (Fixes #3980)#3984
waterWang wants to merge 1 commit into
omnigent-ai:mainfrom
waterWang:fix/overlay-suppress-all-dialogs

Conversation

@waterWang

Copy link
Copy Markdown

Problem

The embedded Browser tab's native WebContentsView paints above ALL renderer DOM — no z-index or portal can put a modal over it. Opening a fork dialog, command palette, agent info, keyboard shortcuts, or any other overlay while the Browser tab is active renders the dialog underneath the browser page. You can hear yourself clicking a dialog you cannot see.

Background

PR #3491 introduced the right mechanism (setOverlaySuppressed in the view registry + useSuppressBrowserView hook) but wired it only to the Share dialog. The fork dialog, command palette, agent info, keyboard shortcuts, and ~20 other overlay surfaces all remained unwired.

Approach

Rather than adding one useSuppressBrowserView(xOpen) per dialog (which is fragile and easy to miss on new dialogs), this PR drives suppression from the shared DialogOverlay component. Every Radix dialog uses this component, so all dialogs get the fix for free.

Non-dialog overlays (lightbox, toasts) that don't use the Radix Dialog component remain unwired — they can be handled case by case in follow-up PRs.

Changes

  • web/src/hooks/useSuppressBrowserView.ts (new) — Ref-counted hook; hides the native view while any overlay is open, restores it only when the last overlay closes. No-op outside Electron.
  • web/src/components/ui/dialog.tsx — Wire useSuppressBrowserView(true) to DialogOverlay. Since it mounts only when the dialog is open, mount → suppress, unmount → restore.
  • web/electron/src/browserViewRegistry.js — Add setOverlaySuppressed method. Hides the active view in place (not detached, so the page keeps running) via view.setVisible(). Flag outlives view swaps so a suppressed state persists across tab switches.
  • web/electron/src/browserIpc.js — Register omnigent:browser-set-overlay-suppressed IPC handler.
  • web/electron/src/preload.js — Expose browserSetOverlaySuppressed on the context bridge.
  • web/electron/test/browserViewRegistry.test.js — 6 new tests covering hide/restore, suppressed-state inheritance, swaps, and introspection.
  • web/src/hooks/useSuppressBrowserView.test.tsx (new) — 5 new tests covering open/close, unmount, no-op, ref-counting, and fallback shells.

Closes #3980

…ixes omnigent-ai#3980)

The embedded Browser tab's native WebContentsView paints above ALL renderer
DOM — no z-index or portal can put a modal over it. The existing fix (PR
omnigent-ai#3491) introduced the right mechanism (setOverlaySuppressed in the view
registry + useSuppressBrowserView hook) but wired it only to the Share dialog.

This commit takes the approach recommended in the issue: drive suppression
from the shared DialogOverlay component so every Radix dialog gets it for
free, rather than adding one useSuppressBrowserView(xOpen) per dialog.

Changes:
- Add useSuppressBrowserView hook (ref-counted, no-op outside Electron)
- Wire it to DialogOverlay, covering all ~20 dialog surfaces at once
- Add setOverlaySuppressed to browserViewRegistry (hide/restore in place)
- Add IPC handler + preload bridge for the new method
- Add full test coverage for hook and registry

Closes omnigent-ai#3980
@github-actions github-actions Bot added the size/L Pull request size: L label Aug 3, 2026
@github-actions
github-actions Bot requested a review from dhruv0811 August 3, 2026 18:01
@dhruv0811

Copy link
Copy Markdown
Contributor

/review

@omnigent-ci

omnigent-ci Bot commented Aug 4, 2026

Copy link
Copy Markdown
Contributor

Polly AI Review

Missing visual demonstration

This is a user-visible UI fix — the whole point is that dialogs (fork, command palette, agent info, etc.) currently render underneath the embedded browser view and now appear on top. The PR description has no screenshot or video (the attachment scan found none). Please attach a short before/after clip or a pair of screenshots (Browser tab active, a dialog opened) so reviewers can confirm the modal now paints over the native view without checking out the branch.

Blocking issues

None. The core mechanism is sound:

  • The main-process setOverlaySuppressed hides the active view in place via setViewVisible (guarded, never throws), keeps the flag persistent, and correctly re-applies !overlaySuppressed on both the create-while-active attach path and the swap/attach path in setActive, so a tab switch or a view created while an overlay is open stays hidden.
  • The renderer hook is ref-counted with symmetric increment/decrement (the increment is gated on the bridge being present, and the cleanup only exists once that guard passes), so overlapping overlays don't unhide early and there's no counter leak. StrictMode's mount→unmount→mount cycle nets out correct.
  • No-op paths outside Electron / on shells predating browserSetOverlaySuppressed are handled and tested.

Security vulnerabilities

None. The IPC handler reuses the existing gateRegistry(event) origin gate and coerces the payload to a boolean (!!args?.suppressed); no new attack surface. No lockfile or dependency-extra changes in this diff.

Non-blocking notes

  • Scope gaps (as documented): only surfaces built on the Radix Dialog component get the fix. dropdown-menu, popover, hover-card, tooltip, lightbox, and toasts do not route through DialogOverlay, so any of those opened over the Browser tab will still paint underneath. The command palette is fine (it wraps Dialog). The PR calls this out — just confirm none of the currently-broken [Bug] Desktop: embedded Browser view paints over every dialog; only the Share dialog is suppressed (fork dialog opens invisibly underneath) #3980 surfaces is a non-Dialog overlay.
  • Suppress-on-every-dialog: DialogOverlay fires suppression for every dialog regardless of whether the Browser tab is active or the dialog overlaps the pane. It's a cheap no-op when no view is active, but it does mean the browser page always blanks behind unrelated modals; acceptable given the native-view constraint.
  • Description/tree mismatch: the description credits PR fix(desktop): share dialog no longer hides behind the active Browser tab #3491 with introducing setOverlaySuppressed + useSuppressBrowserView "wired only to the Share dialog," but both symbols are net-new in this diff and there's no pre-existing Share-dialog wiring in the tree. Worth correcting the narrative so future readers aren't hunting for prior wiring — no code impact.
  • Both new files are missing a trailing newline (cosmetic).

Summary

A well-targeted, correctly-implemented fix: driving suppression from the shared DialogOverlay gets every Radix dialog covered for free, the ref-counting and view-swap/attach re-application are handled properly, and the new tests cover the meaningful paths on both sides of the IPC boundary. No correctness or security blockers. The main ask is a before/after visual demonstration in the PR description, plus a quick confirmation that no #3980 surface relies on a non-Dialog overlay still left unwired.


Automated review by Polly · workflow run

@dhruv0811

Copy link
Copy Markdown
Contributor

Hey @waterWang, thanks a ton for working on this, would be able to share a short demo of the new overlay?? Thank you!

@serena-ruan serena-ruan added the P1-high Priority: major feature broken, no workaround label Aug 6, 2026
@github-actions github-actions Bot added P2-medium Priority: bug with workaround, important feature request and removed P1-high Priority: major feature broken, no workaround labels Aug 6, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

P2-medium Priority: bug with workaround, important feature request size/L Pull request size: L

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug] Desktop: embedded Browser view paints over every dialog; only the Share dialog is suppressed (fork dialog opens invisibly underneath)

3 participants