feat(editor): keys hand authoring — set, stamp, shade; hand edits refresh the notation - #303
Merged
Conversation
|
Important Review skippedNo new commits to review since the last review. ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughAdds keys-track hand authoring with per-note left/right assignments, split-based stamping, undo support, piano-roll shading, menu actions, hand-aware notation fingerprints, tests, and documentation. ChangesKeys hand authoring
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Editor
participant editorAssignHandsBySplit
participant SetTechScalarPerNoteCmd
participant NoteState
participant PianoRoll
Editor->>editorAssignHandsBySplit: enter split pitch
editorAssignHandsBySplit->>SetTechScalarPerNoteCmd: stamp lh/rh values
SetTechScalarPerNoteCmd->>NoteState: update techniques.hand
NoteState->>PianoRoll: redraw notes
PianoRoll->>Editor: show hand-specific shading
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
…efresh the notation The hand arc, Step B (stacked on feat/keys-hand-import / #299). The per-note `hand` field becomes authorable and visible: - Note ▸ Hand (keys): Left / Right / Clear on the selection — one undo step via SetTechScalarCmd. Runtime-guarded to keys arrangements. - Track ▸ Assign hands by split…: the split point is a STAMPING generator, never a live layer (the piano-pedagogy call): pick a split note (default C4, note names or MIDI accepted — new @pure _parseSplitPitchPure), and every targeted note takes lh below / rh at-or-above in ONE undoable command. New SetTechScalarPerNoteCmd — SetTechScalarCmd's per-note-values sibling (no bend special-casing; plain scalar marks only). Selection scopes the stamp; per-note edits win from then on. - Hand shading on the piano roll (View ▸, default ON): LH warm (#e8965a) / RH cool (#5a9de8) body color; unassigned keeps the octave palette so "no call made" never reads as an assignment. Same color-job argument as fretted-in-roll: Y already says the pitch, so color says WHO PLAYS IT. - `hand` joins `_notes_fingerprint`'s identity tuple — the ONE technique that does — closing the tracked trap where a hand EDIT couldn't invalidate a preserved authored sidecar (it would freeze the old hands forever). Editor/wire shapes stay fingerprint-identical; junk enums read unassigned on both. One-time effect: previously stamped payloads re-fingerprint on next save and take the measure-granular merge (unedited bars keep authored hands). The relift honors per-note hands via core split_hands (core PR #992). New src/hand.js module; menus in menu-bar.js; USER-GUIDE "Hands on keys tracks" section. Tests: tests/hand_authoring.test.mjs (4 — parser incl. accidentals and junk, stamp orientation matching core, per-note command exec→undo→redo round-trip incl. a bare-techniques note) + a fingerprint-invalidation pytest (hand flips it; other techniques and junk don't; shapes agree). Gates: 326 pytest, JS suite green (song_fit env failure = pristine main), lint 0 errors / 3 baseline warnings. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EBQCHCNA81E9tHmSDHSe2Q
ChrisBeWithYou
force-pushed
the
feat/keys-hand-surface
branch
from
July 16, 2026 18:57
926c8fb to
d0b4856
Compare
![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 `@CHANGELOG.md`:
- Around line 64-70: Update the “Known tracked follow-ups” section to remove or
revise the clauses claiming hand edits do not refresh notation and relifting
does not honor per-note hands, preserving only follow-ups that remain
unimplemented and eliminating the contradiction with the changelog entry.
In `@docs/USER-GUIDE.md`:
- Line 160: Update the “Hands on keys tracks” section heading to use clear
compound wording, such as “Hands on keys-track arrangements” or “Hands on
keyboard tracks,” while leaving the section content unchanged.
In `@routes.py`:
- Around line 2571-2586: Update the fingerprint tuple built by _tup so its hand
component is always orderable, mapping absent/invalid hands and valid "lh"/"rh"
to consistent comparable values while preserving distinct fingerprints. Ensure
all note and chord-note fingerprint sorting uses these tuples safely, and add a
regression test covering duplicate/unison notes that differ only by hand.
🪄 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: 2a6ff771-a86d-4706-a845-50e6c09b686d
📒 Files selected for processing (9)
CHANGELOG.mddocs/USER-GUIDE.mdroutes.pysrc/commands.jssrc/draw.jssrc/hand.jssrc/menu-bar.jstests/hand_authoring.test.mjstests/test_musicxml_notation.py
…son hands The `hand` field added to `_notes_fingerprint`'s identity tuple used a None sentinel for unassigned. Two notes identical in (t,s,f,sus) but differing in hand — a doubled unison (both hands on one pitch, real in piano writing) with one note assigned and the other not — reach a None-vs-str compare in the sort and raise TypeError, crashing the save. Use "" for unassigned instead: still distinct from "lh"/"rh" (the hand still counts toward invalidation) and totally ordered against them. Regression test crashes pre-fix, passes after. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Collaborator
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/test_musicxml_notation.py (1)
130-132: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAssert hand sensitivity for chord members, not only non-crashing behavior.
This assertion only proves that
_notes_fingerprintreturns a non-empty digest; it would still pass if chord-member hands were ignored. Compare the mixed-hand chord with an all-assigned counterpart and with the reversed member order.Suggested test strengthening
- chord = {"time": 0.0, "notes": [_editor_note(0.0, 60, "rh"), _editor_note(0.0, 60)]} - assert _notes_fingerprint([], [chord]) # no raise → truthy hex digest + mixed_chord_notes = [_editor_note(0.0, 60, "rh"), _editor_note(0.0, 60)] + mixed_chord = {"time": 0.0, "notes": mixed_chord_notes} + all_rh_chord = { + "time": 0.0, + "notes": [_editor_note(0.0, 60, "rh"), _editor_note(0.0, 60, "rh")], + } + reversed_chord = {"time": 0.0, "notes": list(reversed(mixed_chord_notes))} + + assert _notes_fingerprint([], [mixed_chord]) != _notes_fingerprint([], [all_rh_chord]) + assert _notes_fingerprint([], [mixed_chord]) == _notes_fingerprint([], [reversed_chord])🤖 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/test_musicxml_notation.py` around lines 130 - 132, Strengthen the chord-member test around _notes_fingerprint by comparing the mixed-hand chord against an equivalent all-assigned chord and against the same members in reversed order. Assert that hand assignment affects the fingerprint while member ordering remains order-safe, rather than only asserting that a digest is returned.
🤖 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/test_musicxml_notation.py`:
- Around line 130-132: Strengthen the chord-member test around
_notes_fingerprint by comparing the mixed-hand chord against an equivalent
all-assigned chord and against the same members in reversed order. Assert that
hand assignment affects the fingerprint while member ordering remains
order-safe, rather than only asserting that a digest is returned.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: d4407076-74bc-43da-bd6f-43a6b9246fbf
📒 Files selected for processing (2)
routes.pytests/test_musicxml_notation.py
🚧 Files skipped from review as they are similar to previous changes (1)
- routes.py
…ing (CodeRabbit) - CHANGELOG: the #299 'known follow-ups' claimed a hand edit doesn't invalidate the notation sidecar; this PR implements exactly that (hand joins the fingerprint), so drop the now-false clause and note it's done — keeping the still-open split_hands (core #992) and sloppak-loader follow-ups. - USER-GUIDE: 'Hands on keys tracks' -> 'Hands on keyboard tracks' (clearer). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Collaborator
|
@coderabbitai review |
✅ Action performedReview finished.
|
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
2 participants
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.
Replaces #302 — it was stacked on #299's branch and GitHub auto-closed it (unrecoverably: a closed PR can't retarget) when that branch was deleted after the merge. This is the same change rebased onto post-wave main (clean rebase, no conflicts) with all gates re-run: 335 pytest, 189 JS suites green (the 2 failures —
song_fit,mixer_meter_teardown— reproduce on pristine post-wave main; environmental), lint 0 errors / 3 baseline warnings.What
The hand arc, Step B. The per-note
handfield (#299, merged) becomes authorable and visible:C4; note names with accidentals or raw MIDI — new@pure _parseSplitPitchPure), and every targeted note takeslhbelow /rhat-or-above in one undoable command (newSetTechScalarPerNoteCmd). Selection scopes the stamp; per-note edits win from then on.handjoins_notes_fingerprint's identity tuple — the one technique that does — closing the trap flagged in feat(editor): MusicXML keys imports keep their authored hand splits — the per-note hand field arrives #299's changelog where a hand edit couldn't invalidate a preserved authored sidecar. Editor/wire shapes stay fingerprint-identical; junk enums read unassigned on both. One-time effect: previously stamped payloads re-fingerprint on next save and take the measure-granular merge. The relift honors per-note hands via core #992.The pipeline this completes
MusicXML authored hands (musicxml-import #8 → #299 ✅ merged) · reload round-trip (core #990) · hand-aware relift (core #992) · authoring + visibility + honest invalidation (this PR). Next slice: hands-separate practice.
Tests
tests/hand_authoring.test.mjs(parser incl. accidentals + junk; stamp orientation pinned to coresplit_hands; per-note command exec→undo→redo round-trip) + a fingerprint pytest (hand flips it, other techniques and junk don't, shapes agree). USER-GUIDE §5 gains "Hands on keys tracks".🤖 Generated with Claude Code
https://claude.ai/code/session_01EBQCHCNA81E9tHmSDHSe2Q
Summary by CodeRabbit