feat(editor): count-in — N bars of clicks before playback starts - #203
Conversation
The charrette's Count, built as its own feature (the B2 transport bar's Count-in LCD cell and the Count mode toggle were both deferred because the editor had nothing to write through to — this unblocks them). A 'Count: off / 1 / 2 / 4 bars' select next to the metronome. On play, _countInPlanPure (src/transport.js, dep-free) derives the pre-roll from the grid AT THE CURSOR — beats-per-bar from the enclosing measure's downbeat span, beat length from the local gap — so a 3/4 section counts three beats at its own tempo. No grid falls back to a 4/4 bar at 120 so a play gesture never fails; junk bar counts refuse. The engine change is ONE anchor shift: _anchorTransportAtCursor and _startAudioSourceAtCursor take an optional preRoll and pin the wall anchor (and the BufferSource start) into the future by it. Because the whole chart-to-ctx mapping derives from that anchor pair, the guide/ click scheduler and the MIDI record clock follow with zero extra plumbing — recording gets a count-in for free. playbackTick clamps the cursor at the start position while the pre-roll runs (the raw chart time reads negative there). Count-in clicks schedule through the existing _metroClickVoiceAt (tracked voices — a stop mid-count cancels them like any queued clap). Loop wraps and mid-play seeks route through _restartPlaybackAt and stay immediate by design. Pref: localStorage 'editorCountIn' (0/1/2/4), seeded into the select at initAudio like the snap-mode pref; never written to the pack. tests/count_in.test.mjs (5): the 4/4 plan (duration, click offsets, downbeat-only accents), multi-bar accents, meter/tempo AT the cursor on a drifting mixed-meter grid, the no-grid fallback, junk refusals. 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>
…lock in pre-roll The count-in clicks were scheduled onto _guideVoices BEFORE _anchorTransportAtCursor, whose _guideResetSchedule() -> _guideCancelVoices() osc.stop()s every queued voice — cancelling the clicks before they could sound. The count-in was audibly silent (N bars of dead pre-roll, then playback). Schedule the clicks AFTER the anchor instead; nothing between there and playback start cancels voices. Also clamp the MIDI record clock (chartTimeNow) at playStartTime, mirroring playbackTick: during a pre-roll the anchor sits in the future, so a note played over the count would otherwise record at a negative/pre-region chart time. Post-start notes are already >= playStartTime, so the clamp is a no-op for them. Adds tests/count_in_wiring.test.mjs: drives startPlayback with a fake AudioContext and asserts no click voice is cancelled (distinguishing the natural scheduled osc.stop(t) from the no-arg cancel stop), plus the recorder clamp rule. Verified the test fails when the scheduling is moved back before the anchor. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
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 configurable 1-, 2-, and 4-bar count-in playback with tempo- and meter-aware click scheduling, pre-roll transport alignment, recording-time clamping, editor persistence, UI wiring, and automated planner and playback tests. ChangesCount-in playback
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Editor
participant startPlayback
participant _countInPlanPure
participant Transport
participant Audio
participant Metronome
Editor->>startPlayback: start playback or recording
startPlayback->>_countInPlanPure: calculate count-in plan
_countInPlanPure-->>startPlayback: pre-roll duration and clicks
startPlayback->>Transport: anchor cursor with preRoll
startPlayback->>Audio: start source with preRoll
startPlayback->>Metronome: schedule count-in click voices
🚥 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.
🧹 Nitpick comments (2)
tests/count_in_wiring.test.mjs (1)
119-128: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTest doesn't exercise the real
chartTimeNow.This test reimplements the clamp formula inline rather than calling
midi-record.js'schartTimeNow(), which isn't exported. It validates that_transportChartTimePure+Math.maxbehave correctly in isolation, but a regression introduced directly insidechartTimeNow(e.g., dropped clamp, swapped args) would pass this test undetected — despite the docstring's claim that it "pins the recorder clamp (chartTimeNow) rule."Consider exporting
chartTimeNowfor direct testing, or driving the test through the actual recording start/tick path.🤖 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/count_in_wiring.test.mjs` around lines 119 - 128, The recorder clamp test currently validates an inline formula instead of the production chartTimeNow implementation. Export chartTimeNow from midi-record.js and update the “recorder clamp” test to call that exported function directly, preserving the existing pre-roll clamping and post-start passthrough assertions.src/audio.js (1)
260-273: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueFragile: two independent
currentTimereads for one scheduling decision.
_startAudioSourceAtCursorreadsS.audioCtx.currentTimefor.start(), then_anchorTransportAtCursorre-reads it independently forS.playStartWall. Today these happen back-to-back synchronously so they resolve to the same value, but nothing enforces that — a future refactor adding any async step between the two calls would silently desync the audio start time from the transport anchor, breaking count-in alignment without an obvious symptom.Consider computing the wall time once and threading it through both calls.
💡 Illustrative fix
export function _startAudioSourceAtCursor(preRoll = 0) { S.audioSource = S.audioCtx.createBufferSource(); S.audioSource.buffer = S.audioBuffer; ... - S.audioSource.start(preRoll > 0 ? S.audioCtx.currentTime + preRoll : 0, S.cursorTime); - _anchorTransportAtCursor(preRoll); + const startWall = preRoll > 0 ? S.audioCtx.currentTime + preRoll : 0; + S.audioSource.start(startWall, S.cursorTime); + _anchorTransportAtCursor(preRoll, startWall > 0 ? startWall : S.audioCtx.currentTime); }Also applies to: 283-291
🤖 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 260 - 273, Update _startAudioSourceAtCursor to compute the scheduling wall time from S.audioCtx.currentTime once, use that value for audioSource.start, and pass the same value into _anchorTransportAtCursor. Adjust _anchorTransportAtCursor to accept and use the supplied wall time for S.playStartWall, preserving the existing preRoll and cursor 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.
Nitpick comments:
In `@src/audio.js`:
- Around line 260-273: Update _startAudioSourceAtCursor to compute the
scheduling wall time from S.audioCtx.currentTime once, use that value for
audioSource.start, and pass the same value into _anchorTransportAtCursor. Adjust
_anchorTransportAtCursor to accept and use the supplied wall time for
S.playStartWall, preserving the existing preRoll and cursor behavior.
In `@tests/count_in_wiring.test.mjs`:
- Around line 119-128: The recorder clamp test currently validates an inline
formula instead of the production chartTimeNow implementation. Export
chartTimeNow from midi-record.js and update the “recorder clamp” test to call
that exported function directly, preserving the existing pre-roll clamping and
post-start passthrough assertions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 8179c745-0bca-4eff-8055-9162bf1354c3
📒 Files selected for processing (8)
CHANGELOG.mdscreen.htmlsrc/audio.jssrc/main.jssrc/midi-record.jssrc/transport.jstests/count_in.test.mjstests/count_in_wiring.test.mjs
… paths The Count toggle restores the "last non-zero count" from localStorage['editorCountInLast'], but that memory was only written by the toggle and the LCD-cell handler — not by editorSetCountIn itself. Setting the count via #203's toolbar Count: select and then clicking the toggle restored a stale value (or fell back to 1 bar). Record editorCountInLast inside editorSetCountIn whenever it's set to a non-zero value, so the toolbar select, the LCD cell, and the toggle all share one memory. Whitelist ({0,1,2,4}) unchanged; pure editor pref, no chart/history involvement. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
… paths The Count toggle restores the "last non-zero count" from localStorage['editorCountInLast'], but that memory was only written by the toggle and the LCD-cell handler — not by editorSetCountIn itself. Setting the count via #203's toolbar Count: select and then clicking the toggle restored a stale value (or fell back to 1 bar). Record editorCountInLast inside editorSetCountIn whenever it's set to a non-zero value, so the toolbar select, the LCD cell, and the toggle all share one memory. Whitelist ({0,1,2,4}) unchanged; pure editor pref, no chart/history involvement. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* feat(editor): count-in LCD cell + Count transport toggle The pair the B2 transport slice deferred until count-in existed: a Count LCD cell (Off/1/2/4 bars, between Key and Sel) writing through to editorSetCountIn and mirrored on the tick, and a Count mode toggle that arms/disarms the pre-roll, remembering the bar count across the off state (editor pref). Old Customize pref blobs default the new cell to visible. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EBQCHCNA81E9tHmSDHSe2Q * fix(editor): remember LCD count-in selection * fix(editor): record count-in memory in editorSetCountIn for all write paths The Count toggle restores the "last non-zero count" from localStorage['editorCountInLast'], but that memory was only written by the toggle and the LCD-cell handler — not by editorSetCountIn itself. Setting the count via #203's toolbar Count: select and then clicking the toggle restored a stale value (or fell back to 1 bar). Record editorCountInLast inside editorSetCountIn whenever it's set to a non-zero value, so the toolbar select, the LCD cell, and the toggle all share one memory. Whitelist ({0,1,2,4}) unchanged; pure editor pref, no chart/history involvement. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- Co-authored-by: ChrisBeWithYou <chris@rifflarr.local> Co-authored-by: Claude Fable 5 <noreply@anthropic.com> Co-authored-by: byrongamatos <xasiklas@gmail.com>
2 participants
What
The charrette's Count, built as its own feature — this is what the B2 transport bar's Count-in LCD cell (#195) and the Count mode toggle were explicitly deferred behind; both can now follow as small PRs.
A
Count: off / 1 / 2 / 4 barsselect next to the metronome. On play, the pre-roll derives from the grid at the cursor — beats-per-bar from the enclosing measure's downbeat span, beat length from the local gap — so a 3/4 section counts three beats at its own tempo. No tempo map falls back to a 4/4 bar at 120 (a play gesture never fails).One anchor shift
_anchorTransportAtCursor/_startAudioSourceAtCursortake an optionalpreRolland pin the wall anchor (and the BufferSource start) into the future by it. The whole chart→ctx mapping derives from that anchor pair, so the guide/click scheduler and the MIDI record clock follow with zero extra plumbing — recording gets a count-in for free.playbackTickclamps the cursor at the start during the pre-roll. Clicks schedule through the existing_metroClickVoiceAt(tracked voices — a stop mid-count cancels them like any queued clap). Loop wraps and mid-play seeks route through_restartPlaybackAtand stay immediate by design.Pref:
editorCountIn(editor pref, never the pack), seeded atinitAudiolike the snap-mode pref.Tests
tests/count_in.test.mjs(5, real-import): the 4/4 plan (duration, click offsets, downbeat-only accents), multi-bar accents, meter/tempo at the cursor on a drifting mixed-meter grid, the no-grid fallback, junk refusals. Full suite 91/91 · ESLint 0 errors · CHANGELOG updated.🤖 Generated with Claude Code
https://claude.ai/code/session_01EBQCHCNA81E9tHmSDHSe2Q
Summary by CodeRabbit
New Features
Bug Fixes
Tests