Skip to content

Add post-processing audio effects system #271

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
jamiepine merged 11 commits into from
Mar 14, 2026
Merged

Add post-processing audio effects system #271

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
638820c
Add post-processing audio effects system
jamiepine Mar 14, 2026
3d922ec
Fix review findings: toggle logic, preset saving, version lookup, asy…
jamiepine Mar 14, 2026
25134b4
Fix player not loading new version after applying effects
jamiepine Mar 14, 2026
00c5b75
Regenerate as new version, UI polish, and bugfixes
jamiepine Mar 14, 2026
e8d54d5
Add favorites, effects badge on profiles, UI polish
jamiepine Mar 14, 2026
899b902
Add version control to track editor, restyle story list
jamiepine Mar 14, 2026
310a4ac
Add source version selection when applying effects, voices tab overha…
jamiepine Mar 14, 2026
626e374
Auto-select first story when navigating to Stories tab
jamiepine Mar 14, 2026
1c61b47
Glassmorphic active state for sidebar buttons with accent border shine
jamiepine Mar 14, 2026
103e98b
github runners suck
jamiepine Mar 14, 2026
6d261c4
Merge branch 'main' into feat/post-processing-effects
jamiepine Mar 14, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 14 additions & 1 deletion app/src/components/AudioPlayer/AudioPlayer.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -157,8 +157,21 @@ export function AudioPlayer() {
const wavesurfer = wavesurferRef.current;
if (!wavesurfer) return;

// Update store when time changes
// Update store when time changes, stop if past duration
wavesurfer.on('timeupdate', (time) => {
const dur = usePlayerStore.getState().duration;
if (dur > 0 && time >= dur) {
setCurrentTime(dur);
const loop = usePlayerStore.getState().isLooping;
if (loop) {
wavesurfer.seekTo(0);
wavesurfer.play();
} else {
wavesurfer.pause();
setIsPlaying(false);
}
return;
}
Comment on lines +160 to +174
Copy link
Contributor

@coderabbitai coderabbitai bot Mar 14, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🌐 Web query:

In WaveSurfer.js, if pause() is called from a timeupdatecallback when currentTime >= duration, is thefinish event still guaranteed to fire? Please cite docs/issues for the current major version.

💡 Result:

No. In the current major version (WaveSurfer.js v7), the finish event is emitted only in the handler for the media element’s ended event—not when you call pause() yourself. So if you call pause() from a timeupdate callback as soon as currentTime >= duration, you can prevent the underlying ended event from occurring, and therefore finish is not guaranteed to fire. [2]

Docs describe finish as “When the audio finishes playing,” but do not state it will fire if you programmatically pause at (or near) the end. [1]

Sources: WaveSurfer events docs [1], WaveSurfer v7 source (ended ⇒ emit finish) [2].

🏁 Script executed:

# Inspect the AudioPlayer file to understand the full context
# First, check file size to ensure safe reading
wc -l app/src/components/AudioPlayer/AudioPlayer.tsx

Repository: jamiepine/voicebox

Length of output: 112

🏁 Script executed:

# Read the timeupdate handler section (lines 160-174) with context
sed -n '155,180p' app/src/components/AudioPlayer/AudioPlayer.tsx

Repository: jamiepine/voicebox

Length of output: 831

🏁 Script executed:

# Read the finish handler section (lines 410-440) with context
sed -n '410,440p' app/src/components/AudioPlayer/AudioPlayer.tsx

Repository: jamiepine/voicebox

Length of output: 969

Move onFinish callback invocation to timeupdate handler or consolidate all terminal logic in the finish handler.

The timeupdate handler pauses at the end of track (lines 169), which prevents the media element's ended event from firing. Since WaveSurfer.js v7 emits the finish event only when ended fires, the finish handler (lines 416-430) is never reached, making the onFinish callback unreachable.

Either:

  1. Invoke onFinish from the timeupdate handler alongside the pause, or
  2. Remove pause/loop logic from timeupdate and let the media naturally fire ended → finish → onFinish.

Option 2 is cleaner and keeps terminal behavior in one place.

Proposed fix (Option 2) 
-      // Update store when time changes, stop if past duration
+      // Update store when time changes; clamp to duration.
+      // End-of-track loop/stop/callback behavior is handled in `finish`.
       wavesurfer.on('timeupdate', (time) => {
         const dur = usePlayerStore.getState().duration;
-        if (dur > 0 && time >= dur) {
-          setCurrentTime(dur);
-          const loop = usePlayerStore.getState().isLooping;
-          if (loop) {
-            wavesurfer.seekTo(0);
-            wavesurfer.play();
-          } else {
-            wavesurfer.pause();
-            setIsPlaying(false);
-          }
-          return;
-        }
-        setCurrentTime(time);
+        setCurrentTime(dur > 0 ? Math.min(time, dur) : time);
       });
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
// Update store when time changes, stop if past duration
wavesurfer.on('timeupdate', (time) => {
const dur = usePlayerStore.getState().duration;
if (dur > 0 && time >= dur) {
setCurrentTime(dur);
const loop = usePlayerStore.getState().isLooping;
if (loop) {
wavesurfer.seekTo(0);
wavesurfer.play();
} else {
wavesurfer.pause();
setIsPlaying(false);
}
return;
}
// Update store when time changes; clamp to duration.
// End-of-track loop/stop/callback behavior is handled in `finish`.
wavesurfer.on('timeupdate', (time) => {
const dur = usePlayerStore.getState().duration;
setCurrentTime(dur > 0 ? Math.min(time, dur) : time);
});
🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@app/src/components/AudioPlayer/AudioPlayer.tsx` around lines 160 - 174, The
timeupdate handler in wavesurfer.on('timeupdate') is pausing playback when time
>= duration which prevents the media 'ended' event and thus WaveSurfer's
'finish' handler (and your onFinish callback) from running; remove the terminal
pause/loop logic from the timeupdate handler (the block that calls
wavesurfer.pause(), wavesurfer.seekTo(0)/play when isLooping,
setIsPlaying(false), and the early return) and instead let the media naturally
reach 'ended' so the existing wavesurfer.on('finish') handler can run and invoke
onFinish; keep only non-terminal updates (e.g., setCurrentTime) in the
timeupdate handler and rely on the isLooping/finish logic inside the finish
event handler to handle looping and finalization.

setCurrentTime(time);
});

Expand Down
Loading