Repository navigation
Fix Settings panel open latency - #3389
austinywang wants to merge 2 commits into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Note Reviews pausedIt 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 Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughSettings UI is made section-aware end-to-end: the root passes a selected section into the detail view, refresh and reactive updates are gated to visible sections, browser detection is moved to a cancellable async task with generation tracking, insecure-allowlist draft lifecycle is tracked, new picker rows and debug/test helpers added. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Client as Client (cmux test / Presenter)
participant AppRoot as App Root\n(`cmuxApp.swift`)
participant SettingsView as SettingsView\n(detail)
participant Refresher as RefreshManager\n(refreshVisibleSettingsSection)
participant BrowserTask as BrowserDetectTask\n(cancellable Task)
Client->>AppRoot: request open / navigation target
AppRoot->>AppRoot: store pending NavigationTarget
AppRoot->>SettingsView: supply `selectedSection`
SettingsView->>Refresher: refreshVisibleSettingsSection(selectedSection)
Refresher->>Refresher: run only relevant updates
Refresher->>BrowserTask: start cancellable detection (gen N)
BrowserTask-->>Refresher: return result (only apply if gen == current)
Refresher->>SettingsView: update visible-section state
Client->>AppRoot: (debug) query `debug.settings.window_state`
AppRoot->>AppRoot: enumerate NSApp.windows, return window flags
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
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)
8378-8385:⚠️ Potential issue | 🟠 Major | ⚡ Quick winApply the pending Settings target before the first detail render.
consumePendingNavigationTarget()only runs in.onAppear, so the initial body still uses the persisted/defaultselectedSectionRaw. If the last-opened pane was something heavy like Keyboard Shortcuts, a deep-link open will still build that pane before switching, which partially reintroduces the eager work this PR is trying to remove.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/cmuxApp.swift` around lines 8378 - 8385, The initial view build still uses persisted selectedSectionRaw because consumePendingNavigationTarget() is only invoked in .onAppear; fix by applying the pending target before the first render—call SettingsWindowPresenter.consumePendingNavigationTarget() during the view's initialization (or when initializing selectedSection) and, if present, set selectedSection and/or call navigate(to: target, postRequest: true) there rather than waiting for .onAppear; update/remove the duplicate .onAppear logic so symbols involved are SettingsWindowPresenter.consumePendingNavigationTarget(), selectedSection/selectedSectionRaw, and navigate(to:postRequest:) to ensure the pending target is applied prior to the body being built.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Sources/cmuxApp.swift`:
- Around line 5855-5862: The browser-detection call is running synchronously on
the main thread via refreshDetectedImportBrowsers() which invokes
InstalledBrowserDetector.detectInstalledBrowsers(); change
refreshDetectedImportBrowsers() (or the code path inside the
.browser/.browserImport case) to run the heavy detectInstalledBrowsers() work
off the main actor (e.g., in a detached Task or background Task) and then
publish the results back on the main actor using MainActor.run (or Task {
`@MainActor` in ... }) to update UI-bound state like browserImportHintVariantRaw,
browserHistoryEntryCount, and any properties set by
refreshDetectedImportBrowsers(); ensure
InstalledBrowserDetector.detectInstalledBrowsers() is called only from the
background task and not directly on the main thread.
In `@tests_v2/test_settings_open_latency.py`:
- Around line 94-97: The precondition only rejects already-visible Settings
windows but must also reject existing but minimized/off-screen windows to avoid
warm-path refocus; update the check before calling
_measure_visible_settings_open(c, pid) to detect any existing Settings window
(not just visible) — either extend _settings_window_visible to check for
existence/state (minimized/hidden) or add a helper like
_settings_window_exists(pid) and raise cmuxError when it returns true so the
test always measures a true first-present open.
---
Outside diff comments:
In `@Sources/cmuxApp.swift`:
- Around line 8378-8385: The initial view build still uses persisted
selectedSectionRaw because consumePendingNavigationTarget() is only invoked in
.onAppear; fix by applying the pending target before the first render—call
SettingsWindowPresenter.consumePendingNavigationTarget() during the view's
initialization (or when initializing selectedSection) and, if present, set
selectedSection and/or call navigate(to: target, postRequest: true) there rather
than waiting for .onAppear; update/remove the duplicate .onAppear logic so
symbols involved are SettingsWindowPresenter.consumePendingNavigationTarget(),
selectedSection/selectedSectionRaw, and navigate(to:postRequest:) to ensure the
pending target is applied prior to the body being built.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 97eb9135-d8cc-4d37-b73e-543ffc293334
📒 Files selected for processing (3)
Sources/cmuxApp.swiftdocs/issue-3384-settings-open-pr-body.mdtests_v2/test_settings_open_latency.py
Greptile SummaryThis PR eliminates the Settings open latency (2962 ms → 560 ms cold) by wrapping every Settings pane in an The Swift-side logic is sound: Confidence Score: 4/5Safe to merge; the performance fix is correct and the only findings are P2 test/docs hygiene issues. All findings are P2 (private method usage in test, missing teardown, stale docs file). The core SwiftUI change is logically consistent with the existing navigation contract and the measurements confirm the fix works. No P0 or P1 issues found. tests_v2/test_settings_open_latency.py — private Important Files Changed
Sequence DiagramsequenceDiagram
participant User
participant SettingsRootView
participant SettingsView
User->>SettingsRootView: click sidebar entry (e.g. Keyboard Shortcuts)
SettingsRootView->>SettingsRootView: selectedSectionRaw = .keyboardShortcuts
SettingsRootView->>SettingsView: SettingsNavigationRequest (notification)
Note over SettingsView: onReceive fires synchronously
SettingsView->>SettingsView: DispatchQueue.main.async { proxy.scrollTo(...) }
SettingsRootView->>SettingsView: re-render with selectedSection = .keyboardShortcuts
Note over SettingsView: only .keyboardShortcuts pane constructed
SettingsView->>SettingsView: onChange(of: selectedSection) → refreshVisibleSettingsSection()
Note over SettingsView: async scroll fires — anchors now registered
Reviews (1): Last reviewed commit: "Stop Settings from constructing inactive..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
|
|
||
|
|
||
| def _open_settings(c: cmux) -> None: | ||
| c._call("settings.open", {"activate": True}) |
There was a problem hiding this comment.
c._call(...) uses the Python name-mangled private method directly instead of the cmux public API surface. If the cmux client's internal dispatch signature changes, this test will break without an obvious API-contract signal. Prefer whatever public method (e.g. c.call(...) or c.request(...)) the cmux class exposes for sending socket commands.
| with cmux(SOCKET_PATH) as c: | ||
| if _settings_window_visible(pid): | ||
| raise cmuxError("Settings window was already visible before the latency measurement") | ||
|
|
||
| first_open_ms = _measure_visible_settings_open(c, pid) |
There was a problem hiding this comment.
After _measure_visible_settings_open succeeds the Settings window stays visible. On a subsequent run (retry, local re-run, parallel CI job on the same machine) the pre-check on line 94 raises cmuxError("Settings window was already visible") with no guidance on how to recover. Consider closing the window — e.g. via c._call("settings.close", {}) or a focus-away command — in a finally block or a teardown step so the fixture state is clean for retries.
| Fixes https://github.com/manaflow-ai/cmux/issues/3384. | ||
|
|
||
| ## Measurements | ||
|
|
||
| Baseline DEV build, clean profile: | ||
| - Cold Settings open: 2962 ms | ||
| - Warm Settings reopen: 873 ms | ||
|
|
||
| Instrumented DEV build, clean profile: | ||
| - Cold Settings open: 3147 ms to visible window, 3306 ms to first paint probe | ||
| - Warm Settings reopen: 754 ms | ||
|
|
||
| Instrumented DEV build, heavier profile with 6 workspaces and 3 browser panes: | ||
| - Warm Settings reopen: 1077 ms | ||
|
|
||
| Fixed DEV build: | ||
| - Cold Settings open: 560 ms | ||
| - Warm Settings refocus/reopen with the Settings window resident: 8-12 ms | ||
|
|
||
| ## Root Cause | ||
|
|
||
| Settings opens construct one giant SwiftUI detail view for every pane even when the selected pane is Account. The first open shows all 11 pane headers appearing at first presentation, eager Browser import detection on the main thread, repeated workspace palette refreshes, and hundreds of keyboard shortcut recorder rows initialized around first paint. `openWindow()` itself blocks the main actor for about 1283 ms cold and 371-408 ms warm, so this is main-thread view construction and synchronous pane setup rather than off-main work gating paint. | ||
|
|
||
| ## Suspect List | ||
|
|
||
| Confirmed: | ||
| - Eager synchronous SwiftUI tree construction for all Settings panes | ||
| - Eager keyboard shortcut row/recorder construction | ||
| - Eager Browser import disk scan from `InstalledBrowserDetector.detectInstalledBrowsers()` | ||
| - Eager browser history load, though small in this profile | ||
|
|
||
| Ruled out or not implicated by code/logs: | ||
| - `ghostty +list-themes` | ||
| - font enumeration | ||
| - Localizable.xcstrings runtime scan | ||
| - eager Convex/agent profile fetch | ||
| - image asset decode | ||
| - unified config settings utility window (#3024), which uses a separate window | ||
| - recent AppDelegate bootstrap reorder side effects, which do not appear on the Settings open path | ||
|
|
||
| ## Fix Plan | ||
|
|
||
| Make the Settings detail construct only the currently selected top-level pane, keep search/navigation selecting the target pane before scrolling, and move pane-specific `onAppear` work into the pane that needs it. In particular, Browser history/import detection should run only for Browser/Browser Import, workspace color palette reloads only for Workspace Colors, notification status work only for App, and keyboard shortcut rows only for Keyboard Shortcuts. This avoids both the main-thread first-present stall and the warm reopen penalty without adding sleeps or new dependencies. | ||
|
|
||
| ## Regression Budget | ||
|
|
||
| Add a `tests_v2` regression that opens Settings through the app/socket and asserts first present to a visible Settings window stays at or below 1000 ms. Main measured about 2962 ms on the same path, while the fixed path measured about 560 ms. |
There was a problem hiding this comment.
This file is a byte-for-byte copy of the PR description. Checking it into docs/ means it will become stale as follow-up work lands, and readers may mistake it for authoritative documentation rather than a point-in-time write-up. The investigation notes are useful but they belong in the GitHub issue/PR itself, not in the repository's docs tree. Consider removing this file before merging.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Issue 3384 reproduces as a multi-second Settings first-show pause, so the regression opens Settings through the socket and waits until the app can capture the presented Settings window. The test uses the existing debug screenshot command plus standard-library PNG header parsing so tests_v2 does not gain a PyObjC dependency. Constraint: Local test runs are disabled for this repo; CI owns execution. Constraint: tests_v2 runs with plain python3 and cannot assume PyObjC/Quartz is installed. Rejected: Source-text assertions for lazy Settings panes | repo policy requires behavioral tests over implementation-shape checks Confidence: medium Scope-risk: narrow Tested: Not run locally by instruction. Not-tested: CI execution of tests_v2/test_settings_open_latency.py.
2008f6d to
98fd07b
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
tests_v2/test_settings_open_latency.py (1)
75-79:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPrecondition still permits warm-path reuse instead of true first-present measurement.
At Line 76, this only rejects a window that already looks like Settings by size. It does not reject an existing Settings window that is minimized/hidden/off-screen, so
settings.opencan take a fast reuse path and understate first-present latency.Based on learnings:
SettingsWindowPresenter.show(navigationTarget:)reuses an existing SettingsNSWindow(including deminiaturize/refocus) instead of always creating a new one.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests_v2/test_settings_open_latency.py` around lines 75 - 79, The precondition only checks size via _capture_main_window_size and _looks_like_settings_size so it can miss an existing Settings NSWindow that is minimized/hidden and let SettingsWindowPresenter.show(navigationTarget:) reuse it; update the test to ensure a true first-present measurement by detecting and forcing teardown of any existing Settings window before timing: use a stronger check (e.g., enumerating app windows by title/role/accessibility identifier or a helper that queries the Settings window instance) and if found, close or destroy it (or call the presenter to force-create a new instance) so the test cannot take the reuse path; change references to _capture_main_window_size/_looks_like_settings_size to this stronger detection and teardown step.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests_v2/test_settings_open_latency.py`:
- Around line 64-68: _measure_settings_first_capture currently calls
c.open_settings(activate=True) then immediately takes a capture via
_capture_main_window_size, which is racy because open_settings schedules work on
the main queue and may not have presented the Settings window yet; modify
_measure_settings_first_capture to poll (with a short sleep and deadline
timeout) after calling c.open_settings until _capture_main_window_size returns a
capture whose size matches the expected Settings window size (or until a timeout
elapses), then measure the elapsed time from start to that successful capture;
use the existing function names open_settings and _capture_main_window_size and
ensure the polling loop breaks on success or timeout to avoid hanging.
---
Duplicate comments:
In `@tests_v2/test_settings_open_latency.py`:
- Around line 75-79: The precondition only checks size via
_capture_main_window_size and _looks_like_settings_size so it can miss an
existing Settings NSWindow that is minimized/hidden and let
SettingsWindowPresenter.show(navigationTarget:) reuse it; update the test to
ensure a true first-present measurement by detecting and forcing teardown of any
existing Settings window before timing: use a stronger check (e.g., enumerating
app windows by title/role/accessibility identifier or a helper that queries the
Settings window instance) and if found, close or destroy it (or call the
presenter to force-create a new instance) so the test cannot take the reuse
path; change references to _capture_main_window_size/_looks_like_settings_size
to this stronger detection and teardown step.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: c10aa315-0c79-47bc-8c37-e338f851d01c
📒 Files selected for processing (3)
Sources/cmuxApp.swifttests_v2/cmux.pytests_v2/test_settings_open_latency.py
✅ Files skipped from review due to trivial changes (2)
- tests_v2/cmux.py
- Sources/cmuxApp.swift
6928a15 to
98a9232
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Sources/cmuxApp.swift`:
- Around line 5872-5877: The current
refreshBrowserInsecureHTTPAllowlistDraftIfClean relies on comparing
browserInsecureHTTPAllowlistDraft to browserInsecureHTTPAllowlist to infer
edits, which can leave a stale draft marked dirty after external/defaults
changes; introduce an explicit Bool (e.g.
browserInsecureHTTPAllowlistDraftIsDirty) and use it instead of equality to
track user edits: in refreshBrowserInsecureHTTPAllowlistDraftIfClean set
browserInsecureHTTPAllowlistDraft = browserInsecureHTTPAllowlist and set
browserInsecureHTTPAllowlistDraftIsDirty = false when initializing/overwriting
the draft; flip browserInsecureHTTPAllowlistDraftIsDirty = true only from the
editor write handler(s) that mutate the draft; update any logic that currently
checks draft != stored (including save/enablement code around Browser UI) to
check browserInsecureHTTPAllowlistDraftIsDirty so external changes won’t be
overwritten by a stale “dirty” draft.
- Around line 7655-7670: refreshDetectedImportBrowsers currently cancels only
the outer Task (detectedImportBrowserRefreshTask) but the heavy work runs in an
unstructured Task.detached (InstalledBrowserDetector.detectInstalledBrowsers())
so rapid calls can spawn multiple expensive scans; fix by making the detection
cancellable: either (preferred) remove Task.detached and call
InstalledBrowserDetector.detectInstalledBrowsers() directly inside the wrapper
Task so cancellation flows (keep using detectedImportBrowserRefreshTask and
detectedImportBrowserRefreshGeneration/generation checks), or if you must keep a
detached task, store that detached Task in a new property (e.g.
detectedImportBrowserDetectionTask), cancel it before creating a new one, and
await its .value instead of creating an unreferenced detached task; ensure you
still clear detectedImportBrowserRefreshTask and
detectedImportBrowserDetectionTask when done and respect Task.isCancelled and
the generation guard before mutating detectedImportBrowsers.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: a9396d53-cf93-4766-8d1d-9b31451776b3
📒 Files selected for processing (3)
GhosttyTabs.xcodeproj/project.pbxprojSources/Settings/SettingsPickerRows.swiftSources/cmuxApp.swift
✅ Files skipped from review due to trivial changes (1)
- GhosttyTabs.xcodeproj/project.pbxproj
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
There are 2 total unresolved issues (including 1 from previous review).
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 98a9232. Configure here.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Sources/cmuxApp.swift`:
- Around line 8083-8086: Set the initial settings pane from the pending
navigation target before the view body evaluates by calling
SettingsWindowPresenter.consumePendingNavigationTarget() and, if non-nil,
assigning it to selectedSection (instead of waiting for .onAppear); leave the
existing .onAppear handler to only perform the follow-up navigate(to: ,
postRequest: true) scroll/highlight behavior. Specifically, locate the code
using consumePendingNavigationTarget(), selectedSection/selectedSectionRaw, and
navigate(to:postRequest:), move/duplicate the pending-target assignment to the
view initialization (or immediately before body evaluation) so the first render
uses that target, and ensure .onAppear no longer overrides selectedSection but
only calls navigate(to: target, postRequest: true).
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 34fbed62-686a-4f6e-a697-b7cdc531167e
📒 Files selected for processing (4)
GhosttyTabs.xcodeproj/project.pbxprojSources/Settings/SettingsPickerRows.swiftSources/cmuxApp.swifttests_v2/test_settings_open_latency.py
|
Updated after CI review: fixed the Swift file-length guard by moving Settings picker primitives out of cmuxApp.swift, addressed the relevant Settings reopen/deep-link and latency-test race comments, and confirmed the rerun is green including workflow-guard-tests, tests, release-build, ui-regressions, compat-tests, Vercel, and CodeRabbit. |
98a9232 to
08d6b18
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
Sources/cmuxApp.swift (2)
7673-7688:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAvoid
Task.detachedhere unless you also cancel it explicitly.
detectedImportBrowserRefreshTask?.cancel()only cancels the wrapper task. The expensive scan is still running in a detached task, so rapid refreshes can stack multiple full browser-detection passes in the background.💡 Keep a handle to the detached scan so cancellation actually stops the work
+ `@State` private var detectedImportBrowserDetectionTask: Task<[InstalledBrowserCandidate], Never>? @@ private func refreshDetectedImportBrowsers() { detectedImportBrowserRefreshTask?.cancel() + detectedImportBrowserDetectionTask?.cancel() detectedImportBrowserRefreshGeneration &+= 1 let generation = detectedImportBrowserRefreshGeneration - detectedImportBrowserRefreshTask = Task { - let browsers = await Task.detached(priority: .utility) { - InstalledBrowserDetector.detectInstalledBrowsers() - }.value + let detectionTask = Task.detached(priority: .utility) { + InstalledBrowserDetector.detectInstalledBrowsers() + } + detectedImportBrowserDetectionTask = detectionTask + detectedImportBrowserRefreshTask = Task { + let browsers = await detectionTask.value guard !Task.isCancelled else { return } await MainActor.run { guard detectedImportBrowserRefreshGeneration == generation else { return } detectedImportBrowsers = browsers detectedImportBrowserRefreshTask = nil + detectedImportBrowserDetectionTask = nil } } }Does `Task.detached` inherit cancellation from its parent `Task` in Swift concurrency?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/cmuxApp.swift` around lines 7673 - 7688, The current code uses Task.detached inside refreshDetectedImportBrowsers so cancelling detectedImportBrowserRefreshTask does not cancel the expensive InstalledBrowserDetector.detectInstalledBrowsers() scan; fix by making the scan cancellable: either run the scan in the same parent Task (replace Task.detached with Task { InstalledBrowserDetector.detectInstalledBrowsers() }) so it inherits cancellation from detectedImportBrowserRefreshTask, or store the detached task in a separate property (e.g., detectedImportBrowserScanTask) and call .cancel() on it when starting a new refresh; ensure the code checks Task.isCancelled appropriately and clears both detectedImportBrowserRefreshTask and the scan task when finished, while still using detectedImportBrowserRefreshGeneration and generation guard as currently implemented.
8093-8105:⚠️ Potential issue | 🟠 Major | ⚡ Quick winSeed the pending Settings target before the first detail render.
Line 8093 still renders
SettingsViewfrom the persistedselectedSectionRaw, and Line 8103 only swaps to the pending target in.onAppear. If the stored pane is something heavy like Keyboard Shortcuts, a deep-link open still pays for that first render before the navigation target wins.💡 One way to apply the pending target up front
private struct SettingsRootView: View { `@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 = "" + `@State` private var pendingInitialTarget = SettingsWindowPresenter.consumePendingNavigationTarget() private var selectedSection: SettingsNavigationTarget { - SettingsNavigationTarget(rawValue: selectedSectionRaw) ?? .account + pendingInitialTarget + ?? SettingsNavigationTarget(rawValue: selectedSectionRaw) + ?? .account } @@ .onAppear { searchText = "" - let target = SettingsWindowPresenter.consumePendingNavigationTarget() ?? selectedSection + let target = pendingInitialTarget ?? selectedSection + pendingInitialTarget = nil navigate(to: target, postRequest: true) }Does SwiftUI `.onAppear` run only after the initial `body` evaluation/render, or can it change the first render pass?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/cmuxApp.swift` around lines 8093 - 8105, The persisted selected section is still used for the first detail render; pre-seed the pending navigation target before the initial body evaluation by consuming SettingsWindowPresenter.consumePendingNavigationTarget() and applying it to the view's selection state (e.g., selectedSection or selectedSectionRaw) during view initialization (or immediately when the view's state is created) instead of waiting for .onAppear; then keep the existing navigate(to:postRequest:) call for post-appearance behavior so the initial render uses the pending target (references: SettingsView, selectedSection/selectedSectionRaw, SettingsWindowPresenter.consumePendingNavigationTarget(), navigate(to:postRequest:), .onAppear).
🧹 Nitpick comments (1)
Sources/Settings/SettingsPickerRows.swift (1)
13-27: ⚡ Quick winFail fast when a settings JSON path has no search-anchor mapping.
Line 15 currently drops unmapped paths via
compactMap, andvalidate()only checkssupportedSettingsJSONPaths. That can silently break jump-to/highlight wiring for a valid settings path.Suggested patch
func validate(file: StaticString = `#fileID`, line: UInt = `#line`) { guard case .settingsFile(let paths) = self else { return } let unknownPaths = paths.filter { !CmuxSettingsFileStore.supportedSettingsJSONPaths.contains($0) } + let missingAnchorPaths = paths.filter { SettingsSearchIndex.anchorID(forSettingsPath: $0) == nil } precondition( unknownPaths.isEmpty, "Unknown settings.json path(s): \(unknownPaths.joined(separator: ", "))", file: file, line: line ) + precondition( + missingAnchorPaths.isEmpty, + "Missing SettingsSearchIndex anchor(s) for path(s): \(missingAnchorPaths.joined(separator: ", "))", + file: file, + line: line + ) }Based on learnings: “don’t wire navigation/search with a raw anchor string alone… add/update the matching entry in SettingsSearchIndex… This prevents broken jump-to behavior by ensuring the navigation anchor and the search index stay consistent.”
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/Settings/SettingsPickerRows.swift` around lines 13 - 27, The code currently drops unmapped settings paths in searchAnchorIDs via compactMap, silently losing anchors; change searchAnchorIDs to detect and fail fast when SettingsSearchIndex.anchorID(forSettingsPath:) returns nil (e.g., map the paths to optionals, collect any nil-mapped paths and call precondition/fatalError with a clear message listing those paths), and update validate() (which now only checks CmuxSettingsFileStore.supportedSettingsJSONPaths) to also assert that every path has a corresponding SettingsSearchIndex.anchorID(forSettingsPath:) entry so missing search-anchor mappings are caught at runtime; reference searchAnchorIDs, SettingsSearchIndex.anchorID(forSettingsPath:), and validate() to locate the changes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests_v2/test_settings_open_latency.py`:
- Line 22: Update SOCKET_PATH in tests_v2/test_settings_open_latency.py to avoid
the hardcoded "/tmp" fallback: first read os.environ.get("CMUX_SOCKET_PATH"),
and if unset, perform dynamic discovery of the cmux socket (e.g., call an
existing helper like get_cmux_socket_path() or implement a
discover_cmux_socket() that checks the platform-specific App Support socket
locations and returns the found path or raises an error); assign that discovered
path to SOCKET_PATH instead of defaulting to "/tmp/cmux-debug.sock" so tests use
the actual cmux socket when CMUX_SOCKET_PATH is not exported.
---
Duplicate comments:
In `@Sources/cmuxApp.swift`:
- Around line 7673-7688: The current code uses Task.detached inside
refreshDetectedImportBrowsers so cancelling detectedImportBrowserRefreshTask
does not cancel the expensive InstalledBrowserDetector.detectInstalledBrowsers()
scan; fix by making the scan cancellable: either run the scan in the same parent
Task (replace Task.detached with Task {
InstalledBrowserDetector.detectInstalledBrowsers() }) so it inherits
cancellation from detectedImportBrowserRefreshTask, or store the detached task
in a separate property (e.g., detectedImportBrowserScanTask) and call .cancel()
on it when starting a new refresh; ensure the code checks Task.isCancelled
appropriately and clears both detectedImportBrowserRefreshTask and the scan task
when finished, while still using detectedImportBrowserRefreshGeneration and
generation guard as currently implemented.
- Around line 8093-8105: The persisted selected section is still used for the
first detail render; pre-seed the pending navigation target before the initial
body evaluation by consuming
SettingsWindowPresenter.consumePendingNavigationTarget() and applying it to the
view's selection state (e.g., selectedSection or selectedSectionRaw) during view
initialization (or immediately when the view's state is created) instead of
waiting for .onAppear; then keep the existing navigate(to:postRequest:) call for
post-appearance behavior so the initial render uses the pending target
(references: SettingsView, selectedSection/selectedSectionRaw,
SettingsWindowPresenter.consumePendingNavigationTarget(),
navigate(to:postRequest:), .onAppear).
---
Nitpick comments:
In `@Sources/Settings/SettingsPickerRows.swift`:
- Around line 13-27: The code currently drops unmapped settings paths in
searchAnchorIDs via compactMap, silently losing anchors; change searchAnchorIDs
to detect and fail fast when SettingsSearchIndex.anchorID(forSettingsPath:)
returns nil (e.g., map the paths to optionals, collect any nil-mapped paths and
call precondition/fatalError with a clear message listing those paths), and
update validate() (which now only checks
CmuxSettingsFileStore.supportedSettingsJSONPaths) to also assert that every path
has a corresponding SettingsSearchIndex.anchorID(forSettingsPath:) entry so
missing search-anchor mappings are caught at runtime; reference searchAnchorIDs,
SettingsSearchIndex.anchorID(forSettingsPath:), and validate() to locate the
changes.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3f9871ce-d929-4fb8-bbe8-6951aeaecc4d
📒 Files selected for processing (7)
GhosttyTabs.xcodeproj/project.pbxprojSources/Settings/SettingsDebugSocketCommands.swiftSources/Settings/SettingsPickerRows.swiftSources/TerminalController.swiftSources/cmuxApp.swifttests_v2/cmux.pytests_v2/test_settings_open_latency.py
✅ Files skipped from review due to trivial changes (2)
- Sources/TerminalController.swift
- GhosttyTabs.xcodeproj/project.pbxproj
Profiling showed the first Settings presentation was dominated by main-thread SwiftUI construction of every settings pane plus pane-local side effects such as browser import detection, workspace color palette reloads, and shortcut recorder row initialization. The detail now renders only the selected top-level pane, applies pending Settings navigation before the first detail render, preserves dirty Browser allowlist drafts across section changes, and moves browser detection off the main actor. Constraint: The fix must stay surgical and avoid sleeps, new dependencies, broad Settings refactors, or Swift file-length budget growth. Rejected: Pre-warming every pane after launch | keeps the expensive work and risks moving the stall into startup. Rejected: Off-main shell/disk scan cleanup alone | profiling showed the dominant cost was eager main-thread view construction. Confidence: medium Scope-risk: moderate Directive: Keep Settings search/navigation selecting the target section before requesting an anchor scroll; hidden panes do not have live anchors. Tested: ./scripts/reload.sh --tag issue-3384-settings-slow-open --launch succeeded and launched the tagged app. Not-tested: Local tests by instruction; CI owns tests_v2/test_settings_open_latency.py.
08d6b18 to
6e91fce
Compare
|
Updated after the relevant review comments:
I did not address unrelated SettingsPickerRows anchor-validation nitpicks because they are outside the Settings-open latency regression. The old docs-file thread is stale; no markdown file is staged or committed in the current branch. |
|
CI is green on the refreshed head commit 6e91fce. Relevant checks passed: tests, release-build, tests-build-and-lag, ui-regressions, workflow-guard-tests, web-typecheck, web-db-migrations, remote-daemon-tests, build-ghosttykit, both compat-tests lanes, Vercel, Socket, CodeRabbit, and Cursor Bugbot. |
|
Closing — abandoning this branch. Issue #3384 remains open and will be retried. |
This branch was successfully deployed
1 participant
Fixes #3384.
Measurements
Baseline DEV build, clean profile:
Instrumented DEV build, clean profile:
Instrumented DEV build, heavier profile with 6 workspaces and 3 browser panes:
Fixed DEV build:
Root Cause
Settings opened by constructing one giant SwiftUI detail view for every pane even when the selected pane was Account. The first open showed all 11 pane headers appearing at first presentation, eager Browser import detection on the main thread, repeated workspace palette refreshes, and hundreds of keyboard shortcut recorder rows initialized around first paint.
openWindow()itself blocked the main actor for about 1283 ms cold and 371-408 ms warm, so this was main-thread view construction and synchronous pane setup rather than off-main work gating paint.Fix
The Settings detail now constructs only the selected top-level pane and runs pane-specific refresh work only when that pane is visible. Browser import detection is moved off the main actor, dirty Browser HTTP allowlist drafts are preserved while switching sections, pending deep-link targets are applied before the first detail render, and scroll-to navigation is delayed until after the lazy target section has a chance to render its anchors.
Regression Budget
tests_v2/test_settings_open_latency.pyopens Settings through the socket and asserts first present to a capturable Settings window stays at or below 1000 ms. It uses the existing debug screenshot command plus standard-library PNG header parsing, so tests_v2 does not require PyObjC/Quartz. Main measured about 2962 ms on the same path, while the fixed path measured about 560 ms.Verification
./scripts/reload.sh --tag issue-3384-settings-slow-open --launchreachedBUILD SUCCEEDEDduring intermediate verification. LaunchServices returned-600on that intermediate launch attempt, so I reran the required tagged launch after updating the PR.Notes
The investigation notes are kept in the PR body only; no Markdown PR-body draft is committed to the repository.
Summary by CodeRabbit
Performance Improvements
New Features
Bug Fixes
Tests
Note
Medium Risk
Medium risk because it restructures the Settings SwiftUI view composition and refresh side-effects (including async browser detection and draft/dirty state), which could cause missing refreshes or state regressions when switching sections.
Overview
Improves Settings window first-open latency by changing the detail view to only construct and refresh the currently selected top-level section, rather than eagerly building all panes.
Adds section-aware refresh hooks and moves installed-browser import detection off the main thread with cancellable, generation-guarded tasks; also preserves unsaved edits for the browser HTTP allowlist via explicit draft initialization/dirty tracking.
Refactors reusable Settings picker rows into
Settings/SettingsPickerRows.swift, adds a DEBUG socket methoddebug.settings.window_statefor window visibility/size introspection, and introduces atests_v2/test_settings_open_latency.pyregression test that asserts Settings becomes capturable within a 1s budget.Reviewed by Cursor Bugbot for commit 6e91fce. Bugbot is set up for automated code reviews on this repo. Configure here.