#508 Allow seeking audio to next/previous heading#989
Open
benpaj-cps wants to merge 12 commits into
Open
Conversation
Contributor
ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 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 | ✅ 5✅ Passed checks (5 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
3 times, most recently
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
4 times, most recently
from
b5949c3 to
3d2e0b8
Compare
Contributor
Author
|
@chrisvire This is ready for review. |
benpaj-cps
changed the title
benpaj-cps
force-pushed
the
fix/audio-seek-headings/GH-508
branch
from
71d153c to
ce7e2b4
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 540-544: The `hasAudioPlayed()` function is currently checking
`progress > 0` to determine if audio has played, but this is inconsistent with
how `updateTime()` and `play()` now use the `hasPlayed` property. When
verse-number seeking resets progress to 0, the function will incorrectly return
false even though audio has already played, causing verse clicks to remain gated
in downstream components like ScriptureViewSofria.svelte. Update the
`hasAudioPlayed()` function to return the `hasPlayed` property from the current
audio player instead of checking the progress value, ensuring consistent
played-state tracking across all methods.
- Around line 344-346: The condition checking newtime uses a truthy check that
fails when newtime is 0, a valid starttime value. Change the if condition from
checking newtime truthiness to explicitly checking if newtime is not undefined
or null (using !== null or != null), while keeping the check for
currentAudioPlayer.audio, so that the seek function can be called with 0 as a
valid timestamp.
- Around line 259-273: The while loop condition checking for the data-verse
attribute stops too early when encountering title verses. Modify the while loop
at line 259 to continue past `data-verse="title"` elements by updating the
condition to skip title verses and only stop when finding numeric verses.
Additionally, update the find() call that searches the timing array to ensure it
correctly matches numeric verses by filtering or validating the verse value to
exclude title entries, so that the code properly advances next to the first
numeric verse immediately following the heading instead of stopping at the title
marker which has a starttime of 0.
🪄 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: 5ec2c4b0-8603-495a-8cc8-ef4ce0deb99e
📒 Files selected for processing (2)
src/lib/data/audio.tssrc/lib/data/stores/audio.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/lib/data/stores/audio.ts
Contributor
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 3
🤖 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 540-544: The `hasAudioPlayed()` function is currently checking
`progress > 0` to determine if audio has played, but this is inconsistent with
how `updateTime()` and `play()` now use the `hasPlayed` property. When
verse-number seeking resets progress to 0, the function will incorrectly return
false even though audio has already played, causing verse clicks to remain gated
in downstream components like ScriptureViewSofria.svelte. Update the
`hasAudioPlayed()` function to return the `hasPlayed` property from the current
audio player instead of checking the progress value, ensuring consistent
played-state tracking across all methods.
- Around line 344-346: The condition checking newtime uses a truthy check that
fails when newtime is 0, a valid starttime value. Change the if condition from
checking newtime truthiness to explicitly checking if newtime is not undefined
or null (using !== null or != null), while keeping the check for
currentAudioPlayer.audio, so that the seek function can be called with 0 as a
valid timestamp.
- Around line 259-273: The while loop condition checking for the data-verse
attribute stops too early when encountering title verses. Modify the while loop
at line 259 to continue past `data-verse="title"` elements by updating the
condition to skip title verses and only stop when finding numeric verses.
Additionally, update the find() call that searches the timing array to ensure it
correctly matches numeric verses by filtering or validating the verse value to
exclude title entries, so that the code properly advances next to the first
numeric verse immediately following the heading instead of stopping at the title
marker which has a starttime of 0.
🪄 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: 5ec2c4b0-8603-495a-8cc8-ef4ce0deb99e
📒 Files selected for processing (2)
src/lib/data/audio.tssrc/lib/data/stores/audio.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/lib/data/stores/audio.ts
🛑 Comments failed to post (3)
src/lib/data/audio.ts (3)
259-273:
⚠️ Potential issue | 🟠 Major | ⚡ Quick winContinue past title markers when locating the verse after a heading.
data-verse="title"can appear between a section heading and the first numeric verse, so this stops too early and drops the marker because the title timing is0. Filter for numeric verses and match timing tags exactly/prefix-style sonextseeks to the verse after the heading instead of falling through to the next chapter. Based on PR objective: next should advance to the verse immediately following the next section heading.Proposed fix
- let next = nextElementDFS(h); - while (next && !next?.getAttribute('data-verse')) { - next = nextElementDFS(next); - } - - // If present this is the first verse immediately after the heading - const verse = next?.getAttribute('data-verse'); + let next = nextElementDFS(h); + let verse = next?.getAttribute('data-verse'); + while (next && (!verse || !/^\d+$/.test(verse))) { + next = nextElementDFS(next); + verse = next?.getAttribute('data-verse'); + } + + // If present this is the first numeric verse immediately after the heading if (verse === null || verse === undefined) { return; } // find() always returns the first element it matches, so // this will locate the beginning of the first phrase of the verse - const marker = currentAudioPlayer?.timing?.find((v) => v.tag.includes(verse))?.starttime; - if (marker) { + const marker = currentAudioPlayer?.timing?.find((v) => v.tag === verse || v.tag === `${verse}a`)?.starttime; + if (typeof marker === 'number') { headingMarkers.push(marker); }🤖 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/lib/data/audio.ts` around lines 259 - 273, The while loop condition checking for the data-verse attribute stops too early when encountering title verses. Modify the while loop at line 259 to continue past `data-verse="title"` elements by updating the condition to skip title verses and only stop when finding numeric verses. Additionally, update the find() call that searches the timing array to ensure it correctly matches numeric verses by filtering or validating the verse value to exclude title entries, so that the code properly advances next to the first numeric verse immediately following the heading instead of stopping at the title marker which has a starttime of 0.
344-346:
⚠️ Potential issue | 🟠 Major | ⚡ Quick winAllow seeking to timestamp
0.
0is a validstarttime, but the truthy check skips it, so rewinding to the first timing entry can leave playback at the current position.Proposed fix
const newtime = currentAudioPlayer.timing?.[currentAudioPlayer.timeIndex].starttime; - if (newtime && currentAudioPlayer.audio) { + if (typeof newtime === 'number' && currentAudioPlayer.audio) { seek(newtime); }📝 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.const newtime = currentAudioPlayer.timing?.[currentAudioPlayer.timeIndex].starttime; if (typeof newtime === 'number' && currentAudioPlayer.audio) { seek(newtime); }🤖 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/lib/data/audio.ts` around lines 344 - 346, The condition checking newtime uses a truthy check that fails when newtime is 0, a valid starttime value. Change the if condition from checking newtime truthiness to explicitly checking if newtime is not undefined or null (using !== null or != null), while keeping the check for currentAudioPlayer.audio, so that the seek function can be called with 0 as a valid timestamp.
540-544:
⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUse
hasPlayedconsistently for the exported played-state check.
updateTime()andplay()now usehasPlayed, buthasAudioPlayed()still derives state fromprogress > 0; verse-number seeking can remain blocked after audio has started if progress is still/reset to0. Based on downstream usage inScriptureViewSofria.svelte, verse clicks are gated byhasAudioPlayed().Proposed fix
export function hasAudioPlayed() { - return (currentAudioPlayer?.progress ?? 0) > 0; + return currentAudioPlayer?.hasPlayed === true; }Also applies to: 607-609
🤖 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/lib/data/audio.ts` around lines 540 - 544, The `hasAudioPlayed()` function is currently checking `progress > 0` to determine if audio has played, but this is inconsistent with how `updateTime()` and `play()` now use the `hasPlayed` property. When verse-number seeking resets progress to 0, the function will incorrectly return false even though audio has already played, causing verse clicks to remain gated in downstream components like ScriptureViewSofria.svelte. Update the `hasAudioPlayed()` function to return the `hasPlayed` property from the current audio player instead of checking the progress value, ensuring consistent played-state tracking across all methods.
Contributor
Author
|
This needed to be rebased on main after the recent merges, and should be good for review again. |
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
Summary by CodeRabbit