Implement Audio Sharing for Selected Text - #992
Conversation
|
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:
📝 WalkthroughWalkthroughThis PR adds a ShareSelector modal for sharing selected verses as text or trimmed audio. It updates modal dispatch and audio-source resolution, centralizes modal positioning, routes toolbar sharing through the modal, and adds resize-aware text dragging. ChangesVerse sharing and UI updates
Estimated code review effort: 4 (Complex) | ~50 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant TextSelectionToolbar
participant ShareSelector
participant getAudioSourceInfo
participant AudioContext
participant mediabunny
participant shareAudio
User->>TextSelectionToolbar: select share
TextSelectionToolbar->>ShareSelector: open share modal
User->>ShareSelector: choose text or audio
alt Text sharing
ShareSelector->>shareText: share selected text
else Audio sharing
ShareSelector->>getAudioSourceInfo: load chapter source and timing
ShareSelector->>AudioContext: fetch and decode audio
ShareSelector->>mediabunny: trim and encode selected verses
mediabunny->>ShareSelector: return encoded audio
ShareSelector->>shareAudio: share audio file
end
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
e5d1011 to
5ab1d1e
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/lib/components/ShareSelector.svelte (1)
27-40: ⚡ Quick winExtract shared text-share composition into one helper.
Lines 27-40 duplicate the same “selected verses → message →
shareText+logShareContent” logic now also present insrc/lib/components/TextSelectionToolbar.svelte(Lines 138-150). Centralizing this avoids drift in filename/message/analytics behavior.🤖 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/components/ShareSelector.svelte` around lines 27 - 40, Extract the repeated "selected verses → message → shareText + logShareContent" logic into a single helper (e.g., export a function like composeAndShareSelectedText or shareSelectedVerses) and replace the duplicate blocks in shareSelectedText (in ShareSelector.svelte) and the corresponding handler in TextSelectionToolbar.svelte to call that helper; the helper should accept the selectedVerses object (or its derived book, collection, text, and reference), scriptureConfig.name, and internally build the filename, message body, call shareText(scriptureName, message, filename) and then call logShareContent('Text', bookCol, bookAbbrev, reference) so both components share identical filename/message/analytics behavior.
🤖 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/ShareSelector.svelte`:
- Around line 124-129: In shareAudio() in ShareSelector.svelte, modify the catch
after navigator.share(...) to detect a user-cancelled share (error.name ===
'AbortError' or error is a DOMException with name 'AbortError') and return early
so the download fallback is not triggered; only for other errors should the code
continue to createObjectURL(file) and trigger the anchor download. Ensure you
check error?.name and only fall through on non-AbortError cases.
---
Nitpick comments:
In `@src/lib/components/ShareSelector.svelte`:
- Around line 27-40: Extract the repeated "selected verses → message → shareText
+ logShareContent" logic into a single helper (e.g., export a function like
composeAndShareSelectedText or shareSelectedVerses) and replace the duplicate
blocks in shareSelectedText (in ShareSelector.svelte) and the corresponding
handler in TextSelectionToolbar.svelte to call that helper; the helper should
accept the selectedVerses object (or its derived book, collection, text, and
reference), scriptureConfig.name, and internally build the filename, message
body, call shareText(scriptureName, message, filename) and then call
logShareContent('Text', bookCol, bookAbbrev, reference) so both components share
identical filename/message/analytics behavior.
🪄 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: 132775a3-fce6-4d3e-9c2d-793176254d92
📒 Files selected for processing (5)
src/lib/components/ShareSelector.sveltesrc/lib/components/TextSelectionToolbar.sveltesrc/lib/components/VerseOnImage.sveltesrc/lib/data/stores/view.tssrc/routes/+layout.svelte
|
On my phone (Pixel, Android 16, Chrome) the audio share fails in the PWA because of a reported lack of AAC, while the native Android app works fine sharing audio. If this isn't just a bug and there's no simple way to use a different method, it might be good to give a message to the user to the effect of "This feature requires the AAC audio codec to be installed on your phone", rather than silently failing. 
|
9ee24f1 to
4cd56d4
Compare
|
I tried adding another type of audio that it could try to fall back to. Does it work now? |
|
It gets the audio, but uses the download fallback. Also, the filename seems to only be set once per page load (i.e. all further downloads have the same filename, no matter the verse)? |
|
The ironic thing is that the Android app exports to |
|
I fixed the filename only being set once per page load. I have found that the sharing option never seems to work, but I'm not sure if that's something I can fix or not. |
|
I think the AAC problem was due to my accessing it from my phone on LAN over HTTP, whereas AudioEncoder is only supported over HTTPS (see https://developer.mozilla.org/en-US/docs/Web/API/AudioEncoder). Running it on |
|
The Android app adds a bit more information when sharing text. PWA: Android: |
|
However it looks like the URL link from the Android app doesn't work properly. Perhaps that will eventually be another config field, but that's likely a different issue. |
|
As far as I can tell, the copyright information (The World English Bible (WEB) is a Public Domain translation of the Holy Bible.) isn't even exported anywhere (It's not in config, for example). I think what the shared text says is a separate issue from sharing the audio. |
|
Right, okay. After a little digging I found that it comes from the 
Since the share text functionality was in the new modal I didn't realize it predated this PR - that is probably a separate issue. I'll open one for completeness. |
|
I get the following error when trying this on Mark 13, which has audio defined but no local files (i.e. the audio is downloaded from elsewhere). The audio itself plays fine in the PWA, it's only an issue with sharing.
|
|
I was able to figure out how to detect if it has a remote file and share it if it does (Although it seems to just share it as a link, not a file), but the closest I can find to a download option just opens a link to the file. Should I have that as the fallback in case sharing fails, or do something else? It looks like the usual method of fetching the file won't always work for remote files (If their server has the wrong permissions set for cross-site fetches). |
30e8782 to
c230408
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
src/lib/components/ShareSelector.svelte (2)
107-107: ⚡ Quick winUse
letinstead ofvarfor loop variable.Using
varin a for loop creates function-scoped binding which can lead to subtle bugs in closures. Modern JavaScript best practice is to useletfor block-scoped loop variables.♻️ Proposed fix
- for (var j = 0; j < (audioSourceInfo?.timing?.length || 0); j++) { + for (let j = 0; j < (audioSourceInfo?.timing?.length || 0); j++) {🤖 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/components/ShareSelector.svelte` at line 107, Replace the `var` keyword with `let` for the loop variable j in the for loop that iterates over audioSourceInfo?.timing?.length. The var keyword creates function-scoped bindings which can cause subtle closure bugs, while let provides proper block-scoping that is the modern JavaScript best practice for loop variables.
188-209: ⚖️ Poor tradeoffConsider extracting
pickSupportedAudioConfigto a shared utility.The comment on line 209 correctly identifies that this function is duplicated in
VerseOnImage.svelte. Extracting it to a shared module (e.g.,$lib/data/audio.ts) would reduce duplication and ensure consistent codec fallback behavior across the codebase.🤖 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/components/ShareSelector.svelte` around lines 188 - 209, Extract the `pickSupportedAudioConfig` function to a shared utility module (such as a dedicated audio configuration module under the lib directory) to eliminate duplication. Move the function definition from this component to the shared module, then import and use it in both locations where it currently exists. This ensures consistent codec fallback behavior across the codebase and reduces maintenance burden.
🤖 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/ShareSelector.svelte`:
- Line 69: Remove the debug console.log statement that logs
audioSourceInfo?.source from the ShareSelector.svelte component. This appears to
be leftover development code and should be deleted entirely to keep the codebase
clean.
- Around line 78-97: The navigator.share() call in the isRemote block is not
awaited, causing the AbortError catch block to never execute and the download
fallback to always run regardless of share success. Add await before the
navigator.share() call so that promise rejections are properly caught by the
catch block. Additionally, add a return statement after the try-catch block
completes successfully to prevent the download fallback anchor creation and
click from executing when the share succeeds, matching the pattern used
elsewhere in the component.
---
Nitpick comments:
In `@src/lib/components/ShareSelector.svelte`:
- Line 107: Replace the `var` keyword with `let` for the loop variable j in the
for loop that iterates over audioSourceInfo?.timing?.length. The var keyword
creates function-scoped bindings which can cause subtle closure bugs, while let
provides proper block-scoping that is the modern JavaScript best practice for
loop variables.
- Around line 188-209: Extract the `pickSupportedAudioConfig` function to a
shared utility module (such as a dedicated audio configuration module under the
lib directory) to eliminate duplication. Move the function definition from this
component to the shared module, then import and use it in both locations where
it currently exists. This ensures consistent codec fallback behavior across the
codebase and reduces maintenance burden.
🪄 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: f2d04711-2996-4692-903d-ad3e6e21b4e3
📒 Files selected for processing (5)
src/lib/components/ShareSelector.sveltesrc/lib/components/TextSelectionToolbar.sveltesrc/lib/components/VerseOnImage.sveltesrc/lib/data/stores/view.tssrc/routes/+layout.svelte
✅ Files skipped from review due to trivial changes (2)
- src/lib/data/stores/view.ts
- src/lib/components/VerseOnImage.svelte
🚧 Files skipped from review as they are similar to previous changes (2)
- src/routes/+layout.svelte
- src/lib/components/TextSelectionToolbar.svelte
If the timing files exist, they will be a local resource and will not require a cross-site fetch. |
|
In that case, what is going on with Mark 13? It seems to have remote audio, but it also seems to have timing files (It highlights verses as they are read in the audio). |
It can have remote audio with local timing files. The timing files will always be local. Sharing a remote file really isn't an option. For FCBH Audio, it requires an API call to fetch the audio. |
|
What should I do about remote files, then? Should it just treat the chapter like one that doesn't have an audio file at all for the purposes of sharing? |
On the Native app, the audio file has to be downloaded before it can be shared. The same thing should be true for the PWA. |
|
How are we going to handle downloading audio files? They need to be downloaded somewhere where they can be fetched in the future, especially for offline use, right? Do we already have an issue for that? I was thinking I'd seen an issue about downloading audio, but now I can't find it. |
I couldn't find any associated issues either. I messaged @davidmoore1 to ask him to add the Issue(s) since he's the one who volunteered to document the feature. Should this PR move ahead without that feature? Or should we wait for it to be implemented? Thoughts @chrisvire? |
|
I think this needs to wait on #1036 to be implemented. I am marking this as a draft PR until then. |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/routes/+layout.svelte (1)
65-99: 🎯 Functional Correctness | 🔴 Critical | ⚡ Quick winMissing
ModalType.Sharecase in the modal dispatch switch.The
showModal()function handles everyModalTypeexceptShare. WhenTextSelectionToolbarpushes aModalType.Shareentry onto the modal store, the switch will fall through silently and theShareSelectormodal will never open. Add a case that callsshareSelector?.showModal().🐛 Proposed fix: add Share case to the switch
case ModalType.PlaybackSpeed: audioPlaybackSpeed?.showModal(); break; + case ModalType.Share: + shareSelector?.showModal(); + break; case ModalType.Collection: collectionModal?.showModal( data as Parameters<CollectionModal['showModal']>[0] ); break;🤖 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/`+layout.svelte around lines 65 - 99, The showModal() dispatch in +layout.svelte is missing the ModalType.Share branch, so ShareSelector never opens when that modal is queued. Update the switch in showModal() to handle ModalType.Share alongside the other modal cases, and invoke shareSelector?.showModal() there using the existing shareSelector reference.
🤖 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/routes/`+layout.svelte:
- Around line 148-168: The ShareSelector modal is declared and used in the
layout but never mounted, so shareSelector stays undefined and showModal calls
do nothing. Render the ShareSelector component in the same modal container as
the other dialogs in +layout.svelte and bind it with bind:this={shareSelector},
using the existing shareSelector state variable and ShareSelector import so it
can be opened properly.
---
Outside diff comments:
In `@src/routes/`+layout.svelte:
- Around line 65-99: The showModal() dispatch in +layout.svelte is missing the
ModalType.Share branch, so ShareSelector never opens when that modal is queued.
Update the switch in showModal() to handle ModalType.Share alongside the other
modal cases, and invoke shareSelector?.showModal() there using the existing
shareSelector reference.
🪄 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: 279d00d9-a682-4918-8fc2-1bb0e30d7e28
📒 Files selected for processing (5)
src/lib/components/ShareSelector.sveltesrc/lib/components/TextSelectionToolbar.sveltesrc/lib/components/VerseOnImage.sveltesrc/lib/data/stores/view.tssrc/routes/+layout.svelte
✅ Files skipped from review due to trivial changes (1)
- src/lib/components/VerseOnImage.svelte
🚧 Files skipped from review as they are similar to previous changes (3)
- src/lib/components/TextSelectionToolbar.svelte
- src/lib/data/stores/view.ts
- src/lib/components/ShareSelector.svelte
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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/routes/+layout.svelte (1)
65-99: 🎯 Functional Correctness | 🔴 Critical | ⚡ Quick winMissing
ModalType.Sharecase in the modal dispatch switch.The
showModal()function handles everyModalTypeexceptShare. WhenTextSelectionToolbarpushes aModalType.Shareentry onto the modal store, the switch will fall through silently and theShareSelectormodal will never open. Add a case that callsshareSelector?.showModal().🐛 Proposed fix: add Share case to the switch
case ModalType.PlaybackSpeed: audioPlaybackSpeed?.showModal(); break; + case ModalType.Share: + shareSelector?.showModal(); + break; case ModalType.Collection: collectionModal?.showModal( data as Parameters<CollectionModal['showModal']>[0] ); break;🤖 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/`+layout.svelte around lines 65 - 99, The showModal() dispatch in +layout.svelte is missing the ModalType.Share branch, so ShareSelector never opens when that modal is queued. Update the switch in showModal() to handle ModalType.Share alongside the other modal cases, and invoke shareSelector?.showModal() there using the existing shareSelector reference.
🤖 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/routes/`+layout.svelte:
- Around line 148-168: The ShareSelector modal is declared and used in the
layout but never mounted, so shareSelector stays undefined and showModal calls
do nothing. Render the ShareSelector component in the same modal container as
the other dialogs in +layout.svelte and bind it with bind:this={shareSelector},
using the existing shareSelector state variable and ShareSelector import so it
can be opened properly.
---
Outside diff comments:
In `@src/routes/`+layout.svelte:
- Around line 65-99: The showModal() dispatch in +layout.svelte is missing the
ModalType.Share branch, so ShareSelector never opens when that modal is queued.
Update the switch in showModal() to handle ModalType.Share alongside the other
modal cases, and invoke shareSelector?.showModal() there using the existing
shareSelector reference.
🪄 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: 279d00d9-a682-4918-8fc2-1bb0e30d7e28
📒 Files selected for processing (5)
src/lib/components/ShareSelector.sveltesrc/lib/components/TextSelectionToolbar.sveltesrc/lib/components/VerseOnImage.sveltesrc/lib/data/stores/view.tssrc/routes/+layout.svelte
✅ Files skipped from review due to trivial changes (1)
- src/lib/components/VerseOnImage.svelte
🚧 Files skipped from review as they are similar to previous changes (3)
- src/lib/components/TextSelectionToolbar.svelte
- src/lib/data/stores/view.ts
- src/lib/components/ShareSelector.svelte
🛑 Comments failed to post (1)
src/routes/+layout.svelte (1)
148-168: 🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
<ShareSelector>component is never rendered in the template.
ShareSelectoris imported (line 12) and ashareSelectorstate variable is declared (line 113), but the component is never mounted. Withoutbind:this={shareSelector}, the variable will remainundefinedand any call toshareSelector?.showModal()will be a no-op. The component must be rendered alongside the other modal components.🐛 Proposed fix: render ShareSelector in the modal container
<AudioPlaybackSpeed bind:this={audioPlaybackSpeed} /> <FontSelector bind:this={fontSelector} /> + <ShareSelector bind:this={shareSelector} /> </div>📝 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.<div> <!--Div containing the popup modals triggered by the navBar buttons and SideBar entries --> {`#if` isSAB} <!-- Add Note Menu --> <NoteDialog bind:this={noteDialog} /> {/if} <!-- Text Appearance Options Menu --> <TextAppearanceSelector bind:this={textAppearanceSelector} vertOffset={NAVBAR_HEIGHT} /> <CollectionModal bind:this={collectionModal} /> <PlanStopDialog bind:this={planStopDialog} bind:planId={planStopId} vertOffset={NAVBAR_HEIGHT} /> <AudioPlaybackSpeed bind:this={audioPlaybackSpeed} /> <FontSelector bind:this={fontSelector} /> <ShareSelector bind:this={shareSelector} /> </div>🤖 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/`+layout.svelte around lines 148 - 168, The ShareSelector modal is declared and used in the layout but never mounted, so shareSelector stays undefined and showModal calls do nothing. Render the ShareSelector component in the same modal container as the other dialogs in +layout.svelte and bind it with bind:this={shareSelector}, using the existing shareSelector state variable and ShareSelector import so it can be opened properly.
95c5f17 to
f3ab18f
Compare
| <AudioIcon.Volume /> | ||
| {$t['Share_Audio']} | ||
| </button> | ||
| <!--<button |
There was a problem hiding this comment.
Why is this commented out?
There was a problem hiding this comment.
It's commented out because video download has not yet been implemented, but that is where the button would go if it was implemented.
|
Ack! I somehow forgot to actually test it... For some reason it always fails the share attempt on WEB then goes to download... (of course the behavior of navigator.share is platform specific, so idk how much success you'll have reproducing...) Also, I would recommend making the share portion of shareAudio in ShareSelector be more DRY with share.ts |
|
I think that the reason is fails the share attempt on WEB is that the files you were trying to share were probably local files. It seems that the file types that I had it prioritize exporting are ones that Brave does not allow sharing for. I am also finding that the share UI doesn't seem to be very helpful for actually sharing the file, at least on Brave. I have not managed to share the file yet in my testing. Do you have any suggestions for something better than relying on navigator.share? |
I'm not sure why local files make a difference??? I'm not sure if there's something better than navigator.share or not... May be a question for @davidmoore1 ? |
|
Local files make a difference because (At least when it fails to fetch) remote files share a link rather than a file, so it's just text, which is much easier to share. |
Currently it downloads rather than sharing the audio.
Audio will be downloaded if sharing fails, which occurs on most browsers.
38be1d5 to
a9ba6f3
Compare
FyreByrd
left a comment
FyreByrd
left a comment
There was a problem hiding this comment.
With a couple tweaks made in my commit, this functions quite well. Please review the changes that I made before I merge.
|
Also, the issue I ran into much earlier is because it was trying to share with an invalid MIME type (i.e. |
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
src/lib/data/share.ts (1)
26-32: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winInconsistent cancel-handling across share helpers.
shareAudionow special-casesAbortErrorto skip the download fallback when the user dismisses the native share sheet (lines 67-69), butshareText's catch (26-28) andshareImage's catch (43-45) still unconditionally fall through todownloadObject(...). Canceling a text or image share will therefore trigger an unwanted forced download, while canceling an audio share won't. This double standard is worth resolving, especially since reviewers on this PR already suggested consolidating sharing logic inshare.ts.♻️ Suggested fix: extract a shared abort check
} catch (error) { + if ((error as { name?: string })?.name === 'AbortError') { + return; // user intentionally dismissed native share UI + } console.error('Error sharing: ', error); }Apply the same guard to both
shareText's andshareImage's catch blocks (or factor all three functions' native-share-attempt/catch logic into one shared helper).Also applies to: 43-49
🤖 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/share.ts` around lines 26 - 32, Update the catch handling in shareText and shareImage to detect AbortError and return without invoking downloadObject, matching shareAudio’s cancellation behavior. Preserve the existing download fallback for other sharing errors, or extract a shared abort-check helper if consolidating the three share helpers.src/lib/components/TextAppearanceSelector.svelte (2)
206-211: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winGive each theme button an accessible name.
These empty color swatches cannot be identified by screen-reader users. Add a label and selected state.
Proposed fix
<button class="dy-btn-sm" + aria-label={`Select ${t} theme`} + aria-pressed={t === $theme} style:background-color={buttonBackground(t)}🤖 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/components/TextAppearanceSelector.svelte` around lines 206 - 211, Update the theme buttons in the TextAppearanceSelector markup to include an accessible name identifying each theme and expose whether it is currently selected, using the existing theme value and $theme state. Preserve the current click behavior while ensuring screen readers can distinguish the swatches and their selection state.
204-204: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winAvoid constructing the Tailwind grid utility at runtime.
grid-cols-{themes.length}is a runtime class and will not be generated by Tailwind, so the picker can collapse to a single column when the generated utility is missing. Use an inline grid template or a static list of Tailwind classes.🤖 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/components/TextAppearanceSelector.svelte` at line 204, Update the grid container in TextAppearanceSelector to avoid the runtime-generated grid-cols-{themes.length} class; use an inline grid-template-columns style based on themes.length or select from a static set of Tailwind grid classes, preserving the intended column count.src/lib/data/audio.ts (2)
27-30: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winDelete from the same
audioClipUrlsMRUCache, not the player cache.
cache.delete(key)targets the module-levelMRUCache<string, AudioPlayer>even though the eviction callback belongs toaudioClipUrls. Since both caches use the same${collection}-${book}-${chapter}key format, evicting old downloaded clips can also delete a liveAudioPlayerentry by key reuse, pausing and resetting its playback state. UseaudioClipUrls.delete(key)inside the eviction callback.🤖 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 27 - 30, Update the eviction callback in audioClipUrls so it deletes the evicted key from audioClipUrls itself, rather than the separate player cache variable cache. Keep URL.revokeObjectURL(item) unchanged.
872-926: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winRefresh of
curRefsmid-function doesn't extend toaudio/audioSource.After
await findAudioClip(...),curRefsis re-fetched (line 885) to guard against navigation during the async gap, butaudioandaudioSource(computed at lines 874-879, before the first await) are never recomputed. The subsequent BibleBrain fetch (894-902) then mixes the freshly re-readcurRefs.collection/book/chapterwith the possibly-staleaudioSource/getDamIdderived from the pre-navigation chapter, which could resolve the wrong DAM ID if collection/book/chapter changed during thefindAudioClipcall.Separately, this function independently calls
getBibleBrainUrlforfcbhsources — please confirm this doesn't duplicate the equivalent resolution already performed later ingetAudioSourceInfo/getAudio()when this check is called before playback.♻️ Suggested fix: recompute audio/audioSource with the refreshed refs
curRefs = get(refs); + const refreshedAudio = scriptureConfig.bookCollections + ?.find((c) => curRefs.collection === c.id) + ?.books?.find((b) => b.id === curRefs.book) + ?.audio?.find((a) => curRefs.chapter === '' + a.num); + const refreshedAudioSource = refreshedAudio + ? scriptureConfig.audio?.sources[refreshedAudio.src] + : undefined; if (!foundAudioClip) {Then use
refreshedAudio/refreshedAudioSourcein the branches below instead of the pre-awaitaudio/audioSource.🤖 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 872 - 926, Recompute the chapter audio metadata after the post-findAudioClip refs refresh in checkAudioAvailability, using refreshedAudio and refreshedAudioSource derived from the current collection, book, and chapter; use these refreshed values and the matching current references for all subsequent source and download handling. Also verify whether the fcbh getBibleBrainUrl call duplicates resolution in getAudioSourceInfo/getAudio when this check precedes playback, and remove the redundant resolution if so.
🧹 Nitpick comments (3)
src/lib/components/ShareSelector.svelte (3)
128-149: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract
pickSupportedAudioConfigto a shared module.The author's own comment on Line 149 flags this as a duplicate of
pickSupportedAudioConfiginVerseOnImage.svelte. Consider moving it to a shared location (e.g.,$lib/data/audio.ts) so both call sites stay in sync.Want me to draft the extraction and update both call sites?
🤖 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/components/ShareSelector.svelte` around lines 128 - 149, Extract the duplicated pickSupportedAudioConfig function into a shared audio module, exporting it for reuse. Remove the local implementations from ShareSelector.svelte and VerseOnImage.svelte, and update both call sites to import the shared function while preserving the existing candidate order and fallback behavior.
151-157: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
showModal(shareTextOnly: boolean)is called with a possibly-undefined value cast throughas boolean.modal.open(ModalType.Share)(no second arg, in TextSelectionToolbar.svelte) leavesdataasundefined, but the layout casts it straight tobooleanand the modal declares the param as required. Works today only becauseundefinedis falsy.
src/lib/components/ShareSelector.svelte#L151-L157: change the signature toshowModal(shareTextOnly?: boolean)to honestly reflect that callers may omit it.src/routes/+layout.svelte#L93-L95: update the cast todata as boolean | undefined(or drop the cast once the signature is optional) so TypeScript doesn't hide the real runtime type.🤖 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/components/ShareSelector.svelte` around lines 151 - 157, Make showModal in ShareSelector.svelte accept an optional shareTextOnly boolean, reflecting callers that omit modal data. In src/lib/components/ShareSelector.svelte lines 151-157, update showModal’s parameter; in src/routes/+layout.svelte lines 93-95, remove the unsafe boolean cast or change it to boolean | undefined.
44-127: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAudio share path has no analytics logging.
shareSelectedText()logs vialogShareContent('Text', ...), butshareAudioFile()never callslogShareContentfor the new audio path, so audio shares won't be tracked.🤖 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/components/ShareSelector.svelte` around lines 44 - 127, The shareAudioFile function must log successful audio shares through logShareContent, matching the existing shareSelectedText analytics behavior. Add the call after the audio share completes, using the appropriate audio share type and the same relevant reference/content context already available in the function.
🤖 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/ShareSelector.svelte`:
- Around line 122-126: Update the error path in shareAudioFile() so audio
export/share failures both retain the existing console.error logging and provide
user-facing feedback through the component’s established notification or
error-display mechanism. Ensure failures from missing sources, decoding/CORS
issues, and unsupported codecs reach that feedback path while preserving
audioCtx cleanup in finally.
- Around line 78-110: In the verse-trimming loop, replace the falsy `startFrame`
check with an explicit match-found state so a legitimate `startFrame === 0` is
not treated as uninitialized. Update the logic around `startFrame` and
`endFrame` to set the start only on the first matching timing entry while
allowing subsequent entries to extend `endFrame`, preserving complete
first-verse audio.
- Around line 163-167: Replace the hardcoded background-color:red in the
ShareSelector modal’s styling with the existing theme/store-based
PopupBackgroundColor used by the other selector modals, while preserving the
remaining modal styling and positioningCSS.
- Around line 113-121: Sanitize the reference value before passing it as the
filename to shareAudio in the ShareSelector share flow. Use the existing
composite reference from selectedVerses, replacing filename-invalid characters
such as the colon with safe characters while preserving the readable reference
and its appended outputFormat.fileExtension.
---
Outside diff comments:
In `@src/lib/components/TextAppearanceSelector.svelte`:
- Around line 206-211: Update the theme buttons in the TextAppearanceSelector
markup to include an accessible name identifying each theme and expose whether
it is currently selected, using the existing theme value and $theme state.
Preserve the current click behavior while ensuring screen readers can
distinguish the swatches and their selection state.
- Line 204: Update the grid container in TextAppearanceSelector to avoid the
runtime-generated grid-cols-{themes.length} class; use an inline
grid-template-columns style based on themes.length or select from a static set
of Tailwind grid classes, preserving the intended column count.
In `@src/lib/data/audio.ts`:
- Around line 27-30: Update the eviction callback in audioClipUrls so it deletes
the evicted key from audioClipUrls itself, rather than the separate player cache
variable cache. Keep URL.revokeObjectURL(item) unchanged.
- Around line 872-926: Recompute the chapter audio metadata after the
post-findAudioClip refs refresh in checkAudioAvailability, using refreshedAudio
and refreshedAudioSource derived from the current collection, book, and chapter;
use these refreshed values and the matching current references for all
subsequent source and download handling. Also verify whether the fcbh
getBibleBrainUrl call duplicates resolution in getAudioSourceInfo/getAudio when
this check precedes playback, and remove the redundant resolution if so.
In `@src/lib/data/share.ts`:
- Around line 26-32: Update the catch handling in shareText and shareImage to
detect AbortError and return without invoking downloadObject, matching
shareAudio’s cancellation behavior. Preserve the existing download fallback for
other sharing errors, or extract a shared abort-check helper if consolidating
the three share helpers.
---
Nitpick comments:
In `@src/lib/components/ShareSelector.svelte`:
- Around line 128-149: Extract the duplicated pickSupportedAudioConfig function
into a shared audio module, exporting it for reuse. Remove the local
implementations from ShareSelector.svelte and VerseOnImage.svelte, and update
both call sites to import the shared function while preserving the existing
candidate order and fallback behavior.
- Around line 151-157: Make showModal in ShareSelector.svelte accept an optional
shareTextOnly boolean, reflecting callers that omit modal data. In
src/lib/components/ShareSelector.svelte lines 151-157, update showModal’s
parameter; in src/routes/+layout.svelte lines 93-95, remove the unsafe boolean
cast or change it to boolean | undefined.
- Around line 44-127: The shareAudioFile function must log successful audio
shares through logShareContent, matching the existing shareSelectedText
analytics behavior. Add the call after the audio share completes, using the
appropriate audio share type and the same relevant reference/content context
already available in the function.
🪄 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 Plus
Run ID: 7ce76cad-4f42-48e6-a6d7-b55cc34a311e
📒 Files selected for processing (10)
src/lib/components/DownloadSelector.sveltesrc/lib/components/PlanStopDialog.sveltesrc/lib/components/ShareSelector.sveltesrc/lib/components/TextAppearanceSelector.sveltesrc/lib/components/TextSelectionToolbar.sveltesrc/lib/components/VerseOnImage.sveltesrc/lib/data/audio.tssrc/lib/data/share.tssrc/lib/data/stores/view.tssrc/routes/+layout.svelte
| } catch (error) { | ||
| console.error('Error generating audio export:', error); | ||
| } finally { | ||
| await audioCtx?.close(); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Audio-share failures are silently swallowed.
Any failure in shareAudioFile() (missing audio source, decode/CORS failure, unsupported codec) only logs to console.error — the user gets no indication their share attempt failed. Given the known CORS/remote-audio limitations called out in this PR's discussion, this path is likely to be hit in practice and should surface user-facing feedback instead of failing silently.
🤖 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/components/ShareSelector.svelte` around lines 122 - 126, Update the
error path in shareAudioFile() so audio export/share failures both retain the
existing console.error logging and provide user-facing feedback through the
component’s established notification or error-display mechanism. Ensure failures
from missing sources, decoding/CORS issues, and unsupported codecs reach that
feedback path while preserving audioCtx cleanup in finally.
4 participants
Resolves #377. If there is audio timing data, this causes the share button when text is selected to open a modal that gives the user a choice between sharing text and sharing audio.
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Improvements
Bug Fixes