Skip to content

Place config error notice below the tab bar - #15218

Merged
lawrencecchen merged 3 commits into
mainfrom
feat-config-notice-below-tabbar
Sep 28, 2026
Merged

lawrencecchen merged 3 commits into
mainfrom
feat-config-notice-below-tabbar

Conversation

@lawrencecchen

@lawrencecchen lawrencecchen commented Sep 28, 2026 •

Copy link
Copy Markdown
Contributor

The Ghostty config error card overlapped the Bonsplit tab bar by 16pt. It now sits one tab bar height (28pt) lower, so the gap below the tab bar equals the 12pt gap from the right edge.

🤖 Generated with Claude Code


Summary by cubic

Fixes the Ghostty config error card overlapping the Bonsplit tab bar by 16pt. The notice now sits one tab bar height (28pt) below the bar, so the gap under the bar matches the 12pt gap from the right edge.

The offset is measured from the window top using cmux's own chrome heights instead of the host's content layout rect, since the native titlebar behind contentLayoutRect is taller than the titlebar cmux draws. Standard mode clears the titlebar band plus the tab bar; minimal mode clears only the tab bar, since the titlebar band is not drawn there. Tests cover both modes.

Written for commit 5113f47. Summary will update on new commits.

Review in cubic

Summary by CodeRabbit

  • Style
    • The diagnostics notice is positioned below the title bar and tab bar, while keeping its existing horizontal alignment.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
@github-actions

Copy link
Copy Markdown
Contributor

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

@coderabbitai

coderabbitai Bot commented Sep 28, 2026 •

Copy link
Copy Markdown

Review in Change Stack →

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

Warning

Review limit reached

Next included review available in 3 minutes.

Check out review usage here.

View limit details

Limit details: You’ve used all 10 included reviews currently available.

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

Learn how review limits work.

Review configuration:

⚙️ Run configuration

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

Review profile: ASSERTIVE

Plan: Advanced

Run ID: 695b327c-81e8-427c-bbd0-fe241b1a5b3c

📥 Commits

Reviewing files that changed from the base of the PR and between 64b8f7f and 5113f47.

📒 Files selected for processing (3)
  • Sources/GhosttyConfigDiagnosticsNoticePresenter.swift
  • cmux.xcodeproj/project.pbxproj
  • cmuxTests/GhosttyConfigDiagnosticsNoticePlacementTests.swift

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: e4eca77a-9b72-43f4-8fc6-aaeeab91d876

📥 Commits

Reviewing files that changed from the base of the PR and between 68ba825 and 64b8f7f.

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

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 2 remain after this review.


📝 Walkthrough

Walkthrough

The diagnostics notice’s position now uses the host window frame instead of contentLayoutRect. Its vertical offset accounts for the app titlebar height, Bonsplit tab bar height, and 12-point inset. The right offset retains a 12-point inset.

Changes

Diagnostics notice positioning

Layer / File(s) Summary
Update notice offset
Sources/GhosttyConfigDiagnosticsNoticePresenter.swift
The notice’s origin now uses the host window frame. Its vertical position subtracts the app titlebar and Bonsplit tab bar heights, with a 12-point inset on both axes.

Priority: ⬇️ Low

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

Change: Bug fix

Suggested reviewers: teamleaderleo

Merge Risk: ⚪ Minimal · up to 64b8f

The notice is positioned below the titlebar and tab bar with the intended spacing. No merge-blocking issue was identified.

🚥 Pre-merge checks | ✅ 23 | ❌ 2

❌ Failed checks (2 warnings)

Check name Status Explanation Resolution
Description check ⚠️ Warning The description explains the overlap and the resulting position, but it omits the required Testing, Changelog, Demo Video, and Checklist sections from the repository template. Add the required sections. Document tests executed and their results, provide a present-tense changelog entry, attach a UI demo video or screenshots, and complete the checklist with localization and review status. State why any non-applicab…
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 1 functions across 1 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 states the primary UI change: moving the config error notice below the tab bar.
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 Cloud Persistent Session And Early Input ✅ Passed PASS — The pull request changes only Sources/GhosttyConfigDiagnosticsNoticePresenter.swift, adjusting the position of a Ghostty configuration diagnostics panel using window and cmux chrome heights. …
Cmux Swift Actor Isolation ✅ Passed PASS. The PR changes only window-position calculations in GhosttyConfigDiagnosticsNoticePresenter.swift. The presenter is already explicitly @MainActor, and the new WindowChromeMetrics reads occ…
Cmux Swift Blocking Runtime ✅ Passed The pull request changes only notice positioning and adds chrome-height calculations. The diff introduces no semaphore, wait, sleep, delayed dispatch, polling, main-queue sync, or manual lock. The exi…
Cmux Browser Automation Off-Main ✅ Passed The check is not applicable. The PR changes only Sources/GhosttyConfigDiagnosticsNoticePresenter.swift, and the diff only changes NSPanel positioning using WindowChromeMetrics. It adds no `brows…
Cmux Expensive Synchronous Load ✅ Passed PASS: The PR changes only GhosttyConfigDiagnosticsNoticePresenter.swift geometry. It replaces contentLayoutRect positioning with NSWindow frame coordinates and WindowChromeMetrics offsets. The…
Cmux Cache Substitution Correctness ✅ Passed PASS — The diff changes only the placement of a transient NSPanel UI notice. It replaces host.contentLayoutRect geometry with host.frame plus WindowChromeMetrics offsets. The presenter keeps t…
Cmux No Hacky Sleeps ✅ Passed PASS. The pull request changes only Sources/GhosttyConfigDiagnosticsNoticePresenter.swift. The diff changes notice positioning and adds no TypeScript, JavaScript, shell, or build/runtime sleep, time…
Cmux Algorithmic Complexity ✅ Passed The PR changes only Sources/GhosttyConfigDiagnosticsNoticePresenter.swift. The new positioning code performs fixed-time arithmetic using two constant chrome heights and window dimensions. It adds no…
Cmux Swift Concurrency ✅ Passed The pull request changes only notice positioning in GhosttyConfigDiagnosticsNoticePresenter.swift: it adds chrome-height layout constants and updates the panel origin. The diff adds no Task, `Disp…
Cmux Swift @Concurrent ✅ Passed The pull request changes only synchronous geometry in GhosttyConfigDiagnosticsNoticePresenter.show(_:in:), which remains inside the existing @MainActor presenter. The diff adds no async, `noniso…
Cmux Swift Package Boundaries ✅ Passed PASS. The diff only changes placement math in the existing GhosttyConfigDiagnosticsNoticePresenter: it replaces contentLayoutRect positioning with window-frame coordinates and cmux chrome-height o…
Cmux Swiftpm Lockfiles ✅ Passed PASS: The PR changes only Sources/GhosttyConfigDiagnosticsNoticePresenter.swift. It does not change a Package.swift, Package.resolved, .gitignore, workflow, Xcode project package references, o…
Cmux Swift Logging ✅ Passed The PR changes only layout calculations and comments in GhosttyConfigDiagnosticsNoticePresenter.swift. The diff adds no print, debugPrint, dump, NSLog, file/stdout logging, Logger, or diag…
Cmux User-Facing Error Privacy ✅ Passed PASS: The PR changes only the notice panel position. The diff adds chrome-height calculations and changes window-coordinate offsets; it does not add or alter user-facing error text, diagnostics, or re…
Cmux Full Internationalization ✅ Passed The pull request changes only notice placement geometry and adds developer comments in Sources/GhosttyConfigDiagnosticsNoticePresenter.swift. The diff adds no user-facing text, localization key, str…
Cmux Swiftui State Layout ✅ Passed PASS. The PR changes only AppKit window geometry in GhosttyConfigDiagnosticsNoticePresenter.show: it replaces contentLayoutRect coordinates with host-frame coordinates and cmux chrome heights. The…
Cmux Architecture Rethink ✅ Passed PASS. The PR makes a small local positioning fix. It replaces contentLayoutRect offsets with host.frame offsets and uses the existing WindowChromeMetrics constants plus the 12-point inset. The d…
Cmux Swift Auxiliary Window Close Shortcuts ✅ Passed PASS. The PR only changes the notice panel position in Sources/GhosttyConfigDiagnosticsNoticePresenter.swift. The NSPanel and its stable cmux.ghosttyConfigDiagnosticsNotice identifier existed un…
Cmux Source Artifacts ✅ Passed The PR changes only Sources/GhosttyConfigDiagnosticsNoticePresenter.swift. The diff contains hand-written Swift source and positioning comments. It adds no logs, screenshots, recordings, temporary d…
Cmux No Test Or Debug Seam In Production Source ✅ Passed The pull request changes only Sources/GhosttyConfigDiagnosticsNoticePresenter.swift. The diff adjusts panel positioning with WindowChromeMetrics and adds comments; it does not add #if DEBUG or t…
Full details: Description check

Resolution

Add the required sections. Document tests executed and their results, provide a present-tense changelog entry, attach a UI demo video or screenshots, and complete the checklist with localization and review status. State why any non-applicable checks do not apply.

✨ 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.

@github-actions

github-actions Bot commented Sep 28, 2026 •

Copy link
Copy Markdown
Contributor

Dogfood build of 5113f47ddbfaa18c5cc86fe89ae3a3f167928960

cmux DEV pr-15218-5113f47d.app

The link opens this exact commit in the cmux dev menu bar app. The build starts on each push and the page waits until it is ready; a newer push replaces it. It signs in against production, so Cloud or backend changes still need a tagged build with a development backend.

lawrencecchen and others added 2 commits September 28, 2026 01:58
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
The notice now clears cmux's own chrome instead of AppKit's native
titlebar, which is the right fix, but it adds the titlebar band height
unconditionally. WorkspaceTitlebarModeLayer draws that band only when the
presentation mode is not minimal (ContentView.swift wraps
workspaceTitlebarBand in it), so in minimal mode the only chrome above
the workspace content is the Bonsplit tab bar.

Counting 56pt there put the card a titlebar's height too low, floating
over live terminal content. Before this branch minimal mode happened to
land correctly, because contentLayoutRect excludes the native titlebar
and that is about the same 28pt, so shipping as-is would trade the
standard-mode bug for a minimal-mode one.

Pull the height into a static function so the two modes are covered by
unit tests rather than by looking at a screenshot.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
@teamleaderleo

Copy link
Copy Markdown
Collaborator

Review (merge-train). The coordinate-space change is right, and I found one case it gets wrong. I pushed the fix rather than bounce it back to you.

Review

  1. Coordinate space: correct. The window is [.titled, .closable, .miniaturizable, .resizable, .fullSizeContentView] with titlebarAppearsTransparent = true and titleVisibility = .hidden (Sources/AppDelegate.swift:10480-10514), and the SwiftUI root applies .ignoresSafeArea() (Sources/ContentView.swift:3453). The content really does fill the whole frame, so the top of drawn content is at window-local frame.height, not contentLayoutRect.maxY. convertPoint(toScreen:) takes points in the window base coordinate system, whose origin is the bottom-left of the frame, which is the same space contentLayoutRect lives in. The new math is valid and is the reason the card stops landing on the tab bar.

  2. x unchanged: holds. contentLayoutRect only ever insets vertically for a titlebar or toolbar. The one titlebar accessory (TitlebarControlsAccessoryViewController, layoutAttribute = .left) sits inside the titlebar band, and the sidebar is plain SwiftUI content rather than an NSSplitViewController item with tracking separators, so content.maxX == frame.width was already true. Horizontal placement is genuinely untouched.

  3. Minimal mode was wrong. chromeHeight was appTitlebarHeight + bonsplitTabBarHeight unconditionally, but WorkspaceTitlebarModeLayer renders the titlebar band only when the mode is not minimal:

    var body: some View {
        if !isMinimalMode {
            titlebar()
        }
    }

    and ContentView.swift:2648 wraps workspaceTitlebarBand in exactly that layer. So minimal mode has 28pt of chrome, not 56, and the card landed a titlebar height too low, floating over terminal content. It is a regression rather than an unfixed case: on main contentLayoutRect excludes the native titlebar, roughly the same 28pt, so minimal mode happened to be placed correctly before this branch. isMainTerminalWindow matches on the window identifier only and never looks at presentation mode, and workspacePresentationMode is one global @AppStorage key, so every main window is affected when the user is in Minimal.

  4. No test coverage. GhosttyConfigDiagnosticsNoticePresenter had zero references outside its own file and its one call site in AppDelegate.swift.

Fixed (5113f47, pushed to this branch):

static func chromeHeight(isMinimalMode: Bool) -> CGFloat {
    let tabBar = WindowChromeMetrics.bonsplitTabBarHeight
    return isMinimalMode ? tabBar : WindowChromeMetrics.appTitlebarHeight + tabBar
}

called with WorkspacePresentationModeSettings.isMinimal(), plus cmuxTests/GhosttyConfigDiagnosticsNoticePlacementTests.swift covering both modes and their ordering. Wired with scripts/sync-test-wiring; check-pbxproj.sh, lint-pbxproj-test-wiring.sh and check-pbxproj-group-membership.py all pass.

Left

  • The main workspace never sets tabBarVisibility explicitly, and showsTabBar(tabCount:) exists, so a single-tab window may hide the tab bar. Bonsplit is not vendored here so I could not confirm the default. If it does hide, the card sits 28pt lower than ideal with a bigger gap. Cosmetic, no overlap, and out of scope for this fix.
  • Fullscreen standard mode still draws the 28pt band (only the traffic-light padding changes), so 56 stays right there. Not separately dogfooded.

CodeRabbit posted a summary only, no inline threads. cubic is skipping.

Dogfood evidence is queued for the Mac session against the new head 5113f47ddbfaa18c5cc86fe89ae3a3f167928960, covering standard mode and minimal mode. Merging on green once that lands.

@github-actions

Copy link
Copy Markdown
Contributor

CI failure attribution

CI failed on 5113f47ddb (run 36405695293 attempt 1): 1 machine.

Job Verdict Why
macos / macOS compile admission machine the runner went away mid-job (runner cmux10s-mac-mini-glaeda-1)
Matched log lines
macos / macOS compile admission: The self-hosted runner lost communication with the server. Verify the machine is running and has a healthy network connection. Anything in your workflow that terminates the runner process, starves it for CPU/Memory, or blocks its network access can cause this error.

Every failure is a machine failure: re-ran the failed jobs as attempt 2 (the checks show its result).

Written by scripts/ci/classify_failures.py (ci-failure-attribution.yml); signatures are its SIGNATURES table. A machine verdict is the runner's fault, not this PR's.

@lawrencecchen
lawrencecchen enabled auto-merge (squash) September 28, 2026 10:10
@lawrencecchen
lawrencecchen merged commit 62cde14 into main Sep 28, 2026
100 of 104 checks passed
@lawrencecchen
lawrencecchen deleted the feat-config-notice-below-tabbar branch September 28, 2026 10:54
@github-actions

Copy link
Copy Markdown
Contributor

Merge receipt for 5113f47ddb, merged 2026-09-28 10:54:45 UTC

  • Not verified at merge: CI timing (in progress)
  • Verified: ci-status, macOS compile admission, Web complexity, web-validation, CI fast guards, Dogfood build #​15218, Fast static checks, GhosttyKit release check, guards (17), linux-preflight, macOS admission gate, macOS status, and 2 more
  • Skipped by policy: app-host unit tests, admission-placement, browser, Claude wrapper regressions, CLI product tests, late-placement, release-admission, release-build, remote-daemon, suite-coverage, swift-package-tests, tests-build-and-lag, and 5 more
  • Full suite: runs on main after merge.

rustybret pushed a commit to rustybret/bmux that referenced this pull request Sep 28, 2026
0c753fe ci: give each Python lane test an empty Foundation home (manaflow-ai#15289)
62cde14 Place config error notice below the tab bar (manaflow-ai#15218)
436909b Keep an exited terminal's tab edge consistent with its revision (manaflow-ai#15205)
fc882fe Cloud: rebake the devbox ladder with cmux-tui 3412812 (manaflow-ai#15323)
8e6357b Add pr-media.py for putting a clip or screenshot on a PR (manaflow-ai#15295)
1755ea8 ci: place release-build and main's side lanes on the owned minis (manaflow-ai#14797)
447eb04 Keep focused-pane notifications silent unless opted in (manaflow-ai#15233)
f66d18a Dial every discovered Mac concurrently on iOS (manaflow-ai#15127)
dc5a21a ci: place the Iroh release gate's Tailscale job on the owned minis (manaflow-ai#15139)
3887653 docs: hide the Cloud beta note on nightly docs (manaflow-ai#15317)
f4115d7 Center cloud row icon glyphs by their visible pixels (manaflow-ai#15149)
8714160 Let dogfood tours hold modifiers while clicking (manaflow-ai#15239)

# Conflicts:
#	.github/workflows/ci-guards.yml
#	.github/workflows/ci-macos.yml
#	.github/workflows/ci-owned-pool-rescue.yml
#	.github/workflows/ci.yml
#	.github/workflows/iroh-release-gate.yml
#	.github/workflows/remote-daemon.yml
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.

2 participants