From 0d50e8e3e9bcfbc60ab2ff588f57cf05131e4f27 Mon Sep 17 00:00:00 2001 From: austinpower1258 Date: Wed, 3 Jun 2026 13:53:51 -0700 Subject: [PATCH 1/4] Fix #5303: stop browser pane re-running one-time setup on every commit `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 --- Sources/Panels/BrowserPanelView.swift | 64 ++++++++++++++++++++------- cmuxTests/GhosttyConfigTests.swift | 51 +++++++++++++++++++++ 2 files changed, 98 insertions(+), 17 deletions(-) diff --git a/Sources/Panels/BrowserPanelView.swift b/Sources/Panels/BrowserPanelView.swift index ad5044c0597f..28d49dec9ed7 100644 --- a/Sources/Panels/BrowserPanelView.swift +++ b/Sources/Panels/BrowserPanelView.swift @@ -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 one-time + // setup so default registration, settings normalization, and the initial + // empty-state populate run exactly once per view instance instead of per commit. + @State private var didCompleteInitialBrowserPanelSetup = false @State private var browserChromeStyle = BrowserChromeStyle.resolve( for: .light, themeBackgroundColor: GhosttyBackgroundTheme.currentColor() @@ -755,7 +760,45 @@ 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() + refreshBrowserChromeStyle() + 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") + panel.historyStore.loadIfNeeded() +#if DEBUG + logBrowserFocusState(event: "view.onAppear") +#endif + focusModeShortcutHintMonitor.start() + } + + /// Runs the work that must execute exactly once per `BrowserPanelView` instance, + /// independent of how many times SwiftUI fires `.onAppear`. + /// + /// `.onAppear` is not a reliable once-or-on-transition signal for a portal-hosted + /// browser pane — it can re-fire on every CoreAnimation commit. Default + /// registration, settings normalization (which writes `@AppStorage` and would + /// otherwise re-enter the commit pass), and the initial empty-state import + /// populate all belong here so a spurious appear does no work (issue #5303). + private func performInitialBrowserPanelSetupIfNeeded() { + guard !didCompleteInitialBrowserPanelSetup else { return } + didCompleteInitialBrowserPanelSetup = true + UserDefaults.standard.register(defaults: [ BrowserSearchSettings.searchEngineKey: BrowserSearchSettings.defaultSearchEngine.rawValue, BrowserSearchSettings.customSearchEngineNameKey: BrowserSearchSettings.defaultCustomSearchEngineName, @@ -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() } private func handleOmnibarVisibilityChange(_ isVisible: Bool) { diff --git a/cmuxTests/GhosttyConfigTests.swift b/cmuxTests/GhosttyConfigTests.swift index b06a9f20af78..1a2c3aa6b168 100644 --- a/cmuxTests/GhosttyConfigTests.swift +++ b/cmuxTests/GhosttyConfigTests.swift @@ -2205,6 +2205,57 @@ 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 {} + + panel.noteWebViewVisibility(true, reason: "test.visible.first") + 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( From aaffaf41689216f3e84c5de99eebdfcc8631e18c Mon Sep 17 00:00:00 2001 From: austinpower1258 Date: Wed, 3 Jun 2026 14:21:59 -0700 Subject: [PATCH 2/4] Address review: move browser defaults bootstrap to process-once model 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 --- Sources/Panels/BrowserPanel.swift | 74 +++++++++++++++++++++++++++ Sources/Panels/BrowserPanelView.swift | 56 +++++--------------- cmuxTests/GhosttyConfigTests.swift | 50 ++++++++++++++++++ 3 files changed, 136 insertions(+), 44 deletions(-) diff --git a/Sources/Panels/BrowserPanel.swift b/Sources/Panels/BrowserPanel.swift index cf048b5abd49..4c100753f1fa 100644 --- a/Sources/Panels/BrowserPanel.swift +++ b/Sources/Panels/BrowserPanel.swift @@ -4231,6 +4231,77 @@ 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. + static func bootstrapBrowserDefaultsIfNeeded(defaults: UserDefaults = .standard) { + guard !hasBootstrappedBrowserDefaults else { return } + hasBootstrappedBrowserDefaults = true + normalizeBrowserDefaults(defaults: defaults) + } + + /// 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, @@ -4246,6 +4317,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 diff --git a/Sources/Panels/BrowserPanelView.swift b/Sources/Panels/BrowserPanelView.swift index 28d49dec9ed7..ccb31e5aad9e 100644 --- a/Sources/Panels/BrowserPanelView.swift +++ b/Sources/Panels/BrowserPanelView.swift @@ -460,9 +460,9 @@ struct BrowserPanelView: View { @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 one-time - // setup so default registration, settings normalization, and the initial - // empty-state populate run exactly once per view instance instead of per commit. + // 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, @@ -787,52 +787,20 @@ struct BrowserPanelView: View { focusModeShortcutHintMonitor.start() } - /// Runs the work that must execute exactly once per `BrowserPanelView` instance, - /// independent of how many times SwiftUI fires `.onAppear`. + /// 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. Default - /// registration, settings normalization (which writes `@AppStorage` and would - /// otherwise re-enter the commit pass), and the initial empty-state import - /// populate all belong here so a spurious appear does no work (issue #5303). + /// 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(defaults:)`` (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 - - 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, - ]) - - 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 - } - - // Populate the empty-state import list once; `handleCurrentURLChange` - // refreshes it on subsequent new-tab navigations. refreshEmptyStateImportBrowsers() } diff --git a/cmuxTests/GhosttyConfigTests.swift b/cmuxTests/GhosttyConfigTests.swift index 1a2c3aa6b168..ae4feff4f194 100644 --- a/cmuxTests/GhosttyConfigTests.swift +++ b/cmuxTests/GhosttyConfigTests.swift @@ -2223,6 +2223,7 @@ final class BrowserPanelWebViewLifecycleTests: XCTestCase { while panel.webView.isLoading, RunLoop.main.run(mode: .default, before: deadline), Date() < deadline {} + XCTAssertFalse(panel.webView.isLoading, "Timed out waiting for about:blank to finish loading") panel.noteWebViewVisibility(true, reason: "test.visible.first") XCTAssertEqual(panel.webViewLifecycleState, .liveVisible) @@ -2296,6 +2297,55 @@ 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 + defaults.set(BrowserThemeMode.dark.rawValue, forKey: BrowserThemeSettings.modeKey) + defaults.set(validSpacing, forKey: BrowserToolbarAccessorySpacingDebugSettings.key) + + BrowserPanel.normalizeBrowserDefaults(defaults: defaults) + + XCTAssertEqual(defaults.string(forKey: BrowserThemeSettings.modeKey), BrowserThemeMode.dark.rawValue) + 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")) From ecbc20b28cbb6340accd7e7f33b5482b3927f6ea Mon Sep 17 00:00:00 2001 From: austinpower1258 Date: Wed, 3 Jun 2026 14:29:45 -0700 Subject: [PATCH 3/4] fix(tests): disambiguate BrowserThemeMode in normalization test 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 --- cmuxTests/GhosttyConfigTests.swift | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/cmuxTests/GhosttyConfigTests.swift b/cmuxTests/GhosttyConfigTests.swift index ae4feff4f194..b145e7ae5c54 100644 --- a/cmuxTests/GhosttyConfigTests.swift +++ b/cmuxTests/GhosttyConfigTests.swift @@ -2336,12 +2336,16 @@ final class BrowserDefaultsNormalizationTests: XCTestCase { defer { defaults.removePersistentDomain(forName: suiteName) } let validSpacing = BrowserToolbarAccessorySpacingDebugSettings.supportedValues.last ?? BrowserToolbarAccessorySpacingDebugSettings.defaultSpacing - defaults.set(BrowserThemeMode.dark.rawValue, forKey: BrowserThemeSettings.modeKey) + // 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), BrowserThemeMode.dark.rawValue) + XCTAssertEqual(defaults.string(forKey: BrowserThemeSettings.modeKey), validThemeRaw) XCTAssertEqual(defaults.integer(forKey: BrowserToolbarAccessorySpacingDebugSettings.key), validSpacing) } } From 5dfefa4745a487f919d9404f9fec59b42284f726 Mon Sep 17 00:00:00 2001 From: Aziz Albahar Date: Wed, 3 Jun 2026 16:36:03 -0500 Subject: [PATCH 4/4] Address review: make browser defaults bootstrap non-injectable 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) --- Sources/Panels/BrowserPanel.swift | 9 +++++++-- Sources/Panels/BrowserPanelView.swift | 2 +- 2 files changed, 8 insertions(+), 3 deletions(-) diff --git a/Sources/Panels/BrowserPanel.swift b/Sources/Panels/BrowserPanel.swift index 4c100753f1fa..a614951db19a 100644 --- a/Sources/Panels/BrowserPanel.swift +++ b/Sources/Panels/BrowserPanel.swift @@ -4243,10 +4243,15 @@ final class BrowserPanel: Panel, ObservableObject { /// 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. - static func bootstrapBrowserDefaultsIfNeeded(defaults: UserDefaults = .standard) { + /// + /// 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: defaults) + normalizeBrowserDefaults(defaults: .standard) } /// Registers fallback defaults and writes back canonical values for any stored diff --git a/Sources/Panels/BrowserPanelView.swift b/Sources/Panels/BrowserPanelView.swift index ccb31e5aad9e..03cf9db445c3 100644 --- a/Sources/Panels/BrowserPanelView.swift +++ b/Sources/Panels/BrowserPanelView.swift @@ -794,7 +794,7 @@ struct BrowserPanelView: View { /// `.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(defaults:)`` (run from the model + /// ``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.