feat(editor): pickup bars + onset-based tempo detection (workspace-shell D3) - #202
Conversation
…ell D3) Two halves of 'bar 1 beat 1 does not align with the first audible note': PICKUP (anacrusis): a 'Set pickup…' verb on the first measure's sync point (Tempo Map context menu + a registry command, so the B4 menu lists it when both land). Semantics are RE-BAR, not split: the true downbeats sit N beats after the grid start, so every bar boundary shifts EARLIER by (firstBarLen − N) — each bar keeps its own length (varying meters survive), the first bar becomes the N-beat pickup, freed beats fall into the tail. Numbering-only: NO beat time moves — the offset-vs-flex distinction holds by construction (pinned by test) — applied as one undoable TempoGridCmd, so under beat-primary notes re-lift and keep their seconds. Display: with a partial first bar, measure labels shift so the pickup reads as bar 0 and the first full bar as bar 1 — DERIVED from the grid (first bar shorter than second), no stored flag, nothing on the wire. The rule is inlined in the measure-readout pure (keeps it sliceable) and shared as _pickupBarShiftPure for the tempo-map labels; the comments cross-reference. Notes before beat 0 were already representable (A1's converter extrapolates) — now pinned by test. DETECT (from the onset strip): the Sync dialog prefers the already-computed onset strip over re-crunching the raw buffer: _detectTempoFromOnsetsPure votes CONSECUTIVE inter-onset intervals (strength-weighted, octave-folded into 60-220 — consecutive-only matters: multi-lag pairs stack votes on subharmonics and a clean 120 BPM take read as 60 in the first cut; the suite caught it), refines to sub-BPM, and — unlike the autocorrelation — proposes the DOWNBEAT PHASE plus a confidence score, both shown in the dialog. Diffuse votes (rubato) fall back to the existing waveform autocorrelation and say so. The R2 lint gate caught a missing _editorPromptText import in tempo.js before it could ship as a runtime crash — exactly its job. tests/pickup_detect.test.mjs (7): rebar keeps every beat time, refusals (full-bar pickup, junk counts), the derived bar-0 display shift, the pre-beat-0 extrapolation round-trip, clean-signal bpm/phase/confidence, octave folding, and honest degradation on diffuse input. Suite 91/91, ESLint 0 errors. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EBQCHCNA81E9tHmSDHSe2Q Signed-off-by: ChrisBeWithYou <chris@rifflarr.local>
|
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 (4)
📝 WalkthroughWalkthroughAdds pickup-bar support to the tempo map, including numbering-only beat-grid transformations and command wiring. Sync Tempo now prefers onset-strip BPM and phase estimates with confidence-based waveform fallback. New tests cover pickup behavior, beat conversion, and detection quality. ChangesTempo features
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant editorSyncTempo
participant OnsetStrip
participant _detectTempoFromOnsetsPure
participant detectAudioBPM
participant SyncDialog
editorSyncTempo->>OnsetStrip: request onset data via _ensureOnsets()
OnsetStrip-->>editorSyncTempo: return onset events
editorSyncTempo->>_detectTempoFromOnsetsPure: calculate BPM, phase, confidence
alt confidence >= 0.15
_detectTempoFromOnsetsPure-->>SyncDialog: display onset BPM, confidence, and phase
else diffuse or unavailable onset estimate
editorSyncTempo->>detectAudioBPM: calculate fallback BPM
detectAudioBPM-->>SyncDialog: display waveform BPM and fallback hint
end
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
tests/pickup_detect.test.mjs (1)
30-70: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winNo test for varying bar lengths under pickup.
The PR objective states pickup handling "preserves varying bar lengths," but
grid()only generates uniform-length bars, so this claim isn't exercised by any test here. Worth adding a fixture with mixed bar lengths (e.g., a 3-beat bar followed by a 5-beat bar) and asserting inter-bar spacing is preserved after_tempoSetPickupPure.🤖 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/pickup_detect.test.mjs` around lines 30 - 70, Add a mixed-length bar fixture in the pickup tests, such as a 3-beat bar followed by a 5-beat bar, and verify that _tempoSetPickupPure preserves the original beat times and inter-downbeat spacing while applying a valid partial pickup. Keep the assertions focused on the varying bar lengths rather than the uniform grid() helper.src/main.js (1)
1008-1017: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicated pickup-shift logic — drift risk.
This re-implements
_pickupBarShiftPurefromsrc/tempo.jsinline rather than importing it. The comment explains this is intentional for dependency-freeness, but two hand-synced copies of the same 5-line rule will silently diverge if one is edited without the other (no test currently guards cross-file consistency).Given
main.jsalready imports other tempo helpers, importing_pickupBarShiftPuredirectly would remove the duplication with minimal added coupling.🤖 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/main.js` around lines 1008 - 1017, Replace the duplicated pickup-shift calculation in the measure-label logic with the existing _pickupBarShiftPure helper from src/tempo.js. Add it to main.js’s tempo imports and use its result when constructing the label, removing the local dbs loop and shift expression while preserving the existing measure, numerator, and denominator behavior.
🤖 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 907-933: In _tempoPromptPickup, add the same beats[i] truthiness
guard to the ndb search loop before reading .measure, matching the existing d0
loop and _tempoSetPickupPure behavior so falsy beat entries are skipped safely.
---
Nitpick comments:
In `@src/main.js`:
- Around line 1008-1017: Replace the duplicated pickup-shift calculation in the
measure-label logic with the existing _pickupBarShiftPure helper from
src/tempo.js. Add it to main.js’s tempo imports and use its result when
constructing the label, removing the local dbs loop and shift expression while
preserving the existing measure, numerator, and denominator behavior.
In `@tests/pickup_detect.test.mjs`:
- Around line 30-70: Add a mixed-length bar fixture in the pickup tests, such as
a 3-beat bar followed by a 5-beat bar, and verify that _tempoSetPickupPure
preserves the original beat times and inter-downbeat spacing while applying a
valid partial pickup. Keep the assertions focused on the varying bar lengths
rather than the uniform grid() helper.
🪄 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: 572a2bad-033d-4a43-b85e-03eb547a89ee
📒 Files selected for processing (8)
CHANGELOG.mdscreen.htmlsrc/input.jssrc/main.jssrc/shortcuts.jssrc/sync-tempo.jssrc/tempo.jstests/pickup_detect.test.mjs
CodeRabbit: the loop read beats[i].measure without the beats[i] guard every sibling loop in the pickup flow uses — a falsy grid entry would throw instead of degrading. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EBQCHCNA81E9tHmSDHSe2Q Signed-off-by: ChrisBeWithYou <chris@rifflarr.local>
2 participants
What
Two halves of "bar 1 beat 1 doesn't align with the first audible note" (charrette §2/§5), scoped with Christian:
Set pickup (anacrusis)
A Set pickup… verb on the first measure's sync point (Tempo Map context menu + a registry command — the B4 menu (#196) lists it automatically when both land). Semantics are re-bar, not split: the true downbeats sit N beats after the grid start, so every bar boundary shifts earlier by (firstBarLen − N) — each bar keeps its own length (varying meters survive), the first bar becomes the N-beat pickup.
TempoGridCmd, so under beat-primary the notes re-lift and keep their seconds.measure > 0downbeat convention is untouched.Detect tempo from the onset strip
The Sync dialog now prefers the already-computed onset strip over re-crunching the raw buffer:
_detectTempoFromOnsetsPurevotes consecutive inter-onset intervals (strength-weighted, octave-folded into 60–220), refines to sub-BPM, and — unlike the existing autocorrelation — proposes the downbeat phase plus a confidence score, both shown in the dialog. Diffuse votes (rubato, swing feel) fall back to the waveform autocorrelation and say so rather than pretending.Consecutive-only voting matters: multi-lag pairs stack votes on subharmonics, and a clean 120 BPM take read as 60 in the first cut — the suite caught it. The R2 lint gate also caught a missing
_editorPromptTextimport before it could ship as a runtime crash.Tests
tests/pickup_detect.test.mjs(7, real-import): rebar keeps every beat time, refusal cases, the derived bar-0 shift, the pre-beat-0 extrapolation round-trip, clean-signal bpm/phase/confidence, octave folding, honest degradation on diffuse input. Full suite 91/91 · ESLint 0 errors · CHANGELOG updated.🤖 Generated with Claude Code
https://claude.ai/code/session_01EBQCHCNA81E9tHmSDHSe2Q
Summary by CodeRabbit