fix(terminal): map macOS Command+. to interrupt running command like Ctrl+C - #3409
netcatty-bot wants to merge 8 commits into
Conversation
netcatty-bot
added
automation:bot-pr
|
@codex review |
|
To use Codex here, create a Codex account and connect to github. |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a0ef88c085
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // macOS Terminal convention: ⌘. interrupts the running command like | ||
| // Ctrl+C (#3408). Only when nothing is selected so copy wins first. | ||
| const macCommandPeriodInterrupt = | ||
| !hasCopyableSelection && isMacPlatform() && isMacCommandPeriodInterruptChord(e); |
There was a problem hiding this comment.
When the active application has negotiated Kitty event reporting, or input is broadcast to a Kitty-enabled peer, this condition enters the Ctrl+C branch while retaining the original Command-period event. The branch records and broadcasts that event, so the local keyup can emit an unmatched Super+Period release, while a peer encodes Super+Period and suppresses the following legacy \x03 instead of being interrupted. Forward a normalized Ctrl+C event for Kitty bookkeeping and broadcast rather than the original chord.
Useful? React with 👍 / 👎.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 789a5ef2ec
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| } as unknown as KeyboardEvent | ||
| : e; | ||
| const kittyEvent = toKittyKeyboardEvent(interruptEventForKitty); | ||
| const identity = kittyKeyIdentity(interruptEventForKitty); |
There was a problem hiding this comment.
When Kitty event reporting is enabled locally or on a broadcast peer, this records the normalized press under KeyC, but the browser later delivers the physical release as Period; the keyup handler looks up only e.code || e.key, so it neither removes this entry nor sends the paired Ctrl+C release. Although the prior Super+Period mismatch is now normalized, the new identity mismatch leaves Ctrl+C logically held for Kitty consumers; retain a mapping from the physical chord to the normalized event or synthesize its release.
Useful? React with 👍 / 👎.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 671ce69519
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| kittyNormalizedPressAliases.delete(physicalIdentity); | ||
| return null; | ||
| } | ||
| return forwardedPress.event; |
There was a problem hiding this comment.
After a Command-period interrupt with local or broadcast Kitty release tracking, a successful lookup returns without deleting the Period -> KeyC alias. Until focus loss or reconnection, a later Period keyup while another KeyC press is outstanding—for example, overlapping c and . while typing in a report-all TUI—is therefore rewritten as a KeyC release, prematurely releasing C and leaving Period logically pressed until transient state is flushed. Consume the alias on the successful path as well as when no forwarded press is found.
Useful? React with 👍 / 👎.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Netcatty/components/terminal/runtime/createXTermRuntime.ts
Lines 2116 to 2120 in c0ed0c3
Keep normalized interrupt presses separate from physical KeyC
When a Kitty release-reporting session already has the physical C key held, pressing Command+Period upserts the normalized interrupt under the same KeyC entry and replaces that outstanding press (the broadcast map has the same collision). The Period keyup then releases and deletes this shared entry while physical C is still down, and the eventual C keyup has no press to pair with, producing premature or modifier-mismatched releases in report-all TUIs. The new alias deletion only fixes a later stale Period keyup; it does not prevent this simultaneous identity collision, so the normalized press needs independently keyed/stacked bookkeeping.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Carry the dedicated identity through broadcast pairing
With input broadcasting enabled, holding a physical C key and then pressing Command-period still collapses both presses on each peer because this dispatch sends only the normalized event whose code is KeyC; the new pressIdentity exists solely in the source-side maps, while resolveKittyKeyboardBroadcastInput() keys peer state from input.event.code. On a legacy peer the outstanding C entry can suppress the following urgent \x03 as duplicate, and on a Kitty peer the first release clears the shared pairing entry. Fresh evidence after the local alias fix is that no distinct correlation identity crosses this broadcast call, so it must be propagated and used by peer pairing.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const macCommandPeriodInterrupt = | ||
| !hasCopyableSelection && isMacPlatform() && isMacCommandPeriodInterruptChord(e); | ||
| if (urgentInterrupt || macCommandPeriodInterrupt) { |
There was a problem hiding this comment.
On macOS, a user can still assign Meta+. to a snippet or configurable terminal action, but this branch executes before both the snippet loop and checkAppShortcut() and returns after sending an interrupt. The snippet editors accept this chord because findActiveSystemShortcutConflict() only checks configured key bindings and this new hard-coded shortcut was not registered there, so the UI can save a shortcut that silently becomes unusable; either reject/reserve this chord during validation or give configured shortcuts precedence.
Useful? React with 👍 / 👎.
| releaseEvent = { | ||
| ...aliasedRelease.event, | ||
| type: "keyup", | ||
| } as unknown as KeyboardEvent; | ||
| aliasedReleaseIdentity = aliasedRelease.identity; |
There was a problem hiding this comment.
When the source terminal has negotiated Win32 input mode, broadcasting is enabled, and a physical KeyC remains held while Command-period is pressed, the Period keyup is rewritten here to the normalized KeyC event. The subsequent win32InputModeForwardedKeys.delete(identity) therefore consumes the outstanding physical C press and returns true, causing a premature native C release while the eventual real C keyup is dropped. Fresh evidence after the source-map alias fix is that the separate win32InputModeForwardedKeys lookup still uses the rewritten identity; preserve the physical e identity for that lookup and suppress the aliased interrupt's native keyup.
Useful? React with 👍 / 👎.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 15f209d24d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| key: "c", | ||
| code: "KeyC", |
There was a problem hiding this comment.
When macOS uses a non-QWERTY layout (for example, Dvorak), assigning the synthesized event code: "KeyC" makes toKittyKeyboardEvent() obtain that physical key's layout character and getUnicodeKeyCode() prioritizes it. The normalized interrupt can therefore encode as Ctrl+J (or another layout character) instead of Ctrl+C; a Kitty-enabled broadcast peer then suppresses the legacy \x03 fallback and is not interrupted. Construct the normalized Kitty event without unrelated layout metadata, or explicitly force its unshifted character to c.
Useful? React with 👍 / 👎.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1c1fcf3995
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| win32InputModePendingEvent = null; | ||
| win32InputModeForwardedKeys.clear(); | ||
| kittyForwardedKeys.clear(); | ||
| kittyNormalizedPressAliases.clear(); |
There was a problem hiding this comment.
When a source session reconnects between a Command-period keydown and its Period keyup, Terminal.tsx calls resetKittyConnectionInputState() while broadcastForwardedKeys is intentionally retained for pending peer releases. Clearing only this alias makes the later physical keyup unable to find the dedicated normalized identity, so broadcast Kitty/Win32 peers can retain Ctrl+C as logically pressed until another blur flushes the stale entry. Preserve aliases that still have a broadcast press, or flush those presses before clearing them.
Useful? React with 👍 / 👎.
| // can pair the release. | ||
| const kittyNormalizedPressAliases = new Map<string, string>(); | ||
| const kittyNormalizedPressIdentity = (identity: string): string => | ||
| `${identity} mac-period-interrupt`; |
There was a problem hiding this comment.
This template literal contains a literal NUL byte, which causes default repository text-search tooling to classify createXTermRuntime.ts as binary (rg reports only “binary file matches” and suppresses matching lines). That makes searches and text-oriented automation silently miss this large runtime file; use an escaped separator such as \0 or a printable sentinel so the source remains ordinary searchable text.
Useful? React with 👍 / 👎.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f8c64c879f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const aliasedRelease = resolveKittyNormalizedPressRelease(e); | ||
| if (aliasedRelease) { | ||
| releaseEvent = { | ||
| ...aliasedRelease.event, | ||
| type: "keyup", | ||
| } as unknown as KeyboardEvent; | ||
| aliasedReleaseIdentity = aliasedRelease.identity; |
There was a problem hiding this comment.
If Period is already held and auto-repeating when Command is pressed, its original keydown has been forwarded under Period, while the repeated keydown creates this normalized interrupt alias. The eventual physical keyup is then replaced exclusively with the Ctrl+C release: Kitty and broadcast paths leave the original Period press pending until blur, and Win32 deletes the Period entry but emits a Ctrl+C keyup instead, leaving ConPTY's Period logically held. The aliased keyup must release both the pre-existing physical press and the normalized interrupt press, or repeated keydowns must not create the alias in this situation.
Useful? React with 👍 / 👎.
|
The automatic Codex fix was preserved, but it did not pass verification. The PR remains draft for maintainer review. The candidate patch and verification report were preserved as codex-fix-patch-35072855449. View this run: https://github.com/binaricat/Netcatty/actions/runs/35072855449 |
netcatty-bot
added
ready-for-human
2 participants
Summary
\x03) that plain Ctrl+C sends, so long-running commands liketail -fcan be stopped the way macOS users expect.Periodkey code is used, so non-Latin keyboard layouts still work.Why
Issue #3408: on macOS, Terminal.app (and iTerm2) use ⌘. as the standard cancel/interrupt shortcut for a running command. Netcatty does not bind ⌘. to anything, so it is free to be mapped to the Ctrl+C interrupt without conflicting with existing shortcuts (verified against the default key-binding table).
Changes
components/terminal/runtime/terminalInterruptShortcut.ts: new pure helperisMacCommandPeriodInterruptChordrecognizing the ⌘. chord.components/terminal/runtime/createXTermRuntime.ts: the keydown handler's urgent-interrupt branch now also fires for ⌘. on macOS (viaisMacPlatform()), going through the sameinterruptSession/\x03/ trace / broadcast flow as Ctrl+C.components/terminal/runtime/terminalInterruptShortcut.test.ts: unit tests for the chord (modifier requirements, layout-character preference, physical Period fallback).Testing
node --test --import tsx components/terminal/runtime/terminalInterruptShortcut.test.ts— 7 tests pass.node --test --import tsx components/terminal/runtime/*.test.ts— 972 tests pass.npx eslinton the three changed files — clean.npx tsc --noEmitshows only pre-existing errors increateXTermRuntime.ts(verified identical on HEAD viagit stash).tail -f <file>, press ⌘. — the command should abort just like Ctrl+C.Fixes #3408
Automation
@codex review(own/bot PRs only)