Convert svelte components and pages to TS#961
Conversation
|
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 (3)
📝 WalkthroughWalkthroughThis PR converts many Svelte files to TypeScript and Svelte 5 runes, tightens config/store typings (FeatureConfig, PlanItem, SettingsCategory, Layout, Reference), replaces component tab rendering with snippet-based snippets, adds optional chaining and null-safe guards across UI, and updates routes to typed Props with derived state. ChangesTypeScript and Svelte 5 Migration
Estimated code review effort 🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Suggested reviewers
✨ Finishing Touches🧪 Generate unit tests (beta)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 20
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
src/routes/history/+page.svelte (1)
9-14:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winCritical: assigning to a
$derivedvalue will throw at runtime.
historyis now a$derivedbinding, butonClearHistorystill doeshistory = [];on line 13. In Svelte 5 runes, assigning to a derived raises astate_unsafe_mutationerror, so clearing history will fail in production.Either:
- Keep
historyas$state(...)and re-initialize it fromdata.history(as before), or- Make
historyderived from a writable signal thatonClearHistoryresets (e.g., aclearedflag, or haveclearHistory()mutatedata.history).🐛 Proposed fix (revert to $state)
- let history = $derived([...data.history].reverse()); + let history = $state([...data.history].reverse()); async function onClearHistory() { await clearHistory(); history = []; }🤖 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/history/`+page.svelte around lines 9 - 14, The runtime error is caused by assigning to the $derived signal `history` in `onClearHistory` (history = []), which is not allowed; either revert `history` back to a writable signal (use `$state` for `history` and initialize it from `data.history` / [...data.history].reverse() so `onClearHistory` can set it to []), or keep `history` derived but make it depend on a writable source that `onClearHistory` updates (e.g., a `cleared` writable or have `clearHistory()` mutate `data.history`) so `onClearHistory` no longer assigns directly to the derived `history`.src/routes/lexicon/+page.svelte (1)
169-169:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
{#eachcurrentAlphabet as letter}will throw ifcurrentAlphabetis undefined.The derived
currentAlphabetresolves toalphabets.vernacular(which isdata.vernacularAlphabet) oralphabets.reversal[currentReversal.languageId]; the rest of the script already treats both as possibly undefined (e.g.,alphabets.vernacular?.[0]on line 37,currentAlphabet?.indexOf(...)on line 95). Svelte's{#each}requires an iterable, so default to[]here to match the rest of the null-safe pass.🛡️ Proposed fix
- {`#each` currentAlphabet as letter} + {`#each` currentAlphabet ?? [] as letter}🤖 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/lexicon/`+page.svelte at line 169, The {`#each` currentAlphabet as letter} will throw if currentAlphabet is undefined; update the iteration to guard with a default empty array (e.g., iterate over currentAlphabet ?? []) so the template is null-safe and matches other checks around alphabets, currentAlphabet, and alphabets.reversal (referenced symbols: currentAlphabet, alphabets, currentReversal).src/routes/text/+page.svelte (2)
100-102:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUnsubscribed
refs.subscribeleaks subscriptions.This raw subscription is started during component init but never torn down — each navigation to
text/+pageadds another listener that lives for the lifetime of the page session. Use the$refsauto-subscription via$effect, which Svelte will clean up automatically, or capture the unsubscribe and call it fromonDestroy.🛠️ Proposed fix
- refs.subscribe((value) => { - savedScrollPosition = 0; - }); + $effect(() => { + // re-run whenever refs changes + void $refs; + savedScrollPosition = 0; + });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/routes/text/`+page.svelte around lines 100 - 102, The raw subscription to refs via refs.subscribe(...) leaks because it is never unsubscribed; replace it with Svelte's auto-subscription ($refs or $: reactive statement) or capture the unsubscribe function from refs.subscribe and call it in onDestroy to avoid accumulating listeners — locate the refs.subscribe usage and either change to use $refs (or a $: reactive block) to update savedScrollPosition reactively, or store the unsubscribe returned by refs.subscribe and call that unsubscribe inside onDestroy to clean up.
245-257:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win
updateTimeris a function-localundefined, soclearTimeoutis a no-op.Each call to
delayedScroll/delayedSeekdeclares a freshupdateTimerinitialized toundefined, then immediately clears it. Successive calls cannot cancel previously scheduled timers, so callbacks can stack (e.g. multiplescrollTo/seekToVerseinvocations). This is the same pattern thatnewRefScrollcorrectly closes over via an IIFE — please use the same approach here.🛠️ Proposed fix
- function delayedScroll(id: string) { - let updateTimer; - clearTimeout(updateTimer); - updateTimer = setTimeout(() => { - scrollTo(id); - }, 100); - } - function delayedSeek(id: string) { - let updateTimer; - clearTimeout(updateTimer); - updateTimer = setTimeout(() => { - seekToVerse(id); - }, 1000); - } + const delayedScroll = (() => { + let updateTimer: NodeJS.Timeout | undefined; + return (id: string) => { + clearTimeout(updateTimer); + updateTimer = setTimeout(() => scrollTo(id), 100); + }; + })(); + const delayedSeek = (() => { + let updateTimer: NodeJS.Timeout | undefined; + return (id: string) => { + clearTimeout(updateTimer); + updateTimer = setTimeout(() => seekToVerse(id), 1000); + }; + })();🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/routes/text/`+page.svelte around lines 245 - 257, Both delayedScroll and delayedSeek declare a fresh local updateTimer so clearTimeout is a no-op and previously scheduled timers aren't cancelled; change them to close over a persistent timer variable like newRefScroll does (e.g., create an IIFE or move let updateTimer outside the function body) so clearTimeout(updateTimer) actually cancels the prior timer, then assign updateTimer = setTimeout(...). Keep the callbacks calling scrollTo(id) and seekToVerse(id) respectively.src/routes/plans/[id]/+page.svelte (1)
324-372:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winEach-block on possibly-undefined values.
Both
{#eachdata.planData.items as item}(Line 324) and{#eachselectedDay?.refs as ref, index}(Line 352) iterate over expressions that are typed as possiblyundefined(the optional accessselectedDay?.refsand the matchingitems?.[0]derivation on Line 50 confirm this). Iteratingundefinedwith{#each}is a runtime error in Svelte 5. Even if the data is normally populated, narrow the type with an{#if}guard to fail soft.🐛 Proposed guards
- <ul class="dy-menu-horizontal bg-base-200 rounded-box"> - {`#each` data.planData.items as item} + <ul class="dy-menu-horizontal bg-base-200 rounded-box"> + {`#each` data.planData.items ?? [] as item} @@ - {`#each` selectedDay?.refs as ref, index} + {`#if` selectedDay} + {`#each` selectedDay.refs as ref, index} @@ </td> </tr> {/each} + {/if}🤖 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.svelte around lines 324 - 372, Wrap the two risky each-blocks with presence checks so we never call {`#each`} on undefined: guard the list rendering for data.planData.items (use an {`#if` data?.planData?.items} / {:else} fallback) and guard the selectedDay refs with {`#if` selectedDay && selectedDay.refs} before the {`#each` selectedDay?.refs as ref, index} block; keep the existing handlers (onclick, goToDailyReference, referenceCompleted, getReferenceString) unchanged but move them inside the guarded blocks so iteration only runs when the arrays are defined.
🧹 Nitpick comments (26)
src/lib/components/StackView.svelte (1)
112-112: 💤 Low valueMixed Svelte 4/5 event syntax in a TS-converted file.
Other migrated components in this PR (e.g.,
Modal.svelte,+layout.svelte) use the Svelte 5onclick={...}form, but this file still uses the legacyon:click|stopPropagation={...}syntax along with$:reactivity andexport letprops. That's fine as a scoped TS-only conversion, but worth noting if a runes-migration follow-up is planned —|stopPropagationmodifiers do not carry over to Svelte 5 listeners and would need to be replaced withe.stopPropagation()inside the handler.🤖 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/StackView.svelte` at line 112, The event listener still uses Svelte 3/4 modifier syntax on:click|stopPropagation={(e) => insideClick(e.currentTarget)} which is incompatible with Svelte 5; update the element's listener to the Svelte 5 form onclick={...} and move the stopPropagation behavior into the handler by calling e.stopPropagation() before delegating to insideClick (or create a small wrapper function like handleClick that calls e.stopPropagation() and then insideClick(e.currentTarget)); locate the usage by searching for insideClick and the on:click occurrence in StackView.svelte and replace accordingly.src/lib/components/Slider.svelte (1)
9-13: ⚡ Quick winConsider typing the remaining props.
barColor,progressColor,min, andmaxare still implicitanyafter the TS conversion, which defeats much of the migration's safety benefit for this component (e.g.,linePercentarithmetic on line 14 relies onvalue/min/maxbeing numeric).♻️ Suggested typings
- export let barColor; - export let progressColor; + export let barColor: string; + export let progressColor: string; export let value: number; - export let min; - export let max; + export let min: number; + export let max: number;🤖 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/Slider.svelte` around lines 9 - 13, The props barColor and progressColor are still implicitly any and min/max lack numeric types, which risks runtime errors in arithmetic (see linePercent calculation using value, min, max); update the exported props in Slider.svelte so barColor and progressColor are typed (e.g., string) and min and max are typed as number (and consider providing numeric defaults or marking them required) to ensure TypeScript validates the arithmetic involving value, min, and max.src/routes/+page.svelte (1)
2-26: 💤 Low valueMigrate
$app/storesto$app/statefor consistency (SvelteKit 2.15.1)
src/routes/+page.sveltestill importspagefrom$app/storesand uses$page.data..., whilesrc/routes/lexicon/+layout.svelte(and other pages) use$app/staterunes (page.route...), so the codebase mixes store and rune APIs.- SvelteKit 2.12+ deprecates
$app/stores; consider aligning this file (and alsosrc/routes/plans/[id]/settings/+page.svelte, which still uses$app/stores) to$app/state.🤖 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/`+page.svelte around lines 2 - 26, Replace the deprecated store import and usages with the SvelteKit rune API: change the import from "import { page } from '$app/stores'" to "import { page } from '$app/state'", and update all usages of the store-style $page.data to the rune-style access used elsewhere in the repo (e.g., use page.route?.data or the same property access pattern other files use), then adjust the onMount logic that reads $page.data (the audio and ref checks) to read from the page rune instead so navigateToTextReference, gotoRoute, $audioActive and $isFirstLaunch use the rune-based page values consistently with lexicon/+layout.svelte.src/lib/components/BookmarkButton.svelte (1)
6-13: 💤 Low valueUnnecessary
$derivedfor static object mappings.
icons_filledandicons_emptysimply map props to object shapes and don't perform any computation. SincefillColor,emptyColor,BookmarkIcon, andBookmarkOutlineIconare not reactive (they're imported constants and props that don't change within a render), wrapping them in$derived(...)adds unnecessary overhead. Only the final selection (line 15) needs to be derived.♻️ Remove unnecessary $derived wrappers
- const icons_filled = $derived({ + const icons_filled = { color: fillColor, Icon: BookmarkIcon - }); - const icons_empty = $derived({ + }; + const icons_empty = { color: emptyColor, Icon: BookmarkOutlineIcon - }); + };🤖 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/BookmarkButton.svelte` around lines 6 - 13, icons_filled and icons_empty are unnecessarily wrapped in $derived; replace them with plain constant objects (e.g., const icons_filled = { color: fillColor, Icon: BookmarkIcon } and const icons_empty = { color: emptyColor, Icon: BookmarkOutlineIcon }) and remove the $derived calls, leaving only the final selection (the derived value that chooses between filled/empty) as a reactive/derived expression; update any usages of icons_filled/icons_empty accordingly so the final selection logic continues to work unchanged.src/lib/components/ContentHeading.svelte (2)
25-25: ⚡ Quick winRemove debug
console.logfrom render path.
renderLastTextBoxis invoked from the template (line 86) and will log on every render. The logged expression!!exclude.find((x) => x === layout)is also the inverse of what the function actually returns (!exclude.includes(layout)), making the log misleading as well as noisy.🧹 Proposed fix
const exclude: FeatureValue[] = ['image-left-text-right', 'image-right-text-left']; - console.log(`renderLastTextBox: ${!!exclude.find((x) => x === layout)}`); return !exclude.includes(layout);🤖 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/ContentHeading.svelte` at line 25, The debug console.log inside renderLastTextBox is causing noisy and misleading output; remove the console.log line that prints `renderLastTextBox: ${!!exclude.find((x) => x === layout)}` and ensure the function returns the intended boolean using the existing logic (e.g., using exclude.includes(layout) or its negation) without logging; update references to renderLastTextBox (called from the template) if needed to avoid side-effecting prints during render.
13-18: 💤 Low valueVerify the
checkImageSizefallback is intentional (and reduce log noise).Defaulting
checkImageSizeto a fallback that logsconsole.warn('USING checkImageSizeFallback')will emit a warning on every render of everyContentHeadingwhose parent doesn't passcheckImageSize. If callers are expected to provide this prop, consider making it required; otherwise downgrade or remove the warning so it doesn't flood the console at runtime.🤖 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/ContentHeading.svelte` around lines 13 - 18, The inline fallback checkImageSizeFallback used in ContentHeading (declared alongside Props and assigned to checkImageSize) currently logs console.warn('USING checkImageSizeFallback') on every render; either make checkImageSize a required prop by removing the default assignment so callers must pass it, or silence/downgrade the noise by removing the console.warn or replacing it with a single-time warning (e.g., a debug-only or once-only log) inside checkImageSizeFallback; update the assignment and fallback function accordingly (references: checkImageSize, checkImageSizeFallback, ContentHeading, Props).src/lib/navigate/index.ts (1)
28-47: 💤 Low valueSilent no-op when required navigation fields are missing.
When any of
docSet,collection,book, orchapteris absent,navigateToTextreturns without performing any side effects or signaling the issue. Callers that previously could rely on navigation occurring will now silently fail. Consider logging aconsole.warn(or returning a boolean indicating whether navigation happened) so missing-data bugs at call sites are easier to diagnose.🤖 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/navigate/index.ts` around lines 28 - 47, The navigateToText function currently no-ops when any of docSet, collection, book, or chapter is missing; update navigateToText to surface this failure by either returning a boolean (true if navigation happened, false otherwise) or emitting a warning, e.g. call console.warn with the missing fields before returning. Locate navigateToText and add logic that computes which required keys are absent, logs a concise warning referencing those keys (or returns false) and ensure callers preserve existing behavior when true (still call refs.set, addHistory, gotoRoute) while callers can detect failure via the boolean return if you choose that approach.src/lib/components/AudioPlaybackSpeed.svelte (2)
44-44: ⚡ Quick winUse Svelte 5 event attribute syntax for consistency.
This file still uses the legacy Svelte 4
on:click={...}directive while the rest of the PR migrates to the Svelte 5onclick={...}attribute form. Aligning with the new style keeps the codebase consistent and avoids relying on the deprecated event-directive shim.♻️ Proposed fix
- on:click={(e) => setPlaySpeed(e.currentTarget.value)} + onclick={(e) => setPlaySpeed(e.currentTarget.value)}🤖 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/AudioPlaybackSpeed.svelte` at line 44, The click handler uses legacy Svelte 4 directive on:click; update it to the Svelte 5 attribute form onclick and keep the same handler logic — replace on:click={(e) => setPlaySpeed(e.currentTarget.value)} with onclick={(e) => setPlaySpeed(e.currentTarget.value)} in the AudioPlaybackSpeed component so setPlaySpeed continues to receive the clicked element's value.
26-29: ⚡ Quick win
modalThisshould be typed as possibly-undefined.
let modalThis: HTMLDialogElement;is declared without an initializer or| undefined, but it isn't assigned until theModalcomponent binds to it. Other components in this PR (e.g.PlanStopDialog,Dropdown) use theType | undefined = $state(undefined)pattern. Under strict TS this is likely flagged as "used before assigned," andshowModal()will throw if called before mount.🛡️ Proposed fix
- let modalThis: HTMLDialogElement; + let modalThis: HTMLDialogElement | undefined = $state(); export function showModal() { - modalThis.showModal(); + modalThis?.showModal(); }🤖 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/AudioPlaybackSpeed.svelte` around lines 26 - 29, Declare modalThis as possibly undefined and initialize it to undefined, e.g. let modalThis: HTMLDialogElement | undefined = undefined, and update showModal() to guard access (e.g. if (modalThis) modalThis.showModal(); else return or warn) so calling showModal before the component is mounted won't throw; reference the modalThis variable and the showModal() function in AudioPlaybackSpeed.svelte when making the changes.src/lib/components/HtmlBookView.svelte (1)
5-15: ⚡ Quick winType
fetchproperty more specifically.The
fetchproperty is typed asany, which bypasses TypeScript's type checking. Consider using SvelteKit's typedfetchfrom@sveltejs/kitor define a more specific type signature.♻️ Suggested fix
+ import type { Fetch } from '`@sveltejs/kit`'; + export interface Props { references: { collection: string; book: string; }; bodyLineHeight: number; bodyFontSize: number; - fetch: any; + fetch: Fetch; }🤖 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/HtmlBookView.svelte` around lines 5 - 15, The Props interface in HtmlBookView.svelte currently types fetch as any; update Props.fetch to a precise type such as SvelteKit's typed fetch (use "typeof fetch" imported from `@sveltejs/kit`) or a standard fetch signature (e.g., (input: RequestInfo, init?: RequestInit) => Promise<Response>) so TypeScript can validate callers and responses; modify the export interface Props { ... fetch: <chosen type>; } and add the corresponding import if you choose the SvelteKit typeof fetch option.src/lib/components/AudioBar.svelte (1)
104-104: ⚡ Quick winValidate type before casting to string.
The
audio-speedsetting is cast tostringwithout validation. If the setting value isundefined,null, or a non-string type, this could lead to unexpected behavior inupdatePlaybackSpeed.🛡️ Suggested defensive fix
- $effect(() => updatePlaybackSpeed($userSettings['audio-speed'] as string)); + $effect(() => { + const speed = $userSettings['audio-speed']; + if (typeof speed === 'string') { + updatePlaybackSpeed(speed); + } + });🤖 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/AudioBar.svelte` at line 104, The $effect currently casts $userSettings['audio-speed'] to string unguarded before calling updatePlaybackSpeed; validate the value first and only pass a string (or a safe default) to updatePlaybackSpeed. Change the effect that references $userSettings['audio-speed'] so it checks typeof value === 'string' (or value != null) and supplies a fallback (e.g., default speed string or existing setting) when undefined/null/invalid, then call updatePlaybackSpeed with that validated value; locate this logic around the $effect invocation and the updatePlaybackSpeed function to implement the guard and fallback.src/lib/components/ScriptureViewSofria.svelte (1)
12-35: 🏗️ Heavy liftReplace
anytypes with specific types in Props interface.Multiple properties in the
Propsinterface are typed asany(lines 15-16, 18-19, 21-22, 25-26), which defeats the purpose of TypeScript migration. These should have proper type definitions to catch potential runtime errors.Consider defining proper types for:
bodyFontSize,bodyLineHeight(likelynumber)bookmarks,notes,highlights(likely arrays with specific item shapes)maxSelections(likelynumber)references,glossary(likely specific object shapes)selectedVerses(likely a specific store or state type)themeColors(likely a record/object with color properties)verseLayout(likely a specific string literal type or enum)Based on learnings: Type safety prevents runtime errors that this PR aims to address (per issue
#905).🤖 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/ScriptureViewSofria.svelte` around lines 12 - 35, The Props interface uses many any types which removes compile-time safety; update the Props declaration (interface Props) to use specific types: change bodyFontSize and bodyLineHeight to number, maxSelections to number, redLetters to boolean (already typed), viewShowIllustrations/viewShowVerses/viewShowGlossaryWords to boolean (ensure correct), viewShowBibleImages/viewShowBibleVideos to string | boolean as appropriate, bookmarks/notes/highlights to typed arrays (e.g., Array<Bookmark>, Array<Note>, Array<Highlight>) where Bookmark/Note/Highlight are new interfaces describing their shapes, references and glossary to specific object types (e.g., ReferencesMap, GlossaryMap or interfaces for their entries), selectedVerses to the actual store/state type used elsewhere in the app (e.g., SelectedVersesStore or VerseSelection type), themeColors to a Record<string, string> or a ThemeColors interface describing color keys, and verseLayout to a union or enum of allowed string values; add or import these small interfaces/types near the Props definition and replace any with them to restore type safety while keeping SABProskomma typed as SABProskomma.src/lib/components/CollectionSelector.svelte (2)
16-16: ⚡ Quick winDeclare
modalwith$state()for Svelte 5bind:this.The rest of this PR (e.g.,
+layout.svelte) consistently wrapsbind:thistargets in$state(). Using a plainlethere makes the binding non-reactive and inconsistent with the convention applied across the migration; if any future code readsmodalinside$derived/$effect, it will not track.♻️ Proposed fix
- let modal: Modal; + let modal: Modal | undefined = $state();
showModal()should then guard with optional chaining:export function showModal() { - modal.showModal(); + modal?.showModal(); }🤖 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/CollectionSelector.svelte` at line 16, Declare the modal as a Svelte 5 reactive state by using $state() instead of a plain let so bind:this is tracked (replace the current let modal: Modal with a $state-backed modal). Update usages such as showModal() to guard via optional chaining (e.g., modal?.open() / modal?.show()) so calls are safe when modal is undefined and will be tracked in $derived/$effect; reference the Modal type and the showModal function to locate the change.
37-41: 💤 Low valuePrefer a type predicate over the trailing cast.
.filter((x) => !!x)does not narrow(string | undefined)[]tostring[], which is why theas string[] | undefinedcast is needed. A type-predicate filter expresses the intent without a cast and keeps the type system honest.♻️ Proposed fix
auxDocSets: collections ?.slice(1) .map((x) => x?.id) - .filter((x) => !!x) as string[] | undefined + .filter((x): x is string => !!x)🤖 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/CollectionSelector.svelte` around lines 37 - 41, The expression building auxDocSets uses .map(x => x?.id).filter((x) => !!x) and then casts to string[] | undefined; replace the truthy-filter + cast with a proper type predicate so the compiler knows ids are strings: change the filter to use a predicate like (x): x is string => Boolean(x) (or equivalent) on the result of collections?.slice(1).map(x => x?.id), remove the trailing as string[] | undefined cast, and ensure auxDocSets retains the same optional chaining behavior when collections is undefined.src/lib/components/LayoutOptions.svelte (1)
74-74: 💤 Low valueStale TODO comment refers to
Layout.TWO.The enum value was renamed to
Layout.Twoin this diff; please update the TODO so the reference still resolves.📝 Proposed fix
- // TODO: when implementing Layout.TWO, do something for Dropdown.close instead of blur?? + // TODO: when implementing Layout.Two, do something for Dropdown.close instead of blur??🤖 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` at line 74, Update the stale TODO in LayoutOptions.svelte to reference the renamed enum value Layout.Two instead of Layout.TWO and keep the rest of the note intact (e.g., "TODO: when implementing Layout.Two, do something for Dropdown.close instead of blur??"), so the comment resolves correctly against the current enum symbol.src/lib/data/stores/setting.ts (1)
407-407: ⚡ Quick win
JSON.parse(localStorage.userSettings)will throw on corrupt storage; cast hides type risk.
mergeDefaultStorage('userSettings', ...)mostly guarantees the key is set, but a corrupted/non-JSON value will throw at module load and bring down the whole app. Also,as FeatureConfigclaims a guaranteed shape thatJSON.parsecannot enforce. Wrap intry/catchand fall back todefaultUserSettings().🛡️ Proposed fix
-export const userSettings = writable(JSON.parse(localStorage.userSettings) as FeatureConfig); +function loadUserSettings(): FeatureConfig { + try { + return JSON.parse(localStorage.userSettings) as FeatureConfig; + } catch { + return defaultUserSettings(); + } +} +export const userSettings = writable(loadUserSettings());🤖 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/setting.ts` at line 407, userSettings initialization currently directly calls JSON.parse(localStorage.userSettings) which will throw on corrupt/non-JSON data and incorrectly asserts the shape with as FeatureConfig; change the initialization to parse inside a try/catch: attempt JSON.parse(localStorage.userSettings), validate/avoid using a blind cast, and on any error or invalid result fall back to defaultUserSettings(); keep using writable(...) but pass either the parsed safe object or defaultUserSettings(); refer to the existing userSettings export and defaultUserSettings() (and mergeDefaultStorage where relevant) when making the change.src/lib/components/TabsMenu.svelte (2)
59-60: 💤 Low valueRe-narrow
options[opt].tabbefore invoking.icon.
options[opt].tab?.iconcorrectly checks both, but inside the branch{@renderoptions[opt].tab.icon(opt)}performstab.iconwithout optional chaining. Under TS strict null checks this should error because the narrowing ontab?.icondoesn't propagate to the next access. Either aliastabfirst or assert non-null.🛠️ Proposed fix
- {`#if` options[opt].tab?.icon} - {`@render` options[opt].tab.icon(opt)} + {`@const` tabIcon = options[opt].tab?.icon} + {`#if` tabIcon} + {`@render` tabIcon(opt)} {:else} {opt} {/if}🤖 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/TabsMenu.svelte` around lines 59 - 60, The condition uses options[opt].tab?.icon but the render line accesses options[opt].tab.icon without the narrowing; inside TabsMenu.svelte alias the tab first (e.g. const tab = options[opt].tab) or otherwise assert non-null, then use that alias when rendering (check tab?.icon then call tab.icon(opt)) so TypeScript strict-null checks are satisfied and the narrowed type is used for the .icon call.
11-12: 💤 Low value
optionsdefault uses an invisible-tab record; theactivedefault then yieldsundefined.If the consumer passes nothing, the default
options = { '': { visible: true } }is used, but the line above already defaultsactivefromObject.keys(options).filter((x) => options[x].visible)[0]— which works here only by accident because the placeholder tab is marked visible. If you really wantoptionsrequired (as the type annotation says), drop the default. Otherwise either remove the default or document the invariant.🤖 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/TabsMenu.svelte` around lines 11 - 12, The default props create a bogus placeholder so active is accidentally set; remove the default for options (i.e. make options required) and compute active safely from the provided options (use Object.keys(options).find(k => options[k].visible) or fallback to null/first key) so the component doesn't rely on the invisible placeholder; update the active initializer (symbol: active) and the options prop (symbol: options) accordingly.src/lib/components/ContentSingle.svelte (2)
22-28: 💤 Low valueFallback handlers warn on every interaction.
onClickFallbackandcheckImageSizeFallbackare now default values, so any consumer that doesn't pass these props will spamconsole.warnon every render/click. Either drop the warning (the type already documents the contract) or make these props required if a real handler must be provided.🤖 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/ContentSingle.svelte` around lines 22 - 28, The fallback handlers onClickFallback and checkImageSizeFallback currently log console.warn on every render/interaction (functions onClickFallback and checkImageSizeFallback), which spams consumers; remove the console.warn calls and make these functions no-ops instead, or alternatively change the component props to be required so callers must supply handlers. Specifically, replace the console.warn bodies in onClickFallback and checkImageSizeFallback with empty no-op implementations (or adjust the exported prop types to remove defaults and mark them required), and update any type annotations/exports for those props to reflect the chosen approach so consumers won’t get repeated warnings.
30-37: 💤 Low valueDebug
console.logand inverted message inrenderLastTextBox.
!!exclude.find(...)istruewhenlayoutis in the exclude list, but the function returns the negation (!exclude.includes(layout)). The logged value is therefore the opposite of what's returned, which will mislead anyone reading the console. Either remove the log or align it with the return value.🧹 Proposed fix
function renderLastTextBox(layout?: FeatureValue): boolean { if (layout === undefined) { return false; } const exclude: FeatureValue[] = ['image-left-text-right', 'image-right-text-left']; - console.log(`renderLastTextBox: ${!!exclude.find((x) => x === layout)}`); return !exclude.includes(layout); }🤖 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/ContentSingle.svelte` around lines 30 - 37, The debug console.log in renderLastTextBox is emitting a value opposite to the function's return (it logs !!exclude.find(...) which is true when layout is excluded, but the function returns !exclude.includes(layout)), so either remove the console.log line or update it to log the actual return value; locate the renderLastTextBox function and either delete the console.log call or replace its message to reflect the computed result (e.g., log !exclude.includes(layout) or a descriptive string showing whether the last text box will render).config/index.d.ts (1)
161-162: 🏗️ Heavy lift
FeatureConfigis too permissive given how it's used downstream.
Record<string, FeatureValue>means every string key is typed asstring | boolean | number(neverundefined). That hides missing-key bugs and forces consumers to useas boolean/as stringcasts everywhere (see the many... as boolean/... as stringcasts added across this PR, e.g. inLayoutOptions.svelteline 25-26 andtext/+page.sveltelines 141-142, 207). A safer shape is{ [key: string]: FeatureValue | undefined }, which forces explicit narrowing and removes most of those casts.🤖 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 `@config/index.d.ts` around lines 161 - 162, FeatureConfig is too permissive because Record<string, FeatureValue> disallows missing keys and forces unsafe casts; change FeatureConfig so its values may be undefined (e.g., make it {[key: string]: FeatureValue | undefined}) so consumers must narrow or check existence. Update the FeatureConfig type declaration (the FeatureConfig and FeatureValue types) accordingly and then remove unnecessary "as boolean"/"as string" casts in consumers like LayoutOptions.svelte and text/+page.svelte by using proper presence checks or type guards.src/routes/text/+page.svelte (1)
551-557: 💤 Low valueRedundant
ascasts defeat the purpose ofsatisfies.
viewSettingsis already typed precisely viasatisfies HtmlBookViewProps/satisfies ScriptureViewSofriaProps, but each branch above the spread becomes a union, so you re-narrow viaas. The current casts work, but they bypass type checking on the spread. Sinceformat === 'html'already discriminates the union, a small helper or localif/elsekeeps the narrowing without a cast.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/routes/text/`+page.svelte around lines 551 - 557, The spread casts on viewSettings bypass TypeScript's narrowing; remove the redundant "as" casts and instead narrow viewSettings using the discriminant (format) before spreading. For example, in the format === 'html' branch assign viewSettings to a local typed variable (const settings: HtmlBookViewProps = viewSettings) and spread that into <HtmlBookView {...settings} />, and similarly in the else branch assign to a local const typed as ScriptureViewSofriaProps and spread into <ScriptureViewSofria {...settings} /> so the compiler retains proper type-checking for viewSettings.src/routes/contents/[id]/+page.svelte (2)
281-317: 💤 Low valueGuard
{#eachdata.items}against undefined.
data.itemsis consumed by two{#each}blocks but is not narrowed. If the route loader can return without anitemsarray, Svelte 5 will throw at runtime. Cheap to guard with a coalesce.🛡️ Proposed guard
- {`#if` data.nestedItems === true} - {`#each` data.items as item} + {`#if` data.nestedItems === true} + {`#each` data.items ?? [] as item} @@ - {:else} - {`#each` data.items as item} + {:else} + {`#each` data.items ?? [] as item}🤖 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/contents/`[id]/+page.svelte around lines 281 - 317, Guard the two {`#each` data.items as item} blocks against undefined by coalescing to an empty array (e.g., use data.items ?? []) or otherwise ensuring data.items is an array before iteration; update both occurrences in the +page.svelte file where the template iterates over data.items (the two {`#each`} blocks inside the nestedItems conditional and its else branch) so Svelte 5 cannot throw if the loader returns no items.
360-371: 💤 Low valueInconsistent null-safety between
titleandsubtitlehandling.
item.title[$language] ?? item.title.default(Line 362) readsitem.titleunguarded, but the very next block usesitem.subtitle?.[$language] ?? item.subtitle?.default(Line 369). Iftitlecan ever be absent on a content item, this throws. Either guard both fields or document thattitleis required whilesubtitleis optional.♻️ Suggested adjustment
- {`#if` data.features?.['show-titles']} + {`#if` data.features?.['show-titles'] && item.title} <div class="contents-title"> {item.title[$language] ?? item.title.default} </div> {/if}🤖 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/contents/`[id]/+page.svelte around lines 360 - 371, The template accesses item.title unguarded but uses optional chaining for item.subtitle, which can throw if title is missing; update the title handling to match the subtitle by using optional chaining and a safe fallback (e.g., item.title?.[$language] ?? item.title?.default ?? '') or enforce/validate that title is always present before rendering; change the expression in the show-titles block (the reference to item.title and $language) to the null-safe form so both title and subtitle use consistent optional chaining and fallback behavior.src/lib/components/BottomNavigationBar.svelte (1)
21-27: 💤 Low valuePartial null-safety on store/config lookups.
$s?.['ui.bottom-navigation.']['background-color']optional-chains the store but then performs unguarded property access on the inner object — if$sis defined but the style key is missing, the inner index throws. Likewise,scriptureConfig.plans?.plans.lengthonly guardsplans; ifplans.plansis undefined the.lengthaccess throws. Recommend consistent optional chaining all the way through.♻️ Suggested adjustment
- const barBackgroundColor = $derived($s?.['ui.bottom-navigation.']['background-color']); - const barTextColor = $derived($s?.['ui.bottom-navigation.item.text']['color']); - const barTextSelectedColor = $derived($s?.['ui.bottom-navigation.item.text.selected']['color']); + const barBackgroundColor = $derived($s?.['ui.bottom-navigation.']?.['background-color']); + const barTextColor = $derived($s?.['ui.bottom-navigation.item.text']?.['color']); + const barTextSelectedColor = $derived( + $s?.['ui.bottom-navigation.item.text.selected']?.['color'] + ); @@ - const showPlans = (scriptureConfig.plans?.plans.length ?? 0) > 0; + const showPlans = (scriptureConfig.plans?.plans?.length ?? 0) > 0;🤖 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/BottomNavigationBar.svelte` around lines 21 - 27, The lookups are only partially null-safe; update the expressions so nested properties are optional-chained and given safe fallbacks before accessing inner keys or .length: for the style-derived values (barBackgroundColor, barTextColor, barTextSelectedColor) guard the inner index access (e.g. treat $s as possibly missing the style object and default to an empty object/string) so you never index into undefined, and for the counts/arrays (showContents, showPlans) optional-chain the full path and default to 0 or an empty array before reading .length; also ensure config.mainFeatures['search'] is coerced/present-checked rather than assumed. Ensure these changes target the named variables (barBackgroundColor, barTextColor, barTextSelectedColor, showContents, showSearch, showPlans) so the component no longer throws when intermediate objects are absent.src/lib/components/ChapterSelector.svelte (1)
91-127: 💤 Low valueInconsistent key normalization for
chaptersLabelsaccess.
getChapterLabeluseschaptersLabels[Number(chapter)]whilechapterIndicatoruseschaptersLabels[chapter]. In plain JS both resolve to the same string-keyed property, but the inconsistency is confusing and TypeScript's inferred type forchaptersLabelsmay diverge between the two functions. Recommend picking one form (and ideally extracting a tiny helper) to keep the lookup uniform.♻️ Suggested cleanup
- function getChapterLabel(chapter: string) { + function getChapterLabel(chapter: string) { if (chapter === 'i') { return $t['Chapter_Introduction_Symbol']; } - - if (chaptersLabels[Number(chapter)] !== undefined) { - return chaptersLabels[Number(chapter)]; - } - - return numerals.formatNumber(numeralSystem, chapter); + return chaptersLabels[chapter] ?? numerals.formatNumber(numeralSystem, chapter); } @@ - let chapterIndicator = (book: string, chapter: string) => { - let value = ''; - if (chapter === 'i') { - value = $t['Chapter_Introduction_Symbol']; - } else if (chaptersLabels[chapter] !== undefined) { - value = chaptersLabels[chapter]; - } else { - value = numerals.formatNumber(numeralSystem, chapter); - } - return value; - }; + const chapterIndicator = (book: string, chapter: string) => getChapterLabel(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/components/ChapterSelector.svelte` around lines 91 - 127, Normalize how chapter keys are used for chaptersLabels and chapters lookups by choosing a single key format (either always Number(chapter) or always String(chapter)) and extract a small helper (e.g., resolveChapterKey(chapter)) used by getChapterLabel, chapterIndicator, and getVerseCount to perform the conversion and perform the lookup; update all references (getChapterLabel, chapterIndicator, and the chapters[chapter] access in getVerseCount) to call the helper so lookups are uniform and TypeScript types remain consistent.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 30429cfd-98d5-42dc-aba7-e8feba331480
📒 Files selected for processing (56)
config/index.d.tsconvert/convertBooks.tsconvert/convertConfig.tssrc/app.d.tssrc/lib/components/AudioBar.sveltesrc/lib/components/AudioPlaybackSpeed.sveltesrc/lib/components/BookSelector.sveltesrc/lib/components/BookTabs.sveltesrc/lib/components/BookmarkButton.sveltesrc/lib/components/BottomNavigationBar.sveltesrc/lib/components/ChapterSelector.sveltesrc/lib/components/CollectionList.sveltesrc/lib/components/CollectionSelector.sveltesrc/lib/components/ContentCarousel.sveltesrc/lib/components/ContentGrid.sveltesrc/lib/components/ContentHeading.sveltesrc/lib/components/ContentSingle.sveltesrc/lib/components/Dropdown.sveltesrc/lib/components/FontList.sveltesrc/lib/components/FontSelector.sveltesrc/lib/components/HorizontalPanes.sveltesrc/lib/components/HtmlBookView.sveltesrc/lib/components/LayoutOptions.sveltesrc/lib/components/Modal.sveltesrc/lib/components/Navbar.sveltesrc/lib/components/PlanStopDialog.sveltesrc/lib/components/ScriptureViewSofria.sveltesrc/lib/components/SearchResultList.sveltesrc/lib/components/SelectGrid.sveltesrc/lib/components/Settings.sveltesrc/lib/components/Sidebar.sveltesrc/lib/components/Slider.sveltesrc/lib/components/StackView.sveltesrc/lib/components/TabsMenu.sveltesrc/lib/components/TextAppearanceSelector.sveltesrc/lib/components/VerticalPanes.sveltesrc/lib/data/stores/collection.tssrc/lib/data/stores/interface.tssrc/lib/data/stores/localization.tssrc/lib/data/stores/reference.tssrc/lib/data/stores/setting.tssrc/lib/data/stores/theme.tssrc/lib/data/stores/view.tssrc/lib/navigate/index.tssrc/lib/scripts/configUtils.tssrc/lib/scripts/numeralSystem.tssrc/routes/+layout.sveltesrc/routes/+page.sveltesrc/routes/contents/[id]/+page.sveltesrc/routes/history/+page.sveltesrc/routes/lexicon/+layout.sveltesrc/routes/lexicon/+page.sveltesrc/routes/notes/edit/[noteid]/+page.sveltesrc/routes/plans/+page.sveltesrc/routes/plans/[id]/+page.sveltesrc/routes/text/+page.svelte
💤 Files with no reviewable changes (2)
- src/lib/components/HorizontalPanes.svelte
- src/lib/components/VerticalPanes.svelte
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (5)
src/lib/components/BottomNavigationBar.svelte (1)
152-152:⚠️ Potential issue | 🟠 Major | ⚡ Quick winBottom-nav label fallback regressed to a single-language lookup.
item.title[$language]alone can render undefined text. Please restore a default-language fallback.Suggested fix
- import { language, refs, s, theme } from '$lib/data/stores'; + import { language, languageDefault, refs, s, theme } from '$lib/data/stores'; ... - {item.title[$language]} + {item.title[$language] ?? item.title[languageDefault] ?? item.title.default ?? ''}🤖 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/BottomNavigationBar.svelte` at line 152, In BottomNavigationBar.svelte the label lookup item.title[$language] can be undefined; change it to fall back to a default language and/or a safe fallback string (e.g. use item.title[$language] || item.title[DEFAULT_LANGUAGE] || item.title['en'] || '') so the component always renders text; ensure you reference the existing default language constant/store (DEFAULT_LANGUAGE or equivalent) or hardcode a sensible default and guard against item.title being undefined before indexing.src/lib/components/Sidebar.svelte (1)
114-119:⚠️ Potential issue | 🟠 Major | ⚡ Quick winOptional chaining is still incomplete in derived color lookups.
These expressions can still throw if the intermediate style object is missing (
undefined['...']). Please chain the second lookup too.Suggested fix
- const textColor = $derived($s?.['ui.drawer.item.text']['color']); + const textColor = $derived($s?.['ui.drawer.item.text']?.['color']); ... - const contentBackgroundColor = $derived($s?.['ui.background']['background-color']); - const drawerBackgroundColor = $derived($s?.['ui.drawer']['background-color']); + const contentBackgroundColor = $derived($s?.['ui.background']?.['background-color']); + const drawerBackgroundColor = $derived($s?.['ui.drawer']?.['background-color']);🤖 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 114 - 119, The derived color lookups (textColor, iconColor, contentBackgroundColor, drawerBackgroundColor) can still throw when the intermediate style objects are undefined; update each $derived call to use optional chaining on the second property access (e.g. change $s?.['ui.drawer.item.text']['color'] to $s?.['ui.drawer.item.text']?.['color'], and similarly ensure iconColor uses $s?.['ui.drawer.item.icon']?.['color'] || $themeColors['DrawItemIconColor'], contentBackgroundColor uses $s?.['ui.background']?.['background-color'], and drawerBackgroundColor uses $s?.['ui.drawer']?.['background-color']) so lookups are safe.src/lib/components/BookTabs.svelte (1)
29-29:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winRestore fallback language lookup for tab type names.
Using only
name?.[$language]can leavealtempty/undefined when that locale key is missing. Please keep the default-language fallback.Suggested fix
- import { convertStyle, language, monoIconColor, refs, s, theme } from '$lib/data/stores'; + import { convertStyle, language, languageDefault, monoIconColor, refs, s, theme } from '$lib/data/stores'; ... - return scriptureConfig.tabTypes?.[tabType ?? '']?.name?.[$language]; + return ( + scriptureConfig.tabTypes?.[tabType ?? '']?.name?.[$language] ?? + scriptureConfig.tabTypes?.[tabType ?? '']?.name?.[languageDefault] + );🤖 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/BookTabs.svelte` at line 29, The lookup for tab names currently only tries name?.[$language] which can leave alt undefined when that locale key is missing; change the expression in BookTabs.svelte so it first attempts scriptureConfig.tabTypes?.[tabType ?? '']?.name?.[$language] and falls back to scriptureConfig.tabTypes?.[tabType ?? '']?.name?.[scriptureConfig?.defaultLanguage ?? 'en'] (use || or ?? to return the fallback), ensuring the alt value uses the configured default language if the current $language key is absent.src/lib/components/Settings.svelte (1)
60-60:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winGuard
setting.valueswhen indexing list option values.
setting.values![i]can still throw if alistsetting arrives withoutvalues. Prefer safe indexing or a fallback value to avoid crashing the settings page.Suggested fix
- <option value={setting.values![i]}>{$t[entry] || entry}</option> + <option value={setting.values?.[i] ?? entry}>{$t[entry] || entry}</option>🤖 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/Settings.svelte` at line 60, The option rendering currently indexes into setting.values with a non-null assertion (setting.values![i]) which can throw if a `list` setting arrives without `values`; change the lookup to safely access the array and provide a fallback (e.g. use optional chaining like setting.values?.[i] and fall back to entry or an empty string) so the <option> value never throws when values is undefined; update the code in Settings.svelte where the <option> uses setting.values to use this safe lookup.src/routes/notes/edit/[noteid]/+page.svelte (1)
18-20:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
textstate is no longer synchronized withnoteupdates.Switching to
$statefixed binding, but nowtextwon’t refresh ifdata.notechanges while this page instance stays mounted.Suggested fix
- // svelte-ignore state_referenced_locally - let text = $state(note?.text ?? ''); + let text = $state(note?.text ?? ''); + $effect(() => { + text = note?.text ?? ''; + });🤖 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/notes/edit/`[noteid]/+page.svelte around lines 18 - 20, The text local state (let text = $state(...)) stops updating when data.note changes because it was initialized once; update text whenever note changes by deriving or reacting to note updates: locate the symbols text, $state, note (data.note) and $derived/$selectedVerses in +page.svelte and replace the one-time initialization with a reactive sync (e.g., use a derived store or a $: reactive block that sets text to note?.text ?? '' when note changes) so text remains in sync while the page component stays mounted.
🤖 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/Sidebar.svelte`:
- Around line 271-273: The sidebar rendering uses item.title[$language]
directly, which can be empty when a translation is missing; update the
Sidebar.svelte render to use a fallback (e.g., a configured defaultLanguage or
the first available title) whenever item.title[$language] is falsy — replace
direct accesses of item.title[$language] (used for both the image alt and
visible label) with a lookup that returns item.title[$language] ||
item.title[defaultLanguage] || first non-empty value from item.title.
---
Duplicate comments:
In `@src/lib/components/BookTabs.svelte`:
- Line 29: The lookup for tab names currently only tries name?.[$language] which
can leave alt undefined when that locale key is missing; change the expression
in BookTabs.svelte so it first attempts scriptureConfig.tabTypes?.[tabType ??
'']?.name?.[$language] and falls back to scriptureConfig.tabTypes?.[tabType ??
'']?.name?.[scriptureConfig?.defaultLanguage ?? 'en'] (use || or ?? to return
the fallback), ensuring the alt value uses the configured default language if
the current $language key is absent.
In `@src/lib/components/BottomNavigationBar.svelte`:
- Line 152: In BottomNavigationBar.svelte the label lookup item.title[$language]
can be undefined; change it to fall back to a default language and/or a safe
fallback string (e.g. use item.title[$language] || item.title[DEFAULT_LANGUAGE]
|| item.title['en'] || '') so the component always renders text; ensure you
reference the existing default language constant/store (DEFAULT_LANGUAGE or
equivalent) or hardcode a sensible default and guard against item.title being
undefined before indexing.
In `@src/lib/components/Settings.svelte`:
- Line 60: The option rendering currently indexes into setting.values with a
non-null assertion (setting.values![i]) which can throw if a `list` setting
arrives without `values`; change the lookup to safely access the array and
provide a fallback (e.g. use optional chaining like setting.values?.[i] and fall
back to entry or an empty string) so the <option> value never throws when values
is undefined; update the code in Settings.svelte where the <option> uses
setting.values to use this safe lookup.
In `@src/lib/components/Sidebar.svelte`:
- Around line 114-119: The derived color lookups (textColor, iconColor,
contentBackgroundColor, drawerBackgroundColor) can still throw when the
intermediate style objects are undefined; update each $derived call to use
optional chaining on the second property access (e.g. change
$s?.['ui.drawer.item.text']['color'] to $s?.['ui.drawer.item.text']?.['color'],
and similarly ensure iconColor uses $s?.['ui.drawer.item.icon']?.['color'] ||
$themeColors['DrawItemIconColor'], contentBackgroundColor uses
$s?.['ui.background']?.['background-color'], and drawerBackgroundColor uses
$s?.['ui.drawer']?.['background-color']) so lookups are safe.
In `@src/routes/notes/edit/`[noteid]/+page.svelte:
- Around line 18-20: The text local state (let text = $state(...)) stops
updating when data.note changes because it was initialized once; update text
whenever note changes by deriving or reacting to note updates: locate the
symbols text, $state, note (data.note) and $derived/$selectedVerses in
+page.svelte and replace the one-time initialization with a reactive sync (e.g.,
use a derived store or a $: reactive block that sets text to note?.text ?? ''
when note changes) so text remains in sync while the page component stays
mounted.
🪄 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: 829ba453-2af5-4283-829d-5aba31103c7f
📒 Files selected for processing (9)
src/lib/components/BookTabs.sveltesrc/lib/components/BottomNavigationBar.sveltesrc/lib/components/ContentSingle.sveltesrc/lib/components/FontSelector.sveltesrc/lib/components/SearchResultList.sveltesrc/lib/components/Settings.sveltesrc/lib/components/Sidebar.sveltesrc/lib/components/TabsMenu.sveltesrc/routes/notes/edit/[noteid]/+page.svelte
💤 Files with no reviewable changes (1)
- src/lib/components/FontSelector.svelte
✅ Files skipped from review due to trivial changes (1)
- src/lib/components/SearchResultList.svelte
eomerdws
left a comment
eomerdws
left a comment
There was a problem hiding this comment.
Overall looks great! Thanks for fixing the Book selector issue so quickly that I had to take the comment out 👍.
2 participants
Continuation of #960, closes #905
There are still many remaining type errors, mostly because our current type-checking configuration in CI does not catch them. We need to make further tweaks to that configuration, ideally in line with what we have in Scriptoria.
Summary by CodeRabbit
Refactor
Bug Fixes