fix(sound): play concurrently, add back play_in_tab - #874
Conversation
The Conductor migration accidentally serialized play()/play_wave() (each call now waited for the previous one to finish) and dropped play_in_tab() (and its tab UI) entirely, regressing two behaviours the sound module used to have: genuinely overlapping playback, and a tab showing per-sound play bars. - Removed the host-side playback queue in SoundTabPlugin so repeated/ looped play()/play_wave() calls start immediately and overlap, mixed by the shared AudioContext, instead of playing one after another. - Brought back play_in_tab(): it samples the Sound and encodes it as a WAV data URI (pure computation, done module-side rather than via a host round trip), which the sound tab renders as a native <audio controls> play bar. Each call adds a new bar, stacked vertically below any earlier ones, per the clarification on #841. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01B3LRoXRMQ5ADeYMdVTChCZ
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
|
@CodeRabbit review |
✅ Action performedReview finished.
|
WalkthroughThe sound module now supports concurrent ChangesSound playback and tab players
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant SoundModulePlugin
participant play_in_tab
participant SoundTabRpc
participant SoundTabPlugin
SoundModulePlugin->>play_in_tab: validate and convert Sound
play_in_tab->>play_in_tab: sample channels and encode WAV data URI
play_in_tab->>SoundTabRpc: addPlayerToTab(wavDataUri)
SoundTabRpc->>SoundTabPlugin: insert player entry
SoundTabPlugin-->>SoundTabRpc: resolve after insertion
SoundTabRpc-->>play_in_tab: resolve
play_in_tab-->>SoundModulePlugin: return Sound
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 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.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/tabs/Sound/src/index.tsx (1)
201-213: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winPrevent
__ensureAudioContext()from reusing a closedAudioContext.
destroy()closesthis.__audioContextwhen__destroyedis true and__pendingPlaybackCountis zero, but it does not resetthis.__audioContext.__ensureAudioContext()only creates a new context when!this.__audioContext, so any laterplaySamples()/stopRecording()call uses the closed context and fails or hangs. Treat a closedAudioContextas no longer usable and create a new one.🔧 Proposed fix
private __ensureAudioContext(): AudioContext { - if (!this.__audioContext) { + if (!this.__audioContext || this.__audioContext.state === 'closed') { this.__audioContext = new AudioContext(); } return this.__audioContext; }🤖 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/tabs/Sound/src/index.tsx` around lines 201 - 213, Update __ensureAudioContext() to detect when the existing AudioContext has been closed, treating it as unusable and creating a new context just as it does when __audioContext is absent. Preserve reuse of open contexts while ensuring later playSamples() or stopRecording() calls never operate on the closed context finalized by __maybeFinalizeDestroy().
🧹 Nitpick comments (2)
src/tabs/Sound/src/index.tsx (2)
61-88: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssociate the "Sound N" label with its
<audio>control for assistive tech.Currently the label is only a
<p>placed visually above the control; screen readers announce the<audio>element without the "Sound N" context since there is noaria-label/aria-labelledby/<label>association. Addaria-label(orid/aria-labelledby) so the control's accessible name matches its visible label.♿ Proposed fix
- <audio src={player.dataUri} controls style={{ width: '100%' }} /> + <audio src={player.dataUri} controls style={{ width: '100%' }} aria-label={`Sound ${index + 1}`} />🤖 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/tabs/Sound/src/index.tsx` around lines 61 - 88, Update PlayerBarsView so each audio control is programmatically associated with its visible “Sound N” label. Add a unique label identifier per player and reference it from the corresponding audio element via aria-labelledby, or provide the same visible text through aria-label while preserving the existing presentation.
116-134: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winBound the
__playerslist to avoid unbounded growth per Run.
__playersonly ever grows viaaddPlayerToTab()(Line 254) and is never trimmed until the wholeSoundTabPlugininstance is replaced by the next Run. Each entry retains a full base64 WAV data URI plus a rendered<audio>element. A loop that callsplay_in_tab()many times within one Run (a plausible pattern, e.g. adding a bar per note in a sequence) accumulates memory and DOM nodes with no upper bound for the life of that Run, which can noticeably degrade or freeze the tab for longer/larger sounds. Consider capping the list (e.g. keep the most recent N entries) or otherwise bounding growth.♻️ Proposed fix
+ private static readonly MAX_PLAYERS = 50; + async addPlayerToTab(wavDataUri: string): Promise<void> { - this.__players = [...this.__players, { id: this.__nextPlayerId, dataUri: wavDataUri }]; + const nextPlayers = [...this.__players, { id: this.__nextPlayerId, dataUri: wavDataUri }]; + this.__players = + nextPlayers.length > SoundTabPlugin.MAX_PLAYERS + ? nextPlayers.slice(nextPlayers.length - SoundTabPlugin.MAX_PLAYERS) + : nextPlayers; this.__nextPlayerId += 1; this.__emit(); }Also applies to: 253-257
🤖 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/tabs/Sound/src/index.tsx` around lines 116 - 134, Bound the per-run __players collection in addPlayerToTab so repeated play_in_tab() calls cannot grow it indefinitely. Keep only the most recent entries up to a defined maximum, removing older entries and their associated rendered audio resources as needed while preserving newest-entry playback 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.
Outside diff comments:
In `@src/tabs/Sound/src/index.tsx`:
- Around line 201-213: Update __ensureAudioContext() to detect when the existing
AudioContext has been closed, treating it as unusable and creating a new context
just as it does when __audioContext is absent. Preserve reuse of open contexts
while ensuring later playSamples() or stopRecording() calls never operate on the
closed context finalized by __maybeFinalizeDestroy().
---
Nitpick comments:
In `@src/tabs/Sound/src/index.tsx`:
- Around line 61-88: Update PlayerBarsView so each audio control is
programmatically associated with its visible “Sound N” label. Add a unique label
identifier per player and reference it from the corresponding audio element via
aria-labelledby, or provide the same visible text through aria-label while
preserving the existing presentation.
- Around line 116-134: Bound the per-run __players collection in addPlayerToTab
so repeated play_in_tab() calls cannot grow it indefinitely. Keep only the most
recent entries up to a defined maximum, removing older entries and their
associated rendered audio resources as needed while preserving newest-entry
playback behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: f210742f-f22a-40e4-986f-da9b8fab0c5b
📒 Files selected for processing (7)
src/bundles/sound/src/__tests__/sound.test.tssrc/bundles/sound/src/__tests__/utils.tssrc/bundles/sound/src/functions.tssrc/bundles/sound/src/index.tssrc/bundles/sound/src/protocol.tssrc/tabs/Sound/src/__tests__/index.test.tssrc/tabs/Sound/src/index.tsx
…ay-and-play-in-tab # Conflicts: # src/bundles/sound/src/functions.ts
- __ensureAudioContext() now treats a closed AudioContext as absent rather than reusing it: destroy() closes the context but never resets the field, so a playSamples() call still in flight when a Run ends (e.g. sampling was still running) could otherwise be handed back an already-closed, unusable context. - Cap __players to the most recent MAX_PLAYER_BARS entries so a loop calling play_in_tab() many times in one Run can't grow the list (each holding a full WAV data URI plus a rendered <audio> element) without bound for the tab's lifetime. - Associate each play bar's "Sound N" label with its <audio> control via aria-labelledby, so the control's accessible name matches what is shown on screen. Also updates play_in_tab() to use the shared sampleSound() helper (added by the just-merged #869) instead of directly sampling leftWave/rightWave, so it gets the same "sample the source once" fix play() already has for panned sounds - this one only surfaced once resolving this PR's conflict with master pulled sampleSound() in. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012Dv6WCgTvMVbCJmyRUY9pL
|
Resolved the merge conflict with Conflict resolution ( CodeRabbit's 3 findings, all still valid against current code, now fixed:
Added 2 regression tests covering the AudioContext-reuse fix and the player-list cap, plus a Verified locally:
Pushed as two commits: the merge itself, then a separate |
There was a problem hiding this comment.
I have live tested with the following code snippets :
1. play_in_tab — basic: adds a play bar without playing immediately
from sound import sine_sound, play_in_tab
play_in_tab(sine_sound(440, 2))A "Sound 1" bar appears with native controls.
2. play_in_tab — repeated calls stack vertically
from sound import sine_sound, play_in_tab
play_in_tab(sine_sound(440, 1))
play_in_tab(sine_sound(660, 1))
play_in_tab(sine_sound(880, 1))Three bars, "Sound 1"/"Sound 2"/"Sound 3", stacked top to bottom.
3. play_in_tab — zero duration is a silent no-op
from sound import sine_sound, play_in_tab
play_in_tab(sine_sound(440, 0))No error, no new bar. Silently no-ops.
4. play_in_tab — error cases
from sound import sine_sound, play_in_tab
play_in_tab(sine_sound(440, -1)) # "Sound duration must be a finite number greater than or equal to 0"from sound import play_in_tab
play_in_tab(42) # "Expected a Sound for sound, got 42."5. Concurrent play() — the main regression this PR fixes
from sound import sine_sound, play
play(sine_sound(440, 3))
play(sine_sound(660, 3))
play(sine_sound(880, 3))All three tones overlap as a chord for ~3s, instead of playing one after another.
The Feedback I received from live-testing (MAJOR point no 2) along with AI review
-
play_in_tab() has no cap on accumulated bars : memory/DOM growth risk
SoundTabPlugin.addPlayerToTab() (index.tsx:253-257) just appends forever: this.__players = [...this.__players, {...}]. Each entry holds a full base64 WAV string, rendered as its own element. A student loop like for i in range(1000): play_in_tab(sine_sound(440, 1)) — easy to write by accident, e.g. off-by-one in a range, will keep growing that array and the DOM with no limit, unlike play()/stop() which have bounded state. Worth deciding: cap it, evict oldest, or is unbounded genuinely fine for the expected use case? -
No "constructing…" status while play_in_tab() samples a long sound
play() explicitly calls io().notifyConstructing() before sampling starts, specifically so the tab shows "Constructing…" instead of looking stalled during a long sample (there's a comment saying exactly this). play_in_tab() skips that call entirely, soplay_in_tab(sine_sound(440, 30))will show nothing happening for however long sampling takes, then a bar just appears. -
No test coverage for the actual rendered UI (PlayerBarsView)All the new tests hit addPlayerToTab/__players (the plugin's data layer), but nothing renders PlayerBarsView and asserts N bars appear in the right order — the part of the feature the issue actually asked for ("multiple bars arranged vertically") is untested at the rendering layer.
-
(minor/optional) play_in_tab() duplicates play()'s validation block almost verbatim
Same is_sound check + duration < 0/=== 0 handling copy-pasted. More a maintainability nit than a bug — not really "postable" as an issue on its own, but worth mentioning if you're filing a cleanup-oriented comment.
- play_in_tab() now calls notifyConstructing() before sampling, like play() already does, so a long Sound shows "Constructing…" instead of looking stalled until the bar appears. addPlayerToTab() is the corresponding "done" signal (decrementing __constructingCount and recomputing status), matching how playSamples() does it for play(). - Deduplicated play()/play_in_tab()'s near-identical Sound argument validation into a shared assertPlayableSound() helper. - Added browser-mode rendering tests for PlayerBarsView (the actual "multiple bars stacked vertically" UI the issue asked for), which previously only had coverage at the addPlayerToTab() data layer. Exported PlayerBarsView so it's directly renderable in tests, and brought in the same vitest-browser-react/@vitest/browser-playwright setup already used by the Rune/Matrix/Curve tabs. The player-list cap Akshay's review also flagged (point 1) was already fixed in the previous commit before the review landed. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012Dv6WCgTvMVbCJmyRUY9pL
The previous commit's git add silently dropped these files (one bad pathspec in a multi-path invocation aborts the whole add) - this is the rest of that change: - assertPlayableSound() dedup in functions.ts, and play_in_tab()'s new notifyConstructing() call - addPlayerToTab()'s matching __constructingCount decrement - the PlayerBarsView rendering tests and their vitest-browser-react/ browser-playwright devDependencies - 2 new bundle-level tests (notifyConstructing ordering) and 1 new tab-level test (constructing status closes out via addPlayerToTab) Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012Dv6WCgTvMVbCJmyRUY9pL
…tion play_in_tab() calls A zero-duration Sound is a valid neutral element (for consecutively()/ simultaneously(), e.g. as a reduce() starting value), not an error - but play_in_tab() previously just silently added nothing to the tab for one, which could read as the call having been dropped. Adds a new addZeroDurationPlayerToTab() RPC method (deliberately separate from addPlayerToTab(), not a variant of it): no sampling happens for a zero-duration Sound, so play_in_tab() never calls notifyConstructing() for one either, and folding this into addPlayerToTab() would incorrectly decrement __constructingCount on behalf of a call that never incremented it - which could cancel out an unrelated, genuinely concurrent play_in_tab() call's still-in- flight notifyConstructing(). PlayerBarEntry becomes a discriminated union so PlayerBarsView can render a plain "zero duration sound" placeholder line in the entry's call-order position, instead of a native <audio> control. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012Dv6WCgTvMVbCJmyRUY9pL
|
Per the discussion above: Implementation: added
Added tests on both sides: bundle-level ( Verified locally: |
2 participants
Summary
Fixes #841. The Conductor migration of the
soundmodule regressed two behaviours from before the migration:play()/play_wave()calls used to overlap (genuine concurrent playback); after the migration they silently serialized instead (each call waited for the previous one to finish before starting).play_in_tab()(and its tab UI, a play bar per call) was dropped entirely.This PR restores both:
SoundTabPlugin) playback queue, so repeated/loopedplay()/play_wave()calls now start as soon as their own sampling finishes and are mixed together by the sharedAudioContext, instead of queueing behind whatever's already playing.play_in_tab(): brought back as a new exported function. It samples the givenSoundand encodes it as adata:audio/wav;base64,...URI (pure computation — noAudioContextneeded, so this happens module-side rather than via a host round trip), which the sound tab renders as a native<audio controls>play bar. Per the clarification on the issue, each call adds a new bar to the existing sound tab, stacked vertically below any earlier ones (using the browser's own play/pause/scrub controls, rather than a custom widget).Test plan
yarn testpasses for bothsrc/bundles/sound(69 tests, including newplay_in_tabcoverage) andsrc/tabs/Sound(22 tests, including new concurrency +addPlayerToTabcoverage)yarn tscandyarn lintpass for both packages (no new lint errors; one neweslint-disable-next-linewarning matches the existing pattern already present for every other validated function in the file)yarn buildsucceeds for both packages🤖 Generated with Claude Code
https://claude.ai/code/session_01B3LRoXRMQ5ADeYMdVTChCZ