fix(website): make pages cacheable in the browser and stabilize the ETag - #1130
Conversation
Every page carried `max-age=0`, so a stored copy was never reusable and each view paid a full round trip even when nothing had changed. Assets cached fine, which is why the site looked healthy. Raise the browser copy to 60s: small enough that nobody holds pre-deploy HTML for long, large enough to serve back/forward, repeat visits, and a second tab from disk. That alone would not have helped much, because the revalidation it replaces could never succeed. `copy-cmd` minted its aria-describedby id from a module-scope counter that never resets in a long-lived server, so two renders of the same page emitted different bytes, hashed to different ETags, and no `If-None-Match` could ever match. Every revalidation shipped the whole document instead of an empty 304. The description sentence is identical for every instance, so the hint element moves to the root layout and all instances reference the one id. Nothing per-instance is generated, which is what makes the output stable. The new SSR test asserts determinism generically rather than checking for this one counter, since any timestamp, random value, or incrementing id in any component would reintroduce the same silent failure.
The ETag / 304 path silently never engages when a page renders different bytes each time, and nothing errors, so the failure is invisible unless you know to look for it. Also flag that max-age=0 means the browser never reuses a stored copy, which is the part that surprised me on our own site.
My first pass moved the aria-describedby target into the root layout so every instance could share one id. That was wrong. A component's own accessible description is part of the component, and putting it in the layout means copy-cmd only works in pages that render through THAT layout: an error boundary, global-error, another app, or a scaffold template would each ship a dangling aria-describedby pointing at nothing, which is worse for a screen reader than having no description at all. The description now sits inside the click target as visually-hidden text, so it joins the accessible name after the command. That needs no id, so the render stays byte-stable (the point of #1127) and the component is self-contained again. Because the hidden text shares the click target, _copy reads the slot's assigned nodes instead of the target's textContent, so the clipboard still gets only the command. Verified in a browser: the accessible name reads "npm create webjs@latest my-app Copy command to clipboard", the hidden span is position:absolute, and scrollWidth still equals clientWidth, so it adds no scroll to the overflow-x container.
|
Design correction: the accessible description belongs inside the component, not the layout The first pass here moved The goal was only ever to stop generating a per-instance id, because the id came from a The version that landed puts the description inside the click target as visually-hidden The one consequence worth knowing about: the hidden text now shares the click target with the Two things I checked rather than assumed, since this is a visible marketing surface: A per-instance id was never available, for the record. |
Review findings on the description-inside-the-button change. The description shares an inline container with the command, so a visitor who triple-clicks the line and hits Ctrl+C was getting "npm create webjs@latest my-app Copy command to clipboard". The copy BUTTON was already safe, but the button is not the only way people copy a command. The hidden text is now select-none, and the command gets its own [data-copy-source] wrapper so _copy reads it directly rather than going through the slot, which also removes an untested null branch that would have failed silently by copying nothing. Verified in a real browser: a genuine triple-click yields exactly the command, and Ctrl+A over the document does not pick the description up. The test asserts the class rather than the computed style, because this harness does not load the Tailwind stylesheet and a programmatic Range ignores user-select entirely, so a range-based assertion would have failed while real browsers behaved correctly. The determinism test now renders each page THROUGH the root layout. It was checking page subtrees, which left app/layout.ts (the file this whole change edits) with no coverage, and would have missed the CSP nonce the layout stamps into four script tags. Also adds /docs/getting-started, a different nondeterminism class since it reads markdown at SSR. Docs: the cache page claimed a "public" cacheControl is what opts a page into conditional GET. The real gate is any value that is not no-store or private. Adds the CSP nonce to the list of determinism hazards, and syncs the same note into the skill's built-ins reference, which #1127 named and the first pass missed.
vivek7405
left a comment
There was a problem hiding this comment.
Went through this again after the description moved inside the component. The caching half holds up: the header change is one line, the ETag fix is the real content, and the determinism test is the right shape now that it renders through the layout.
The one that mattered was the hidden description sharing an inline container with the command. The copy button was safe, but a triple-click and Ctrl+C is how a lot of people actually take a command off a page, and that was pasting the hint along with it. Fixed with select-none plus a dedicated wrapper for the command, and I confirmed the real behaviour in a browser rather than trusting the unit test, which turned out to be measuring the wrong thing entirely.
Two things I left alone deliberately. The Cloudflare side is dashboard-only so it stays on #1127. And the router reading its deploy-detection headers through the same HTTP cache it is trying to detect staleness in is a genuine framework gap, not something to paper over here, so it is filed as #1131 and the tradeoff is written into the layout comment.
Docs were the weakest part of the first pass: the cache page stated the ETag gate wrong and the skill reference never got the note at all.
The inline sr-only description dragged in a [data-copy-source] wrapper (so _copy would not put the hint on the clipboard) and select-none (so a triple-click would not select it). Three moving parts to deliver one static sentence is bad DX for anyone touching this component later. A title attribute does the same job with none of them: it is the accessible description for the role=button target (the command stays the accessible name), it adds no text content, so selections and _copy see only the command with no wrapper needed, it is a static string, so the render stays byte-stable (the #1127 requirement), and it gives a native hover tooltip for free. The component's net diff against main is now: remove the counter, remove the sr-only span, add one attribute.
|
Simplification: the description is now a title attribute, in The inline sr-only description I landed after the first review round worked, but it needed a A The tradeoff to know about: Net effect on the component versus main is now three lines: remove the counter, remove the |
Review of the PR's own coverage found the two headline behaviours were unguarded at the layer that matters. Nothing read the Cache-Control header, so reverting max-age back to 0 shipped green, and the ETag / 304 round trip (the acceptance criterion on #1127) had only ever been verified manually with curl against the origin. This boots the real request handler in prod mode, asserts a positive max-age and a non-private, non-no-store value, then replays the ETag and requires an empty 304 carrying the same validator. Both counterfactuals verified red: max-age=0 fails the header assertion, and reintroducing the copy-cmd counter fails the replay on / with a 200, while /docs/getting-started stays green, so a nondeterministic component only reddens the pages that actually render it.
vivek7405
left a comment
There was a problem hiding this comment.
Went back over the diff asking one question: if someone reverted each fix line by line, which test goes red? The component half was well covered (the determinism suite plus the browser contract tests), but both headline behaviours failed the check. Nothing read the Cache-Control header, so flipping max-age back to 0 shipped green, and the ETag round trip, the acceptance criterion on the issue, had only been proven with curl against the origin, never in the suite.
Closed with an end-to-end test through the real request handler in prod mode: positive max-age, no no-store/private, then an ETag replay that must come back as an empty 304. Both counterfactuals verified red, and the failure modes are cleanly separated: a header revert fails the header assertion, a reintroduced counter fails the replay on / while /docs/getting-started stays green, since that page renders no copy-cmd. The rest of the diff reads right to me now, and the component change is down to the three lines it should be.
Closes #1127
No page on webjs.dev was ever served from a browser cache. Assets were (
/components/copy-cmd.tsshows200 OK (from disk cache)on a repeat visit), which is what made this look like a framework problem rather than a configuration one. It is neither: the site opted into caching correctly, but two separate things cancelled it out.What was wrong
The browser was told never to reuse a page. Every page carried
max-age=0, so a stored copy always required a round trip.s-maxage=600only ever applies to a shared cache, so the browser layer was doing exactly what it was asked to do.The revalidation that replaced it could never succeed.
copy-cmdminted itsaria-describedbyid from a module-scope counter. A counter never resets in a long-lived server, so two renders of the same page emitted different bytes, hashed to different ETags, and noIf-None-Matchcould ever match. Every revalidation shipped the whole document (about 72KB for the home page) instead of an empty 304. Verified against the origin, where two consecutive fetches of/differed only incopy-cmd-hint-116versuscopy-cmd-hint-120.What this changes
max-agegoes to 60 seconds. That is the smallest value that produces real browser cache hits on back/forward, repeat visits, and a second tab, while bounding how long a reader can hold pre-deploy HTML to one minute. I lefts-maxageandstale-while-revalidatealone deliberately, see below.copy-cmdstops generating an id at all. The "Copy command to clipboard" description becomes atitleattribute on the click target: the command stays the accessible name, the title supplies the accessible description (and a native hover tooltip), and nothing per-instance appears in the output, which is what makes the render byte-stable. The component's net diff is three lines: remove the counter, remove the sr-only span, add one attribute.This took three attempts, recorded honestly: first a layout-owned shared element (wrong, it couples the component to one layout and dangles anywhere else), then inline visually-hidden text (worked, but needed a
data-copy-sourcewrapper plusselect-noneto keep the hint off the clipboard and out of selections, three moving parts for one static sentence). Thetitleattribute needs none of that because it adds no text content. A per-instance id was never salvageable:querySelectorthrows during SSR, so the id cannot be derived from the slotted command text, which leaves a counter, and the counter is the bug.The new SSR test asserts determinism generically rather than checking for this one counter, because the failure mode is a class. Any timestamp, random value, or incrementing id in any component reintroduces it, and nothing else in the suite would notice: the page still renders, every content assertion still passes, and the only symptom is a caching layer that quietly never engages.
What this does NOT fix
Cloudflare returns
cf-cache-status: DYNAMICon every HTML URL and strips the origin's ETag (present onwebjs.up.railway.app, absent onwebjs.dev). Cloudflare does not cache HTML without an explicit Cache Rule, and something on the HTML-modifying side is removing the validator (Email Address Obfuscation is on by default and rewrites the body). Both are dashboard-only, so they are recorded on #1127 rather than fixed here. Browser caching works after this merges regardless, since Cloudflare passesCache-Controlthrough untouched.I did not raise
s-maxage. #1095 is the un-fingerprinted/public/tailwind.csslink going stale at the edge across a deploy, and caching HTML harder at the edge compounds it: a cached document plus a cached-stale stylesheet is exactly the broken layout that issue describes. That half is worth doing after #1095 lands.Test plan
npx webjs testinwebsite/: 109 pass, 0 failnpm run test:browserinwebsite/: 24 pass, 0 fail across 3 filesrender-determinism.test.tsrenders four routes twice through the root layout and asserts byte-identical output, reporting the first divergent line. Red when the counter is reintroduced.conditional-get.test.tsis the test of record: it boots the real request handler in prod mode, asserts a positivemax-ageand a non-no-store/privatevalue, then replays the ETag and requires an empty 304. Reverting the header tomax-age=0fails the header assertion; reintroducing the counter fails the replay on/with a 200 while/docs/getting-startedstays green, so a nondeterministic component only reddens pages that render it. Without this test, a one-line revert of the header fix shipped green._copycannot pick up anything else), and two renders of the same command emit identical markup.Docs
website/app/docs/cache/page.tsgains a note that render determinism is a precondition for the ETag path, and thatmax-age=0means a stored copy is never directly reused. Neither was written down anywhere, which is part of why this sat unnoticed.Follow-up filed
#1128. While adding the layout comment I wrote the component's tag in angle brackets inside
<!-- ... -->and SSR instantiated it as a real element, dropping the rest of the comment including its closing-->. That is a genuine framework bug ininjectDSD, which scans for registered tags with a flat regex that has no comment awareness. Filed with a minimal repro rather than fixed here.Note for review
website/app/layout.tsis also being edited onfeat/ui-gallery-on-webjs-devright now. My hunk there is now a single line (thecacheControlvalue), but expect a rebase.