Skip to content

Fix hidden Settings CPU during Codex output - #4661

Merged
lawrencecchen merged 10 commits into
mainfrom
feat-settings-cpu-instrumentation
May 25, 2026
Merged

lawrencecchen merged 10 commits into
mainfrom
feat-settings-cpu-instrumentation

Conversation

@lawrencecchen

@lawrencecchen lawrencecchen commented May 23, 2026 •

Copy link
Copy Markdown
Contributor

Summary

  • unmount the expensive Settings content tree while the Settings window is minimized or closing
  • avoid refocusing the same Settings window on repeated WindowAccessor callbacks
  • add regression coverage for repeated Settings configure calls

Verification

  • CMUX_SKIP_ZIG_BUILD=1 ./scripts/reload.sh --tag feat-settings-cpu-instrumentation
  • sampled production and tagged builds while real Codex was running in cmux; hidden Settings no longer remains the hot SettingsRootView/SettingsView path in the fixed build

Dogfood

Open Settings, start a Codex terminal, minimize Settings, and scroll while Codex is active. The terminal should remain responsive without hidden Settings consuming the main-thread layout budget.


View with Codesmith Autofix with Codesmith
Need help on this PR? Tag @codesmith with what you need. Autofix is disabled.


Note

Medium Risk
Changes how the Settings window renders and preserves state across minimize/restore, plus tweaks focus/navigation behavior; regressions could impact Settings UX or navigation timing but are contained to the Settings window.

Overview
Reduces hidden Settings CPU usage by introducing SettingsWindowRootView, which unmounts the heavy Settings view tree while the window is miniaturized/closing and restores it on deminiaturize/key/main, rendering a lightweight placeholder to maintain the minimum window size.

Adds an @Observable SettingsDraftState to persist drafts and navigation UI state (search text, split visibility, socket password draft, HTTP allowlist draft) across unmount/remount, including sync logic to avoid overwriting unsaved allowlist edits.

Refines SettingsWindowPresenter behavior to avoid refocusing on repeated configure(window:) calls and to defer clearing/posting navigation requests when the existing Settings window is miniaturized; adds DEBUG-only focus injection plus new tests covering both regressions.

Reviewed by Cursor Bugbot for commit 24741af. Bugbot is set up for automated code reviews on this repo. Configure here.


Summary by cubic

Stops hidden Settings from burning CPU during Codex output by unmounting its view tree when the window is minimized or closing. Preserves drafts, search/sidebar state, selected section, and scroll; defers navigation and focus work until the window is restored.

  • Bug Fixes
    • Introduced SettingsWindowRootView to unmount SettingsRootView while miniaturized/closing and remount on deminiaturize/key/main; uses a lightweight placeholder to keep min size and holds only a weak NSWindow reference.
    • Added shared @Observable SettingsDraftState to persist drafts and navigation UI (search text, split visibility); syncs the insecure HTTP allowlist without overwriting unsaved edits; restores selected section/sidebar on remount and applies initial content navigation only once to preserve scroll.
    • Updated SettingsWindowPresenter.configure(window:) to focus only when the window changes; added a DEBUG-only focus hook and a regression test.
    • When showing Settings with an existing miniaturized window, keep navigation pending and defer SettingsNavigationRequest until restore; added a test.

Written for commit 4e8cdc0. Summary will update on new commits. Review in cubic

Summary by CodeRabbit

  • New Features

    • Persistent draft state for Settings so edits (password, HTTP allowlist) are tracked, can be saved or reset, and stay in sync with stored values.
    • Settings UI now toggles actual content vs placeholder when the window is miniaturized to preserve layout.
  • Bug Fixes

    • Navigation is deferred while the settings window is miniaturized and repeated refocus is avoided when reusing the same window.
  • Tests

    • Added async UI tests for focus, deferred navigation, and miniaturized-window scenarios.

Review Change Stack

@chatgpt-codex-connector

Copy link
Copy Markdown

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.
To continue using code reviews, add credits to your account and enable them for code reviews in your settings.

@vercel

vercel Bot commented May 23, 2026 •

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
cmux Ready Ready Preview, Comment May 25, 2026 11:07am
cmux-staging Building Building Preview, Comment May 25, 2026 11:07am

@coderabbitai

coderabbitai Bot commented May 23, 2026 •

Copy link
Copy Markdown

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review
📝 Walkthrough

Walkthrough

SettingsWindowPresenter gains a DEBUG test focus hook and defers posting navigation when reusing a miniaturized window. A new SettingsWindowRootView captures and configures the NSWindow, conditionally renders settings UI, and provides a shared @Observable SettingsDraftState to SettingsRootView/SettingsView.

Changes

Settings Window Focus and Rendering

Layer / File(s) Summary
Focus handler refactoring and test support
Sources/App/SettingsWindowPresenter.swift, cmuxTests/SettingsWindowPresenterTests.swift
Adds a DEBUG test focus slot and setFocusHandlerForTests(_:)/resetForTests(). focus(_:) delegates to the test handler or performFocus(_:). configure(window:) schedules focus but checks the configured window still matches before calling. show(...) defers clearing/posting navigation when reusing a miniaturized window. Tests validate single refocus and preserved pending navigation when miniaturized.
Settings window rendering wrapper with lifecycle management and draft state
Sources/cmuxApp.swift
Replaces direct SettingsRootView() with SettingsWindowRootView() that captures the hosting NSWindow, calls SettingsWindowPresenter.configure(window:), and toggles shouldRenderSettingsContent based on miniaturize/key/visibility/close notifications, rendering a sized placeholder when hidden. Introduces SettingsDraftState (@Observable) owned by the root view and passed into SettingsRootView/SettingsView as @Bindable, migrating allowlist and socket-password draft, sync, save, and reset behaviors to the shared draft model.
sequenceDiagram
  participant SettingsWindowRootView
  participant SettingsWindowPresenter
  participant focusHandlerForTests
  participant performFocus
  participant SettingsNavigationRequest
  participant NSWindow
  SettingsWindowRootView->>NSWindow: capture hosting window
  SettingsWindowRootView->>SettingsWindowPresenter: configure(window:)
  SettingsWindowPresenter->>focusHandlerForTests: focus(window) (if set)
  SettingsWindowPresenter->>performFocus: focus(window) (fallback)
  SettingsWindowPresenter->>SettingsNavigationRequest: post(...) (only if !NSWindow.isMiniaturized)
Loading

🎯 3 (Moderate) | ⏱️ ~20 minutes

🐰 I tuned the focus, made it bend and hop,
Wrapped the window so the view won't stop,
Tests keep watch so focus runs just once,
Mini windows wait — navigation paused in its bunce. 🥕

🚥 Pre-merge checks | ✅ 15 | ❌ 2

❌ Failed checks (1 warning, 1 inconclusive)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
Description check ❓ Inconclusive The PR description is well-structured with a summary of what changed and why, verification steps, dogfooding instructions, and references to testing. However, the required template sections (Testing, Demo Video, Review Trigger, and Checklist) are not present. Complete the required template sections: add explicit Testing section detailing test changes, include Demo Video (if applicable), add Review Trigger comment block, and complete the Checklist items.
✅ Passed checks (15 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately describes the primary change—reducing CPU usage from hidden Settings during Codex output—which aligns with the core purpose of unmounting the Settings view tree when miniaturized.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Cmux Swift Actor Isolation ✅ Passed SettingsDraftState is @MainActor @Observable (correct for UI store), WeakSettingsWindowReference is @MainActor, SettingsWindowPresenter is @MainActor enum. All proper isolation.
Cmux Swift Blocking Runtime ✅ Passed No blocking patterns found: async Task scheduling for focus (not blocking), settings state refactoring, and test-only Task.yield() in DEBUG code.
Cmux No Hacky Sleeps ✅ Passed All PR changes are Swift files (.swift); rule explicitly scopes to TypeScript, JavaScript, shell, and non-Swift build/runtime scripts. Swift code covered by separate 'swift-blocking-runtime' rule.
Cmux Swift Concurrency ✅ Passed PR uses @Observable (modern Swift), not Combine; .onReceive at SwiftUI boundary is allowed; no DispatchQueue or fire-and-forget Tasks added.
Cmux Swift @Concurrent ✅ Passed SettingsWindowPresenter @MainActor with synchronous functions; Task explicitly @MainActor. SettingsDraftState @MainActor with sync properties only. No nonisolated async or @concurrent violations.
Cmux Swift File And Package Boundaries ✅ Passed PR adds 40 net lines to oversized cmuxApp.swift (below 250-line threshold) with small UI glue code; no new oversized files, no mixed responsibilities, no misplaced feature logic.
Cmux Swift Logging ✅ Passed All logging in new production code complies with Swift logging rules: cmuxDebugLog is DEBUG-guarded only, no NSLog/print/debugPrint added, no Logger violations, and tests are appropriate.
Cmux User-Facing Error Privacy ✅ Passed PR introduces no new user-facing error messages or privacy violations; changes are structural/performance-focused with DEBUG-only test APIs.
Cmux Full Internationalization ✅ Passed PR reuses only pre-existing localized strings with full locale coverage; new debug-only focusHandlerForTests is behind #if DEBUG; no new unlocalized user-facing text added.
Cmux Swiftui State Layout ✅ Passed Uses @Observable pattern, proper @Bindable bindings, value snapshots in lists, and event-handler-only state mutations. Complies with swiftui-state-layout.md.
Cmux Architecture Rethink ✅ Passed Correctness fixes with clear invariants. Required platform bridges. No timing repairs, sleep, polling, locks, or split lifecycle ownership detected.
Cmux Swift Auxiliary Window Close Shortcuts ✅ Passed PR modifies existing "cmux.settings" window (already registered in cmuxAuxiliaryWindowIdentifiers); adds no new unregistered cmux.* identifiers; lint script passes with 24 identifiers checked.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch feat-settings-cpu-instrumentation

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Comment thread Sources/cmuxApp.swift

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@Sources/cmuxApp.swift`:
- Around line 637-638: The Settings tree is being torn down on minimize because
the code swaps SettingsWindowRootView (or SettingsRootView) with Color.clear,
which recreates transient UI state; instead always instantiate
SettingsWindowRootView and preserve its state by lifting transient state into a
retained model (e.g., SettingsViewModel : ObservableObject) that's created
outside the conditional and injected via EnvironmentObject or a singleton; then
gate only the expensive rendering inside SettingsWindowRootView (use conditional
child views, lazy loading, .opacity/hidden, or an internal if to skip heavy
subviews) so the root view and its model are not replaced when
cmuxAppearanceColorScheme changes. Ensure you reference and update
SettingsWindowRootView and any SettingsRootView usages to accept the retained
model and stop swapping the root view for Color.clear.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 21aca163-753f-474d-a4b1-7abaa69331be

📥 Commits

Reviewing files that changed from the base of the PR and between ddd647d and 21c4333.

📒 Files selected for processing (3)
  • Sources/App/SettingsWindowPresenter.swift
  • Sources/cmuxApp.swift
  • cmuxTests/SettingsWindowPresenterTests.swift

Comment thread Sources/cmuxApp.swift
@greptile-apps

greptile-apps Bot commented May 23, 2026 •

Copy link
Copy Markdown
Contributor

Greptile Summary

This PR fixes excessive CPU usage caused by the hidden Settings window continuing to render its full view tree while miniaturized during Codex output. It introduces SettingsWindowRootView, which unmounts SettingsRootView when the window is miniaturized or closing and remounts it on restore, backed by a shared @Observable SettingsDraftState that persists drafts and sidebar/search state across mount cycles.

  • SettingsWindowPresenter.configure(window:) now skips re-focusing on repeated calls for the same window, and show() defers navigation request posting when the existing window is miniaturized, clearing pending targets only after restore.
  • SettingsDraftState carries socketPasswordDraft, browserInsecureHTTPAllowlistDraft (with sync logic to avoid overwriting unsaved edits), settingsColumnVisibility, and settingsSearchText so these survive unmount/remount cycles.
  • New regression tests cover both the repeated-configure focus fix and the miniaturized-window deferred navigation path.

Confidence Score: 4/5

Safe to merge for the CPU fix, but the unmount/remount mechanism introduces a scroll-position regression that is reproducible on every miniaturize/restore cycle.

The core CPU optimization is correct and well-tested. However, SettingsView.onAppear always calls applySettingsNavigation → proxy.scrollTo(sectionID, anchor: .top) — a call that previously fired only once on initial window open. Now that SettingsView is unmounted and remounted on every miniaturize/restore, this call fires on every restore, silently discarding the user's scroll position within the selected section. The rest of the state-preservation machinery (drafts, search text, column visibility) works correctly.

Sources/cmuxApp.swift — specifically SettingsView.onAppear (lines 7805–7825) where applySettingsNavigation is called unconditionally without a guard for remount vs. fresh open.

Important Files Changed

Filename Overview
Sources/App/SettingsWindowPresenter.swift Adds shouldFocusAfterConfiguration guard to avoid re-focusing on repeated configure(window:) calls; defers navigation clearing when the existing window is miniaturized; focusHandlerForTests/setFocusHandlerForTests are correctly gated behind #if DEBUG.
Sources/cmuxApp.swift Introduces SettingsWindowRootView that unmounts SettingsRootView while miniaturized; adds SettingsDraftState (@Observable) to persist drafts and nav UI state across unmount cycles. Scroll position within the selected section resets on every restore because SettingsView.onAppear always calls applySettingsNavigation → proxy.scrollTo (not guarded for remounts). SettingsDraftState lacks fileprivate access control.
cmuxTests/SettingsWindowPresenterTests.swift Adds regression tests for repeated configure(window:) focus and deferred navigation when window is miniaturized; uses a TestSettingsWindow subclass to override isMiniaturized — clean, deterministic test scaffolding.

Sequence Diagram

sequenceDiagram
    participant User
    participant SwiftUI as SwiftUI Window Group
    participant SWRV as SettingsWindowRootView
    participant SRV as SettingsRootView
    participant SV as SettingsView
    participant SWP as SettingsWindowPresenter

    User->>SwiftUI: Open Settings
    SwiftUI->>SWRV: "Mount (shouldRenderSettingsContent=true)"
    SWRV->>SWP: configure(window:) via WindowAccessor
    SWP-->>SWRV: focus scheduled (Task)
    SWRV->>SRV: Mount
    SRV->>SWP: consumePendingNavigationTarget()
    SRV->>SV: Mount
    SV->>SV: onAppear → applySettingsNavigation (scroll to section top)

    User->>SwiftUI: Minimize window
    SwiftUI-->>SWRV: didMiniaturizeNotification
    SWRV->>SWRV: setContentVisibility(false)
    SWRV->>SRV: Unmount (SettingsRootView torn down)
    SRV->>SV: Unmount (scroll position lost)

    User->>SWP: show(navigationTarget:) while miniaturized
    SWP->>SWP: Keep pendingNavigationTarget, call focus → deminiaturize

    SwiftUI-->>SWRV: didDeminiaturizeNotification
    SWRV->>SWRV: setContentVisibility(true)
    SWRV->>SRV: Remount
    SRV->>SRV: onAppear → consumePendingNavigationTarget
    SRV->>SV: Remount
    SV->>SV: onAppear → applySettingsNavigation (scroll to section top ← resets position)
Loading

Reviews (6): Last reviewed commit: "Merge remote-tracking branch 'origin/mai..." | Re-trigger Greptile

Comment thread Sources/cmuxApp.swift
Comment on lines +8741 to +8786
@State private var shouldRenderSettingsContent = true

var body: some View {
Group {
if shouldRenderSettingsContent {
SettingsRootView()
} else {
Color.clear
.frame(
minWidth: SettingsWindowPresenter.minimumSize.width,
minHeight: SettingsWindowPresenter.minimumSize.height
)
}
}
.background(WindowAccessor { window in
self.window = window
SettingsWindowPresenter.configure(window: window)
setContentVisibility(!window.isMiniaturized)
})
.onReceive(NotificationCenter.default.publisher(for: NSWindow.didMiniaturizeNotification)) { notification in
guard notification.object as? NSWindow === window else { return }
setContentVisibility(false)
}
.onReceive(NotificationCenter.default.publisher(for: NSWindow.didDeminiaturizeNotification)) { notification in
guard notification.object as? NSWindow === window else { return }
setContentVisibility(true)
}
.onReceive(NotificationCenter.default.publisher(for: NSWindow.didBecomeKeyNotification)) { notification in
guard notification.object as? NSWindow === window else { return }
setContentVisibility(true)
}
.onReceive(NotificationCenter.default.publisher(for: NSWindow.didBecomeMainNotification)) { notification in
guard notification.object as? NSWindow === window else { return }
setContentVisibility(true)
}
.onReceive(NotificationCenter.default.publisher(for: NSWindow.willCloseNotification)) { notification in
guard notification.object as? NSWindow === window else { return }
setContentVisibility(false)
}
}

private func setContentVisibility(_ isVisible: Bool) {
guard shouldRenderSettingsContent != isVisible else { return }
shouldRenderSettingsContent = isVisible
}
}

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)

private static var pendingNavigationTarget: SettingsNavigationTarget?
private static var pendingContentNavigationTarget: SettingsNavigationTarget?
private static var shouldOpenWhenConfigured = false
private static var focusHandler: @MainActor (NSWindow) -> Void = { performFocus($0) }

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 New singleton side channel in production type

focusHandler is a mutable static closure that lives in the production portion of SettingsWindowPresenter (outside #if DEBUG), making arbitrary focus behaviour injectable at runtime even though the mutation API (setFocusHandlerForTests) is guarded. Per the project's architectural-rethink rule, introducing a new singleton side channel — even one whose default is always performFocus — leaves the production invariant implicit rather than enforced. Consider moving the property and its default assignment inside #if DEBUG as well, and calling performFocus directly in the production focus(_:) body.

Rule Used: Flag Swift fixes that patch symptoms while leaving... (source)

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!

Comment thread Sources/cmuxApp.swift Outdated
Comment thread Sources/cmuxApp.swift

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
Sources/cmuxApp.swift (1)

6065-6081: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Hide raw storage errors from the Settings UI.

Line 6079 and Line 6091 interpolate error.localizedDescription directly into user-facing status copy. That can leak Keychain/OS implementation details. Show a generic localized failure message here and keep raw error text out of the UI.

As per coding guidelines, "user-facing errors, alerts, command output, API error bodies, or recovery copy must not expose ... raw upstream messages."

Also applies to: 6084-6092

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@Sources/cmuxApp.swift` around lines 6065 - 6081, In saveSocketPassword, do
not interpolate error.localizedDescription into the user-facing
socketPasswordStatusMessage; instead set socketPasswordStatusMessage to a
generic localized failure string (e.g.,
"settings.automation.socketPassword.saveFailed") and socketPasswordStatusIsError
= true, and record the raw error details only to a non-UI log (e.g., use os_log
or your existing logging utility) referencing SocketControlPasswordStore and the
caught error; ensure draftState.socketPasswordDraft handling remains unchanged.
♻️ Duplicate comments (1)
Sources/cmuxApp.swift (1)

7731-7738: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Only initialize the retained HTTP allowlist draft once.

Line 7737 repopulates draftState.browserInsecureHTTPAllowlistDraft on every onAppear. Because SettingsWindowRootView tears down SettingsView while the window is miniaturized, restoring the window runs this again and discards unsaved allowlist edits, so the minimize-preservation regression still exists for this field.

🩹 Proposed fix
 `@MainActor`
 `@Observable`
 final class SettingsDraftState {
     var browserInsecureHTTPAllowlistDraft = BrowserInsecureHTTPSettings.defaultAllowlistText
+    var didInitializeBrowserInsecureHTTPAllowlistDraft = false
     var socketPasswordDraft = ""
 }
         .onAppear {
             notificationStore.refreshAuthorizationStatus()
             browserThemeMode = BrowserThemeSettings.mode(defaults: .standard).rawValue
             browserImportHintVariantRaw = BrowserImportHintSettings.variant(for: browserImportHintVariantRaw).rawValue
             didLoadBrowserHistoryForSettings = BrowserHistoryStore.shared.isLoaded
             browserHistoryEntryCount = didLoadBrowserHistoryForSettings ? BrowserHistoryStore.shared.entries.count : 0
-            draftState.browserInsecureHTTPAllowlistDraft = browserInsecureHTTPAllowlist
+            if !draftState.didInitializeBrowserInsecureHTTPAllowlistDraft {
+                draftState.browserInsecureHTTPAllowlistDraft = browserInsecureHTTPAllowlist
+                draftState.didInitializeBrowserInsecureHTTPAllowlistDraft = true
+            }
             reloadWorkspaceTabColorSettings()
             refreshNotificationCustomSoundStatus()
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@Sources/cmuxApp.swift` around lines 7731 - 7738, The code in onAppear
currently overwrites draftState.browserInsecureHTTPAllowlistDraft every time
(using browserInsecureHTTPAllowlist), which discards unsaved edits when
SettingsView is torn down/recreated; change the assignment so it only
initializes the draft once — e.g., in the onAppear block check if
draftState.browserInsecureHTTPAllowlistDraft is nil or empty (or otherwise
uninitialized/not-dirty) and only then set it from browserInsecureHTTPAllowlist;
keep all other onAppear steps (notificationStore.refreshAuthorizationStatus,
browserThemeMode, etc.) unchanged and reference
draftState.browserInsecureHTTPAllowlistDraft and browserInsecureHTTPAllowlist
when applying the guard.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Outside diff comments:
In `@Sources/cmuxApp.swift`:
- Around line 6065-6081: In saveSocketPassword, do not interpolate
error.localizedDescription into the user-facing socketPasswordStatusMessage;
instead set socketPasswordStatusMessage to a generic localized failure string
(e.g., "settings.automation.socketPassword.saveFailed") and
socketPasswordStatusIsError = true, and record the raw error details only to a
non-UI log (e.g., use os_log or your existing logging utility) referencing
SocketControlPasswordStore and the caught error; ensure
draftState.socketPasswordDraft handling remains unchanged.

---

Duplicate comments:
In `@Sources/cmuxApp.swift`:
- Around line 7731-7738: The code in onAppear currently overwrites
draftState.browserInsecureHTTPAllowlistDraft every time (using
browserInsecureHTTPAllowlist), which discards unsaved edits when SettingsView is
torn down/recreated; change the assignment so it only initializes the draft once
— e.g., in the onAppear block check if
draftState.browserInsecureHTTPAllowlistDraft is nil or empty (or otherwise
uninitialized/not-dirty) and only then set it from browserInsecureHTTPAllowlist;
keep all other onAppear steps (notificationStore.refreshAuthorizationStatus,
browserThemeMode, etc.) unchanged and reference
draftState.browserInsecureHTTPAllowlistDraft and browserInsecureHTTPAllowlist
when applying the guard.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 2186a383-c0f4-449f-a37c-2a360a644304

📥 Commits

Reviewing files that changed from the base of the PR and between 9d6f660 and 6a337a7.

📒 Files selected for processing (1)
  • Sources/cmuxApp.swift

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (2)
Sources/cmuxApp.swift (2)

8745-8776: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Restore the selected settings anchor after remount.

Tearing down the whole SettingsRootView here means its existing restore path runs again on every minimize/restore, but that path only restores the section-level target. A user who had a specific search result or deep setting selected gets bounced back to the section root, so navigation state still is not actually preserved across the new unmount/remount cycle.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@Sources/cmuxApp.swift` around lines 8745 - 8776, The current logic tears down
SettingsRootView on minimize by toggling shouldRenderSettingsContent, which
causes only section-level restoration and loses deep selection; instead preserve
and restore the exact selected anchor by either (A) keeping SettingsRootView
mounted always and merely hide its visual content (replace the Color.clear
remount strategy with a hidden/opacity/visibility toggle) or (B) if you must
unmount, capture the current deep selection from draftState (e.g. a
selectedAnchor / selection path on the DraftState used by SettingsRootView)
before setContentVisibility(false) and reapply that exact selection to
draftState when setContentVisibility(true) runs (hooks handling
NSWindow.didDeminiaturizeNotification / didBecomeKeyNotification /
didBecomeMainNotification and where SettingsWindowPresenter.configure is
called). Ensure you reference shouldRenderSettingsContent, SettingsRootView,
draftState, setContentVisibility, and SettingsWindowPresenter.configure when
implementing the change so the deep-selection is preserved across
minimize/remount cycles.

8777-8780: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Clear socketPasswordDraft when Settings is explicitly closed (Cmd+W).

  • In Sources/cmuxApp.swift, the NSWindow.willCloseNotification handler for the Settings window only calls setContentVisibility(false) and does not reset draftState.socketPasswordDraft (and draftState is stored as @State in SettingsWindowRootView).
  • Result: an unsaved socket password draft can persist and reappear when Settings is reopened; clear sensitive drafts in this close handler.
.onReceive(NotificationCenter.default.publisher(for: NSWindow.willCloseNotification)) { notification in
    guard notification.object as? NSWindow === window else { return }
    setContentVisibility(false)
}
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@Sources/cmuxApp.swift` around lines 8777 - 8780, The
NSWindow.willCloseNotification handler only calls setContentVisibility(false)
and must also clear the Settings view's draft state to avoid leaking an unsaved
socket password; update the handler in Sources/cmuxApp.swift (the closure that
checks notification.object as? NSWindow === window) to reset
draftState.socketPasswordDraft (and any other sensitive draft fields on the
`@State-held` SettingsWindowRootView) when the window is closed so the draft does
not persist when Settings is reopened.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Outside diff comments:
In `@Sources/cmuxApp.swift`:
- Around line 8745-8776: The current logic tears down SettingsRootView on
minimize by toggling shouldRenderSettingsContent, which causes only
section-level restoration and loses deep selection; instead preserve and restore
the exact selected anchor by either (A) keeping SettingsRootView mounted always
and merely hide its visual content (replace the Color.clear remount strategy
with a hidden/opacity/visibility toggle) or (B) if you must unmount, capture the
current deep selection from draftState (e.g. a selectedAnchor / selection path
on the DraftState used by SettingsRootView) before setContentVisibility(false)
and reapply that exact selection to draftState when setContentVisibility(true)
runs (hooks handling NSWindow.didDeminiaturizeNotification /
didBecomeKeyNotification / didBecomeMainNotification and where
SettingsWindowPresenter.configure is called). Ensure you reference
shouldRenderSettingsContent, SettingsRootView, draftState, setContentVisibility,
and SettingsWindowPresenter.configure when implementing the change so the
deep-selection is preserved across minimize/remount cycles.
- Around line 8777-8780: The NSWindow.willCloseNotification handler only calls
setContentVisibility(false) and must also clear the Settings view's draft state
to avoid leaking an unsaved socket password; update the handler in
Sources/cmuxApp.swift (the closure that checks notification.object as? NSWindow
=== window) to reset draftState.socketPasswordDraft (and any other sensitive
draft fields on the `@State-held` SettingsWindowRootView) when the window is
closed so the draft does not persist when Settings is reopened.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: bd5808d5-eef9-43e8-802a-1d450d1a87da

📥 Commits

Reviewing files that changed from the base of the PR and between 6a337a7 and 18d8f96.

📒 Files selected for processing (2)
  • Sources/App/SettingsWindowPresenter.swift
  • Sources/cmuxApp.swift

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Cursor Bugbot has reviewed your changes and found 1 potential issue.

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit f184ec6. Configure here.

Comment thread Sources/cmuxApp.swift
.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.

@lawrencecchen

Copy link
Copy Markdown
Contributor Author

@coderabbitai review

Current head is 24741af. The old state-loss findings were addressed by retaining SettingsDraftState outside the gated Settings content, preserving search/column/drafts, and moving the test focus hook fully behind DEBUG.

@coderabbitai

coderabbitai Bot commented May 23, 2026

Copy link
Copy Markdown

@lawrencecchen Sure, I'll review the changes at the current head (24741af). Thanks for the context on the state-loss fixes and the DEBUG-gating of the test focus hook!

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@lawrencecchen
lawrencecchen merged commit 28b0a78 into main May 25, 2026
25 of 28 checks passed
@lawrencecchen
lawrencecchen deleted the feat-settings-cpu-instrumentation branch May 25, 2026 11:41
hhsw2015 pushed a commit to hhsw2015/cmux that referenced this pull request May 29, 2026
Upstream PR manaflow-ai#4661 added shouldRenderSettingsContent toggle to skip
SettingsRootView render while window is hidden/closed. On macOS 26 SwiftUI
no longer fires didBecomeKey/didBecomeMain consistently when an existing
NSWindow is brought back via SettingsWindowPresenter.show, so the
willCloseNotification's setContentVisibility(false) sticks and the
reopened window renders Color.clear.

Re-asserting setContentVisibility(true) inside the WindowAccessor closure
runs every time the SwiftUI Window backs onto an NSWindow, which is the
single point we can rely on across macOS 26 reopen cycles.
hhsw2015 pushed a commit to hhsw2015/cmux that referenced this pull request May 29, 2026
settings: drop shouldRenderSettingsContent gate. Upstream PR manaflow-ai#4661 only
toggles it via didBecomeKey/willClose notifications, but on macOS 26
SwiftUI Windows reuse the same NSWindow on reopen and the
didBecomeKey is not re-delivered, leaving the gate stuck at false on
the second open. Always render the SettingsRootView body — the CPU
regression that motivated the gate (Codex output spam while Settings
is hidden) is rare enough not to matter.

top tab hover: expand the trigger zone from 12px to 40px while hovered
so the pointer can move horizontally across the revealed tab bar
without exiting the hot zone and triggering a flicker hide. Resting
size stays at 12px so it doesn't capture clicks meant for the layout
below.
hhsw2015 pushed a commit to hhsw2015/cmux that referenced this pull request May 29, 2026
Restore PR manaflow-ai#4661's shouldRenderSettingsContent gate (preserves the CPU
optimization for Settings hidden behind Codex spam) but add two
recovery paths for macOS 26's NSWindow-reuse reopen flow:

1. WindowAccessor closure forces visibility=true. The SwiftUI Window's
   body remounts on reopen, so this reliably re-fires.
2. .onAppear forces visibility=true. Belt-and-suspenders for cases
   where only the parent view remounts.

didBecomeKey/Main/willClose notifications still drive the gate during
normal use; the new paths only matter on the broken reopen edge.

This branch was successfully deployed

1 active deployment
Preview – cmux — 4e8cdc09 Deployed May 25, 2026 by vercel[bot]
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant