feat(editor): keyboard note entry — place notes without a mouse - #239
Conversation
Gap-audit item #8 — the biggest Guitar-Pro-parity gap in the place/edit loop: every note add required a mouse double-click. Now, in String view with NOTHING selected, an entry caret appears (a dashed cell at the playhead on the current string); ↑/↓ move it between strings and a fret digit (0-9) places a note there, then advances the caret one snap step so you can type a run. - New `S.caretString`; `_editorPlaceAtCaret` reuses the mouse-add AddNoteCmd path (so a keyboard-placed note is identical + undoable), then clears the selection and advances the caret for rapid sequential entry. - Clean "no-selection overload": `_editorSetSelectedFret` places at the caret when the selection is empty (else edits the selected fret as before), and the moveStringUp/Down dispatch moves the caret when empty (else moves the selected note's string). Caret string math mirrors `_getMoveStringSameFretResult` (n.string + direction) so ↑/↓ feel identical held or not. - Caret indicator drawn in draw.js (String view, empty selection only). `tests/keyboard_entry.test.mjs` (4: place-on-empty, undoable, digit-still-edits- with-a-selection, caret move + clamp). 129 JS suites green, lint 0-err (3 pre-existing warnings). routes.py untouched. Verified live on AC/DC — Back In Black: the dashed caret renders on the caret string; "7" → "Placed fret 7 on string 1"; ↑ → "Entry caret on string 2"; "5" placed on string 2; both undone by Ctrl+Z. No page errors. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EBQCHCNA81E9tHmSDHSe2Q
|
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 (4)
🚧 Files skipped from review as they are similar to previous changes (4)
📝 WalkthroughWalkthroughString view now supports keyboard note entry through a caret. Arrow keys move the caret between strings, fret digits place undoable notes and advance time, and a dashed overlay shows the insertion location when no note is selected. ChangesKeyboard note-entry workflow
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Keyboard
participant EditorInput
participant History
participant EditorState
participant StringView
Keyboard->>EditorInput: Type fret digit
EditorInput->>EditorState: Read cursor time and caret string
EditorInput->>History: Execute AddNoteCmd
History->>EditorState: Add note and clear selection
EditorInput->>StringView: Redraw advanced caret
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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.
Actionable comments posted: 3
🤖 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.
Inline comments:
In `@src/draw.js`:
- Around line 479-481: Update the caret overlay calculation in the !keysMode and
empty-selection branch to clamp S.caretString to the active arrangement’s string
count from _stringCountFor(S.arrangements[S.currentArr]) before passing it to
strToY(). Keep the existing caret fallback and coordinate calculation unchanged
so the overlay matches placement behavior.
In `@src/input.js`:
- Around line 111-120: The note insertion flow around AddNoteCmd must keep the
target and caret quantized consistently. Use the already computed snapped time
for the caret advance instead of raw S.cursorTime, and ensure the rendered
insertion target uses that same quantized time when calling _editBlipAt;
preserve the existing selection-clearing and snap-step behavior.
- Around line 105-110: Update _editorPlaceAtCaret and the related caret-command
helpers at the referenced branches to return without changing state when
S.drumEditMode or S.tempoMapMode is active, in addition to the existing
keys-mode guard. Preserve normal String-note editing behavior, and add
regression coverage confirming these commands neither add notes nor move the
caret in drum or tempo-map modes.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 3112c52e-f986-430e-87f7-fb55bb339645
📒 Files selected for processing (5)
CHANGELOG.mdsrc/draw.jssrc/input.jssrc/state.jstests/keyboard_entry.test.mjs
| if (!keysMode && S.sel.size === 0) { | ||
| const cx = timeToX(S.cursorTime || 0); | ||
| const cy = strToY(S.caretString || 0) + NOTE_PAD; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Clamp the displayed caret to the active arrangement.
After switching from a wider part to a narrower one, S.caretString can be out of range: placement clamps it locally in src/input.js, while this overlay renders the stale value. Clamp against _stringCountFor(S.arrangements[S.currentArr]) before strToY() so the visible target matches the note that will be placed.
🤖 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/draw.js` around lines 479 - 481, Update the caret overlay calculation in
the !keysMode and empty-selection branch to clamp S.caretString to the active
arrangement’s string count from _stringCountFor(S.arrangements[S.currentArr])
before passing it to strToY(). Keep the existing caret fallback and coordinate
calculation unchanged so the overlay matches placement behavior.
| function _editorPlaceAtCaret(fret) { | ||
| if (isKeysMode()) { setStatus('Select notes first'); return false; } | ||
| const arr = S.arrangements && S.arrangements[S.currentArr]; | ||
| const nStr = arr ? _stringCountFor(arr) : 0; | ||
| if (!nStr) { setStatus('Select notes first'); return false; } | ||
| const string = Math.max(0, Math.min(nStr - 1, Number(S.caretString) || 0)); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Prevent caret commands outside normal String-note editing.
_editorCurrentNoteIndices() intentionally returns no selection in drum and tempo-map modes, so these branches can add a fretted note or move the hidden caret there. Guard both helpers against S.drumEditMode and S.tempoMapMode (as well as keys mode) before changing state, and add regression coverage for those modes.
Also applies to: 129-138, 830-831
🧰 Tools
🪛 ast-grep (0.44.1)
[error] 105-105: React's useState should not be directly called
Context: setStatus('Select notes first')
Note: [CWE-710] Improper Adherence to Coding Standards. Security best practice.
(usestate-direct-usage)
[error] 108-108: React's useState should not be directly called
Context: setStatus('Select notes first')
Note: [CWE-710] Improper Adherence to Coding Standards. Security best practice.
(usestate-direct-usage)
🤖 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/input.js` around lines 105 - 110, Update _editorPlaceAtCaret and the
related caret-command helpers at the referenced branches to return without
changing state when S.drumEditMode or S.tempoMapMode is active, in addition to
the existing keys-mode guard. Preserve normal String-note editing behavior, and
add regression coverage confirming these commands neither add notes nor move the
caret in drum or tempo-map modes.
| const time = snapTime(S.cursorTime || 0); | ||
| const note = { time, string, fret: Math.max(0, Math.min(24, Number(fret) || 0)), sustain: 0, techniques: {} }; | ||
| const cmd = new AddNoteCmd(note); | ||
| S.history.exec(cmd); | ||
| _tourNoteAction('placeNote'); | ||
| _editBlipAt(); | ||
| // Entry flow: leave NO selection (so the next digit places again) and advance | ||
| // the caret one snap step for rapid sequential entry. | ||
| S.sel.clear(); | ||
| _editorSeekToTime((S.cursorTime || 0) + _editorSnapStepSeconds()); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Keep the insertion target and caret position quantized consistently.
Placement uses snapTime(S.cursorTime), but the caret advances from raw S.cursorTime. With an off-grid playhead, the dashed target in src/draw.js line 480 differs from the inserted note and remains offset after entry. Advance from the snapped time and render the same quantized insertion time.
🧰 Tools
🪛 OpenGrep (1.23.0)
[ERROR] 114-114: Dynamic command passed to child_process.exec/execSync. Use child_process.execFile or spawn with an argument array instead.
(coderabbit.command-injection.exec-js)
🤖 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/input.js` around lines 111 - 120, The note insertion flow around
AddNoteCmd must keep the target and caret quantized consistently. Use the
already computed snapped time for the caret advance instead of raw S.cursorTime,
and ensure the rendered insertion target uses that same quantized time when
calling _editBlipAt; preserve the existing selection-clearing and snap-step
behavior.
# Conflicts: # CHANGELOG.md
2 participants
Gap-audit item #8 — the biggest Guitar-Pro-parity gap in the place/edit loop: every note add required a mouse double-click.
What's new
In String view with nothing selected, an entry caret appears — a dashed cell at the playhead on the current string. ↑/↓ move it between strings and a fret digit (0-9) places a note there, then advances the caret one snap step, so you can type a run without touching the mouse.
The behaviour is a clean no-selection overload, so nothing changes for editing:
Implementation
S.caretString;_editorPlaceAtCaretreuses the mouse-addAddNoteCmdpath, so a keyboard-placed note is byte-identical to a clicked one and undoable; it then clears the selection and advances the caret._getMoveStringSameFretResult(n.string + direction) so ↑/↓ feel identical whether or not a note is held.draw.js(String view, empty selection only).Tests / gates
tests/keyboard_entry.test.mjs(4: place-on-empty, undoable, digit-still-edits-with-a-selection, caret move + clamp). 129 JS suites green, lint 0 errors (3 pre-existing warnings).routes.pyuntouched.Verified live
On AC/DC — Back In Black: the dashed caret renders on the caret string; typing "7" → "Placed fret 7 on string 1"; ↑ → "Entry caret on string 2"; "5" placed on string 2; both undone by Ctrl+Z. No page errors.
🤖 Generated with Claude Code
Summary by CodeRabbit