refactor(editor): move EditHistory to src/history.js (R2, step 14) - #165
Merged
Conversation
src/main.js 18,592 -> 18,471. Seventeenth module; the graph stays acyclic. The 47 command classes stay in main.js: they are interleaved with the feature code that constructs them and each reaches deep into it. Only the stack lifts cleanly, and it needs nothing from them beyond the duck-type it always used (exec/rollback plus songScope/pitchPreserving/suggestResolved). history.js imports S + bumpEditGen from state.js and isKeysMode / updatePianoRange / _rollReadOnly / _rollLockNotice from keys.js. The three remaining main.js symbols — _historyEnsureArr, draw, updateStatus — would close a cycle, so they arrive through setHistoryHooks(), matching canvas.js's setCanvas() and geometry.js's setLaneMetrics(). All three are hoisted function declarations, so the top-level wiring call is order-safe. The typeof guards throughout the old block existed only because it was sliced into test sandboxes that lacked those symbols. With real imports they are dead, so they are gone, and the three duplicated read-only-roll checks in exec/doUndo/doRedo collapse into one _locked(cmd). Tests: 13 suites sliced @pure:edit-history and fabricated their own `S`, which the sliced command classes then shared. Importing the real class would have split that object in two, so they now seed the REAL S (tests/_history_env.mjs) and drive the REAL lock through real view state instead of stubbing _rollReadOnly to a boolean. That exposed a lie in the old harness: roll_position_cycle forced _rollReadOnly() => true for a keys-DATA part, which is never read-only. Six suites were CJS and are now .mjs. Verified beyond the unit tests, because unit tests cannot see the hook wiring — they construct EditHistory directly. A headless harness deletes a note, undoes, and redoes, counting repaints via a clearRect frame hook. Commenting out the setHistoryHooks() call makes it fail on exactly the silent half: the model reverts, the canvas keeps the stale paint, and the undo/redo buttons still look right. That is the step-9b failure mode, caught this time. node --test 88/88, pytest 248/248, npm run lint 0 errors, Codex clean, all 10 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 (18)
📝 WalkthroughWalkthrough
ChangesEditHistory module extraction and test harness migration
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)
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
This PR refactors the editor’s undo/redo stack by extracting EditHistory from the monolithic src/main.js into a dedicated module (src/history.js), while preserving the existing command “duck typing” contract and avoiding import cycles via an explicit hook-wiring function.
Changes:
- Moved
EditHistoryintosrc/history.jsand replacedmain.js’s in-file implementation with an import +setHistoryHooks({ ensureArr, draw, updateStatus })wiring call. - Consolidated the read-only-roll gate into a single
_locked(cmd)helper and removed the former sandbox-onlytypeofguards (now real imports). - Updated affected test suites to import the real
EditHistory, seed the realS, and drive the real roll lock through shared helpers intests/_history_env.mjs.
Reviewed changes
Copilot reviewed 18 out of 18 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| src/history.js | New module containing EditHistory, read-only lock gating, stack cap, and hook wiring for cycle-breaking callbacks. |
| src/main.js | Imports EditHistory/setHistoryHooks and wires hooks to _historyEnsureArr, draw, and updateStatus; removes the old inlined class block. |
| tests/_history_env.mjs | New shared helper to seed real state, stub minimal DOM/status plumbing, and install counting history hooks for suites. |
| tests/view_switcher.test.mjs | Switches from slicing @pure:edit-history to importing EditHistory and using _history_env to drive lock state. |
| tests/suggest_position_wiring.test.mjs | Imports real history and predicates; rewires env creation to share real S.history and observe real lock notice/status. |
| tests/strings_modal.test.mjs | Stops slicing edit-history; seeds real state and uses real EditHistory in the modal command tests. |
| tests/section_undo.test.mjs | Converts to ESM and uses imported EditHistory with real S seeding + hook tracking. |
| tests/section_coverage.test.mjs | Replaces source-shape slicing of _afterEdit() with behavioral assertion via editGen bump after calling EditHistory._afterEdit(). |
| tests/roll_position_cycle.test.mjs | Imports real lock notice/predicate and uses seeded view state to reflect the true lock semantics (incl. keys parts not being read-only). |
| tests/roll_edge_resize.test.mjs | Uses real EditHistory and real _rollReadOnly driven through seeded state rather than stubbed predicates. |
| tests/rename_part.test.mjs | Seeds real S and uses imported EditHistory so sliced command shares the same state object history closes over. |
| tests/inspector_time.test.mjs | Converts to ESM and uses imported EditHistory for dispatcher history instances; removes sliced edit-history block. |
| tests/edit_history_reset.test.mjs | Imports EditHistory and relies on _history_env’s DOM stubs for undo/redo button state checks. |
| tests/duplicate_selection.test.mjs | Seeds real S + hooks once and removes the sliced history block dependency for this suite. |
| tests/drum_velocity.test.mjs | Converts to real S seeding + imported EditHistory; removes sliced history usage. |
| tests/drum_undo.test.mjs | Converts to ESM and uses imported EditHistory with real S seeding + hook tracking. |
| tests/cross_arr_undo.test.mjs | Uses setHistoryHooks() to connect sliced _historyEnsureArr to the imported EditHistory without creating a module cycle. |
| CHANGELOG.md | Documents the refactor, the cycle-breaking hook wiring, and the test harness changes. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
byrongamatos
added a commit
that referenced
this pull request
Jul 9, 2026
…s (R2, step 16) (#167) * refactor(editor): move the annotation lanes to src/annotation-lanes.js (R2, step 16) src/main.js 17,756 -> 16,078. Nineteenth module; the graph stays acyclic. 1,678 lines — the tone lane, the anchor lane and the handshape lane, which turned out to be a contiguous tail of the IIFE. They move together because they lean on each other: the handshape lane positions itself off _anchorLaneTopY, and both it and the anchor lane share _currentAnchorArr. Split apart, those would be cross-module imports for no gain. main.js keeps the canvas event routing (deciding which strip is under the cursor) and forwards to the on*LaneMouse* handlers. Four main.js symbols travel back — draw, hideContextMenu, snapTime, _editorPromptText — and would close a cycle, so they arrive through setLaneHooks(). snapTime stays behind because its onset-snap path reaches _ensureOnsets and the onset cache; _editorPromptText stays because it owns a modal and the shared _editorPromptCancel handle. Neither is a lane concern. TONE_LANE_H moved to geometry.js, where ANCHOR_LANE_H and HS_LANE_H already live. The tones modal's three window.* handlers became exported functions that main.js re-attaches: a top-level `window.x =` throws when the module is imported under node. Two internal call sites that reached back through window.editorHideTonesModal() now call the module-local function (Codex). Also fixes a regression this refactor's own predecessors introduced. `draw` is reassigned near the bottom of main.js to a wrapper that refreshes seven toolbar buttons before repainting. setHistoryHooks() and setDrumHooks() were handed the bare identifier, so they captured the ORIGINAL function at wiring time; every undo, redo and drum-density toggle has been skipping those refreshes since #165/#166. The canvas repaints either way, which is exactly why it went unnoticed — the only visible symptom is the drum-density button keeping its "Rows: Full" label after the grid has collapsed to Compact. All three hook sites now take `_drawLive = (...args) => draw(...args)`, resolving the live binding at call time as the in-IIFE call sites always did. Found by Codex; verify_drum.py now asserts the label, and fails on the pre-fix code. Tests: anchor_authoring (CJS -> .mjs) and handshape_authoring stop brace-matching declarations out of main.js and import them. Verified beyond the unit tests, which cannot see hook wiring: verify_lanes.py clicks the anchor lane and asserts an anchor is added, the canvas repaints (_hooks.draw), the anchor lands on the grid rather than under the cursor (_hooks.snapTime), and the tones modal opens through the re-attached window.* handler. Comment out setLaneHooks() and all 88 unit tests still pass while three of its eight checks fail. node --test 88/88, pytest 248/248, npm run lint 0 errors, Codex clean, all 12 headless harnesses pass. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * docs(editor): correct stale IIFE / brace-matching comments in the moved code Copilot, on #167. The three comments travelled verbatim with the code and now describe an ES module: TONE_LANE_H comes from geometry.js rather than being declared at the top of the IIFE; _editorConfirmToneDefinitions is an ordinary export, not a plain function relying on the global -> IIFE fallback; and _handshapeSpanFrets is imported, not brace-matched out of main.js. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
byrongamatos
added a commit
that referenced
this pull request
Jul 9, 2026
… step 17) (#168) * refactor(editor): collapse the per-module hooks into src/host.js (R2, step 17) history.js, drum.js and annotation-lanes.js each grew a setXHooks() for the same reason: they need a few main.js symbols that cannot be imported back without closing a cycle. By the fourth module the SAME four callbacks — draw, hideContextMenu, snapTime, editorPromptText — were being threaded through three separate hook objects, and the next extraction (the note command classes) needs nine. That is the moment to stop duplicating. One `host` object, wired once. A new module imports `host` and calls `host.draw()`; no new plumbing, no fourth setter. No behaviour change: every former _hooks.X() call site resolves to the same callback (verified by Codex, one-for-one, including the two renamed keys _editorPromptText -> editorPromptText and ensureArr). The draw thunk stays, and host.js's header now carries the warning where the next person will actually read it: `draw` is reassigned near the bottom of main.js to a button-refreshing wrapper, so passing the bare identifier captures the original function and the refreshes silently stop. That shipped in #165/#166. The header says: pass a thunk, and check `grep -n '^\s*<name> = '` before wiring anything. The inert defaults are type-honest rather than uniformly no-op — snapTime is the identity, editorPromptText resolves to null (a cancelled prompt) — so a module imported under node with no host wired degrades instead of crashing. That is exactly how the unit tests exercise them, which is also why the unit tests cannot see the wiring: comment out setHostHooks() and all 88 still pass, while verify_history.py, verify_drum.py and verify_lanes.py all fail. node --test 88/88, pytest 248/248, npm run lint 0 errors, Codex clean, all 12 headless harnesses pass. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * docs(editor): make the host.js grep portable and drop stale setXHooks references Copilot, on #168. - host.js suggested `grep -n '^\s*<name> = '`. GNU grep accepts `\s` as an extension so it works here, but BSD/macOS grep treats it as a literal 's' and returns a silent zero match — the worst possible answer for a check whose whole job is to catch a reassignment. Switched to a POSIX class and said why. - drum_undo.test.mjs comment still named setDrumHooks. - Three CHANGELOG entries in the same Unreleased block still described the per-module setters that this PR removes, so the release notes contradicted the code they ship with. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
byrongamatos
added a commit
that referenced
this pull request
Jul 9, 2026
… 19) (#170) * refactor(editor): move the Tempo Map editor to src/tempo.js (R2, step 19) src/main.js 15,042 -> 13,347. Twenty-second module; the graph stays acyclic. 1,720 lines: the measure model, _tempoMapDraw, the mouse handlers, the sync inspector, tap-tempo, beat-lock respacing, and the two undo commands (TempoGridCmd, TempoMapCmd). main.js is now 37% smaller than the 21,176 lines this refactor started from. The banner said 2,179 lines, but ~440 of those are the drum editor's mouse handlers and toolbar buttons, which have no banner of their own and were left behind by the drum.js lift. The real tempo region ends at TempoMapCmd. Cutting there is what kept the dependency surface honest. main.js keeps _finalizeActiveDrag. It dispatches whatever canvas drag is in flight — tempo, drum, handshape, pan — before a mode switch, so it belongs to none of them; it reaches back as host.finalizeActiveDrag(). Fifteen main.js symbols travel the other way: the transport (startPlayback / stopPlayback), the A/B loop strip (four callbacks), the toolbar readouts, and getMousePos. `_recState` is a REASSIGNED module scalar rather than a function, so it cannot cross as a value at all — it is wired as the predicate `host.isRecording: () => _recState === 'recording'`, a closure that reads the live binding. Same class of trap as `draw` in #165/#166, caught this time by looking for it. Tests: 15 suites stopped slicing @pure: blocks and command classes out of main.js. Ten were CJS and are now .mjs. A few now rely on host's inert defaults where they used to inject no-op stubs — equivalent, and Codex confirmed it. Verified beyond the unit tests, which cannot see host wiring: verify_tempo.py arms drum-edit mode, enters Tempo Map, and asserts the canvas repaints, that _tempoMapDraw paints its HUD line, and that the drum button relabels itself from "🎸 Back to Notes" back to "🥁 Edit Drums" — the only visible proof that host.refreshDrumEditButton fired when tempo mode kicked drum mode out. Comment out `draw` and `refreshDrumEditButton` and all 89 unit tests still pass while four of the harness's nine checks fail. node --test 89/89, pytest 248/248, npm run lint 0 errors (7 warnings, was 8), Codex clean, all 14 headless harnesses pass. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * docs(editor): drop the dead sync-inspector memo and correct stale headers Copilot, on #170. - _tempoSyncInspectorState was computed, assigned, and never compared: dead code, and a lint warning. Removed rather than 'completed' into an early return, because that early return would be WRONG. The DOM this function writes is not a pure function of the signature: the BPM field is deliberately left alone while it has focus, so skipping the writes on an unchanged signature would strand whatever the user typed and abandoned, with nothing else to restore it. The comment now says so. - tempo.js's header hard-coded 'Fourteen main.js symbols'. The number was already wrong (fifteen) and would rot again. Removed. - Five test headers still said the helpers they import live in src/main.js. Copilot named two; the other three had the same defect. The five suites that genuinely still slice main.js keep their references, which are accurate. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
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.
Seventeenth module.
src/main.js18,592 → 18,471; the graph stays acyclic.What moved, and what didn’t
EditHistoryonly. The 47 command classes stay inmain.js— they are interleaved with the feature code that constructs them and each reaches deep into it. The stack needs nothing from them beyond the duck-type it always used (exec/rollback, plussongScope/pitchPreserving/suggestResolved), so it lifts cleanly on its own.history.jsimportsS+bumpEditGenfromstate.js, andisKeysMode/updatePianoRange/_rollReadOnly/_rollLockNoticefromkeys.js. Three symbols remain inmain.jsand would close a cycle:_historyEnsureArr,draw,updateStatus. They arrive viasetHistoryHooks()— the same shape ascanvas.js’ssetCanvas()andgeometry.js’ssetLaneMetrics(). All three are hoisted function declarations, so the top-level wiring call is order-safe.The
typeofguards scattered through the old block (typeof _rollReadOnly === function) existed only because it was sliced into test sandboxes that lacked those symbols. Real imports make them dead, so they are gone — and the three duplicated read-only-roll checks inexec/doUndo/doRedocollapse into one_locked(cmd).The test problem, and what fixing it exposed
Thirteen suites sliced
@pure:edit-historyout ofmain.jsand fed it a fabricatedS, which the sliced command classes then shared. Importing the real class would have split that object in two — the fabricated-Strap. They now seed the realSthroughtests/_history_env.mjs, and drive the real lock through real view state rather than stubbing_rollReadOnlyto a boolean.That immediately caught a lie in the old harness:
roll_position_cycleforced_rollReadOnly() ⇒ truefor a keys-DATA part, which is never read-only. The new precondition assert refuses to pretend otherwise.Six of the thirteen were CJS and are now
.mjs.section_coveragestopped text-slicing_afterEdit()and now just calls it, assertingeditGenactually advances.Why a headless harness, again
Unit tests construct
EditHistorydirectly, so they are structurally blind to the hook wiring.verify_history.pydeletes a note, undoes, redoes, and counts repaints through aclearRectframe hook.Commenting out the
setHistoryHooks(...)call makes it fail on exactly the silent half:The model reverts, the canvas keeps the stale paint, and the buttons still look right. That is the step-9b failure mode — caught this time.
Verification
node --test88/88 ·pytest248/248 ·npm run lint0 errors (10 pre-existing warnings) · Codex clean · all 10 headless harnesses pass.The
main.jsdiff is 121 deletions against 6 added lines: the import, and the hook call with its comment.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes