Implement audio download#1069
Open
TheNonPirate wants to merge 25 commits into
Open
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds IndexedDB-backed offline audio storage, a download confirmation modal with progress and cancellation, and playback checks that request missing downloads before audio starts. ChangesOffline audio downloads
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant AudioBar
participant TextPage
participant AudioData
participant IndexedDB
participant Layout
participant AudioDownloadModal
AudioBar->>TextPage: checkAudioAvailability()
TextPage->>AudioData: checkAudioAvailability()
AudioData->>IndexedDB: findAudioClip(collection, book, chapter)
IndexedDB-->>AudioData: clip or missing result
AudioData->>Layout: open DownloadAudio modal
Layout->>AudioDownloadModal: showModal(audioPath)
AudioDownloadModal->>IndexedDB: addAudioClip(item, audioPath, abortController)
IndexedDB-->>AudioDownloadModal: progress and download result
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ 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 |
TheNonPirate
marked this pull request as ready for review
July 13, 2026 21:03
![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: 8
🤖 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/components/AudioDownloadModal.svelte`:
- Line 53: Update the console.error call in the audio download error handler to
remove the gibberish debugging text, while retaining a clear “Error downloading
audio” message and the existing err details.
- Around line 57-66: Update finishModal in AudioDownloadModal so it closes the
modal after downloadAudio completes, regardless of whether a clip was added, an
error occurred, or the operation was aborted. Preserve the existing error
reporting and timeout behavior before invoking the modal-close action.
- Around line 107-109: Add an onclick handler to the “No” button in
AudioDownloadModal so it invokes the modal’s existing close/dismiss behavior.
Reuse the established close handler or state update already used by the
component, keeping the “Yes” action and button labeling unchanged.
In `@src/lib/data/audio.ts`:
- Around line 761-768: Update addAudioClip so it returns true only after
audioClips.add successfully stores nextItem; when bookIndex is undefined or
negative and storage is skipped, return false so
AudioDownloadModal.downloadAudio does not treat the operation as successful.
- Around line 728-755: Update the fetch call in the audio download flow to pass
abortController.signal, then validate response.ok before reading response.body
and return false for HTTP failures. Preserve the existing read-loop cancellation
and progress behavior for successful responses.
- Line 756: Remove the chunks.splice call so the complete downloaded audio chunk
collection is preserved before constructing the Blob. Do not replace it with
another truncation or filtering operation.
- Around line 834-836: Update getAudioSourceInfo’s foundAudioClip handling to
avoid accumulating Blob URLs: cache and reuse the URL for each clip, or revoke
the previous clip URL when switching sources. Ensure URLs created through
URL.createObjectURL are eventually passed to URL.revokeObjectURL while
preserving the returned audio source behavior.
In `@src/routes/text/`+page.svelte:
- Around line 498-501: Update the audio toggle handler around
checkForAudioDownload so it awaits the async result and only toggles
$audioActive when audio download is available. Preserve the existing behavior of
leaving the audio bar inactive when the check fails, matching the guarded
playback behavior in AudioBar.
🪄 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: cab9e3a3-082f-464d-917e-e37272eca91b
📒 Files selected for processing (7)
convert/convertConfig.tssrc/lib/components/AudioBar.sveltesrc/lib/components/AudioDownloadModal.sveltesrc/lib/data/audio.tssrc/lib/data/stores/view.tssrc/routes/+layout.sveltesrc/routes/text/+page.svelte
FyreByrd
requested changes
Jul 14, 2026
FyreByrd
left a comment
FyreByrd
left a comment
Collaborator
There was a problem hiding this comment.
I found a styling bug in Modal.svelte when switching between themes that you could fix here.
Change the source of Modal.svelte to
<!--
@component
A simple modal component from DaisyUI. Closes when clicked outside of.
See https://daisyui.com/components/modal/#modal-that-closes-when-clicked-outside
@prop { String } id - ID for the modal.
@prop { Function } content - Snippet containing the content of the modal.
@prop { String } addCSS - CSS to inject into the model contents div/form.
@prop { Function } onclose - Function to run when Modal closes.
-->
<script lang="ts">
import { direction, themeColors } from '$lib/data/stores';
import type { Snippet } from 'svelte';
interface Props {
id: string;
children?: Snippet;
styling?: string;
onclose?: () => void;
dialog?: HTMLDialogElement;
}
let { id, children, styling, onclose, dialog = $bindable() }: Props = $props();
/**
* This exported function allows buttons/labels
* in other divs to trigger the modal popup
*/
export function showModal() {
dialog?.showModal();
}
</script>
<dialog
bind:this={dialog}
{id}
{onclose}
class="dy-modal cursor-pointer"
style:direction={$direction}
>
<form
method="dialog"
style={styling ?? `background-color:${$themeColors['DialogBackgroundColor']};`}
class="dy-modal-box overflow-y-visible relative"
>
{@render children?.()}
<!--This is the snippet for the popup's actual contents-->
</form>
<form method="dialog" class="dy-modal-backdrop">
<!--This allows the modal to be closed when the user taps outside of the contents div/form-->
<button>close</button>
</form>
</dialog>| $userSettings['audio-auto-download'] = 'auto'; | ||
| } | ||
| downloadProgress = 1; | ||
| abortController = new AbortController(); |
Collaborator
There was a problem hiding this comment.
Ooh. Not in this PR, but you may want to look into adding AbortController to some of the non-audio downloads (i.e. Verse-on-Image)
The service worker cache disappears when the app is updated, so we need to use the database method, not the service worker cache method.
TheNonPirate
force-pushed
the
feature/download-audio-offline/1036
branch
from
July 14, 2026 17:59
6b0fa50 to
d0fdb85
Compare
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/routes/text/+page.svelte (1)
508-512: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
checkAudioAvailability()is not awaited in the toggle handler.The async result is ignored and
$audioActiveis toggled unconditionally, so the audio bar appears even when audio is unavailable. ThePlayButtonhandler inAudioBardoes await and check the result, so playback is correctly gated — but the inconsistent UX (bar shown with no playable audio) could confuse users.🔧 Proposed fix
- onclick={() => { + onclick={async () => { if (!$audioActive) { - checkAudioAvailability(); + const audioAvailable = await checkAudioAvailability(); + if (!audioAvailable) { + return; + } } $audioActive = !$audioActive; }}🤖 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/routes/text/`+page.svelte around lines 508 - 512, Update the audio toggle handler around checkAudioAvailability so it awaits the asynchronous availability check and toggles $audioActive only when audio is available; preserve the existing behavior for deactivation and align the gating with AudioBar’s PlayButton handler.
🤖 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.
Duplicate comments:
In `@src/routes/text/`+page.svelte:
- Around line 508-512: Update the audio toggle handler around
checkAudioAvailability so it awaits the asynchronous availability check and
toggles $audioActive only when audio is available; preserve the existing
behavior for deactivation and align the gating with AudioBar’s PlayButton
handler.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 57c27ef0-d2cc-4db2-963d-fef38e0fb0c4
📒 Files selected for processing (8)
convert/convertConfig.tssrc/lib/components/AudioBar.sveltesrc/lib/components/AudioDownloadModal.sveltesrc/lib/components/Modal.sveltesrc/lib/data/audio.tssrc/lib/data/stores/view.tssrc/routes/+layout.sveltesrc/routes/text/+page.svelte
🚧 Files skipped from review as they are similar to previous changes (5)
- src/routes/+layout.svelte
- src/lib/data/audio.ts
- src/lib/components/AudioDownloadModal.svelte
- src/lib/data/stores/view.ts
- convert/convertConfig.ts
Contributor
Author
|
Do we have a way to test FCBH Bible Brain audio? |
Collaborator
Add an audio source in the test project in SAB. Unfortunately the current FCBH audio source is semi-broken, not sure how that happened... |
Contributor
Author
|
There's still an issue that I haven't figure out yet, which may be with Bible Brain audio files specifically, or may just be something that I noticed while testing them: |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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 869-895: Update the audio download flow around getBibleBrainUrl
and findAudioClip so findAudioClip checks IndexedDB before any Bible Brain
lookup for FCBH sources. Reuse a locally stored clip when available and only
call getBibleBrainUrl when no local clip can satisfy the request, while
preserving existing handling for download sources and remote errors.
- Around line 896-904: Update the foundAudioClip handling so it returns false
only when a downloadable source successfully triggers the DownloadAudio modal.
For bundled assets or sources without the download access method, do not reject
playback through this branch; preserve the existing auto-download and modal
visibility behavior for downloadable sources.
In `@src/lib/data/audioclipsDB.ts`:
- Around line 12-35: Define the full clip identity as [docSet, collection, book,
chapter] across persistence and playback: update the AudioClips schema and
openAudioClips indexes/keying, replace clip writes with put() and require docSet
in lookups in audioclipsDB.ts, pass docSet and include it in the FCBH cache key
and download-source cache identity in audio.ts, and include refs.docSet in the
availability lookup; apply these changes at src/lib/data/audioclipsDB.ts lines
12-35 and 87-108, and src/lib/data/audio.ts lines 710-721, 740-750, and 891-895.
In `@src/lib/data/mrucache.ts`:
- Around line 39-41: Update the MruCache clear() method to iterate over the
cache entries with a for...of loop instead of calling forEach on
this.cache.keys(). Preserve invoking cleanupItem with each cached value and key
before clearing the cache.
🪄 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: 19671e44-afe7-42fa-b85e-82f4d29453b2
📒 Files selected for processing (6)
src/lib/components/AudioDownloadModal.sveltesrc/lib/data/audio.tssrc/lib/data/audioclipsDB.tssrc/lib/data/mrucache.tssrc/lib/scripts/mediaUtils.tssrc/routes/text/+page.svelte
🚧 Files skipped from review as they are similar to previous changes (1)
- src/lib/components/AudioDownloadModal.svelte
Comment on lines
+12
to
+35
| interface AudioClips extends DBSchema { | ||
| audioclips: { | ||
| key: number; | ||
| value: AudioItem; | ||
| indexes: { | ||
| 'collection, book, chapter': string[]; | ||
| date: string; | ||
| }; | ||
| }; | ||
| } | ||
| let audioDB: Awaited<ReturnType<typeof openDB<AudioClips>>> | null = null; | ||
| async function openAudioClips() { | ||
| if (!audioDB) { | ||
| audioDB = await openDB<AudioClips>('audioclips', 1, { | ||
| upgrade(db) { | ||
| const audioStore = db.createObjectStore('audioclips', { | ||
| keyPath: 'date' | ||
| }); | ||
| audioStore.createIndex('collection, book, chapter', [ | ||
| 'collection', | ||
| 'book', | ||
| 'chapter' | ||
| ]); | ||
| audioStore.createIndex('date', ['date']); |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Define one unique, full clip identity across persistence and playback.
src/lib/data/audioclipsDB.ts#L12-L35: use[docSet, collection, book, chapter]as the unique key/index.src/lib/data/audioclipsDB.ts#L87-L108: replace clips withput()and requiredocSetin lookups.src/lib/data/audio.ts#L710-L721: passdocSetand include it in the FCBH cache key.src/lib/data/audio.ts#L740-L750: includedocSetin download-source lookup and cache identity.src/lib/data/audio.ts#L891-L895: include the currentrefs.docSetin availability lookup.
📍 Affects 2 files
src/lib/data/audioclipsDB.ts#L12-L35(this comment)src/lib/data/audioclipsDB.ts#L87-L108src/lib/data/audio.ts#L710-L721src/lib/data/audio.ts#L740-L750src/lib/data/audio.ts#L891-L895
🤖 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/audioclipsDB.ts` around lines 12 - 35, Define the full clip
identity as [docSet, collection, book, chapter] across persistence and playback:
update the AudioClips schema and openAudioClips indexes/keying, replace clip
writes with put() and require docSet in lookups in audioclipsDB.ts, pass docSet
and include it in the FCBH cache key and download-source cache identity in
audio.ts, and include refs.docSet in the availability lookup; apply these
changes at src/lib/data/audioclipsDB.ts lines 12-35 and 87-108, and
src/lib/data/audio.ts lines 710-721, 740-750, and 891-895.
Contributor
Author
|
It seems this bug also exists on main, and I think it's caused by an audio load that doesn't finish until after the chapter is switched, causing it to start playing the wrong chapter's audio, which then cannot be cancelled once the correct audio loads. |
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/lib/data/audio.ts (2)
878-888: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winCache the
refsstore value to avoid subscription overhead inside loops.Calling
get(refs)synchronously creates and destroys a store subscription. Running this repeatedly inside the nested.find()array methods for collections, books, and chapters causes unnecessary CPU and garbage-collection overhead. Extractingget(refs)to a local variable once before the search prevents this.♻️ Proposed refactor
+ const currentRefs = get(refs); const audio = scriptureConfig.bookCollections - ?.find((c) => get(refs).collection === c.id) - ?.books?.find((b) => b.id === get(refs).book) - ?.audio?.find((a) => get(refs).chapter === '' + a.num); + ?.find((c) => currentRefs.collection === c.id) + ?.books?.find((b) => b.id === currentRefs.book) + ?.audio?.find((a) => currentRefs.chapter === '' + a.num); if (audio && get(userSettings)['audio-access-method'] === 'download') { const audioSource = scriptureConfig.audio?.sources[audio.src]; const foundAudioClip = await findAudioClip({ - collection: get(refs).collection || '', - book: get(refs).book || '', - chapter: get(refs).chapter || '' + collection: currentRefs.collection || '', + book: currentRefs.book || '', + chapter: currentRefs.chapter || '' });🤖 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 878 - 888, Cache the `refs` store value once before the nested collection, book, and audio searches, then use that local snapshot throughout the `.find()` predicates and subsequent `findAudioClip` arguments instead of repeatedly calling `get(refs)`.
138-140: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winCancel the orphaned audio download.
When the user switches chapters quickly,
player !== currentAudioPlayercorrectly prevents the stale player from updating. However, the browser will continue to download the orphaned audio file in the background. To save bandwidth and resources, cancel the download by clearing the source.♻️ Proposed refactor
a.onloadedmetadata = () => { if (player !== currentAudioPlayer) { + a.removeAttribute('src'); + a.load(); // Forces the browser to stop fetching the previous source return; }🤖 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 138 - 140, Update the stale-player branch in the audio playback flow to clear the orphaned player’s source before returning when player !== currentAudioPlayer. Preserve the existing guard that prevents stale updates while ensuring the abandoned download is canceled.
🤖 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.
Nitpick comments:
In `@src/lib/data/audio.ts`:
- Around line 878-888: Cache the `refs` store value once before the nested
collection, book, and audio searches, then use that local snapshot throughout
the `.find()` predicates and subsequent `findAudioClip` arguments instead of
repeatedly calling `get(refs)`.
- Around line 138-140: Update the stale-player branch in the audio playback flow
to clear the orphaned player’s source before returning when player !==
currentAudioPlayer. Preserve the existing guard that prevents stale updates
while ensuring the abandoned download is canceled.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 898aee7f-7bd1-4a5f-beb1-b470d2cc3bb1
📒 Files selected for processing (2)
src/lib/data/audio.tssrc/lib/data/mrucache.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/lib/data/mrucache.ts
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
2 participants
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.
Resolves #1036. This PR implements audio downloading using an internal database to store audio.
Summary by CodeRabbit