Repository navigation
fix: the PWA cached a build and never handed over, so deploys were in… - #18
Merged
Merged
Conversation
…visible
vite.config.ts sets registerType: 'prompt'. That means a newly deployed
service worker installs, reaches the 'installed' state, and then WAITS until
something calls updateServiceWorker(). src/pwa.ts exported the hook for that
and said, in its own comment, that the component belonged to another
workstream. No such component was ever written and nothing imported the hook.
So the waiting worker waited forever. Anyone who had opened the site once kept
being served the cached old build on every subsequent visit, indefinitely.
Every deploy was invisible to exactly the people who use the app most.
This is how a sign-in page could be live, correct, reachable and verified on
the server - the bundle fetched over curl contained it - while a returning
visitor still saw the version from before it existed. The server was right and
the browser was serving something else, and nothing in the app could ever
correct that.
WHY NOT SWITCH TO autoUpdate
Because 'prompt' is the right choice and the comment defending it is right: an
automatic activation reloads the page, and doing that during a 120-minute mock
test is how a candidate loses a paper. The quiz engine autosaves, so it would
probably survive - "probably" is not good enough for a licence rehearsal. The
fix is to build the missing half, not to remove the safeguard.
WHAT IT DOES
* needRefresh -> offers an update. Never takes it. A test asserts that
updateServiceWorker is not called until the student clicks.
* offlineReady -> confirms once on first install, then auto-dismisses. This
app is genuinely useful offline and that is worth saying once.
* An hourly registration.update() poll, because browsers only check for a
new worker on navigation - a tab left open all day would otherwise never
notice a deploy.
Neither state is a modal and neither traps focus: an interruption that blocks
the page is worse than a stale asset. role="status" with aria-live="polite" -
worth knowing, not worth cutting off a screen reader mid-sentence.
Positioned above the bottom navigation on phones, using the same 60px + safe
area inset that AppShell reserves, because covering the navigation with a
notice would strand someone on whatever page they were on. Both buttons are
44px: this can appear mid-question, and a mis-tap that reloads is precisely
what must not happen.
440 -> 449 tests.
NOTE FOR ANYONE ALREADY AFFECTED: this fix cannot reach a browser that is
already pinned to an old worker, because that browser will not load the new
code that contains it. One hard reload, or unregistering the worker in
DevTools, is needed once. After that the prompt handles every future deploy.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
MockTestQuiz loaded questions from the static chapter JSON through localData,
which carries correct_option_index, and the engine scored locally. The answer
key was one network-tab open away, which makes a mock exam worthless as a
rehearsal for a licence examination. The Worker has scored papers server-side
all along; nothing called it.
When a backend is configured the page now starts a real attempt
(POST /api/mock/start), renders the server's paper, autosaves option refs, and
submits for the server to mark. With no backend configured the local path is
untouched and still works - that deployment is supported, not legacy.
THE correctIndex PROBLEM
Question.correctIndex is a required number and the engine scores off it. A
remote paper has no key at all: AttemptQuestion has no field to carry one.
The fix is a second named sentinel in features/quiz/answerKey.ts, beside the
-1 that already meant "the local bank has no usable key":
NO_ANSWER_KEY = -1 local bank has no key
KEY_HELD_BY_SERVER = -2 a key exists; only the server has it
A discriminated union would make reading a key off a remote question a compile
error rather than a value check, and it was rejected deliberately:
correctIndex is part of localData.Question, which is the element type of
QuizState.questions, the draft companion arrays, loadQuestionsByIds,
describeReference and every quiz component's props. Splitting it ripples
through the persistence format and all four quiz surfaces to re-prove a
guarantee the wire already enforces absolutely.
What the sentinel buys is that -2 is < 0, so every guard that already refused
to score an unkeyed question refuses to score a server-keyed one with no new
branch - a future reader who has never seen that file still falls into the
safe path. Faking a real index, say 0, would have made the engine score
locally and reveal a verdict mid-exam: precisely the bug being removed.
One gap the sentinel alone left: after revealAll, selectVerdict would have
reported 'unkeyed' - "no answer key is available" - which is false. Verdict
gained 'withheld': there is a key, it was graded, not here.
ALSO
* useQuiz gained restoredSelections and deadlineFromDuration, so a paper
resumes on a device that never wrote a local draft and recomputes its
deadline from the server's remaining time rather than trusting an instant
this device wrote against its own clock.
* services/mockAttempt.ts is pure translation - server paper to engine
questions, selections to opaque refs, server summary to the result screen -
and persists both idempotency keys BEFORE their requests, so a lost
response is recoverable rather than a second attempt.
* A failed start shows the server's message and does NOT fall back to local
scoring. Falling back would silently restore the cheatable path.
449 -> 492 tests. The ones that matter assert the absence of things: no /data/
request on the remote path, no verdict mid-paper however the student answers,
and submission carrying refs rather than indexes.
NOT VERIFIED AGAINST THE LIVE WORKER. Everything is jsdom against a mocked
api client - no end-to-end run, no real refs, no real rate limiting. The
deadline the engine holds also lands a few hundred milliseconds after the
server's, so an answer in that window can be dropped by the server clamp;
bounded by autosave, not eliminated.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
App.css predates the design system: 5,203 lines, 408 class names, and only 6
media queries. 39 of those 408 classes had any rule inside one. 369 had none -
the entire landing page below the nav, the app topbar, dashboard cards,
practice hub, mock tests, results, chapters, bookmarks, profile, settings,
flashcards. On a phone-first product, for candidates studying on phones.
THE BUG THIS FOUND
Every grid is repeat(auto-fit, minmax(Npx, 1fr)). minmax()'s lower bound is a
hard floor, and body carries overflow-x: hidden - so content past the viewport
is CLIPPED rather than offered a scrollbar. .tests-grid has a 300px floor and
overflows its 288px container on any 320px phone; nine more grids with
180-280px floors overflow at 280px. Silent, and invisible to anyone testing at
desktop width.
Each grid now uses minmax(min(100%, Npx), 1fr), keeping its OWN N - collapsing
.progress-grid's 240px onto a shared 280px would drop it from two columns to
one at ~600px, which is a regression rather than a fix. If a very old WebView
cannot parse min(), the declaration is invalid and the base rule survives, so
the fallback is exactly today's behaviour.
THE :root BLOCK WAS DEAD
Its 15 literals were already shadowed by src/index.css's :root:root (0,2,0
against 0,1,0), so the rendered values were the tokens, not what the file
said. --accent-yellow rendered #f59e0b while reading #fbbf24; --text-muted
rendered --color-fg-muted while reading #71717a. Anyone reading App.css to
learn a colour learned the wrong one. Now it references the tokens it was
already resolving to.
WHAT MOVED
* 36 colour literals to exact primitives, every swap byte-identical, plus 5
dead fallbacks removed - var(--text-muted, #94a3b8) could never render its
literal because --text-muted is always defined.
* 84 font-sizes and 49 border-radii to the scale, exact matches only.
* A new max-width: 640px block taking responsive coverage from 39 of 408
class names to 81.
It sits at the END of the file on purpose: every rule in it is (0,1,0), the
same specificity as what it corrects, and the chapter-selector, books and
flashcards blocks are defined after the existing 640px query - a rule placed
there would lose to them.
WHAT DELIBERATELY DID NOT MOVE
142 rgb()/rgba() values, because the token layer has no alpha vocabulary and
tokens.css rejects color-mix() for old Android WebViews; mapping
rgba(255,255,255,0.1) to --color-border would render #e4e4e7 in light mode, a
visible change. 31 hex literals with no token within reach, or that match a
semantic token in dark mode only and would flip in light. 76 border-radius
values whose commonest sizes - 12px, 8px, 16px - have no token at all;
--radius-sm is 6px, two off. All font-weights, all spacing, two of three
transition durations, and z-index, which runs 2-200 here while the --z- scale
starts at 1000.
The rule throughout: an inexact swap is a visual change wearing a refactor's
clothing.
SIZE - THE TARGET WAS MISSED, AND THE NUMBER IS REPORTED
Render-blocking CSS 14.30 -> 14.74 kB gzip, +436 B, +3.0%. The brief asked for
flat or down. The token work was byte-neutral (+76 B across three slices); the
whole increase is ~100 lines of responsive CSS that did not exist before,
mostly unique selectors, which gzip cannot fold away. Flat was reachable only
by giving up either the type scale or the phone rules. Both were kept and the
number is stated rather than buried.
NOT VISUALLY VERIFIED. Nobody looked at a rendered page. Safety rests on
slices 1-3 being value-identical substitutions, checked against both themes,
using only theme-independent primitives; and on slice 4 living entirely inside
max-width queries, so nothing above 640px changes at all. 492 tests, tsc,
eslint and the build all clean.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Removing the landing page's name modal took the markup and left 83 lines styling it. scripts/find-dead-css.mjs reports the six class names as unreachable, which they are - Landing.tsx renders no name-portal or dialog element at all any more. Two of them, .name-portal-header and .name-portal-input, survived the tool's earlier pass only because Landing.module.css MENTIONS them in a comment, and the tool deliberately counts a mention anywhere as use. That bias is right - it is what stops the tool deleting something reachable - and the cost is that debris like this needs a second pass once the comment is the last reference. Render-blocking CSS 14.74 -> 14.60 kB gzip. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Every /api/* response went out with no Cache-Control and no Expires -
including GET /api/auth/session, which answers "who is signed in on this
browser". Verified against the deployed Worker: the only header of that family
was `vary: Origin`.
A response with no cache directives is not forbidden from being stored. It is
left to heuristics, and the caches that apply those heuristics include
corporate proxies, ISP caches, and anything a Cloudflare Cache Rule is later
pointed at. `vary: Origin` does not help, because it does not vary on the
session cookie. The failure mode is one student being served another
student's identity on a shared connection.
Cloudflare's defaults almost certainly were not caching these - no extension
in the path, and cf-cache-status was absent - but that is a default, not a
guarantee, and it is one Cache Rule away from changing.
WHAT CHANGED
* `send()` sets `Cache-Control: no-store` unless the handler overrides it.
`no-store` rather than `no-cache`: no-cache permits storing and merely
requires revalidation, which still leaves a copy of a personalised
response on disk.
* The error boundary and the 404 handler set it too. An error can carry a
rate-limit state or a message about a specific account, and the 404's path
is attacker-controlled.
* `Vary` now names Cookie as well as Origin. With no-store nothing should be
stored at all; this is the second line, so that relaxing an endpoint to
public later cannot quietly become a cross-account leak. The CORS
middleware runs after send() and was overwriting Vary with Origin alone,
so it had to be fixed in both places.
* `HandlerResult.cacheControl` is the opt-IN for genuinely public content.
Opt-in, never opt-out: an endpoint written next year is private without
its author thinking about it.
WHAT WAS ALREADY RIGHT, AND WAS CHECKED
* Session tokens: 128 bits from crypto.getRandomValues.
* The cookie value is never a database key - the row id is its SHA-256, so a
dump of `sessions` yields nothing anyone can sign in with.
* Every lookup checks revoked_at IS NULL, both the idle and the absolute
expiry, and that the user is still active. The idle slide is clamped to
the absolute ceiling, so sliding cannot renew a stolen cookie forever.
* Cookie flags: HttpOnly, SameSite=Lax, Secure in production, Path=/.
* Login mints a NEW session rather than reusing the caller's, so session
fixation does not apply.
* Logout revokes server-side, not just in the browser.
* The service worker caches only same-origin /data/ and fonts. The API is a
different origin, so no authenticated response can enter that cache, and
/api/ is denylisted from the navigation fallback besides.
STILL OPEN, not fixed here: logging in does not revoke the session the caller
arrived with. The browser overwrites that cookie so it is unreachable in
practice, but a copy stolen beforehand stays valid until it expires.
116 -> 122 worker tests.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
This branch was successfully 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
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.
…visible
vite.config.ts sets registerType: 'prompt'. That means a newly deployed
service worker installs, reaches the 'installed' state, and then WAITS until
something calls updateServiceWorker(). src/pwa.ts exported the hook for that
and said, in its own comment, that the component belonged to another
workstream. No such component was ever written and nothing imported the hook.
So the waiting worker waited forever. Anyone who had opened the site once kept
being served the cached old build on every subsequent visit, indefinitely.
Every deploy was invisible to exactly the people who use the app most.
This is how a sign-in page could be live, correct, reachable and verified on
the server - the bundle fetched over curl contained it - while a returning
visitor still saw the version from before it existed. The server was right and
the browser was serving something else, and nothing in the app could ever
correct that.
WHY NOT SWITCH TO autoUpdate
Because 'prompt' is the right choice and the comment defending it is right: an
automatic activation reloads the page, and doing that during a 120-minute mock
test is how a candidate loses a paper. The quiz engine autosaves, so it would
probably survive - "probably" is not good enough for a licence rehearsal. The
fix is to build the missing half, not to remove the safeguard.
WHAT IT DOES
updateServiceWorker is not called until the student clicks.
app is genuinely useful offline and that is worth saying once.
new worker on navigation - a tab left open all day would otherwise never
notice a deploy.
Neither state is a modal and neither traps focus: an interruption that blocks
the page is worse than a stale asset. role="status" with aria-live="polite" -
worth knowing, not worth cutting off a screen reader mid-sentence.
Positioned above the bottom navigation on phones, using the same 60px + safe
area inset that AppShell reserves, because covering the navigation with a
notice would strand someone on whatever page they were on. Both buttons are
44px: this can appear mid-question, and a mis-tap that reloads is precisely
what must not happen.
440 -> 449 tests.
NOTE FOR ANYONE ALREADY AFFECTED: this fix cannot reach a browser that is
already pinned to an old worker, because that browser will not load the new
code that contains it. One hard reload, or unregistering the worker in
DevTools, is needed once. After that the prompt handles every future deploy.
Co-Authored-By: Claude Opus 5 (1M context) noreply@anthropic.com
What changed and why
How it was tested
Explain the diff in your own words
Checklist
npx tsc -b,npm run lint,npx vitest runandnpm run buildall pass locallynode scripts/validate-questions.mjspasses — required if this touches question data or the loaderFrontendCI job prints the number)eslint-disable,@ts-ignore,@ts-expect-erroror skipped test; if there is one, the PR explains whyfrontend/src/data/bookChapters.generated.tsis regenerated and committed, if chapter JSONs changeddocs/still describe what the code actually doesIf this changes question data
correct_option_index1-based andcorrect_optionmatching the text at that index exactlydocs/question-format.mdfrontend/src/data/quarantine.jsonhave had their underlying rows actually fixed