refactor(editor): extract chord templates & handshapes to src/chords.js (R2, step 7) - #151
Conversation
…js (R2, step 7) src/main.js 20,601 -> 20,160. chords.js holds the chord load/save round-trip: flattenChords/_flattenArrChords, the whole @pure:chord-relink helper set (relinkChordTemplate, _buildPreservedTemplates, buildHandshapeChordIdMap, dropOrphanedHandshapes, remapHandshapeChordIds, the sanitizers, _groupFn), reconstructChords, the handshape wire-coercion helpers, and the handshape dirty-count trio lifted out of the Handshape-lane section. Reads S and lanes(); no DOM. Graph stays acyclic: chords -> {state, lanes}. main.js's whole diff is the deletions plus the import block. HS_ORPHAN_EPS, _safeWireBool and _wireFloat stay module-private (nothing outside needs them). THE TEST REWORK, which is what blocked this in step 6: - chord_relink: the block was already pure, so it becomes a plain real-import suite (23 cases). No sandbox at all. - handshape_authoring: hybrid. Imports the chord-template helpers; still slices `_handshapeSpanFrets`, which stays in main.js. - suggest_position_wiring: the one that mattered. Its makeReconstruct() built a `new Function` sandbox around reconstructChords' SOURCE and fed it a fabricated `S` plus a `lanes: () => 6` stub. Importing the real function would have closed over the real S from state.js and silently bypassed that fixture — the suite would have stayed green while testing the wrong object. It now drives the real S and the real lanes(), and pins the premise the old stub asserted by fiat: `assert.strictEqual(lanes(), 6)` for its 6-string 'Lead' fixture. Verified: node --test 85/85, pytest 248/248. main.js diff mechanically checked to be deletions + the import block. No unused import, no shadowing, no cycle. Served from local uvicorn on core@main (R0): src/chords.js 200 as text/javascript. All four headless Chromium harnesses green (draw path, state round-trip, hit test, resize) — the load path runs flattenChords, so a real chart rendering is direct evidence for it. reconstructChords runs only at save/build, so its cover is the unit suites, which now drive the real function against the real S. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
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)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughChord flattening, chord reconstruction, template relinking, and handshape remapping are extracted into ChangesChord/handshape module extraction and ESM test migration
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant reconstructChords
participant arr
participant S
participant handshapes
reconstructChords->>arr: rebuild notes, chords, chord_templates
reconstructChords->>S: reset history and clear dropped selection
reconstructChords->>handshapes: drop orphans and remap chord_id values
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the Comment |
There was a problem hiding this comment.
Pull request overview
This PR continues the editor ES-module split (R2 step 7) by extracting chord load/save round-trip logic (chord templates + handshapes) out of the monolithic src/main.js into a new, DOM-free src/chords.js, and refactors the affected test suites to use real imports where safe.
Changes:
- Added
src/chords.jsand movedflattenChords/_flattenArrChords, chord-template relink helpers,reconstructChords, and handshape normalization/dirty tracking into it. - Updated
src/main.jsto import chord/handshape utilities fromsrc/chords.jsand removed the inlined implementations. - Reworked test suites to prefer ESM imports over source-text sandboxing, including driving
reconstructChordsagainst realS/lanes()where required.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/suggest_position_wiring.test.mjs | Switches to ESM imports; drives real reconstructChords, S, and lanes() to avoid fixture bypass. |
| tests/handshape_authoring.test.mjs | Imports chord-template helpers from src/chords.js while continuing to slice _handshapeSpanFrets from src/main.js. |
| tests/chord_relink.test.mjs | Converts chord relink testing from source-slicing to direct imports from src/chords.js. |
| src/main.js | Replaces large in-file chord/handshape implementations with imports from src/chords.js. |
| src/chords.js | New module containing chord flatten/reconstruct and chord-template/handshape helper logic. |
| CHANGELOG.md | Documents ES-module migration step 7 and the accompanying test refactors. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| * Reads `S` and `lanes()`; no DOM. `reconstructChords` resets `S.history` | ||
| * because it replaces every note object, invalidating index-based undo. |
There was a problem hiding this comment.
Fixed. You're right, and this one was my comment (new in this PR), not moved code. Solo notes survive by reference — suggest_position_wiring asserts exactly that (a note field leaks on solos). What actually invalidates index-based undo is that chord members are rebuilt and arr.notes/arr.chords are replaced wholesale. Reworded.
| const frets = new Array(L).fill(-1); | ||
| for (const n of group) { | ||
| if (n.string >= 0 && n.string < L) frets[n.string] = n.fret; | ||
| } | ||
| const fretKey = frets.join(','); |
There was a problem hiding this comment.
Good catch — this is a real latent bug and I fixed it at the root.
relinkChordTemplate looks preserved templates up via _fretKeyForL, and buildHandshapeChordIdMap re-keys the rebuilt ones the same way. The raw frets.join(',') here was the odd one out: two chords differing only by NaN vs undefined in a slot minted two templates that both relinked to the same preserved entry, and the handshape chord_id remap would land on whichever came first.
_fretKeyForL is byte-identical to join(',') whenever every fret is finite — which is why no real chart ever tripped it. Added tests/chords.test.mjs, and verified the new case fails against the old raw-join keying and passes on the fix. (Pre-existing; moved verbatim in this step.)
Three Copilot findings on #151. Two were pre-existing code moved verbatim in step 7; one was my own module header. 1. [real bug] reconstructChords keyed its local template-dedupe map with a raw `frets.join(',')`, but relinkChordTemplate looks preserved templates up — and buildHandshapeChordIdMap re-keys the rebuilt ones — via _fretKeyForL, which folds every non-finite slot to -1. Two chords differing only by NaN vs undefined in one slot therefore minted TWO templates that both relinked to the SAME preserved entry, and a handshape chord_id remap would land on whichever came first. Keying via _fretKeyForL is identical to join(',') for any chart whose frets are all finite, which is why nothing caught it. 2. [dead code] `const soloNotes = []` in reconstructChords was never read or written. Removed. 3. [comment] The module header claimed reconstructChords "replaces every note object". It doesn't: solo notes survive by reference (suggest_position_wiring asserts exactly that — a note field LEAKS on solos), while chord members are rebuilt and arr.notes/arr.chords are replaced wholesale. That array replacement is what invalidates index-based undo. Reworded. New tests/chords.test.mjs drives reconstructChords and flattenChords against the real S and the real lanes(): the dedupe regression (verified to FAIL on the old raw-join keying and pass on the fix), template sharing across identical voicings, _fretKeyForL's equivalence to join(',') on finite frets, and a flatten -> reconstruct round-trip. node --test 86/86, pytest 248/248, headless draw + hit-test harnesses green. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
![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/chords.js`:
- Around line 349-353: Solo notes are still carrying chord-only provenance after
flattening, so clear all inherited chord metadata before pushing them into
newNotes. In the solo-note path in chords.js, update the logic around the
group[0] handling to remove not just _fn but also _fromChord and _chordId from
the note object before it is saved through arr.notes. Keep the fix localized to
the lone-note branch so chord members retain their metadata while flattened solo
notes do not.
- Around line 40-42: The chord-flattening logic in the notes push path is
treating a valid zero time as missing. Update the fallback used in the code that
builds notes in the chord handling flow so the `cn.time` value is preserved when
it is 0, using a nullish-style fallback instead of a truthy check. Keep the
change localized to the note creation logic in the chord processing function.
- Around line 170-178: In relinkChordTemplate, the preserved template is being
copied too literally: frets.slice() can retain an outdated handshape width, and
arp is being coerced with a boolean cast that treats the string "false" as true.
Update relinkChordTemplate to normalize the returned frets to the current chart
width using the existing chord/template normalization helpers, and coerce arp
with the same wire-safe parsing approach used elsewhere in chords.js. Keep the
logic centered on relinkChordTemplate, _fretKeyForL, and _normFingers so
preserved handshape-only templates stay consistent.
🪄 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: 89f27c9b-ef75-44f2-9a99-9194c7639aa6
📒 Files selected for processing (6)
CHANGELOG.mdsrc/chords.jssrc/main.jstests/chord_relink.test.mjstests/handshape_authoring.test.mjstests/suggest_position_wiring.test.mjs
| export function relinkChordTemplate(frets, preserved, L) { | ||
| const old = preserved[_fretKeyForL(frets, L)]; | ||
| const name = (old && typeof old.name === 'string') ? old.name : ''; | ||
| return { | ||
| name, | ||
| frets: frets.slice(), | ||
| fingers: _normFingers(old && old.fingers, L), | ||
| displayName: (old && typeof old.displayName === 'string') ? old.displayName : name, | ||
| arp: !!(old && old.arp), |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Normalize relinked template frets and wire-coerce arp.
For preserved handshape-only templates, frets.slice() can keep a stale 6-wide array on wider charts, and !!old.arp flips "false" to true.
Proposed fix
export function relinkChordTemplate(frets, preserved, L) {
- const old = preserved[_fretKeyForL(frets, L)];
+ const normFrets = new Array(L);
+ for (let i = 0; i < L; i++) {
+ normFrets[i] = (Array.isArray(frets) && Number.isFinite(frets[i])) ? frets[i] : -1;
+ }
+ const old = preserved[_fretKeyForL(normFrets, L)];
const name = (old && typeof old.name === 'string') ? old.name : '';
return {
name,
- frets: frets.slice(),
+ frets: normFrets,
fingers: _normFingers(old && old.fingers, L),
displayName: (old && typeof old.displayName === 'string') ? old.displayName : name,
- arp: !!(old && old.arp),
+ arp: _safeWireBool(old && old.arp, false),📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export function relinkChordTemplate(frets, preserved, L) { | |
| const old = preserved[_fretKeyForL(frets, L)]; | |
| const name = (old && typeof old.name === 'string') ? old.name : ''; | |
| return { | |
| name, | |
| frets: frets.slice(), | |
| fingers: _normFingers(old && old.fingers, L), | |
| displayName: (old && typeof old.displayName === 'string') ? old.displayName : name, | |
| arp: !!(old && old.arp), | |
| export function relinkChordTemplate(frets, preserved, L) { | |
| const normFrets = new Array(L); | |
| for (let i = 0; i < L; i++) { | |
| normFrets[i] = (Array.isArray(frets) && Number.isFinite(frets[i])) ? frets[i] : -1; | |
| } | |
| const old = preserved[_fretKeyForL(normFrets, L)]; | |
| const name = (old && typeof old.name === 'string') ? old.name : ''; | |
| return { | |
| name, | |
| frets: normFrets, | |
| fingers: _normFingers(old && old.fingers, L), | |
| displayName: (old && typeof old.displayName === 'string') ? old.displayName : name, | |
| arp: _safeWireBool(old && old.arp, false), |
🤖 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/chords.js` around lines 170 - 178, In relinkChordTemplate, the preserved
template is being copied too literally: frets.slice() can retain an outdated
handshape width, and arp is being coerced with a boolean cast that treats the
string "false" as true. Update relinkChordTemplate to normalize the returned
frets to the current chart width using the existing chord/template normalization
helpers, and coerce arp with the same wire-safe parsing approach used elsewhere
in chords.js. Keep the logic centered on relinkChordTemplate, _fretKeyForL, and
_normFingers so preserved handshape-only templates stay consistent.
There was a problem hiding this comment.
Both halves are real — I reproduced them against the real module — but I am deferring them out of this PR rather than folding them in. Reasons below.
relinkChordTemplate([3,2,0,-1,-1,-1], {}, 8).frets.length → 6 (not 8)
relinkChordTemplate([3,-1,...], {"3,-1,...": {arp:"false"}}, 6).arp → true
The 6-wide case is reachable through buildHandshapeChordIdMap's preserve-append path, which calls relinkChordTemplate(old.frets, …, L) with template frets straight off the wire. And !!old.arp on the string "false" is exactly the trap _safeWireBool was written for — it lives in this same module and simply is not used here.
Why not here: this PR is a verbatim move (its diff of main.js is deletions plus an import block, checked mechanically), and both fixes change the saved wire payload — template frets width, and whether arpeggio flips on. That wants checking against routes.py's validator and a real save round-trip, not a rider on a mechanical extraction. Tracked in the follow-up issue, with this repro.
There was a problem hiding this comment.
@byrongamatos, that's a reasonable call — since this PR is a mechanical, verbatim extraction, and both fixes touch the saved wire payload (template frets width and the arp boolean semantics), they deserve their own validation pass against routes.py's wire validator and a real save round-trip rather than riding along here.
Would you like me to open a follow-up GitHub issue capturing this repro (the two relinkChordTemplate cases you shared) so it's tracked? If so, I'll include:
- The two repro snippets (6-wide truncation on wider charts via
buildHandshapeChordIdMap's preserve-append path, andarp: "false"coercing totruevia!!old.arp) - The suggested fix direction (normalize
fretsto widthL, use_safeWireBoolforarp) - A note that it needs verification against
routes.py's validator and a save round-trip - Backlinks to this PR and comment, and your username
Let me know and I'll go ahead and create it.
| // A lone note is not a chord, so it carries no harmony fn. Drop any | ||
| // `_fn` it inherited (e.g. a chord note dragged out) so the internal | ||
| // field can't ride into the saved wire via arr.notes. | ||
| delete group[0]._fn; | ||
| newNotes.push(group[0]); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Clear all chord-only metadata from solo notes.
Line 352 drops _fn, but solo notes can still retain _fromChord and _chordId from flattening, leaking editor-only chord provenance into arr.notes.
Proposed fix
// field can't ride into the saved wire via arr.notes.
delete group[0]._fn;
+ delete group[0]._fromChord;
+ delete group[0]._chordId;
newNotes.push(group[0]);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // A lone note is not a chord, so it carries no harmony fn. Drop any | |
| // `_fn` it inherited (e.g. a chord note dragged out) so the internal | |
| // field can't ride into the saved wire via arr.notes. | |
| delete group[0]._fn; | |
| newNotes.push(group[0]); | |
| // A lone note is not a chord, so it carries no harmony fn. Drop any | |
| // `_fn` it inherited (e.g. a chord note dragged out) so the internal | |
| // field can't ride into the saved wire via arr.notes. | |
| delete group[0]._fn; | |
| delete group[0]._fromChord; | |
| delete group[0]._chordId; | |
| newNotes.push(group[0]); |
🤖 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/chords.js` around lines 349 - 353, Solo notes are still carrying
chord-only provenance after flattening, so clear all inherited chord metadata
before pushing them into newNotes. In the solo-note path in chords.js, update
the logic around the group[0] handling to remove not just _fn but also
_fromChord and _chordId from the note object before it is saved through
arr.notes. Keep the fix localized to the lone-note branch so chord members
retain their metadata while flattened solo notes do not.
There was a problem hiding this comment.
Real, but lower severity than Major, and deferred with the others.
Reproduced: flatten a 2-note chord, delete one member, reconstructChords() → the surviving solo keeps _fromChord and _chordId (_fn is deleted, those two are not), and _buildSaveBody ships notes untouched — it only strips the arrangement-level _anchorEditCount/_handshapeEditCount. So they do reach the wire.
What keeps it cosmetic: _fromChord and _chordId are write-only. flattenChords sets them and nothing anywhere reads them back — grep across src/ finds no reader, only the tag site and a stale comment claiming reconstructChords groups by time+_fromChord (it groups by time alone). So the leak is extra fields on saved notes, not a mis-grouping.
Fixing it still changes the saved payload, so it rides along with the other two in the follow-up issue — together with that stale comment.
There was a problem hiding this comment.
@byrongamatos Agreed — thanks for reproducing it and confirming the write-only nature of _fromChord/_chordId. That's a fair read: since nothing in src/ reads them back, the impact is cosmetic payload bloat rather than a correctness bug in grouping/reconstruction. Deferring it alongside the other two and the stale comment about grouping by time+_fromChord makes sense.
Want me to open a follow-up issue to track this bundle (the _fromChord/_chordId leak, the other two deferred items, and the stale comment fix)? If so, let me know the other two items you're referring to (or just confirm and I'll base it on this thread), and I'll file it with a link back to this PR and comment.
|
Follow-up for the deferred findings: #152 (verified repros for the 6-wide Fixed in this PR: all three Copilot findings, including the real one — |
…olo provenance (#153) Closes #152. Three pre-existing data-integrity bugs in the chord save path, surfaced by CodeRabbit on #151 and each verified by running the real module. 1. relinkChordTemplate stored `frets.slice()` verbatim. A preserved template can arrive narrower than the chart — buildHandshapeChordIdMap's preserve-append hands it a template straight off the wire — so a 6-wide row survived on a 7/8-string chart, and a non-finite slot (undefined / NaN / a hand-edited string) rode through untouched. It now stores the width-normalized row. The fold that _fretKeyForL already applied for the lookup key is now a shared `_normFretsToL`, used by both. Key, preserved-lookup and stored row can no longer disagree — the same class of bug as the raw-join dedupe key fixed in #151. `fingers` was always padded to L this way; `frets` now matches. 2. `arp: !!(old && old.arp)` turned the STRING "false" into true. _safeWireBool exists in that same module for exactly this — its own comment names the case — and simply was not used. A hand-edited or legacy sloppak with `arp: "false"` no longer switches arpeggio on across a load->save round-trip. 3. A chord reduced to one note left `_fromChord` / `_chordId` on the survivor; they are now cleared alongside `_fn`. None of the three can reach disk — the backend's `_note()` whitelists the keys it writes — but `_fn` is READ BACK by `_groupFn`, so a stale one would be adopted by majority vote if that note were later dragged into a chord. The comment there claimed the delete was about the wire. It isn't; corrected. Also fixed the stale claim above flattenChords that reconstructChords groups by "time+_fromChord" (it groups by rounded time). NOT a bug, checked and closed out in the issue: `time: cn.time || ch.time` in _flattenArrChords does not drop first-beat notes. Chord-member `time` is absolute, so a chord on beat one has cn.time === ch.time === 0. Verified against Arcturus - The Sham Mirrors - Kinetic.feedpak: 66 chords, every member time equal to its chord's, none zero. Verified: node --test 86/86, pytest 248/248. Eight new cases in tests/chords.test.mjs, and EACH was re-run against the un-fixed code to confirm it fails there — a guard that passes both ways guards nothing. End-to-end: a real save -> reload round-trip through the running server on the chord-heavy Arcturus pack (8 templates, 66 chords). Intercepted the actual POST body: every fret row L-wide, every fret finite, every `arp` a real boolean, no editor-internal field on any note; "Saved successfully"; templates and chords both survive the reload. That proves the new shape is ACCEPTED by routes.py — the discriminating evidence for the bugs themselves is the unit suite. Headless draw / hit-test / resize harnesses green. Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Step 7 of the editor's ES-module split (R2).
main.js20,601 → 20,160 — the largest cut so far, and the first where the test rework was the actual work.What moved
src/chords.js(446 lines) — the chord load/save round-trip. ReadsSandlanes(); no DOM. Graph stays acyclic:chords → {state, lanes}.flattenChords/_flattenArrChords— fold chords into notes on load@pure:chord-relinkhelper set —relinkChordTemplate,_buildPreservedTemplates,buildHandshapeChordIdMap,dropOrphanedHandshapes,remapHandshapeChordIds, the sanitizers,_groupFnreconstructChords— rebuild chords + re-link templates + remap handshapechord_ids on save_ensureHandshapes,_bumpHandshapesDirty,_handshapesAreDirty) lifted out of the Handshape-lane section 18k lines further downmain.js's entire diff is the deletions plus the import block.HS_ORPHAN_EPS,_safeWireBooland_wireFloatstay module-private.The test rework — the reason step 6 stopped here
Three suites drove this cluster by concatenating its source text into a
new Functionsandbox.chord_relink— the block was already pure, so it becomes a plain real-import suite. 23 cases, no sandbox at all.handshape_authoring— hybrid. Imports the chord-template helpers; still slices_handshapeSpanFrets, which stays inmain.js. It never calledreconstructChords; it replays the logic.suggest_position_wiring— the one that mattered. ItsmakeReconstruct()wrappedreconstructChords' source in a sandbox and fed it a fabricatedSplus alanes: () => 6stub:Naively importing the real function would have closed over the real
Sfromstate.jsand quietly bypassed that fixture. The suite would have stayed green while testing an object the test never populated. So it now drives the realSand the reallanes()— and pins the premise the stub had asserted by fiat:(It really is:
'Lead', no tuning array, notes on strings 0–3 → guitar baseline 6.) It also clearsLC.activefirst, since no draw frame is open andlanes()must genuinely compute.Verification
node --test85/85,pytest248/248.main.jsdiff mechanically checked to be deletions + the import block. No unused import, no shadowing, no cycle.src/chords.js200 astext/javascript.flattenChords, so a real chart rendering its notes is direct evidence for it.reconstructChordsruns only at save/build, so it isn't reachable from those harnesses. Its cover is the unit suites — which, as of this PR, drive the real function against the realSinstead of a copy of its source text.makeReconstruct()re-seedsSon every call, andnode --testgives each file its own process).🤖 Generated with Claude Code
Summary by CodeRabbit