Uh oh!
There was an error while loading. Please reload this page.
feat(quiz): a real abandon endpoint, wired to Discard (#537 G4) - #591
Merged
Conversation
Discard had no server side: quiz home wrote a localStorage flag and left the row in_progress until D2's 24h sweep found it, so the resume strip came back on every other device. The route writes the same `abandoned_at` stamp `_sweep_abandoned` writes — the client is just allowed to say "now". Owner-checked like the other attempt routes, 409 on an already-submitted attempt, and idempotent: a second call is a 200 no-op carrying the stamp already on the row, so a retry after a dropped response is free. The write is the same conditional claim submit uses (`completed_at IS NULL AND abandoned_at IS NULL`), so a concurrent submit and abandon cannot both win; the loser re-reads and 409s rather than reporting a discard that never happened. No schema change — `abandoned_at` has existed since D2. `GET /attempts` deliberately still LISTS abandoned rows (it is the history reader, D4); what changes is the derived status, which is what the strip filters on. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
`useQuizHome::discard` is now the whole gesture: `dismissAttempt` hides the row in this browser synchronously (so the strip is gone on the next render whatever the network does), `abandonAttempt` makes it durable, and the refresh re-reads the world. The screen just states the intent — splitting the two writes across the component is how they drifted apart in the first place, with the row hidden here and closed nowhere. A failed abandon is swallowed with a console.warn rather than surfaced: the student said discard, so putting the quiz back on screen (or a red toast over a discard that visibly worked) would both be worse answers, and the backend's 24h sweep is still the backstop. The resume strip needed no filter change — `GET /attempts` reports the derived status, `discoverResumable` already offers only `in_progress` rows, and `getAttempt` now answers `resumable: false`, so both discovery paths drop the attempt on a reload and on any other device. The Chapter 1 resume journey is updated to match and is UNRUN here (the controller owns the stack): it now asserts `abandoned_at` is set, and proves the server is what hides the attempt by wiping both quiz storage keys and reloading — with a positive control first, since "the strip is absent" means nothing unless the same visit was just shown to offer it. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…journey (#537 G4) Important 1 — the cross-device assertion was one round trip too early. `otherDeviceVisit` synced on the attempts listing and the proposal card, both of which land at `setLoaded`; in the REGRESSED case the strip only renders after `discoverResumable` awaits a further `GET /attempts/{id}`, so the count check fired while the failure was still in flight. The helper now returns the listing's parsed body and the journey asserts the SERVER'S status for the attempt (`in_progress` on the positive control, `abandoned` after the discard). The strip check stays as corroboration. M2 — the DB read is synchronized to the abandon POST via the file's own `page.waitForResponse` idiom, not to the click (the strip vanishes optimistically, so the click races the write being checked). M3 — submit's claim now filters `abandoned_at IS NULL` too, symmetric with abandon's. G4 makes the interleaving reachable from the UI (mid-quiz in one tab, Discard in another): the old single-null filter let both claims win and left the row completed AND abandoned, with `_refuse_if_abandoned` only ever a non-atomic pre-read. A lost claim now re-reads before choosing its 409, so a discarded attempt answers QUIZ_ATTEMPT_ABANDONED rather than impersonating a double submit — the frontend maps those codes to different copy. M4 `import type { Page }`. M5 the R-3 row's "Cost if wrong" now describes the failed-call fallback, which is all that is left of it. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Warning Review limit reachedNext included review available in 12 minutes. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (19)
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 |
This pull request has been ignored for the connected project Preview Branches by Supabase. |
Deploying with |
| Status | Name | Latest Commit | Preview URL | Updated (UTC) |
|---|---|---|---|---|
| ✅ Deployment successful! View logs | frontend-staging | b4f1a35 | Commit Preview URL Branch Preview URL | Aug 26 2026, 06:40 PM |
Merge-gate review of PR #591, backend half. D2/D3 — both lost-claim branches read the row back as `(rows or [{}])[0]`, which made a vanished row indistinguishable from an open one: abandon answered 200 `{status: "abandoned", abandoned_at: <now>}` with a timestamp nothing had written, and submit ran `_refuse_if_abandoned({})` as a no-op and reported ALREADY_COMPLETED. An empty re-read is now the 404 the top of each route already gives, and abandon no longer substitutes its own clock for a write it did not make. D3 also put a SELECT in a window that used to be infallible — losing submit's claim went straight to a 409 with no further I/O. A transient PostgREST failure there now degrades back to that 409 instead of 500ing an ordinary double-click. D10/D11 — the select-by-id + 404 + require_self preamble was verbatim in four routes; the already-completed 409 was hand-rolled five times. Both are now `_load_owned_attempt` / `_refuse_if_completed` (+ `_attempt_not_found` / `_already_completed`). D4 (backend half) — QUIZ_ATTEMPT_ABANDONED's sentence said "expired", which is wrong on the path this PR creates. `abandoned_at` records when a row closed, never who closed it, so the copy now covers both. D7/D8/D12 — test fidelity, which is why the above were invisible. `_Attempts` honours its filters on select/select_with_count (a fake that answers every query cannot fail on a dropped `id=eq.`), models `prefer_return_minimal` (the real client returns [] in that mode, so a claim refactored to minimal reads as LOST on every request), and stops duplicating the attempt-row fixtures byte-for-byte from test_quiz_lifecycle_d. `_factory`'s dead `responses` param is gone. D14 — accepted-risk comment at the /answer insert; filed as #597. D15 — noted at the claim that a TTL-dead row's stamp records the click. Refs #537, #591, #597
Merge-gate review of PR #591, frontend half. D1 — `discard` fired `refresh()` synchronously beside the abandon POST, so the re-read routinely PREDATED the write and came back `in_progress`. The strip then stayed hidden only because `discoverResumable` skips locally dismissed ids — the exact localStorage single point of failure G4 exists to retire, and one that fails outright in a private window (session.ts's writeJson no-ops and the abandon error is swallowed, so the quiz the student just discarded comes straight back). It also flashed the whole home screen to a skeleton and re-fired describeConcept to change one row. The slot is now cleared directly. Nothing else on quiz home derives from an unfinished attempt — the ranking and the "missed N last time" join both read COMPLETED attempts only (proposals.ts) — so the re-read bought nothing and raced the write it meant to observe. D9 — the three vitest assertions guarding this passed for the wrong reason: `resumable` was null merely because refresh() had invalidated the load key. They now pin the ordering (the strip is gone while the POST is still in flight, with `status` still "ready"), the read counts, and — the real regression — that the strip stays gone with localStorage unavailable. D4 (frontend half) — QUIZ_ATTEMPT_ABANDONED said "That quiz expired after a day", copy from the TTL-sweep era that reads as a bug to the student who pressed Discard thirty seconds ago. D5/D6 — the journey's `otherDeviceVisit` hardcoded both storage keys, re-declared the listing type and inlined openQuizHome's body; a key rename left the other-device leg green and vacuous. It now imports STORAGE_KEY / DISMISSED_KEY / AttemptsPage from @/lib (precedent: quiz-errors.spec.ts) and calls openQuizHome. The post-discard proposal wait gets SUBMIT_TIMEOUT like every sibling. D13 — the G4 origin story was retold at length in six places; one canonical telling (routes/quiz.py::abandon_attempt) plus pointers. Refs #537, #591
…G4)
Merge-gate re-review, two blockers + a ride-along.
B1 — the D1 regression test ("hides it on the server's word, not on
localStorage's") passed locally and FAILED CI: `expect(isDismissed("open"))
.toBe(false)` got `true`, because the block never took. jsdom is lockfile
pinned, so the only difference is the Node version — CI is on 22 (ci.yml:86),
this box on 26 — and `vi.spyOn(window.localStorage, "setItem")` intercepts on
one and not the other. (`Storage.prototype` intercepts on neither: jsdom hands
out a Proxy.) The premise silently evaporated, leaving the flagship test for
the discard/refresh race asserting nothing on CI.
Blocked at the ACCESSOR instead — `Object.defineProperty(window,
"localStorage", { get() { throw ... } })` — which is what a browser with site
data disabled actually does and what `session.ts::storage()` already catches
into `null`. No dependency on how jsdom hands out the Storage object.
Restored in a `finally` so a failure cannot leak broken storage into the rest
of the file. Re-verified against the pre-fix `discard`: still fails on the
real assertion (the strip comes back), not on the premise.
B2 — the contract spec's R-3 still described Discard as "…then the abandon
call, then a refresh". There is no refresh. Rewritten, and it now also records
that `AbandonResult.abandoned_at` is nullable and that a 200 can carry
`status: "in_progress"`.
Ride-along — the D8 protection did not cover submit's WINNING claim: every
submit test in test_quiz_abandon_g4.py loses the claim for other reasons, so
all five stay green under a minimal-mode refactor. Asserted at the one test
that wins it, the way abandon's claim already does.
Refs #537, #591Uh oh!
There was an error while loading. Please reload this page.
AndresL230 added a commit
that referenced
this pull request
Aug 26, 2026
Two add/add conflicts, both resolved by keeping BOTH sides: * routes/quiz.py — G4 (#591) appended `abandon_attempt` and this branch appended `_gamification_block` at the same point in the file. Kept the route where G4 put it and the helper directly above its caller. * tests/integration/test_quiz_subcutaneous_db.py — G4's abandon tests and G8's inline-XP test landed in the same slot. Both kept. Re-verified against G4's rework of submit: the atomic claim still filters on both `completed_at is.null` and `abandoned_at is.null`, the lost-claim path still re-reads to separate 409-completed from 409-abandoned from 404-deleted, and `_gamification_block` still runs after both `award_xp_safe` and the `check_achievements` pass. TestTheSnapshotIsTakenLast re-verified by mutation (hoisting the call above the award turns it red).
AndresL230 added a commit
that referenced
this pull request
Aug 28, 2026
Two conflicts, both in the files #590 and this branch both edited. models/__init__.py: #590 flipped `include_answer_key`'s default to false and replaced its comment with the canonical lifecycle account; this branch added `source_attempt_id` / `missed_question_hashes` and the validator that ties them together. Kept both — the flipped default and its prose verbatim, the G5 fields ahead of it, the validator after. routes/quiz.py: this branch relocated the agent-call except ladder out of `generate_quiz` into `_generate_or_502` so the re-serve branch can catch a failed top-up and still serve what it recovered; main (#592) added `has_graph=True` to the same call. Kept the relocation and carried the new argument (and its rationale) into the helper. #590's keyless projection, #591's attempt helpers and #592's hoisted lookups merged cleanly and are untouched — re-served questions still flow through the single `_client_questions` call, which strips `question_hash`/`provenance` on both the keyless and the opt-in keyed branch. Two G5 tests were written against the pre-flip default and are re-pointed: the keyed-projection test now opts in explicitly, and the keyless one adopts main's shared `assert_keyless_projection` fixture, which grounds the check in the answer key actually stored for the attempt. That fixture then caught G5's conditional top-level `source` block, so the shared key literal now names optional fields instead of requiring every response to carry them. The F5 assertions narrow to `quiz_reserve.missed_questions`: #592 makes an unrelated reporter fire on the same request under these mocks. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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 freeto 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.
Closes the G4 gap in
docs/superpowers/specs/2026-08-22-quiz-frontend-contract.md(R-3), whose spec row is updated in this PR to say so.
Refs #537
The problem
"Discard" on the quiz resume strip had no server side. It wrote a
dismissedAttemptsentry to localStorage and left the attempt rowin_progressuntil D2's lazy 24h sweep found it — so the strip came back onthe student's phone, in a second tab, and in this browser the moment storage
was cleared.
lib/quiz/session.tscarried the seam asTODO(#537-followup: abandon endpoint).The change
POST /api/quiz/attempts/{attempt_id}/abandon(backend/routes/quiz.py).No schema change — it writes the same
abandoned_atstamp_sweep_abandonedalready writes; the client is just allowed to say "now".
require_self, exactly like the other attempt routes(404 unknown → 403 not yours, in that order).
QUIZ_ATTEMPT_ALREADY_COMPLETEDon a submitted attempt: the score,mastery and XP are paid out and a discard could not take them back.
the row, so a retry after a dropped response is free. A row the TTL sweep
already claimed reports the sweep's timestamp, not a fresh one.
completed_at IS NULL AND abandoned_at IS NULL), so a concurrent submit and abandon cannot both win;the loser re-reads and 409s instead of reporting a discard that never
happened.
statusis produced by_attempt_status, never the literal — this endpointmust not be the one place that can disagree with the read paths.
Submit's claim is now symmetric. It filtered on
completed_at IS NULLalone, with
_refuse_if_abandonedin front of it as a non-atomic pre-read.This PR makes that interleaving reachable from the UI — a quiz open mid-question
in one tab, Discard pressed in another — and both claims would win, leaving the
row completed and abandoned. Submit's claim now requires
abandoned_at IS NULLtoo, and a lost claim re-reads before choosing its 409 so a discardedattempt answers
QUIZ_ATTEMPT_ABANDONEDinstead of impersonating a doublesubmit (the client maps those codes to different copy).
Client:
abandonAttemptinlib/quiz/api.ts, anduseQuizHome::discardas the whole gesture — hide locally (instant), clear the resume slot, abandon
(durable). No refresh: the merge-gate section below has why the original one
had to go. The screen just states the intent. A failed abandon is swallowed with a
console.warn: the student said discard, so resurrecting the quiz would bethe worse answer and the 24h sweep is still the backstop.
The resume strip needed no filter change.
GET /attemptsreports thederived status,
discoverResumablealready offers onlyin_progressrows,and
getAttemptnow answersresumable: false— so both discovery paths dropthe attempt on a reload and on any other device.
GET /attemptsdeliberatelystill lists abandoned rows: it is D4's history reader, and filtering it would
break history to fix the strip.
Test evidence
backend/tests/test_quiz_abandon_g4.py— new, 14 tests. RED10 failed(route absent) → GREEN. Covers the 200 + stamp, the claim's filters, the
idempotent repeat (and that it writes nothing twice), a TTL-swept row, 409 on
completed, the concurrent-submit race, 404, 403 for a foreign attempt
(real
require_selfrestored — the lane stubs it), and thatgetAttempt,the listing,
/answerand/submitall agree afterwards. Its fake modelsthe PostgREST filter grammar rather than returning a canned value, so "the
claim won" and "the claim was refused" are actually distinguishable.
between submit's pre-read and its claim leaves
completed_atnull, pays outno mastery and 409s as
QUIZ_ATTEMPT_ABANDONED— RED without the addedfilter (the claim won and the request ran on past it), GREEN with it — plus a
guard that an ordinary double submit still reads as
QUIZ_ATTEMPT_ALREADY_COMPLETED.ruff check .clean.backend/tests/integration/test_quiz_subcutaneous_db.py— abandon stamps thereal row (read back through psycopg, not PostgREST), 409 after submit, and
the IDOR negative extended to abandon. Integration-marked; not run locally.
7 failedwith the implementation stashed → GREEN102 files / 1152 tests.tsc --noEmitclean,eslint0 errors.frontend/e2e/quiz-journeys.spec.ts— the resume journey now waits on theabandon POST, asserts
abandoned_atis set in the DB, and reloads with bothquiz storage keys wiped (the other-device case). What it pins there is the
listing's own status for the attempt —
in_progressas a positivecontrol,
abandonedafter the discard — because every rendered consequenceof that payload is a further round trip away, so a check on the strip alone
would pass on timing. The strip's absence is kept as corroboration.
Not run in this branch; the lane owns the stack.
Lane runs (overnight 2026-08-23, local stack under the flock, function mode)
abandoned_atset → server statusabandonedon a fresh context → strip absent)GET /attemptsjudged correct; 1 Important — the cross-device e2e assertion could fire before a regression surfaced → fixed by asserting the server's own status first; submit's atomic claim made symmetric with abandon's) + scoped re-review clean.Merge-gate review (2026-08-26)
origin/mainmerged in (f6ed004 / 6225bba / 025474a — no conflicts; #590'sinclude_answer_keyflip is in the generate handler, clear of these hunks).Two commits on top, from a
/code-reviewpass whose findings were verifiedagainst the code before being acted on.
The race the PR re-created (the one that mattered)
useQuizHome::discardfiredrefresh()synchronously beside the abandonPOST. The re-read routinely predated the write and came back
in_progress, sothe resume strip stayed hidden only because
discoverResumableskips locallydismissed ids — the localStorage single point of failure G4 exists to retire.
With storage unavailable (private window:
session.ts::writeJsonno-ops andthe abandon error is swallowed) the discarded quiz came straight back. It also
cost a full home-screen skeleton flash and a re-fired
describeConceptperdiscard.
Fixed by clearing the resumable slot directly and dropping the global re-read.
Nothing else on quiz home derives from an unfinished attempt — the ranking
and the "missed N last time" join both read COMPLETED attempts only
(
proposals.ts) — so the re-read bought nothing and raced the write it meantto observe.
dismissAttemptstill runs first, as the across-loads backstop.Honest failures on both claim losers
(rows or [{}])[0]thenabandoned_at = current.get(...) or nowreported{status: "abandoned", abandoned_at: <now>}for a row that may not exist, with a timestamp nothingwrote. An empty re-read is now the same 404 the top of the route gives, and
the branch never substitutes its own clock for a write it did not make.
_refuse_if_abandoned({})is a no-op,so a vanished row reported ALREADY_COMPLETED), and the new SELECT sat in a
window that used to be infallible — losing the claim went straight to a 409.
A transient PostgREST failure there now degrades back to that 409 rather
than 500ing an ordinary double-click.
_load_owned_attemptand_refuse_if_completedreplace the select+404+require_selfpreamble (verbatim in four routes) and the hand-rolledalready-completed 409 (five copies).
Copy
QUIZ_ATTEMPT_ABANDONEDstill read "That quiz expired after a day" — copyfrom the TTL-sweep era, and a bug to the student who pressed Discard thirty
seconds ago.
abandoned_atrecords when a row closed, never whoclosed it, so nothing on the wire can tell the two apart: both the client copy
and the server sentence now cover both. Contract table updated to match.
Test fidelity (why the above were invisible)
_Attempts.select/select_with_countignored their filters and alwaysreturned the stored row, so no test could fail on a dropped or mis-built
id=eq./user_id=eq., and the history-listing assertion was pinned on aread nobody made. Both honour filters now, and the listing test asserts what
the fake was asked for.
prefer_return_minimal, butdb/connection.pyreturns[]in that mode — a claim refactored to minimal would read as LOST on every
request (every submit 409s, no payout) with the suite green. Modelled, plus
an explicit assertion that abandon's claim is not minimal.
resumablewas null merely becauserefresh()had bumped the load key. They now pin the ordering (strip gonewhile the POST is still in flight,
statusstill"ready"), the readcounts, and the storage-unavailable case that is the real regression.
otherDeviceVisithardcoded both storage keys, re-declaredthe listing type and inlined
openQuizHome's body — a key rename would haveleft the other-device leg green and vacuous. Now imports
STORAGE_KEY/DISMISSED_KEY/AttemptsPagefrom@/lib(precedent:quiz-errors.spec.ts). The post-discard proposal wait gotSUBMIT_TIMEOUTlike every sibling.
telling (
routes/quiz.py::abandon_attempt) plus pointers.Follow-up: #597
POST /attempts/{id}/answeris the last quiz write path guarded by a pre-readalone. G4 turns "the attempt closed mid-request" from a 24h TTL event into a
button, so a Discard landing between the refusals and the insert records a
graded response on a closed attempt. PostgREST cannot make that cross-table
INSERT conditional atomically — closing it needs a trigger or an RPC, i.e. a
migration — so it ships as an accepted risk with a comment at the insert.
Inert rather than harmful: an abandoned attempt can never be submitted
(submit's claim filters on
abandoned_at IS NULL), so the orphan pays out nomastery, XP or achievement. Filed as #597.
Two notes, deliberately not changes
abandoned_aton a TTL-dead row records the discard CLICK, not when theattempt went quiet. Noted at the claim. It matters only if a consumer ever
reads the column as an elapsed time; none does today (the derived status
only asks whether it is set), and "when did this go quiet" is
created_atplus the last response — what
_attempt_statusalready computes.toHaveCount(0)used to witness the skeletonrather than the optimistic hide, since
refresh()blanked the screen. Withthe race fix there is no skeleton, so the paired
expect(quiz-proposal).toBeVisible()is what makes it mean "gone from arendered screen"; that is now stated where the assertion is.
One push-back
D2 also asked to drop the
or nowfallback on abandon's winning claim.Left in place: there,
nowis the value the request itself PATCHed, sopreferring the echoed column and falling back to it yield the same timestamp —
it is not a fabrication. The fabricated one was the loser branch's, and that
is gone.
Verification
pytest tests/ -q✅ 2247 passed / 82 skipped;ruff check .✅vitest run✅ 1154 passed / 102 files;tsc --noEmit✅;npm run lint✅ 0 errorsnpx playwright test e2e/quiz-journeys.spec.ts --list✅ collects 9 tests(the
@/libimports resolve). The journey itself is unrun here — thestack is the controller's.