diff --git a/Sources/Panels/BrowserPanel.swift b/Sources/Panels/BrowserPanel.swift index cf048b5abd49..a614951db19a 100644 --- a/Sources/Panels/BrowserPanel.swift +++ b/Sources/Panels/BrowserPanel.swift @@ -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, @@ -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 diff --git a/Sources/Panels/BrowserPanelView.swift b/Sources/Panels/BrowserPanelView.swift index ad5044c0597f..03cf9db445c3 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 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() @@ -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" @@ -798,7 +780,6 @@ 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") @@ -806,6 +787,23 @@ struct BrowserPanelView: View { 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() + } + private func handleOmnibarVisibilityChange(_ isVisible: Bool) { if !isVisible { hideSuggestions() diff --git a/cmuxTests/GhosttyConfigTests.swift b/cmuxTests/GhosttyConfigTests.swift index b06a9f20af78..b145e7ae5c54 100644 --- a/cmuxTests/GhosttyConfigTests.swift +++ b/cmuxTests/GhosttyConfigTests.swift @@ -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 {} + XCTAssertFalse(panel.webView.isLoading, "Timed out waiting for about:blank to finish loading") + + 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( @@ -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"))