Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds in-thread fade state and time-stepped volume ramping; playback commands (Play, Prepared, Pause, Resume, Stop, SetVolume) now create, update, or cancel ActiveFade actions and the audio loop shortens its recv timeout while a fade is active. Changes
Sequence DiagramsequenceDiagram
participant Queue as Command Queue
participant Thread as Audio Thread
participant Sink as Audio Sink
participant State as Playback State
rect rgba(120,180,240,0.5)
Queue->>Thread: AudioCommand::Play / Prepared / Pause / Resume / Stop / SetVolume
Thread->>Thread: Create / Cancel / Update ActiveFade (target, steps)
Thread->>State: Emit immediate state changes (e.g., Playing)
end
rect rgba(180,200,140,0.5)
loop Main loop while ActiveFade exists
Thread->>Thread: Compute next ramped volume
Thread->>Sink: Set volume to ramped value
Thread->>Thread: Check fade completion
end
end
rect rgba(200,140,180,0.5)
Note over Thread,Sink: On fade completion -> perform final action
Thread->>Sink: pause()/stop() or clear fade
Thread->>State: Emit final state (Paused/Stopped/Idle)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
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.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src-tauri/src/audio/engine.rs (1)
398-402:⚠️ Potential issue | 🟠 MajorKeep paused slider updates off the live sink.
Line 401 writes the slider value into the paused sink itself. If the user changes volume while paused, Line 368 reads that full value on
Resumeand playback restarts at target volume instead of fading back in. WhilePlaybackState::Paused, update only the shared target and defers.set_volume(v)until playback resumes.Minimal fix
AudioCommand::SetVolume(v) => { *volume.write().unwrap() = v; - if let Some(ref s) = sink { - s.set_volume(v); + if *state.read().unwrap() != PlaybackState::Paused { + if let Some(ref s) = sink { + s.set_volume(v); + } } emit_state(&app, &state, &position_ms, &duration_ms, &volume); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src-tauri/src/audio/engine.rs` around lines 398 - 402, When handling AudioCommand::SetVolume, avoid calling s.set_volume(v) when playback is paused; instead only update the shared target volume (the volume RwLock) so Resume will apply it and trigger the fade-in logic. Concretely, in the AudioCommand::SetVolume branch (where volume.write().unwrap() is assigned and sink is referenced), add a check of the current playback state (PlaybackState::Paused) and skip calling s.set_volume(v) if paused—leave updating the shared volume value intact and let the Resume handling code perform the actual sink.set_volume and fade behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src-tauri/src/audio/engine.rs`:
- Around line 291-318: The new sink can emit audio before the Prepared branch
mutes it, so update start_streaming() to initialize the freshly created sink
muted: call set_volume(0.0) on the sink before calling append(...) or
set_volume(volume) there, and remove/replace any immediate set_volume(volume)
calls in start_streaming() so the fade-in spawned in the Prepared branch (the
task created for new_sink_raw that ramps volume over steps) is the sole owner of
the volume transition; reference start_streaming(), the Prepared branch handling
new_sink_raw/current_fade, and the session_id/current_session logic to ensure
the sink is muted immediately to avoid leaking audio from stale sessions.
---
Outside diff comments:
In `@src-tauri/src/audio/engine.rs`:
- Around line 398-402: When handling AudioCommand::SetVolume, avoid calling
s.set_volume(v) when playback is paused; instead only update the shared target
volume (the volume RwLock) so Resume will apply it and trigger the fade-in
logic. Concretely, in the AudioCommand::SetVolume branch (where
volume.write().unwrap() is assigned and sink is referenced), add a check of the
current playback state (PlaybackState::Paused) and skip calling s.set_volume(v)
if paused—leave updating the shared volume value intact and let the Resume
handling code perform the actual sink.set_volume and fade behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 22e9d02e-97f8-42eb-8d94-cb20c8afe11a
📒 Files selected for processing (1)
src-tauri/src/audio/engine.rs
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src-tauri/src/audio/engine.rs (1)
379-397:⚠️ Potential issue | 🔴 Critical
Stopdoes not invalidate the active load session.Line 394 marks playback as stopped, but
current_sessionis unchanged. If the user stops while a track is still buffering/downloading, the laterPreparedcheck at Line 294 still accepts that session, so playback can restart after the stop. The same gap also lets a canceled load surfaceLoadFailedlater. Please invalidate the session onStopso any in-flight loader result is rejected as stale.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src-tauri/src/audio/engine.rs` around lines 379 - 397, In the AudioCommand::Stop branch, also invalidate the in-flight load session so later loader results are rejected as stale: clear or advance current_session (e.g., set current_session = None or bump the session token used by Prepared/session checks) at the end of the Stop handling after aborting current_fade and before/when setting PlaybackState::Stopped; ensure this uses the same session identifier that the Prepared acceptance logic compares so any pending Prepared/LoadFailed callbacks will no longer match the active session.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src-tauri/src/audio/engine.rs`:
- Around line 342-355: The code sets PlaybackState::Paused before the fade task
actually calls s.pause(), which can leave an emptied sink in Paused state and
prevent end-of-track detection; change the logic so the state remains
PlaybackState::Playing (or introduce a transient PlaybackState::Pausing) until
the fade background task completes and calls s.pause(), then update the shared
state to Paused inside that task after s.pause() finishes; update references to
current_fade and the spawn closure (the async block that reads start_vol, loops,
and calls s.pause()) to perform the state transition there so the empty-sink
check that only treats PlaybackState::Playing as finishable will still observe
Playing until pause truly occurs.
---
Outside diff comments:
In `@src-tauri/src/audio/engine.rs`:
- Around line 379-397: In the AudioCommand::Stop branch, also invalidate the
in-flight load session so later loader results are rejected as stale: clear or
advance current_session (e.g., set current_session = None or bump the session
token used by Prepared/session checks) at the end of the Stop handling after
aborting current_fade and before/when setting PlaybackState::Stopped; ensure
this uses the same session identifier that the Prepared acceptance logic
compares so any pending Prepared/LoadFailed callbacks will no longer match the
active session.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: c4bc73bb-0dab-443c-b17d-8336aac9fcb3
📒 Files selected for processing (1)
src-tauri/src/audio/engine.rs
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src-tauri/src/audio/engine.rs (1)
540-547: Consider removing unused_volumeparameter.The
_volumeparameter (line 544) is no longer used since the sink is now always initialized at0.0(line 717). If no other callers depend on this signature, consider removing it to reduce API surface.♻️ Proposed cleanup
fn start_streaming( video_id: &str, state: &Arc<RwLock<PlaybackState>>, stream_handle: &rodio::OutputStreamHandle, - _volume: f32, eq_settings: &Arc<RwLock<EqSettings>>, app: &tauri::AppHandle, ) -> Result<Sink, crate::error::AppError> {And update the call site at line 264-271:
match start_streaming( &video_id_clone, &state_clone, &stream_handle_clone, - vol, &eq_settings_clone, &app_clone, ) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src-tauri/src/audio/engine.rs` around lines 540 - 547, The start_streaming function currently has an unused parameter `_volume`; remove `_volume` from the function signature of start_streaming and from all call sites (e.g., where start_streaming is invoked) so the API matches the implementation that initializes the sink volume to 0.0; update any references or tests that pass that argument to no longer provide it and run a build to ensure no remaining uses exist.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src-tauri/src/audio/engine.rs`:
- Around line 341-359: The pause fade task can complete after being aborted and
overwrite state to PlaybackState::Paused; fix by adding a shared cancellation
flag and checking it inside the spawned task: create an Arc<AtomicBool> (e.g.
fade_cancelled) tied to current_fade and set it to false when spawning in
AudioCommand::Pause, set it to true when aborting current_fade (as done by the
track-ended logic), clone the Arc into the tauri::async_runtime::spawn closure
and check fade_cancelled.load(Ordering::SeqCst) immediately before calling
s.pause() and again before writing *state_clone.write().unwrap() =
PlaybackState::Paused so the task bails without mutating state if cancelled;
ensure current_fade stores both the JoinHandle and its cancellation Arc so other
code (track-ended path) can set the flag before aborting.
---
Nitpick comments:
In `@src-tauri/src/audio/engine.rs`:
- Around line 540-547: The start_streaming function currently has an unused
parameter `_volume`; remove `_volume` from the function signature of
start_streaming and from all call sites (e.g., where start_streaming is invoked)
so the API matches the implementation that initializes the sink volume to 0.0;
update any references or tests that pass that argument to no longer provide it
and run a build to ensure no remaining uses exist.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 40473064-3887-46d4-a275-c756efaf7b95
📒 Files selected for processing (1)
src-tauri/src/audio/engine.rs
…ess and correct logic
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src-tauri/src/audio/engine.rs (1)
341-361:⚠️ Potential issue | 🟠 MajorRace condition between pause fade and track completion remains.
The past review identified that
JoinHandle::abort()only cancels at the next.awaitpoint. If the pause fade task has completed its finalsleep().awaitand is about to executes.pause()and writePlaybackState::Paused, the abort won't prevent it. This can leave the engine inPausedstate after track-ended already set it toIdleand stopped the sink.The suggested mitigation was an
Arc<AtomicBool>cancellation flag checked befores.pause()and the state write.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src-tauri/src/audio/engine.rs` around lines 341 - 361, The pause fade task can still run s.pause() and set PlaybackState::Paused after it was aborted, causing a race with track completion; to fix, introduce an Arc<AtomicBool> cancel flag that is created when spawning the fade (e.g. alongside current_fade), clone it into the spawned task and have the task check flag.load(Ordering::SeqCst) immediately before calling s.pause() and before writing PlaybackState::Paused, and when you cancel the fade (where you previously call current_fade.take().abort()) set the flag to true (and then abort the JoinHandle) so the aborted task will skip pausing and state mutation; update references to current_fade, the spawned closure, s.pause(), and state writes accordingly.
🧹 Nitpick comments (1)
src-tauri/src/audio/state.rs (1)
10-10: Consider serde representation forPausing.The
Displayimpl correctly mapsPausingto"paused"for frontend events (viato_string()). However, serde'srename_all = "snake_case"will serializePausingas"pausing". If thePlaybackStateis ever serialized directly (e.g., in logs, persistence, or new APIs), this would expose the transient state differently than the Display output.If intentional, this is fine. If not, consider adding
#[serde(rename = "paused")]toPausingfor consistency.Also applies to: 23-24
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src-tauri/src/audio/state.rs` at line 10, The serde representation for the PlaybackState enum is inconsistent with its Display mapping: add #[serde(rename = "paused")] to the Pausing variant (and likewise add explicit #[serde(rename = "...")] to any other variants mentioned at lines 23-24 that need to match Display) so that serializing PlaybackState yields the same string values as Display::to_string(); locate the enum PlaybackState and annotate the Pausing variant (and the other referenced variants) with the appropriate #[serde(rename = "...")] attributes to ensure consistency.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src-tauri/src/audio/engine.rs`:
- Around line 341-361: The pause fade task can still run s.pause() and set
PlaybackState::Paused after it was aborted, causing a race with track
completion; to fix, introduce an Arc<AtomicBool> cancel flag that is created
when spawning the fade (e.g. alongside current_fade), clone it into the spawned
task and have the task check flag.load(Ordering::SeqCst) immediately before
calling s.pause() and before writing PlaybackState::Paused, and when you cancel
the fade (where you previously call current_fade.take().abort()) set the flag to
true (and then abort the JoinHandle) so the aborted task will skip pausing and
state mutation; update references to current_fade, the spawned closure,
s.pause(), and state writes accordingly.
---
Nitpick comments:
In `@src-tauri/src/audio/state.rs`:
- Line 10: The serde representation for the PlaybackState enum is inconsistent
with its Display mapping: add #[serde(rename = "paused")] to the Pausing variant
(and likewise add explicit #[serde(rename = "...")] to any other variants
mentioned at lines 23-24 that need to match Display) so that serializing
PlaybackState yields the same string values as Display::to_string(); locate the
enum PlaybackState and annotate the Pausing variant (and the other referenced
variants) with the appropriate #[serde(rename = "...")] attributes to ensure
consistency.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: fb9566c4-5e29-4055-ab07-bf26c213b441
📒 Files selected for processing (2)
src-tauri/src/audio/engine.rssrc-tauri/src/audio/state.rs
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src-tauri/src/audio/engine.rs (1)
351-360:⚠️ Potential issue | 🟠 MajorGuard the pause fade completion path against stale-task state overwrite.
Line 359 writes
Pausedunconditionally after async sleeps. Sinceabort()is cooperative, a stale task can still reach this block and override newer states (e.g.,Playing,Stopped,Idle) after Resume/Stop/track-end handling.🔧 Proposed fix
let state_clone = state.clone(); current_fade = Some(tauri::async_runtime::spawn(async move { let start_vol = s.volume(); let steps = 10; for i in (0..steps).rev() { s.set_volume(start_vol * (i as f32 / steps as f32)); tokio::time::sleep(Duration::from_millis(10)).await; } - s.pause(); - *state_clone.write().unwrap() = PlaybackState::Paused; + let mut st = state_clone.write().unwrap(); + if *st != PlaybackState::Pausing { + return; // stale/cancelled fade; do not mutate sink/state + } + s.pause(); + *st = PlaybackState::Paused; }));Also applies to: 517-519
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src-tauri/src/audio/engine.rs` around lines 351 - 360, The fade task unconditionally sets *state_clone.write().unwrap() = PlaybackState::Paused, allowing a stale/aborted-but-still-running task to overwrite newer states; fix by adding a monotonically-incremented fade token in the shared state and capturing that token in the closure when spawning the fade (i.e., increment a fade_counter in the same place you set current_fade), then before calling s.pause() and writing PlaybackState::Paused check that the captured token still equals the current fade token in state_clone; only then perform s.pause() and set the state to Paused (apply the same token-guard pattern to the other fade at lines ~517-519).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src-tauri/src/audio/engine.rs`:
- Around line 351-360: The fade task unconditionally sets
*state_clone.write().unwrap() = PlaybackState::Paused, allowing a
stale/aborted-but-still-running task to overwrite newer states; fix by adding a
monotonically-incremented fade token in the shared state and capturing that
token in the closure when spawning the fade (i.e., increment a fade_counter in
the same place you set current_fade), then before calling s.pause() and writing
PlaybackState::Paused check that the captured token still equals the current
fade token in state_clone; only then perform s.pause() and set the state to
Paused (apply the same token-guard pattern to the other fade at lines ~517-519).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 9bdf78d6-fda2-44e7-8e3b-4359287191db
📒 Files selected for processing (1)
src-tauri/src/audio/engine.rs
FrogSnot
left a comment
FrogSnot
left a comment
There was a problem hiding this comment.
I really love this feature. Audio fades eliminate pops and improve UX. However, the current implementation has several significant issues that need to be addressed before merging.
Arc<Sink>is unsound
rodio::Sink is !Sync it makes no thread-safety guarantees for concurrent access. Wrapping it in Arc and calling .set_volume() / .stop() from spawned tokio tasks while the audio thread also accesses it is a data race. This compiles because Arc<T: Send> allows moving ownership, but concurrent access from multiple threads without Sync is undefined behavior.
Keep Sink on the audio thread only. Instead of spawning async tasks that touch the sink directly, run the fade loop inline on the audio thread using the existing recv_timeout loop. Between command polls, step the fade volume. This keeps all sink access single-threaded.
- Track overlap on skip. Fade-out does not block
When AudioCommand::Play fires for a new track, the old sink gets a 75ms fade-out spawned as fire-and-forget (tauri::async_runtime::spawn). But the new track starts streaming immediately without waiting. This creates ~75ms of two tracks playing simultaneously. The opposite of eliminating pops.
Either do the fade-out synchronously (blocking the audio thread briefly), or wait for the fade task to complete before starting the new track.
recv_timeoutreduced from 50ms to 10ms. 5x CPU increase
The audio thread's poll interval was changed from 50ms to 10ms, causing 100 wake-ups/second instead of 20. This is constant overhead even when idle. The fade tasks are async and don't depend on the poll rate.
Restore recv_timeout(Duration::from_millis(50)). If you adopt the inline fade approach from fix 1, you can temporarily use a shorter timeout only during active fades.
Pausingstate leaks into track-end detection
matches!(*state, PlaybackState::Playing | PlaybackState::Pausing)If a track reaches its end during a pause fade, the track-end logic triggers and auto-advances. A pausing track should not auto-advance.
Only check for PlaybackState::Playing in track-end detection, not Pausing.
use tokio;bare import does nothing
Line use tokio; is unnecessary. tokio::time::sleep is called via full path. Remove the bare import.
- Fade-out on Stop. State vs audio desync
Stop sets state to Stopped immediately, but the fade-out runs for 150ms after. The frontend sees "stopped" while audio is still audible.
Either fade synchronously before updating state, or introduce a Stopping transitional state (similar to Pausing).
The cleanest fix for issues 1-4 is to run fades inline on the audio thread instead of spawning async tasks:
// Pseudocode for inline fade
let mut fade: Option<FadeState> = None;
loop {
let timeout = if fade.is_some() { 10 } else { 50 };
let first = rx.recv_timeout(Duration::from_millis(timeout));
// Step any active fade
if let Some(ref mut f) = fade {
if f.step(&sink, &volume) {
fade = None; // fade complete
}
}
// Process commands...
}This keeps all Sink access on a single thread (no Arc needed), naturally blocks new-track start until fade-out completes, and only uses fast polling during active fades.
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src-tauri/src/audio/engine.rs (1)
343-356:⚠️ Potential issue | 🟡 MinorTrack-end detection may be skipped during the pause fade window.
Setting state to
Pausedimmediately (Line 346) means the track-end check at Line 509 (state == PlaybackState::Playing) won't trigger during the ~100ms fade. If the track's final samples drain during this window, the player remains paused on an empty sink without emittingtrack-finished.While the probability is low (requires track to end precisely during the fade), consider adding the
Pausedstate to the track-end condition, or deferring the state change until afters.pause():🛡️ Option A: Extend track-end check to include Paused state
- } else if *state.read().unwrap() == PlaybackState::Playing { + } else { + let st = *state.read().unwrap(); + if st == PlaybackState::Playing || st == PlaybackState::Paused { eprintln!("[sunder] track finished"); track_ended = true; + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src-tauri/src/audio/engine.rs` around lines 343 - 356, The pause handler sets PlaybackState::Paused immediately (AudioCommand::Pause) which can prevent the track-end detection logic (the check that compares state to PlaybackState::Playing) from firing during the short active_fade window; either include Paused in that track-end condition so finished tracks are still detected while fading, or defer changing state to PlaybackState::Paused until after the fade and actual s.pause() completes and then call emit_state(&app, &state, &position_ms, &duration_ms, &volume); — update the AudioCommand::Pause branch (where active_fade is created and emit_state is called) and/or the track-end detection predicate to reference PlaybackState::Paused so the player emits track-finished correctly.
🧹 Nitpick comments (2)
src-tauri/src/audio/engine.rs (2)
390-400: Volume change during fade causes abrupt jump.When
SetVolumeis called during an active fade-in (e.g., afterResume), Lines 396-397 immediately set the sink volume and clear the fade. This can cause an audible pop if the fade was mid-transition.Consider updating the fade's
target_volinstead of cancelling the fade, allowing the transition to continue smoothly toward the new target:♻️ Smoother volume adjustment during fade
AudioCommand::SetVolume(v) => { *volume.write().unwrap() = v; if let Some(ref s) = sink { let st = state.read().unwrap().clone(); if st != PlaybackState::Paused { - s.set_volume(v); - active_fade = None; + if let Some(ref mut f) = active_fade { + // Update target instead of cancelling + f.target_vol = v; + } else { + s.set_volume(v); + } } } emit_state(&app, &state, &position_ms, &duration_ms, &volume); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src-tauri/src/audio/engine.rs` around lines 390 - 400, When handling AudioCommand::SetVolume in the SetVolume branch, avoid cancelling an in-progress fade by overwriting active_fade; instead, if an active_fade exists update its target_vol to the new v so the current ramp continues smoothly toward the new level, and only call s.set_volume(v) and clear active_fade when there is no active_fade (or when you explicitly want an immediate jump). Locate symbols: AudioCommand::SetVolume handler, volume (RwLock), sink (s), state (PlaybackState), active_fade, and s.set_volume; change the logic so that when st != PlaybackState::Paused and sink exists you either update active_fade.target_vol = v (if active_fade.is_some()) or perform the immediate s.set_volume(v) and active_fade = None (if no fade).
206-220: Consider extracting fade step constants.The fade configuration (10 steps, 10ms timeout) is duplicated across multiple locations. Extracting these as constants would improve maintainability and make it easier to tune fade behavior.
♻️ Suggested refactor
+const FADE_STEPS: u32 = 10; +const FADE_STEP_MS: u64 = 10; + // Fade structures for inline processing enum FadeAction { Pause, SetVolume, } struct ActiveFade { start_vol: f32, target_vol: f32, steps_total: u32, steps_taken: u32, action: FadeAction, }Then use
FADE_STEPSin allActiveFadeinitializations andFADE_STEP_MSfor the timeout.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src-tauri/src/audio/engine.rs` around lines 206 - 220, Duplicate hardcoded fade parameters are used when creating ActiveFade and scheduling timeouts; extract them into constants (e.g., FADE_STEPS: u32 = 10 and FADE_STEP_MS: u64 = 10) and replace all literal occurrences with these constants. Update any ActiveFade initializations (start_vol/target_vol/steps_total/steps_taken/action) and any timeout/sleep calls that use 10 or 10ms to use FADE_STEPS and FADE_STEP_MS respectively, keeping the FadeAction enum, ActiveFade struct and active_fade variable names intact.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src-tauri/src/audio/engine.rs`:
- Around line 343-356: The pause handler sets PlaybackState::Paused immediately
(AudioCommand::Pause) which can prevent the track-end detection logic (the check
that compares state to PlaybackState::Playing) from firing during the short
active_fade window; either include Paused in that track-end condition so
finished tracks are still detected while fading, or defer changing state to
PlaybackState::Paused until after the fade and actual s.pause() completes and
then call emit_state(&app, &state, &position_ms, &duration_ms, &volume); —
update the AudioCommand::Pause branch (where active_fade is created and
emit_state is called) and/or the track-end detection predicate to reference
PlaybackState::Paused so the player emits track-finished correctly.
---
Nitpick comments:
In `@src-tauri/src/audio/engine.rs`:
- Around line 390-400: When handling AudioCommand::SetVolume in the SetVolume
branch, avoid cancelling an in-progress fade by overwriting active_fade;
instead, if an active_fade exists update its target_vol to the new v so the
current ramp continues smoothly toward the new level, and only call
s.set_volume(v) and clear active_fade when there is no active_fade (or when you
explicitly want an immediate jump). Locate symbols: AudioCommand::SetVolume
handler, volume (RwLock), sink (s), state (PlaybackState), active_fade, and
s.set_volume; change the logic so that when st != PlaybackState::Paused and sink
exists you either update active_fade.target_vol = v (if active_fade.is_some())
or perform the immediate s.set_volume(v) and active_fade = None (if no fade).
- Around line 206-220: Duplicate hardcoded fade parameters are used when
creating ActiveFade and scheduling timeouts; extract them into constants (e.g.,
FADE_STEPS: u32 = 10 and FADE_STEP_MS: u64 = 10) and replace all literal
occurrences with these constants. Update any ActiveFade initializations
(start_vol/target_vol/steps_total/steps_taken/action) and any timeout/sleep
calls that use 10 or 10ms to use FADE_STEPS and FADE_STEP_MS respectively,
keeping the FadeAction enum, ActiveFade struct and active_fade variable names
intact.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 64b80700-707e-4c50-afae-b5224a83f7a6
📒 Files selected for processing (1)
src-tauri/src/audio/engine.rs
2 participants
Description
This PR implements smooth volume transitions (100-500 ms) for core playback actions. This eliminates abrupt audio "pops".
The Great Sunder Checklist
reqwestor TLS) without discussing it in an issue first.cargo fmtandcargo clippylocally.Summary by CodeRabbit