refactor(editor): extract the keys / piano-roll model to src/keys.js (R2, step 8b) - #155
Merged
Merged
Conversation
…(R2, step 8b)
src/main.js 20,127 -> 19,963 — under 20k for the first time. keys.js holds which
view a part opens in (viewFor, isKeysMode, isKeysArr, _rollReadOnly) and the
persisted per-part preference behind it, the roll's MIDI<->y geometry (midiToY,
yToMidi, pianoLaneCount, noteToMidi/midiToNote/midiToString/midiToFret, isBlackKey,
midiToFreq, PIANO_OCTAVE_COLORS, KEYS_PATTERN), and the sounding-pitch context that
renders a fretted part read-only in the roll (_rollPitchCtx, _rollMidiForNote).
_rollLockNotice stays in main.js — it calls setStatus.
main.js's whole diff is the deletions plus the import block. Graph stays acyclic:
keys -> {geometry, lanes, notes, state, theory}.
LIVE BINDINGS, not a container. PIANO_LANE_H and pianoRange are reassigned per
arrangement, but their SOLE writer — updatePianoRange — moved with them, so they
are `export let` and importers read them live and cannot write them. That is the
step-5 rule (geometry's lane metrics), not the step-4 one (lanes.js's LC, whose
writers had to stay in draw()/onMouseMove()). The question is always "can the
writer move?", not "is it reassigned?".
Tests: four more suites off the slicer path.
- keyboard_gutter, keyboard_gutter_dblclick: @pure:midi-freq is gone; both
import midiToFreq / _inKeyboardGutterPure. dblclick still brace-extracts
onDblClick, which stays in main.js.
- rename_part: was regex-lifting `const KEYS_PATTERN = ...` out of the source;
now imports it and injects it into the @pure:rename-arr sandbox.
- view_switcher: the real rework. It used to re-evaluate _viewPrefs/_viewPrefsSave/
viewFor/isKeysArr/isKeysMode/_rollReadOnly/updatePianoRange from SOURCE inside a
sandbox, against a fabricated `S` and an injected localStorage stub. It now
drives the real module against the real S, installs the stub on globalThis
(module code resolves `localStorage` at call time), bounces S.filename to bust
_viewPrefs' per-song memo between cases, and reads pianoRange back through the
NAMESPACE — a destructured copy would go stale, so that read is what proves the
live binding actually updates for importers.
Verified: node --test 86/86, pytest 248/248. main.js diff mechanically checked to
be deletions + the import block. No assignment to PIANO_LANE_H/pianoRange survives
in main.js (it would now throw); the only writers are the four lines inside
updatePianoRange. No unused import; _viewForPure is the one test-only export.
FIFTH headless harness, written for this step: no existing harness opened the roll
(they all use a 6-string guitar chart). It discriminates the two views by what they
paint — a fretted part draws bold-12px lane labels, a roll draws one keyboard-gutter
fillRect per semitone of pianoRange. On Arcturus: Lead → 6 lane labels / 0 gutter
rows; Keys → 0 labels / 60 rows; Lead forced into the roll via the view pref → 60
rows, i.e. SOUNDING pitch (the wire packing string*24+fret would span ~140 rows);
switching back restores the lanes. The C-row octave labels are not usable as a
signal — drawPianoLabels only draws them when PIANO_LANE_H >= 7, and a wide range
puts it at 4-6px. All four existing harnesses green.
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 (7)
📝 WalkthroughWalkthroughThis PR extracts piano-roll/keys logic from ChangesKeys/piano-roll model extraction
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
Note
Copilot was unable to run its full agentic suite in this review.
This PR continues the ES-module migration by extracting the keys/piano-roll model out of src/main.js into a dedicated src/keys.js module and updating tests to import real implementations instead of slicing/eval’ing source blocks.
Changes:
- Added
src/keys.jscontaining view-pref logic, roll pitch mapping, and piano-roll geometry/state. - Updated
src/main.jsto import keys/piano-roll constants and functions fromsrc/keys.js. - Updated multiple test suites to use real module imports and simplified extraction/sandboxing; updated changelog entry.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/view_switcher.test.mjs | Switches from source-slicing to importing src/keys.js, stubs globalThis.localStorage, and asserts live export bindings. |
| tests/rename_part.test.js | Migrates test to ESM-style imports and uses real KEYS_PATTERN. |
| tests/keyboard_gutter_dblclick.test.js | Migrates test to ESM-style imports and uses real _inKeyboardGutterPure. |
| tests/keyboard_gutter.test.js | Migrates test to ESM-style imports and uses real midiToFreq / _inKeyboardGutterPure. |
| src/main.js | Removes inlined keys/piano-roll code and imports equivalents from src/keys.js. |
| src/keys.js | New module implementing keys/piano-roll model, including persisted view prefs and roll geometry helpers. |
| CHANGELOG.md | Documents the extraction/migration and the updated testing approach. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // keys.js reads the real `S` and the ambient `localStorage`. Install a stub on | ||
| // globalThis (module code resolves it at call time) and seed the real S, instead | ||
| // of re-evaluating the module's source inside a sandbox. | ||
| function makeViewEnv(seedS, seed = {}) { |
Comment on lines
74
to
79
| const map = new Map(Object.entries(seed)); | ||
| const ls = { | ||
| globalThis.localStorage = { | ||
| getItem: k => (map.has(k) ? map.get(k) : null), | ||
| setItem: (k, v) => { map.set(k, String(v)); }, | ||
| removeItem: k => { map.delete(k); }, | ||
| map, | ||
| }; |
Comment on lines
+55
to
+63
| export function _viewPrefs() { | ||
| const key = 'editorViewPref:' + (S.filename || ''); | ||
| if (_viewPrefFor === key && _viewPrefCache) return _viewPrefCache; | ||
| _viewPrefFor = key; | ||
| _viewPrefCache = {}; | ||
| // Unsaved songs don't read a bare slot every unsaved song would share. | ||
| if (!S.filename) return _viewPrefCache; | ||
| try { | ||
| const raw = localStorage.getItem(key); |
This was referenced Jul 10, 2026
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 8b of the editor's ES-module split (R2).
main.jsdrops under 20k for the first time: 20,127 → 19,963.What moved
src/keys.js(206 lines) — everything about the piano roll and which view a part opens in.viewFor,isKeysMode,isKeysArr,_rollReadOnly, plus the persisted per-part preference (_viewPrefs,_viewPrefsSave,_partViewKeyPure,_viewForPure,KEYS_PATTERN)midiToY,yToMidi,pianoLaneCount,pianoRange,PIANO_LANE_H,updatePianoRange,PIANO_OCTAVE_COLORSnoteToMidi,midiToNote,midiToString,midiToFret,isBlackKey,midiToFreq,_inKeyboardGutterPure_rollPitchCtx,_rollMidiForNote_rollLockNoticestays behind — it callssetStatus.main.js's entire diff is the deletions plus the import block. Graph stays acyclic:keys → {geometry, lanes, notes, state, theory}.Live bindings, not a container
PIANO_LANE_HandpianoRangeare reassigned per arrangement. But their sole writer,updatePianoRange, moved with them — so they'reexport let, importers read them live, and an importer that tries to write gets aTypeError.That's the step-5 rule (geometry's lane metrics), not the step-4 one (
lanes.js'sLC, whose writersdraw()andonMouseMove()had to stay behind). The question is always "can the writer move?", never "is it reassigned?". No container, no rename:main.js's 24PIANO_LANE_Hand 9pianoRangereferences are untouched.Tests — four more off the slicer path
keyboard_gutter,keyboard_gutter_dblclick—@pure:midi-freqretired; both importmidiToFreq/_inKeyboardGutterPure.dblclickstill brace-extractsonDblClick, which stays inmain.js.rename_part— was regex-liftingconst KEYS_PATTERN = …straight out of the source text. Now imports it and injects it into the@pure:rename-arrsandbox.view_switcher— the real rework. It used to re-evaluate_viewPrefs/viewFor/isKeysArr/isKeysMode/_rollReadOnly/updatePianoRangefrom source inside anew Functionsandbox, against a fabricatedSand an injectedlocalStorage. It now drives the real module against the realS, installs the stub onglobalThis(module code resolveslocalStorageat call time), bouncesS.filenameto bust_viewPrefs' per-song memo between cases, and readspianoRangeback through the namespace — a destructured copy would go stale, so that read is what proves the live binding actually updates for importers.Verification
node --test86/86,pytest248/248.No surviving assignment to
PIANO_LANE_H/pianoRangeanywhere inmain.js— one would now throw. The only writers are the four lines insideupdatePianoRange. No unused import;_viewForPureis the single test-only export.A fifth headless harness, written for this step, because no existing one ever opened the roll — draw, hit-test and resize all use a 6-string guitar chart. It discriminates the two views by what they paint: a fretted part draws bold-12px lane labels; a roll draws one keyboard-gutter
fillRectper semitone ofpianoRange.E A D G B eE A D G B eThat row count is the assertion: 60 rows means the roll rendered the fretted part at sounding pitch. The wire packing
string*24 + fretwould span roughly 140 rows for the same notes. (The C-row octave labels are unusable as a signal —drawPianoLabelsonly draws them whenPIANO_LANE_H >= 7, and a wide range puts it at 4–6px. Cost me a probe to find out.)All four existing harnesses green. Codex preflight: NO ISSUES.
Next
keys.jswas what Drawing (1,046 lines) and Hit-testing were coupled to. Step 9 can take them, oncecanvas/ctxare lifted — and their writer is a singleinit()assignment fromdocument.getElementById, so by the rule above it moves assetCanvas(el)and they too become live bindings.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Tests