Skip to content

Show one settings section at a time - #12993

Open
austinywang wants to merge 1 commit into
mainfrom
task-12959-settings-sections
Open

austinywang wants to merge 1 commit into
mainfrom
task-12959-settings-sections

Conversation

@austinywang

@austinywang austinywang commented Sep 19, 2026

Copy link
Copy Markdown
Contributor

Fixes #12959.

Settings now mounts only the selected section in the detail pane, so switching sidebar rows changes panes instead of building one long scrolling page. The detail title follows the selected section, and the Settings window keeps minimize and zoom controls visible but disabled per the macOS HIG.

Validation:

  • git diff --check
  • Swift package tests could not run in this Linux worktree because the Swift toolchain is unavailable.

Localization audit: no new user-facing strings were introduced; existing localized section titles are reused.


View with [code]smith Autofix with [code]smith
Need help on this PR? Tag @codesmith-bot with what you need. Autofix is disabled.


Summary by cubic

Settings now displays one section at a time in the detail pane, replacing the previous single long scrolling page with all sections. The navigation title follows the selected section, and the window's minimize and zoom controls stay visible but disabled per macOS HIG.

Inactive sections are no longer added to the view hierarchy, so their controls are not constructed until needed.

Written for commit 4a92714. Summary will update on new commits.

Review in cubic

Summary by CodeRabbit

  • New Features
    • Settings windows now display the title of the currently selected section.
    • Inactive settings sections no longer render their content.
    • Minimize and zoom controls remain visible but are disabled in the settings window.

@github-actions

Copy link
Copy Markdown

Note

Pull Request opener @austinywang is not an author or co-author of any commit in this PR (commit identities: lawrencecchen). The CLA check will still proceed and requires every listed identity plus @austinywang to have signed.

All contributors have signed the CLA ✍️ ✅
Posted by the CLA Assistant Lite bot.

@coderabbitai

coderabbitai Bot commented Sep 19, 2026

Copy link
Copy Markdown

Review Change StackReview Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository: manaflow-ai/cmux/.coderabbit.yaml

Review profile: ASSERTIVE

Plan: Advanced

Run ID: 29134fba-87ee-4402-8b9a-07513c8bd88d

📥 Commits

Reviewing files that changed from the base of the PR and between 9c2ba78 and 4a92714.

📒 Files selected for processing (4)
  • Packages/macOS/CmuxSettingsUI/Sources/CmuxSettingsUI/Scene/SettingsSectionSlot.swift
  • Packages/macOS/CmuxSettingsUI/Sources/CmuxSettingsUI/Scene/SettingsWindowScene+Sections.swift
  • Packages/macOS/CmuxSettingsUI/Sources/CmuxSettingsUI/Scene/SettingsWindowScene.swift
  • Sources/App/SettingsWindowFactory.swift

Included review availability: Your plan provides up to 10 included reviews per hour; 3 remain after this review.


📝 Walkthrough

Walkthrough

The settings window now renders only the selected section, updates its navigation title to that section, and keeps minimize and zoom controls visible but disabled.

Changes

Settings pane behavior

Layer / File(s) Summary
Selected section rendering
Packages/macOS/CmuxSettingsUI/Sources/CmuxSettingsUI/Scene/SettingsSectionSlot.swift, Packages/macOS/CmuxSettingsUI/Sources/CmuxSettingsUI/Scene/SettingsWindowScene+Sections.swift, Packages/macOS/CmuxSettingsUI/Sources/CmuxSettingsUI/Scene/SettingsWindowScene.swift
SettingsSectionSlot renders content or a placeholder only when its section is selected. Inactive sections render EmptyView. The detail scroll view uses the selected section title.
Window control state
Sources/App/SettingsWindowFactory.swift
The minimize and zoom controls remain visible but are disabled.

Priority: ➖ Normal

Estimated code review effort: 2 (Simple) | ~10 minutes

Change: Bug fix · Severity of issue fixed: Medium

Sequence Diagram(s)

sequenceDiagram
  participant SettingsWindowScene
  participant SettingsSectionSlot
  participant ScrollView
  SettingsWindowScene->>SettingsSectionSlot: Pass isActive for selected section
  SettingsSectionSlot-->>SettingsWindowScene: Render content or EmptyView
  SettingsWindowScene->>ScrollView: Set navigationTitle to selectedSection.title
Loading

Important

Pre-merge checks failed

Please resolve all errors before merging. Addressing warnings is optional.

❌ Failed checks (1 error, 1 warning)

Check name Status Explanation Resolution
Cmux Architecture Rethink ❌ Error The new active-pane condition breaks the existing mount lifecycle and leaves invalid state representable. SettingsWindowScene+Sections.swift now derives isActive from selectedSection, while `Set… Make selectedSection the single source of truth for the detail-pane lifecycle. Remove the automatic progressive queue/awaitingAppearance chain from this path, or redefine the mount model so it tracks only the selected section and on-d…
Docstring Coverage ⚠️ Warning Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 2 functions across 4 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (23 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly summarizes the primary change: displaying one Settings section at a time.
Description check ✅ Passed The description explains what changed, why it changed, and how validation was performed. It omits the requested demo video and checklist, but it provides the core information needed to review the UI b…
Linked Issues check ✅ Passed The changes satisfy the coding objectives in #12959. SettingsSectionSlot renders content only when isActive is true, and the section helper matches that flag to selectedSection. This gives one a…
Out of Scope Changes check ✅ Passed The changes stay within #12959. The slot activation logic, pane title update, and disabled window controls directly implement the issue. The preserved search state and progressive mounting support the…
Cmux Cloud Persistent Session And Early Input ✅ Passed PASS. The authoritative diff changes only Settings SwiftUI pane visibility, the Settings navigation title, and AppKit Settings window button state. It does not change Cloud terminal creation, cmux-tui…
Cmux Swift Actor Isolation ✅ Passed PASS. The diff only adds UI state and UI operations. SettingsSectionSlot was already @MainActor in the base and remains a SwiftUI View; the enclosing SettingsWindowRoot was already `@MainActor…
Cmux Swift Blocking Runtime ✅ Passed PASS: The PR does not introduce or materially expand any blocking or timing-based synchronization. The changed lines add SwiftUI view selection, .navigationTitle, and disabled AppKit controls. The o…
Cmux Browser Automation Off-Main ✅ Passed PASS: The pull request changes only Settings SwiftUI/AppKit files. The authoritative diff adds section activation, a navigation title, and disabled Settings window controls. It does not add or move an…
Cmux Expensive Synchronous Load ✅ Passed PASS: The authoritative diff changes only Settings SwiftUI view composition, navigation title, and AppKit button state. SettingsSectionSlot adds an isActive condition and calls content() only fo…
Cmux Cache Substitution Correctness ✅ Passed PASS: The reviewed diff changes SwiftUI section visibility, navigation title, and AppKit window-button state. It does not replace an authoritative read with a cached or opportunistic value in a persis…
Cmux No Hacky Sleeps ✅ Passed PASS. The reviewed range changes only four .swift files. The applicable rule is limited to TypeScript, JavaScript, shell, and non-Swift build/runtime scripts; Swift timing is explicitly covered by a…
Cmux Algorithmic Complexity ✅ Passed PASS. The pull request adds no scalable collection scan, sort, filter, join, or batch rescan. The changed code only compares each slot's SettingsSectionID with selectedSection, conditionally build…
Cmux Swift Concurrency ✅ Passed PASS. The pull request adds only SwiftUI view selection, a navigation title, and AppKit button-state updates. The exact added lines contain no DispatchQueue, DispatchGroup, Combine state, completi…
Cmux Swift @Concurrent ✅ Passed PASS: The pull request adds no async, await, nonisolated, or @concurrent code. The changed declarations remain synchronous and @MainActor-isolated where UI work requires it. The existing `Ta…
Cmux Swift Package Boundaries ✅ Passed PASS. The authoritative diff changes only SwiftUI settings view composition in the existing CmuxSettingsUI package and AppKit window-control glue in Sources/App/SettingsWindowFactory.swift. `Setti…
Cmux Swiftpm Lockfiles ✅ Passed PASS. The authoritative PR diff changes only four Swift source files: three under Packages/macOS/CmuxSettingsUI/Sources/... and Sources/App/SettingsWindowFactory.swift. It does not change a `Packa…
Cmux Swift Logging ✅ Passed PASS: The pull request adds no print, debugPrint, dump, NSLog, ad hoc file logging, or sensitive-data logging. The existing Logger declaration and log.fault call in `SettingsWindowFactory.…
Cmux User-Facing Error Privacy ✅ Passed PASS. The authoritative diff changes SwiftUI section visibility, the detail navigation title, and window button state. It adds no user-facing error, alert, command output, API error body, or recovery …
Cmux Full Internationalization ✅ Passed The PR adds no new user-facing literal text and changes no localization catalogs, Info.plist strings, or locale files. The new .navigationTitle(selectedSection.title) reuses `SettingsSectionID.title…
Cmux Swiftui State Layout ✅ Passed PASS. The PR adds a plain isActive value, conditional EmptyView, .navigationTitle, and AppKit button settings. It does not add ObservableObject, @Published, @StateObject, `@EnvironmentObje…
Cmux Swift Auxiliary Window Close Shortcuts ✅ Passed PASS. The pull request changes the existing Settings window content, title, and button state. It does not add a window or close-key workaround. SettingsWindowPresenter already assigns the stable `cm…
Cmux Source Artifacts ✅ Passed The PR changes only four tracked .swift source files under Packages/.../Sources and Sources/App. The diff adds SwiftUI/AppKit product behavior, comments, and a flag; it adds no logs, screenshots…
Cmux No Test Or Debug Seam In Production Source ✅ Passed The pull request changes four production Swift files, but it adds no test/debug seam. The diff contains no #if DEBUG, test-build guard, XCTest hook, debug/test-named member, visibility widening, or …
Full details: Cmux Architecture Rethink

Explanation

The new active-pane condition breaks the existing mount lifecycle and leaves invalid state representable. SettingsWindowScene+Sections.swift now derives isActive from selectedSection, while SettingsSectionMountModel still expects every mounted slot to report onAppear and advances through sectionDidAppear. After the initial selected section appears, the model mounts the next section, but that slot is inactive and renders EmptyView, so its appearance callback cannot run. The model remains permanently incomplete until the user happens to select the pending section. The unchanged cold-mount tests still wait for isComplete and expect every section to mount, which confirms the changed behavior conflicts with the existing lifecycle invariant. This is a causal split between selection state and progressive-mount state, not an existing debt alone.

Resolution

Make selectedSection the single source of truth for the detail-pane lifecycle. Remove the automatic progressive queue/awaitingAppearance chain from this path, or redefine the mount model so it tracks only the selected section and on-demand navigation target. Keep onAppear only for deferred navigation handling; do not call sectionDidAppear to mount an inactive next slot. Update the mount model and cold-mount tests to assert that only the selected section is mounted initially, that selecting or searching for another section explicitly mounts that section, and that no inactive section remains pending as an implicit lifecycle transition.

  • Fix all pre-merge checks with AI
✨ Finishing Touches 💡 1
📝 Generate docstrings 💡
  • Commit to this branch
  • Create a new PR
🧪 Generate unit tests (beta)
  • Commit to this branch
  • Create a new PR

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.

@blacksmith-sh

blacksmith-sh Bot commented Sep 19, 2026

Copy link
Copy Markdown

Found 2 test failures on Blacksmith runners:

Failures

Test View Logs
beta archive command stamps the beta bundle id/
beta archive command stamps the beta bundle id
View Logs
beta export-only lane succeeds with fake Apple tools/
beta export-only lane succeeds with fake Apple tools
View Logs

Fix with [code]smith
Need help on this PR? Tag @codesmith-bot with what you need.

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.

Settings shows every section on one scrolling page

2 participants