feat(editor): audio regions — place, trim, declick & window a stem's audio (Track Regions PR4)#358
Conversation
|
Warning Review limit reached
Next review available in: 16 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 (10)
📝 WalkthroughWalkthroughAudio regions now control per-region playback windows, declicked trims, and waveform slices. A new undoable trim command updates region windows while preserving content and names. Tests cover scheduling, waveform mapping, undo/redo, and save→reload stability. ChangesAudio regions
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Timeline
participant StemScheduler
participant RegionResolver
participant AudioBufferSourceNode
Timeline->>StemScheduler: request playback at cursor
StemScheduler->>RegionResolver: resolve track regions
RegionResolver-->>StemScheduler: region placements and media windows
StemScheduler->>AudioBufferSourceNode: schedule trimmed region sources
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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)
src/audio.js (2)
524-548: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
_audioRegionPlacementsPurelogic verified against tests; consider sharing the window-resolution math withparts-view.js.The beat-to-time placement,
srcIn/srcOutclamping, and default full-span fallback here are functionally identical to_regionWaveWindowPureinsrc/parts-view.js(Lines 117-126) — sameMath.max(0, srcIn), sameNumber.isFinite(so) && so > srcIn ? min(so,dur) : durclamp, same beat-0-relative placement trick. Extracting a sharedregion.jshelper (e.g._regionMediaWindowPure(region, dur, beatToTime)returning{srcIn, srcOut, startBeatTime}) would remove this duplication and prevent the two call sites from silently drifting apart on a future edge-case 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 `@src/audio.js` around lines 524 - 548, Extract the shared region media-window calculation used by _audioRegionPlacementsPure and _regionWaveWindowPure into a common region.js helper, such as _regionMediaWindowPure. Preserve the existing srcIn/srcOut clamping, default full-span fallback, and beat-0-relative startBeatTime behavior, then update both call sites to reuse the helper.
2221-2278: 🩺 Stability & Availability | 🔵 Trivial | ⚖️ Poor tradeoffPer-region scheduling wiring is coherent but has no direct test coverage.
The region loop (placement →
_regionStartPure→ optional declick gain →node.start()) correctly threadsbaseShift + source.offset + region.startBeatTime, matches the "byte-identical for the default region" invariant, and cleans up transient gain nodes viaonended. However, this integration path (as opposed to the pure helpers it composes) isn't exercised by any test in this diff — only the pure functions are unit-tested. This is already acknowledged in the PR objectives as requiring runtime dogfooding, but worth calling out as a residual coverage gap before merge, given it's the actual playback wiring users will hear.🤖 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 2221 - 2278, Add direct integration coverage for _startStemSources, exercising the full region placement, timing, optional declick gain, node.start scheduling, and cleanup wiring rather than only testing pure helpers. Include the default full-span region behavior and a trimmed/moved region case, verifying the expected playback scheduling and gain-node lifecycle.
🤖 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 524-548: Extract the shared region media-window calculation used
by _audioRegionPlacementsPure and _regionWaveWindowPure into a common region.js
helper, such as _regionMediaWindowPure. Preserve the existing srcIn/srcOut
clamping, default full-span fallback, and beat-0-relative startBeatTime
behavior, then update both call sites to reuse the helper.
- Around line 2221-2278: Add direct integration coverage for _startStemSources,
exercising the full region placement, timing, optional declick gain, node.start
scheduling, and cleanup wiring rather than only testing pure helpers. Include
the default full-span region behavior and a trimmed/moved region case, verifying
the expected playback scheduling and gain-node lifecycle.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: d185ed50-c662-4f6c-8461-31300b85143c
📒 Files selected for processing (9)
CHANGELOG.mdsrc/audio.jssrc/parts-view.jssrc/region-commands.jstests/audio_shift.test.mjstests/region_trim.test.mjstests/region_waveform.test.mjstests/test_track_regions.pytests/track_regions.test.mjs
…(regions PR4, step 1)
Track-regions PR4 turns audio playback from one-source-per-stem into one-source-
per-REGION so an audio block can be placed/trimmed independently. This lands the
load-bearing pure core: _regionStartPure(cursor, startTime, srcIn, srcOut) →
{play, offset, delay, duration}, the trimmable generalization of
_audioBufferStartPure. It expresses trim purely as BufferSource start()/duration
args — the media buffer is never stretched (audio-immutable preserved).
The whole-stem case is the degenerate region [srcIn=0, srcOut=duration] placed at
the audio shift, and the test pins it byte-identical to _audioBufferStartPure for
every cursor position (before / within / past the buffer) — so the later
scheduler rewire can't regress today's single-source path. Also covers a trimmed
[srcIn,srcOut) window (offset/duration/past-tail) and the untrimmed-tail case
(null duration → scheduler omits the 3rd start() arg).
No wiring yet — the scheduler still calls the whole-stem path; _regionStartPure
is unused until step 2 rewires _startStemSources to iterate a track's regions.
Tests: tests/audio_shift.test.mjs (3 new _regionStartPure cases).
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_017xGPjDBF8NTwTK7VQvizix
Signed-off-by: ChrisBeWithYou <chris@rifflarr.local>
…ons PR4, step 3) The editing verb that makes _regionStartPure's trim args meaningful. Adjusts a region's WINDOW only, never its content: srcIn/srcOut (audio media in/out points, the file's own seconds) or startBeat/lenBeat (notation beat window). Notes that fall outside a narrowed notation window are HIDDEN by membership, never deleted — a later widen brings them straight back. Container-only: touches nothing but track.regions[] (no editGen/content churn), so rollback restores the raw regions[] verbatim, including deleting a `regions` key that was never there. _trimPatchPure whitelists the four window fields (finite number | explicit null → "clear this bound"), dropping content/junk so a trim can't smuggle anything onto a region. srcOut:null clears the out-point; trimming the implicit default region materializes it (undo deletes the key); an unknown region id or all-dropped patch is a true no-op that never materializes the default (untouched packs stay byte-identical). Tests: tests/region_trim.test.mjs (7 cases — audio/notation trim, exec→undo→redo round-trip through S.history, name preserved, default-materialize, no-op edges). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017xGPjDBF8NTwTK7VQvizix Signed-off-by: ChrisBeWithYou <chris@rifflarr.local>
… PR4, step 4) PR4's persistence contract is "trim … save→reload yields an identical window (no start/len drift)." The plumbing already exists end-to-end — the frontend _trackSessionNormalizePure / _trackRegionsNormalizePure and the backend _coerce_track_regions both carry srcIn/srcOut (PR1/PR3) — but the round-trip tests only ever exercised a NOTATION window (startBeat/lenBeat). Nothing proved an AUDIO trim survives the full round-trip, so a future change to either serializer could silently drop or drift srcIn/srcOut and stay green. Pin it on both ends, with FRACTIONAL trim values to catch any rounding drift: - frontend (tests/track_regions.test.mjs): a trimmed audio region survives normalize → JSON wire → normalize identically (and is carried verbatim on save). - backend (tests/test_track_regions.py): the same region survives _coerce_track_session → YAML dump/load → _coerce_track_session identically. Both pass as-is (no product change needed — this closes the coverage gap, not a bug). Full JS 311 pass, pytest 382 pass, lint clean. Note: tombstoning an INDIVIDUALLY-removed audio region (so it can't resurrect on reload) is deferred with audio-region delete, which is still select-only. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017xGPjDBF8NTwTK7VQvizix Signed-off-by: ChrisBeWithYou <chris@rifflarr.local>
Wires _regionStartPure into playback: _startStemSources now places each non-active
audio source's track as one BufferSource PER REGION instead of one per stem. A
project with no authored regions has a single full-span region, so this is
byte-identical to the old whole-stem path (pinned: _audioRegionPlacementsPure ∘
_regionStartPure on the default region reproduces _audioBufferStartPure for every
cursor). A moved/trimmed region plays its media window [srcIn,srcOut) starting at
the group shift + timeOf(startBeat), so it rides the tempo map for placement while
its content plays at natural speed; a muted region is skipped.
- _audioRegionPlacementsPure(regions, bufferDuration, beatToTime) — pure: resolve
a track's regions[] (default → one [0,dur] at beat 0) into {startBeatTime, srcIn,
srcOut, muted}, clamping srcOut to the buffer and mapping startBeat via beatToTime.
- _trackRegionsForSourceId — the audio track (by sourceId) whose regions to place.
- playingStemSources is now sourceId → node[] (one per region); _stopStemSources
and _pruneStaleStems stop every node in each list.
Scope (phase 1, per TRACK-REGIONS-DESIGN "Flagged seams"): the ACTIVE/reference
source (S.audioSource) and the audition-slow single-stream path are unchanged —
region scheduling applies to the sample-accurate non-active sources. Declick
ramps + windowed waveform land in step 5.
Tests: tests/audio_shift.test.mjs (3 _audioRegionPlacementsPure cases incl. the
default-chain bit-identical proof). Full JS 311 pass, lint clean.
⚠️ Needs dogfood verification: multi-region + trimmed playback against real audio.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_017xGPjDBF8NTwTK7VQvizix
Signed-off-by: ChrisBeWithYou <chris@rifflarr.local>
…ep 5) Two edge treatments for trimmed audio regions, both keeping the untrimmed path byte-identical. Declick: a trimmed region (srcIn > 0 or srcOut < bufferDuration) now routes its BufferSource through a per-region gain that fades from silence over ~5 ms at its first sample and back over ~5 ms at its last, so a cut that doesn't land on a zero crossing doesn't pop. A full-span region has no gain node and connects straight to the stem gain, unchanged. The transient gain is released on the source's onended. _declickEnvelopePure builds the ramp points (fade-in only for an open window; fades shrink to duration/2 so they never overlap). Windowed waveform: the audio-lane thumbnail is drawn as one WINDOWED pass PER region — each region shows only its [srcIn,srcOut) media slice under its own block, placed at the group shift + its beat-0-relative start. The default full-span region resolves to the whole buffer at `shift`, so a region-free lane draws exactly as before. _regionWaveWindowPure computes each region's chart span + media window. Also hardened step 2's placement to be beat-0-relative (timeOf(startBeat) − timeOf(0)) so the default region contributes zero offset whatever the grid origin. Tests: tests/audio_shift.test.mjs (_declickEnvelopePure), tests/region_waveform.test.mjs (_regionWaveWindowPure incl. grid-origin independence). Full JS 312 pass, lint clean.⚠️ Needs dogfood verification: the declick is inaudible and trim/move waveforms track their blocks against real audio. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017xGPjDBF8NTwTK7VQvizix Signed-off-by: ChrisBeWithYou <chris@rifflarr.local>
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017xGPjDBF8NTwTK7VQvizix Signed-off-by: ChrisBeWithYou <chris@rifflarr.local>
cd27af2 to
79bbd61
Compare
1 participant
Completes Track Regions PR4 — a stem's audio becomes movable, trimmable blocks. Builds on the merged region model/render/place stack (#332–#334, #344, #349).
What it does
_startStemSourcesnow schedules oneBufferSourceper region instead of one per stem. A region plays only its media window[srcIn, srcOut)at its placed start (group shift + the region's beat-0-relative position), so a moved/trimmed audio block rides the tempo map for placement while its content plays at natural speed — never stretched. Muted regions are skipped.TrimRegionCmd— a window-only, undoable trim (audiosrcIn/srcOut, notationstartBeat/lenBeat); notes outside a narrowed notation window are hidden, never deleted._trackSessionNormalizePure) and backend (_coerce_track_session) with fractional values. (Plumbing already existed from PR1/PR3; this closed the audio-trim coverage gap.)[srcIn, srcOut)slice.The invariant
A project with no authored regions has a single full-span region and is byte-identical to before — playback, scheduling, and the waveform are unchanged. This is pinned:
_audioRegionPlacementsPure ∘ _regionStartPureon the default region reproduces_audioBufferStartPurefor every cursor.Phase-1 scope (per
TRACK-REGIONS-DESIGN.mdflagged seams)The active/reference source (
S.audioSource) and the audition-slow single-stream path stay whole-track; region scheduling applies to the sample-accurate non-active sources. Audio-region delete and split/join/loop are later tiers.Tests
New pure cores each unit-tested —
_regionStartPure,_audioRegionPlacementsPure,_declickEnvelopePure(tests/audio_shift.test.mjs),TrimRegionCmd(tests/region_trim.test.mjs), audio-trim round-trip (tests/track_regions.test.mjs+tests/test_track_regions.py),_regionWaveWindowPure(tests/region_waveform.test.mjs). Full JS 312 pass, pytest 382 pass, ESLint 0 errors.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes