Convert stores and page loads to TS#960
Merged
Merged
Conversation
Contributor
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughThis PR modernizes the app's architecture by introducing a typed modal system, refactoring audio playback state management with enums, adding comprehensive TypeScript annotations across stores and database layers, converting SvelteKit page loaders to typed const form, and migrating component data access from store to props. It also removes legacy script utilities and strengthens type safety throughout the codebase. ChangesModal and Store Refactoring
Audio System Refactoring
SvelteKit Route Loader and Page Data Modernization
Database Schema and Instance Typing
Scripture Module and Config Loading
Script Module Removal and Import Cleanup
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
🚥 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 unit tests (beta)
|
![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: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/lib/data/stores/storage.ts (1)
7-21:⚠️ Potential issue | 🟠 Major | ⚡ Quick winGuard
JSON.parseto prevent hard failures on corrupted storage (Line 14).A malformed localStorage value will throw here and can break initialization paths. Add a safe parse fallback before merging.
Proposed change
export const mergeDefaultStorage = (name: string, value?: Record<string, unknown>) => { @@ - } else if (storage && value) { + } else if (storage && value) { // If it does exist, then set the keys that don't exist - const update = JSON.parse(storage); + let update: Record<string, unknown>; + try { + update = JSON.parse(storage) as Record<string, unknown>; + } catch { + update = {}; + } Object.keys(value).forEach((key) => { if (!Object.hasOwn(update, key)) { update[key] = value[key]; } });🤖 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/storage.ts` around lines 7 - 21, mergeDefaultStorage currently calls JSON.parse(storage) without guarding against corrupted/malformed data which can throw; wrap the parse in a safe try/catch (when storage exists) and if parsing fails treat it as absent (e.g., set update = value or {}), then proceed with the existing merge logic and write back to localStorage; specifically modify the mergeDefaultStorage function to catch JSON.parse errors on the result of localStorage.getItem(name), use a safe fallback for update, and ensure localStorage.setItem is called with the correct merged object.
🧹 Nitpick comments (5)
src/routes/highlights/+page.svelte (1)
54-58: ⚡ Quick winType the
Propsinterface in the$props()destructuring.The
Propsinterface is declared but not applied tolet { data }. Change tolet { data }: Props = $props();to enforce type checking on thedataprop. This pattern is already followed in other converted pages (e.g.,src/routes/share/+page.svelteandsrc/routes/quiz/[collection]/[id]/+page.svelte).Proposed change
- let { data } = $props(); + let { data }: Props = $props();🤖 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/highlights/`+page.svelte around lines 54 - 58, The Props interface is declared but not applied to the $props() destructuring; update the destructuring so the data variable is typed using Props by changing the declaration of let { data } to use the Props type (i.e., annotate the result of $props() with Props) so the component enforces type checking for data; locate the Props interface and the let { data } = $props() line in src/routes/highlights/+page.svelte and add the type annotation to the destructuring (using Props).src/lib/data/stores/audio.ts (1)
43-43: ⚡ Quick winUse an environment-safe timeout handle type for
timer(Line 43).
NodeJS.Timeoutis inappropriate for browser-only code. The actual implementation usessetInterval()at line 347, so useReturnType<typeof setInterval> | nullfor compatibility. Alternatively,ReturnType<typeof setTimeout> | nullworks equivalently since both return the same type across all environments.Proposed change
export interface AudioPlayer { - timer: NodeJS.Timeout | null; + timer: ReturnType<typeof setInterval> | null; }🤖 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/audio.ts` at line 43, The field declaration "timer: NodeJS.Timeout | null;" is using a Node-specific type and should be made environment-safe; locate the declaration named timer and replace its type with a platform-agnostic one such as "ReturnType<typeof setInterval> | null" (or "ReturnType<typeof setTimeout> | null") to match the usage of setInterval at runtime, ensuring the rest of the code (the setInterval/setTimeout calls and any clears) continue to accept the updated type.src/lib/data/stores/scripture.ts (1)
258-288: 💤 Low valueFallback objects are missing the
referencefield fromSelectiontype.
getVerseByIndexandgetVerseByVerseNumberreturn an object missing thereferencefield when the index is invalid. This creates a type inconsistency with theSelectiontype defined at lines 202-209.♻️ Suggested fix
} else { const selection = { docSet: '', collection: '', book: '', chapter: '', + reference: '', verse: '' }; return selection; }Apply to both
getVerseByIndex(lines 264-272) andgetVerseByVerseNumber(lines 280-288).🤖 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/scripture.ts` around lines 258 - 288, The fallback objects returned by getVerseByIndex and getVerseByVerseNumber lack the required Selection.reference field, causing a type mismatch with the Selection type (defined around Selection). Update the fallback objects in both functions (getVerseByIndex and getVerseByVerseNumber) to include a reference property (e.g., reference: '' or other appropriate default) so the returned object matches the Selection shape.src/lib/data/stores/view.ts (1)
128-135: 💤 Low valueConsider using
get()instead ofupdate()for read-only operations.The
length()method usesupdate()solely to read the stack length, which is unnecessary overhead. Theget()function fromsvelte/storeis already imported and would be simpler.♻️ Suggested refactor
length: () => { - let length = 0; - update((stack) => { - length = stack.length; - return stack; // No modification to stack, just returning current length - }); - return length; + return get({ subscribe }).length; }Or expose
subscribeto a local variable and useget()on it.🤖 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/view.ts` around lines 128 - 135, The length() method currently calls update() only to read the stack length; replace that with a read-only get() usage (imported from svelte/store) or capture the store's subscribe into a local variable and call get(stackStore) to obtain stack.length directly; update the length() implementation to return get(stackStore).length (or use the local subscribe/get approach) and remove the unnecessary update() call — reference the length() function and the update/get utilities when making the change.src/lib/data/scripture.ts (1)
14-29: 💤 Low valueCommented code references removed
configimport.Lines 19-22 reference
config.bookCollections[0]but theconfigimport was removed. If this code is ever uncommented, it will fail to compile. Consider either removing the dead code or adding a TODO with context.🤖 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/scripture.ts` around lines 14 - 29, The commented block inside initProskomma references config.bookCollections but the config import was removed; either delete the dead commented lines (lines that use config.bookCollections and related docSet logic) or restore a proper import and add a clear TODO explaining why config is needed before uncommenting; target the initProskomma function and the commented docSet logic (the block that builds docSet from config.bookCollections and the commented loadDocSet call) and ensure any future uncommenting also reintroduces a matching import for config or documents why the config dependency was removed.
🤖 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/audio.ts`:
- Around line 316-318: The end tag assignment for the last timing entry is using
the first element incorrectly; in the block where you check if (j ===
currentAudioPlayer!.timing!.length - 1) replace end =
currentAudioPlayer!.timing![0].tag with end = currentAudioPlayer!.timing![j].tag
so the last element's tag is used (reference currentAudioPlayer, timing, j, and
end variables).
In `@src/lib/data/bookmarks.ts`:
- Around line 70-74: The current conditional uses if (bookIndex) which treats 0
as falsy and -1 as truthy, causing valid index 0 to be skipped and invalid -1 to
be accepted; change the check around the findIndex() result (bookIndex) to
explicitly test for a non-negative index (e.g., bookIndex >= 0 or bookIndex !==
-1) before creating nextItem and calling bookmarks.add('bookmarks', nextItem)
and notifyUpdated(), so only valid indices are inserted.
In `@src/lib/data/history.ts`:
- Around line 54-63: The setTimeout callback closes over shared mutable
variables nextItem and nextTimer causing races in addHistory; fix by capturing
the values into locals when scheduling (e.g., const capturedItem = nextItem;
const capturedTimer = nextTimer) and have the callback reference those
capturedItem/capturedTimer instead of nextItem/nextTimer, remove the
clearTimeout(nextTimer!) call entirely, and only clear the outer
nextItem/nextTimer if they still strictly equal the capturedTimer (to avoid
wiping a newer scheduled entry) after awaiting history.add and invoking
callback.
In `@src/lib/data/notes.ts`:
- Around line 68-73: The condition using `if (bookIndex)` incorrectly treats 0
as falsy; update the check around where `bookIndex` is used (the block that
creates `nextItem`, calls `notes.add('notes', nextItem)` and `notifyUpdated()`)
to allow a valid index of 0 and reject only missing or not-found values by
explicitly testing for undefined and -1 (e.g., ensure `bookIndex !== undefined
&& bookIndex !== -1`), so notes for the first book are added correctly; keep the
rest of the logic (creating `nextItem`, calling `notes.add` and `notifyUpdated`)
unchanged.
In `@src/routes/plans/`+page.ts:
- Around line 4-10: The route's load function currently returns a hardcoded
empty array, dropping persisted plans; replace the placeholder return in export
const load: PageLoad with actual plan retrieval by calling the app's persistent
plan loader (e.g., await getPlans(), getAllPlans(), or loadPlansFromStore) or
fetching the plans endpoint, keep the depends('note:plans') call, and return the
retrieved plans as { plans: fetchedPlans as PlanItem[] } so the route restores
its previous data contract.
---
Outside diff comments:
In `@src/lib/data/stores/storage.ts`:
- Around line 7-21: mergeDefaultStorage currently calls JSON.parse(storage)
without guarding against corrupted/malformed data which can throw; wrap the
parse in a safe try/catch (when storage exists) and if parsing fails treat it as
absent (e.g., set update = value or {}), then proceed with the existing merge
logic and write back to localStorage; specifically modify the
mergeDefaultStorage function to catch JSON.parse errors on the result of
localStorage.getItem(name), use a safe fallback for update, and ensure
localStorage.setItem is called with the correct merged object.
---
Nitpick comments:
In `@src/lib/data/scripture.ts`:
- Around line 14-29: The commented block inside initProskomma references
config.bookCollections but the config import was removed; either delete the dead
commented lines (lines that use config.bookCollections and related docSet logic)
or restore a proper import and add a clear TODO explaining why config is needed
before uncommenting; target the initProskomma function and the commented docSet
logic (the block that builds docSet from config.bookCollections and the
commented loadDocSet call) and ensure any future uncommenting also reintroduces
a matching import for config or documents why the config dependency was removed.
In `@src/lib/data/stores/audio.ts`:
- Line 43: The field declaration "timer: NodeJS.Timeout | null;" is using a
Node-specific type and should be made environment-safe; locate the declaration
named timer and replace its type with a platform-agnostic one such as
"ReturnType<typeof setInterval> | null" (or "ReturnType<typeof setTimeout> |
null") to match the usage of setInterval at runtime, ensuring the rest of the
code (the setInterval/setTimeout calls and any clears) continue to accept the
updated type.
In `@src/lib/data/stores/scripture.ts`:
- Around line 258-288: The fallback objects returned by getVerseByIndex and
getVerseByVerseNumber lack the required Selection.reference field, causing a
type mismatch with the Selection type (defined around Selection). Update the
fallback objects in both functions (getVerseByIndex and getVerseByVerseNumber)
to include a reference property (e.g., reference: '' or other appropriate
default) so the returned object matches the Selection shape.
In `@src/lib/data/stores/view.ts`:
- Around line 128-135: The length() method currently calls update() only to read
the stack length; replace that with a read-only get() usage (imported from
svelte/store) or capture the store's subscribe into a local variable and call
get(stackStore) to obtain stack.length directly; update the length()
implementation to return get(stackStore).length (or use the local subscribe/get
approach) and remove the unnecessary update() call — reference the length()
function and the update/get utilities when making the change.
In `@src/routes/highlights/`+page.svelte:
- Around line 54-58: The Props interface is declared but not applied to the
$props() destructuring; update the destructuring so the data variable is typed
using Props by changing the declaration of let { data } to use the Props type
(i.e., annotate the result of $props() with Props) so the component enforces
type checking for data; locate the Props interface and the let { data } =
$props() line in src/routes/highlights/+page.svelte and add the type annotation
to the destructuring (using Props).
🪄 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: 7eed5b73-6d63-4956-8ba8-f0e8325f3077
📒 Files selected for processing (57)
src/lib/components/AudioBar.sveltesrc/lib/components/PlayButton.sveltesrc/lib/components/ScriptureViewSofria.sveltesrc/lib/components/Sidebar.sveltesrc/lib/components/TextAppearanceSelector.sveltesrc/lib/data/analytics.tssrc/lib/data/audio.tssrc/lib/data/bookmarks.tssrc/lib/data/highlights.tssrc/lib/data/history.tssrc/lib/data/notes.tssrc/lib/data/planProgressItems.tssrc/lib/data/planStates.tssrc/lib/data/plans.tssrc/lib/data/scripture.tssrc/lib/data/stores/annotation.tssrc/lib/data/stores/audio.tssrc/lib/data/stores/index.tssrc/lib/data/stores/interface.tssrc/lib/data/stores/localization.jssrc/lib/data/stores/localization.tssrc/lib/data/stores/log.tssrc/lib/data/stores/scripture.tssrc/lib/data/stores/setting.tssrc/lib/data/stores/storage.tssrc/lib/data/stores/store-types.jssrc/lib/data/stores/theme.tssrc/lib/data/stores/view.tssrc/lib/icons/audio/index.tssrc/lib/icons/image/index.tssrc/lib/icons/index.tssrc/lib/scripts/query.jssrc/lib/scripts/render.jssrc/routes/+layout.sveltesrc/routes/+page.tssrc/routes/about/+page.tssrc/routes/bookmarks/+page.tssrc/routes/contents/[id]/+page.sveltesrc/routes/contents/[id]/+page.tssrc/routes/highlights/+page.sveltesrc/routes/highlights/+page.tssrc/routes/history/+page.tssrc/routes/lexicon/+layout.sveltesrc/routes/notes/+page.sveltesrc/routes/notes/+page.tssrc/routes/notes/edit/[noteid]/+page.sveltesrc/routes/notes/edit/[noteid]/+page.tssrc/routes/plans/+page.tssrc/routes/plans/[id]/+page.sveltesrc/routes/plans/[id]/+page.tssrc/routes/plans/[id]/settings/+page.tssrc/routes/quiz/[collection]/[id]/+page.sveltesrc/routes/share/+page.sveltesrc/routes/share/+page.tssrc/routes/text/+page.jssrc/routes/text/+page.sveltesrc/routes/text/+page.ts
💤 Files with no reviewable changes (5)
- src/lib/scripts/query.js
- src/lib/scripts/render.js
- src/lib/data/stores/store-types.js
- src/routes/text/+page.js
- src/lib/data/stores/localization.js
eomerdws
approved these changes
May 20, 2026
Contributor
eomerdws
left a comment
eomerdws
left a comment
There was a problem hiding this comment.
This is really good work. My comments aren't blocking an approval review, but one is a question for consideration and the other is I like the approach you took in this area. In fact you did it in at least one other place, but I didn't want you to have to resolve it multiple times.
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.
Continuation of #959, part 2/3 of #905
Changes:
groupStoreSummary by CodeRabbit
Bug Fixes
Refactor