Implement Audio Sharing for Selected Text#992
Conversation
📝 WalkthroughWalkthroughThis PR introduces a new ShareSelector modal that lets users share selected verses as text or export them as trimmed audio. The modal replaces the previous VerseOnImage modal in the type registry, integrates with TextSelectionToolbar to trigger on verse selection, and wires into the layout for dispatch and rendering. Audio sharing supports remote URL passthrough and local decode/trim/encode workflows with fallback to download. ChangesAudio and text sharing modal
Sequence Diagram(s)sequenceDiagram
participant User
participant ShareSelector
participant TextShare as shareText
participant AudioSource
participant AudioContext
participant Mediabunny
participant Navigator as navigator.share
participant Fallback as Download link
rect rgba(200, 150, 255, 0.5)
User->>ShareSelector: Click Share Text
ShareSelector->>ShareSelector: Build reference & message
ShareSelector->>TextShare: shareText(filename, message)
ShareSelector->>ShareSelector: logShareContent()
end
rect rgba(150, 200, 255, 0.5)
User->>ShareSelector: Click Share Audio
ShareSelector->>ShareSelector: pickSupportedAudioConfig()
ShareSelector->>AudioSource: getAudioSourceInfo()
alt Remote source (http/https)
AudioSource->>Navigator: navigator.share({ url })
alt Share succeeded
Navigator->>User: Share dialog
else Share failed
ShareSelector->>Fallback: Click download link
end
else Local source
ShareSelector->>AudioContext: Fetch & decode audio
AudioContext->>AudioContext: Trim per verse (timing data)
AudioContext->>Mediabunny: Encode to aac/opus/wav
Mediabunny->>ShareSelector: File blob
ShareSelector->>Navigator: navigator.canShare({ files: [File] })
alt canShare true
Navigator->>User: Share dialog
else canShare false
ShareSelector->>Fallback: Create download link
end
end
ShareSelector->>AudioContext: Close context
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
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 |
TheNonPirate
force-pushed
the
feature/share-audio/377
branch
from
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. 
|
TheNonPirate
force-pushed
the
feature/share-audio/377
branch
from
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.
|
Currently it downloads rather than sharing the audio.
Audio will be downloaded if sharing fails, which occurs on most browsers.
|
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). |
TheNonPirate
force-pushed
the
feature/share-audio/377
branch
from
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
3 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