refactor(editor): move the open-string pitch model into src/lanes.js (R2, step 8a) - #154
Merged
Merged
Conversation
…(R2, step 8a) _GUITAR_OPEN_MIDI / _BASS_OPEN_MIDI (module-private), _openMidiForArr, and the @pure:fret-pitch helper _soundingPitchPure move out of src/main.js into src/lanes.js. They are the string->pitch half of the same model: lanes.js already knows bass-vs-guitar and string counts, and _openMidiForArr's extended-range rules (a perfect fourth below the lowest string; a 6-string bass appending high C) are the same rules laneLabels() encodes as B↓ / C↑. main.js's whole diff is the deletions plus two more names on the lanes.js import. Graph unchanged and still acyclic: lanes -> state. @pure:fret-pitch was the most-sliced block left in the suite — five consumers. All five now drive the real function: - fret_key_highlight: was slicing fret-pitch + brace-extracting _absolutePitch; now imports _soundingPitchPure and is down to the one brace-extract. - suggest_position, roll_position_cycle, suggest_position_move, view_switcher: hybrids. They still slice the blocks that remain in main.js (suggest-position, position-cycle, suggest-marks, noteToMidi, _rollMidiForNote, updatePianoRange) but inject the REAL _soundingPitchPure into their sandbox instead of concatenating its source text. Four more files off the CJS slicer path. _openMidiForArr had NO test. tests/lanes.test.mjs now covers it: standard guitar and bass tunings, extended low strings a perfect fourth below the current lowest, the 6-string-bass special case (append high C, keep the low B underneath), the bass-E1-is-an-octave-below-guitar-E2 relationship, and capo-added-exactly-once through _soundingPitchPure. Verified: node --test 86/86, pytest 248/248. main.js diff mechanically checked to be deletions + two import names. No unused import; the two open-MIDI constants are module-private (nothing outside lanes.js reads them). All four headless Chromium harnesses green — the draw harness's 39 (degree-label, colour) pairs run straight through the moved _soundingPitchPure on the real fretted draw path. This is the prerequisite for step 8b (src/keys.js, the piano-roll / keys-mode model), whose _rollPitchCtx and _rollMidiForNote are the two callers. 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 (9)
📝 WalkthroughWalkthroughOpen-string MIDI pitch and sounding-pitch calculation logic is extracted from src/main.js into new exported functions in src/lanes.js. main.js is updated to import these helpers and its absolute-pitch formula is revised to drop capo. Multiple test files are converted to ESM and updated to import/inject the new helpers directly instead of extracting source blocks. ChangesOpen-string pitch model migration
Estimated code review effort: 3 (Moderate) | ~25 minutes 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 There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ast-grep (0.44.1)src/main.jsast-grep timed out on this file Comment |
There was a problem hiding this comment.
Pull request overview
Refactors the editor’s fretted-string pitch model by relocating the open-string MIDI tables and the sounding-pitch pure helper into src/lanes.js, aligning pitch logic with the lane/string model as part of the ongoing ES-module split (R2 step 8a).
Changes:
- Moved
_openMidiForArrplus module-private open-string MIDI constants and_soundingPitchPurefromsrc/main.jsintosrc/lanes.js, and updatedmain.jsimports accordingly. - Updated multiple tests that previously source-sliced
@pure:fret-pitchto instead import and inject the real_soundingPitchPurefromsrc/lanes.js. - Added direct unit coverage for
_openMidiForArrand_soundingPitchPurebehavior intests/lanes.test.mjs, and documented the change inCHANGELOG.md.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| tests/view_switcher.test.mjs | Switches from slicing @pure:fret-pitch to importing/injecting _soundingPitchPure from src/lanes.js. |
| tests/suggest_position.test.mjs | Injects real _soundingPitchPure into the @pure:suggest-position sandbox instead of concatenating the old block. |
| tests/suggest_position_move.test.mjs | Injects _soundingPitchPure into the move environment to decouple from the removed fret-pitch block. |
| tests/roll_position_cycle.test.mjs | Injects _soundingPitchPure for the position-cycle sandbox after the helper move. |
| tests/lanes.test.mjs | Adds explicit tests for _openMidiForArr edge cases and capo composition via _soundingPitchPure. |
| tests/fret_key_highlight.test.mjs | Replaces fret-pitch block extraction with a real import of _soundingPitchPure, leaving only _absolutePitch brace-extraction. |
| src/main.js | Removes the open-string MIDI tables and @pure:fret-pitch block; imports _openMidiForArr and _soundingPitchPure from lanes.js. |
| src/lanes.js | Adds the open-string MIDI constants, _openMidiForArr, and exports _soundingPitchPure alongside lane model code. |
| CHANGELOG.md | Records ES-module migration step 8a and newly added test coverage for the moved helpers. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
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
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.
Step 8a of the editor's ES-module split (R2). A small move that unblocks a big one — and it retires the most-sliced
@pure:block left in the suite.What moved
Into
src/lanes.js:_GUITAR_OPEN_MIDI/_BASS_OPEN_MIDI(module-private),_openMidiForArr, and the@pure:fret-pitchhelper_soundingPitchPure.They're the string→pitch half of the same model.
lanes.jsalready knows bass-vs-guitar and string counts, and_openMidiForArr's extended-range rules — a perfect fourth below the current lowest string, except a 6-string bass appends a high C — are the very ruleslaneLabels()encodes asB↓andC↑. Keeping them in two files was the split.main.js's entire diff is the deletions plus two more names on itslanes.jsimport (checked by reconstructing it). Graph unchanged, still acyclic.Five source-slicers, one block
@pure:fret-pitchhad more consumers than any block left in the suite. All five now drive the real function:fret_key_highlight— was slicingfret-pitchand brace-extracting_absolutePitch. Now imports_soundingPitchPureand is down to the single brace-extract.suggest_position,roll_position_cycle,suggest_position_move,view_switcher— hybrids. They still slice what remains inmain.js(suggest-position,position-cycle,suggest-marks,noteToMidi,_rollMidiForNote,updatePianoRange), but inject the real_soundingPitchPureinto their sandboxes rather than concatenating its source.That's four more files off the CJS slicer path.
suggest_positionneeded one extra touch: importing the name collided with its own destructure of the sandbox result._openMidiForArrhad no test at allNow it does, in
tests/lanes.test.mjs:[40,45,50,55,59,64]and bass[28,33,38,43]_soundingPitchPureVerification
node --test86/86,pytest248/248.lanes.jsreads them. No dangling@pure:fret-pitchreference anywhere.(degree-label, colour)pairs run straight through the moved_soundingPitchPureon the real fretted draw path, since the in-key highlight resolves each note to sounding pitch.Next
This is the prerequisite for step 8b —
src/keys.js, the piano-roll / keys-mode model (isKeysMode,viewFor,pianoLaneCount,midiToY,updatePianoRange,_rollPitchCtx,_rollMidiForNote). Those last two are exactly the callers of what moved here, andupdatePianoRangeis the sole writer ofPIANO_LANE_H/pianoRange— so by the step-5 rule, they go as live bindings with no container.keys.jsin turn is what Drawing (1,046 lines) is coupled to.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Tests