feat(editor): metric modulation at the selected sync point - #96
Conversation
|
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 (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds a Metric Modulation feature to Tempo Map mode: a new ChangesMetric Modulation Feature
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant Editor
participant PureHelpers as TempoModulate Helpers
participant TempoMapCmd
User->>Editor: Press M or click "Modulate…" in Tempo Map mode
Editor->>Editor: _editorModulateTempoAtSelection()
Editor->>User: Prompt for pivot/ratio
User-->>Editor: Provide ratio input
Editor->>PureHelpers: _tempoModulationRatioPure(input)
PureHelpers-->>Editor: Parsed ratio or null
Editor->>PureHelpers: _tempoModulateRunPure(S.beats, target, ratio)
PureHelpers-->>Editor: Modulated beats or null (refused)
Editor->>TempoMapCmd: Commit modulated beats
TempoMapCmd-->>Editor: Apply change
Editor->>User: Update BPM display, redraw, show status
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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)screen.jsast-grep timed out on this file Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
screen.js (1)
12771-12826: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueOptional: extract the repeated "next downbeat" scan into a helper.
The forward search for the next
measure > 0index is duplicated four times (_tempoMeasureBpmAtPure, the run-collection loop, the apply loop, and thelastNdbIIFE). A single_nextDownbeatPure(beats, from)helper would remove the duplication and let you drop the redundant re-scan inside the apply loop by reusing indices already computed while buildingrunStarts.Correctness itself looks sound:
runStartsonly ever pushes indices whose BPM is non-null, which guarantees a validndbin the apply loop, andres.beatsis a full clone so it can't aliasS.beats.🤖 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 `@screen.js` around lines 12771 - 12826, The forward scan for the next `measure > 0` downbeat is duplicated across `_tempoMeasureBpmAtPure`, `_tempoModulateRunPure`, and the `lastNdb` logic, and the apply loop re-finds indices that were already discovered. Extract a shared `_nextDownbeatPure(beats, from)` helper, then reuse it everywhere and carry the computed next-downbeat index through `runStarts` so `_tempoModulateRunPure` doesn’t re-scan during the apply phase.
🤖 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.
Nitpick comments:
In `@screen.js`:
- Around line 12771-12826: The forward scan for the next `measure > 0` downbeat
is duplicated across `_tempoMeasureBpmAtPure`, `_tempoModulateRunPure`, and the
`lastNdb` logic, and the apply loop re-finds indices that were already
discovered. Extract a shared `_nextDownbeatPure(beats, from)` helper, then reuse
it everywhere and carry the computed next-downbeat index through `runStarts` so
`_tempoModulateRunPure` doesn’t re-scan during the apply phase.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 5338255f-d335-4643-9a9c-1cda035c7b79
📒 Files selected for processing (3)
CHANGELOG.mdscreen.jstests/tempo_modulate.test.js
Prog/classical charting needs real metric modulations (new tempo = old x ratio at a pivot). Adds an M command in Tempo Map mode + a "Modulate..." button in the sync-point inspector: pick a pivot preset (quarter=dotted quarter x2/3, dotted=quarter x3/2, quarter=eighth x1/2, eighth=quarter x2) or type any ratio (3:2, 2/3, 0.75). The new tempo applies from the selected measure THROUGH ITS UNIFORM RUN, stopping at the first measure whose BPM materially differs (>0.5%) — a hand-authored downstream tempo change is a natural pole the re-space never crosses, so verified sections stay untouched (design D18). Interior beats re-space PROPORTIONALLY, preserving swung/uneven sub-beat fractions; everything after the run rigid-shifts by the accumulated delta; beat count is unchanged so the edit rides TempoMapCmd (notes remap per the tempo-ride scope, exactly like a BPM edit) as one undoable command. Results yielding measures below the minimum span are refused. Tests: tests/tempo_modulate.test.js (7 cases: ratio parsing, run boundary at a tempo change, span math + tail shift, swing preservation, mid-run selection, min-span refusal, invalid targets). node --check clean; all 26 JS test files pass. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JEoFeTPSnz4NpwwCG52hnu
The final parseFloat fallback in _tempoModulationRatioPure silently accepted leading-numeric garbage: '3:2:1'/'2/3/4'/'3abc' fail the anchored fraction regex, then parseFloat truncates them to 3/2/3 — plausible ratios that land inside the caller's 0.2-5 bounds and get APPLIED, contradicting the garbage-rejected contract. Require the whole trimmed string to be a clean bare decimal before accepting; otherwise return null. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
7b78e30 to
92470eb
Compare
Summary
Prog and classical charts need real metric modulations — new tempo = old × ratio at a pivot. This adds an
Mcommand in Tempo Map mode (both shortcut profiles) plus a Modulate… button in the sync-point inspector: pick a pivot preset (♩=♩. ×2/3 · ♩.=♩ ×3/2 · ♩=♪ ×1/2 · ♪=♩ ×2) or type any ratio (3:2,2/3,0.75).The key semantics:
TempoMapCmd: one undoable command, and notes remap per the tempo-ride scope exactly like a BPM edit.No spec change — the beat grid already expresses any ratio; this is pure authoring surface (per the rhythm-meter seat's charrette ruling).
Verification
node --check screen.jscleantests/tempo_modulate.test.js— 7 cases via the@pure:tempo-modulateblock (ratio parsing incl. presets/fractions/junk, run boundary stops at a tempo change with the downstream section's spacing pinned, span math + tail shift, swing-fraction preservation, mid-run selection scope, min-span refusal, invalid targets)🤖 Generated with Claude Code
https://claude.ai/code/session_01JEoFeTPSnz4NpwwCG52hnu
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests