feat: verify if audio is reachable during conversion#1059
Conversation
The full convert config process had been running twice in order to have the program type at the beginning.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThis PR centralizes configuration type guards and feature lookups, adds typed audio-source and BibleBrain media utilities, refactors conversion to extract program types and verify media access, and moves quiz randomization helpers into a shared module. ChangesConfig conversion and media verification
Shared configuration and audio utilities
Shared quiz array utilities
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant convertConfig
participant verifyMediaAvailability
participant getBibleBrainUrl
participant RemoteServer
convertConfig->>verifyMediaAvailability: await verifyMediaAvailability(config, verbose)
verifyMediaAvailability->>getBibleBrainUrl: resolve fcbh media path
getBibleBrainUrl->>RemoteServer: fetch BibleBrain API
RemoteServer-->>getBibleBrainUrl: path or error
verifyMediaAvailability->>RemoteServer: fetch media URL with OPTIONS
RemoteServer-->>verifyMediaAvailability: CORS and allowed methods
verifyMediaAvailability-->>convertConfig: verified config or error
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint install timed out. The project may have too many dependencies for the sandbox. 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 |
FyreByrd
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 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 `@convert/convertConfig.ts`:
- Around line 1388-1422: The remote verification block in convertConfig’s
Promise.all(fetch(...)) calls is unbounded and can hang or overload hosts. Add a
timeout and limit concurrent HEAD requests for the source.media verification
path, and apply the same fix to the similar verification block referenced by the
comment. Update the logic around getBibleBrainUrl/path resolution and the
fetch(..., { method: 'HEAD' }) branch so stalled requests fail fast and large
batches are processed in controlled parallelism.
- Around line 1401-1403: The download URL construction in the `source.type ===
'download'` branch ignores `source.folder`, which can make valid `AudioSource`
entries look unreachable. Update the path-building logic in `convertConfig` to
include `source.folder` when composing the HEAD/download URL, alongside the
existing `source.address` and selected filename, so download availability checks
match the actual source layout.
- Around line 1332-1335: The verification source map in convertConfig should not
exclude `fcbh` entries just because they lack `download` in `accessMethods`,
since that bypasses the `source.type === 'fcbh'` validation path. Update the
`sources` निर्माण in the `Object.entries(config.audio?.sources ?? {})` chain so
`fcbh` sources are retained in the verification set, while keeping the existing
download-based filtering for other source types. Use the `sources` map-building
logic and the `source.type` validation branch as the key places to adjust.
- Around line 1391-1397: The BibleBrain lookup in the `getBibleBrainUrl()` call
can reject before returning an `{ error }`, so `Promise.all` aborts instead of
handling the failure per item. Update the logic around the `source.type ===
'fcbh' && v.type === 'book'` path in `convertConfig.ts` to catch rejected
lookups for each media item, set `pass = false`, and continue so the intended
access-method fallback still runs. Keep the handling localized to the existing
`getBibleBrainUrl`/`res.error` flow and preserve the current logging with the
item context (`key`, `v.collection`, `v.book`, `v.chapter`).
In `@src/lib/scripts/mediaUtils.ts`:
- Around line 16-31: The BibleBrain helper currently throws on fetch, JSON
parsing, or missing data instead of consistently returning an error object.
Update the request flow in the media utility helper that builds the BibleBrain
URL and parses the response so it catches network/parse failures and also treats
a missing data[0].path as an error case. Make the helper always resolve to the
same union shape by returning { error } for failures and only returning { path }
when the response contains a valid path.
🪄 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: 08d55e98-442c-4ebe-a4e2-74eaa14b1aa4
📒 Files selected for processing (12)
config/index.d.tsconvert/convertConfig.tsconvert/index.tssrc/lib/components/ScriptureViewSofria.sveltesrc/lib/components/SearchForm.sveltesrc/lib/components/Sidebar.sveltesrc/lib/data/audio.tssrc/lib/data/stores/setting.tssrc/lib/scripts/configUtils.tssrc/lib/scripts/mediaUtils.tssrc/routes/+page.sveltesrc/routes/text/+page.svelte
|
Does this occur when running |
|
I'm guessing that since |
Yeah... I have that concern too, but I'm not quite sure what the best workaround would be... |
|
From a bit of asking a chatbot and testing some things, it seems that |
eomerdws
left a comment
eomerdws
left a comment
There was a problem hiding this comment.
You might consider changing that one variable name if it is a typo. Otherwise It looks good to me.
I do like moving getRandomItem and shuffleArray (with the refactored name) moved into its own exported location.
3 participants
Supports #1036, #992
When parsing the config, check all specified audio sources for if their associated files are reachable via the fetch API.
If one of the files fails to be fetched, the audio source has
downloadremoved as an access method and a warning is printed.If after these checks have run, an audio source has no remaining access methods an error is thrown and conversion fails.
Notes:
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Bug Fixes
Chores