Skip to content
Merged
36 changes: 30 additions & 6 deletions Sources/App/SettingsWindowPresenter.swift
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,9 @@ enum SettingsWindowPresenter {
private static var pendingNavigationTarget: SettingsNavigationTarget?
private static var pendingContentNavigationTarget: SettingsNavigationTarget?
private static var shouldOpenWhenConfigured = false
#if DEBUG
private static var focusHandlerForTests: (@MainActor (NSWindow) -> Void)?
#endif

static func configure(
openWindow: @escaping @MainActor () -> Void,
Expand All @@ -33,16 +36,19 @@ enum SettingsWindowPresenter {
}

static func configure(window: NSWindow) {
let shouldFocusAfterConfiguration = settingsWindow !== window
settingsWindow = window
window.identifier = NSUserInterfaceItemIdentifier(windowIdentifier)
window.isRestorable = false
window.minSize = minimumSize
window.contentMinSize = minimumSize
clampToVisibleAreaIfNeeded(window)
attachToPreferredParent(window)
Task { @MainActor in
guard settingsWindow === window else { return }
focus(window)
if shouldFocusAfterConfiguration {
Task { @MainActor in
guard settingsWindow === window else { return }
focus(window)
}
}
}

Expand All @@ -64,10 +70,13 @@ enum SettingsWindowPresenter {
pendingContentNavigationTarget = navigationTarget

if let window = existingWindow() {
pendingNavigationTarget = nil
pendingContentNavigationTarget = nil
let shouldDeferNavigation = window.isMiniaturized
if !shouldDeferNavigation {
pendingNavigationTarget = nil
pendingContentNavigationTarget = nil
}
focus(window)
if let navigationTarget {
if let navigationTarget, !shouldDeferNavigation {
SettingsNavigationRequest.post(navigationTarget)
}
return
Expand Down Expand Up @@ -115,6 +124,11 @@ enum SettingsWindowPresenter {
pendingNavigationTarget = nil
pendingContentNavigationTarget = nil
shouldOpenWhenConfigured = false
focusHandlerForTests = nil
}

static func setFocusHandlerForTests(_ handler: @escaping @MainActor (NSWindow) -> Void) {
focusHandlerForTests = handler
}
#endif

Expand All @@ -128,6 +142,16 @@ enum SettingsWindowPresenter {
}

private static func focus(_ window: NSWindow) {
#if DEBUG
if let focusHandlerForTests {
focusHandlerForTests(window)
return
}
#endif
performFocus(window)
}

private static func performFocus(_ window: NSWindow) {
if window.isMiniaturized {
window.deminiaturize(nil)
}
Expand Down
138 changes: 108 additions & 30 deletions Sources/cmuxApp.swift
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import AppKit
import SwiftUI
import Observation
import Darwin
import Bonsplit
import UniformTypeIdentifiers
Expand Down Expand Up @@ -634,11 +635,8 @@ struct cmuxApp: App {
}

Window(String(localized: "settings.title", defaultValue: "Settings"), id: SettingsWindowPresenter.windowID) {
SettingsRootView()
SettingsWindowRootView()
.cmuxAppearanceColorScheme(appearanceMode)
Comment thread
coderabbitai[bot] marked this conversation as resolved.
.background(WindowAccessor { window in
SettingsWindowPresenter.configure(window: window)
})
}
.defaultSize(width: 980, height: 680)
.windowResizability(.contentMinSize)
Expand Down Expand Up @@ -5322,8 +5320,7 @@ struct SettingsView: View {
@State private var didRequestBrowserImportDetection = false
@State private var isDetectingImportBrowsers = false
@State private var browserImportDetectionGeneration = 0
@State private var browserInsecureHTTPAllowlistDraft = BrowserInsecureHTTPSettings.defaultAllowlistText
@State private var socketPasswordDraft = ""
@Bindable var draftState: SettingsDraftState
@State private var socketPasswordStatusMessage: String?
@State private var socketPasswordStatusIsError = false
@State private var notificationCustomSoundStatusMessage: String?
Expand Down Expand Up @@ -5806,7 +5803,7 @@ struct SettingsView: View {
}

private var browserInsecureHTTPAllowlistHasUnsavedChanges: Bool {
browserInsecureHTTPAllowlistDraft != browserInsecureHTTPAllowlist
draftState.browserInsecureHTTPAllowlistDraft != browserInsecureHTTPAllowlist
}

private var hasCustomNotificationSoundFilePath: Bool {
Expand Down Expand Up @@ -6087,7 +6084,7 @@ struct SettingsView: View {
}

private func saveSocketPassword() {
let trimmed = socketPasswordDraft.trimmingCharacters(in: .whitespacesAndNewlines)
let trimmed = draftState.socketPasswordDraft.trimmingCharacters(in: .whitespacesAndNewlines)
guard !trimmed.isEmpty else {
socketPasswordStatusMessage = String(localized: "settings.automation.socketPassword.enterFirst", defaultValue: "Enter a password first.")
socketPasswordStatusIsError = true
Expand All @@ -6096,7 +6093,7 @@ struct SettingsView: View {

do {
try SocketControlPasswordStore.savePassword(trimmed)
socketPasswordDraft = ""
draftState.socketPasswordDraft = ""
socketPasswordStatusMessage = String(localized: "settings.automation.socketPassword.saved", defaultValue: "Password saved.")
socketPasswordStatusIsError = false
} catch {
Expand All @@ -6108,7 +6105,7 @@ struct SettingsView: View {
private func clearSocketPassword() {
do {
try SocketControlPasswordStore.clearPassword()
socketPasswordDraft = ""
draftState.socketPasswordDraft = ""
socketPasswordStatusMessage = String(localized: "settings.automation.socketPassword.cleared", defaultValue: "Password cleared.")
socketPasswordStatusIsError = false
} catch {
Expand Down Expand Up @@ -7012,15 +7009,15 @@ struct SettingsView: View {
: String(localized: "settings.automation.socketPassword.subtitleUnset", defaultValue: "No password set. External clients will be blocked until one is configured.")
) {
HStack(spacing: 8) {
SecureField(String(localized: "settings.automation.socketPassword.placeholder", defaultValue: "Password"), text: $socketPasswordDraft)
SecureField(String(localized: "settings.automation.socketPassword.placeholder", defaultValue: "Password"), text: $draftState.socketPasswordDraft)
.textFieldStyle(.roundedBorder)
.frame(width: 170)
Button(hasSocketPasswordConfigured ? String(localized: "settings.automation.socketPassword.change", defaultValue: "Change") : String(localized: "settings.automation.socketPassword.set", defaultValue: "Set")) {
saveSocketPassword()
}
.buttonStyle(.bordered)
.controlSize(.small)
.disabled(socketPasswordDraft.trimmingCharacters(in: .whitespacesAndNewlines).isEmpty)
.disabled(draftState.socketPasswordDraft.trimmingCharacters(in: .whitespacesAndNewlines).isEmpty)
if hasSocketPasswordConfigured {
Button(String(localized: "settings.automation.socketPassword.clear", defaultValue: "Clear")) {
clearSocketPassword()
Expand Down Expand Up @@ -7349,7 +7346,7 @@ struct SettingsView: View {
.font(.caption)
.foregroundStyle(.secondary)

TextEditor(text: $browserInsecureHTTPAllowlistDraft)
TextEditor(text: $draftState.browserInsecureHTTPAllowlistDraft)
.font(.system(size: 12, weight: .regular, design: .monospaced))
.frame(minHeight: 86)
.padding(6)
Expand Down Expand Up @@ -7811,7 +7808,7 @@ struct SettingsView: View {
browserImportHintVariantRaw = BrowserImportHintSettings.variant(for: browserImportHintVariantRaw).rawValue
didLoadBrowserHistoryForSettings = BrowserHistoryStore.shared.isLoaded
browserHistoryEntryCount = didLoadBrowserHistoryForSettings ? BrowserHistoryStore.shared.entries.count : 0
browserInsecureHTTPAllowlistDraft = browserInsecureHTTPAllowlist
Comment thread
cursor[bot] marked this conversation as resolved.
draftState.syncBrowserInsecureHTTPAllowlistFromSavedValue(browserInsecureHTTPAllowlist)
reloadWorkspaceTabColorSettings()
refreshNotificationCustomSoundStatus()
let target = SettingsWindowPresenter.consumePendingContentNavigationTarget()
Expand All @@ -7832,11 +7829,9 @@ struct SettingsView: View {
.onChange(of: notificationSoundCustomFilePath) { _, _ in
refreshNotificationCustomSoundStatus()
}
.onChange(of: browserInsecureHTTPAllowlist) { oldValue, newValue in
.onChange(of: browserInsecureHTTPAllowlist) { _, newValue in
// Keep draft in sync with external changes unless the user has local unsaved edits.
if browserInsecureHTTPAllowlistDraft == oldValue {
browserInsecureHTTPAllowlistDraft = newValue
}
draftState.syncBrowserInsecureHTTPAllowlistFromSavedValue(newValue)
}
.onReceive(BrowserHistoryStore.shared.$entries) { entries in
guard BrowserHistoryStore.shared.isLoaded else { return }
Expand Down Expand Up @@ -7962,7 +7957,8 @@ struct SettingsView: View {
browserHostWhitelist = BrowserLinkOpenSettings.defaultBrowserHostWhitelist
browserExternalOpenPatterns = BrowserLinkOpenSettings.defaultBrowserExternalOpenPatterns
browserInsecureHTTPAllowlist = BrowserInsecureHTTPSettings.defaultAllowlistText
browserInsecureHTTPAllowlistDraft = BrowserInsecureHTTPSettings.defaultAllowlistText
draftState.browserInsecureHTTPAllowlistDraft = BrowserInsecureHTTPSettings.defaultAllowlistText
draftState.browserInsecureHTTPAllowlistSyncedValue = BrowserInsecureHTTPSettings.defaultAllowlistText
notificationSound = NotificationSoundSettings.defaultValue
notificationSoundCustomFilePath = NotificationSoundSettings.defaultCustomFilePath
notificationCustomSoundStatusMessage = nil
Expand Down Expand Up @@ -8040,7 +8036,7 @@ struct SettingsView: View {
sidebarMatchTerminalBackground = false
showOpenAccessConfirmation = false
pendingOpenAccessMode = nil
socketPasswordDraft = ""
draftState.socketPasswordDraft = ""
socketPasswordStatusMessage = nil
socketPasswordStatusIsError = false
refreshDetectedImportBrowsers()
Expand Down Expand Up @@ -8095,7 +8091,8 @@ struct SettingsView: View {
}

private func saveBrowserInsecureHTTPAllowlist() {
browserInsecureHTTPAllowlist = browserInsecureHTTPAllowlistDraft
browserInsecureHTTPAllowlist = draftState.browserInsecureHTTPAllowlistDraft
draftState.browserInsecureHTTPAllowlistSyncedValue = draftState.browserInsecureHTTPAllowlistDraft
}

private func refreshDetectedImportBrowsers() {
Expand Down Expand Up @@ -8817,22 +8814,104 @@ private struct GlobalHotkeySection: View {
}
}

private struct SettingsWindowRootView: View {
@State private var draftState = SettingsDraftState()
@State private var windowReference = WeakSettingsWindowReference()
@State private var shouldRenderSettingsContent = true

var body: some View {
Group {
if shouldRenderSettingsContent {
SettingsRootView(draftState: draftState)
} else {
Color.clear
.frame(
minWidth: SettingsWindowPresenter.minimumSize.width,
minHeight: SettingsWindowPresenter.minimumSize.height
)
}
Comment thread
cursor[bot] marked this conversation as resolved.
}
.background(WindowAccessor { window in
windowReference.window = window
SettingsWindowPresenter.configure(window: window)
setContentVisibility(!window.isMiniaturized)
})
.onReceive(NotificationCenter.default.publisher(for: NSWindow.didMiniaturizeNotification)) { notification in
guard isObservedWindow(notification.object) else { return }
setContentVisibility(false)
}
.onReceive(NotificationCenter.default.publisher(for: NSWindow.didDeminiaturizeNotification)) { notification in
guard isObservedWindow(notification.object) else { return }
setContentVisibility(true)
}
.onReceive(NotificationCenter.default.publisher(for: NSWindow.didBecomeKeyNotification)) { notification in
guard isObservedWindow(notification.object) else { return }
setContentVisibility(true)
}
.onReceive(NotificationCenter.default.publisher(for: NSWindow.didBecomeMainNotification)) { notification in
guard isObservedWindow(notification.object) else { return }
setContentVisibility(true)
}
.onReceive(NotificationCenter.default.publisher(for: NSWindow.willCloseNotification)) { notification in
guard isObservedWindow(notification.object) else { return }
setContentVisibility(false)
windowReference.window = nil
}
}

private func isObservedWindow(_ object: Any?) -> Bool {
guard
let notificationWindow = object as? NSWindow,
let window = windowReference.window
else {
return false
}
return notificationWindow === window
}

private func setContentVisibility(_ isVisible: Bool) {
guard shouldRenderSettingsContent != isVisible else { return }
shouldRenderSettingsContent = isVisible
}
}
Comment on lines +8820 to +8876

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 Ephemeral @State lost on every miniaturize/restore cycle

Because SettingsRootView is fully torn down when shouldRenderSettingsContent goes false, any @State that isn't backed by @SceneStorage is silently discarded. Concretely, searchText resets to "" and columnVisibility resets to .all each time the window is miniaturized then restored. A user who typed a search query, minimizes Settings while Codex runs, and then restores it will find the search field blank — that's a behavioral change from the pre-PR code where the tree was never unmounted. @SceneStorage values (section, sidebar entry) survive, but non-persisted view state does not.

Rule Used: Flag SwiftUI changes that can cause stale state, b... (source)


@MainActor
private final class WeakSettingsWindowReference {
weak var window: NSWindow?
}

@MainActor
@Observable
final class SettingsDraftState {
var browserInsecureHTTPAllowlistDraft = BrowserInsecureHTTPSettings.defaultAllowlistText
var browserInsecureHTTPAllowlistSyncedValue = BrowserInsecureHTTPSettings.defaultAllowlistText
var socketPasswordDraft = ""
var settingsColumnVisibility: NavigationSplitViewVisibility = .all
var settingsSearchText = ""

func syncBrowserInsecureHTTPAllowlistFromSavedValue(_ savedValue: String) {
if browserInsecureHTTPAllowlistDraft == browserInsecureHTTPAllowlistSyncedValue {
browserInsecureHTTPAllowlistDraft = savedValue
}
browserInsecureHTTPAllowlistSyncedValue = savedValue
}
}

private struct SettingsRootView: View {
@Bindable var draftState: SettingsDraftState
@SceneStorage("selectedSettingsSection") private var selectedSectionRaw = SettingsNavigationTarget.account.rawValue
@SceneStorage("selectedSettingsSidebarEntry") private var selectedSidebarEntryID = SettingsSearchIndex.defaultSelectionID
@State private var columnVisibility: NavigationSplitViewVisibility = .all
@State private var searchText = ""

private var selectedSection: SettingsNavigationTarget {
SettingsNavigationTarget(rawValue: selectedSectionRaw) ?? .account
}

private var sidebarEntries: [SettingsSearchEntry] {
SettingsSearchIndex.entries(matching: searchText)
SettingsSearchIndex.entries(matching: draftState.settingsSearchText)
}

private var isSearching: Bool {
!searchText.trimmingCharacters(in: .whitespacesAndNewlines).isEmpty
!draftState.settingsSearchText.trimmingCharacters(in: .whitespacesAndNewlines).isEmpty
}

private var sidebarSelection: Binding<String> {
Expand All @@ -8843,7 +8922,7 @@ private struct SettingsRootView: View {
}

var body: some View {
NavigationSplitView(columnVisibility: $columnVisibility) {
NavigationSplitView(columnVisibility: $draftState.settingsColumnVisibility) {
List(selection: sidebarSelection) {
if sidebarEntries.isEmpty {
Text(String(localized: "settings.search.noResults", defaultValue: "No Results"))
Expand All @@ -8858,22 +8937,21 @@ private struct SettingsRootView: View {
.listStyle(.sidebar)
.navigationTitle(String(localized: "settings.title", defaultValue: "Settings"))
.searchable(
text: $searchText,
text: $draftState.settingsSearchText,
placement: .sidebar,
prompt: Text(String(localized: "settings.search.prompt", defaultValue: "Search"))
)
.navigationSplitViewColumnWidth(210)
} detail: {
SettingsView()
SettingsView(draftState: draftState)
}
.navigationSplitViewStyle(.balanced)
.frame(minWidth: SettingsWindowPresenter.minimumSize.width, minHeight: SettingsWindowPresenter.minimumSize.height)
.onChange(of: searchText) { _, newValue in
.onChange(of: draftState.settingsSearchText) { _, newValue in

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Root navigates on every remount

Medium Severity

Unmounting settings while minimized means SettingsRootView is recreated on restore. Its onAppear always calls navigate(to:selectedSection, postRequest:true) when no pending target, reposting SettingsNavigationRequest even though SettingsView already applied initial navigation via didApplyInitialContentNavigation.

Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit f184ec6. Configure here.

guard newValue.trimmingCharacters(in: .whitespacesAndNewlines).isEmpty else { return }
selectedSidebarEntryID = SettingsSearchIndex.sectionID(for: selectedSection)
}
.onAppear {
searchText = ""
if let target = SettingsWindowPresenter.consumePendingNavigationTarget() {
navigate(to: target, postRequest: true)
} else {
Expand Down
Loading
Loading