Attach workspace rename and color alerts to their window - #8544
Conversation
The Rename Workspace alert (sidebar context menu, SwiftUI tab row, and the keyboard-shortcut path), the custom workspace color prompt, and the Rename Tab alert all presented via bare NSAlert.runModal(), which shows a detached alert window that macOS places on whichever screen it deems main - on multi-monitor setups that is often not the screen showing cmux. Present them through runCmuxModal(presentingWindow:) instead, resolving the host via mainWindowContainingWorkspace so each alert attaches as a sheet to the window that owns the workspace. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Bugbot is paused — on-demand spend limit reachedBugbot uses usage-based billing for this team and has hit its on-demand spend limit. A team admin can raise the spend limit in the Cursor dashboard, or wait for the next billing cycle to continue. |
📝 WalkthroughWalkthroughAlert presentation across application, content, sidebar, and workspace flows now uses ChangesWorkspace alert presentation
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested reviewers: Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 inconclusive)
✅ Passed checks (24 passed)
✨ 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 |
Greptile SummaryThis PR attaches workspace rename and color alerts to the window that owns the workspace. The main changes are:
Confidence Score: 5/5This looks safe to merge.
Important Files Changed
Reviews (3): Last reviewed commit: "Set alert input focus via willPresent in..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| _ = alert.runCmuxModal( | ||
| presentingWindow: AppDelegate.shared?.mainWindowContainingWorkspace(workspaceId) | ||
| ) |
There was a problem hiding this comment.
When an invalid color such as ZZZZZZ is submitted, this alert is requested immediately after the color sheet's modal loop returns. If the outgoing sheet is still attached during dismissal, runCmuxModal sees an occupied host and falls back to runModal(), so the validation alert can appear detached on another monitor.
Rule Used: Flag Swift fixes that patch symptoms while leaving... (source)
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
There was a problem hiding this comment.
The invalid-color alert presents after NSApp.runModal returns, which is strictly after the first sheet's didEnd completion ran; AppKit detaches the sheet before that completion (the shipped pattern in AppDelegate+WorkspaceActionSave.swift presents a second sheet directly inside the first's completion). If AppKit ever left attachedSheet set at that instant, runCmuxModal falls back to app-modal, which is the pre-fix behavior, so the alert stays functional.
— Claude Code
There was a problem hiding this comment.
You're right. NSApp.runModal returns only after the sheet's didEnd completion has fully run and AppKit has already detached the sheet — the window's attachedSheet is nil by the time the invalid-color alert is requested. My concern about a race with an occupied host was unfounded. The existing pattern in AppDelegate+WorkspaceActionSave.swift confirms this is the established idiom. I'll withdraw the comment.
| _ = alert.runCmuxModal( | ||
| presentingWindow: AppDelegate.shared?.mainWindowContainingWorkspace(tab.id) | ||
| ) |
There was a problem hiding this comment.
When the AppKit sidebar color prompt receives an invalid value, it requests this alert immediately after the first sheet's modal loop returns. If that sheet remains attached during dismissal, runCmuxModal takes its bare runModal() fallback and the validation alert can appear on the wrong monitor.
Rule Used: Flag Swift fixes that patch symptoms while leaving... (source)
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
There was a problem hiding this comment.
Same rationale as the ContentView thread: the second alert presents after runModal returns (post-didEnd, sheet already detached, matching the production second-sheet pattern in AppDelegate+WorkspaceActionSave.swift), and the appModal fallback keeps the alert functional in the worst case.
— Claude Code
Match the sidebar group-rename prompt's focus dance: initialFirstResponder alone was reliable under app-modal runModal, but sheet presentation keys the sheet on the host window's schedule, so also make the field first responder and select its text on the next main-loop turn. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Bugbot is paused — on-demand spend limit reachedBugbot uses usage-based billing for this team and has hit its on-demand spend limit. A team admin can raise the spend limit in the Cursor dashboard, or wait for the next billing cycle to continue. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Sources/Sidebar/AppKitList/Cells/SidebarWorkspaceRowCommands.swift`:
- Around line 163-171: Replace the DispatchQueue.main.async focus workaround in
promptRename with the runCmuxModal willPresent callback, moving
makeFirstResponder and selectText into that closure. Apply the same change in
promptCustomColor; update both affected locations in
Sources/Sidebar/AppKitList/Cells/SidebarWorkspaceRowCommands.swift (lines
163-171 and 203-211) while preserving the existing modal behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: a617a099-1361-48de-acb8-7f494130e94a
📒 Files selected for processing (1)
Sources/Sidebar/AppKitList/Cells/SidebarWorkspaceRowCommands.swift
CodeRabbit and the cmux pre-merge checks flagged the DispatchQueue.main.async focus dance as a banned timing-repair pattern. runCmuxModal already exposes a synchronous willPresent hook that fires just before the modal session begins, so set makeFirstResponder and selectText there in all six touched alert sites (including the four pre-existing async hops in the functions this PR already changes). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Bugbot is paused — on-demand spend limit reachedBugbot uses usage-based billing for this team and has hit its on-demand spend limit. A team admin can raise the spend limit in the Cursor dashboard, or wait for the next billing cycle to continue. |
1 participant
The Rename Workspace alert always appeared on a different monitor than the cmux window on multi-monitor setups. It was presented with bare
NSAlert.runModal(), which creates a detached alert window that macOS centers on whichever screen it considers main rather than the screen showing cmux.All affected alerts now go through the existing
runCmuxModal(presentingWindow:)presenter, resolving the host window withmainWindowContainingWorkspace, so each alert attaches as a sheet to the window that owns the workspace (also correct with multiple cmux windows open). Migrated call sites:SidebarWorkspaceRowCommands.promptRename(AppKit sidebar context menu, the reported path)TabItemView.promptRename(SwiftUI sidebar parity path)AppDelegate.promptRenameSelectedWorkspace(rename keyboard shortcut)Workspace.promptRenamePanel(Rename Tab alert)No new user-facing strings; all localization keys unchanged. No automated regression test: sheet-vs-detached presentation of a blocking modal has no practical unit-level assertion, verification is via tagged-build dogfood.
🤖 Generated with Claude Code
Need help on this PR? Tag
/codesmithwith what you need. Autofix is disabled.Summary by cubic
Attach workspace rename and color alerts to their owning window as sheets so they appear on the correct monitor. Also focus and select the input on open using the modal’s willPresent hook.
Written for commit 495b05f. Summary will update on new commits.
Summary by CodeRabbit