feat(editor): banded spectral-flux onset detection - #248
Merged
Merged
Conversation
The pass-1 onset detector (_onsetTimesFromPeaksPure) was broadband RMS-rise on
the waveform envelope, blind to the events that matter most for charting: a note
entering a sustained/pedaled chord (no total-energy rise), a low B0/A0 bass
attack (a clean transient but no pitch), and it can't discriminate kick vs snare
vs hat (no frequency information). P2-2 — the first code PR of the "make the
automatic map good" 2nd pass; de-risks segment-first mapping (P2-3) and makes
Map Health (P2-4) meaningful.
New pure src/onsets.js, zero dependency: an iterative radix-2 FFT, an STFT
(Hann-windowed), half-wave-rectified spectral flux in THREE bands (low ≲150Hz =
kick/bass-attack, mid = snare, high = hats/subdivision), an adaptive local-median
threshold, local-max peak-pick, and parabolic sub-hop interpolation for ~ms
timing off a coarse hop. The FFT/STFT front-end is factored separably for a
future audio-to-MIDI lane.
Wired behind _ensureOnsets() (audio.js): the emitted shape is the pass-1 [{t,s}]
EXTENDED to [{t,s,bands:{lo,mid,hi}}], so every consumer is unchanged; the RMS
detector stays as a labelled fallback (_onsetDetectorLabel). _ensureOnsetsShifted
now carries the bands through the shift. Computed once per load (downsampled to
~22kHz, cached).
Tests: tests/onsets_spectral_flux.test.mjs (12) against SYNTHETIC signals with
known answers — FFT (cosine→its bin), Hann shape, band-bin math, decimation,
flux frames (silence→burst spike; 60Hz→lo band, 8kHz→hi band), peak-pick
(adaptive threshold / refractory / parabolic sub-hop), and the full pipeline on a
click train + junk-input degradation. 135 JS green, lint 0-err, routes.py
untouched. Live-verified on AC/DC: 675 banded onsets, one-time compute 147ms
(cached after), 3-way band split (113 lo / 285 mid / 277 hi) — real kick/snare/
cymbal separation.
FOLLOW-UP (noted, not in this PR): the ~150ms compute is synchronous + on-demand
(onset strip is off by default; RMS covers the pre-compute instant). Chunk across
rAF or a Worker for zero-jank on a strip-enabled load.
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: 45 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 (2)
📝 WalkthroughWalkthroughAdds a pure-JS banded spectral-flux onset detector with sub-frame timing and per-band strengths, integrates it with cached audio onset generation and RMS fallback, preserves metadata during shifting, and adds unit, integration, robustness, and changelog coverage. ChangesOnset detection pipeline
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant AudioCache
participant SpectralFlux
participant RMSDetector
participant OnsetConsumers
AudioCache->>SpectralFlux: Analyze decoded PCM
SpectralFlux->>AudioCache: Return time-aligned onset events with band strengths
AudioCache->>RMSDetector: Fall back if spectral flux is unavailable or fails
AudioCache->>OnsetConsumers: Provide cached and shifted onset objects
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/audio.js (1)
236-241: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winSilent catch hides spectral-flux failures.
The bare
catch (_) {}swallows any exception from the spectral-flux path with no trace, so a regression there silently and invisibly downgrades every load to the less frequency-aware RMS fallback. A one-line debug log would preserve current behavior while making the degradation observable.♻️ Suggested tweak
- } catch (_) { /* fall through to the RMS envelope detector */ } + } catch (err) { + console.warn('[onsets] spectral-flux detection failed, falling back to RMS:', err); + }🤖 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/audio.js` around lines 236 - 241, Update the catch block surrounding _spectralFluxOnsetsPure in the spectral-flux detection path to emit a debug log containing the caught error, while preserving the existing fallback to the RMS envelope detector.
🤖 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 `@src/audio.js`:
- Around line 236-241: Update the catch block surrounding
_spectralFluxOnsetsPure in the spectral-flux detection path to emit a debug log
containing the caught error, while preserving the existing fallback to the RMS
envelope detector.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 5b922663-dae8-4fbd-a064-f46650b64819
📒 Files selected for processing (4)
CHANGELOG.mdsrc/audio.jssrc/onsets.jstests/onsets_spectral_flux.test.mjs
This was referenced Jul 13, 2026
Three defects in the spectral-flux detector, all found by benchmarking the pipeline against a synthetic click train and tracing _ensureOnsets' callers. 1. Onsets landed systematically EARLY. A frame spans samples [f*hop, f*hop+fftSize) and is not zero-padded, so timestamping it at f*hopSec reports the window's START, not the energy it measured. Measured bias on a click train: -9.7 ms mean, -17 ms worst. Onsets feed tempo-snap, onset-snap and Sync phase, so that is a constant skew on all of them. Carry `centreSec` on the frames and add it at peak-pick time. 2. The default hop equalled fftSize, i.e. ZERO overlap, so adjacent flux frames shared no samples and the parabolic "sub-hop" interpolation had no smooth curve to interpolate — it was noise. Hop at 50% overlap. Combined with (1): mean bias -9.7 ms -> -3.0 ms, worst -17 ms -> -4.7 ms. 3. The onset cache was invalidated only in computeWaveform(), which early-returns when there is no buffer — but loadCDLC (file-ops.js) and the create-mode import (create.js) drop S.audioBuffer/S.waveformPeaks directly and never reach it. The second song loaded therefore kept song ONE's onsets: the onset strip, onset-snap, tempo-snap and Sync phase all silently aligned to the wrong recording. Key the cache on the analysed source's identity so it invalidates itself for every caller instead of relying on each one to remember. Math.hypot -> sqrt(re^2+im^2) in the magnitude inner loop (hypot's overflow-safe scaling is ~5x slower and this runs millions of times); that pays for the doubled frame count from (2). A 5-minute 44.1 kHz song still analyses in ~320 ms. Per CodeRabbit: the flux failure path no longer swallows the exception silently — a regression there would have downgraded every load to the coarse RMS detector with no trace. Regression tests (both verified RED on the pre-fix code): - click train: every click within 10 ms, mean bias under 5 ms - onset cache: clearing the buffer drops the cache; a second song gets its own onsets, not the first song's - silence / NaN / Infinity audio -> [], no non-finite t or s ever emitted Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 absolute node_modules symbolic link from the repository,
ensuring no machine-specific path remains tracked. Keep node_modules as a
normally ignored dependency directory rather than replacing it with another
committed link.
In `@src/audio.js`:
- Around line 246-266: Update _ensureOnsets around the spectral-flux and
waveformPeaks fallback to memoize an empty spectral-flux result while
S.waveformPeaks is unavailable, preventing repeated analysis calls. Track the
pending “peaks not ready” state distinctly from a genuine empty onset result so
the RMS path runs once waveformPeaks becomes available, while preserving
existing cache behavior when onsets are found.
🪄 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: 8c5527ab-24d8-490e-b3d8-127a06d70872
📒 Files selected for processing (4)
node_modulessrc/audio.jssrc/onsets.jstests/onsets_spectral_flux.test.mjs
🚧 Files skipped from review as they are similar to previous changes (1)
- src/onsets.js
Comment on lines
+246
to
266
| if (S.audioBuffer) { | ||
| try { | ||
| const flux = _spectralFluxOnsetsPure(S.audioBuffer.getChannelData(0), S.audioBuffer.sampleRate); | ||
| if (flux && flux.length) { | ||
| _onsetCache = flux; _onsetCacheKey = key; _onsetDetector = 'spectral-flux'; | ||
| return _onsetCache; | ||
| } | ||
| } catch (err) { | ||
| // Never silent: a regression in the flux path would otherwise downgrade | ||
| // every load to the coarser RMS detector with no trace at all. | ||
| console.warn('[editor] spectral-flux onset detection failed, falling back to RMS:', err); | ||
| } | ||
| } | ||
| // Fallback: the pass-1 RMS-envelope detector (no decoded buffer, or flux found | ||
| // nothing). Same [{t, s}] shape, minus the per-band strengths. | ||
| const pk = S.waveformPeaks; | ||
| if (!pk || !pk.bins || !pk.rms) return null; | ||
| _onsetCache = _onsetTimesFromPeaksPure(pk.rms, dur / pk.bins); | ||
| _onsetCacheKey = key; | ||
| _onsetDetector = 'rms'; | ||
| return _onsetCache; |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the relevant implementation and surrounding context.
git ls-files src/audio.js tests | sed -n '1,200p'
echo '--- src/audio.js around the reported lines ---'
cat -n src/audio.js | sed -n '220,290p'
echo '--- search for onset cache usage and related tests ---'
rg -n "_onsetCache|_onsetCacheKey|_ensureOnsets|spectral-flux|waveformPeaks" src testsRepository: got-feedBack/feedBack-plugin-editor
Length of output: 14439
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map any nearby helper implementations to understand whether empty/failed flux
# results are intentionally cached elsewhere.
ast-grep outline src/audio.js
echo '--- related pure helpers if present ---'
rg -n "function _spectralFluxOnsetsPure|const _spectralFluxOnsetsPure|_onsetTimesFromPeaksPure" srcRepository: got-feedBack/feedBack-plugin-editor
Length of output: 5363
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect helper semantics and the main call sites that can repeatedly invoke _ensureOnsets().
echo '--- src/onsets.js spectral flux helper ---'
cat -n src/onsets.js | sed -n '220,300p'
echo '--- src/waveform.js call site ---'
cat -n src/waveform.js | sed -n '1,140p'
echo '--- other direct call sites in source ---'
rg -n "_ensureOnsets\(" src/*.js src/**/*.jsRepository: got-feedBack/feedBack-plugin-editor
Length of output: 8828
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect tests that exercise onset caching and whether empty/failed paths are expected.
cat -n tests/onsets_spectral_flux.test.mjs | sed -n '190,250p'
echo '--- any tests around empty flux / missing peaks ---'
rg -n "empty|null|waveformPeaks|spectral-flux|onset" tests/onsets_spectral_flux.test.mjs tests/onset_strip.test.js tests/tempo_onset_snap.test.mjsRepository: got-feedBack/feedBack-plugin-editor
Length of output: 9904
Cache the empty-onset path separately
If spectral-flux returns no onsets before S.waveformPeaks is ready, _ensureOnsets() keeps re-running the expensive flux analysis on every call. Memoize that negative/pending state too, but make it distinguish “peaks not ready yet” so the RMS fallback can still run once peaks exist.
🤖 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/audio.js` around lines 246 - 266, Update _ensureOnsets around the
spectral-flux and waveformPeaks fallback to memoize an empty spectral-flux
result while S.waveformPeaks is unavailable, preventing repeated analysis calls.
Track the pending “peaks not ready” state distinctly from a genuine empty onset
result so the RMS path runs once waveformPeaks becomes available, while
preserving existing cache behavior when onsets are found.
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>
…ctral-flux # Conflicts: # CHANGELOG.md
byrongamatos
added a commit
that referenced
this pull request
Jul 13, 2026
The chunked driver split _spectralFluxOnsetsPure into a plan/step pair, while #248 fixed bugs INSIDE the frame loop it moved. Git merges those cleanly and silently keeps the old, broken loop on the chunked path. Carried across by hand: · hop = fftSize/2 (was fftSize at the _spectralFluxOnsetsPlan call site, which overrode the default, so the chunked path still ran zero-overlap frames and its sub-hop interpolation was interpolating noise) · frames carry centreSec, added at peak-pick (a frame reports the window START, so onsets read ~10ms early on every consumer) · sqrt over Math.hypot in the magnitude inner loop · _ensureOnsets keeps #248's identity-keyed cache (taking this branch's side wholesale would have reverted that BLOCKER: a new song inheriting the old song's onsets), now combined with the async upgrade + job cancellation Kept main's onset test file (a superset) and adapted its cache-key test to the async contract: flux now arrives via a background job, so the test drains it. Both of #248's guards — the click-train bias test and the cache-key test — pass against the chunked path, which is what proves the reconciliation. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
byrongamatos
added a commit
that referenced
this pull request
Jul 13, 2026
* feat(editor): banded spectral-flux onset detection
The pass-1 onset detector (_onsetTimesFromPeaksPure) was broadband RMS-rise on
the waveform envelope, blind to the events that matter most for charting: a note
entering a sustained/pedaled chord (no total-energy rise), a low B0/A0 bass
attack (a clean transient but no pitch), and it can't discriminate kick vs snare
vs hat (no frequency information). P2-2 — the first code PR of the "make the
automatic map good" 2nd pass; de-risks segment-first mapping (P2-3) and makes
Map Health (P2-4) meaningful.
New pure src/onsets.js, zero dependency: an iterative radix-2 FFT, an STFT
(Hann-windowed), half-wave-rectified spectral flux in THREE bands (low ≲150Hz =
kick/bass-attack, mid = snare, high = hats/subdivision), an adaptive local-median
threshold, local-max peak-pick, and parabolic sub-hop interpolation for ~ms
timing off a coarse hop. The FFT/STFT front-end is factored separably for a
future audio-to-MIDI lane.
Wired behind _ensureOnsets() (audio.js): the emitted shape is the pass-1 [{t,s}]
EXTENDED to [{t,s,bands:{lo,mid,hi}}], so every consumer is unchanged; the RMS
detector stays as a labelled fallback (_onsetDetectorLabel). _ensureOnsetsShifted
now carries the bands through the shift. Computed once per load (downsampled to
~22kHz, cached).
Tests: tests/onsets_spectral_flux.test.mjs (12) against SYNTHETIC signals with
known answers — FFT (cosine→its bin), Hann shape, band-bin math, decimation,
flux frames (silence→burst spike; 60Hz→lo band, 8kHz→hi band), peak-pick
(adaptive threshold / refractory / parabolic sub-hop), and the full pipeline on a
click train + junk-input degradation. 135 JS green, lint 0-err, routes.py
untouched. Live-verified on AC/DC: 675 banded onsets, one-time compute 147ms
(cached after), 3-way band split (113 lo / 285 mid / 277 hi) — real kick/snare/
cymbal separation.
FOLLOW-UP (noted, not in this PR): the ~150ms compute is synchronous + on-demand
(onset strip is off by default; RMS covers the pre-compute instant). Chunk across
rAF or a Worker for zero-jank on a strip-enabled load.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01EBQCHCNA81E9tHmSDHSe2Q
* perf(editor): compute banded onsets in the background (chunked)
Follow-up to #248. The banded spectral-flux analysis was synchronous (~150ms
one-time), which froze a frame when the onset strip / snap first needed it. Now
it runs in the background: _ensureOnsets returns the cheap RMS-envelope onsets
IMMEDIATELY and kicks a chunked spectral-flux job that upgrades the cache +
redraws when done.
onsets.js: the STFT is split into a resumable plan/step (_spectralFluxPlan +
_spectralFluxStep) so the frame loop can run in bounded batches; the one-shot
_spectralFluxFramesPure / _spectralFluxOnsetsPure are unchanged wrappers over
it (bit-identical output). audio.js: _startOnsetFluxJob downsamples once then
steps ~1500 frames per requestAnimationFrame, swapping in the sharper onsets on
completion; cancelled on new-audio (computeWaveform) and teardownAudio.
Tests: tests/onset_chunk.test.mjs (4) — stepping in ANY budget split is
bit-identical to the one-shot frames + onsets, the done-flag fires exactly at
the last frame, and a zero-frame plan is empty-and-done. Existing onset suite
still green. 136 JS green, lint 0-err, routes.py untouched. Live-verified on
AC/DC: first _ensureOnsets returns in 23.5ms as 'rms' (907), then upgrades to
'spectral-flux' (675, banded) in the background — no synchronous freeze.
Stacks on #248 (feat/editor-onset-spectral-flux); rebase onto main when it merges.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01EBQCHCNA81E9tHmSDHSe2Q
* fix(editor): drop the background onset job when its buffer leaves the session
Two lifecycle bugs in the chunked onset job.
1. A stale job wrote the OLD song's onsets into the NEW session's cache.
Loading an audio-less song nulls S.audioBuffer directly (file-ops.js
loadCDLC, create.js editorApplyCreateResult) — it never reaches
computeWaveform()/teardownAudio(), so the job's `cancelled` flag was
never set and its spectral-flux result landed in the fresh session's
cache (and repainted). Re-check the precondition on every resume: the
job snapshots its buffer and bails if S.audioBuffer is no longer it.
Also only clear _onsetJob when the handle is still ours, so a cancelled
job's last tick can't null out the job that replaced it.
2. The chunk driver was requestAnimationFrame: undefined under node (every
module here has to stay node-importable — _ensureOnsets() threw
ReferenceError the moment a decoded buffer was present) and frozen in a
backgrounded window, which stalled the analysis until the editor was
looked at again. Drive it on setTimeout instead — a chunk that runs
BETWEEN frames also costs the paint less than one that runs in it.
tests/onset_job.test.mjs pins both (plus a positive control that the
upgrade does land, and finite onset times).
* refactor(editor): drive the background job through _spectralFluxOnsetsPlan
_startOnsetFluxJob hand-copied _spectralFluxOnsetsPure's pipeline (the
22050 target, the factor formula, fftSize/hop 512) and audio.js stopped
importing that function, so the SHIPPED onsets and the TESTED onsets were
two separate copies of one tuning — retune the pure function and the editor
would silently keep the old numbers. Split the setup out as
_spectralFluxOnsetsPlan and have both paths build their plan with it; the
chunked driver is now the only thing that differs between them.
onset_chunk.test.mjs pins plan+step+pick == _spectralFluxOnsetsPure at 44.1k
(so the internal downsample is in the comparison too).
---------
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>
byrongamatos
added a commit
that referenced
this pull request
Jul 13, 2026
) * feat(editor): Map Health — per-bar grid-vs-onset drift review lens The whole "review an automatic map" 2nd pass had no review surface until now (P2-4). Map Health scores, per measure, how well the grid agrees with the detected onsets and paints a thin three-state wash under the ruler, so drift is visible wherever you chart. New pure src/map-health.js: _mapHealthPure(beats, onsets, opts) → per-measure {driftFrac, coverage, band} + overall. Per beat resid = |beat.time − nearestOnset|, reported as a FRACTION of the local beat interval (25ms is inaudible at 60bpm but half a subdivision on 200-bpm 16ths — colour by fraction, never raw ms). Per-measure = the MEDIAN driftFrac over EVIDENCED beats (median so one expressive off-beat note can't drag a bar red) + coverage. THREE states, the third non-negotiable: green (agrees, <5%), amber (drifting, 5-12%), red (disagrees with PRESENT onsets, >12%), and GREY when there are no onsets to judge (silence / sustained / held / pedaled) — NEUTRAL, never red. Colouring an unmeasurable held bar red is crying wolf; the author learns to ignore red, which is fatal. Wiring (ruler.js): a ~5px wash under the beat ticks, memoized on editGen + the onset-cache identity (no per-frame recompute), off by default, a view flag (no history). Toggle in the Tempo/Grid menu (audio-only). Reads only S.beats + _ensureOnsets() — so it rides #248's banded onsets once they land, and degrades gracefully to the RMS detector meanwhile (worse onsets → more grey, never wrong). Tests: tests/map_health.test.mjs (10) — aligned→green, offset 8%→amber (the signature fail-on-main case), onsets-removed→grey NOT red, held-bar→grey, median robustness (one expressive onset doesn't flag the timekeeper bar), >12%→red, band thresholds, tempo/meter independence, degenerate input. 135 JS green, lint 0-err, routes.py untouched. Live-verified on AC/DC: 105 measures, overall green @ 88.6% coverage, 68 green / 26 amber / 2 red / 9 grey (not crying wolf), toggle + persistence + stable memo, wash renders on the ruler. FOLLOW-UP (noted): click a hot bar → jump to Tempo Map + offer the G suggest; LCD pill; distinct-signature reporting (lay-back vs nudge vs ramp); octave "reads 2×, halve?" one-click. Core review surface ships here. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EBQCHCNA81E9tHmSDHSe2Q * fix(editor): Map Health — chart-time onsets, shift-aware memo, full final bar Review fixes on the Map Health lens: - HIGH: the lens compared chart-time beats against BUFFER-time onsets (_ensureOnsets). Any non-zero S.audioShift made the whole map read red. Consume _ensureOnsetsShifted() — the chart-time view, which is what every other beat-vs-onset consumer uses. - HIGH: _ensureOnsetsShifted() reallocates on every call when the shift is non-zero, so keying the memo on it would miss every frame and put the O(bars x beats) scan back on the draw path. Key on the RAW cache identity plus the shift scalar instead (and invalidate on a shift change, which the old key could not see at all). - MED: the unclosed final measure ended AT the last beat, clipping the last bar's wash a beat short and collapsing a grid that ends on a closing downbeat to zero width. Extrapolate one beat past the last beat, keeping the canonical _tempoMeasures rule that every downbeat starts a measure. - LOW: the persisted flag let an audio-less chart paint an all-grey strip with no menu row to switch it off. Gate the draw on S.audioBuffer, the same gate as the audioOnly menu row (CodeRabbit). - LOW: _bandFor made exactly greenMax green, contradicting its own header (green is strictly under greenMax) (CodeRabbit). - Toggling on now reports through setStatus like every sibling toggle, and says so explicitly when there are no transients to judge against — an all-grey strip is honest but looks broken when it is silent. 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>
byrongamatos
added a commit
that referenced
this pull request
Jul 13, 2026
…ix (#251) * feat(editor): Map Health — per-bar grid-vs-onset drift review lens The whole "review an automatic map" 2nd pass had no review surface until now (P2-4). Map Health scores, per measure, how well the grid agrees with the detected onsets and paints a thin three-state wash under the ruler, so drift is visible wherever you chart. New pure src/map-health.js: _mapHealthPure(beats, onsets, opts) → per-measure {driftFrac, coverage, band} + overall. Per beat resid = |beat.time − nearestOnset|, reported as a FRACTION of the local beat interval (25ms is inaudible at 60bpm but half a subdivision on 200-bpm 16ths — colour by fraction, never raw ms). Per-measure = the MEDIAN driftFrac over EVIDENCED beats (median so one expressive off-beat note can't drag a bar red) + coverage. THREE states, the third non-negotiable: green (agrees, <5%), amber (drifting, 5-12%), red (disagrees with PRESENT onsets, >12%), and GREY when there are no onsets to judge (silence / sustained / held / pedaled) — NEUTRAL, never red. Colouring an unmeasurable held bar red is crying wolf; the author learns to ignore red, which is fatal. Wiring (ruler.js): a ~5px wash under the beat ticks, memoized on editGen + the onset-cache identity (no per-frame recompute), off by default, a view flag (no history). Toggle in the Tempo/Grid menu (audio-only). Reads only S.beats + _ensureOnsets() — so it rides #248's banded onsets once they land, and degrades gracefully to the RMS detector meanwhile (worse onsets → more grey, never wrong). Tests: tests/map_health.test.mjs (10) — aligned→green, offset 8%→amber (the signature fail-on-main case), onsets-removed→grey NOT red, held-bar→grey, median robustness (one expressive onset doesn't flag the timekeeper bar), >12%→red, band thresholds, tempo/meter independence, degenerate input. 135 JS green, lint 0-err, routes.py untouched. Live-verified on AC/DC: 105 measures, overall green @ 88.6% coverage, 68 green / 26 amber / 2 red / 9 grey (not crying wolf), toggle + persistence + stable memo, wash renders on the ruler. FOLLOW-UP (noted): click a hot bar → jump to Tempo Map + offer the G suggest; LCD pill; distinct-signature reporting (lay-back vs nudge vs ramp); octave "reads 2×, halve?" one-click. Core review surface ships here. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EBQCHCNA81E9tHmSDHSe2Q * feat(editor): Map Health click-through — a hot bar takes you to the fix Follow-up to #249. "Tempo is wrong here" → "click the red bar, the fix is waiting." Clicking a DRIFTING (amber/red) bar in the Map Health wash now enters Tempo Map mode, scrolls the bar into view, anchors Suggest on that bar's downbeat (S.tempoSel), and tells the user G is waiting — so the fix is one keypress away. Green/grey bars aren't actionable and fall through to the normal ruler scrub. map-health.js: each measure now carries beatIdx (the S.beats downbeat index) so the click-through can anchor Suggest exactly. ruler.js: _mapHealthBarAt(t) + _mapHealthClickThrough(t) (enter Tempo Map first — it clears the selection — THEN set tempoSel), wired into rulerOnMouseDown for clicks on the thin wash at the ruler's bottom edge. Tests: map_health.test.mjs gains a beatIdx case (downbeat indices 0/4/8 across 3 bars). 135 JS green, lint 0-err, routes.py untouched. Live-verified on AC/DC: clicking amber bar 6 enters Tempo Map, sets tempoSel to its downbeat (idx 20), and posts "Bar 6 drifts 7% … press G"; green + grey bars return false (scrub). Stacks on #249 (feat/editor-map-health); rebase onto main when it merges. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EBQCHCNA81E9tHmSDHSe2Q * fix(editor): Map Health click-through — hit-test the painted wash, not a guess Review fixes on the click-through: · The click target now IS the painted rect. MAP_HEALTH_BAND_H is one constant shared by _drawMapHealthBand and the mousedown gate (was: paints 5px, clicks 6px), and the gate demands x >= LABEL_W — the wash clamps its spans to the label gutter's right, so a click on the bare gutter was claiming whatever bar xToTime() happened to land in and teleporting you there. · A click-through arriving while ALREADY in Tempo Map now clears tempoSelMulti. _editorTempoSuggestFit re-anchors on a live multi-range's first downbeat, which outranks tempoSel — so with a range selected, Suggest silently fit from the wrong bar while the status line promised the bar you clicked. · The lead-in scroll is bounded by the viewport. Zoom caps at 2000 px/s, so a viewport can be shorter than 0.5s: the flat 0.5s lead pushed the very downbeat you clicked off the RIGHT edge. · The status copy resolves the Suggest key from the command registry instead of hardcoding "G" — two shortcut profiles exist. Copy also no longer claims G "fits" the barlines; it proposes a fit (a ghost-handle click accepts). Six regression tests in tests/ruler.test.mjs drive the real onset detector; five fail on the pre-fix code. 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>
byrongamatos
added a commit
that referenced
this pull request
Jul 13, 2026
#252) * 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 * fix(editor): harden the segment-first tempo engine against crash, hang 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> * fix(editor): window the downbeat-phase search to its own segment 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> * 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>
byrongamatos
added a commit
that referenced
this pull request
Jul 13, 2026
* 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>
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
2 participants
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.
The first code PR of the "make the automatic map good" 2nd pass (P2-2). It replaces the onset detector — the amber attack markers the onset strip shows and note-drags snap to, and the primary surface a transcriber places against.
Why
The pass-1 detector (
_onsetTimesFromPeaksPure) was broadband RMS-rise on the waveform envelope. Every pedagogy seat demanded "detect on onsets/bands, not total loudness," because RMS-rise goes blind on:It also de-risks segment-first mapping (P2-3) and makes the Map Health lens (P2-4) meaningful.
What
New pure, zero-dependency
src/onsets.js:Wired behind
_ensureOnsets()so it's a drop-in: the shape is the pass-1[{t, s}]extended to[{t, s, bands:{lo,mid,hi}}]— every consumer unchanged,_ensureOnsetsShiftedcarries the bands through. The RMS detector stays as a labelled fallback. Computed once per load (downsampled to ~22 kHz), cached.Tests
tests/onsets_spectral_flux.test.mjs(12) against synthetic signals with known-correct answers — FFT (a cosine → exactly its bin), Hann shape, band-bin math, decimation, flux frames (silence→burst spike; a 60 Hz tone lights the lo band, an 8 kHz tone the hi band), peak-pick (adaptive threshold / refractory / parabolic sub-hop), the full click-train pipeline, and junk-input degradation.routes.pyuntouched.Follow-up (noted, not in this PR)
The ~150 ms analysis is currently synchronous + on-demand (the onset strip is off by default, and the RMS detector covers the instant before it completes). A fast-follow can chunk it across rAF or a Worker for zero jank on a strip-enabled load.
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Tests