Repository navigation
feat: keep learner answers through a reload or crash - #628
Open
NoahFreelove wants to merge 3 commits into
Open
NoahFreelove wants to merge 3 commits into
NoahFreelove wants to merge 3 commits into
Conversation
Answers only existed in the Zustand store until the learner pressed Submit, so a refresh, closed tab, crash, or expired session lost everything typed since the attempt began. submitAssignment is called from exactly two places — the Submit button and timer expiry — and nothing else ever wrote the answers anywhere. Mirror them into localStorage instead. Edits schedule a debounced write keyed by attempt id; reopening the attempt merges the draft back over the questions fetched from the server. The merge is one-directional: the server's copy wins every field it already holds, and the draft only fills gaps. A stale local value can therefore never overwrite a graded answer, which is the failure that would matter most. Storage stays bounded without a cleanup job. Starting or resuming an attempt drops every draft but that one, so a learner accumulates at most a single entry no matter how many assignments they open, and a fresh attempt cannot inherit a previous one's answers. Submitting clears the entry and cancels any pending write, so a debounced save cannot land afterwards and resurrect what was just submitted. Entries also carry a timestamp and are discarded after a week. Every storage access is wrapped: quota limits, Safari private mode, and server rendering all fail closed, because losing crash-recovery must never break the page the learner is using. Uploads are excluded — a File cannot be revived from localStorage, and storing its name would promise a recovery that never happens.
The hook was introduced already commented out and never ran in production. Its only importer was its own test file, and the four question components carried nothing but a commented-out call, so it contributed no behaviour and shipped in no bundle. Answers are now kept through a reload by the local draft store, which needs neither this hook nor a request per keystroke, so keeping a second unreachable implementation around only invites someone to wire up the wrong one. `submitQuestion` and the server route it calls are left in place. They are a working per-question save endpoint and the natural foundation if this ever becomes a server-side feature; only the dead client hook goes.
Review of this branch found the storage-bounding claim was not true in the shipped flow: hydration sets the attempt id through the generic setLearnerStore, so the named setter carrying clearOtherDrafts was dead code and a draft accumulated for every attempt ever opened. The attempt-change side effects now live in setLearnerStore itself — cancel the debounced write captured for the previous attempt, then prune every other draft — and the named setter routes through it. Hydration also prunes directly, because a reload resumes the same id and skips the store update entirely. A wiring test drives the real store the way the page does; the unit suite alone proved the functions correct while nothing called them. True/false answers were the one answer type the draft skipped: DraftAnswer had no boolean field and setAnswerChoice never scheduled a save, so exactly the recovery this feature promises was missing for those questions. The field round-trips now, checked by type rather than truthiness everywhere, because false is a chosen answer and must neither vanish from the draft nor beat a server-held false in the merge. Restored drafts are now validated field by field. loadDraft trusted the envelope and returned entries as-is, so a hand-edited object where a string belongs would flow into the store and break rendering on every visit for up to a week — localStorage is user-editable input like any other. Non-string fields, non-boolean choices, oversized values, and keys that are not question ids are dropped on read; a timestamp from the future is discarded like an expired one. Erasing every answer now actually removes the draft: pickAnswer stored empty strings, so the documented clear-on-erase behavior only worked in a test that fed it a state the store never produces. The test now uses the real erased shape, and the module header no longer overclaims multi-tab safety — two tabs are last-writer-wins, which is acceptable for crash recovery and now said plainly.
|
This pull request has had no activity for 30 days, so it has been marked |
skills-network-bot
Bot
added
the
stale
This branch has not been deployed
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.
PR Description
Overview:
Learner answers only existed in the Zustand store until Submit, so a refresh, crash, closed tab, or expired session lost everything typed since the attempt began. Answers are now mirrored into localStorage (debounced, keyed by attempt id) and merged back when the attempt reopens. The merge is one-directional — the server's copy wins every field it already holds, so a stale draft can never overwrite a graded answer. Also removes the never-shipped per-question autosave hook (its only importer was its own test).
Post-review hardening in the final commit:
setLearnerStore, bypassing the named setter that pruned old drafts. Attempt-change side effects now live insetLearnerStoreitself (cancel the pending debounced write, prune other drafts), the named setter routes through it, and hydration prunes directly for the same-id reload case.Type of Issue:
feat): New functionality or feature added.bug): Issue or bug resolved.chore): Maintenance, refactoring, or non-functional changes.doc): Documentation improvements or additions.Change Type:
Test Coverage
Evidence:
tsc --noEmitadds no new errors (3 pre-existing in author-store tests), eslint clean.__proto__entry), and future timestamps are all dropped on read.Impact / Risk
Filecannot be revived from localStorage).pagehide, so up to ~800ms of typing can miss the draft on a crash. The pre-existing zustand persist of all answers (no TTL) is a separate follow-up candidate.Rollback
git revert e933d82a 3b337075 349c9817— client-only; stalemark:draft:v1:*entries left in browsers expire via their own TTL check or sit inert.Reviewer Focus
mergeDraftIntoQuestions, especially the type-not-truthiness check forfalsetrue/false answers.setLearnerStore(stores/learner.ts) — it also runs on the timer-expiry path, whereactiveAttemptIdgoes tonulland every draft is dropped.lib/learner-draft.ts(MAX_DRAFT_FIELD_CHARS,MAX_DRAFT_CHOICES).