Bound overflowing confirmation dialog content - #8296
Conversation
Greptile SummaryThis PR introduces
Confidence Score: 5/5Safe to merge; the alert-content bounding logic is well-contained AppKit work that gracefully degrades to native presentation for short text. All changed paths are modal-alert presentation code on the main actor; the new measurement and scroll-view construction is synchronous AppKit layout with no persistence or concurrency side-effects. Previous review concerns (duplicate layout pass, wrong occurrence removal) are resolved. The one inconsistency is a single missed migration in the resume-command approval dialog — it continues to use runModal() directly as it did before this PR, so no regression is introduced. Sources/TerminalController+ControlSurfaceContext4.swift — uses content.apply + runModal() directly while all other migrated callers now use runCmuxModal(content:). Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A["alert.runCmuxModal(content:)"] --> B["content.apply(to: alert, presentingWindow:)"]
B --> C{scrollableDetails?}
C -- yes --> D["CmuxAlertScrollableDetailsView(flattenedText)"]
D --> E{isContentHeightCapped?}
E -- no --> F["alert.informativeText = flattenedText\n(native layout)"]
E -- yes --> G{informativeText empty?}
G -- yes --> H["accessoryView = flattenedMeasurement\n(scroll all)"]
G -- no --> I["CmuxAlertScrollableDetailsView(summary)\ncheck 20% budget"]
I --> J{summaryOverflows?}
J -- yes --> H
J -- no --> K["informativeText = summary\naccessoryView = scrollableDetails view"]
C -- no --> L["CmuxAlertScrollableDetailsView(informativeText)"]
L --> M{isContentHeightCapped?}
M -- no --> F
M -- yes --> N["informativeText = ''\naccessoryView = overflowView"]
B --> O["resolve hostWindow\nactivate app"]
O --> P{hostWindow & no attached sheet?}
P -- yes --> Q["beginSheetModal"]
P -- no --> R["runModal (app-modal)"]
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
flowchart TD
A["alert.runCmuxModal(content:)"] --> B["content.apply(to: alert, presentingWindow:)"]
B --> C{scrollableDetails?}
C -- yes --> D["CmuxAlertScrollableDetailsView(flattenedText)"]
D --> E{isContentHeightCapped?}
E -- no --> F["alert.informativeText = flattenedText\n(native layout)"]
E -- yes --> G{informativeText empty?}
G -- yes --> H["accessoryView = flattenedMeasurement\n(scroll all)"]
G -- no --> I["CmuxAlertScrollableDetailsView(summary)\ncheck 20% budget"]
I --> J{summaryOverflows?}
J -- yes --> H
J -- no --> K["informativeText = summary\naccessoryView = scrollableDetails view"]
C -- no --> L["CmuxAlertScrollableDetailsView(informativeText)"]
L --> M{isContentHeightCapped?}
M -- no --> F
M -- yes --> N["informativeText = ''\naccessoryView = overflowView"]
B --> O["resolve hostWindow\nactivate app"]
O --> P{hostWindow & no attached sheet?}
P -- yes --> Q["beginSheetModal"]
P -- no --> R["runModal (app-modal)"]
Reviews (7): Last reviewed commit: "Use shared alert window resolution" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
|
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:
📝 WalkthroughWalkthroughAdds shared structured alert content and bounded, selectable scrolling views. Confirmation, resume, process, workspace, and configuration dialogs now use screen-aware sizing, while close-workspace flows propagate formatted details and validate visible controls. ChangesAlert content and sizing
Modal and confirmation integration
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant CloseWorkflow
participant TabManager
participant NSAlert
participant CmuxAlertContent
participant CmuxAlertScrollableDetailsView
CloseWorkflow->>TabManager: submit message and scrollableDetails
TabManager->>NSAlert: run structured confirmation
NSAlert->>CmuxAlertContent: apply using screen geometry
CmuxAlertContent->>CmuxAlertScrollableDetailsView: create capped details view
CmuxAlertScrollableDetailsView->>NSAlert: install scrollable accessory
Possibly related issues
Possibly related PRs
Suggested reviewers: Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (2 errors, 1 warning)
✅ Passed checks (22 passed)
✨ Finishing Touches🧪 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/TerminalController+ControlSurfaceContext4.swift (1)
134-149: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winReorder localized string placeholders to match accessory view layout.
When
binding.commandis extracted into the scrollable accessory view byCmuxAlertContent, it is removed from theinformativeTextand rendered natively at the bottom of theNSAlert. Because the command is currently placed before the working directory in the localized string, extracting it inverts the reading order (the "Working directory" line will appear above the command).Consider updating the localized strings and format arguments so the visual order matches the text when the accessory view is used.
Sources/TerminalController+ControlSurfaceContext4.swift#L134-L149: ReordersurfaceResumeApproval.proposal.messageso the command placeholder is at the end.Sources/Workspace.swift#L2491-L2505: ReordersurfaceResumeApproval.runPrompt.messageso the command placeholder is at the end.🤖 Prompt for 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. In `@Sources/TerminalController`+ControlSurfaceContext4.swift around lines 134 - 149, Reorder the command and working-directory placeholders in the localized message format and their arguments so the command appears last, matching CmuxAlertContent’s separatingScrollableDetails layout. Apply this to Sources/TerminalController+ControlSurfaceContext4.swift:134-149 for surfaceResumeApproval.proposal.message and Sources/Workspace.swift:2491-2505 for surfaceResumeApproval.runPrompt.message; preserve the existing displayed content and localization keys.
🤖 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/Panels/BrowserWebExtensionSupport`+WKWebExtensionControllerDelegate.swift:
- Around line 394-397: Update confirmPermissionRequest so the nil
scrollableDetails fallback uses CmuxAlertContent(informativeText:
informativeText) instead of CmuxAlertContent.scrollingAll(informativeText),
while preserving the existing separated-details path.
In `@Sources/TaskManagerWindowController.swift`:
- Around line 245-256: Replace the String(format:) calls in the single-process
and multi-process kill-message branches with String.localizedStringWithFormat,
preserving the existing localized strings and arguments so process IDs and
counts use the user’s locale.
---
Outside diff comments:
In `@Sources/TerminalController`+ControlSurfaceContext4.swift:
- Around line 134-149: Reorder the command and working-directory placeholders in
the localized message format and their arguments so the command appears last,
matching CmuxAlertContent’s separatingScrollableDetails layout. Apply this to
Sources/TerminalController+ControlSurfaceContext4.swift:134-149 for
surfaceResumeApproval.proposal.message and Sources/Workspace.swift:2491-2505 for
surfaceResumeApproval.runPrompt.message; preserve the existing displayed content
and localization keys.
🪄 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: 3099699c-0e66-4f03-baca-13e0f2fce2e4
📒 Files selected for processing (17)
Sources/AppDelegate+WorkspaceActionSave.swiftSources/CmuxAlertContent.swiftSources/CmuxAlertScrollableDetailsView.swiftSources/CmuxConfigExecutor.swiftSources/CmuxModalAlertPresentation.swiftSources/DockSplitStore+CloseConfirmation.swiftSources/Panels/BrowserWebExtensionSupport+WKWebExtensionControllerDelegate.swiftSources/TabManager.swiftSources/TaskManagerWindowController.swiftSources/TerminalController+ControlSurfaceContext4.swiftSources/Workspace.swiftSources/WorkspaceActionSaveDialogAccessory.swiftSources/WorkspaceCloseTabsBatching.swiftcmux.xcodeproj/project.pbxprojcmuxTests/CmuxAlertContentTests.swiftcmuxTests/WorkspaceActionSaveDialogAccessoryTests.swiftcmuxUITests/CloseWorkspacesConfirmDialogUITests.swift
|
CodeRabbit outside-diff resume-command ordering finding: no code change. The overflow layout intentionally keeps fixed metadata—including the working directory—in the native informative text while the user-sized command moves into the accessory below it. Reordering the localization placeholders would also change the existing compact/native dialog order, even though compact content does not need an accessory. The overflow order is therefore an intentional consequence of keeping fixed context visible, not a lost or incorrect value. |
|
Follow-up to the earlier CodeRabbit resume-order reply: after maintainer triage, addressed in 4108017. Both resume-prompt formats now place the localized working-directory line before the command placeholder, their format arguments match that order, and the English/Japanese catalog values were updated together. This preserves reading order when the command moves into the accessory view. |
…-8295-close-dialog-scroll # Conflicts: # Sources/Panels/BrowserWebExtensionSupport+WKWebExtensionControllerDelegate.swift
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. |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
Sources/Workspace.swift (3)
11298-11307: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winCheck for
attachedSheetto prevent queuing sheets behind existing dialogs.To maintain consistent behavior with
runCmuxModalAlert, ensure the window does not already have an attached sheet before attemptingbeginSheetModal. Without this check, the close confirmation may queue invisibly behind an existing sheet.💚 Proposed fix
let content = CmuxAlertContent(informativeText: message) // Prefer a sheet if we can find a window, otherwise fall back to modal. - if let window = NSApp.keyWindow ?? NSApp.mainWindow { + if let window = NSApp.keyWindow ?? NSApp.mainWindow, window.attachedSheet == nil { content.apply(to: alert, presentingWindow: window) return await withCheckedContinuation { continuation in alert.beginSheetModal(for: window) { response in🤖 Prompt for 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. In `@Sources/Workspace.swift` around lines 11298 - 11307, Update the sheet presentation branch in the alert flow to check the selected window’s attachedSheet before calling beginSheetModal. If a sheet is already attached, skip the sheet path and allow the existing modal fallback behavior, matching runCmuxModalAlert.
1871-1886: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winReplace legacy
DispatchQueue.main.asyncAfterwith modern Swift concurrency.As per path instructions, avoid introducing new
DispatchQueue.main.asyncAfteror callback pyramids whenTaskandTask.sleepare appropriate. Since the timeout handler already hops into aTask {@mainactor... }, you can remove the GCD wrapper and handle the delay directly within theTask.♻️ Proposed refactor
guard let timeout else { return } - DispatchQueue.main.asyncAfter(deadline: .now() + timeout) { [weak self, registration] in - Task { `@MainActor` [weak self, registration] in - guard - let self, - self.hasPendingTerminalInputObserver(registration, forPanelId: panelId) - else { - return - } - - self.removePendingTerminalInputObserver(registration, forPanelId: panelId) - `#if` DEBUG - NSLog("[CmuxConfig] surface not ready after 3s, dropping command (%d chars)", text.count) - `#endif` - } + Task { `@MainActor` [weak self, registration] in + try? await Task.sleep(nanoseconds: UInt64(timeout * 1_000_000_000)) + guard + let self, + self.hasPendingTerminalInputObserver(registration, forPanelId: panelId) + else { + return + } + + self.removePendingTerminalInputObserver(registration, forPanelId: panelId) + `#if` DEBUG + NSLog("[CmuxConfig] surface not ready after 3s, dropping command (%d chars)", text.count) + `#endif` }🤖 Prompt for 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. In `@Sources/Workspace.swift` around lines 1871 - 1886, Replace the DispatchQueue.main.asyncAfter wrapper in the timeout handling with a Task-based delay using Task.sleep, keeping the existing `@MainActor` task and observer validation/removal logic in the timeout handler. Preserve weak self and registration capture, and return cleanly if the sleep is cancelled or throws.Source: Path instructions
42-44: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winProvide a safety explanation for
@unchecked Sendableor use@MainActor.As per coding guidelines, do not mark shared mutable reference types as
@unchecked Sendablewithout a clear safety explanation. IfWorkspacePendingTerminalInputObserveris only mutated on the main thread, apply explicit@MainActorisolation instead to inheritSendablesafely without disabling compiler checks.🤖 Prompt for 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. In `@Sources/Workspace.swift` around lines 42 - 44, Update WorkspacePendingTerminalInputObserver to use `@MainActor` isolation if all accesses to its mutable observer property occur on the main thread, and adjust callers as needed to preserve that isolation. Otherwise, retain `@unchecked` Sendable only with a clear safety explanation documenting synchronization for observer access.Source: Coding guidelines
🤖 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.
Outside diff comments:
In `@Sources/Workspace.swift`:
- Around line 11298-11307: Update the sheet presentation branch in the alert
flow to check the selected window’s attachedSheet before calling
beginSheetModal. If a sheet is already attached, skip the sheet path and allow
the existing modal fallback behavior, matching runCmuxModalAlert.
- Around line 1871-1886: Replace the DispatchQueue.main.asyncAfter wrapper in
the timeout handling with a Task-based delay using Task.sleep, keeping the
existing `@MainActor` task and observer validation/removal logic in the timeout
handler. Preserve weak self and registration capture, and return cleanly if the
sleep is cancelled or throws.
- Around line 42-44: Update WorkspacePendingTerminalInputObserver to use
`@MainActor` isolation if all accesses to its mutable observer property occur on
the main thread, and adjust callers as needed to preserve that isolation.
Otherwise, retain `@unchecked` Sendable only with a clear safety explanation
documenting synchronization for observer access.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 0641372c-e367-4d68-b5cb-494981766b5d
📒 Files selected for processing (4)
Resources/Localizable.xcstringsSources/DockSplitStore+CloseConfirmation.swiftSources/TabManager.swiftSources/Workspace.swift
💤 Files with no reviewable changes (1)
- Sources/DockSplitStore+CloseConfirmation.swift
|
Review follow-up for 0b3a8ab:
Parser/typecheck, project/test wiring, determinism, package policy, and budget checks pass. Focused CI and canonical autoreview are running on this commit. |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Sources/Workspace.swift (1)
11298-11311: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsolidate window resolution for modal presentation.
This PR introduces
NSApp.cmuxMainWindowForModalPresentation()which provides robust resolution by filtering out auxiliary windows like settings panels. Consider using it here instead of the rawkeyWindow ?? mainWindowfallback to align perfectly with the rest of the dialogs.♻️ Proposed refactor
let content = CmuxAlertContent(informativeText: message) // Prefer a sheet if we can find a window, otherwise fall back to modal. - if let window = NSApp.keyWindow ?? NSApp.mainWindow, + if let window = NSApp.cmuxMainWindowForModalPresentation(), window.attachedSheet == nil { content.apply(to: alert, presentingWindow: window) return await withCheckedContinuation { continuation in🤖 Prompt for 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. In `@Sources/Workspace.swift` around lines 11298 - 11311, Update the modal presentation window lookup in the alert flow to use NSApp.cmuxMainWindowForModalPresentation() instead of the raw keyWindow ?? mainWindow fallback. Preserve the existing sheet presentation when a suitable window without an attached sheet is returned, and retain the modal fallback otherwise.
🤖 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.
Outside diff comments:
In `@Sources/Workspace.swift`:
- Around line 11298-11311: Update the modal presentation window lookup in the
alert flow to use NSApp.cmuxMainWindowForModalPresentation() instead of the raw
keyWindow ?? mainWindow fallback. Preserve the existing sheet presentation when
a suitable window without an attached sheet is returned, and retain the modal
fallback otherwise.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 3fb9d57e-5ba2-4fd2-ae1f-e66e6acfd62f
📒 Files selected for processing (10)
Sources/CmuxAlertContent.swiftSources/CmuxModalAlertPresentation.swiftSources/ContentView+SavedLayoutCommands.swiftSources/DockSplitStore+CloseConfirmation.swiftSources/QuitConfirmationAlertPresenter.swiftSources/SidebarWorkspaceGroupDialogs.swiftSources/TabManager.swiftSources/TaskManagerWindowController.swiftSources/Workspace.swiftcmuxTests/CmuxAlertContentTests.swift
|
Follow-up review fix in 0d07477: the async Workspace close prompt now resolves its host through NSApp.cmuxMainWindowForModalPresentation(), then verifies attachedSheet is nil before beginning the sheet. This keeps auxiliary windows out of the path and shares the same host-selection policy as the rest of the alert presenter. |
|
Final automated-review triage for the current HEAD:
All inline review threads are resolved. The final canonical autoreview is clean (confidence 0.90), including the merge-conflict gate and cmux policy checks. |
1 participant
Summary
CmuxAlertContentpolicy that keeps compact alerts native but moves overflowing user-sized details into a selectable internal scroll view capped at 40% of the presenting screensizeToFit()and reuse the measured overflowing view as the presented accessory, keeping measurement and presentation in syncThis fixes #8295.
Closes #8295
Regression coverage
PONG; these failures are not presented as red-regression evidenceValidation
CmuxAlertContentTests: 4/4 passed on final HEAD018e3d1ca2in Actions run 29544286373WorkspaceActionSaveDialogAccessoryTests: passed in Actions run 29542004606xcrun swiftc -typecheckfor the new alert types./scripts/check-pbxproj.sh./scripts/lint-pbxproj-test-wiring.sh(534 test files)python3 scripts/check-test-determinism.pyPackage.resolvedpolicy checks.github/swift-warning-budget.tsv; every new Swift file remains below 500 linesPer the issue instructions, I did not run local
xcodebuildtests or a local app build. The requestedpython3 scripts/swift_file_length_budget.pycheck could not run because that script and.github/swift-file-length-budget.tsvdo not exist on currentmain; no budget file was created or modified.Summary by CodeRabbit
New Features
Bug Fixes
Tests