refactor(app): carve the keyboard-shortcuts subsystem into static/js/shortcuts.js (R3d)#922
Merged
Conversation
…shortcuts.js (R3d) 19 declarations + 23 TOP-LEVEL STATEMENTS. 922 lines. app.js 3,243 -> 2,325 (-28%). The panel registry, both global keydown dispatchers, the library arrow-nav, and the whole plugin-facing shortcut API. ━━━ MOST OF THIS SUBSYSTEM WAS NOT DECLARATIONS ━━━ A declaration-seeded dependency closure reports this cluster as 10 names, 246 lines. It is 42 statements and 922. window.registerShortcut, createShortcutPanel, getAllShortcuts, unregisterShortcut, clearWindowShortcuts, the panel registry, and BOTH global keydown dispatchers are bare TOP-LEVEL STATEMENTS at app.js's top level. A call-graph scan sees NONE of them. That blind spot has now cost three times: * it nearly shipped a dead library A-Z rail (#896) — 43 of library.js's exports were referenced only from app.js's window contract; * it threw "Assignment to constant variable" in the session carve (#921), where the autoplay gate's top-level statements wrote state that had just become a read-only import; * and here it under-reported the slice by 3x. The extractor takes them by construction now — any top-level statement that TOUCHES a moved binding comes along — and the SEED is closed to a FIXED POINT, because those statements have their own dependencies (_modifiersMatch, _isShortcutActive, _handleLibArrowNav, _gridColumns…) that the declaration closure never walked. Seed -> pull the statements -> the statements need more names -> re-seed. Iterate until it stops growing. ━━━ syncLibrarySong GOES ACROSS THE SEAM, NOT THROUGH AN IMPORT ━━━ The library arrow-nav calls it on Enter. It cannot be imported: syncLibrarySong reaches showScreen/playSong, and a module importing app.js closes a cycle. It is the ONE name here that had to stay behind, so it comes across the host seam — which is exactly what the seam is for. host.js throws loudly if the wiring is ever dropped, and tests/js/host_contract.test.js fails in CI if the hook drifts. VERIFIED. A/B against origin/main in two browsers, IDENTICAL, zero page errors — and driven for real, not merely present: the plugin API (register / unregister / getAll / panels), THE GLOBAL KEYDOWN DISPATCHER actually firing a registered shortcut, that same shortcut correctly SUPPRESSED while typing in a text input, and `?` opening the help modal. node 1045, pytest 2425, ESLint 0 (no-cycle clean), host contract 2/2, Codex 0. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe PR moves keyboard shortcut handling from ChangesKeyboard shortcut subsystem
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Browser
participant shortcutsjs
participant ShortcutPanel
Browser->>shortcutsjs: document keydown
shortcutsjs->>shortcutsjs: Check dispatch blocking
shortcutsjs->>ShortcutPanel: Match key, modifiers, and scope
ShortcutPanel-->>shortcutsjs: Return registered handler
shortcutsjs->>Browser: Prevent default and execute handler
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
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.
19 declarations + 23 top-level statements. 922 lines.
app.js3,243 → 2,325 (−28%).The panel registry, both global
keydowndispatchers, the library arrow-nav, and the whole plugin-facing shortcut API.Most of this subsystem was not declarations
A declaration-seeded dependency closure reports this cluster as 10 names, 246 lines.
It is 42 statements and 922.
window.registerShortcut,createShortcutPanel,getAllShortcuts,unregisterShortcut,clearWindowShortcuts, the panel registry, and both globalkeydowndispatchers are bare top-level statements. A call-graph scan sees none of them.That blind spot has now cost three times:
library.js's exports were referenced only from app.js's window contract;The extractor takes them by construction now — any top-level statement that touches a moved binding comes along — and the seed is closed to a fixed point, because those statements have their own dependencies (
_modifiersMatch,_isShortcutActive,_handleLibArrowNav,_gridColumns…) that the declaration closure never walked.syncLibrarySonggoes across the seam, not through an importThe library arrow-nav calls it on Enter. It cannot be imported:
syncLibrarySongreachesshowScreen/playSong, and a module importing app.js closes a cycle.It's the one name here that had to stay behind, so it comes across the host seam — which is exactly what the seam is for.
host.jsthrows loudly if the wiring is ever dropped, andtests/js/host_contract.test.jsfails in CI if the hook drifts.Verification
A/B against
origin/mainin two browsers — identical, zero page errors — and driven for real, not merely present:?opening the help modalnode 1045 · pytest 2425 · ESLint 0 (no-cycle clean) · host contract 2/2 · Codex 0.
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
?and dismissible with Escape or the backdrop.Improvements