#508 Allow seeking audio to next/previous heading#989
Open
benpaj-cps wants to merge 12 commits into
Open
Conversation
Contributor
📝 WalkthroughWalkthroughAdds heading-marker-based skip navigation: AudioPlayer gets hasPlayed and optional headingMarkers; skip(direction) now pauses, preserves play state, lazily computes/caches DOM-derived heading markers, uses a 2s AUDIO_SEEK_THRESHOLD to decide local seeks vs refs.skip, and resumes playback. updateTime/play use hasPlayed. ChangesHeading-marker audio navigation
Sequence DiagramsequenceDiagram
participant User
participant Skip as skip(direction)
participant Markers as getHeadingMarkers()
participant DOM as DOM(div.s / data-verse)
participant Timing as currentAudioPlayer.timing
participant Audio as currentAudioPlayer.seek()
User->>Skip: click prev/next
Skip->>Skip: pause, record wasPlaying
Skip->>Timing: check timing & loaded
alt timing missing or audio not ready
Skip->>Skip: decide threshold -> local seek or refs.skip
else timing available
Skip->>Markers: compute or read cached headingMarkers
Markers->>DOM: scan div.s and DFS to next data-verse
DOM->>Timing: map verse tags to timing start times
Markers->>Skip: return markers[]
Skip->>Skip: compare progress vs AUDIO_SEEK_THRESHOLD and markers
alt seek within chapter to marker
Skip->>Audio: seek(targetMarker)
else delegate to chapter skip
Skip->>Skip: call refs.skip(direction)
end
end
Skip->>Skip: updateTime()
Skip->>User: resume playback if wasPlaying
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
Contributor
Author
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 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/lib/data/audio.ts`:
- Line 221: Remove the leftover debug console.log calls in src/lib/data/audio.ts
— specifically the lines that call console.log with template strings like `found
heading ${h.getHTML()}` and the other console.log statements around the
heading/paragraph processing (the debug prints at lines referenced in the
review). Locate the block that iterates headings/paragraphs (references to
h.getHTML(), p.getText(), etc.) and delete those console.log(...) calls or
replace them with the project's sanctioned logger if persistent logging is
required.
- Line 154: Remove the debug console.log calls in src/lib/data/audio.ts (e.g.,
the "console.log('skip')" and the other console.log occurrences) before merge;
delete these console.log lines or replace them with the project's structured
logger (e.g., processLogger.debug or logger.debug) if the information is needed,
ensuring you reference and update the surrounding functions in the file where
they appear so behavior and return flow remains correct (remove the
console.log(...) statements inside the relevant functions and use the existing
logging utility or nothing).
- Line 217: The variable headingMarkers is declared with let but never
reassigned; change its declaration to const so it remains immutable while still
allowing mutation via push()—update the headingMarkers declaration (where it is
initialized to [0.0]) to use const instead of let in src/lib/data/audio.ts to
reflect that it is only mutated, not reassigned.
- Around line 190-206: The loop that decides where to seek can perform an
out-of-bounds access when calling seek(currentAudioPlayer.headingMarkers[i +
1]); update the logic in the loop that iterates
currentAudioPlayer.headingMarkers to guard any i+1 access: before calling
seek(currentAudioPlayer.headingMarkers[i + 1]) verify i + 1 <
currentAudioPlayer.headingMarkers.length (and handle the case by seeking to the
last marker or current marker instead), and ensure play() is still resumed when
wasPlaying is true; adjust the conditions around direction,
currentAudioPlayer.progress, and AUDIO_SEEK_THRESHOLD accordingly to use the
safe alternative when i+1 would be out of range.
- Around line 241-243: The push to headingMarkers uses an unsafe non-null
assertion on currentAudioPlayer.duration; replace this with a safe fallback and
validation: compute a candidate value from
currentAudioPlayer?.timing?.at(-1)?.endtime ?? currentAudioPlayer?.duration ??
null, ensure it's a finite number (or >= 0) before pushing, and skip or push a
defined default (e.g. 0) if the candidate is null/invalid; update the code
around headingMarkers.push(...) and reference currentAudioPlayer, timing, and
duration when implementing this guard.
- Around line 179-188: The code uses an unsafe non-null assertion on
headingMarkers?.at(-2)! which can be undefined and breaks the pipeline; compute
a safe value first (e.g., const lastButOne = headingMarkers &&
headingMarkers.length >= 2 ? headingMarkers.at(-2) : undefined) and then change
the second part of the if to check lastButOne !== undefined before comparing
progress (e.g., direction >= 0 && lastButOne !== undefined &&
currentAudioPlayer.progress >= lastButOne), removing the non-null assertion and
guarding against headingMarkers being undefined or too short; keep the existing
refs.skip(...) call and adjust the log to reference lastButOne if needed.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7f9da34d-40fa-4eda-962f-aa70bc5dd8ee
📒 Files selected for processing (2)
src/lib/data/audio.tssrc/lib/data/stores/audio.ts
benpaj-cps
force-pushed
the
fix/audio-seek-headings/GH-508
branch
2 times, most recently
from
d4c1975 to
3c1b5a3
Compare
benpaj-cps
force-pushed
the
fix/audio-seek-headings/GH-508
branch
from
9223462 to
5bcee7c
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/lib/data/audio.ts`:
- Around line 194-213: When replaying from a local heading seek the store can
remain stale because pause() writes a paused state, seek(...) writes that paused
object to audioPlayerStore, and play() changes playback without updating the
store; modify the local heading-seek path in the loop (the block that calls
seek(...) then play()) to re-sync the store after play() returns by writing the
up-to-date currentAudioPlayer back into audioPlayerStore (or call the existing
function that updates the store for the active player), ensuring
$audioPlayer.playing reflects the resumed state; reference the functions
pause(), seek(), play(), the currentAudioPlayer object, and audioPlayerStore
when making this change.
- Around line 177-186: The code treats headingMarkers.at(-2) === 0 (only
sentinels) as falsy and incorrectly falls through for forward skips; change the
logic in the block that computes and checks finalIntermediateMarker so it only
considers a real intermediate heading when headingMarkers.length > 2 (or
finalIntermediateMarker > 0). Concretely, in the computation of
finalIntermediateMarker and the subsequent if that decides whether to seek to a
marker vs. call skip(1), ensure finalIntermediateMarker is undefined/null when
headingMarkers contains only the two sentinels (e.g., set
finalIntermediateMarker only if headingMarkers.length > 2 or explicitly check
finalIntermediateMarker > 0), so next() will call skip(1) to advance to the next
chapter when there are no further headings in the current chapter.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: aa36fd9b-f2b1-40ed-a990-f39d0bf09e95
📒 Files selected for processing (2)
src/lib/data/audio.tssrc/lib/data/stores/audio.ts
benpaj-cps
force-pushed
the
fix/audio-seek-headings/GH-508
branch
3 times, most recently
from
710d8a5 to
b5949c3
Compare
benpaj-cps
force-pushed
the
fix/audio-seek-headings/GH-508
branch
from
b5949c3 to
3d2e0b8
Compare
Contributor
Author
|
@chrisvire This is ready for review. |
benpaj-cps
changed the title
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.
Fixes #508.
I tested this on Mark 1 in the CUK Bible with a generated timing file from
aeneas. The heading search works even with several nested HTML elements between the heading and the next verse.Summary by CodeRabbit