fix: close selected canvas nodes and confirm macOS quit - #410
Merged
Conversation
# Conflicts: # CHANGELOG.md
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
💡 Change Scope
📝 What Does This PR Do?
Fixes #405. Cmd+W on macOS and Ctrl+W on Windows/Linux close the node receiving keyboard focus in the active canvas, including terminal/Agent and editor input without canvas selection. That focused node takes priority over stale canvas selection. Without node focus, one selected node is closed; ambiguous multiselection shows an in-app warning. With neither focus nor selection, the shortcut does nothing and the application stays open.
Cmd+Q on macOS requires two non-repeat presses within 1.5 seconds and shows a localized confirmation hint after the first press. Explicit menu Quit remains available; Windows/Linux Ctrl+Q retains its existing terminal semantics.
🏗️ Large Change Spec (Required if "Large Change" is checked)
1. Context & Business Logic
Application shortcuts are reserved at Electron's input boundary before native accelerators and terminal/page input. Chromium's deliberate quit confirmation is adapted to Electron: actual macOS Cmd+Q input can omit keyup, so confirmation uses two non-repeat keydowns.
Terminal body and editor clicks intentionally establish input focus without canvas selection. Close therefore resolves actual DOM focus in the active canvas first, then falls back to a single selected node. It never mutates global selection to make the shortcut work. Hidden nodes and focus in another canvas are excluded. Documents retain their existing save/conflict close flow for both focused and selected targets.
2. State Ownership & Invariants
Main owns transient quit confirmation and lifecycle cancellation. Renderer owns DOM focus, selection, and existing node close operations. Preload validates the one-way semantic event contract.
Cancellation emits IPC only when a hint is pending. Cleanup retains the original WebContents reference so a destroyed BrowserWindow is not dereferenced from its closed callback. No executable-rule impact.
3. Verification Plan & Regression Layer
Final input-focus correction:
OPENCOVE_REQUIRE_STAGED=1 pnpm pre-commitexited 0 on the final four staged files. Related tests: 33 passed; native terminal recovery: 8 passed; full macOS Electron E2E: 326 passed, 83 skipped (22.2 minutes), with zero retries and no crash fallback. No source changes occurred during this successful gate.Focused unit regressions first failed against the old selection requirement, then passed after the correction. Coverage includes no selection, stale single/multiple selection, external-canvas focus, hidden nodes, disabled canvas, document focus/selection save delegation, duplicate closes, and late failures.
Three targeted close E2E cases passed after a fresh build (7.2 seconds), then passed again in the full gate: clicking and typing in an xterm textarea or note textarea with zero selected canvas nodes, plus existing empty-canvas/selected-node behavior. These tests use real UI clicks and typed input, then Electron
webContents.sendInputEventto simulate the native Main input stream and assertbefore-input-eventdelivery. They do not claim physical keyboard verification. Page-level Playwright keyboard injection did not exercise this Main shortcut path. The macOS double-press quit E2E also passed in the full gate.Latest-head CI is green for
b89674192aa18b9e7ca393aa69637c41296f535f(run). Windows passed 47 E2E tests / 4 skipped, explicitly including terminal and note input-focus Ctrl+W cases and empty-canvas/selected-node behavior. Linux modifier policy remains covered by unit tests. The initial local attempt stopped at a new test-variable lint error, corrected before the successful full gate.The first CI attempt failed two existing macOS tests: a sidebar width sample near the end of its CSS transition and a drag fault-injection pending-state check across two asynchronous evaluations. Both passed once locally after fresh builds with zero retries; the failed CI shards then passed on one rerun without code changes. These initial failures are retained here rather than counted as a clean first CI attempt. The branch was synchronized with main; only CHANGELOG required manual conflict resolution, preserving all entries.
✅ Delivery & Compliance Checklist
pnpm pre-commitis completely green.CLA.md).DEVELOPMENT.mdarchitectural boundaries.📸 Screenshots / Visual Evidence
Playwright assertions cover actual terminal/note input focus, target removal, sibling/app survival, and confirmed quit behavior. No screenshot is attached and no review-only media is committed. This PR remains unmerged for user testing.