Skip to content

Fix Return key for machine deletion confirmation - #16683

Merged
austinywang merged 1 commit into
mainfrom
issue-delete-machine-enter
Oct 2, 2026
Merged

austinywang merged 1 commit into
mainfrom
issue-delete-machine-enter

Conversation

@austinywang

@austinywang austinywang commented Oct 2, 2026 •

Copy link
Copy Markdown
Contributor

Problem

The machine deletion warning sheet does not consistently activate Delete when the user presses Return, so keyboard-only confirmation can appear unresponsive.

Change

Bind Return to the destructive Delete button, make it the alert's default button and initial responder, and bind Escape to Cancel. This uses AppKit's explicit alert key handling so sheet presentation and modal presentation behave the same.

Validation

  • python3 scripts/verify-local.py

Changelog

Fixed machine deletion confirmation so Return activates Delete.

— unregistered


View with [code]smith Autofix with [code]smith
Need help on this PR? Tag @codesmith-bot with what you need. Autofix is disabled.


Summary by cubic

Fixes the machine deletion confirmation so pressing Return always activates Delete, even when the alert is presented as a sheet. Also binds Escape to Cancel.

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

Review in cubic

Summary by CodeRabbit

  • Bug Fixes
    • The delete confirmation now responds consistently to keyboard input: Return selects Delete, while Escape selects Cancel. The Delete button is also focused when the confirmation appears.

@github-actions

github-actions Bot commented Oct 2, 2026

Copy link
Copy Markdown
Contributor

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

@coderabbitai

coderabbitai Bot commented Oct 2, 2026 •

Copy link
Copy Markdown

Review in Change Stack →

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

🧰 Additional context used
📚 Code guidelines (2)
.github/review-bot-rules/swift-architectural-rethink.md — configured
.github/review-bot-rules/source-control-artifacts.md — configured

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: 8d1038ef-0c99-4b4e-8440-4804eea3a368

📥 Commits

Reviewing files that changed from the base of the PR and between b415d22 and fed38c4.

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

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


📝 Walkthrough

Walkthrough

The delete confirmation now binds Return to the destructive Delete button and Escape to Cancel. It also sets Delete as the default button and initial first responder.

Changes

Delete confirmation

Layer / File(s) Summary
Delete confirmation keyboard controls
Sources/Cloud/MachineRowActions.swift
The confirmation configures Delete as destructive, assigns it Return, and makes it the default button and initial first responder. Cancel receives Escape.

Priority: ⬇️ Low

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

Change: Bug fix

Suggested reviewers: teamleaderleo

Merge Risk: ⚪ Minimal · up to fed38

The confirmation now has explicit keyboard controls for Delete and Cancel. No actionable merge-blocking risk is established.

🚥 Pre-merge checks | ✅ 23 | ❌ 2

❌ Failed checks (2 warnings)

Check name Status Explanation Resolution
Description check ⚠️ Warning The description explains the problem, intended behavior, validation command, and changelog entry. However, it does not follow the required template fully: it omits the Testing and Demo Video sections,… Use the required section headings. Add a Testing section that states whether python3 scripts/verify-local.py passed and what remains unverified. Add a Demo Video section with a video or screenshots, or explain why none is available. Add the…
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 and concisely describes the main change: fixing Return-key handling for machine deletion confirmation.
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 PR changes only Sources/Cloud/MachineRowActions.swift, and the diff only updates presentDeleteConfirmation to bind Return/Escape and set AppKit alert responders. It does not change Cloud…
Cmux Swift Actor Isolation ✅ Passed PASS. The PR only changes AppKit alert configuration inside the existing @MainActor presentDeleteConfirmation method. It adds no model, service protocol, Sendable reference type, logger, or back…
Cmux Swift Blocking Runtime ✅ Passed The PR changes only Sources/Cloud/MachineRowActions.swift. The diff adds AppKit button key-equivalent and responder configuration for Delete and Cancel. It adds no semaphore, blocking wait, sleep, d…
Cmux Browser Automation Off-Main ✅ Passed PASS: The pull request changes only Sources/Cloud/MachineRowActions.swift. The diff updates NSAlert keyboard handling for machine deletion. It does not change browser socket commands, WebKit/AppKi…
Cmux Expensive Synchronous Load ✅ Passed PASS: The PR only changes Sources/Cloud/MachineRowActions.swift to configure NSAlert buttons and responders. It adds no agent-history loader, file read, JSON/JSONL parsing, directory scan, per-rec…
Cmux Cache Substitution Correctness ✅ Passed The PR only changes Sources/Cloud/MachineRowActions.swift to configure NSAlert button key equivalents, defaultButtonCell, and initialFirstResponder for delete confirmation. The diff does not r…
Cmux No Hacky Sleeps ✅ Passed PASS: The pull request changes only Sources/Cloud/MachineRowActions.swift, which is Swift and outside this check's scope. The diff adds explicit alert key handling and responder configuration. It do…
Cmux Algorithmic Complexity ✅ Passed PASS — The diff only adds key handling to presentDeleteConfirmation. It does not add loops, batch rescans, sorting, filtering, joins, or hot-path collection processing. The sole collection operation…
Cmux Swift Concurrency ✅ Passed PASS: The diff only changes NSAlert/NSButton keyboard and responder configuration in presentDeleteConfirmation. It adds no Dispatch queues, Combine state, new completion-handler APIs, or fire-and-fo…
Cmux Swift @Concurrent ✅ Passed The diff only changes synchronous NSAlert button and responder configuration inside the existing @MainActor presentDeleteConfirmation function. It introduces no async, nonisolated, `@concurr…
Cmux Swift Package Boundaries ✅ Passed PASS: The diff only updates Sources/Cloud/MachineRowActions.swift to configure NSAlert buttons and responders. The changed code is AppKit alert wiring for the machine deletion confirmation. It doe…
Cmux Swiftpm Lockfiles ✅ Passed PASS. The pull request changes only Sources/Cloud/MachineRowActions.swift. It does not modify a Package.swift, Package.resolved, .gitignore, workflow, Xcode project package reference, or depen…
Cmux Swift Logging ✅ Passed The PR changes only alert button and responder configuration in Sources/Cloud/MachineRowActions.swift. The added lines contain no print, debugPrint, dump, NSLog, ad hoc logging, Logger, or…
Cmux User-Facing Error Privacy ✅ Passed PASS: The changed code is in the user-facing machine deletion confirmation path (confirmDelete calls presentDeleteConfirmation), but the diff changes only AppKit keyboard and responder behavior. I…
Cmux Full Internationalization ✅ Passed PASS: The pull request changes only keyboard behavior for an existing localized delete confirmation. The existing Delete, Cancel, title, and message strings remain routed through `String(localized:def…
Cmux Swiftui State Layout ✅ Passed The diff changes only Sources/Cloud/MachineRowActions.swift. It updates an AppKit NSAlert confirmation by configuring NSButton key handling and responders. The file contains no new SwiftUI state…
Cmux Architecture Rethink ✅ Passed PASS. The diff is a small local AppKit correctness fix in presentDeleteConfirmation. It adds no timing repair, polling, lock, observer, mutable state, side channel, or second lifecycle owner. Return…
Cmux Swift Auxiliary Window Close Shortcuts ✅ Passed PASS. The PR changes keyboard handling on an existing NSAlert used as a machine deletion confirmation. The code presents it with beginSheetModal or runModal and does not add or materially change a sta…
Cmux Source Artifacts ✅ Passed The pull request changes only Sources/Cloud/MachineRowActions.swift. The diff contains hand-written Swift source that updates NSAlert keyboard handling. It does not add logs, screenshots, recordin…
Cmux No Test Or Debug Seam In Production Source ✅ Passed PASS. The PR changes only normal NSAlert keyboard behavior in Sources/Cloud/MachineRowActions.swift. It adds button key equivalents and responder configuration for the production delete confirmati…
Full details: Description check

Explanation

The description explains the problem, intended behavior, validation command, and changelog entry. However, it does not follow the required template fully: it omits the Testing and Demo Video sections, provides no test result or limitation, and omits the checklist, including the required UI localization and review confirmations.

Resolution

Use the required section headings. Add a Testing section that states whether python3 scripts/verify-local.py passed and what remains unverified. Add a Demo Video section with a video or screenshots, or explain why none is available. Add the applicable checklist confirmations, including the localization audit and review status.

  • Fix all pre-merge checks with AI
✨ 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
  • Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Autopilot is currently an internal CodeRabbit preview.


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.

@austinywang austinywang added the dev-build Build a fleet dogfood build of each push (newest head under load) label Oct 2, 2026
@github-actions

github-actions Bot commented Oct 2, 2026 •

Copy link
Copy Markdown
Contributor

Dogfood build of fed38c403b77fb406833e8270a36c3d192206f71

cmux DEV pr-16683-fed38c40.app

The link opens this exact commit in the cmux dev menu bar app; the page waits until the build is ready. Builds run only while this PR has the dev-build label. Under load the fleet builds the newest push each time a worker frees up, so some pushes are skipped. It signs in against production, so Cloud or backend changes still need a tagged build with a development backend.

Dogfood tours of fed38c40

cloud-machine-author-tour at fed38c40: not run

skipped: CI built this head on a runner pool whose products the UI test Macs cannot load, and media never compiles one; gh workflow run pr-media.yml -f pr=<n> -f allow_compile=true does

Tours are picked by the paths globs in dogfood/scenarios/*.json; a Dogfood-tours: a, b line in the description picks them instead (none turns this off). Look at every frame before merging: a green tour only means no step failed.

@austinywang
austinywang merged commit 22d59ac into main Oct 2, 2026
111 of 112 checks passed
@austinywang
austinywang deleted the issue-delete-machine-enter branch October 2, 2026 08:34
@github-actions

github-actions Bot commented Oct 2, 2026

Copy link
Copy Markdown
Contributor

Merge receipt for fed38c403b: every check was green at merge (16 verified; 17 skipped by policy). Full suite runs on main after merge.

rustybret pushed a commit to rustybret/bmux that referenced this pull request Oct 2, 2026
644fd5e Remove inline Open in cmux action from port rows (manaflow-ai#16350)
59821f4 fix: use weak var instead of weak let for macOS 26 / Swift 6 compat (manaflow-ai#9653)
e7a4e0a fix(cmux-tui): satisfy reconnect clippy lint (manaflow-ai#16758)
ee61823 fix(cloud): name the first machine workspace workspace-1 (manaflow-ai#16754)
8f28c09 test: isolate fake-socket CLI tests from the launching cmux shell (manaflow-ai#16562)
22d59ac Fix Return key for machine deletion confirmation (manaflow-ai#16683)
5049234 Cloud: 5 VMs per seat (4 vCPU/8 GB), Max 16 vCPU/32 GB, no free machines (manaflow-ai#16207)
13d77d6 fix: make dashboard team switching finish before refresh (manaflow-ai#16680)
3952ab3 Fix initial Cloud workspace layout restore (manaflow-ai#16690)
4ac2ec4 Fix optimistic selection for Cloud workspace creation (manaflow-ai#16672)
b34697f Remove Cloud agent star button (manaflow-ai#16700)

# Conflicts:
#	.github/workflows/ci-guards.yml
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

dev-build Build a fleet dogfood build of each push (newest head under load)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant