Repository navigation
Keep native Browser pages when switching servers - #5490
Merged
Merged
Conversation
…iews-server-switch
brsbl
marked this pull request as ready for review
October 14, 2026 20:59
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 69cf772c86
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| let stopped = false; | ||
| let generation = 0; | ||
| let registryServerOrigin: string | null = null; | ||
| let serverUrl = args.getServerUrl(); |
There was a problem hiding this comment.
When applyServerTarget() starts on the builtin target and then activateLocalServerMoveIfLocked() changes the target to the committed move destination, reconnect() has already captured the builtin URL. Descriptor lookup for that origin normally fails after the move, but every scheduled attempt now retries the same cached URL instead of consulting getServerUrl() as before, so native Browser automation remains disconnected from the moved server until another explicit reconnect occurs.
Useful? React with 👍 / 👎.
Comment on lines
+1108
to
+1109
| for (const popup of entry.popupWindows) { | ||
| if (!popup.isDestroyed()) popup.hide(); |
There was a problem hiding this comment.
If a native page has a popup playing audio when the user switches servers, popup.hide() only removes the window from view; it does not mute or pause the popup's separate WebContents. The parent entry is muted through applyEntryVisibility(), but its popups are not, so content from the inactive server can remain audible until the popup closes or the user switches back.
Useful? React with 👍 / 👎.
Comment on lines
+1105
to
+1107
| for (const entry of entries.values()) { | ||
| entry.visible = false; | ||
| applyEntryVisibility(entry, entry.hostWindow); |
There was a problem hiding this comment.
Automation-created views are initialized with backgroundThrottling: false, and parking a server only marks them invisible and mutes media. Consequently, every retained automation page from previously visited servers can continue running timers and scripts at full foreground frequency, causing CPU and battery usage to accumulate across server switches; restore throttling while these entries are parked and reinstate their automation setting when activated.
Useful? React with 👍 / 👎.
brsbl
added a commit
that referenced
this pull request
Oct 15, 2026
…ar drawer (#5495) ## Human comments ## What was wrong In macOS desktop windows narrower than 768px, New thread in the sidebar drawer sits under the traffic lights, and the sidebar toggle draws on top of it (reported on Discord after desktop-v0.46.0). #5229 (`ff9484883`) moved the navigation rail into the compact drawer and dropped the macOS start inset from `SidebarTopReserveRow`. The drawer's header row now starts right after the rail, at x=52. That is under the lights (x=18–78), and the sidebar toggle, fixed at x=84–112, overlaps it. ## What changed - `SidebarTopReserveRow` (`apps/app/src/components/sidebar/SidebarChrome.tsx`) pads the drawer's header row so New thread starts at x=116, just after the sidebar toggle. That is where it sat in 0.45.0. - The padding applies only on macOS desktop, in the compact drawer (not the framed sidebar), and outside full screen, where there are no traffic lights. - Wide framed windows, browsers and other platforms are unchanged. No wire, CLI or contract changes. ## How you verified - New `SidebarChrome.test.tsx`: the compact macOS drawer reserves the space, while the framed window and a compact browser window don't. CI failed with only the test ([run 38601538235](https://github.com/get-bb/bb/actions/runs/38601538235), app-8: "expected false to be true") and passes with the fix. - Real Mac (macOS 26.5.2), using the Electron desktop dev app built from `main` (before) and from this branch (after), with the same seeded thread. The window was resized natively to 700×800 with the sidebar drawer open, and to 1100×800. - **700px:** New thread moves from x=52–287, under the lights and the toggle, to x=116–287. The hit test at its center returns New thread. - **1100px:** the framed layout is identical before and after. - The colored dots are stand-ins drawn at the traffic lights' native position (18, 18). The native buttons can't be captured without Screen Recording permission. - Not covered: a narrow window in native full screen, where the padding is off by design because there are no lights. - Remote CI runs the tests; nothing was run locally. - CI: every job passes except the plugin SDK npm version guard in Checks. Plugin Fork Check was skipped by change-based selection. The guard fails because `@get-bb/plugin-sdk@0.6.43` was published to npm at 15:11 UTC today, and building the SDK from `main` now produces a different `dist/provider-bridge-testing.js`. This PR doesn't touch the SDK. Every PR based on current `main` without a version bump fails the same way (for example #5490 and #5443), and #5492 passes because it bumps to 0.6.44. It clears once a bump lands on `main` and this PR's CI reruns. | Screen / state | Viewport | Before | After | | --- | --- | --- | --- | | Thread · sidebar drawer open (zoom) | macOS desktop · 700×800 |  |  | | Thread · sidebar drawer open | macOS desktop · 700×800 |  |  | | Thread · framed sidebar (unchanged) | macOS desktop · 1100×800 |  |  | ### Merge order with "Compact the macOS app frame and move headers into the title bar" (#5529) Resolved: the PRs can merge in either order. #5495 used to pad the narrow-window drawer header with a literal `88px`, which assumed the sidebar toggle sits at the 84px reserve; with #5529's wider reserve New thread would have started under the toggle (measured at an earlier 88px reserve: New thread at x=112 under the toggle at x=88–116). #5495 now includes `5bd8c8b85`, which derives that padding from `--bb-macos-traffic-light-reserve` (falling back to 84px), so New thread starts 4px after the toggle under either geometry. Measured on a real Mac at 700px: | Build | Sidebar toggle | New thread | | --- | --- | --- | | #5495 head `5bd8c8b85` on main | x=84–112 | x=116 | | #5529 head `96084fc15` + #5495 | x=87–115 | x=119 | BB-Thread-ID: thr_eigbyjruhs 🤖 Generated with [Claude Code](https://claude.com/claude-code) > AGENT GENERATED --------- Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
v-lebedev
pushed a commit
to vrata-tg/vrata-bb
that referenced
this pull request
Oct 23, 2026
## Human comments ## What was wrong Switching BB servers destroyed every native Browser view but retained the window identity. Returning to the original server left saved tabs trying to attach missing views, producing a blank page and losing unsaved state. ## What changed - Browser pages survive server switches with their navigation history and unsaved state. - Each server has an isolated view registry; inactive views are hidden and automation leases are revoked. - Window closure and app exit release retained views across every server. ## How you verified - The local/enrolled daemon regression [failed before the fix](https://github.com/get-bb/bb/actions/runs/38599373329/job/115974641500). - [Remote CI passed](https://github.com/get-bb/bb/actions/runs/38635702642) at `69cf772c86f3a4d2e52b7a33f5477da2380f23b8`, including desktop tests, typechecking, SDK publishing, and Windows desktop smoke. - Electron 44.3.0: switching servers and reloading BB preserved the same native page, history, and unsaved input. **User QA passed.** brsbl approved the native server-switch behavior at `633a598b0d780099270405c9d6ff7d50f2a9386f` and reconfirmed all three PRs on October 14, 2026. Main integration preserved the feature patch; automated native verification passed at the final head above. ### Before and after Before: `37749a87bcdd41e8853c6fb487700e64ebbdfd3d` · After: `69cf772c86f3a4d2e52b7a33f5477da2380f23b8`. Identical fixture, route, 1280 × 900 window, and 474 × 758 Browser viewport. Fresh Electron content captures at 2×. The blank before image is the Browser content region after returning to server A; after is the surviving native page. | Native Browser after switching back | Before | After | | --- | --- | --- | | Page and unsaved note |  |  | Context before the fix:  <details> <summary>Final review, follow-ups, and scope</summary> Fresh per-PR Slop Cop reviewed `37749a87bcdd41e8853c6fb487700e64ebbdfd3d..69cf772c86f3a4d2e52b7a33f5477da2380f23b8` on October 14, 2026. Security found no actionable issues. Architecture and code quality confirmed popup audio; performance confirmed inactive-page CPU use. No P0/P1 findings. E2E reused the completed exact-head Electron pass with two isolated servers and real input. The completed GitHub review added one independently validated P2 and duplicated the other two findings. **P2 — Broker retries can retain the old target after a local-server move.** `reconnect()` captures the builtin URL before `activateLocalServerMoveIfLocked()` can select the committed destination. Scheduled retries reuse that URL, leaving native automation disconnected until another explicit reconnect. Follow-up: reconnect using the final resolved target while preserving generation guards. Done when a committed move discovered during target application connects to the destination without another user switch. [Review comment](get-bb/bb#5490 (comment)). **P2 — Hidden popup audio can continue.** Parking hides popup windows but does not mute their separate WebContents. Follow-up: mute/pause inactive popup contents without destroying them. Done when a playing popup becomes silent on A → B and restores correctly on return. [Review comment](get-bb/bb#5490 (comment)). **P2 — Inactive automation pages remain unthrottled.** Automation pages retain foreground timer/frame behavior across server switches. Follow-up: enable background throttling while parked and restore the active setting, including CDP teardown. Done when inactive animated pages reduce work while page state and active automation survive return. [Review comment](get-bb/bb#5490 (comment)). Two Slim PR passes retained the existing view manager, visibility/reload helpers, origin guards, and cleanup coverage. Before → after: production +88/−42 → +88/−42; tests +100/−2 → +100/−2; docs +7/−0 → +7/−0; generated 0 → 0. Local/enrolled cases protect separate descriptor paths; window-close/app-exit cases protect separate cleanup paths. No code changed during these passes; P2s remain follow-ups. </details> BB-Thread-ID: thr_8f2nidvuyn > AGENT GENERATED
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
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.
Human comments
What was wrong
Switching BB servers destroyed every native Browser view but retained the window identity. Returning to the original server left saved tabs trying to attach missing views, producing a blank page and losing unsaved state.
What changed
How you verified
69cf772c86f3a4d2e52b7a33f5477da2380f23b8, including desktop tests, typechecking, SDK publishing, and Windows desktop smoke.User QA passed. brsbl approved the native server-switch behavior at
633a598b0d780099270405c9d6ff7d50f2a9386fand reconfirmed all three PRs on October 14, 2026. Main integration preserved the feature patch; automated native verification passed at the final head above.Before and after
Before:
37749a87bcdd41e8853c6fb487700e64ebbdfd3d· After:69cf772c86f3a4d2e52b7a33f5477da2380f23b8.Identical fixture, route, 1280 × 900 window, and 474 × 758 Browser viewport. Fresh Electron content captures at 2×. The blank before image is the Browser content region after returning to server A; after is the surviving native page.
Context before the fix:
Final review, follow-ups, and scope
Fresh per-PR Slop Cop reviewed
37749a87bcdd41e8853c6fb487700e64ebbdfd3d..69cf772c86f3a4d2e52b7a33f5477da2380f23b8on October 14, 2026. Security found no actionable issues. Architecture and code quality confirmed popup audio; performance confirmed inactive-page CPU use. No P0/P1 findings. E2E reused the completed exact-head Electron pass with two isolated servers and real input. The completed GitHub review added one independently validated P2 and duplicated the other two findings.P2 — Broker retries can retain the old target after a local-server move.
reconnect()captures the builtin URL beforeactivateLocalServerMoveIfLocked()can select the committed destination. Scheduled retries reuse that URL, leaving native automation disconnected until another explicit reconnect. Follow-up: reconnect using the final resolved target while preserving generation guards. Done when a committed move discovered during target application connects to the destination without another user switch. Review comment.P2 — Hidden popup audio can continue. Parking hides popup windows but does not mute their separate WebContents. Follow-up: mute/pause inactive popup contents without destroying them. Done when a playing popup becomes silent on A → B and restores correctly on return. Review comment.
P2 — Inactive automation pages remain unthrottled. Automation pages retain foreground timer/frame behavior across server switches. Follow-up: enable background throttling while parked and restore the active setting, including CDP teardown. Done when inactive animated pages reduce work while page state and active automation survive return. Review comment.
Two Slim PR passes retained the existing view manager, visibility/reload helpers, origin guards, and cleanup coverage. Before → after: production +88/−42 → +88/−42; tests +100/−2 → +100/−2; docs +7/−0 → +7/−0; generated 0 → 0. Local/enrolled cases protect separate descriptor paths; window-close/app-exit cases protect separate cleanup paths. No code changed during these passes; P2s remain follow-ups.
BB-Thread-ID: thr_8f2nidvuyn