feat(editor): four keybind profiles — FeedBack / Logical / Cableton / Legacy (EOF) - #266
Conversation
|
Warning Review limit reached
Next review available in: 24 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (7)
📝 WalkthroughWalkthroughThe editor now supports FeedBack, Logical, Cableton, and Legacy shortcut profiles with inherited and overridden mappings. Profile cycling and selectors expose all four profiles, while Loop toggle and Song Fit are registered and dispatched as global commands. ChangesShortcut profile system
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant EditorUser
participant ShortcutProfileSelector
participant _editorDispatchFeedbackShortcut
participant _editorTableCommandForKeyPure
participant _editorRunEofCommand
participant EditorCommand
EditorUser->>ShortcutProfileSelector: choose or cycle shortcut profile
EditorUser->_editorDispatchFeedbackShortcut: press shortcut
_editorDispatchFeedbackShortcut->_editorTableCommandForKeyPure: resolve profile override or FeedBack mapping
_editorTableCommandForKeyPure-->>_editorDispatchFeedbackShortcut: command identifier
_editorDispatchFeedbackShortcut->_editorRunEofCommand: dispatch command
_editorRunEofCommand->>EditorCommand: run toggleLoopRegion or songFit
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/menu-bar.js (1)
189-193: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winTransport ▸ Loop region should use the registry command row. The current
{ fn: 'editorToggleLoopRegion' }entry renders an empty key column, so the Logical/Cableton shortcut never appears in the menu. Switching it to{ cmd: 'toggleLoopRegion' }would surface the accelerator.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/menu-bar.js` around lines 189 - 193, Update the Loop region menu entry in the menu definition to use the registry command key toggleLoopRegion instead of the direct editorToggleLoopRegion function reference, preserving its label so the configured accelerator appears.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/menu-bar.js`:
- Around line 189-193: Update the Loop region menu entry in the menu definition
to use the registry command key toggleLoopRegion instead of the direct
editorToggleLoopRegion function reference, preserving its label so the
configured accelerator appears.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: be8e6525-d50f-4dea-8dac-49f9dfb0b799
📒 Files selected for processing (7)
CHANGELOG.mddocs/USER-GUIDE.mdscreen.htmlsrc/input.jssrc/menu-bar.jssrc/shortcuts.jstests/keybind_profiles.test.mjs
8a68de5 to
8692d5b
Compare
|
Applied — good catch. Also landed two self-review fixes on this head, both real:
Both now have regression guards in |
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
🧹 Nitpick comments (3)
tests/keybind_profiles.test.mjs (3)
18-18: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winRedundant static import may execute before the global mocks it depends on.
import * as shortcuts from '../src/shortcuts.js'(line 31) is a static import; per ES module semantics it is fully resolved — meaningshortcuts.js's top-level code runs — before any of this file's own top-level statements execute, including theglobalThis.document/localStorage/windowmock setup on lines 20-24. The subsequentawait import(...)on line 26 just returns the already-cached, already-evaluated module namespace; it can't re-runshortcuts.jswith the mocks in place. Ifshortcuts.js's module-level code reads any of those globals at import time (which is presumably why the mocks exist), it will see the real Node globals instead, not these stubs.Since the dynamic import already yields the full module namespace, drop the redundant static import and reference everything through the dynamically-imported object.
🔧 Proposed fix
const { EDITOR_PROFILE_NAMES, EDITOR_PROFILE_OVERRIDES, _editorProfileCollisionsPure, _editorShortcutRowsPure, _editorTableCommandForKeyPure, editorSetShortcutProfile, -} = await import('../src/shortcuts.js'); -import * as shortcuts from '../src/shortcuts.js'; +} = shortcuts; +const shortcuts = await import('../src/shortcuts.js');Also applies to: 26-31
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/keybind_profiles.test.mjs` at line 18, Remove the static shortcuts import from the test module so shortcuts.js is not evaluated before the global document, localStorage, and window mocks are installed. Keep the existing dynamic import after mock setup, and update all references to the statically imported shortcuts binding to use that dynamically imported module namespace.
23-23: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
localStoragestub can't verify Legacy's persisted-key compatibility.The mock's
setItemis a no-op (line 23), so the profile-selection test only asserts the in-memoryeditorShortcutProfilevalue, not what actually gets persisted. Given the PR explicitly calls out "Legacy retaining its internal ID for localStorage compatibility" as an intentional guarantee, a spy-backed mock that recordssetItemcalls would let this test assert the persisted key/value directly rather than only the exported variable.Also applies to: 167-177
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/keybind_profiles.test.mjs` at line 23, Update the localStorage stub in tests/keybind_profiles.test.mjs to record setItem calls and stored key/value pairs instead of discarding writes, then extend the profile-selection test around the legacy profile to assert that the persisted value uses Legacy’s compatibility ID. Keep the existing in-memory editorShortcutProfile assertion.
113-118: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winHardcoded
TEMPO_MAP_SIGSrisks silent drift from the real overlay.This literal array duplicates the actual tempo-map overlay signature set that presumably lives in
src/shortcuts.js. If that overlay changes there, this list won't reflect it and the "disjoint from tempo-map overlay" guarantee silently stops being meaningful without the test failing. Prefer importing the canonical list (if exported) rather than re-declaring it here.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/keybind_profiles.test.mjs` around lines 113 - 118, The test’s hardcoded TEMPO_MAP_SIGS can drift from the real tempo-map overlay. Update the test to import and reuse the canonical overlay signature list exported from src/shortcuts.js, then keep both _editorProfileCollisionsPure assertions against that shared list and remove the duplicate literal.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@tests/keybind_profiles.test.mjs`:
- Line 18: Remove the static shortcuts import from the test module so
shortcuts.js is not evaluated before the global document, localStorage, and
window mocks are installed. Keep the existing dynamic import after mock setup,
and update all references to the statically imported shortcuts binding to use
that dynamically imported module namespace.
- Line 23: Update the localStorage stub in tests/keybind_profiles.test.mjs to
record setItem calls and stored key/value pairs instead of discarding writes,
then extend the profile-selection test around the legacy profile to assert that
the persisted value uses Legacy’s compatibility ID. Keep the existing in-memory
editorShortcutProfile assertion.
- Around line 113-118: The test’s hardcoded TEMPO_MAP_SIGS can drift from the
real tempo-map overlay. Update the test to import and reuse the canonical
overlay signature list exported from src/shortcuts.js, then keep both
_editorProfileCollisionsPure assertions against that shared list and remove the
duplicate literal.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: e11d76f4-a5a8-4f44-9946-8ecc856ec519
📒 Files selected for processing (7)
CHANGELOG.mddocs/USER-GUIDE.mdscreen.htmlsrc/input.jssrc/menu-bar.jssrc/shortcuts.jstests/keybind_profiles.test.mjs
🚧 Files skipped from review as they are similar to previous changes (6)
- CHANGELOG.md
- src/menu-bar.js
- screen.html
- docs/USER-GUIDE.md
- src/shortcuts.js
- src/input.js
Four profiles: feedback/eof keep their hand resolvers; logical (Logic defaults: K click, Q quantize, ,/. transport, C cycle, Alt+' marker, Ctrl+R repeat) and cableton (Live defaults: Ctrl+U quantize, Ctrl+1/2 grid, Ctrl+4 snap, O click, Ctrl+Shift+F follow, Ctrl+L loop) resolve an override table first and fall through to the FeedBack meaning. Shadowed commands relocate or go explicitly keyless (absent-vs-'' in the registry keys drives honest display). toggleLoopRegion and songFit join the registry; Song Fit gains a Tempo/Grid menu row. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EBQCHCNA81E9tHmSDHSe2Q
…d commands
Review fixes on the keybind-profile PR.
1. Logical bound Ctrl+R to duplicateSelection ("authentic: Cmd-R Repeat").
The Electron host registers an app menu with {role:'reload'}, whose
CmdOrCtrl+R accelerator is handled in the MAIN process before the renderer
ever sees keydown — the dispatcher's e.preventDefault() cannot reclaim it.
So Ctrl+R would have reloaded the editor and dropped unsaved chart edits
instead of duplicating. Dropped the binding: duplicateSelection already
answers Ctrl+D in every profile (input.js handles it outside the resolvers),
so the command loses nothing and the row now displays the chord that works.
2. Logical stranded two commands with no keyboard at all: 'K' -> metronome left
cyclePickDirection keyless, and 'C' -> loop left toggleGuideClap keyless.
Cableton relocates both of ITS shadowed commands, so this was inconsistent as
well as lossy. Relocated them the same way: Shift+K and Ctrl+Shift+C.
Two regression guards added, the checks that would have caught both:
- no delta profile orphans a command the default profile can reach, and every
override round-trips through the resolver it claims;
- no profile binds a host-reserved chord (Ctrl+R/Ctrl+W/Ctrl+Q/F5).
Verified by exhaustive enumeration of the chord space: no profile double-binds a
chord, no profile loses a command (except EOF/toggleSnap, which is pre-existing
on main), and the feedback + eof resolvers are byte-for-byte identical to main
across all 403 bindings — no existing user's keys change.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
CodeRabbit finding, valid. This PR registered toggleLoopRegion as a registry
command and bound it (Logical C, Cableton Ctrl+L), but the Transport menu entry
was still the old direct-fn form { label, fn: 'editorToggleLoopRegion' }. The
fn branch of _menuModelPure renders `it.key || ''`, so the menu showed an empty
accelerator column and the new keybinding was invisible there. Switching to
{ cmd: 'toggleLoopRegion' } takes the label, key and ready/planned state from
the registry — same as its sibling { cmd: 'toggleLoopAB' } directly below it.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
.gitignore lists node_modules/ (trailing slash), which matches a directory but not the worktree symlink, so git add -A tracked it.
aabe1b1 to
0003993
Compare
2 participants
What
Fully fleshed-out shortcut profiles so a charter's DAW muscle memory just works:
,/.rewind/forward by beat, C cycle (loop the selected region), Alt+' create marker (Add section), Ctrl+R repeat (Duplicate selection).Architecture: the new profiles are deltas. Each is a frozen sig→command override table resolved first; any key the table doesn't claim falls through to its FeedBack meaning — so every editor-specific command (techniques, tempo mapping, string moves) works identically in all profiles, and the old FeedBack key remains a harmless alias for relocated commands. Where a DAW key displaces a FeedBack one (Logical's K displaces pick-direction cycling; Cableton's O displaces pop, Ctrl+L displaces select-like), the displaced command relocates or goes explicitly keyless — and the registry's absent-vs-'' distinction makes the shortcut panel display exactly what resolves, never a stolen key.
Also in this PR (both improve the command palette's coverage):
toggleLoopRegionandsongFitjoin the command registry — Song Fit gains its first menu home (Tempo/Grid ▸ Song Fit), and the loop toggle is what Logical's C / Cableton's Ctrl+L drive.Tests
tests/keybind_profiles.test.mjs(9, fail on main): every authentic binding resolves; fall-through inheritance; the shadow rule including honest keyless display; tempo-map overlay reachable through the fall-through; override sigs disjoint from the tempo-map overlay (validator); four-profile plumbing with unknown-value fallback; EOF rows byte-identical; the two new registry commands.user_guide_content,menu_model,eof_shortcutsall green unchanged. Full suite 147 green, lint 0 errors,routes.pyuntouched.docs/USER-GUIDE.mdupdated.Live-verified
Real pak: switched to Logical → K toggles the metronome ("Metronome on — clicks follow the beat grid"), **
,steps the playhead back one beat; switched to Cableton → Ctrl+4 flipsS.snapEnabled, O toggles the metronome off; Legacy still selects and announces itself. Zero page errors.🤖 Generated with Claude Code
https://claude.ai/code/session_01EBQCHCNA81E9tHmSDHSe2Q
Summary by CodeRabbit