refactor(editor): move the note command classes to src/commands.js (R2, step 18) - #169
Conversation
…2, step 18) src/main.js 16,088 -> 15,042. Twenty-first module; the graph stays acyclic. All 18 note command classes plus the helpers that construct and execute them. This is the chunk the previous six steps kept working around: the memory note said "47 command classes remain, interleaved with their feature code". They are not, in fact, all interleaved — 18 of them sit in one contiguous run under the Undo/Redo banner, and step 17's shared `host` object is what made the lift cheap. Nine main.js callbacks would otherwise have needed a ninth setXHooks(). commands.js is DOM-FREE, and that is the boundary, not a coincidence. Two functions would have broken it, so they moved back to main.js: _rollConfirmPosition builds the ambiguous-pitch popover _resizeForLaneChange schedules a requestAnimationFrame Both arrive as host hooks. Keeping them out is exactly what lets the suites import the real classes under node with no document — and it removed the last `typeof resizeCanvas === 'function'` guard, which only ever existed to keep a sliced block eval-able in a sandbox. Tests: eight suites stopped brace-matching classes out of main.js. suggest_position_wiring lost its entire 40-line sandbox and 46 lines overall. import_guitar_track was CJS and is now .mjs. Converting strings_modal exposed a fixture that only made sense under a stub. Its "low-side add" case used a 4-string 'Banjo' against an injected `arr => arr.tuning.length`. The REAL _stringCountFor has no banjo: it reads a 4-length tuning on a non-bass part as a padded guitar and reports 6, so the add produced 7 strings, not 5. The fixture is a 'Bass' now — the behaviour under test (low add shifts lanes, undo round-trips) is unchanged, but it is finally being tested against the model the editor actually ships. While picking this step I found and fixed a bug in my own section-coupling scanner: it stripped strings with regexes, so `/\//g` read as a line comment and deleted the rest of the line, and `/['"]/` opened a phantom string that ate code until the next quote. It under-reported this region's dependencies by three. Rewritten as a single-pass tokenizer that tracks regex literals, with a self-check that no top-level declaration is lost (saved alongside the harnesses). Verified beyond the unit tests, which cannot see host wiring: verify_commands.py selects a note, presses Up, and asserts it moves one lane, the canvas repaints, the inspector follows, and undo restores both. Comment out the single `editorCurrentNoteIndices` hook and all 88 unit tests still pass while four of its seven checks fail — the inert default returns [], so _execMoveString finds nothing to move and reports nothing. node --test 88/88, pytest 248/248, npm run lint 0 errors, Codex clean (it diffed commands.js against HEAD:src/main.js and found zero deltas beyond the expected `export` and `host.*` rewrites), all 13 headless harnesses pass. 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 (4)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe PR moves command execution into ChangesCommand layer refactor
Estimated code review effort: 4 (Complex) | ~60 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)
Comment |
There was a problem hiding this comment.
Pull request overview
Refactors the editor to move the contiguous block of 18 “note command” undo/redo classes (and their helper executors) out of src/main.js into a new src/commands.js, and updates the host-hook wiring and test harnesses to import the real command implementations under node.
Changes:
- Introduces
src/commands.jscontaining the note command classes plus helpers such as move-to-string, suggest-position write path, string add/remove, and chart-replace swap/restore. - Updates
src/main.jsto import these commands/helpers and wires additional host hooks (editBlipAt,editorCurrentNoteIndices,renderInspector,resizeForLaneChange,rollConfirmPosition). - Refactors multiple unit tests to stop slicing classes/functions out of
main.jsand instead import real commands + use host hooks.
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/suggest_position_wiring.test.mjs | Switches from slicing main.js blocks to importing commands + wiring required host hooks. |
| tests/suggest_position_move.test.mjs | Imports real pitch-move command/helpers and seeds real state rather than eval-slicing. |
| tests/strings_modal.test.mjs | Imports string commands/normalize helper; stubs required host hook for resize side effects. |
| tests/roll_position_cycle.test.mjs | Imports real cycle-position executor and move-to-string command; uses host hook for selection. |
| tests/roll_edge_resize.test.mjs | Imports sustain resize commands directly instead of brace-slicing. |
| tests/inspector_time.test.mjs | Uses real commands; ensures the “real S” notes array matches the test-controlled array. |
| tests/import_guitar_track.test.mjs | Converts to ESM; imports swap/restore helpers and continues slicing the remaining import helpers. |
| tests/cross_arr_undo.test.mjs | Imports MoveNoteCmd directly and removes sliced class extraction. |
| src/main.js | Removes inlined note commands/helpers; imports from commands.js and expands host-hook wiring. |
| src/host.js | Adds additional host hook slots needed by commands.js while keeping inert defaults for node. |
| src/commands.js | New module containing the 18 note command classes + helper/executor functions. |
| CHANGELOG.md | Documents the refactor, the DOM boundary intent, and the associated test updates. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
src/main.js (1)
1211-1234: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove the stale AddStringCmd doc fragment here
Theposition/string-count paragraph doesn’t describe_resizeForLaneChange; keep only the layout-side-effect note. If that text is still needed, move it ontoAddStringCmdinsrc/commands.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/main.js` around lines 1211 - 1234, The leading `position`/string-count documentation inside `_resizeForLaneChange` is stale and does not belong on this helper. Remove that AddStringCmd-specific paragraph from `src/main.js` and keep only the layout-side-effect explanation for `_resizeForLaneChange`; if that description is still needed, move it to `AddStringCmd` in `src/commands.js` instead.tests/import_guitar_track.test.mjs (1)
26-41: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winNaive brace-counter extraction duplicated across 3 test files.
extractFnhere,extractNamedintests/inspector_time.test.mjs, andextractReintests/cross_arr_undo.test.mjsare three independent, near-identical brace-matching helpers, none of which are string/regex/comment-aware. That's the same failure mode the changelog says was just fixed in the section-coupling scanner (naive brace/regex stripping mis-parsing regex literals). It's harmless today only because the extracted functions happen to contain no{/}inside strings or regexes — a fragile invariant to rely on silently across three copies of the same logic.Consider factoring one shared, slightly more robust extraction helper into a test-utils module and having all these suites import it.
🤖 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/import_guitar_track.test.mjs` around lines 26 - 41, The brace-based source extraction logic is duplicated and fragile across this test and the other two suites. Factor the repeated helper into a shared test utility and update the callers in extractFn, extractNamed, and extractRe to use it so the brace-matching behavior is centralized and easier to harden against strings, regexes, and comments.
🤖 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 12-14: The changelog entry in the note-command classes summary is
truncated: the `src/main.js` size statement should include the missing unit.
Update the sentence around the `src/main.js` reference so it reads naturally and
fully (for example, by adding the intended unit after the number), preserving
the rest of the wording in `CHANGELOG.md`.
In `@src/commands.js`:
- Line 29: Remove the unused midiToNote import from the import list in
commands.js. Keep the other symbols from keys.js that are actually used, and
update the import statement so it only includes PIANO_LANE_H, _rollPitchCtx, and
isKeysArr.
---
Nitpick comments:
In `@src/main.js`:
- Around line 1211-1234: The leading `position`/string-count documentation
inside `_resizeForLaneChange` is stale and does not belong on this helper.
Remove that AddStringCmd-specific paragraph from `src/main.js` and keep only the
layout-side-effect explanation for `_resizeForLaneChange`; if that description
is still needed, move it to `AddStringCmd` in `src/commands.js` instead.
In `@tests/import_guitar_track.test.mjs`:
- Around line 26-41: The brace-based source extraction logic is duplicated and
fragile across this test and the other two suites. Factor the repeated helper
into a shared test utility and update the callers in extractFn, extractNamed,
and extractRe to use it so the brace-matching behavior is centralized and easier
to harden against strings, regexes, and comments.
🪄 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: 0599ef07-9963-4ac8-af3b-24715eedfc71
📒 Files selected for processing (12)
CHANGELOG.mdsrc/commands.jssrc/host.jssrc/main.jstests/cross_arr_undo.test.mjstests/import_guitar_track.test.mjstests/inspector_time.test.mjstests/roll_edge_resize.test.mjstests/roll_position_cycle.test.mjstests/strings_modal.test.mjstests/suggest_position_move.test.mjstests/suggest_position_wiring.test.mjs
Copilot, on #169. Two findings, both correct: - commands.js claimed to be DOM-free but reached `document` transitively through setStatus (src/ui.js), which throws under node. One guard in the shared function rather than at its ~180 call sites. - `midiToNote` was an unused import. Added tests/commands_dom_free.test.mjs to hold the boundary. It is the one suite that must NOT import tests/_history_env.mjs, since that installs a document stub and would hide exactly this regression. The first draft of that suite was VACUOUS: it asserted `_execMoveString(1)` does not throw, but with an empty selection that returns before ever calling setStatus. Deleting the guard left it green. It now drives `_execCyclePosition(1)`, whose first branch is setStatus('Select notes first') — which fails on the unguarded code, as a guard's test must. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Both correct, both fixed — the first one especially, because it meant the module header was lying.
To hold the boundary I added Worth recording that my first draft of that suite was vacuous. It asserted
|
Twenty-first module.
src/main.js16,088 → 15,042. All 18 note command classes plus the helpers that construct and execute them.The chunk the last six steps kept working around
My own memory note said “47 command classes remain, interleaved with their feature code.” That was only half true — 18 of them sit in one contiguous run under the Undo/Redo banner. And step 17’s shared
hostobject is what made the lift cheap: ninemain.jscallbacks would otherwise have needed a ninthsetXHooks().commands.jsis DOM-free, on purposeTwo functions would have broken that, so they moved back to
main.js:_rollConfirmPosition_resizeForLaneChangerequestAnimationFrameBoth arrive as host hooks. Keeping them out is exactly what lets the suites import the real classes under node with no
document— and it retired the lasttypeof resizeCanvas === functionguard, which only ever existed to keep a sliced block eval-able in a sandbox.Tests
Eight suites stopped brace-matching classes out of
main.js.suggest_position_wiringlost its entire 40-line sandbox and 46 lines overall.import_guitar_trackwas CJS and is now.mjs.Converting
strings_modalexposed a fixture that only made sense under a stub. Its “low-side add” case used a 4-stringBanjoagainst an injectedarr => arr.tuning.length. The real_stringCountForhas no banjo: it reads a 4-length tuning on a non-bass part as a padded guitar and reports 6, so the add produced 7 strings, not 5. It is aBassnow. The behaviour under test is unchanged — it is finally being tested against the model the editor actually ships.A bug in my own tooling
While picking this step, the section-coupling scanner I use to rank candidates told me this region needed 4
main.jssymbols. It needed 7.It stripped strings with regexes, so
/\//gread as a line comment and deleted the rest of its line, and/["]/opened a phantom string that swallowed code until the next quote. Both occur inmain.js. It is now a single-pass tokenizer that tracks regex literals, with a self-check asserting no top-level declaration is lost between raw and stripped source (raw=539 stripped=539 lost=0).no-undefcaught the three it missed here, which is why the boundary is right anyway.Why a headless harness, again
verify_commands.pyselects a note, presses Up, and asserts it moves one lane, the canvas repaints, the inspector follows the model, and undo restores both.editorCurrentNoteIndicesunwiredComment out that one hook and all 88 unit tests still pass. The inert default returns
[], so_execMoveStringfinds nothing to move and reports nothing — no error, no status line, an entirely silent dead key.Verification
node --test88/88 ·pytest248/248 ·npm run lint0 errors (9 warnings) · all 13 headless harnesses pass.Codex diffed
commands.jsagainstHEAD:src/main.jsand found zero deltas beyond the expectedexportkeywords andhost.*rewrites.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes