ReviewOS

stacks/stx

Router prefetch caches an empty container-attrs string, stripping class from the routed container on a prefetched navigation

#1947
Closed chris opened this 10 days ago ·
10 days ago

Summary

The router's navigate path reads X-STX-Container-Attrs and caches it. Both prefetch paths do not: they never read the header, and they call setCache with five arguments where it takes six. A link that was prefetched before it was clicked therefore serves a cache entry whose container-attrs string is undefined, and setContainerAttrs then strips the attributes the previous navigation had applied to the routed container.

Confirmed on @stacksjs/stx@0.2.231. It was present in 0.2.198 too and survived the router rewrite between the two — line numbers below are from 0.2.231.

The code

dist/chunk-bqdbf8k2.js

Signature takes six (line 104):

function setCache(key,html,layout,group,title,cattrs){

The navigate path reads the header (line 547) and passes all six:

var newCAttrs=r.headers.get('X-STX-Container-Attrs')||'';

Both prefetch call sites pass five (lines 1512 and 1628):

setCache(key,result.html,result.layout,result.layoutGroup,result.title)

The response handler feeding the second one reads only four headers — X-STX-Layout, X-STX-Layout-Group, X-STX-Title, X-STX-Runtime — and never X-STX-Container-Attrs. So cattrs arrives undefined and the entry is cached with no attributes.

Symptom

Our dashboard routes send class="app-shell" in that header; it carries the max-width and centring for the routed <main>.

  1. Hover a dashboard link — prefetch stores the entry with empty attrs.
  2. Click it — the cache hit applies ''.
  3. setContainerAttrs walks data-stx-cattrs from the previous navigation, finds class absent from the incoming (empty) map, and removes it.
  4. <main class="app-shell"> becomes <main>, and the page renders edge to edge with a horizontal scrollbar.

It presents as intermittent, because it only happens for links that happened to be hovered first. A hard reload always "fixes" it, because a full load rebuilds the document from server HTML — which sends the class in the markup rather than the header. Both of those made it read like a CSS problem rather than a cache problem.

Suggested fix

Have the prefetch response handler read X-STX-Container-Attrs alongside the other four and pass it through as the sixth argument, matching the navigate path:

var pCAttrs = r.headers.get('X-STX-Container-Attrs') || ''
// ...
setCache(key, result.html, result.layout, result.layoutGroup, result.title, result.cattrs)

Since the parameter is optional in JS, the arity mismatch is silent — a caller that forgets it gets undefined rather than an error, which is probably why it survived a rewrite. It may be worth having setCache require the argument explicitly.

Workaround

router.prefetch: false, which is what we ship. Prefetch is a latency optimisation and correct layout is not optional, so turning it off is a fine trade — but it would be nice to have it back.

10 days ago

Fixed in eb0bd6e.

Your diagnosis was exactly right, including why it survived a rewrite: setCache takes six parameters, the navigate path passed all six, and both prefetch paths passed five. JavaScript accepts that silently, so the entry was stored with cattrs undefined and setContainerAttrs then stripped the class the previous navigation had applied.

Rather than add the missing read at each of the two call sites, both now go through one shared reader that pulls all the headers — including X-STX-Container-Attrs — from the response. Hand-assembling the same object in three places is what let them drift apart, so there is now one place to drift.

Covered by five tests in packages/router/test/prefetch-container-attrs.test.ts: a navigation served from the prefetch cache, a hover-prefetch storing the attributes, and the case from your report where a prefetched entry must not strip what the previous navigation applied.

You should be able to drop the router.prefetch: false workaround.

Sign in to comment on this issue.