Repository navigation
Fix #5303: stop browser pane re-running one-time setup on every CoreAnimation commit - #5311
Conversation
`BrowserPanelView.onAppear` re-fired on every CoreAnimation commit for the portal-hosted browser pane, and `handleBrowserPanelAppear()` did process-once work on every call: `UserDefaults.register(defaults:)`, five settings normalization blocks that write `@AppStorage`, and a respawned empty-state import detection task. A live `sample` showed this burning ~39% of main-thread CPU and re-issuing webview navigation inside commit handlers. Split the appear path: a run-once guard (`didCompleteInitialBrowserPanelSetup`) gates default registration, settings normalization, and the initial empty-state populate into `performInitialBrowserPanelSetupIfNeeded()`, which now runs at most once per view instance. The per-appear path keeps only cheap, idempotent calls; genuine state transitions are already covered by the dedicated `.onChange` observers on `body`. This removes the heavy per-commit work and the `@AppStorage`-write-during-commit invalidation edge that fed the loop. Moving normalization out of the repeated path also stops re-asserting webview visibility work every commit, so a live webview is no longer restored and re-navigated repeatedly (the WebContent churn behind #5302). Adds a regression guard asserting redundant visible notifications on a live webview do not churn lifecycle or replace the webview (so no re-navigation). The view-level run-once gating itself is not cleanly unit-testable without SwiftUI hosting; it is verified via the issue's sample. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughMoves per-appear UserDefaults registration into a process-once BrowserPanel bootstrap, guards BrowserPanelView’s initial setup with a view-local ChangesIdempotent browser pane initialization
🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly Related PRs
🚥 Pre-merge checks | ✅ 17 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (17 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
Greptile SummaryFixes #5303 by moving process-once browser defaults work out of
Confidence Score: 5/5Safe to merge; the static guard is correctly serialized on @mainactor, the view's appear path is genuinely idempotent, and both new test suites cover the fixed invariants. The root cause is correctly addressed. The static hasBootstrappedBrowserDefaults flag on a @mainactor class inherits MainActor isolation, so concurrent reads/writes are impossible. The @State guard bounds the first-appear write to a single extra pass, which is harmless. All remaining appear-path calls are independently idempotent. No new timing dependencies, blocking primitives, or actor-isolation mistakes were introduced. No files require special attention. Important Files Changed
Sequence DiagramsequenceDiagram
participant App
participant BrowserPanel
participant UserDefaults
participant BrowserPanelView
participant SwiftUI
App->>BrowserPanel: init(workspaceId:…)
BrowserPanel->>BrowserPanel: bootstrapBrowserDefaultsIfNeeded()
Note over BrowserPanel: hasBootstrappedBrowserDefaults == false
BrowserPanel->>UserDefaults: normalizeBrowserDefaults(defaults: .standard)
UserDefaults-->>BrowserPanel: register fallbacks + canonicalize legacy values
BrowserPanel->>BrowserPanel: "hasBootstrappedBrowserDefaults = true"
SwiftUI->>BrowserPanelView: .onAppear (1st — genuine)
BrowserPanelView->>BrowserPanelView: performInitialBrowserPanelSetupIfNeeded()
Note over BrowserPanelView: didCompleteInitialBrowserPanelSetup = false
BrowserPanelView->>BrowserPanelView: refreshEmptyStateImportBrowsers()
BrowserPanelView->>BrowserPanel: noteWebViewVisibility(true, …)
SwiftUI->>BrowserPanelView: .onAppear (Nth — spurious CA commit)
BrowserPanelView->>BrowserPanelView: performInitialBrowserPanelSetupIfNeeded()
Note over BrowserPanelView: didCompleteInitialBrowserPanelSetup == true — early return
BrowserPanelView->>BrowserPanel: noteWebViewVisibility(true, …)
Note over BrowserPanel: visibility unchanged — early return
Reviews (4): Last reviewed commit: "Address review: make browser defaults bo..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| let deadline = Date().addingTimeInterval(1.0) | ||
| while panel.webView.isLoading, | ||
| RunLoop.main.run(mode: .default, before: deadline), | ||
| Date() < deadline {} |
There was a problem hiding this comment.
If about:blank doesn't finish loading within 1 second the spin exits silently, and the subsequent noteWebViewVisibility(true, …) / XCTAssertEqual(…, .liveVisible) call may either pass by coincidence or produce a misleading failure without explaining that the load deadline was exceeded. The neighboring test testRestoredHistoryBackDoesNotEmitNewTabLifecycleState adds XCTAssertFalse(panel.webView.isLoading, "Timed out…") immediately after the same spin; this test should do the same.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
There was a problem hiding this comment.
Fixed — added XCTAssertFalse(panel.webView.isLoading, "Timed out waiting for about:blank to finish loading") after the spin, matching the neighboring tests.
— Claude Code
| @@ -766,7 +809,7 @@ struct BrowserPanelView: View { | |||
| BrowserProfilePopoverDebugSettings.verticalPaddingKey: BrowserProfilePopoverDebugSettings.defaultVerticalPadding, | |||
| BrowserThemeSettings.modeKey: BrowserThemeSettings.defaultMode.rawValue, | |||
| ]) | |||
| refreshBrowserChromeStyle() | |||
|
|
|||
| let resolvedThemeMode = BrowserThemeSettings.mode(defaults: .standard) | |||
| if browserThemeModeRaw != resolvedThemeMode.rawValue { | |||
| browserThemeModeRaw = resolvedThemeMode.rawValue | |||
| @@ -787,23 +830,10 @@ struct BrowserPanelView: View { | |||
| if browserProfilePopoverVerticalPaddingRaw != resolvedProfilePopoverVerticalPadding { | |||
| browserProfilePopoverVerticalPaddingRaw = resolvedProfilePopoverVerticalPadding | |||
| } | |||
| panel.noteWebViewVisibility( | |||
| isVisibleInUI && isCurrentPaneOwner, | |||
| reason: "view.onAppear" | |||
| ) | |||
| panel.refreshAppearanceDrivenColors() | |||
| panel.setBrowserThemeMode(browserThemeMode) | |||
| applyPendingAddressBarFocusRequestIfNeeded() | |||
| syncURLFromPanel() | |||
| // If the browser surface is focused but has no URL loaded yet, auto-focus the omnibar. | |||
| autoFocusOmnibarIfBlank() | |||
| syncWebViewResponderPolicyWithViewState(reason: "onAppear") | |||
|
|
|||
| // Populate the empty-state import list once; `handleCurrentURLChange` | |||
| // refreshes it on subsequent new-tab navigations. | |||
| refreshEmptyStateImportBrowsers() | |||
| panel.historyStore.loadIfNeeded() | |||
| #if DEBUG | |||
| logBrowserFocusState(event: "view.onAppear") | |||
| #endif | |||
| focusModeShortcutHintMonitor.start() | |||
| } | |||
There was a problem hiding this comment.
@State flag still writes state during the first CoreAnimation commit pass
performInitialBrowserPanelSetupIfNeeded sets didCompleteInitialBrowserPanelSetup = true before the @AppStorage writes, which correctly gates subsequent calls. However, that @State mutation itself happens inside .onAppear—the same commit pass the PR is trying to clean up—so the first appear still schedules a re-render that fires .onAppear once more, at which point the guard early-returns. The loop is now bounded to two passes rather than infinite, which is the real fix, but the @State flag is a new mutable piece of state that creates a second owner for "has one-time setup run?" alongside whatever owns the BrowserPanel model lifecycle.
Per the cmux-swift-architectural-rethink rule, UserDefaults.register(defaults:) and the five @AppStorage normalization writes are process-once / app-once work that belongs in a startup or model initialization site (e.g., the BrowserPanel init or a dedicated settings-boot function), not in a SwiftUI .onAppear handler gated by view-scoped @State. Moving them out would make the fix robust across view identity changes and remove the remaining first-appear re-render entirely.
Rule Used: Flag Swift fixes that patch symptoms while leaving... (source)
There was a problem hiding this comment.
Good call — moved UserDefaults.register(defaults:) and the five settings-normalization writes out of .onAppear into BrowserPanel.normalizeBrowserDefaults(defaults:), run once per process via bootstrapBrowserDefaultsIfNeeded() from BrowserPanel.init. The process-scoped guard survives view-identity changes (a @State flag would reset on remount), so the settings work truly runs once. The injected UserDefaults also makes it unit-testable; added BrowserDefaultsNormalizationTests. The view's first-appear path now only seeds view-local state (the empty-state import list).
— Claude Code
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 2 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
… init Greptile (P2) flagged that `UserDefaults.register(defaults:)` and the five `@AppStorage` settings-normalization writes are app-once work that should not live in `BrowserPanelView.onAppear` gated by view-scoped `@State`: a `@State` guard resets whenever the view changes identity (a remount re-runs it), so it does not robustly bound the work under the very remount loop this PR addresses. Move registration + normalization into `BrowserPanel.normalizeBrowserDefaults(defaults:)`, invoked once per process via `bootstrapBrowserDefaultsIfNeeded()` from `BrowserPanel.init` (before any setting is read). The function takes an injected `UserDefaults`, so it is unit-testable against a scratch suite without touching `UserDefaults.standard`. The view's first-appear path now only seeds view-local state (the empty-state import list). Tests: - Add `BrowserDefaultsNormalizationTests`: out-of-range/legacy raw values are rewritten to canonical form and registered fallbacks are available; valid in-range values are preserved (red without normalization). - Add the timeout-guard assertion after the RunLoop spin in the lifecycle regression test, matching the sibling tests (greptile + cubic nit). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
GhosttyConfigTests.swift imports both the app target (which declares BrowserThemeMode) and CmuxSettings (which declares a public same-named enum), so a bare `BrowserThemeMode.dark` was ambiguous and failed the test-target build. Resolve the app-target enum via the app-only `BrowserThemeSettings` type instead. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Greptile's design note: bootstrapBrowserDefaultsIfNeeded(defaults:) took an injectable UserDefaults while its run-once guard is process-wide, so any call after the first would silently no-op for a different suite. The bootstrap now always targets .standard; tests keep exercising normalizeBrowserDefaults(defaults:) directly with a scratch suite. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This branch was successfully deployed
2 participants
Fixes #5303.
Problem
A live 3-second
sampleof production cmux showed the main thread spending ~39% of on-CPU time re-evaluatingBrowserPanelView.bodyand re-runninghandleBrowserPanelAppear()inside CoreAnimation commit handlers. For the portal-hosted browser pane,.onAppearre-fires on essentially everyCA::Transaction::commit, andhandleBrowserPanelAppear()was doing process-once work on every call:UserDefaults.standard.register(defaults:)— re-registering the whole defaults dictionary every commit.@AppStorage(theme mode, import-hint variant, toolbar spacing, two profile-popover paddings) — writing state during the commit pass, which feeds SwiftUI invalidation.refreshEmptyStateImportBrowsers()— cancelling and respawning a detached browser-detectionTaskevery commit.In addition, the repeated visibility re-assertion drove
noteWebViewVisibility → restoreDiscardedWebViewIfNeeded → performNavigation → browserLoadRequest, re-navigating the WKWebView and churning the WebContent helper process (a plausible engine for the leak in #5302).Root cause
.onAppearis being treated as a "run once / on transition" signal, but it is not reliable for a portal-hosted pane — it re-fires every commit. Doing one-time setup and@AppStoragewrites in that path makes every spurious appear expensive and re-entrant.Fix
Move the one-time work to where it belongs and gate the rest:
UserDefaults.register(defaults:)and the five settings-normalization writes now live inBrowserPanel.normalizeBrowserDefaults(defaults:), run once per process viaBrowserPanel.bootstrapBrowserDefaultsIfNeeded()fromBrowserPanel.init. The process-scoped guard survives view remounts (a view-scoped flag would reset on identity change), and the injectedUserDefaultsmakes the normalization unit-testable.@State.performInitialBrowserPanelSetupIfNeeded()runs at most once perBrowserPanelViewinstance and only seeds view-local state (the empty-state import list).handleBrowserPanelAppear()keeps only cheap, already-idempotent calls. Genuine state transitions (visibility, focus, URL, omnibar, profile, color scheme) are independently handled by the dedicated.onChangeobservers onbody, so re-running the slim per-appear path is harmless.This removes the heavy per-commit work and the
@AppStorage-write-during-commit invalidation edge, eliminating the whole class of "appear re-fires every commit → re-does one-time setup / re-writes state / re-navigates the webview." A live webview is no longer restored and re-navigated by redundant appears (restoreDiscardedWebViewIfNeededalready guards onisDiscardedForMemory, andnoteWebViewVisibilityearly-returns when visibility is unchanged).Verification
samplecall tree.sample <cmux-pid> 3: theBrowserPanelView.body/handleBrowserPanelAppear/browserLoadRequestframes underCA::Transaction::commitshould be gone and main-thread CPU should drop.Tests
Adds
testRedundantVisibleNotificationsDoNotChurnLiveWebViewtoBrowserPanelWebViewLifecycleTests: once a webview is live and visible, 32 redundantnoteWebViewVisibility(true, …)calls (the shape a spurious appear produces) must not churn lifecycle, replace the webview, or record a new transition — so no re-navigation is issued.Adds
BrowserDefaultsNormalizationTests: against a scratchUserDefaults(suiteName:), out-of-range/legacy stored values are rewritten to canonical form, registered fallbacks are available for unset keys, and already-valid values are left untouched.Related performance evidence: #5305 (tracking audit; this PR removes the render-loop CPU engine documented there).
The view-level run-once gating itself is not cleanly unit-testable without SwiftUI hosting in the unit target, so there is no red-first commit for that piece; it is verified via the issue's sample and the behavioral guard above.
🤖 Generated with Claude Code
Need help on this PR? Tag
/codesmithwith what you need. Autofix is disabled.Note
Medium Risk
Touches hot-path browser panel lifecycle and UserDefaults on every panel init, but behavior is narrowed to idempotent normalization and existing visibility early-returns; regression coverage was added.
Overview
Fixes #5303 by stopping portal-hosted
BrowserPanelViewfrom treating every repeated.onAppear(CoreAnimation commits) as first-time setup.Process-once work moves out of
handleBrowserPanelAppear():UserDefaultsfallback registration and canonicalization of legacy/out-of-range browser settings now run viaBrowserPanel.bootstrapBrowserDefaultsIfNeeded()/normalizeBrowserDefaults(defaults:)fromBrowserPanelinit, guarded once per process. The view no longer re-registers defaults or re-syncs theme/import-hint/toolbar/profile debug keys on each appear (removing@AppStorage-write-driven invalidation during commits).View-once work is limited to
performInitialBrowserPanelSetupIfNeeded()withdidCompleteInitialBrowserPanelSetup, which only seeds the empty-state import browser list; per-appear handling keeps idempotent visibility/chrome/focus/history calls.Tests add
BrowserDefaultsNormalizationTests(scratchUserDefaults) andtestRedundantVisibleNotificationsDoNotChurnLiveWebViewto ensure redundantnoteWebViewVisibility(true, …)does not replace the liveWKWebViewor churn lifecycle.Reviewed by Cursor Bugbot for commit 5dfefa4. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by cubic
Stops the browser panel from re-running one-time setup on every CoreAnimation commit, fixing #5303. Defaults registration and settings normalization now run once per process in the model; the view’s appear path stays light, avoiding render-loop churn and redundant
WKWebViewnavigation.bootstrapBrowserDefaultsIfNeeded()), which now always targetsUserDefaults.standard; normalization remains innormalizeBrowserDefaults(defaults:)for unit tests..onAppearnow does only cheap, idempotent work.BrowserDefaultsNormalizationTestsandtestRedundantVisibleNotificationsDoNotChurnLiveWebView; fixed test build by disambiguating theBrowserThemeModeenum in the normalization test.Written for commit 5dfefa4. Summary will update on new commits.
Summary by CodeRabbit
Bug Fixes
Tests