fix(audio): async decode playback edge cases#186
Merged
Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (5)
📝 WalkthroughWalkthroughAdds pending-decode tracking for cold playback, enforces decode ownership and library membership before applying results, clears stale decode state on stop and rescan, updates playback tests, and extracts ChangesAsync decode edge-case fixes
AudioHandle module extraction
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant UI
participant HonkHonk
participant DecodeTask
participant NowPlaying
UI->>HonkHonk: request_play(sound)
HonkHonk->>HonkHonk: set playing optimistically
HonkHonk->>DecodeTask: queue or update PendingDecode
DecodeTask-->>HonkHonk: Decoded / Err
alt decode succeeds and sound exists
HonkHonk->>NowPlaying: remove stale envelope
HonkHonk->>HonkHonk: cache PCM and start playback
else decode fails or sound removed
HonkHonk->>HonkHonk: clear owned optimistic UI
end
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
#152 deferred the cold-press highlight until decode succeeded, so the first press of every sound in a session showed no feedback until decode completed — against the snappy-UI doctrine (#111: keep the optimistic instant highlight, never slow the click->play path). Restore it: a cold cache miss claims `playing` and the pending now-playing state immediately, then the async decode confirms the playhead on success. A decode failure, or a sound removed mid-decode, now releases the optimistic highlight through a shared owns-the-UI guard, so it is never left stuck on a sound that produced no audio. The generation/terminal-event invariant is preserved: a superseded older press never retakes a newer press's highlight. Reviving the optimistic path also un-orphans now_playing::pending(), which #152 had left as dead code. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…undary testable Two review follow-ups for #152, plus the test infrastructure they needed. - reconcile_playback_with_library now sends StopVoice for the highlighted sound when its library entry vanishes mid-play, instead of only clearing the UI and leaving the engine to honk the removed sound to completion ("ghost honk"). The highlighted voice id is the current play_generation, since only the owning press holds the highlight. - Add a test-only command tap to AudioHandle (the pipewire command channel can't be drained synchronously, so no test could previously assert what reached the engine). Pin the core invariant the PR is about: a cold press fires exactly one Play when its decode lands, and a decode landing after StopAll is stale and fires none. - Extract AudioHandle + test_handle out of the over-budget engine.rs (927 lines) into a focused handle.rs (78), so the tap lands in a compliant file and engine.rs shrinks rather than grows. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/ui/now_playing.rs (1)
101-104: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueConsider guarding against evicting the currently displayed waveform.
remove_envelopeunconditionally deletes the cached envelope. If the PCM-cache LRU eviction victim happens to be the sound currently shown viaactive_id(e.g., still playing), the waveform would blank out mid-playback until the sound's envelope is regenerated on a future decode. Existing tests only cover eviction of a non-active sound (pcm_eviction_removes_matching_waveform_envelope), not the currently-playing case.This is a narrow, self-recovering visual glitch rather than a functional break, but worth a quick check on whether the eviction call site (
evict_waveform_envelopesinsrc/app/playback.rs) can ever target the active sound given the AudioStore's LRU recency semantics.💡 Optional defensive guard
pub(crate) fn remove_envelope(&mut self, id: &str) { - self.envelopes.remove(id); + if self.active_id.as_deref() != Some(id) { + self.envelopes.remove(id); + } }🤖 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/ui/now_playing.rs` around lines 101 - 104, Guard the waveform cache eviction path so the currently displayed/playing envelope is not removed. Update remove_envelope in NowPlaying to check against the active_id state (or add the protection at evict_waveform_envelopes in playback handling) before deleting from envelopes, so the active sound keeps its waveform visible until playback changes. Keep the existing non-active eviction behavior intact and verify the PCM eviction path cannot blank the waveform for the active item.
🤖 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/ui/now_playing.rs`:
- Around line 101-104: Guard the waveform cache eviction path so the currently
displayed/playing envelope is not removed. Update remove_envelope in NowPlaying
to check against the active_id state (or add the protection at
evict_waveform_envelopes in playback handling) before deleting from envelopes,
so the active sound keeps its waveform visible until playback changes. Keep the
existing non-active eviction behavior intact and verify the PCM eviction path
cannot blank the waveform for the active item.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 17d76f7c-326f-45d7-9ee5-71028fda152e
📒 Files selected for processing (8)
src/app/mod.rssrc/app/playback.rssrc/app/playback/tests.rssrc/app/recording.rssrc/audio/engine.rssrc/audio/handle.rssrc/audio/mod.rssrc/ui/now_playing.rs
Fixes the CI lint (cargo fmt --check) failure on this branch. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
A decode cancelled by StopAll/interrupt shares its sound id with a re-press's fresh pending entry. Landing first, it consumed that entry: a stale error cleared the new press's optimistic highlight and the real decode result was then dropped on arrival, so the sound never played. Pending entries are now consumed only by the task that owns them (matching voice id); stale landings fall through and are dropped. Codex review finding [P2] on PR #186. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
A late decode evicting the active sound's PCM under cache pressure also removed its waveform envelope, blanking the visible waveform mid-play even though the engine keeps playing from its own Arc. The active sound's envelope is now exempt from eviction. Codex review finding [P3] / CodeRabbit nitpick on PR #186. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Collaborator
Author
|
Second-reviewer pass (Codex GPT-5.5, adversarial) + CodeRabbit nitpick, all addressed:
Verified locally: fmt clean, clippy -D warnings clean, full test suite green (hermetic XDG override). |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/app/playback/tests.rs (1)
236-245: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePrefer captured generation values over hardcoded literals.
dispatch(&app, 2)/dispatch(&app, 1)hardcode the expected generation counter values, whereas the sibling test below (stale_same_id_decode_does_not_consume_new_pending_decode) capturesapp.play_generationright after each press. Hardcoding is fragile if the generation counter's starting value or increment behavior changes later.♻️ Suggested tweak
let _ = app.request_play(&a, false); + let gen_a = app.play_generation; let _ = app.request_play(&b, false); - let _ = app.handle_decoded("b".into(), Ok(pcm(4)), dispatch(&app, 2)); + let gen_b = app.play_generation; + let _ = app.handle_decoded("b".into(), Ok(pcm(4)), dispatch(&app, gen_b)); assert!(app.now_playing.envelope("b").is_some()); // The late older decode for A lands and evicts B's PCM under cache // pressure while B is still the active now-playing sound. - let _ = app.handle_decoded("a".into(), Ok(pcm(8)), dispatch(&app, 1)); + let _ = app.handle_decoded("a".into(), Ok(pcm(8)), dispatch(&app, gen_a));🤖 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/app/playback/tests.rs` around lines 236 - 245, The concurrent playback test uses hardcoded generation values in dispatch calls, which should instead be captured from the app state like the sibling test does. Update the test around request_play and handle_decoded to read the current play generation after each press and pass those captured values into dispatch, using the existing app.play_generation and dispatch helpers to keep the assertions resilient to generation counter changes.
🤖 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/app/playback/tests.rs`:
- Around line 228-298: The test module is growing past the 400-line limit, so
split the long `src/app/playback/tests.rs` file before adding more cases. Move
the new coverage out of the current
`pcm_eviction_keeps_active_waveform_envelope` and
`stale_same_id_decode_does_not_consume_new_pending_decode` area into a separate,
scenario-focused test file or module (for example, grouping decode ownership and
eviction tests) and keep `tests.rs` as a smaller entry point.
---
Nitpick comments:
In `@src/app/playback/tests.rs`:
- Around line 236-245: The concurrent playback test uses hardcoded generation
values in dispatch calls, which should instead be captured from the app state
like the sibling test does. Update the test around request_play and
handle_decoded to read the current play generation after each press and pass
those captured values into dispatch, using the existing app.play_generation and
dispatch helpers to keep the assertions resilient to generation counter changes.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 8c8f816d-01fe-4c4c-8cb5-e4c71cf99620
📒 Files selected for processing (4)
src/app/playback.rssrc/app/playback/tests.rssrc/audio/mod.rssrc/ui/now_playing.rs
🚧 Files skipped from review as they are similar to previous changes (3)
- src/ui/now_playing.rs
- src/audio/mod.rs
- src/app/playback.rs
Both advisories are DoS-via-untrusted-XML in quick-xml < 0.41. The sole consumer is wayland-scanner, a build-time proc-macro parsing vendored Wayland protocol XML; no untrusted XML is parsed at runtime, so neither issue is reachable in the shipped binary. wayland-scanner 0.31.10 (latest) still pins quick-xml ^0.39 — drop these ignores when it moves to 0.41. Closes #192. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
anyhow 1.0.102 is flagged unsound (Error::downcast_mut UB after Error::context); 1.0.103 is the patched release already on main. Regenerates the Flatpak cargo-sources.json for the lock change. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
tests.rs hit 465 lines after the decode-ownership regressions landed. Fixtures move to test_support.rs; warm-path, eviction, and library- reconciliation tests move to cache_tests.rs; decode-ownership lifecycle tests stay in tests.rs. No test behavior changes. Addresses CodeRabbit review on PR #186. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…rals Addresses CodeRabbit nitpick on PR #186; keeps the concurrent-decode tests resilient to generation-counter changes. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Collaborator
Author
|
Follow-up items from the latest CodeRabbit pass, both addressed:
Also bumped |
…opped) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
# Conflicts: # deny.toml
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Design decisions
Validation
Closes #152
Summary by CodeRabbit