Skip to content

Fix #5303: stop browser pane re-running one-time setup on every CoreAnimation commit #5311

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
austinywang merged 5 commits into from
Jun 4, 2026
Merged

Fix #5303: stop browser pane re-running one-time setup on every CoreAnimation commit #5311

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
0d50e8e
Fix #5303: stop browser pane re-running one-time setup on every commit
austinywang Jun 3, 2026
5f6df91
Merge remote-tracking branch 'origin/main' into issue-5303-browser-re…
austinywang Jun 3, 2026
aaffaf4
Address review: move browser defaults bootstrap to process-once model…
austinywang Jun 3, 2026
ecbc20b
fix(tests): disambiguate BrowserThemeMode in normalization test
austinywang Jun 3, 2026
5dfefa4
Address review: make browser defaults bootstrap non-injectable
azooz2003-bit Jun 3, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
79 changes: 79 additions & 0 deletions Sources/Panels/BrowserPanel.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4231,6 +4231,82 @@ final class BrowserPanel: Panel, ObservableObject {
return instanceID == webViewInstanceID
}

/// Tracks whether the process-once browser defaults bootstrap has run.
private static var hasBootstrappedBrowserDefaults = false

/// Registers browser fallback defaults and normalizes any legacy/out-of-range
/// stored settings to their canonical form, exactly once per process.
///
/// This is app-once work, not per-view work. Keeping it out of
/// `BrowserPanelView.onAppear` is what fixes the issue #5303 render loop:
/// `.onAppear` can re-fire on every CoreAnimation commit for a portal-hosted
/// pane, and a view-scoped `@State` guard resets whenever the view changes
/// identity (a remount re-runs it). A process-scoped guard runs the work once
/// regardless of how many panels or view instances come and go.
///
/// Always targets `UserDefaults.standard`: the guard is process-wide, so an
/// injectable suite here would silently no-op for every caller after the first.
/// Tests exercise ``normalizeBrowserDefaults(defaults:)`` directly with a
/// scratch suite instead.
static func bootstrapBrowserDefaultsIfNeeded() {
guard !hasBootstrappedBrowserDefaults else { return }
hasBootstrappedBrowserDefaults = true
normalizeBrowserDefaults(defaults: .standard)
}

/// Registers fallback defaults and writes back canonical values for any stored
/// browser setting whose raw value is legacy or out of range.
///
/// Pure with respect to the injected `defaults`, so it is unit-testable against
/// a scratch `UserDefaults(suiteName:)` without touching `UserDefaults.standard`.
static func normalizeBrowserDefaults(defaults: UserDefaults) {
defaults.register(defaults: [
BrowserSearchSettings.searchEngineKey: BrowserSearchSettings.defaultSearchEngine.rawValue,
BrowserSearchSettings.customSearchEngineNameKey: BrowserSearchSettings.defaultCustomSearchEngineName,
BrowserSearchSettings.customSearchEngineURLTemplateKey: BrowserSearchSettings.defaultCustomSearchEngineURLTemplate,
BrowserSearchSettings.searchSuggestionsEnabledKey: BrowserSearchSettings.defaultSearchSuggestionsEnabled,
BrowserToolbarAccessorySpacingDebugSettings.key: BrowserToolbarAccessorySpacingDebugSettings.defaultSpacing,
BrowserProfilePopoverDebugSettings.horizontalPaddingKey: BrowserProfilePopoverDebugSettings.defaultHorizontalPadding,
BrowserProfilePopoverDebugSettings.verticalPaddingKey: BrowserProfilePopoverDebugSettings.defaultVerticalPadding,
BrowserThemeSettings.modeKey: BrowserThemeSettings.defaultMode.rawValue,
])

let resolvedThemeMode = BrowserThemeSettings.mode(defaults: defaults)
let currentThemeRaw = defaults.string(forKey: BrowserThemeSettings.modeKey)
?? BrowserThemeSettings.defaultMode.rawValue
if currentThemeRaw != resolvedThemeMode.rawValue {
defaults.set(resolvedThemeMode.rawValue, forKey: BrowserThemeSettings.modeKey)
}

let resolvedHintVariant = BrowserImportHintSettings.variant(defaults: defaults)
let currentHintRaw = defaults.string(forKey: BrowserImportHintSettings.variantKey)
?? BrowserImportHintSettings.defaultVariant.rawValue
if currentHintRaw != resolvedHintVariant.rawValue {
defaults.set(resolvedHintVariant.rawValue, forKey: BrowserImportHintSettings.variantKey)
}

let resolvedToolbarSpacing = BrowserToolbarAccessorySpacingDebugSettings.current(defaults: defaults)
let currentToolbarSpacing = (defaults.object(forKey: BrowserToolbarAccessorySpacingDebugSettings.key) as? Int)
?? BrowserToolbarAccessorySpacingDebugSettings.defaultSpacing
if currentToolbarSpacing != resolvedToolbarSpacing {
defaults.set(resolvedToolbarSpacing, forKey: BrowserToolbarAccessorySpacingDebugSettings.key)
}

let resolvedHorizontalPadding = BrowserProfilePopoverDebugSettings.currentHorizontalPadding(defaults: defaults)
let currentHorizontalPadding = (defaults.object(forKey: BrowserProfilePopoverDebugSettings.horizontalPaddingKey) as? NSNumber)?.doubleValue
?? BrowserProfilePopoverDebugSettings.defaultHorizontalPadding
if currentHorizontalPadding != resolvedHorizontalPadding {
defaults.set(resolvedHorizontalPadding, forKey: BrowserProfilePopoverDebugSettings.horizontalPaddingKey)
}

let resolvedVerticalPadding = BrowserProfilePopoverDebugSettings.currentVerticalPadding(defaults: defaults)
let currentVerticalPadding = (defaults.object(forKey: BrowserProfilePopoverDebugSettings.verticalPaddingKey) as? NSNumber)?.doubleValue
?? BrowserProfilePopoverDebugSettings.defaultVerticalPadding
if currentVerticalPadding != resolvedVerticalPadding {
defaults.set(resolvedVerticalPadding, forKey: BrowserProfilePopoverDebugSettings.verticalPaddingKey)
}
}

init(
workspaceId: UUID,
profileID: UUID? = nil,
Expand All @@ -4246,6 +4322,9 @@ final class BrowserPanel: Panel, ObservableObject {
isRemoteWorkspace: Bool = false,
remoteWebsiteDataStoreIdentifier: UUID? = nil
) {
// Register fallback defaults and normalize legacy/out-of-range settings once
// per process, before any setting is read below or by the SwiftUI view.
Self.bootstrapBrowserDefaultsIfNeeded()
self.id = UUID()
self.workspaceId = workspaceId
let requestedProfileID = profileID ?? BrowserProfileStore.shared.effectiveLastUsedProfileID
Expand Down
60 changes: 29 additions & 31 deletions Sources/Panels/BrowserPanelView.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -459,6 +459,11 @@ struct BrowserPanelView: View {
@State private var suppressNextFocusGainedSelectAll: Bool = false
@State private var isBrowserProfileMenuPresented = false
@State private var isBrowserThemeMenuPresented = false
// `.onAppear` is not a reliable once-signal for a portal-hosted pane: it can
// re-fire on every CoreAnimation commit (issue #5303). This guards the first-
// appearance view-state seed (the empty-state import list) so a spurious appear
// does no work. App-once settings work lives in the model bootstrap, not here.
@State private var didCompleteInitialBrowserPanelSetup = false
@State private var browserChromeStyle = BrowserChromeStyle.resolve(
for: .light,
themeBackgroundColor: GhosttyBackgroundTheme.currentColor()
Expand Down Expand Up @@ -755,38 +760,15 @@ struct BrowserPanelView: View {
}

private func handleBrowserPanelAppear() {
// One-time setup must not re-run on every commit; `.onAppear` can re-fire
// repeatedly for a portal-hosted pane (issue #5303). Everything below the
// setup call is idempotent and cheap, and genuine state transitions are
// already handled by the dedicated `.onChange` observers on `body`, so
// re-running this per appear is harmless once the heavy/one-time work is
// gated out.
performInitialBrowserPanelSetupIfNeeded()
startOmnibarSuggestionRefreshConsumer()
UserDefaults.standard.register(defaults: [
BrowserSearchSettings.searchEngineKey: BrowserSearchSettings.defaultSearchEngine.rawValue,
BrowserSearchSettings.customSearchEngineNameKey: BrowserSearchSettings.defaultCustomSearchEngineName,
BrowserSearchSettings.customSearchEngineURLTemplateKey: BrowserSearchSettings.defaultCustomSearchEngineURLTemplate,
BrowserSearchSettings.searchSuggestionsEnabledKey: BrowserSearchSettings.defaultSearchSuggestionsEnabled,
BrowserToolbarAccessorySpacingDebugSettings.key: BrowserToolbarAccessorySpacingDebugSettings.defaultSpacing,
BrowserProfilePopoverDebugSettings.horizontalPaddingKey: BrowserProfilePopoverDebugSettings.defaultHorizontalPadding,
BrowserProfilePopoverDebugSettings.verticalPaddingKey: BrowserProfilePopoverDebugSettings.defaultVerticalPadding,
BrowserThemeSettings.modeKey: BrowserThemeSettings.defaultMode.rawValue,
])
refreshBrowserChromeStyle()
let resolvedThemeMode = BrowserThemeSettings.mode(defaults: .standard)
if browserThemeModeRaw != resolvedThemeMode.rawValue {
browserThemeModeRaw = resolvedThemeMode.rawValue
}
let resolvedHintVariant = BrowserImportHintSettings.variant(for: browserImportHintVariantRaw)
if browserImportHintVariantRaw != resolvedHintVariant.rawValue {
browserImportHintVariantRaw = resolvedHintVariant.rawValue
}
let resolvedToolbarAccessorySpacing = BrowserToolbarAccessorySpacingDebugSettings.resolved(browserToolbarAccessorySpacingRaw)
if browserToolbarAccessorySpacingRaw != resolvedToolbarAccessorySpacing {
browserToolbarAccessorySpacingRaw = resolvedToolbarAccessorySpacing
}
let resolvedProfilePopoverHorizontalPadding = BrowserProfilePopoverDebugSettings.resolvedHorizontalPadding(browserProfilePopoverHorizontalPaddingRaw)
if browserProfilePopoverHorizontalPaddingRaw != resolvedProfilePopoverHorizontalPadding {
browserProfilePopoverHorizontalPaddingRaw = resolvedProfilePopoverHorizontalPadding
}
let resolvedProfilePopoverVerticalPadding = BrowserProfilePopoverDebugSettings.resolvedVerticalPadding(browserProfilePopoverVerticalPaddingRaw)
if browserProfilePopoverVerticalPaddingRaw != resolvedProfilePopoverVerticalPadding {
browserProfilePopoverVerticalPaddingRaw = resolvedProfilePopoverVerticalPadding
}
panel.noteWebViewVisibility(
isVisibleInUI && isCurrentPaneOwner,
reason: "view.onAppear"
Expand All @@ -798,14 +780,30 @@ struct BrowserPanelView: View {
// If the browser surface is focused but has no URL loaded yet, auto-focus the omnibar.
autoFocusOmnibarIfBlank()
syncWebViewResponderPolicyWithViewState(reason: "onAppear")
refreshEmptyStateImportBrowsers()
panel.historyStore.loadIfNeeded()
#if DEBUG
logBrowserFocusState(event: "view.onAppear")
#endif
focusModeShortcutHintMonitor.start()
}

/// Runs the view-state initialization that should happen on first appearance,
/// independent of how many times SwiftUI fires `.onAppear` for the same view
/// instance.
///
/// `.onAppear` is not a reliable once-or-on-transition signal for a portal-hosted
/// browser pane — it can re-fire on every CoreAnimation commit (issue #5303).
/// Default registration and settings normalization are app-once work and live in
/// ``BrowserPanel/bootstrapBrowserDefaultsIfNeeded()`` (run from the model
/// init), not here. This method only seeds view-local state: the initial
/// empty-state import list, which `handleCurrentURLChange` refreshes on subsequent
/// new-tab navigations.
private func performInitialBrowserPanelSetupIfNeeded() {
guard !didCompleteInitialBrowserPanelSetup else { return }
didCompleteInitialBrowserPanelSetup = true
refreshEmptyStateImportBrowsers()
}
Comment on lines 801 to 805

@greptile-apps greptile-apps Bot Jun 3, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 @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)

@austinywang austinywang Jun 3, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


private func handleOmnibarVisibilityChange(_ isVisible: Bool) {
if !isVisible {
hideSuggestions()
Expand Down
105 changes: 105 additions & 0 deletions cmuxTests/GhosttyConfigTests.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2205,6 +2205,58 @@ final class BrowserPanelWebViewLifecycleTests: XCTestCase {
XCTAssertEqual(panel.webViewLifecycleState, .liveVisible)
}

/// Regression guard for the issue #5303 render loop: `BrowserPanelView.onAppear`
/// re-fired on every CoreAnimation commit and re-asserted webview visibility,
/// which restored + re-navigated the webview repeatedly. Once the webview is live
/// and visible, redundant visibility notifications (the shape a spurious appear
/// produces) must be no-ops: no lifecycle churn and no webview replacement, so no
/// re-navigation is issued.
func testRedundantVisibleNotificationsDoNotChurnLiveWebView() {
let panel = BrowserPanel(
workspaceId: UUID(),
initialURL: URL(string: "about:blank")!,
isRemoteWorkspace: false
)
defer { panel.close() }

let deadline = Date().addingTimeInterval(1.0)
while panel.webView.isLoading,
RunLoop.main.run(mode: .default, before: deadline),
Date() < deadline {}
Comment on lines +2222 to +2225

@greptile-apps greptile-apps Bot Jun 3, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Missing timeout-guard assertion after RunLoop spin

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!

@austinywang austinywang Jun 3, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed — added XCTAssertFalse(panel.webView.isLoading, "Timed out waiting for about:blank to finish loading") after the spin, matching the neighboring tests.

— Claude Code

XCTAssertFalse(panel.webView.isLoading, "Timed out waiting for about:blank to finish loading")

panel.noteWebViewVisibility(true, reason: "test.visible.first")
Comment thread
cubic-dev-ai[bot] marked this conversation as resolved.
XCTAssertEqual(panel.webViewLifecycleState, .liveVisible)

let webViewAfterFirst = panel.webView
let instanceIDAfterFirst = panel.webViewInstanceID
let reasonAfterFirst = panel.webViewLastVisibilityChangeReason
let changeAtAfterFirst = panel.webViewLastVisibilityChangeAt

var observedStates: [BrowserWebViewLifecycleState] = []
var cancellable: AnyCancellable?
cancellable = panel.$webViewLifecycleState.dropFirst().sink { state in
observedStates.append(state)
}
defer { cancellable?.cancel() }

// Simulate `.onAppear` re-firing many times in one commit storm.
for index in 0..<32 {
panel.noteWebViewVisibility(true, reason: "test.visible.spurious-\(index)")
}

XCTAssertEqual(panel.webViewLifecycleState, .liveVisible)
XCTAssertTrue(observedStates.isEmpty, "Redundant visible notes churned lifecycle: \(observedStates)")
XCTAssertTrue(panel.webView === webViewAfterFirst, "A live webview must not be replaced by redundant visibility notes")
XCTAssertEqual(panel.webViewInstanceID, instanceIDAfterFirst)
XCTAssertEqual(
panel.webViewLastVisibilityChangeReason,
reasonAfterFirst,
"Redundant visible notes must early-return without recording a new transition"
)
XCTAssertEqual(panel.webViewLastVisibilityChangeAt, changeAtAfterFirst)
}

func testRestoredHistoryBackDoesNotEmitNewTabLifecycleState() {
let discardedAt = Date(timeIntervalSince1970: 300)
let panel = BrowserPanel(
Expand Down Expand Up @@ -2245,6 +2297,59 @@ final class BrowserPanelWebViewLifecycleTests: XCTestCase {
}
}

@MainActor
final class BrowserDefaultsNormalizationTests: XCTestCase {
/// Moving default registration + settings normalization out of
/// `BrowserPanelView.onAppear` into the model bootstrap (issue #5303) keeps the
/// canonicalization behavior: an out-of-range or legacy raw value stored in
/// defaults is rewritten to its canonical form, and registered fallbacks are
/// available for unset keys.
func testNormalizeRewritesOutOfRangeAndLegacyValues() throws {
let suiteName = "cmux.browserDefaultsNormalizationTests.\(UUID().uuidString)"
let defaults = try XCTUnwrap(UserDefaults(suiteName: suiteName))
defer { defaults.removePersistentDomain(forName: suiteName) }

// Out-of-range / invalid raw values that must be canonicalized.
defaults.set("not-a-real-mode", forKey: BrowserThemeSettings.modeKey)
defaults.set("not-a-real-variant", forKey: BrowserImportHintSettings.variantKey)
defaults.set(999, forKey: BrowserToolbarAccessorySpacingDebugSettings.key)
defaults.set(999.0, forKey: BrowserProfilePopoverDebugSettings.horizontalPaddingKey)
defaults.set(-5.0, forKey: BrowserProfilePopoverDebugSettings.verticalPaddingKey)

BrowserPanel.normalizeBrowserDefaults(defaults: defaults)

XCTAssertEqual(defaults.string(forKey: BrowserThemeSettings.modeKey), BrowserThemeSettings.defaultMode.rawValue)
XCTAssertEqual(defaults.string(forKey: BrowserImportHintSettings.variantKey), BrowserImportHintSettings.defaultVariant.rawValue)
XCTAssertEqual(defaults.integer(forKey: BrowserToolbarAccessorySpacingDebugSettings.key), BrowserToolbarAccessorySpacingDebugSettings.defaultSpacing)
XCTAssertEqual(defaults.double(forKey: BrowserProfilePopoverDebugSettings.horizontalPaddingKey), BrowserProfilePopoverDebugSettings.defaultHorizontalPadding, accuracy: 0.0001)
XCTAssertEqual(defaults.double(forKey: BrowserProfilePopoverDebugSettings.verticalPaddingKey), BrowserProfilePopoverDebugSettings.defaultVerticalPadding, accuracy: 0.0001)

// Registered fallbacks are available for keys that were never set.
XCTAssertEqual(defaults.string(forKey: BrowserSearchSettings.searchEngineKey), BrowserSearchSettings.defaultSearchEngine.rawValue)
}

/// Already-canonical, in-range values must be left untouched (no clobbering of
/// valid user settings during normalization).
func testNormalizePreservesValidValues() throws {
let suiteName = "cmux.browserDefaultsNormalizationTests.\(UUID().uuidString)"
let defaults = try XCTUnwrap(UserDefaults(suiteName: suiteName))
defer { defaults.removePersistentDomain(forName: suiteName) }

let validSpacing = BrowserToolbarAccessorySpacingDebugSettings.supportedValues.last ?? BrowserToolbarAccessorySpacingDebugSettings.defaultSpacing
// Resolve the app-target theme mode via the app-only settings type; the bare
// `BrowserThemeMode` is ambiguous here because this file also imports
// `CmuxSettings`, which declares a same-named enum.
let validThemeRaw = BrowserThemeSettings.mode(for: "dark").rawValue
defaults.set(validThemeRaw, forKey: BrowserThemeSettings.modeKey)
defaults.set(validSpacing, forKey: BrowserToolbarAccessorySpacingDebugSettings.key)

BrowserPanel.normalizeBrowserDefaults(defaults: defaults)

XCTAssertEqual(defaults.string(forKey: BrowserThemeSettings.modeKey), validThemeRaw)
XCTAssertEqual(defaults.integer(forKey: BrowserToolbarAccessorySpacingDebugSettings.key), validSpacing)
}
}

final class BrowserNewTabNavigationSeedTests: XCTestCase {
func testPreservesOriginalRequestHeadersMethodBodyAndBypassHost() throws {
let url = try XCTUnwrap(URL(string: "https://www.linkedin.com/redir/redirect?url=https%3A%2F%2Fexample.com"))
Expand Down
Loading