Migrate Quizzes to Svelte 5 and TS (with minor improvements)#958
Merged
Conversation
- Replaces run() with effect() for reactivity - Uses $state and $derived for all dynamic values - Handles audio playback, quiz progression, and result saving - Ensures full client-side functionality without server code
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. 📝 WalkthroughWalkthroughA quiz type system is formalized in the config layer with new ChangesQuiz Type System & Component Migration
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 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: 6
🧹 Nitpick comments (3)
src/routes/quiz/[collection]/[id]/+page.svelte (2)
124-126: 💤 Low valueUse the destructured
collectionfor consistency.
collectionis already destructured from$derived(data)on line 71 and is used everywhere else in this file (line 309). Readingdata.collectionhere bypasses the local binding for no benefit and is slightly inconsistent.♻️ Suggested change
function getImageSource(image: string) { - return illustrations[`./${data.collection}-${quiz?.id}-${image}`] ?? ''; + return illustrations[`./${collection}-${quiz?.id}-${image}`] ?? ''; }🤖 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/quiz/`[collection]/[id]/+page.svelte around lines 124 - 126, The getImageSource function uses data.collection directly instead of the local destructured variable collection; update getImageSource (function name: getImageSource) to reference the already-destructured collection variable (replace data.collection with collection) so it is consistent with other usages and uses the local binding when building the illustrations key (illustrations[`./${collection}-${quiz?.id}-${image}`]).
284-291: 💤 Low valueShow
displayLabelrather than the rawquizIdin the locked banner.
quizIdis the bare book id (e.g.,Q01), whiledisplayLabelis the user-facing name computed in+page.ts. UsingquizIdhere makes the locked screen look technical/inconsistent with the unlocked navbar (which usesdisplayLabelviaBookSelector).🩹 Proposed fix
{`#if` locked} <div class="quiz-locked"> - <div class="quiz-locked-title">{quizId}</div> + <div class="quiz-locked-title">{displayLabel}</div>🤖 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/quiz/`[collection]/[id]/+page.svelte around lines 284 - 291, The locked banner is showing the raw quizId; replace it with the user-facing displayLabel computed in +page.ts so the locked view matches the navbar. In +page.svelte, use the displayLabel provided by the page load (e.g., data.displayLabel or the local displayLabel prop) instead of {quizId}; if displayLabel isn't already passed into the component, export/forward it from the +page.ts load result so the template can render {displayLabel} (falling back to quizId only if displayLabel is missing).config/index.d.ts (1)
344-353: 💤 Low valueAvoid coupling UI state (
clicked) to the config schema type.
clickedis a transient UI flag, not part of the persisted quiz config. Embedding it inQuizAnswer(which is also imported byconvert/convertBooks.ts, an SFM→JSON converter) bleeds runtime UI concerns into the source-of-truth schema and makes the type lie about what's in*.jsonquiz assets. Consider deriving a UI-only type in the component instead.♻️ Suggested split between schema and UI state
export type QuizAnswer = { //\aw or \ar correct: boolean; text?: string; image?: string; audio?: string; explanation?: QuizExplanation; - // field is used extensively in UI, adding here for type-safety - clicked?: boolean; };Then in
+page.svelte:type QuizAnswerUI = QuizAnswer & { clicked?: 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 `@config/index.d.ts` around lines 344 - 353, The QuizAnswer type currently includes a UI-only field clicked which couples transient UI state into the persisted quiz schema; remove clicked from the exported QuizAnswer type and instead create a derived UI type where needed (e.g., in +page.svelte declare QuizAnswerUI = QuizAnswer & { clicked?: boolean }) and update any consumers accordingly (notably convert/convertBooks.ts should keep using the pure QuizAnswer for SFM→JSON conversion). Ensure no other modules rely on QuizAnswer.clicked; if they do, switch those callers to use the UI-specific type.
🤖 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/routes/quiz/`[collection]/[id]/+page.svelte:
- Line 77: The component never flips the local saved state so the {`#if` !saved}
gate around the {`#await` addQuiz(...)} can’t prevent duplicate writes; update the
flow so that after addQuiz resolves you set saved = true (or memoize the addQuiz
call) — locate the local saved variable (initialized via let saved =
$state(false)), the {`#await` addQuiz(...)} block and the resolution handling of
addQuiz and assign saved = true in the success branch (or implement a one-shot
helper/wrapper around addQuiz) so the await block only runs once per intended
save.
- Around line 90-94: The current conditional uses
book?.quizFeatures?.['shuffle-questions'] as a boolean so any non-empty string
(e.g. "false") will be truthy and trigger shuffle; update the check in the
questions initialization (const questions: QuizQuestion[] = $derived(...)) to
compare the feature string explicitly against the expected value (e.g. ===
'true' or === 'yes') before calling shuffle, so only the intended configured
value enables shuffling.
- Around line 1-19: The module script block uses TypeScript syntax (generics and
type annotations in getRandomItem<T> and shuffle<T>) but is not declared as
TypeScript; update the <script module> tag to include lang="ts" so Svelte treats
that block as TypeScript (i.e. change <script module> to <script module
lang="ts">) and then ensure getRandomItem and shuffle keep their existing
signatures/annotations.
- Around line 299-301: The replacement call is passing a number to
String.prototype.replace; change the second argument to an explicit string
(e.g., String(currentQuestionIdx) or `${currentQuestionIdx}`) where the template
uses {$t['Quiz_Score_Page_Message_After'].replace('%n%',
currentQuestionIdx)}—update that expression to stringify currentQuestionIdx so
the replace call satisfies TypeScript's expected string | function type.
- Around line 321-336: The cols calculation currently forces a boolean ternary
and discards numeric column values; change the expression so nullish coalescing
applies to currentQuestion.columns first (e.g. set cols =
currentQuestion.columns ?? (imgFormat ? 2 : 1)) so explicit numeric column
settings like 3+ are honored, and keep using the dynamic grid class
grid-cols-{cols}; additionally add a safelist entry in tailwind.config.cjs to
generate grid-cols-* classes (e.g. a pattern/safelist for
grid-cols-1..grid-cols-N or a regex like /grid-cols-(\d+)/) so Tailwind JIT
picks up the dynamic grid-cols-{cols} classes at build time.
In `@src/routes/quiz/`[collection]/[id]/+page.ts:
- Around line 26-30: The variable dependentQuizId may be undefined because
book.quizFeatures['access-after'] can be missing; update the logic around
dependentQuizId and checkQuizAccess to guard against undefined (or normalize to
null) before calling checkQuizAccess. Specifically, in the block guarded by
book?.quizFeatures?.['access-type'] === 'after', validate that
book.quizFeatures['access-after'] is a non-empty string (or provide a safe
default), only call checkQuizAccess(dependentQuizId) when dependentQuizId is
defined, and set locked appropriately (e.g., default to locked if the dependent
ID is missing), or change dependentQuizId's declared type to include undefined
and handle that case in checkQuizAccess usage.
---
Nitpick comments:
In `@config/index.d.ts`:
- Around line 344-353: The QuizAnswer type currently includes a UI-only field
clicked which couples transient UI state into the persisted quiz schema; remove
clicked from the exported QuizAnswer type and instead create a derived UI type
where needed (e.g., in +page.svelte declare QuizAnswerUI = QuizAnswer & {
clicked?: boolean }) and update any consumers accordingly (notably
convert/convertBooks.ts should keep using the pure QuizAnswer for SFM→JSON
conversion). Ensure no other modules rely on QuizAnswer.clicked; if they do,
switch those callers to use the UI-specific type.
In `@src/routes/quiz/`[collection]/[id]/+page.svelte:
- Around line 124-126: The getImageSource function uses data.collection directly
instead of the local destructured variable collection; update getImageSource
(function name: getImageSource) to reference the already-destructured collection
variable (replace data.collection with collection) so it is consistent with
other usages and uses the local binding when building the illustrations key
(illustrations[`./${collection}-${quiz?.id}-${image}`]).
- Around line 284-291: The locked banner is showing the raw quizId; replace it
with the user-facing displayLabel computed in +page.ts so the locked view
matches the navbar. In +page.svelte, use the displayLabel provided by the page
load (e.g., data.displayLabel or the local displayLabel prop) instead of
{quizId}; if displayLabel isn't already passed into the component,
export/forward it from the +page.ts load result so the template can render
{displayLabel} (falling back to quizId only if displayLabel is missing).
🪄 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: ad8c757f-1d25-4b00-b7cf-8d9941a725a7
📒 Files selected for processing (4)
config/index.d.tsconvert/convertBooks.tssrc/routes/quiz/[collection]/[id]/+page.sveltesrc/routes/quiz/[collection]/[id]/+page.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
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/quiz.ts (1)
19-23:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFix compound index key type from
stringto[string, string]— The'collection, book'index is created from['collection', 'book'](line 34), so its key type should be a tuple of the indexed field types, notstring. Declaring it as[string, string]aligns with the idb library's DBSchema typing for compound indexes, eliminates the type mismatch with thegetAll([item.collection, item.book])call at line 84, and allows you to remove the apologetic comment.Proposed changes
interface Quiz extends DBSchema { quiz: { key: number; value: QuizScore; indexes: { - 'collection, book': string; + 'collection, book': [string, string]; date: number; }; }; }- // I have no clue how to make the type system happy here... -Aidan const result = await index.getAll([item.collection, 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/quiz.ts` around lines 19 - 23, The compound index key in the Quiz DB schema is incorrectly typed as string; update the index key type for 'collection, book' from string to a tuple [string, string] in the indexes declaration to match the actual index created from ['collection', 'book'] and the getAll([item.collection, item.book]) usage, and then remove the obsolete comment that apologizes for the mismatch; this aligns the schema typing with the idb DBSchema expectations and fixes the type error.
🧹 Nitpick comments (1)
src/lib/data/quiz.ts (1)
74-93: ⚡ Quick win
openQuiz()return type is now nullable, but onlyaddQuizguards against it.After the typing change on line 26,
openQuiz()resolves to... | null, yetgetQuiz,findQuiz, andcheckQuizAccesscall methods directly on the result. UnderstrictNullChecksthese would be type errors; at runtime they wouldTypeErrorifquizDBever stayednull(e.g., anopenDBrejection on a prior call left state inconsistent). The cleanest fix is to makeopenQuizitself return a non-null handle so every caller benefits.♻️ Suggested change
async function openQuiz() { if (!quizDB) { quizDB = await openDB<Quiz>('quiz', 1, { upgrade(db) { const quizStore = db.createObjectStore('quiz', { keyPath: 'date' }); quizStore.createIndex('collection, book', ['collection', 'book']); quizStore.createIndex('date', 'date'); } }); } return quizDB; }You can then drop the
if (!quiz)check inaddQuizand the barequiz.*accesses in the other helpers become type-safe without further changes.🤖 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/quiz.ts` around lines 74 - 93, openQuiz() now returns a nullable DB handle but callers (getQuiz, findQuiz, checkQuizAccess, addQuiz) call methods directly; change openQuiz to always return a non-null DB handle (or throw on failure) so its return type is non-nullable, update its signature accordingly, and remove the redundant null-guard in addQuiz (the other helpers can keep calling openQuiz() and use the DB methods without extra checks). Ensure references to openQuiz, getQuiz, findQuiz, checkQuizAccess, and addQuiz are updated to the new non-null return type so TypeScript strictNullChecks no longer errors.
🤖 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/quiz.ts`:
- Around line 58-68: The guard using if (bookIndex) is wrong because findIndex
returns 0 for the first match and -1 when not found; update the condition around
bookIndex (from the block that constructs nextItem and calls quiz.add('quiz',
nextItem)) to only proceed when bookIndex is a non-negative number (e.g.,
bookIndex !== undefined && bookIndex >= 0 or Number.isInteger(bookIndex) &&
bookIndex >= 0) so the first book (index 0) is accepted and missing-books (-1)
are rejected; keep the rest of the logic (spreading item, setting date and
bookIndex, calling quiz.add) unchanged.
In `@tailwind.config.cjs`:
- Line 6: The safelist entry only preserves base grid-cols-* classes; update the
safelist pattern configuration (the safelist array entry with pattern
/grid-cols-/) to include responsive variants so classes like md:grid-cols-4 are
kept; add a variants array such as ['sm','md','lg','xl','2xl'] (or the specific
responsive prefixes you use) to that safelist object so Tailwind won't purge
responsive grid-cols utilities.
---
Outside diff comments:
In `@src/lib/data/quiz.ts`:
- Around line 19-23: The compound index key in the Quiz DB schema is incorrectly
typed as string; update the index key type for 'collection, book' from string to
a tuple [string, string] in the indexes declaration to match the actual index
created from ['collection', 'book'] and the getAll([item.collection, item.book])
usage, and then remove the obsolete comment that apologizes for the mismatch;
this aligns the schema typing with the idb DBSchema expectations and fixes the
type error.
---
Nitpick comments:
In `@src/lib/data/quiz.ts`:
- Around line 74-93: openQuiz() now returns a nullable DB handle but callers
(getQuiz, findQuiz, checkQuizAccess, addQuiz) call methods directly; change
openQuiz to always return a non-null DB handle (or throw on failure) so its
return type is non-nullable, update its signature accordingly, and remove the
redundant null-guard in addQuiz (the other helpers can keep calling openQuiz()
and use the DB methods without extra checks). Ensure references to openQuiz,
getQuiz, findQuiz, checkQuizAccess, and addQuiz are updated to the new non-null
return type so TypeScript strictNullChecks no longer errors.
🪄 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: 0a3bf1f1-cb32-4ef2-99a0-c38f9a27ec45
📒 Files selected for processing (5)
config/index.d.tssrc/lib/data/quiz.tssrc/routes/quiz/[collection]/[id]/+page.sveltesrc/routes/quiz/[collection]/[id]/+page.tstailwind.config.cjs
🚧 Files skipped from review as they are similar to previous changes (3)
- config/index.d.ts
- src/routes/quiz/[collection]/[id]/+page.ts
- src/routes/quiz/[collection]/[id]/+page.svelte
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.
Rebase of #888 with some improvements.
Original PR comment from #888:
Summary by CodeRabbit
Refactor
New / Improved UI
Style