Skip to content

Close confirmations: one tab defaults to Close, more than one to Cancel - #489

Merged
BenevolentFutures merged 3 commits into
mainfrom
close-tab-default
Oct 1, 2026
Merged

BenevolentFutures merged 3 commits into
mainfrom
close-tab-default

Conversation

@BenevolentFutures

@BenevolentFutures BenevolentFutures commented Oct 1, 2026 •

Copy link
Copy Markdown
Contributor

Atin's ask: closing one tab should default to Close; closing more than one tab should default to Cancel. Since #469 every destructive close card started on Cancel, which made the most common close take an extra keystroke.

Return now closes on c11's close card when the close takes one tab

  • Close tab? (Cmd+W, the tab X, Ghostty's runtime close)
  • Closing an area that holds one tab
  • Close workspace? for an unpinned workspace holding one tab (Cmd+W on a last tab routes here)

Unchanged: closes of more than one tab and pinned workspaces start on Cancel. Cmd+D never accepts a destructive card. The NSAlert dialogs (Close window?, Close other tabs, the legacy fallback alert) keep their existing default.

Mechanism: ConfirmContent.defaultSelection (nil keeps the role-derived default); WorkspaceCloseInteractionRuntime honours it on present.

Tests: one unit test: a single-tab close card starts on Close and Return confirms it.

🤖 Generated with Claude Code

Since close safety (#469) every destructive close card started on Cancel,
so closing a single tab took an arrow key or a click. That is the most
common close, and the extra step costs more than it saves. Now Return
closes whenever the close takes one tab:

- Close tab? (pane card, Ghostty's runtime close, and the legacy alert)
- closing an area that holds one tab
- Close other tabs when it closes one tab
- Close workspace? for an unpinned workspace holding one tab (Cmd+W on a
  last tab goes through this)
- Close window? for a window holding one such workspace

Anything that closes more than one tab, and a pinned workspace, still
starts on Cancel. Cmd+D still never accepts a destructive card.

ConfirmContent gains defaultSelection (nil keeps the role-derived
default), and the workspace-close runtime honours it on present.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@github-actions github-actions Bot added the drawbridge:review Drawbridge: routed to maintainer review label Oct 1, 2026
The NSAlert paths (Close window?, Close other tabs, the legacy close-tab
fallback) keep their existing Cancel default; AppKit re-assigns alert key
equivalents at present time, so the default belongs on the cards only.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@github-actions

github-actions Bot commented Oct 1, 2026 •

Copy link
Copy Markdown
Contributor

Deep review — #489 "Close confirmations: one tab defaults to Close, more than one to Cancel"

Thanks for this — it's a tight, well-scoped follow-up to #469, and the PR description's enumeration of exactly which flows change (and which deliberately stay on Cancel) made the review fast. The mechanism is small and the invariant that actually matters — destructive cards can't be accepted by a reflexive Cmd+D — is preserved.

Summary

The PR threads an optional ConfirmContent.defaultSelection (nil = keep the role-derived default) through both overlay runtimes — AreaInteractionRuntime honours it in initialSelection(for:) (so present and the queued-advance path both pick it up), and WorkspaceCloseInteractionRuntime.present reads it directly. Single-tab close entry points opt in to .confirm; multi-tab and pinned-workspace closes fall through to Cancel. My read: the logic is correct and consistent across every close path I traced.

Alignment

In scope. This is a UX-correctness fix on c11's own overlay surfaces implementing the maintainer's explicit ask — no tenant-config writes, no upstream-shared code, no typing-latency hot paths, no new user-facing strings, no CLI/socket/skill surface touched. It belongs here.

What I verified in context

  • The "one tab" predicate is the right notion per scope. workspaceCloseTakesOneTab keys on !isPinned && panels.count == 1, and panels is the flat [UUID: any TabContent] surface map (Workspace.swift:5370), so it genuinely means "this workspace holds one tab." The pane path uses bonsplitTabs.count == 1; presentConfirmClose ("Close tab?") is always a single surface — both its callers (Workspace.swift:11528, WorkspaceManager.swift:3104) close exactly one tab/surface, so the unconditional defaultSelection: .confirm there is sound.
  • Multi-item paths stay on Cancel. The single-workspace close routes closeWorkspacesWithConfirmation → closeWorkspaceWithConfirmation → closeWorkspaceIfRunningProcess (WorkspaceManager.swift:2980), which now passes defaultsToClose: workspaceCloseTakesOneTab(workspace). The multi-workspace call at WorkspaceManager.swift:2796 does not pass the flag, so it nils → Cancel. Correct.
  • The destructive guard survives. Single-tab cards stay role: .destructive, so hasActiveDestructiveConfirm is still true and Cmd+D can't accept them — testSingleTabCloseStartsOnClose asserts exactly that.
  • Queued cards honour the new default. advance() re-derives selection through initialSelection(for:), which reads content.defaultSelection, so a queued single-tab close picks up .confirm when it becomes active. (Behaviourally covered, though not by a test in this diff — see finding 2.)
  • No prompt-injection content in the diff or comments.

Findings

1. (minor — your call) Single-tab area close is .criticalDestructive and now Return-defaults to Close. In didRequestClosePane (Sources/Workspace.swift:13234) the card is unconditionally .criticalDestructive (the red-glow, "don't fat-finger this" treatment), while the new defaultSelection: bonsplitTabs.count == 1 ? .confirm : nil makes Return fire that destructive button for the one-tab case. Note this path closes the whole area/pane, not just a tab — when the pane holds one tab it's internally consistent with "one tab → Close," but the emphasised critical styling and the reflexive-Return default now pull in opposite directions. Consider either dropping to .standard style when bonsplitTabs.count == 1, or leaving single-tab area closes on Cancel, so the visual weight and the default button agree. Not a blocker — just the one spot where "close one tab → Close" and "this is styled to look scary on purpose" overlap.

2. (nit) New non-overlay behaviour has no unit coverage. The only added test exercises AreaInteractionRuntime with an explicit defaultSelection. The two new decision seams — workspaceCloseTakesOneTab (the !isPinned && panels.count == 1 predicate, including the pinned-workspace exclusion) and WorkspaceCloseInteractionRuntime.present reading defaultSelection — and the queued-advance re-derivation aren't asserted anywhere. workspaceCloseTakesOneTab is a pure static function and trivially unit-testable; a one-liner asserting pinned → false and single-unpinned → true would lock in the exact behaviour the maintainer asked for. Flagging only so the gap is deliberate rather than accidental.

Recommendation

Merge as-is. The logic is correct and the destructive-accept invariant holds across every path I traced; finding 1 is an optional UX-polish judgment the maintainer can weigh against the explicit "one tab → Close" directive, and finding 2 is a cheap-but-optional coverage add. Nice, careful work.


🌉 Drawbridge deep review (lane 3) — an automated pass; a maintainer has been notified and makes the final call.


🌉 Drawbridge deep review (lane 3) — an automated pass; a maintainer has been notified and makes the final call.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@BenevolentFutures
BenevolentFutures merged commit 89f7eca into main Oct 1, 2026
11 checks passed
@BenevolentFutures
BenevolentFutures deleted the close-tab-default branch October 1, 2026 21:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

drawbridge:review Drawbridge: routed to maintainer review

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant