feat(editor): segment-first tempo mapping engine + Scan preview (P2-3) - #252
Conversation
The 2nd-pass default outcome of Detect/Sync: instead of a whole-song scale (too coarse) or a per-bar march (runaway off-phase drift), propose a SMALL number of tempo-intent SEGMENTS. This PR lands the pure detection engine + a preview-only Scan action; the confirm bar + Apply (one TempoGridCmd) are the follow-up, so nothing here mutates the map — the analysis is surfaced to be seen/trusted first. New pure src/tempo-segment.js: - _localTempoSeriesPure — windowed local tempo by autocorrelating the strength-weighted onset impulse train over [40..300 bpm], with an OCTAVE GUARD (harmonic support) + a log-normal TEMPO PRIOR (Parncutt/Klapuri) so real audio doesn't read 2× fast (eighth-hats) or half-time; weak/low-energy windows are unmapped. - _segmentTempoPure — 5-pt median smooth, within-3% grouping, ramp-coalesce of monotone runs of short segments (Theil-Sen slope, anchor-immune), min-duration merge of noise blips, maxSegments cap. A single-tempo song → exactly ONE segment; over-segmentation is actively fought. - _downbeatPhasePure — phase is a SEPARATE decision: score bar phase by the low-band (kick) onsets so the downbeat lands on beat 1, not the loud snare backbeat (2 & 4). - _segmentSeedGridPure — the S.beats skeleton (uniform per constant, accel per ramp; unmapped seeds nothing) that Apply will lift into a TempoGridCmd. Wired: editorScanTempoZones (tempo.js) runs the engine on _ensureOnsetsShifted and reports the zones via the status line (no commit); Tempo/Grid menu item + window binding. Tests: tests/tempo_segment.test.mjs (6) — the fail-on-main fixtures: 120×16 → 140×16 → 4-bar rit-to-90 ⇒ exactly 3 segments [constant, constant, ramp] with the right BPMs + boundary; single-tempo ⇒ exactly ONE; octave guard reads 120 not 240 under ghost hits; phase seed picks beat 1 on the kick, not the snare; seed-grid skeleton; degenerate input. 135 JS green, lint 0-err, routes.py untouched. Live-verified on AC/DC: 5 plausible zones all near the true ~90 bpm (no octave garbage), reported via the Scan action. FOLLOW-UP (noted): the confirm bar (segment bands on the ruler, drag/split/merge) + Apply = one TempoGridCmd from _segmentSeedGridPure + bounded per-segment _suggestFitPure refine; ramp→ramp-marker waits on P2-7. Real-audio zone quality sharpens with #248's low-band onsets (cleaner kick pulse than broadband RMS). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EBQCHCNA81E9tHmSDHSe2Q
|
Warning Review limit reached
Next review available in: 36 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughAdds a pure tempo-segmentation engine, tests for tempo analysis and grid seeding, and a non-mutating Tempo/Grid preview action exposed through the editor menu and window API. ChangesTempo zone preview
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant EditorMenu
participant Window
participant editorScanTempoZones
participant TempoEngine
participant Status
EditorMenu->>Window: invoke editorScanTempoZones
Window->>editorScanTempoZones: delegate scan request
editorScanTempoZones->>TempoEngine: analyze shifted onsets
TempoEngine-->>editorScanTempoZones: tempo-intent zones
editorScanTempoZones->>Status: report scan result
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (5)
src/menu-bar.js (1)
229-229: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a regression test for the new menu-to-window contract.
Assert that the row dispatches to
editorScanTempoZoneswhen audio is present and is hidden without audio. This protects both the newfnname and itsaudioOnlygating.🤖 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/menu-bar.js` at line 229, Add a regression test for the menu row labeled “Scan for tempo zones (preview)” that verifies it dispatches to editorScanTempoZones when audio is present and is hidden when audio is absent, covering both the fn value and audioOnly gating.tests/tempo_segment.test.mjs (1)
97-103: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMissing regression test for the
maxSegmentscap-merge path.All fixtures here stay at ≤3 segments, so the cap-merge branch in
_segmentTempoPure(src/tempo-segment.jslines 196-207) — which has a correctness bug when the merged pair includes arampsegment — is never exercised. Worth adding a case with >8 raw segments including a non-monotone ramp to lock in the fix.🤖 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/tempo_segment.test.mjs` around lines 97 - 103, The degenerate-input tests do not cover the maxSegments cap-merge path in _segmentTempoPure. Add a regression fixture with more than eight raw segments, including a non-monotone ramp segment, and assert the expected capped output so merging a ramp pair is exercised and remains correct.src/tempo-segment.js (3)
131-148: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
closeSegandbuildRunduplicate the same segment-construction logic.Both compute
ts/ys/med/slope/span/delta/isRampand build near-identical segment objects. Extracting a shared_buildSegment(pts, bpm, from, to, rampFrac)helper used by both call sites would remove the duplication and the risk of the two copies drifting apart.Also applies to: 164-179
🤖 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-segment.js` around lines 131 - 148, Extract the duplicated segment calculations and object construction from closeSeg and buildRun into a shared _buildSegment(pts, bpm, from, to, rampFrac) helper. Replace both call sites with this helper, preserving the existing ramp detection, rounding, confidence, boundaries, and metadata behavior.
63-93: 🎯 Functional Correctness | 🔵 TrivialUnnormalized ACF biases toward faster tempo reads.
The autocorrelation sum at line 72 (
sum += impulse[s+i]*impulse[s+i+lag]) haswb - lagterms — fewer terms aslaggrows. Since larger lag = slower tempo, raw (non‑length‑normalized) ACF systematically favors smaller lags (faster tempo), which is the same direction as the "reads 2× fast" error the octave guard exists to fix. The confidence formula (acf[pick]/zero, line 91) inherits the same bias, so confidence itself skews higher for faster misreads. This also gets worse for tail windows wherewbcan shrink tolagMax + 1(line 63), leaving very few terms for large-lag candidates.Normalizing by the overlap count (
sum / (wb - lag)) before applying the prior would remove this systematic bias and reduce reliance on the octave-guard/prior to compensate for 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 `@src/tempo-segment.js` around lines 63 - 93, Normalize each lag’s ACF in the autocorrelation loop near bestLag by dividing sum by its overlap count, wb - lag, before storing it in acf and applying priorW. Use the normalized acf values consistently for candidate scoring and confidence calculation, while preserving the existing lag selection, octave candidates, and zero-energy handling.
258-261: 🎯 Functional Correctness | 🔵 Trivial"&3 kick" bonus is 4/4-specific but applied for any
beatsPerBar.
dThree = Math.abs(inBar - beatPeriod * (bpb / 2))scores "half the bar" as a kick-emphasis point, which corresponds to beat 3 in 4/4 but has no comparable musical meaning for otherbpbvalues (e.g. 3/4). SincebeatsPerBaris a documented option, this heuristic will bias phase selection oddly for non-4/4 input.Consider gating the
dThreebonus behindbpb === 4(or making it configurable per meter) rather than applying it unconditionally.🤖 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-segment.js` around lines 258 - 261, Gate the dThree “beat 3” kick bonus in the scoring logic so it is applied only when bpb === 4. Preserve the dDown nearest-downbeat scoring for all meters and avoid treating the half-bar position as a kick emphasis for other beatsPerBar values.
🤖 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-segment.js`:
- Around line 196-207: The cap-merge in _segmentTempoPure must only merge
adjacent pairs whose kind is 'constant'; stop merging when no eligible constant
pair remains instead of converting a ramp. Update src/tempo-segment.js lines
196-207 accordingly, and add a test in tests/tempo_segment.test.mjs lines 97-103
that creates more than 8 raw segments with a non-monotone ramp and verifies the
ramp remains unchanged after capping.
- Around line 278-289: Harden the beat-generation loop in _segmentSeedGridPure
by validating the computed period before advancing t. If period is non-finite or
<= 0, stop processing the current segment (or otherwise exit the loop) so t +=
period cannot cause an infinite loop; preserve normal interpolation and beat
generation for positive finite periods.
In `@src/tempo.js`:
- Around line 691-695: Update the onset validation around _ensureOnsetsShifted
so missing onset analysis remains the “load audio first” case, while arrays with
at least four finite onsets proceed to _localTempoSeriesPure. Lower the
sparse-data threshold to the detector’s minimum and provide an appropriate
insufficient-data status without treating 4–7 onsets as missing analysis.
---
Nitpick comments:
In `@src/menu-bar.js`:
- Line 229: Add a regression test for the menu row labeled “Scan for tempo zones
(preview)” that verifies it dispatches to editorScanTempoZones when audio is
present and is hidden when audio is absent, covering both the fn value and
audioOnly gating.
In `@src/tempo-segment.js`:
- Around line 131-148: Extract the duplicated segment calculations and object
construction from closeSeg and buildRun into a shared _buildSegment(pts, bpm,
from, to, rampFrac) helper. Replace both call sites with this helper, preserving
the existing ramp detection, rounding, confidence, boundaries, and metadata
behavior.
- Around line 63-93: Normalize each lag’s ACF in the autocorrelation loop near
bestLag by dividing sum by its overlap count, wb - lag, before storing it in acf
and applying priorW. Use the normalized acf values consistently for candidate
scoring and confidence calculation, while preserving the existing lag selection,
octave candidates, and zero-energy handling.
- Around line 258-261: Gate the dThree “beat 3” kick bonus in the scoring logic
so it is applied only when bpb === 4. Preserve the dDown nearest-downbeat
scoring for all meters and avoid treating the half-bar position as a kick
emphasis for other beatsPerBar values.
In `@tests/tempo_segment.test.mjs`:
- Around line 97-103: The degenerate-input tests do not cover the maxSegments
cap-merge path in _segmentTempoPure. Add a regression fixture with more than
eight raw segments, including a non-monotone ramp segment, and assert the
expected capped output so merging a ramp pair is exercised and remains correct.
🪄 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: 29e40aee-7942-4e0f-8e32-d9e1f1578999
📒 Files selected for processing (6)
CHANGELOG.mdsrc/main.jssrc/menu-bar.jssrc/tempo-segment.jssrc/tempo.jstests/tempo_segment.test.mjs
| // Cap: merge the most-similar adjacent constant pair until within budget. | ||
| while (segs.length > maxSegments) { | ||
| let bi = 0, bd = Infinity; | ||
| for (let i = 0; i + 1 < segs.length; i++) { | ||
| const d = Math.abs(segs[i].bpmEnd - segs[i + 1].bpmStart); | ||
| if (d < bd) { bd = d; bi = i; } | ||
| } | ||
| const a = segs[bi], b = segs[bi + 1]; | ||
| segs.splice(bi, 2, { tStart: a.tStart, tEnd: b.tEnd, kind: 'constant', | ||
| bpmStart: (a.bpmStart + b.bpmEnd) / 2, bpmEnd: (a.bpmStart + b.bpmEnd) / 2, | ||
| conf: Math.min(a.conf, b.conf), _from: a._from, _to: b._to }); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Cap-merge kind-blind bug is both incorrect and untested. The maxSegments cap-merge in _segmentTempoPure merges the closest-BPM adjacent pair regardless of kind, contradicting its own comment and risking silent flattening of a real ramp into 'constant'; no existing test exercises this branch to catch it.
src/tempo-segment.js#L196-L207: restrict the merge search tokind === 'constant'pairs (and stop rather than force-convert a ramp when no such pair remains) — see the proposed diff on this range.tests/tempo_segment.test.mjs#L97-L103: add a test producing >8 raw segments that includes a non-monotone ramp, asserting the ramp survives the cap-merge unchanged.
📍 Affects 2 files
src/tempo-segment.js#L196-L207(this comment)tests/tempo_segment.test.mjs#L97-L103
🤖 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-segment.js` around lines 196 - 207, The cap-merge in
_segmentTempoPure must only merge adjacent pairs whose kind is 'constant'; stop
merging when no eligible constant pair remains instead of converting a ramp.
Update src/tempo-segment.js lines 196-207 accordingly, and add a test in
tests/tempo_segment.test.mjs lines 97-103 that creates more than 8 raw segments
with a non-monotone ramp and verifies the ramp remains unchanged after capping.
| const onsets = _ensureOnsetsShifted(); | ||
| if (!onsets || onsets.length < 8) { | ||
| setStatus('Scan for tempo zones needs the recording’s onset analysis — load audio first.'); | ||
| return true; | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Distinguish missing onset analysis from sparse data.
An onset array with 4–7 entries is treated as “load audio first,” although _localTempoSeriesPure is designed to accept four or more finite onsets. Short recordings therefore receive the wrong status and never reach detection. Separate !onsets from insufficient-data handling, or align the threshold and message with the intended minimum.
🧰 Tools
🪛 ast-grep (0.44.1)
[error] 692-692: React's useState should not be directly called
Context: setStatus('Scan for tempo zones needs the recording’s onset analysis — load audio first.')
Note: [CWE-710] Improper Adherence to Coding Standards. Security best practice.
(usestate-direct-usage)
🤖 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 691 - 695, Update the onset validation around
_ensureOnsetsShifted so missing onset analysis remains the “load audio first”
case, while arrays with at least four finite onsets proceed to
_localTempoSeriesPure. Lower the sparse-data threshold to the detector’s minimum
and provide an appropriate insufficient-data status without treating 4–7 onsets
as missing analysis.
…g and a flattened ramp Review fixes on the P2-3 scan engine. Scan itself is confirmed preview-only (no S.beats/note/undo writes, nothing stashed on state), but the engine it calls had three real defects, each with a regression test that fails on the pre-fix code: - _localTempoSeriesPure threw `RangeError: Invalid typed array length` when every onset sat at t < 0. Reachable today: a negative S.audioShift slides the recording earlier, and _ensureOnsetsShifted() maps the shift onto every onset, so tEnd goes negative and sizes the impulse buffer to a negative length. The exception escaped straight out of the Scan menu handler. Now returns []. - _segmentSeedGridPure spun forever on a non-positive/non-finite beat period (bpm 0, negative, or Infinity): `t += period` never advanced toward seg.tEnd, so the loop pushed beats until the tab died. It also emitted a NON-monotonic grid when two segments overlapped in time. That array is what #253 lifts into a TempoGridCmd, and beatOf()/timeOf() binary-search S.beats — a grid that is not strictly increasing silently corrupts every note in the song. Both are live inputs once segments are user-confirmable, so the contract is enforced here, in the producer. - The maxSegments cap-merge hard-coded kind:'constant', flattening a real rit/accel into a fake steady tempo — and "closest BPM at the join" is exactly the search that likes to pick a ramp. It now describes the merged span by its own endpoints (a.bpmStart -> b.bpmEnd) and keeps the cap honoured. Also: the scan reported a weak pulse as confidently as a strong one. Rubato or ambient audio still yields a tidy-looking row of zones with garbage BPMs, so the status line now says LOW CONFIDENCE when the weakest zone is shaky (clean loop ~0.8, jittered ~0.3, no pulse ~0.2), and sparse audio no longer tells the user to "load audio first" when audio is already loaded. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/tempo_segment.test.mjs (1)
127-136: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
[0, 0]case doesn't actually exercise the degenerate-BPM guard.For
bpmStart=0, bpmEnd=0,_segmentSeedGridPure'sseg.bpmStart || seg.bpmEnd || 120fallback coerces both zeros to the120default (0 is falsy), sop0/p1become0.5and a normal 20-beat 120bpm grid is emitted — thep0 > 0/Number.isFiniteguard is never actually triggered for this case. The assertion still passes, but it passes because of the unrelated default-fallback, not because the "explicit zero bpm" input was recognized as degenerate and refused. The other three fixtures ([-60,-60],[Infinity,Infinity],[120,-120]) do correctly hit the guard and return[].Consider either asserting the actual fallback behavior explicitly for
[0,0](e.g. assert it degrades to 120bpm rather than just checking< 1000), or tightening the source guard so an explicit0is distinguished from "unset" rather than relying on truthy-fallback coercion.🤖 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/tempo_segment.test.mjs` around lines 127 - 136, Update the test case for [0, 0] in “seed grid refuses degenerate BPM instead of looping forever” to assert the intended behavior explicitly: either verify _segmentSeedGridPure’s current fallback produces the normal 120 BPM grid, or change the fallback and guard so explicitly provided zero BPM values are treated as degenerate and return an empty grid. Keep the other degenerate-BPM fixtures and finite-time assertions unchanged.
🤖 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 `@node_modules`:
- Line 1: Remove the tracked node_modules symbolic link from version control and
add node_modules to the repository’s ignore configuration so dependency
directories are not tracked in future commits.
---
Nitpick comments:
In `@tests/tempo_segment.test.mjs`:
- Around line 127-136: Update the test case for [0, 0] in “seed grid refuses
degenerate BPM instead of looping forever” to assert the intended behavior
explicitly: either verify _segmentSeedGridPure’s current fallback produces the
normal 120 BPM grid, or change the fallback and guard so explicitly provided
zero BPM values are treated as degenerate and return an empty grid. Keep the
other degenerate-BPM fixtures and finite-time assertions unchanged.
🪄 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: 3a53d9a5-b53c-49ab-855a-3d7e03d3a6f9
📒 Files selected for processing (4)
node_modulessrc/tempo-segment.jssrc/tempo.jstests/tempo_segment.test.mjs
🚧 Files skipped from review as they are similar to previous changes (1)
- src/tempo-segment.js
The phase search filtered onsets at t >= tStart with no upper bound, so it scored every onset to the end of the song. On a multi-zone map a later zone's pulse votes on this zone's phase and drags bar 1 off the beat its own audio lands on. Bound the search to [tStart, tEnd). Routed up from the review of #253, which commits this grid via Apply. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Swept in by a git add -A in the review worktree; it is a mode-120000 blob pointing at an absolute local path and breaks any fresh clone. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…irst # Conflicts: # CHANGELOG.md # src/main.js # src/menu-bar.js
This branch predates every fix that landed with its parent, and git merges the
two cleanly while reverting them. Reconciled by hand — main is the base for
src/tempo-segment.js, and only _segmentRoughMapPure is grafted on:
· KEPT the strictly-increasing seed-grid guard and the degenerate-BPM guard
(both BLOCKERs: this branch's copy would spin forever on bpm<=0, and emit a
non-monotonic grid that beatOf/timeOf binary-search — silent note corruption)
· KEPT the cap-merge ramp fix and the windowed downbeat-phase search, and the
rough map now passes tEnd so a later zone cannot drag this zone's bar 1
· KEPT main's Scan copy (sparse-vs-missing audio, LOW CONFIDENCE warning)
Also corrected the phase-seed note: it said bands were never emitted, so phase
always seeded off broadband strength and could lock the snare backbeat. That was
true when this branch was written; the banded detector now carries bands through
the audio shift, so on a decoded recording the kick does drive the phase. The
RMS fallback still has no bands, and the note now says exactly that.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* feat(editor): segment-first tempo mapping engine + Scan preview (P2-3) The 2nd-pass default outcome of Detect/Sync: instead of a whole-song scale (too coarse) or a per-bar march (runaway off-phase drift), propose a SMALL number of tempo-intent SEGMENTS. This PR lands the pure detection engine + a preview-only Scan action; the confirm bar + Apply (one TempoGridCmd) are the follow-up, so nothing here mutates the map — the analysis is surfaced to be seen/trusted first. New pure src/tempo-segment.js: - _localTempoSeriesPure — windowed local tempo by autocorrelating the strength-weighted onset impulse train over [40..300 bpm], with an OCTAVE GUARD (harmonic support) + a log-normal TEMPO PRIOR (Parncutt/Klapuri) so real audio doesn't read 2× fast (eighth-hats) or half-time; weak/low-energy windows are unmapped. - _segmentTempoPure — 5-pt median smooth, within-3% grouping, ramp-coalesce of monotone runs of short segments (Theil-Sen slope, anchor-immune), min-duration merge of noise blips, maxSegments cap. A single-tempo song → exactly ONE segment; over-segmentation is actively fought. - _downbeatPhasePure — phase is a SEPARATE decision: score bar phase by the low-band (kick) onsets so the downbeat lands on beat 1, not the loud snare backbeat (2 & 4). - _segmentSeedGridPure — the S.beats skeleton (uniform per constant, accel per ramp; unmapped seeds nothing) that Apply will lift into a TempoGridCmd. Wired: editorScanTempoZones (tempo.js) runs the engine on _ensureOnsetsShifted and reports the zones via the status line (no commit); Tempo/Grid menu item + window binding. Tests: tests/tempo_segment.test.mjs (6) — the fail-on-main fixtures: 120×16 → 140×16 → 4-bar rit-to-90 ⇒ exactly 3 segments [constant, constant, ramp] with the right BPMs + boundary; single-tempo ⇒ exactly ONE; octave guard reads 120 not 240 under ghost hits; phase seed picks beat 1 on the kick, not the snare; seed-grid skeleton; degenerate input. 135 JS green, lint 0-err, routes.py untouched. Live-verified on AC/DC: 5 plausible zones all near the true ~90 bpm (no octave garbage), reported via the Scan action. FOLLOW-UP (noted): the confirm bar (segment bands on the ruler, drag/split/merge) + Apply = one TempoGridCmd from _segmentSeedGridPure + bounded per-segment _suggestFitPure refine; ramp→ramp-marker waits on P2-7. Real-audio zone quality sharpens with #248's low-band onsets (cleaner kick pulse than broadband RMS). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EBQCHCNA81E9tHmSDHSe2Q * feat(editor): apply a segment-first rough map (P2-3 Apply) The committing half of Scan (#252 was preview-only). "Apply rough map from tempo zones" turns the detected segments into an actual beat grid and installs it as ONE undoable TempoGridCmd — notes keep their SECONDS and ride the recording, their beat coords re-lift onto the new grid, and Ctrl+Z restores the previous grid exactly. A deliberate two-step: Scan to review the zones, Apply to commit; the drag/split/merge confirm bar is the follow-up. tempo-segment.js: new pure _segmentRoughMapPure(onsets) — detect segments → seed each downbeat PHASE from the kick onsets (_downbeatPhasePure, so bar 1 sits on the kick, not the snare backbeat) → build the grid in the editor's beat shape (downbeats {time, measure, den}; interior {time, measure: -1}). tempo.js: editorApplyTempoZones runs it on _ensureOnsetsShifted and execs the TempoGridCmd; Tempo/Grid menu item + window binding. Tests: tempo_segment.test.mjs gains a rough-map case (grid in the real beat shape — downbeats carry den, interior = -1, ~0.5s beats at 120bpm, 2 zones). 135 JS green, lint 0-err, routes.py untouched. Live-verified on AC/DC: Apply installs a new 393-beat grid (101 downbeats, phase-seeded first downbeat at 3.22s) as +1 undo; a note at t=10.623s KEEPS its seconds and re-lifts its beat (13.501→11.272); Undo restores the 419-beat grid + the note's beat exactly. Stacks on #252 (feat/editor-segment-first); rebase onto main when it lands. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EBQCHCNA81E9tHmSDHSe2Q * fix(editor): gate Apply on an open song + stop naming keys in UI copy Review fixes for the segment-first rough-map Apply (P2-3). - HIGH: editorApplyTempoZones called S.history.exec() with no session guard. The Tempo menu row is gated `audioOnly` (S.audioBuffer), which does NOT imply an open song — so audio-with-no-session reached the exec with S.history at its declared default of null and threw "TypeError: Cannot read properties of null (reading 'exec')". Scan can skip the guard (it never writes); Apply cannot. - Status copy + CHANGELOG hardcoded the "G" accelerator. Two shortcut profiles exist and the menu resolves accelerators from the registry, so UI copy names the COMMAND (Suggest barline fit), never its key. - Comments/CHANGELOG claimed the downbeat phase is "kick-seeded". It is not, yet: _downbeatPhasePure prefers bands.lo but _ensureOnsets emits {t, s} only, so phase seeds off broadband strength and can still land on a backbeat. Say so rather than overclaim; banded onsets are the fix. New tests/tempo_segment_apply.test.mjs drives the real command end-to-end (real S, real EditHistory, onsets synthesised from S.waveformPeaks) and pins the tempo-model invariant: the installed grid is strictly monotonic and finite; every timed object — notes, chords, sections, anchors, handshapes, phrases, drum hits, including a pickup before the first segment — keeps its SECONDS exactly while its beat re-derives; Apply is ONE undoable command and undo restores the previous grid verbatim plus every beat exactly; and both gates hold. The no-session case fails on the pre-fix code with the exact TypeError. Suite 136 pass / 0 fail; ESLint 0 errors, 3 warnings (baseline). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * chore(editor): drop the stray node_modules symlink from this branch Swept in by a git add -A in the review worktree; it is a mode-120000 blob pointing at an absolute local path and breaks any fresh clone. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- Co-authored-by: ChrisBeWithYou <chris@rifflarr.local> Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Co-authored-by: byrongamatos <xasiklas@gmail.com>
The 2nd-pass default outcome of Detect/Sync (P2-3, rhythm seat's boldest): instead of a whole-song scale (too coarse) or a per-bar march (runaway off-phase drift), propose a small number of tempo-intent segments. This PR lands the pure detection engine + a preview-only Scan action; the confirm bar + Apply (one
TempoGridCmd) are the follow-up — so nothing here mutates the map, the analysis is surfaced to be seen and trusted first.Engine (
src/tempo-segment.js, pure)_localTempoSeriesPure— windowed local tempo by autocorrelating the strength-weighted onset train over [40..300 bpm], with an octave guard (harmonic support) + a log-normal tempo prior (Parncutt/Klapuri) so real audio doesn't read 2× fast (eighth-hats) or half-time; weak/low-energy windows areunmapped._segmentTempoPure— 5-pt median smooth → within-3% grouping → ramp-coalesce of monotone short-segment runs (Theil-Sen slope, anchor-immune) → min-duration merge of noise blips → maxSegments cap. A single-tempo song → exactly ONE segment; over-segmentation is actively fought._downbeatPhasePure— phase is a separate decision: score bar phase by the low-band (kick) onsets so the downbeat lands on beat 1, not the loud snare backbeat._segmentSeedGridPure— theS.beatsskeleton (uniform per constant, accel per ramp; unmapped seeds nothing) that Apply will lift into aTempoGridCmd.Wired:
editorScanTempoZonesruns the engine on_ensureOnsetsShiftedand reports the zones via the status line (no commit) — a Tempo/Grid ▸ Scan for tempo zones (preview) menu item.Tests
tests/tempo_segment.test.mjs(6) — the fail-on-main fixtures:120×16 → 140×16 → 4-bar rit to 90⇒ exactly 3 segments[constant, constant, ramp]with the right BPMs + boundary; single-tempo ⇒ exactly ONE; octave guard reads 120 not 240 under ghost hits; phase seed picks beat 1 on the kick, not the snare; seed-grid skeleton; degenerate input.routes.pyuntouched.Follow-up (noted, not in this PR)
The confirm bar (segment bands on the ruler, drag/split/merge) + Apply = one
TempoGridCmdfrom_segmentSeedGridPure+ a bounded per-segment_suggestFitPurerefine; ramp→ramp-marker waits on P2-7. Real-audio zone quality sharpens with #248's low-band onsets (a cleaner kick pulse than broadband RMS).🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Tests