Skip to content

fix: stop audio on navigation - #137

Merged
rindrics merged 1 commit into
mainfrom
issue-136-stop-audio
Feb 13, 2026
Merged

fix: stop audio on navigation#137
rindrics merged 1 commit into
mainfrom
issue-136-stop-audio

Conversation

@rindrics

@rindrics rindrics commented Feb 13, 2026

Copy link
Copy Markdown
Owner

Summary by CodeRabbit

  • Bug Fixes

    • Player now reliably stops playback and resets current position when the component is unmounted, releasing audio resources and timers.
  • New Features

    • Added an explicit unmount/cleanup path integrated into normal playback flow.
    • Added lifecycle guards to prevent resume/play attempts after unmount and to safely cancel pending resume timers.

@qodo-free-for-open-source-projects

Copy link
Copy Markdown

Review Summary by Qodo

Stop audio playback on page navigation

🐞 Bug fix

Grey Divider

Walkthroughs

Description
• Add cleanup effect to stop audio on component unmount
• Prevent audio from continuing during page navigation
• Update TLA+ model with Unmount action for state verification
• Document issue and solution in GitHub issue tracker
Diagram
flowchart LR
  A["AudioPlayer Component"] -->|useEffect cleanup| B["cleanupAudio function"]
  B -->|stops playback| C["Audio stops on unmount"]
  D["Page Navigation"] -->|triggers unmount| C
  E["TLA+ Model"] -->|Unmount action| F["Verify state transitions"]
Loading

Grey Divider

File Changes

1. .github/issues/stop-audio-on-navigation.md 📝 Documentation +20/-0

GitHub issue documentation for audio navigation bug

• Documents the issue where audio continues playing during page navigation
• Explains root cause: missing cleanup effect in AudioPlayer component
• References existing cleanup pattern in SegmentPlayButton component
• Outlines solution approach for both React component and TLA+ model

.github/issues/stop-audio-on-navigation.md


2. models/AudioPlayer.tla ✨ Enhancement +8/-0

Add Unmount action to TLA+ model

• Adds new Unmount action to represent component unmounting behavior
• Unmount action transitions playState to stopped and resets currentSegment
• Integrates Unmount into Next state transition definition
• Enables formal verification of cleanup behavior across all states

models/AudioPlayer.tla


3. src/components/AudioPlayer.tsx 🐞 Bug fix +7/-0

Add cleanup effect for audio on unmount

• Adds useEffect hook with cleanup function on component unmount
• Calls cleanupAudio function to stop audio playback when component unmounts
• Prevents audio from continuing during page navigation or component removal
• Uses cleanupAudio dependency in effect array for proper cleanup

src/components/AudioPlayer.tsx


Grey Divider

Qodo Logo

@coderabbitai

coderabbitai Bot commented Feb 13, 2026

Copy link
Copy Markdown
📝 Walkthrough

Walkthrough

Adds an explicit Unmount transition to the AudioPlayer TLA+ model and introduces component-level unmount cleanup in the React AudioPlayer to stop playback, clear timeouts, and prevent post-unmount callbacks.

Changes

Cohort / File(s) Summary
Formal Model
models/AudioPlayer.tla
Adds exported Unmount action: sets playState' = "stopped", currentSegment' = 0, and UNCHANGED <<loopEnabled, selectedSegments>>; integrates Unmount as an alternative in the Next transition.
Component Implementation
src/components/AudioPlayer.tsx
Adds isMountedRef and resumeTimeoutRef, guards callbacks with mount checks, clears pending resume timeouts, and adds an unmount cleanup effect that stops audio and removes timeouts/listeners.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Poem

🐰
I hopped in code, then gently stopped,
Cleared my timers, audio dropped.
Pages turn — no lingering tune,
A tidy hop beneath the moon. 🎵

🚥 Pre-merge checks | ✅ 4 | ❌ 2
❌ Failed checks (2 warnings)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
Merge Conflict Detection ⚠️ Warning ❌ Merge conflicts detected (3 files):

⚔️ CHANGELOG.md (content)
⚔️ models/AudioPlayer.tla (content)
⚔️ src/components/AudioPlayer.tsx (content)

These conflicts must be resolved before merging into main.
Resolve conflicts locally and push changes to this branch.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately describes the main change: adding audio cleanup on component unmount during navigation.
Linked Issues check ✅ Passed All requirements from issue #136 are met: cleanup effect added to AudioPlayer, TLA+ model updated with Unmount action, and isMountedRef guards added.
Out of Scope Changes check ✅ Passed All changes are directly scoped to resolving issue #136: lifecycle management and safety guards in AudioPlayer, plus TLA+ model update.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch issue-136-stop-audio
⚔️ Resolve merge conflicts (beta)
  • Auto-commit resolved conflicts to branch issue-136-stop-audio
  • Create stacked PR with resolved conflicts
  • Post resolved changes as copyable diffs in a comment

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@github-actions

github-actions Bot commented Feb 13, 2026

Copy link
Copy Markdown

Visit the preview URL for this PR (updated for commit f86879d):

https://izuminokami-kanesada--pr137-issue-136-stop-audio-3satj9qw.web.app

(expires Fri, 20 Feb 2026 09:25:10 GMT)

🔥 via Firebase Hosting GitHub Action 🌎

Sign: 4c4412227845b968bcb4c8b6996048cdd07fd6de

@rindrics
rindrics force-pushed the issue-136-stop-audio branch from f6d11db to 5104015 Compare February 13, 2026 05:31
@qodo-free-for-open-source-projects

Copy link
Copy Markdown

Code Review by Qodo

🐞 Bugs (1) 📘 Rule violations (0) 📎 Requirement gaps (0)

Grey Divider


Action required

1. Async resume restarts audio 🐞 Bug ⛯ Reliability
Description
The new unmount cleanup only calls cleanupAudio(), but AudioPlayer schedules a setTimeout resume on
language change that is not canceled on unmount. If navigation happens before the timeout fires, the
callback can run after unmount, call playSegment(), and create/play a new Audio instance—so audio
may still continue after navigation.
Code

src/components/AudioPlayer.tsx[R125-130]

+  // Stop audio on unmount (e.g. page navigation)
+  useEffect(() => {
+    return () => {
+      cleanupAudio();
+    };
+  }, [cleanupAudio]);
Evidence
On unmount the component calls cleanupAudio(), which pauses and nulls the current audio element but
does not cancel any pending timers. Separately, handleLanguageChange schedules a setTimeout that
calls setIsPlaying(true) and playSegment(wasSegment). playSegment constructs a new Audio(url) and
starts playback. Because the timeout is not cleared on unmount, navigation shortly after a language
change can recreate and play audio after cleanup ran.

src/components/AudioPlayer.tsx[109-130]
src/components/AudioPlayer.tsx[224-240]
src/components/AudioPlayer.tsx[132-148]
src/components/AudioPlayer.tsx[180-184]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
AudioPlayer now calls `cleanupAudio()` on unmount, but there are asynchronous callbacks that can outlive the component (notably a `setTimeout` used to resume after language change). If navigation happens before that timer fires, the callback can still run after unmount and call `playSegment()` (creating a new `Audio()`), so audio may continue after navigation.

### Issue Context
- Unmount cleanup currently only pauses/removes listeners on the current `audioRef`.
- `handleLanguageChange()` schedules a timer that resumes playback.
- `playSegment()` also has async promise callbacks (`audio.play().catch`) that can call `setState` after unmount.

### Fix Focus Areas
- src/components/AudioPlayer.tsx[109-130]
- src/components/AudioPlayer.tsx[224-240]
- src/components/AudioPlayer.tsx[132-148]
- src/components/AudioPlayer.tsx[180-184]

### Suggested approach
1. Add a `resumeTimeoutRef` to store the timeout id; clear it:
  - before scheduling a new timeout
  - inside the unmount cleanup effect
2. Add an `isUnmountedRef` (or `isMountedRef`) set in the unmount cleanup and check it before:
  - calling `setIsPlaying`/`setCurrentSegment`
  - calling `playSegment` from delayed callbacks
  - handling `audio.play().catch(...)`

This ensures the PR guarantee (“stop audio on navigation”) holds even if the user navigates during pending async operations.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


Grey Divider

ⓘ The new review experience is currently in Beta. Learn more

Grey Divider

Qodo Logo

Comment on lines +125 to +130
// Stop audio on unmount (e.g. page navigation)
useEffect(() => {
return () => {
cleanupAudio();
};
}, [cleanupAudio]);

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Action required

1. Async resume restarts audio 🐞 Bug ⛯ Reliability

The new unmount cleanup only calls cleanupAudio(), but AudioPlayer schedules a setTimeout resume on
language change that is not canceled on unmount. If navigation happens before the timeout fires, the
callback can run after unmount, call playSegment(), and create/play a new Audio instance—so audio
may still continue after navigation.
Agent Prompt
### Issue description
AudioPlayer now calls `cleanupAudio()` on unmount, but there are asynchronous callbacks that can outlive the component (notably a `setTimeout` used to resume after language change). If navigation happens before that timer fires, the callback can still run after unmount and call `playSegment()` (creating a new `Audio()`), so audio may continue after navigation.

### Issue Context
- Unmount cleanup currently only pauses/removes listeners on the current `audioRef`.
- `handleLanguageChange()` schedules a timer that resumes playback.
- `playSegment()` also has async promise callbacks (`audio.play().catch`) that can call `setState` after unmount.

### Fix Focus Areas
- src/components/AudioPlayer.tsx[109-130]
- src/components/AudioPlayer.tsx[224-240]
- src/components/AudioPlayer.tsx[132-148]
- src/components/AudioPlayer.tsx[180-184]

### Suggested approach
1. Add a `resumeTimeoutRef` to store the timeout id; clear it:
   - before scheduling a new timeout
   - inside the unmount cleanup effect
2. Add an `isUnmountedRef` (or `isMountedRef`) set in the unmount cleanup and check it before:
   - calling `setIsPlaying`/`setCurrentSegment`
   - calling `playSegment` from delayed callbacks
   - handling `audio.play().catch(...)`

This ensures the PR guarantee (“stop audio on navigation”) holds even if the user navigates during pending async operations.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

@rindrics
rindrics force-pushed the issue-136-stop-audio branch from 5104015 to f86879d Compare February 13, 2026 09:23

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
src/components/AudioPlayer.tsx (1)

246-256: ⚠️ Potential issue | 🟠 Major

Stale closure: playSegment captures the old lang inside the timeout.

setLang(newLang) on line 243 triggers a re-render that produces a new playSegment with the updated language. However, the setTimeout callback on line 254 closes over the playSegment from the current render (still using the old lang). After the 100ms delay, this will start playback with the previous language, not the newly selected one.

You already maintain playSegmentRef (line 210-212) for exactly this purpose — use it here.

Proposed fix
       resumeTimeoutRef.current = setTimeout(() => {
         resumeTimeoutRef.current = null;
         if (!isMountedRef.current) return;
         setIsPlaying(true);
-        playSegment(wasSegment);
+        playSegmentRef.current?.(wasSegment);
       }, 100);

@rindrics
rindrics merged commit 0cbd41f into main Feb 13, 2026
6 checks passed
@rindrics
rindrics deleted the issue-136-stop-audio branch February 13, 2026 11:52
@rindrics rindrics mentioned this pull request Feb 13, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

ページ遷移時に音声再生が停止しない

1 participant