fix: ignore keyboard lock state when matching shortcuts (#327) - #331
Open
kbwo wants to merge 4 commits into
Open
fix: ignore keyboard lock state when matching shortcuts (#327)#331kbwo wants to merge 4 commits into
kbwo wants to merge 4 commits into
Conversation
Ctrl+E (returnToMenu) is reported to do nothing while a session is attached when Num Lock or Caps Lock is on: the kitty keyboard protocol adds a lock-state bit to the CSI-u modifier mask, and shortcutManager only matches the mask without those bits. Add an end-to-end test that runs the real CLI in a pseudo terminal against a throwaway git repo, attaches a session backed by a fake command, writes one key sequence, and checks whether the menu comes back. It covers the ASCII control code, the CSI-u form without lock bits (both work) and the Num Lock / Caps Lock variants (both are swallowed), so the reported behaviour is pinned down. The lock-bit expectations assert the current broken behaviour and have to be flipped to `true` when the issue is fixed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01P7fASJhs75iXWmgndrabpv
Ctrl+E (returnToMenu) did nothing inside a session whenever Num Lock or Caps Lock was on. Extended keyboard protocols report the modifiers of a keypress as a number that also carries the keyboard's lock state (+64 Caps Lock, +128 Num Lock), and shortcutManager only compared the incoming bytes against pre-built strings whose modifier field was hardcoded to 5 (ctrl). A Ctrl+E arriving as ESC[101;133u matched none of them and was forwarded to the child process instead, with no feedback. Match the three parameterized sequences (CSI u, modifyOtherKeys and CSI 1;<mod><letter>) by parsing the modifier field and masking out the lock bits, rather than by string comparison against a fixed list. The pre-built list now only holds the forms that have no modifier field: the ASCII control code and bare escape. Key release events are ignored so a release cannot trigger the shortcut a second time. The end-to-end expectations for the Num Lock and Caps Lock variants are flipped from the reproduction to the fixed behaviour, and the unit tests cover both lock bits, their combination, an embedded sequence, and the cases that must still not match (another key, an added real modifier, ctrl missing, alt instead of ctrl). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01P7fASJhs75iXWmgndrabpv
The end-to-end test failed on GitHub Actions with "menu never rendered" while passing locally. Ink does not paint frames to stdout when it believes it runs in CI: it keeps the frame in memory and writes it only on unmount (the isInCi branch in ink/build/ink.js, fed by the is-in-ci package, which checks CI and CONTINUOUS_INTEGRATION). The child inherited CI=true from the runner, so the menu never appeared and the harness timed out before it could send a key. Drop those two variables from the environment handed to the child. The harness drives a real pseudo terminal and has to see the menu the way a user does, which is exactly what Ink's CI mode suppresses. Reproduced locally with `CI=true bun run test` before and after. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01P7fASJhs75iXWmgndrabpv
`bun install` runs the build through the `prepare` script, so the test suite also picks up the compiled copies under dist/. The compiled test still pointed at `returnToMenuPtyHarness.ts`, which does not exist there, and failed in CI with "Module not found" while the src copy passed. Resolve the harness next to the test file by the extension that is actually present, the same way the harness resolves the CLI entry point. The dist copy then drives the built dist/cli.js, so the run also covers the build output. Reproduced locally with `bun run build && CI=true bun run test`. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01P7fASJhs75iXWmgndrabpv
kbwo
marked this pull request as ready for review
September 2, 2026 14:06
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
1 participant
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.
Fixes #327.
Problem
Ctrl+E(thereturnToMenushortcut) silently did nothing inside an attached session whenever Num Lock or Caps Lock was on. The key was forwarded to the child process with no error and no visual feedback, which is very hard to diagnose from the user side.Extended keyboard protocols report the modifiers of a keypress as a single number that also carries the lock state of the keyboard (
+64Caps Lock,+128Num Lock) alongside the modifiers actually being held.shortcutManagercompared the incoming bytes against pre-built strings whose modifier field was hardcoded to5(ctrl), so aCtrl+Earriving asESC[101;133umatched nothing and fell through tosession.process.write(data)inSession.tsx.Laptops commonly boot with Num Lock on internally even without a numpad, so this hits every Ctrl shortcut for those users.
Approach
The three sequences that carry the modifiers as a number — kitty's CSI-u, xterm/tmux
modifyOtherKeys, and theESC[1;<mod><letter>form reported in #82/#107 — are now matched by parsing the modifier field and masking out the lock bits, instead of by string comparison against a fixed list. The pre-built list keeps only the forms that have no modifier field (the ASCII control code and bare escape), so there is a single place that decides which extended sequence means which shortcut.Key release events (
...:3u) are excluded, so a release cannot fire the shortcut a second time.Verification
Developed test-first; both test levels were confirmed failing before the fix.
End-to-end (
src/e2e/): a new test boots the real CLI in a pseudo terminal against a throwaway git repo, attaches a session backed by a fake command, writes one key sequence, and checks whether the menu comes back. It covers the ASCII control code, CSI-u without lock bits, and the Num Lock and Caps Lock variants. The two lock variants reproduced the issue on the first commit of this branch and pass on the second.The harness runs as a separate
bunprocess because the pseudo terminal needs Bun'sBun.TerminalAPI while Vitest runs under node; it is skipped wherebunor a Unix pseudo terminal is unavailable.Unit (
shortcutManager.test.ts): both lock bits, their combination, the uppercase code point,modifyOtherKeys, and a sequence embedded in a larger chunk — plus the cases that must still not match, so the masking cannot become too permissive: a different key carrying a lock bit, an added real modifier (shift), ctrl missing, and alt instead of ctrl.bun run lint,bun run typecheckandbun run testall pass.Review notes
The two commits are meant to be read in order: the first one only adds the end-to-end test and pins down the broken behaviour, the second one fixes it and flips those expectations.
🤖 Generated with Claude Code
https://claude.ai/code/session_01P7fASJhs75iXWmgndrabpv