Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
WalkthroughChangesStandard-Zen draft persistence
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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/components/quiz/ZenQuizContainer.tsx (1)
164-186: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winDraft-flush failure during completion strands the user with no retry path.
When
flushDraft(true)fails inside the completion effect,handleSessionCompleteis never called, sosaveErrornever becomes true and the existing "We couldn't save your results" retry UI (inquizContent) never renders. SincepauseTimer()already fired,secondsstops changing, and the effect (keyed onseconds) won't re-run — the user is left on the "Initializing quiz session..." screen (lines 404-436) with, at best, a staticpersistenceNoticebanner and no retry button.handleExithandles the same failure with an explicit toast (line 196-199); the completion path doesn't.🛠️ Minimal mitigation (toast) — a full fix also needs a retry action reachable from the initializing branch
void (async (): Promise<void> => { if (draftEligible && !(await flushDraft(true))) { throw new Error("Draft flush failed before result creation."); } await handleSessionComplete(elapsedSeconds); })().catch(() => { hasSavedResultRef.current = false; + addToast( + "error", + "Your latest progress was not saved. Please try finishing the quiz again.", + ); });Also applies to: 404-436
🤖 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/components/quiz/ZenQuizContainer.tsx` around lines 164 - 186, Update the completion flow in the effect around flushDraft and handleSessionComplete so a draft-flush failure is surfaced through the existing save-error/retry mechanism or an explicit user-facing toast, rather than only resetting hasSavedResultRef. Ensure the initializing branch around quizContent also exposes a reachable retry action after failure, allowing completion to be attempted again while preserving the paused timer state.
🧹 Nitpick comments (5)
tests/e2e/zen-draft.spec.ts (1)
109-122: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueShare the runtime cache name instead of duplicating it.
public/sw.jsalready definesRUNTIME_CACHE = "certprep-runtime-v5", buttests/e2e/zen-draft.spec.tsrepeats the literal. Move this to a shared test/source constant so the cache-version bump stays in one place.🤖 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 `@tests/e2e/zen-draft.spec.ts` around lines 109 - 122, Replace the duplicated "certprep-runtime-v5" literal in the cache assertion within the zen draft test with a shared constant derived from the existing RUNTIME_CACHE definition in public/sw.js. Expose or reuse that constant through the project’s established shared test/source mechanism so future cache-version updates require changing only one value.tests/unit/components/quiz/ZenQuizContainer.test.tsx (1)
218-229: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winMissing coverage for the
sessionKeyMappingsdraft-disabling branch.Per
ZenQuizContainer.tsx,draftEligiblealso requiressessionKeyMappings === null. Theit.eachmatrix coverssessionKind,isSmartRound,isTopicStudy,isSRSReview,isInterleavedbut omits a case withsessionKeyMappingsset to a non-null value.Suggested addition
it.each([ ["remixed Zen", { sessionKind: "remixed_zen" as const }], ["Smart Round", { isSmartRound: true }], ["Topic Study", { isTopicStudy: true }], ["SRS Review", { isSRSReview: true }], ["Interleaved Practice", { isInterleaved: true }], + ["remixed key mappings", { sessionKeyMappings: { a: "a" } }], ])("never enables standard drafts for %s", (_label, 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 `@tests/unit/components/quiz/ZenQuizContainer.test.tsx` around lines 218 - 229, Add a `sessionKeyMappings` case to the existing “never enables standard drafts” `it.each` matrix in the ZenQuizContainer tests, using a non-null mapping value, and assert it produces `draftEligible: false` like the other disabling conditions.tests/unit/components/dashboard/QuizCard.test.tsx (1)
82-100: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winTest title implies exclusivity but only covers the positive case.
"shows Continue Quiz only when a compatible local draft exists" doesn't assert the negative case (no
hasResumableDraft/false→ link absent). Consider adding that assertion to actually validate the "only when" claim.Suggested addition
+ it("hides Continue Quiz when no resumable draft exists", () => { + render( + <QuizCard quiz={quiz} stats={attemptedStats} onStart={vi.fn()} onDelete={vi.fn()} />, + ); + expect( + screen.queryByRole("link", { name: "Continue Quiz" }), + ).not.toBeInTheDocument(); + });🤖 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 `@tests/unit/components/dashboard/QuizCard.test.tsx` around lines 82 - 100, Extend the “shows Continue Quiz only when a compatible local draft exists” test to also render QuizCard without hasResumableDraft (or with it set to false) and assert the “Continue Quiz” link is absent, while preserving the existing positive-case assertions.tests/unit/components/quiz/hooks/useQuizSession.test.tsx (1)
34-46: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winNo test exercises
useQuizSession's draft field mapping.The new
useZenDraftSessionmock is wired in but no test in this file passesdraftEligible: trueor asserts thatdraftDecision/draftSaveStatus/resumeDraft/etc. are correctly derived fromuseZenDraftSession's return values. This mapping is exercised indirectly viaZenQuizContainer.test.tsx, but a direct unit test here would pin down this hook's specific contract.🤖 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 `@tests/unit/components/quiz/hooks/useQuizSession.test.tsx` around lines 34 - 46, The useZenDraftSession mock is present, but useQuizSession’s draft field mapping lacks direct coverage. Add a focused useQuizSession test with draftEligible: true and non-default mocked draft values, then assert draftDecision, draftSaveStatus, and the resume/start-over/resume-as-new-attempt/flushDraft handlers are mapped from useZenDraftSession correctly.src/components/quiz/ZenQuizContainer.tsx (1)
412-420: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicated
exitDescriptionternary.The same draft-status-dependent description logic is repeated verbatim across both
QuizLayoutrender branches. Extract once to avoid drift.♻️ Suggested extraction
+ const exitDescription = draftEligible + ? draftSaveStatus === "conflict" + ? "A newer tab owns this saved draft. Exiting will not overwrite it." + : draftSaveStatus === "error" + ? "Your latest progress has not been saved. Close this dialog and retry after the device save succeeds." + : "Your progress is saved on this device. You can continue this quiz later." + : "Exiting ends this session. This mode does not save resumable progress."; + if (isInitializing || !currentQuestion) { return ( <QuizLayout ... - exitDescription={ - draftEligible - ? draftSaveStatus === "conflict" - ? "..." - : draftSaveStatus === "error" - ? "..." - : "..." - : "..." - } + exitDescription={exitDescription} >Also applies to: 446-454
🤖 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/components/quiz/ZenQuizContainer.tsx` around lines 412 - 420, Extract the draft-status-dependent description currently assigned to exitDescription into a single local value in the containing render scope, using draftEligible and draftSaveStatus. Reuse that value in both QuizLayout branches, including the branches around the existing exitDescription assignments, and preserve all current messages and fallback behavior.
🤖 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/components/quiz/hooks/useZenDraftSession.ts`:
- Around line 337-390: The session-initialization effect around load must not
rerun for unrelated quiz-record reference changes, because its cleanup invokes
flushDraft and resetSession. Stabilize the quiz input returned by useQuiz or
narrow this effect’s dependencies to the initialization seed fields used by
resolveQuizHash and currentQuiz—quiz.id, quiz.quiz_hash, and
quiz.questions/version—while keeping title or option updates out of this
initialization lifecycle.
In `@src/hooks/useDatabase.ts`:
- Around line 205-223: Update the useLiveQuery callback in the zen draft status
flow to catch failures from assessZenDraft or related database reads and return
a usable empty or partial Map<string, ZenDraftCompatibility> instead of
rejecting. Preserve successful per-draft compatibility results and ensure
DashboardClient can finish loading when an individual status assessment fails.
- Around line 207-210: Update the quizzes query in the Promise.all within the
draft-status recomputation to filter by userId before converting results to an
array, matching the user-scoped query pattern used by zenDrafts and other
queries in useDatabase.
In `@tests/unit/components/dashboard/DashboardClient.empty-states.test.tsx`:
- Around line 206-265: Replace the internal debugging narration in the test case
“renders styled search empty state when category filter hides all quizzes” with
a single concise comment stating that zero results require combining the
selected category with a nonmatching search term, since category options always
correspond to existing quizzes. Keep the test behavior and assertions unchanged.
In `@tests/unit/components/quiz/hooks/useZenDraftSession.test.tsx`:
- Around line 106-116: Update the test around the IndexedDB wait and the
saveMessage assertion so it also waits for the hook state to re-render after
persistence completes. Poll or await result.current.saveMessage until it equals
"Saved on this device." instead of asserting it synchronously immediately after
the database wait.
---
Outside diff comments:
In `@src/components/quiz/ZenQuizContainer.tsx`:
- Around line 164-186: Update the completion flow in the effect around
flushDraft and handleSessionComplete so a draft-flush failure is surfaced
through the existing save-error/retry mechanism or an explicit user-facing
toast, rather than only resetting hasSavedResultRef. Ensure the initializing
branch around quizContent also exposes a reachable retry action after failure,
allowing completion to be attempted again while preserving the paused timer
state.
---
Nitpick comments:
In `@src/components/quiz/ZenQuizContainer.tsx`:
- Around line 412-420: Extract the draft-status-dependent description currently
assigned to exitDescription into a single local value in the containing render
scope, using draftEligible and draftSaveStatus. Reuse that value in both
QuizLayout branches, including the branches around the existing exitDescription
assignments, and preserve all current messages and fallback behavior.
In `@tests/e2e/zen-draft.spec.ts`:
- Around line 109-122: Replace the duplicated "certprep-runtime-v5" literal in
the cache assertion within the zen draft test with a shared constant derived
from the existing RUNTIME_CACHE definition in public/sw.js. Expose or reuse that
constant through the project’s established shared test/source mechanism so
future cache-version updates require changing only one value.
In `@tests/unit/components/dashboard/QuizCard.test.tsx`:
- Around line 82-100: Extend the “shows Continue Quiz only when a compatible
local draft exists” test to also render QuizCard without hasResumableDraft (or
with it set to false) and assert the “Continue Quiz” link is absent, while
preserving the existing positive-case assertions.
In `@tests/unit/components/quiz/hooks/useQuizSession.test.tsx`:
- Around line 34-46: The useZenDraftSession mock is present, but
useQuizSession’s draft field mapping lacks direct coverage. Add a focused
useQuizSession test with draftEligible: true and non-default mocked draft
values, then assert draftDecision, draftSaveStatus, and the
resume/start-over/resume-as-new-attempt/flushDraft handlers are mapped from
useZenDraftSession correctly.
In `@tests/unit/components/quiz/ZenQuizContainer.test.tsx`:
- Around line 218-229: Add a `sessionKeyMappings` case to the existing “never
enables standard drafts” `it.each` matrix in the ZenQuizContainer tests, using a
non-null mapping value, and assert it produces `draftEligible: false` like the
other disabling conditions.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 3ade4164-deaa-4331-aa90-8fef7e8ab1ca
📒 Files selected for processing (42)
docs/ARCHITECTURE.mddocs/plans/2026-07-22-local-zen-draft-persistence.mdsrc/app/globals.csssrc/app/quiz/[id]/zen/page.tsxsrc/components/dashboard/DashboardClient.tsxsrc/components/dashboard/DashboardSkeleton.tsxsrc/components/dashboard/QuizCard.tsxsrc/components/dashboard/QuizGrid.tsxsrc/components/dashboard/QuizSortControls.tsxsrc/components/quiz/QuizLayout.tsxsrc/components/quiz/ZenQuizContainer.tsxsrc/components/quiz/hooks/useQuizPersistence.tssrc/components/quiz/hooks/useQuizSession.tssrc/components/quiz/hooks/useZenDraftSession.tssrc/db/dbInstance.tssrc/db/index.tssrc/db/results.tssrc/db/zenDrafts.tssrc/hooks/useDatabase.tssrc/hooks/useQuizSubmission.tssrc/lib/dataExport.tssrc/stores/quizSessionStore.tssrc/types/quiz.tssrc/types/zenDraft.tstests/e2e/helpers/db.tstests/e2e/offline-sync.spec.tstests/e2e/zen-draft.spec.tstests/unit/components/dashboard/DashboardClient.empty-states.test.tsxtests/unit/components/dashboard/DashboardClient.test.tsxtests/unit/components/dashboard/DashboardSkeleton.test.tsxtests/unit/components/dashboard/QuizCard.test.tsxtests/unit/components/dashboard/QuizGrid.test.tsxtests/unit/components/dashboard/QuizSortControls.test.tsxtests/unit/components/quiz/QuizLayout.test.tsxtests/unit/components/quiz/ZenQuizContainer.test.tsxtests/unit/components/quiz/hooks/useQuizSession.test.tsxtests/unit/components/quiz/hooks/useZenDraftSession.test.tsxtests/unit/db/standardZenCompletion.test.tstests/unit/db/zenDraftMigration.test.tstests/unit/db/zenDrafts.test.tstests/unit/stores/quizSessionStore.test.tstests/unit/styles/dashboard-theme-overrides.test.ts
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
Owner
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
TJZine
changed the title
Merged
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
1 participant
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.
📋 Description
🔗 Related Issues
🧪 Type of Change
📸 Screenshots
🧪 How Has This Been Tested?
Test Configuration:
✅ Checklist
📝 Additional Notes
Summary by CodeRabbit
New Features
Bug Fixes
Documentation