refactor(editor): move the Tempo Map editor to src/tempo.js (R2, step 19) - #170
Merged
Merged
Conversation
… 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>
There was a problem hiding this comment.
Pull request overview
This PR continues the editor modularization by extracting the Tempo Map editor (rendering, interactions, sync inspector, tap-tempo, beat-lock respacing, and undo commands) out of src/main.js into a new src/tempo.js, and updates the surrounding host-hook wiring and tests to import real module exports instead of slicing @pure: blocks.
Changes:
- Move Tempo Map editor implementation from
src/main.jsinto newsrc/tempo.js, preserving cross-module dependencies viahosthooks. - Update
src/main.jsto import tempo functionality, wire the additionalhosthooks, and remove the inlined Tempo Map block. - Convert/adjust tempo-related and loop/grid-related tests to import from
src/tempo.js(and remove legacy CJS test counterparts), and update the changelog entry.
Reviewed changes
Copilot reviewed 21 out of 21 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| src/tempo.js | New module containing Tempo Map editor logic, commands, pure helpers, and UI wiring. |
| src/main.js | Imports/wires tempo exports via host, and deletes the previously inlined Tempo Map region. |
| src/host.js | Extends the host surface with transport/loop/toolbar hooks needed by src/tempo.js. |
| CHANGELOG.md | Documents the refactor milestone and the new module boundary. |
| tests/tempo_sync_inspector.test.mjs | Switches from slicing main.js to importing _tempoSyncInspectorStatePure from tempo.js. |
| tests/tempo_modulate.test.mjs | Switches from slicing main.js to importing modulation helpers from tempo.js. |
| tests/tempo_map_timesig.test.mjs | New ESM test covering time-signature helpers via real imports from tempo.js. |
| tests/tempo_map_timesig.test.js | Removes legacy CJS slicing-based time-signature test. |
| tests/tempo_map_guidance.test.mjs | Switches from slicing main.js to importing HUD/message pures from tempo.js. |
| tests/tempo_map_bpm.test.mjs | New ESM test covering BPM helpers via real imports from tempo.js. |
| tests/tempo_map_bpm.test.js | Removes legacy CJS slicing-based BPM test. |
| tests/tempo_flatten.test.mjs | Switches to importing flatten/BPM helpers from tempo.js. |
| tests/tempo_beat_drag.test.mjs | Switches to importing _tempoApplyDrag and bounds pure directly from tempo.js. |
| tests/tap_tempo.test.mjs | Switches from slicing main.js to importing tap-tempo pures from tempo.js. |
| tests/midi_tempo_import.test.mjs | Keeps slicing for MIDI-tempo choice pures in main.js, but imports TempoGridCmd from tempo.js. |
| tests/measure_readout.test.mjs | Keeps slicing for readout pure in main.js, but injects _tempoNormalizeDenominatorPure imported from tempo.js. |
| tests/loop_undo_mode.test.mjs | Updates loop/undo harness to use real TempoMapCmd from tempo.js and host hooks. |
| tests/loop_beats.test.mjs | Updates loop/grid harness to use real TempoGridCmd from tempo.js and host hooks. |
| tests/beat_primary.test.mjs | Updates to import beat-primary helpers/command from tempo.js and seed real shared state. |
| tests/beat_lock.test.mjs | Updates to import beat-lock pures from tempo.js while keeping a targeted main.js slice for an integration case. |
| tests/beat_converter.test.mjs | Switches to importing _makeTimeRemap from tempo.js as a real module export. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
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 (8)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (7)
📝 WalkthroughWalkthroughThe Tempo Map editor logic is moved into ChangesTempo Map editor extraction
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Editor
participant main.js
participant tempo.js
participant host
participant State
Editor->>main.js: invoke Tempo Map action
main.js->>tempo.js: call imported editor helper
tempo.js->>State: update beat grid and timed objects
tempo.js->>host: refresh loop and toolbar UI
host-->>Editor: render updated editor state
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 |
…ders 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>
Collaborator
Author
|
All four addressed. The if (document.activeElement !== bpmEl) bpmEl.value = state.bpmValue;The DOM this function writes is not a pure function of On the header count: you are right that it rots. It was already wrong — fifteen, not fourteen — so I removed the number rather than correcting it. On the test headers: you flagged two. Three more had the same defect ( |
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
src/tempo.js (1)
347-360: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDead memo signature —
_tempoSyncInspectorStateis computed but never checked.Unlike
_refreshTempoMapButton'sif (sig === _tempoMapBtnState) return;guard, this function always re-touches the DOM regardless of whethersigchanged. Lint flags the assignment as unused.🔧 Add the missing early-return, matching the sibling function's convention
const sig = `${visible}|${S.tempoSel}|${state.label}|${state.bpmValue}|${state.bpmDisabled}|${state.numeratorValue}|${state.denominatorValue}|${state.signatureDisabled}|${state.canInsert}|${state.canDelete}|${state.hint}`; + if (sig === _tempoSyncInspectorState) return; _tempoSyncInspectorState = sig;As per static analysis hints:
'_tempoSyncInspectorState' is assigned a value but never used(lint, line 347).🤖 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/tempo.js` around lines 347 - 360, The `_refreshTempoSyncInspector` memo signature is being assigned to `_tempoSyncInspectorState` but never used, so add the same early-return guard used by `_refreshTempoMapButton`: compute `sig`, compare it against `_tempoSyncInspectorState`, and exit early when they match before touching the DOM. Keep the existing state-building logic in `_refreshTempoSyncInspector` and `_tempoSyncInspectorStatePure`, but ensure the cached signature is actually checked to avoid redundant updates and satisfy the lint warning.Source: Linters/SAST tools
🤖 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/tempo.js`:
- Around line 1488-1532: Dragging a locked pole updates the beat position but
does not refresh the persisted lock state, so the saved lock time can drift out
of sync. Update _tempoMapOnDragEnd to persist beat locks after a successful
TempoMapCmd commit, reusing the existing _saveBeatLocks logic so dragged locked
beats stay aligned. Make sure the fix is applied in the drag commit path for
both tempo-sync and tempo-beat flows, and keep the lock restore behavior
consistent with _restoreBeatLocks.
- Around line 372-383: Keep the BPM gate consistent in both refresh paths: the
`_refreshTempoSyncInspector()` branch that handles `!visible` should not
unconditionally clear `editor-bpm.disabled`, because `draw()` invokes it after
`_refreshTempoMapButton()` and can re-enable the global BPM field incorrectly.
Reuse the same `disableGlobalBpm` logic used elsewhere in `draw()`/tempo refresh
flow so the `bpmEl` disabled state, opacity, and title stay aligned with Tempo
Map mode and multi-tempo state.
In `@tests/loop_undo_mode.test.mjs`:
- Around line 57-70: The shared module-level S singleton in makeEnv is only
partially reset, which can leak state between tests. Update the test setup
around Object.assign(realS, ...) to fully isolate S for each makeEnv call,
either by rebuilding a fresh state object or by clearing/restoring all mutable
fields before assigning seed data, so prior TempoMapCmd and loop helper
mutations cannot persist across tests.
In `@tests/midi_tempo_import.test.mjs`:
- Around line 150-161: The tests are sharing and mutating the imported singleton
S through Object.assign(realS, ...), so state from one TempoGridCmd test can
leak into the next. Update the setup in these TempoGridCmd cases to use a fresh,
isolated song object per test (or fully reset all relevant S fields before each
assertion) so exec/rollback mutations do not persist across tests. Keep the fix
localized to the test helpers/fixtures around realS, grid, and TempoGridCmd.
---
Nitpick comments:
In `@src/tempo.js`:
- Around line 347-360: The `_refreshTempoSyncInspector` memo signature is being
assigned to `_tempoSyncInspectorState` but never used, so add the same
early-return guard used by `_refreshTempoMapButton`: compute `sig`, compare it
against `_tempoSyncInspectorState`, and exit early when they match before
touching the DOM. Keep the existing state-building logic in
`_refreshTempoSyncInspector` and `_tempoSyncInspectorStatePure`, but ensure the
cached signature is actually checked to avoid redundant updates and satisfy the
lint warning.
🪄 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: 8e3ed84a-1db8-440f-9ccc-d6c01e04af0a
📒 Files selected for processing (21)
CHANGELOG.mdsrc/host.jssrc/main.jssrc/tempo.jstests/beat_converter.test.mjstests/beat_lock.test.mjstests/beat_primary.test.mjstests/loop_beats.test.mjstests/loop_undo_mode.test.mjstests/measure_readout.test.mjstests/midi_tempo_import.test.mjstests/tap_tempo.test.mjstests/tempo_beat_drag.test.mjstests/tempo_flatten.test.mjstests/tempo_map_bpm.test.jstests/tempo_map_bpm.test.mjstests/tempo_map_guidance.test.mjstests/tempo_map_timesig.test.jstests/tempo_map_timesig.test.mjstests/tempo_modulate.test.mjstests/tempo_sync_inspector.test.mjs
💤 Files with no reviewable changes (2)
- tests/tempo_map_bpm.test.js
- tests/tempo_map_timesig.test.js
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.
Twenty-second module.
src/main.js15,042 → 13,347 — and now 37% smaller than the 21,176 lines this refactor started from.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).Where to cut
The section banner claims 2,179 lines. About 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.jslift — the scan window simply ran on until the next banner. The real tempo region ends atTempoMapCmd. Cutting there is what kept the dependency surface honest; cutting at the banner would have dragged the whole drum command set back in.main.jskeeps the drag dispatcher_finalizeActiveDragcommits or clears whatever canvas drag is in flight — tempo, drum, handshape, pan — before a mode switch. It belongs to none of them, so it stays put and reaches back ashost.finalizeActiveDrag().A reassigned scalar, again
Fifteen
main.jssymbols travel the other way: the transport, the A/B loop strip, the toolbar readouts,getMousePos.One of them is not a function.
_recStateis a reassigned module scalar, so it cannot cross as a value at all — an ES import binding would be read-only, and a hook capturing it would freezeidleforever. It is wired as a predicate instead:Same class of trap as
drawin #165/#166. Caught this time by looking for it.Tests
Fifteen suites stopped slicing
@pure:blocks and command classes out ofmain.js. Ten were CJS and are now.mjs. A few now rely onhost’s inert defaults where they used to inject no-op stubs — equivalent, and Codex confirmed the mapping.Why a headless harness, again
verify_tempo.pyarms drum-edit mode, then enters Tempo Map:draw+refreshDrumEditButtonunwired_tempoMapDrawpaints its HUD lineThat last button check is the interesting one. Entering Tempo Map leaves drum-edit mode, and
host.refreshDrumEditButtonis the only thing that relabels the button afterwards. Without it the toolbar keeps offering “🎸 Back to Notes” for a mode the editor is no longer in.Comment out those two hooks and all 89 unit tests still pass.
Verification
node --test89/89 ·pytest248/248 ·npm run lint0 errors (7 warnings, down from 8) · Codex clean (no corrupted rewrites, no import cycles,isRecordingclosure correct) · all 14 headless harnesses pass.🤖 Generated with Claude Code
Summary by CodeRabbit