Skip to content

Make Settings a top-level peer window, not a floating child (#5081) - #5083

Merged
austinywang merged 4 commits into
mainfrom
issue-5081-settings-window-level
Jun 1, 2026
Merged

austinywang merged 4 commits into
mainfrom
issue-5081-settings-window-level

Conversation

@austinywang

@austinywang austinywang commented Jun 1, 2026 •

Copy link
Copy Markdown
Contributor

Fixes #5081.

Problem

The Settings window stays painted on top of the main cmux window. Clicking the main window cannot raise it above Settings — Settings only recedes if you close it.

Two independent instances of the same class of bug:

  1. Settings was attached to the main window via parentWindow.addChildWindow(window, ordered: .above) in SettingsWindowPresenter. An AppKit child window is pinned above its parent forever and can never recede when the user clicks the parent — exactly the reported symptom. This was introduced in Keep Settings layered above the main window #3612 to surface the main window behind Settings on a global hotkey, but addChildWindow over-delivered into a permanent float.
  2. ConfigSettingsView explicitly set window.level = .floating on the user-facing Config editor — another peer window that should not float.

Fix (class of bugs, not just Settings)

  • SettingsWindowPresenter no longer creates a parent-child relationship. performFocus orders the preferred main window front once and then fronts Settings as an independent peer — same initial "Settings in front of its app" layering Keep Settings layered above the main window #3612 wanted, but normal click-to-raise ordering stays intact afterwards. The whole child-attachment + parent-close-detach observer apparatus is deleted (~80 lines); "Settings survives the main window closing" is now automatic because there is no child relationship to tear down.
  • ConfigSettingsView adopts the peer level instead of .floating.
  • New NSWindow.adoptCmuxPeerWindowLevel() is the single, documented seam that states the .normal peer-window invariant. Both Settings and the Config editor use it, so the only remaining way to float a window is a deliberate, commented level = .floating at the call site (the #if DEBUG HUD/lab panels keep that, intentionally). A new top-level peer window can no longer accidentally inherit floating behavior.

Acceptance criteria

  • ✅ Click main window → it comes above Settings; Settings recedes (standard ordering).
  • ✅ Re-clicking Settings raises it normally.
  • ✅ Settings is not a child of the main window — clicking the main window does not close it; still reachable via Cmd+, / App menu when hidden behind.
  • ✅ Settings recedes with cmux when switching to another app (no cross-app float).
  • ✅ Config editor gets the same treatment; DEBUG HUD/lab panels that intentionally float are unchanged.

Tests

Two-commit red/green:

  • Commit 1 inverts the SettingsWindowPresenter tests to assert the peer invariant (never a child window, stays .normal, survives the main window closing). These fail against the child-window implementation.
  • Commit 2 lands the fix and a direct test for adoptCmuxPeerWindowLevel() bringing a .floating window back to .normal. AppKit z-order itself isn't unit-testable, but the structural invariants that caused the bug are.

🤖 Generated with Claude Code


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


Note

Low Risk
macOS window ordering and presentation only; no auth, data, or network changes. Behavioral risk is limited to how auxiliary windows layer with the main window.

Overview
Fixes #5081 by treating Settings and the Config editor as normal top-level windows instead of windows that stay pinned above the main cmux window.

Settings no longer uses addChildWindow on the main window (that relationship kept Settings above the parent permanently). Focus now does a one-time orderFront on the preferred main window, then brings Settings forward as a peer at .normal, so click-to-raise works again. The parent/child attach, detach, and close observers are removed.

Config editor drops window.level = .floating in favor of the same peer behavior.

New NSWindow.adoptCmuxPeerWindowLevel() centralizes the .normal invariant for top-level auxiliary windows. Tests assert Settings is never a child window and stays at normal level.

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


Summary by cubic

Make Settings a top‑level peer window instead of a floating child, so it no longer stays pinned above the main window. The Config editor adopts the same behavior; fixes #5081.

  • Bug Fixes
    • Removed the child-window attachment; focusing now surfaces the preferred main window once, then fronts Settings as a peer.
    • Added NSWindow.adoptCmuxPeerWindowLevel() and used it in Settings and the Config editor to enforce .normal.
    • Quoted App/NSWindow+CmuxPeerWindow.swift in cmux.xcodeproj to fix an xcodebuild parse error.
    • Synced with latest main and resolved project.pbxproj conflict; no behavior changes.

Written for commit f6ed774. Summary will update on new commits.

Review in cubic

Summary by CodeRabbit

  • Bug Fixes
    • Settings and editor windows now act as independent peer windows: improved layering, focus, ordering, and visibility when other app windows change or close. Editor windows no longer use floating layering and remain at normal window level.
  • Tests
    • Updated test suite to verify peer-window behavior and window level handling.

austinywang and others added 2 commits May 31, 2026 20:45
Settings is attached to the main window via addChildWindow, which pins it
above the parent forever — clicking the main window can never raise it above
Settings. These tests assert the peer-window invariant (never a child window,
stays .normal level, survives the main window closing) and fail against the
current child-window implementation. The fix lands in the next commit.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The Settings window was attached to the main window via addChildWindow(ordered:
.above), and ConfigSettingsView set window.level = .floating. Both pin the
window above the main window forever: a child window can never recede when the
user clicks its parent, so clicking the main cmux window could not raise it
above Settings. This is the bug reported in #5081.

Fix the class of bugs, not just Settings:

- SettingsWindowPresenter no longer creates a parent-child relationship. To
  preserve PR #3612's intent (a global hotkey / app activation surfaces the
  main window behind Settings), performFocus now orders the preferred main
  window front *once* and then fronts Settings as an independent peer. Normal
  click-to-raise ordering is left fully intact afterwards. The entire
  child-attachment + parent-close-detach observer apparatus is deleted (~80
  lines): "Settings survives the main window closing" is now automatic because
  there is no child relationship to tear down.
- ConfigSettingsView stops forcing .floating; it is a peer editor window.
- Add NSWindow.adoptCmuxPeerWindowLevel() as the single, documented seam that
  states the .normal peer-window invariant. Both Settings and the Config editor
  adopt it, so the only remaining way to float a window is a deliberate,
  commented level = .floating at the call site (DEBUG HUD/lab panels keep that).

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@vercel

vercel Bot commented Jun 1, 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 Jun 1, 2026 6:27am
cmux-staging Building Building Preview, Comment Jun 1, 2026 6:27am

@coderabbitai

coderabbitai Bot commented Jun 1, 2026 •

Copy link
Copy Markdown

Review Change Stack

📝 Walkthrough

Walkthrough

This PR moves Settings and related editor windows from floating/child behavior to independent top-level peers by adding an NSWindow helper to set .normal level, updating SettingsWindowPresenter to order peers during focus, changing ConfigSettingsView to adopt the peer level, updating tests, and wiring the new source into the Xcode project.

Changes

Settings window as independent peer

Layer / File(s) Summary
Peer window level adoption contract
Sources/App/NSWindow+CmuxPeerWindow.swift
New @MainActor extension method adoptCmuxPeerWindowLevel() sets window level to .normal, documenting the cmux peer-window contract.
Settings presenter: peer ordering and state changes
Sources/App/SettingsWindowPresenter.swift
Removes parent-close observation and child-window attachment; adds pending navigation targets and shouldOpenWhenConfigured flag; configure(window:) adopts peer level and clamps to visible area; performFocus(_:) orders preferred parent front as a peer (deminimizing first), activates the app, and brings Settings forward; resetForTests no longer detaches parents.
Config editor: adopt peer level
Sources/Settings/ConfigSettingsView.swift
Replaces .floating level with adoptCmuxPeerWindowLevel() in configureWindow(_:), marking the editor as a top-level peer window.
Tests: verify peer behavior
cmuxTests/SettingsWindowPresenterTests.swift
Tests updated to assert settingsWindow.parent == nil, that preferred main windows do not list Settings as a child, Settings remains at .normal across preferred-parent changes and close events; adds test for adoptCmuxPeerWindowLevel() converting .floating -> .normal.
Project: include new source in Xcode target
cmux.xcodeproj/project.pbxproj
Adds PBX build/file reference and includes App/NSWindow+CmuxPeerWindow.swift in Sources group and cmux target build phase.

Sequence Diagram(s):

sequenceDiagram
  participant User
  participant SettingsWindowPresenter
  participant PreferredMainWindow
  participant SettingsWindow
  participant App
  User->>SettingsWindowPresenter: performFocus()
  SettingsWindowPresenter->>SettingsWindow: adoptCmuxPeerWindowLevel()
  SettingsWindowPresenter->>PreferredMainWindow: deminiaturize() / orderFront(nil)
  SettingsWindowPresenter->>App: activate(ignoringOtherApps: true)
  SettingsWindowPresenter->>SettingsWindow: orderFront(nil)
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Possibly related issues

  • manaflow-ai/cmux#5081: This PR implements the core fix described — Settings is set to .normal level and treated as a peer so it no longer stays above the main window.

Possibly related PRs

  • manaflow-ai/cmux#4661: Overlaps in changes to SettingsWindowPresenter focus/configuration flows and related state handling.

"🐰 I hopped in to mend the floating plight,
A peer at normal level, calm and right.
Click the main window — it rises anew,
Settings stays friendly, no floating to-do.
Cheers from a rabbit with a tiny review!"

🚥 Pre-merge checks | ✅ 17 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 15.38% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (17 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and specifically describes the main change: converting Settings from a floating child window to a top-level peer window, directly addressing issue #5081.
Linked Issues check ✅ Passed All requirements from #5081 are met: Settings is no longer a floating child window, uses standard macOS window ordering, removes permanent floating behavior, remains accessible via menu, and Config editor receives identical treatment.
Out of Scope Changes check ✅ Passed All changes directly support the stated objectives: new NSWindow extension, Settings presenter refactoring, Config editor updates, project configuration, and test updates are all scoped to fixing the window layering issue.
Cmux Swift Actor Isolation ✅ Passed New method properly marked @MainActor; static variables added within existing @MainActor enum context storing Sendable types; SwiftUI View inherently isolated. No violations.
Cmux Swift Blocking Runtime ✅ Passed No blocking or timing-based synchronization patterns introduced; Task usage is non-blocking async dispatch with no sleeps/waits, and window ordering uses standard non-blocking AppKit APIs.
Cmux No Hacky Sleeps ✅ Passed Check does not apply: PR modifies only Swift files and Xcode config. Rule scope excludes Swift (covered by separate check), only covering TypeScript, JavaScript, shell, and non-Swift build scripts.
Cmux Algorithmic Complexity ✅ Passed All collection operations iterate over tiny fixed-size collections: NSApp.windows (handful of windows) and ConfigSource.allCases (2-case enum), both explicitly bounded per the rule's PASS criteria.
Cmux Swift Concurrency ✅ Passed No DispatchQueue, Combine, completion handlers, or new fire-and-forget Tasks introduced. PR removes notification observers and replaces parent-child windows with peer ordering.
Cmux Swift @Concurrent ✅ Passed No @concurrent annotation violations found: no missing @concurrent on nonisolated async work, no @concurrent on sync functions, all UI-bound async work properly isolated.
Cmux Swift File And Package Boundaries ✅ Passed New 29-line AppKit extension, SettingsWindowPresenter reduced by 69 lines, ConfigSettingsView +3 lines. All within size thresholds, no mixed responsibilities.
Cmux Swift Logging ✅ Passed All Swift production code changes comply with swift-logging.md rules. The only logging statement added (cmuxDebugLog) is properly #if DEBUG guarded and follows established patterns.
Cmux User-Facing Error Privacy ✅ Passed No user-facing error messages, alerts, or sensitive information exposed. All changes are window management code with developer-only comments and DEBUG-guarded test utilities.
Cmux Full Internationalization ✅ Passed No new user-facing text without proper localization. All user-facing strings in ConfigSettingsView use String(localized:) with complete translations for en and ja locales.
Cmux Swiftui State Layout ✅ Passed ConfigSettingsView uses @State correctly, AppKit bridge pattern is proper, no new @Published/@observableobject patterns, no GeometryReader, no lazy-list store refs, no render-time mutations detected.
Cmux Architecture Rethink ✅ Passed PR removes observer/child-window patterns, replaces with peer-level ordering. Introduces one clear invariant (adoptCmuxPeerWindowLevel). No timing, locks, or symptom patches detected.
Cmux Swift Auxiliary Window Close Shortcuts ✅ Passed Both windows modified (Settings, Config editor) have stable cmux.* identifiers already registered in cmuxAuxiliaryWindowIdentifiers; no new unregistered windows introduced.
Description check ✅ Passed PR description is comprehensive, well-structured, and covers all required template sections with clear problem statement, fix explanation, acceptance criteria, and testing approach.

✏️ 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 issue-5081-settings-window-level

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.

@greptile-apps

greptile-apps Bot commented Jun 1, 2026 •

Copy link
Copy Markdown
Contributor

Greptile Summary

This PR fixes the "Settings window floats above the main window forever" bug by removing the AppKit parent-child relationship (addChildWindow) that permanently pinned Settings above the main terminal window. It also drops the level = .floating assignment in the Config editor.

  • SettingsWindowPresenter sheds ~80 lines of child-window wiring (attach, detach, close observer); performFocus now orders both windows front as independent peers — main window first, then Settings — giving the same initial "Settings in front" layering without the permanent float.
  • NSWindow.adoptCmuxPeerWindowLevel() is introduced as a single, @MainActor-annotated seam that sets level = .normal and documents the peer-window invariant; both Settings and the Config editor call it.
  • Tests are updated with a two-commit red/green strategy asserting no parent-child relationship, .normal level, and that the helper correctly resets a floating window.

Confidence Score: 5/5

Safe to merge — window ordering and level changes are localized to Settings and the Config editor, with targeted structural tests and no auth or data paths involved.

The change deletes the parent-child wiring that caused the bug and replaces it with a simple, well-documented peer-window pattern. The new adoptCmuxPeerWindowLevel() helper is a one-liner with no side effects beyond setting level = .normal. Actor isolation is correct throughout, no timing primitives are introduced, and the tests directly assert the structural invariants that prevented the bug from being caught earlier.

No files require special attention.

Important Files Changed

Filename Overview
Sources/App/NSWindow+CmuxPeerWindow.swift New extension adding adoptCmuxPeerWindowLevel() — a single, well-documented seam that enforces .normal level for peer windows; correctly annotated @MainActor.
Sources/App/SettingsWindowPresenter.swift Removes ~80 lines of child-window wiring (addChildWindow, close observer, detach); performFocus now orders the main window front as a peer before fronting Settings via makeKeyAndOrderFront — same initial layering, no permanent float. All actors/isolation unchanged.
Sources/Settings/ConfigSettingsView.swift Replaces window.level = .floating with window.adoptCmuxPeerWindowLevel() in the configureWindow helper; fix is minimal and correctly scoped.
cmux.xcodeproj/project.pbxproj Adds NSWindow+CmuxPeerWindow.swift to the app Sources build phase; path is correctly quoted to avoid xcodebuild parse errors from the + character.
cmuxTests/SettingsWindowPresenterTests.swift Tests inverted to assert the peer invariant (no parent-child relationship, .normal level, Settings survives main window close); new test for adoptCmuxPeerWindowLevel resetting a floating window.

Sequence Diagram

sequenceDiagram
    participant User
    participant SettingsWindowPresenter
    participant MainWindow as Main Window (peer)
    participant SettingsWindow as Settings Window (peer)

    Note over SettingsWindowPresenter: configure(window:) called by SwiftUI Window scene
    SettingsWindowPresenter->>SettingsWindow: "adoptCmuxPeerWindowLevel() level = .normal"
    SettingsWindowPresenter->>SettingsWindow: clampToVisibleAreaIfNeeded()
    SettingsWindowPresenter-->>SettingsWindowPresenter: "Task { focus(window) }"

    Note over SettingsWindowPresenter: performFocus() no addChildWindow
    SettingsWindowPresenter->>SettingsWindow: adoptCmuxPeerWindowLevel() defensive reset
    SettingsWindowPresenter->>MainWindow: orderFront(nil) surfaces main first as peer
    SettingsWindowPresenter->>SettingsWindowPresenter: NSRunningApplication.activate()
    SettingsWindowPresenter->>SettingsWindow: makeKeyAndOrderFront(nil) Settings lands above main
    SettingsWindowPresenter->>SettingsWindow: orderFrontRegardless()

    User->>MainWindow: click main window
    Note over MainWindow,SettingsWindow: Standard click-to-raise main comes forward Settings recedes no permanent float
Loading

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

@cubic-dev-ai cubic-dev-ai 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.

No issues found across 5 files

Re-trigger cubic

…dow.swift

Old-style plist values containing '+' must be quoted; the unquoted
path = App/NSWindow+CmuxPeerWindow.swift made xcodebuild fail to read the
project ("missing semicolon in dictionary on line 963"), failing every
build-dependent check. Matches the quoting of existing AppDelegate+*.swift refs.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…indow-level

# Conflicts:
#	cmux.xcodeproj/project.pbxproj

This branch was successfully deployed

1 active deployment
Preview – cmux — f6ed774b Deployed Jun 1, 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.

Settings window stays on top of main cmux window; click on main window does not raise it

1 participant