Convert generated config objects to TS#959
Merged
Merged
Conversation
TODO: move as much as possible from static into gen-assets
Contributor
📝 WalkthroughWalkthroughMoves generated runtime config JS into typed TypeScript assets under src/gen-assets, introduces FeatureConfig types, updates conversion tasks to emit TS, updates many imports to ChangesConfiguration Module Migration to TypeScript and Generated Assets
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![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: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
src/lib/data/analytics.ts (3)
26-40:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winAdd null-safety for book object access.
Line 29 accesses
book.abbreviationwithout checking ifbookis undefined. SincegetBooknow uses optional chaining and can returnundefined, this will throw a runtime error when the book is not found.🛡️ Proposed fix
export function logScreenView(item: HistoryItem) { const book = getBook({ ...item }); const chapter = item.chapter; - const bookAbbrev = book.abbreviation; + const bookAbbrev = book?.abbreviation; const damId = getDamId({ book, chapter }); const params = { screenName: item.book,🤖 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/analytics.ts` around lines 26 - 40, In logScreenView, avoid accessing book.abbreviation when getBook(...) may return undefined: update logScreenView to handle a missing book (from getBook) by either returning early if book is falsy or supplying safe fallbacks (e.g., const bookAbbrev = book?.abbreviation ?? null) before calling getDamId and analytics.log; ensure getDamId is called with a valid book/chapter or skip logging if book is missing so no runtime error occurs.
61-82:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winAdd null-safety for book object access.
Lines 64 and 66 access
book.idandbook.abbreviationwithout checking ifbookis undefined. SincegetBookcan returnundefined, this will throw a runtime error.🛡️ Proposed fix
export function logAudioDuration(audioPlayer: AudioPlayer) { const bookCol = audioPlayer.collection; const book = getBook({ collection: bookCol, book: audioPlayer.book }); - const screenName = book.id; + const screenName = book?.id; const chapter = audioPlayer.chapter; - const bookAbbrev = book.abbreviation; + const bookAbbrev = book?.abbreviation; const damId = getDamId({ book, 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/analytics.ts` around lines 61 - 82, logAudioDuration currently assumes getBook(...) returns a valid book and directly accesses book.id and book.abbreviation, which can throw if book is undefined; modify logAudioDuration to null-check the result of getBook (or guard with an early return) before accessing book.id, book.abbreviation, or calling getDamId; if you choose to return early, bail out before constructing params/ calling analytics.log, otherwise provide safe defaults for screenName/bookAbbrev and only call getDamId when book is present (refer to functions logAudioDuration and getBook and the variable names book, screenName, bookAbbrev, damId).
42-59:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winAdd null-safety for book object access.
Lines 45 and 47 access
book.idandbook.abbreviationwithout checking ifbookis undefined. SincegetBookcan returnundefined, this will throw a runtime error.🛡️ Proposed fix
export function logAudioPlay(audioPlayer: AudioPlayer) { const bookCol = audioPlayer.collection; const book = getBook({ collection: bookCol, book: audioPlayer.book }); - const screenName = book.id; + const screenName = book?.id; const chapter = audioPlayer.chapter; - const bookAbbrev = book.abbreviation; + const bookAbbrev = book?.abbreviation; const damId = getDamId({ book, 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/analytics.ts` around lines 42 - 59, logAudioPlay calls getBook and then unconditionally accesses book.id and book.abbreviation which can be undefined; update logAudioPlay to null-safe the book usage by checking if book is present before accessing its properties (e.g., if book is falsy, either return early or set screenName/bookAbbrev to safe fallback values) and ensure getDamId is only called with a valid book and chapter (or guarded similarly); reference the logAudioPlay function, the getBook call, and getDamId to locate and fix the accesses.src/lib/components/Sidebar.svelte (1)
251-258:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUpdate ESLint enable comment to re-enable both disabled rules.
Line 251 disables both
svelte/no-navigation-without-baseandsvelte/no-navigation-without-resolve, but line 258 only re-enablessvelte/no-navigation-without-base. Change the enable comment to:<!-- eslint-enable svelte/no-navigation-without-base, svelte/no-navigation-without-resolve -->🤖 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/Sidebar.svelte` around lines 251 - 258, The ESLint disable/enable pair in Sidebar.svelte around the external link anchor is mismatched: the opening comment disables both svelte/no-navigation-without-base and svelte/no-navigation-without-resolve but the closing comment only re-enables svelte/no-navigation-without-base; update the closing eslint-enable comment next to the anchor (the block containing the <a href={item.link['default']} ...>) to re-enable both rules by listing svelte/no-navigation-without-base, svelte/no-navigation-without-resolve so the linter state is correctly restored.
🧹 Nitpick comments (2)
src/lib/components/LayoutOptions.svelte (1)
30-38: 💤 Low valueConsider simplifying the feature lookup pattern.
The pattern
ds?.features['bc-allow-single-pane'] ?? ds?.features['bc-layout-allow-single-pane']repeats the optional chain. While functionally correct, it could be clearer with a helper or intermediate variable if features access is common.However, the current implementation correctly handles missing
dsorfeatures, and the filter on line 90 (x.singlePane === true) properly excludes undefined values.🤖 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/LayoutOptions.svelte` around lines 30 - 38, The repeated optional chaining when resolving the single-pane feature can be simplified: inside the allDocSets mapping (function using ds) extract features into an intermediate variable (e.g., const features = ds?.features) and then compute singlePane using features?.['bc-allow-single-pane'] ?? features?.['bc-layout-allow-single-pane']; update the mapping that builds allDocSets to use that intermediate variable so lookups are clearer while preserving existing null/undefined behaviour and the downstream filter on singlePane.src/lib/data/stores/collection.ts (1)
11-13: 💤 Low valueType safety: Boolean cast could mask non-boolean feature values.
The
as booleancast on line 13 bypasses type checking. Ifbc-layout-allow-single-paneis not actually a boolean (e.g., a string like'yes'), the cast will succeed but the value will be truthy regardless of intent. Consider runtime validation if feature values can be non-boolean.🤖 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/stores/collection.ts` around lines 11 - 13, The singlePane assignment uses a raw "as boolean" cast which can mask non-boolean feature values; replace the cast with explicit runtime validation: check ds.features['bc-layout-allow-single-pane'] is of type boolean before using it, otherwise coerce/parse acceptable truthy string forms (if your feature source uses strings) or fall back to a safe default (e.g., false) and keep the existing ds.features['bc-allow-single-pane'] precedence; update the code referencing singlePane and related logic to rely on this validated boolean value rather than an unchecked cast.
🤖 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 1630-1633: The file currently exports config plus dictionaryConfig
and scriptureConfig via unsafe casts (export const config, dictionaryConfig,
scriptureConfig) which misrepresents runtime shape; change generation to export
only the single runtime value as export const config: Readonly<AppConfig> and
remove the unconditional exports dictionaryConfig and scriptureConfig, and
instead generate and export small runtime type-guard functions (e.g.,
isDictionaryConfig(config): config is DictionaryConfig and
isScriptureConfig(config): config is ScriptureConfig) or accessor functions
(e.g., getDictionaryConfig(config) / getScriptureConfig(config)) that check
config.programType at runtime and either return the properly typed value or
throw; keep AppConfig, DictionaryConfig and ScriptureConfig types but ensure any
cast to those variants is guarded by the new runtime checks rather than emitted
as unconditional as Readonly<DictionaryConfig> / Readonly<ScriptureConfig>.
In `@src/lib/components/HistoryCard.svelte`:
- Around line 18-24: The reactive assignments (docSet, bcName, bookName,
chapterVerseSeparator, reference) must guard against bc being undefined to avoid
values like "undefined_undefined" and "undefined" separators; update each
expression to use safe fallbacks (e.g., use bc?.languageCode ?? '' and bc?.id ??
'' for docSet, compute bcName as null if bc is missing or
scriptureConfig.bookCollections?.length == 1, find bookName only if bc?.books is
an array otherwise fallback to history.book or empty string, set
chapterVerseSeparator = bc?.features?.['ref-chapter-verse-separator'] ?? '' and
build reference by checking history.chapter and history.verse and using the safe
separator) so stale/mismatched history entries produce empty/null values rather
than "undefined" tokens.
In `@src/lib/components/SearchForm.svelte`:
- Around line 22-30: The code assumes dictionaryConfig.writingSystems exists
when computing specialCharacters in the isDAB branch; guard against undefined by
using a null-safe fallback (e.g., treat writingSystems as an empty object/array)
before calling Object.values so Object.values(...) cannot throw. Update the
isDAB branch that references dictionaryConfig.writingSystems (and the
specialCharacters assignment) to use optional chaining or a fallback like
dictionaryConfig.writingSystems ?? {} (or || {}) so the subsequent
filter/flatMap runs on an empty collection when writingSystems is missing.
In `@src/lib/components/StackView.svelte`:
- Line 7: StackView.svelte currently imports config and directly accesses
config.bookCollections and config.mainFeatures; update it to use scriptureConfig
(the same named export used elsewhere) and add optional chaining when accessing
bookCollections and mainFeatures (e.g.,
scriptureConfig.bookCollections?.find(...) and
scriptureConfig.mainFeatures?.some/?.map as needed) so look for references to
config.bookCollections and config.mainFeatures in this file and replace them
with scriptureConfig.bookCollections? and scriptureConfig.mainFeatures? guarded
accesses to prevent runtime errors.
In `@src/lib/data/bookmarks.ts`:
- Around line 67-69: The current computation of bookIndex accesses `.books`
after an optional `?.find()` which can be undefined; change the expression that
computes bookIndex (the variable named bookIndex that uses
scriptureConfig.bookCollections?.find((x) => x.id ===
item.collection)?.books.findIndex(...)) to guard against a missing collection by
using safe chaining or a fallback—for example call `.books?.findIndex(...)` on
the result of the `.find(...)` or fallback to an empty array before
`.findIndex`; ensure you reference scriptureConfig.bookCollections, the
`.find((x) => x.id === item.collection)` result, and item.book when making this
change.
In `@src/lib/data/highlights.ts`:
- Around line 79-81: The current chain can throw because .books is accessed on
the result of scriptureConfig.bookCollections?.find(...); update the lookup to
guard against a missing collection by using optional chaining on books (or
storing the result of scriptureConfig.bookCollections?.find(...) in a variable
like collection and checking it) and then call findIndex only when
collection?.books exists; target the expression involving bookIndex,
scriptureConfig.bookCollections, the find(...) on selectedVerse.collection, and
the subsequent .books.findIndex to implement the safer check.
In `@src/lib/data/navigation.ts`:
- Around line 42-46: The code uses this.config.bookCollections?.[0] and then
force-unwraps collection (collection!) when calling this.goto, which will throw
if bookCollections is missing or empty; update the navigation logic in the
method containing this.goto to guard against undefined collection by checking
that this.config.bookCollections and collection?.books?.[0] exist (or
return/throw early) before calling this.goto (and similarly ensure
this.docSets[0] exists), and compute the chapter with a safe fallback from
collection.books[0].chaptersN (e.g., default to '1' if chaptersN is falsy) so no
non-null assertions are required.
- Around line 143-146: The current expression may access undefined when the
collection isn't found because it uses ?.find(...) only for bookCollections but
then directly accesses .books; change to fully guarded access by either using
optional chaining at each step (e.g., ensure the result of
this.config.bookCollections?.find(...) is optional-chained before .books) or
capture the found collection in a variable (e.g., const coll =
this.config.bookCollections?.find(c => c.id === this.collection)) and then
safe-check coll?.books?.find(...) and so on to locate the book and audio by
this.book and String(x.num) === this.chapter; ensure every property access that
can be undefined uses optional chaining or an existence check to avoid runtime
exceptions.
In `@src/lib/data/notes.ts`:
- Around line 65-67: The code computes bookIndex by chaining
scriptureConfig.bookCollections?.find(...)? .books.findIndex(...) but if the
find returns undefined accessing .books will throw; change the logic in the
calculation of bookIndex (the expression that uses
scriptureConfig.bookCollections, the find callback and .books) to safely handle
a missing collection by using optional chaining on .books (e.g.,
?.books?.findIndex(...)) or defaulting the found collection to an empty
object/array before calling findIndex so that bookIndex is computed without
throwing when item.collection is not present.
In `@src/lib/data/stores/setting.ts`:
- Around line 219-222: The fallback passed to Object.keys is incorrect: replace
the array fallback with an empty object so Object.keys(config.audio?.sources ||
{}) is used in the hasAudioSourceWitbDownload computation; update the call that
references config.audio?.sources within the hasAudioSourceWitbDownload
expression (and any similar usages) to use || {} instead of || [] so the type
correctly represents an object when sources is undefined.
- Around line 202-204: The fallback for Object.keys is incorrect: replace the
array fallback with an object so Object.keys is called on an object when
config.audio?.sources is undefined; update the expression that uses
Object.keys(config.audio?.sources || []) in the code that checks accessMethods
length (the line referencing config.audio?.sources and accessMethods) to use
Object.keys(config.audio?.sources || {}) instead so the types and semantics are
correct.
---
Outside diff comments:
In `@src/lib/components/Sidebar.svelte`:
- Around line 251-258: The ESLint disable/enable pair in Sidebar.svelte around
the external link anchor is mismatched: the opening comment disables both
svelte/no-navigation-without-base and svelte/no-navigation-without-resolve but
the closing comment only re-enables svelte/no-navigation-without-base; update
the closing eslint-enable comment next to the anchor (the block containing the
<a href={item.link['default']} ...>) to re-enable both rules by listing
svelte/no-navigation-without-base, svelte/no-navigation-without-resolve so the
linter state is correctly restored.
In `@src/lib/data/analytics.ts`:
- Around line 26-40: In logScreenView, avoid accessing book.abbreviation when
getBook(...) may return undefined: update logScreenView to handle a missing book
(from getBook) by either returning early if book is falsy or supplying safe
fallbacks (e.g., const bookAbbrev = book?.abbreviation ?? null) before calling
getDamId and analytics.log; ensure getDamId is called with a valid book/chapter
or skip logging if book is missing so no runtime error occurs.
- Around line 61-82: logAudioDuration currently assumes getBook(...) returns a
valid book and directly accesses book.id and book.abbreviation, which can throw
if book is undefined; modify logAudioDuration to null-check the result of
getBook (or guard with an early return) before accessing book.id,
book.abbreviation, or calling getDamId; if you choose to return early, bail out
before constructing params/ calling analytics.log, otherwise provide safe
defaults for screenName/bookAbbrev and only call getDamId when book is present
(refer to functions logAudioDuration and getBook and the variable names book,
screenName, bookAbbrev, damId).
- Around line 42-59: logAudioPlay calls getBook and then unconditionally
accesses book.id and book.abbreviation which can be undefined; update
logAudioPlay to null-safe the book usage by checking if book is present before
accessing its properties (e.g., if book is falsy, either return early or set
screenName/bookAbbrev to safe fallback values) and ensure getDamId is only
called with a valid book and chapter (or guarded similarly); reference the
logAudioPlay function, the getBook call, and getDamId to locate and fix the
accesses.
---
Nitpick comments:
In `@src/lib/components/LayoutOptions.svelte`:
- Around line 30-38: The repeated optional chaining when resolving the
single-pane feature can be simplified: inside the allDocSets mapping (function
using ds) extract features into an intermediate variable (e.g., const features =
ds?.features) and then compute singlePane using
features?.['bc-allow-single-pane'] ?? features?.['bc-layout-allow-single-pane'];
update the mapping that builds allDocSets to use that intermediate variable so
lookups are clearer while preserving existing null/undefined behaviour and the
downstream filter on singlePane.
In `@src/lib/data/stores/collection.ts`:
- Around line 11-13: The singlePane assignment uses a raw "as boolean" cast
which can mask non-boolean feature values; replace the cast with explicit
runtime validation: check ds.features['bc-layout-allow-single-pane'] is of type
boolean before using it, otherwise coerce/parse acceptable truthy string forms
(if your feature source uses strings) or fall back to a safe default (e.g.,
false) and keep the existing ds.features['bc-allow-single-pane'] precedence;
update the code referencing singlePane and related logic to rely on this
validated boolean value rather than an unchecked cast.
🪄 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: a37d4fad-9574-4868-8980-4b40bee149ed
📒 Files selected for processing (66)
.gitignoreconfig/index.d.tsconvert/convertConfig.tsconvert/tests/dab/convertConfigDAB.test.tspackage.jsonsrc/app.d.tssrc/lib/components/AudioBar.sveltesrc/lib/components/BookSelector.sveltesrc/lib/components/BookTabs.sveltesrc/lib/components/BottomNavigationBar.sveltesrc/lib/components/ChapterSelector.sveltesrc/lib/components/CollectionSelector.sveltesrc/lib/components/FontList.sveltesrc/lib/components/HistoryCard.sveltesrc/lib/components/HtmlBookView.sveltesrc/lib/components/IconCard.sveltesrc/lib/components/LayoutOptions.sveltesrc/lib/components/PlayButton.sveltesrc/lib/components/ScriptureViewSofria.sveltesrc/lib/components/SearchForm.sveltesrc/lib/components/SearchResultCard.sveltesrc/lib/components/SelectGrid.sveltesrc/lib/components/Sidebar.sveltesrc/lib/components/StackView.sveltesrc/lib/components/TextAppearanceSelector.sveltesrc/lib/components/TextSelectionToolbar.sveltesrc/lib/data/analytics.tssrc/lib/data/annotation-share.tssrc/lib/data/audio.tssrc/lib/data/bookmarks.tssrc/lib/data/highlights.tssrc/lib/data/language.tssrc/lib/data/navigation.tssrc/lib/data/notes.tssrc/lib/data/quiz.tssrc/lib/data/scripture.jssrc/lib/data/stores/analytics.tssrc/lib/data/stores/collection.tssrc/lib/data/stores/plan.tssrc/lib/data/stores/reference.tssrc/lib/data/stores/setting.tssrc/lib/lexicon/components/EntryView.sveltesrc/lib/scripts/configUtils.tssrc/lib/scripts/numeralSystem.tssrc/lib/scripts/scripture-reference-utils.tssrc/lib/search/data/repositories/config-repository.tssrc/lib/video/index.tssrc/routes/+layout.sveltesrc/routes/+page.sveltesrc/routes/about/+page.sveltesrc/routes/bookmarks/+page.jssrc/routes/contents/[id]/+page.sveltesrc/routes/highlights/+page.jssrc/routes/history/+page.jssrc/routes/lexicon/+layout.sveltesrc/routes/lexicon/+layout.tssrc/routes/lexicon/+page.sveltesrc/routes/notes/+page.jssrc/routes/plans/+page.sveltesrc/routes/plans/[id]/+page.sveltesrc/routes/quiz/[collection]/[id]/+page.sveltesrc/routes/quiz/[collection]/[id]/+page.tssrc/routes/search/[collection]/[[savedResults]]/+page.tssrc/routes/share/+page.jssrc/routes/share/+page.sveltesrc/routes/text/+page.svelte
Contributor
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 (3)
src/lib/data/stores/interface.js (1)
6-20:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winPotential runtime crash:
directionmay beundefinedbefore calling.toLowerCase().The optional chaining on lines 13 and 16–17 prevents crashes when accessing nested config properties, but if those chains short-circuit to
undefined,directionbecomesundefined. Line 19 will then throw a TypeError when calling.toLowerCase()onundefined.This can occur when:
- Line 13:
config.interfaceLanguagesis undefined orwritingSystems[code]is undefined.- Lines 16–17: the collection is not found, or it has no
style, orstylehas notextDirection.🛡️ Proposed fix to add a fallback direction
export const direction = derived([refs, userSettings], ([$refs, $userSettings]) => { var direction = config.mainFeatures['settings-app-layout-direction'] ? $userSettings['app-layout-direction'] : config.mainFeatures['app-layout-direction']; if (direction === 'interface-language') { const code = $userSettings['interface-language']; if (code) { direction = config.interfaceLanguages?.writingSystems[code].textDirection; } } else if (direction === 'text') { direction = scriptureConfig.bookCollections?.find((x) => x.id === $refs.collection)?.style ?.textDirection; } - return direction.toLowerCase(); + return direction?.toLowerCase() ?? 'ltr'; });🤖 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/stores/interface.js` around lines 6 - 20, The derived store "direction" can become undefined before calling .toLowerCase(); update the derived([refs, userSettings], ...) implementation to ensure a safe fallback (e.g. default to 'ltr' or another sensible string) whenever config.interfaceLanguages?.writingSystems[code]?.textDirection or scriptureConfig.bookCollections...style?.textDirection resolves to undefined, and call .toLowerCase() only on that guaranteed string; modify the logic inside the exported const direction (the derived callback referencing $refs, $userSettings, config.interfaceLanguages, and scriptureConfig.bookCollections) to coalesce undefined to the fallback prior to returning.src/lib/data/stores/theme.js (1)
49-63:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd fallback for undefined
config.styles.The optional chaining operator returns
undefinedwhenconfig.stylesis undefined, not the empty object{}passed toreduce. This means the derived storeswill returnundefinedinstead of an empty object, which could cause runtime errors when code tries to access style properties like$s['styleName'].This is inconsistent with line 6, which correctly uses
?? []to provide a fallback. Add?? {}after thereducecall to ensure the store always returns an object.🛡️ Proposed fix to add fallback
export const s = derived(themeColors, ($themeColors) => { - return config.styles?.reduce((styleProperties, style) => { + return (config.styles?.reduce((styleProperties, style) => { const properties = style.properties; let newProperties = { ...properties }; if (Object.prototype.hasOwnProperty.call(newProperties, 'background-color')) { newProperties['background-color'] = resolveColor( newProperties['background-color'], $themeColors ); } if (Object.prototype.hasOwnProperty.call(newProperties, 'color')) { newProperties['color'] = resolveColor(newProperties['color'], $themeColors); } styleProperties[style.name] = newProperties; return styleProperties; - }, {}); + }, {})) ?? {}; });🤖 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/stores/theme.js` around lines 49 - 63, The reduce call on config.styles can return undefined when config.styles is undefined; update the return expression so the derived store always yields an object by appending the nullish-coalescing fallback (?? {}) after the reduce invocation (i.e., change the current return of config.styles?.reduce(..., {}) to config.styles?.reduce(..., {}) ?? {}), keeping the existing logic around styleProperties, style.name, resolveColor and the properties copy intact.src/routes/plans/[id]/+page.js (1)
6-12:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winAdd a guard to handle when
planConfigis undefined.The code path where
allPlansis empty or no matching plan exists will causeplanConfigto be undefined. When passed togetPlanDataon line 12, it immediately accessesplanConfig.jsonFilenamewithout null-checking, causing a runtime error. Add a guard clause before callinggetPlanData, or ensuregetPlanDatavalidates thatplanConfigis defined.🤖 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/plans/`[id]/+page.js around lines 6 - 12, The code does not handle the case where planConfig is undefined before calling getPlanData; add a guard after computing planConfig (from allPlans.find(...)) that checks for a missing plan and returns or throws an appropriate response (e.g., 404 or redirect) instead of calling getPlanData with undefined, or alternatively update getPlanData to validate its planConfig param and throw a clear error; reference planConfig, allPlans, params.id and getPlanData when making the change so the missing-plan path is handled explicitly.
♻️ Duplicate comments (1)
src/lib/data/highlights.ts (1)
79-81:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winGuard
.bookswith optional chaining to avoid runtime throw.On Line 81, if no matching collection is found, accessing
.booksthrows. This is the same issue previously reported and still present.🔧 Safe lookup
const bookIndex = scriptureConfig.bookCollections ?.find((x) => x.id === selectedVerse.collection) - ?.books.findIndex((x) => x.id === selectedVerse.book); + ?.books?.findIndex((x) => x.id === selectedVerse.book);🤖 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/highlights.ts` around lines 79 - 81, The lookup for bookIndex can throw when no collection is found because it accesses .books directly; update the chain in the expression that computes bookIndex (which references scriptureConfig.bookCollections, selectedVerse.collection, and selectedVerse.book) to guard .books with optional chaining (or use a safe fallback array) so that you call findIndex only if books exists and return a safe default (e.g. -1) when not found.
🧹 Nitpick comments (2)
src/lib/data/stores/theme.js (1)
13-13: ⚡ Quick winUse strict equality for string comparison.
The loose equality operator
==can cause unexpected behavior due to type coercion. Since theme names are strings, use strict equality===instead.♻️ Proposed fix
- const theme = config.themes?.find((x) => x.name == $theme); + const theme = config.themes?.find((x) => x.name === $theme);Also applies to: 19-19
🤖 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/stores/theme.js` at line 13, Replace loose equality with strict equality for theme name comparisons: update the finder call in const theme = config.themes?.find((x) => x.name == $theme) to use ===, and change the other occurrence noted (the comparison at line 19) to use === as well so both comparisons use strict string equality; keep the predicate and variable names (x.name and $theme) unchanged.src/lib/data/highlights.ts (1)
90-97: ⚡ Quick winRename exported type
HighlighSearchtoHighlightSearch.The current exported type name has a typo and leaks into public API/signatures (
findHighlight,removeHighlights). Fixing it now avoids propagating a misspelled contract.✏️ Suggested rename
-export type HighlighSearch = { +export type HighlightSearch = { collection: string; book: string; chapter: string; verse: string; }; -export async function findHighlight(item: HighlighSearch) { +export async function findHighlight(item: HighlightSearch) { ... -export async function removeHighlights(selectedVerses: HighlighSearch[]) { +export async function removeHighlights(selectedVerses: HighlightSearch[]) {Also applies to: 125-125
🤖 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/highlights.ts` around lines 90 - 97, Rename the exported type HighlighSearch to HighlightSearch and update all usages to match the correct spelling: change the type declaration export type HighlighSearch to export type HighlightSearch and update function signatures and references such as findHighlight(item: HighlighSearch) and any other consumers like removeHighlights to accept HighlightSearch instead; ensure any imports/exports and tests are updated to the new identifier so the public API no longer exposes the misspelled name.
🤖 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/highlights.ts`:
- Line 2: The import path "import { scriptureConfig } from '$assets/config';" is
unresolved; update the import in src/lib/data/highlights.ts to the correct
generated/config alias (e.g. replace '$assets/config' with the actual module
path such as '$gen-assets/config' or the real relative path to the generated
config) and/or add the corresponding path alias in tsconfig/vite so the resolver
can find scriptureConfig; ensure the symbol scriptureConfig remains imported by
name and run typecheck to confirm resolution.
In `@src/lib/data/stores/localization.js`:
- Line 22: The code accesses config.translationMappings.defaultLang without
optional chaining which can throw if translationMappings is null/undefined;
update the expression in the localization lookup (the line using
config.translationMappings?.mappings[key][config.translationMappings.defaultLang]
and related logic) to safely use optional chaining for defaultLang (e.g.,
config.translationMappings?.defaultLang) so that the fallback lookup mirrors the
other safe accesses to config.translationMappings?.mappings and does not cause a
TypeError when translationMappings is missing.
- Line 22: The code re-reads config.translationMappings.defaultLang instead of
using the already-computed languageDefault variable; update the expression that
accesses
config.translationMappings?.mappings[key][config.translationMappings.defaultLang]
to use languageDefault (i.e.,
config.translationMappings?.mappings[key][languageDefault]) so the
function/logic that references languageDefault reuses that variable consistently
and avoids duplicate access.
In `@src/lib/data/stores/scripture.js`:
- Around line 68-74: The code reads scriptureConfig.bookCollections?.find(...)?
.features['ref-chapter-verse-separator'] without guarding that features exists,
which can throw or produce undefined separators; update the three
reference-formatting sites where separator and bookName are computed to safely
access features (e.g., use optional chaining like
.features?.['ref-chapter-verse-separator'] or check for features before
indexing) and provide a sensible default separator when missing (e.g.,
fallbackSeparator = ':' or ' '), then use that fallback for the variable named
separator and ensure bookName computation still falls back to item.book; apply
the same pattern to the other two identical blocks that compute
separator/bookName so no reference becomes "undefined".
---
Outside diff comments:
In `@src/lib/data/stores/interface.js`:
- Around line 6-20: The derived store "direction" can become undefined before
calling .toLowerCase(); update the derived([refs, userSettings], ...)
implementation to ensure a safe fallback (e.g. default to 'ltr' or another
sensible string) whenever
config.interfaceLanguages?.writingSystems[code]?.textDirection or
scriptureConfig.bookCollections...style?.textDirection resolves to undefined,
and call .toLowerCase() only on that guaranteed string; modify the logic inside
the exported const direction (the derived callback referencing $refs,
$userSettings, config.interfaceLanguages, and scriptureConfig.bookCollections)
to coalesce undefined to the fallback prior to returning.
In `@src/lib/data/stores/theme.js`:
- Around line 49-63: The reduce call on config.styles can return undefined when
config.styles is undefined; update the return expression so the derived store
always yields an object by appending the nullish-coalescing fallback (?? {})
after the reduce invocation (i.e., change the current return of
config.styles?.reduce(..., {}) to config.styles?.reduce(..., {}) ?? {}), keeping
the existing logic around styleProperties, style.name, resolveColor and the
properties copy intact.
In `@src/routes/plans/`[id]/+page.js:
- Around line 6-12: The code does not handle the case where planConfig is
undefined before calling getPlanData; add a guard after computing planConfig
(from allPlans.find(...)) that checks for a missing plan and returns or throws
an appropriate response (e.g., 404 or redirect) instead of calling getPlanData
with undefined, or alternatively update getPlanData to validate its planConfig
param and throw a clear error; reference planConfig, allPlans, params.id and
getPlanData when making the change so the missing-plan path is handled
explicitly.
---
Duplicate comments:
In `@src/lib/data/highlights.ts`:
- Around line 79-81: The lookup for bookIndex can throw when no collection is
found because it accesses .books directly; update the chain in the expression
that computes bookIndex (which references scriptureConfig.bookCollections,
selectedVerse.collection, and selectedVerse.book) to guard .books with optional
chaining (or use a safe fallback array) so that you call findIndex only if books
exists and return a safe default (e.g. -1) when not found.
---
Nitpick comments:
In `@src/lib/data/highlights.ts`:
- Around line 90-97: Rename the exported type HighlighSearch to HighlightSearch
and update all usages to match the correct spelling: change the type declaration
export type HighlighSearch to export type HighlightSearch and update function
signatures and references such as findHighlight(item: HighlighSearch) and any
other consumers like removeHighlights to accept HighlightSearch instead; ensure
any imports/exports and tests are updated to the new identifier so the public
API no longer exposes the misspelled name.
In `@src/lib/data/stores/theme.js`:
- Line 13: Replace loose equality with strict equality for theme name
comparisons: update the finder call in const theme = config.themes?.find((x) =>
x.name == $theme) to use ===, and change the other occurrence noted (the
comparison at line 19) to use === as well so both comparisons use strict string
equality; keep the predicate and variable names (x.name and $theme) unchanged.
🪄 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: 7cdb43b1-06b9-4ba9-a0ff-148518cd74f8
📒 Files selected for processing (13)
.prettierignoresrc/lib/components/HistoryCard.sveltesrc/lib/components/StackView.sveltesrc/lib/data/highlights.tssrc/lib/data/navigation.tssrc/lib/data/stores/audio.jssrc/lib/data/stores/interface.jssrc/lib/data/stores/localization.jssrc/lib/data/stores/scripture.jssrc/lib/data/stores/setting.tssrc/lib/data/stores/theme.jssrc/routes/plans/[id]/+page.jssrc/routes/plans/[id]/settings/+page.js
🚧 Files skipped from review as they are similar to previous changes (3)
- src/lib/data/navigation.ts
- src/lib/components/HistoryCard.svelte
- src/lib/data/stores/setting.ts
FyreByrd
changed the title
FyreByrd
changed the title
eomerdws
approved these changes
May 18, 2026
Contributor
eomerdws
left a comment
eomerdws
left a comment
There was a problem hiding this comment.
Looking through this PR it all looks straight forward. I couldn't find anything that failed building or testing several apps, nor could I find any glaring code issues that code rabbit hadn't already brought up.
Looks good.
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.
Changes:
AppConfigtypeScriptureConfigandDictionaryConfigSummary by CodeRabbit
Refactor
Style
Bug Fixes