feat(editor): loop A/B compare — alternate recording and guide per loop pass - #122
Conversation
…op pass Roadmap 1.6, the last Phase-1 slice that was waiting on #109 (it mutes the reference through the mixer transparent _refGain). The ear-training loop: while looping, each pass alternates RECORDING (reference audible, claps off) and GUIDE (reference muted, claps on — overriding the claps pref in both directions), so a charter hears their chart against the artist one pass apart. - Phase flips ride the loop wrap (before _restartPlaybackAt so the ramp lands with the wrap); every (re)start/seek begins on the recording pass; stop mid-guide-pass restores the fader level; clearing the loop region disarms A/B; song load resets it (session-only by design). - Arming A/B with a region set but looping off arms the loop too. - _guideTimerSync now schedules whenever A/B is active (guide passes need claps even with the pref off). - Toolbar A/B button next to Loop + registry toggleLoopAB (Alt+B). Tests: tests/loop_ab.test.js (5) — pref-override truth table, strict two-cycle, mute-only-during-active-playing-guide (incl. the stop-restores case), and a composed 4-pass alternation. Full suite green except the pre-existing CRLF section_coverage failure (#116). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JEoFeTPSnz4NpwwCG52hnu
|
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. 📝 WalkthroughWalkthroughAdds a Loop A/B compare transport mode that alternates each loop pass between recording and guide behavior, with new UI/shortcut wiring, playback integration, validation, and changelog notes. ChangesLoop A/B compare feature
Estimated code review effort: 3 (Moderate) | ~30 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant UI as A/B button or shortcut
participant screenjs as screen.js
participant Scheduler as guide scheduler
participant RefGain as reference gain
User->>UI: toggle A/B
UI->>screenjs: _editorToggleLoopAB()
screenjs->>screenjs: set _abOn and _abPhase
screenjs->>RefGain: apply reference-gain automation
loop each loop pass
Scheduler->>screenjs: loop wrap
screenjs->>screenjs: _abNextPhasePure()
screenjs->>RefGain: update target
screenjs->>Scheduler: _abClapsEnabledPure()
end
User->>UI: clear or stop loop
UI->>screenjs: disarm / stop
screenjs->>screenjs: clear _abOn and reset phase
screenjs->>RefGain: reapply reference-gain automation
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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: 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 `@screen.js`:
- Around line 6014-6018: The A/B state is being reset too late in the
restart/seek flow, allowing playbackTick() and _guideTimerSync() to run while
_abPhase still reflects the previous guide state. Update the restart/seek logic
around _guideTimerSync() and _abTick()/playback startup so _abPhase is set to
'recording' before any timer sync or _guideTick() can fire, ensuring the first
playback tick always starts from the recording pass.
- Around line 6773-6776: The song-load reset in the A/B state handling only
clears the session flags, but it leaves the reference gain automation and A/B
button UI stale after a guide pass. Update the same reset path around _abOn and
_abPhase to also refresh the audio/UI state by reapplying the reference gain
automation and synchronizing the A/B button styling/state whenever a new song is
loaded.
🪄 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: ee2936c8-8603-4949-a788-c5723e53a9b3
📒 Files selected for processing (4)
CHANGELOG.mdscreen.htmlscreen.jstests/loop_ab.test.js
- ref fader no longer un-mutes an active A/B guide pass (route ref bus moves through the A/B-aware target; guarded for the @pure sandbox) - mid-play A/B arming delegates to _setLoopRegionEnabled so the cursor seeks into the loop region instead of riding pre-loop audio - disabling the loop mid-guide-pass restores the recording to its fader level instead of leaving it silently muted - regression tests drive the real stateful runtime; all 3 fail pre-fix Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
@coderabbitai review |
✅ Action performedReview finished.
|
- reset A/B phase + ref gain BEFORE the first playbackTick/_guideTimerSync so a fresh start/seek can't schedule a guide pass off a stale phase (and the first-play fade stays the last ref-gain automation) - on song load, also reapply ref gain + sync scheduler + refresh controls so a load mid-guide-pass never leaves a stale mute or button styling Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Addressed in f112506: reset |
|
Addressed in f112506: the song-load reset now also calls |
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/loop_ab.test.js (1)
154-168: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for
_guideTimerSyncgating.The PR explicitly changes
_guideTimerSyncto run whenever A/B is active (so guide passes clap even with claps disabled in prefs), but this sandbox only ever wires it as a no-op (() => {}, line 168) with no spy. None of the added tests assert that it's actually invoked when A/B is active, so a regression here (e.g. it stops being called, or is called at the wrong time) would go undetected by this suite.♻️ Suggested spy wiring
- const spies = { setLoopRegionEnabled: [] }; + const spies = { setLoopRegionEnabled: [], guideTimerSync: 0 }; @@ - () => {}, // _guideTimerSync + () => { spies.guideTimerSync++; }, // _guideTimerSyncThen assert
spies.guideTimerSyncincrements at the expected call sites (e.g. after_editorToggleLoopAB()turns A/B on).🤖 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/loop_ab.test.js` around lines 154 - 168, Add test coverage for the `_guideTimerSync` gating behavior in the A/B sandbox. Wire the `_guideTimerSync` dependency in the `new Function` harness to a spy instead of a no-op, then assert it is called when `_editorToggleLoopAB` enables A/B and at any other expected A/B-active call sites. Use the existing `spies` object and the `_editorToggleLoopAB`, `_guideTimerSync`, and `setPhase` helpers to verify the invocation timing.
🤖 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 `@tests/loop_ab.test.js`:
- Around line 154-168: Add test coverage for the `_guideTimerSync` gating
behavior in the A/B sandbox. Wire the `_guideTimerSync` dependency in the `new
Function` harness to a spy instead of a no-op, then assert it is called when
`_editorToggleLoopAB` enables A/B and at any other expected A/B-active call
sites. Use the existing `spies` object and the `_editorToggleLoopAB`,
`_guideTimerSync`, and `setPhase` helpers to verify the invocation timing.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 78685b62-9d98-4bdc-aca8-6551d33ed222
📒 Files selected for processing (2)
screen.jstests/loop_ab.test.js
🚧 Files skipped from review as they are similar to previous changes (1)
- screen.js
# Conflicts: # CHANGELOG.md
2 participants
Roadmap 1.6 — the last "Hear your chart" slice, unblocked by #109 merging (it mutes the reference through the mixer''s transparent
_refGain). With this, Phase 1 is complete except the wafonts-gated pitched synth (1.2/1.5).What
The ear-training loop: while looping, each pass alternates between the recording (reference audible, claps off) and the guide (reference muted, claps on — overriding the claps pref in both directions so a guide pass is never silent and a recording pass is never cluttered). A charter hears what they charted against what the artist played, one pass apart.
startPlayback— begins on the recording pass, so the real thing always comes first from a fresh position._guideTimerSyncschedules whenever A/B is active, so guide passes clap even with the claps pref off.toggleLoopAB(Alt+B, both profiles, collision-checked).Tests
tests/loop_ab.test.js(5): the pref-override truth table, the strict two-cycle phase flip, mute-only-during-an-active-playing-guide-pass (including the stop-restores case), and a composed 4-pass alternation asserting each pass is exactly one surface — never both, never neither. Full suite green except the pre-existing CRLFsection_coveragefailure (#116 fixes it).Fresh region vs the open queue — guide scheduler + loop controls + one toolbar button.
🤖 Generated with Claude Code
https://claude.ai/code/session_01JEoFeTPSnz4NpwwCG52hnu
Summary by CodeRabbit