ReviewOS

stacks/wildloop

Detail pages serve no server-rendered heading (metadata fixed for trails)

#1012
Closed glennmichael123 opened this 9 days ago · {{ comments.length }} {{ comments.length === 1 ? 'comment' : 'comments' }}
9 days ago

Problem

A trail page serves no heading and no metadata of its own. Everything that identifies which trail it is arrives only after hydration, so a crawler, a shared link preview, and a screen reader before the runtime boots all get a page about nothing in particular.

Fetched from the QA app at /trail/9:

served
<title>Trail Details - Wildloop
og:titleWildloop — Trail discovery and GPS tracking for runners and hikers
meta[name=description]the site-wide default
<h1>{{ trail.name }}, under display: none

Every trail page serves those same four values, so all of them are indistinguishable from one another and from the site root.

/trails has the heading half of the same problem — resources/views/trails.stx:1120 is <h1 ...>{{ listHeading() }}</h1>, served as that literal string — though its <title> and description are specific and correct.

The home page is fine, and shows what the difference is: resources/views/index.stx:462 is <h1 class="hero-title">Find your next adventure</h1> — literal text, rendered server-side.

Why it matters

Half a million trail pages are the catalogue. They are the reason to index the site at all, and right now each one tells a crawler the same generic thing. A shared link to a specific trail previews as the site's front page.

The heading has a second cost. <h1> is how a screen reader user establishes what a page is about, and until the runtime boots there is none — on a page whose entire purpose is one named trail. The markup is worse than absent: it contains the literal characters {{ trail.name }}.

This is also the half the .pw.ts specs cannot see. They drive a real browser, so the runtime has already filled all of it in by the time they assert. The gap is only visible in what the server sent, which is what tests/browser/served-markup.test.ts now reads.

Why the two pages fail differently

They need different fixes.

/trail/{id} — resources/views/trail/[id].stx:840. The <h1> sits inside <div :if="trail">, and trail is falsy server-side, so the whole header block is hidden rather than rendered. The metadata is generic for the same underlying reason: nothing resolves the trail before the response is written.

/trails — resources/views/trails.stx:1120. Not gated by anything; the heading is simply a signal expression, and {{ listHeading() }} is not evaluated server-side. Its title and description are already correct, so only the heading needs doing.

Proposed scope

  • Resolve the trail server-side for /trail/{id} and render its name into the <h1>, the <title>, og:title, og:description and meta[name=description]. The id is in the path, so this is a question of where the data is bound rather than of fetching something unavailable.
  • Give /trails a server-rendered heading. Its three list modes already drive listHeading(); the default — the catalogue — is the one a crawler sees and is static.
  • A trail that does not exist should render its not-found state server-side too, rather than an empty header, so a 404 is a 404 to a crawler.
  • Worth checking the other :if-gated headers at the same time: feed.stx, clubs.stx, records.stx, challenges.stx and settings.stx all contain an <h1>, and the ones behind a client condition will have this shape.

Leave the interactive parts client-rendered. Nothing here asks for the filters, the map or the review sheet to be server-rendered — only the identity of the page.

Acceptance

  • /trail/{id} serves an <h1> containing the trail's name, with no {{ anywhere in the rendered text.
  • Its <title>, og:title and description name the trail, and two different trails serve different values for each.
  • /trails serves a rendered <h1>.
  • A nonexistent trail id serves its not-found state in the markup.
  • The two cases currently skipped in tests/browser/served-markup.test.ts are unskipped, and the metadata assertion is strengthened: asserting the trail title merely differs from the home page's passes against Trail Details - Wildloop, which is how this went unnoticed.

Found by tests/browser/served-markup.test.ts, on its first run.

9 days ago

Correction: the fix proposed above is wrong, and would make this worse

/trails is done in 9c02bc1. /trail/{id} is not, and the reason I gave for how to fix it — "the id is in the path, so this is a question of where the data is bound" — is incorrect. The data binding is not the blocker.

config/server.ts sets cache.renderVary: 'source', which keys the render cache by file path alone. One render of trail/[id].stx is served for every trail. So rendering the trail's name into the <h1>, the <title> or the og: tags would mean whichever trail rendered first supplying them for all ~600k pages — strictly worse than a generic title, because a wrong specific answer is worse than an honestly generic one.

resources/composables/usePageSeo.ts already documents this, including the part I missed: social scrapers do not run JavaScript, so og: tags stay whatever the shell carries, and that is the half the client-side fix cannot reach. Googlebot does run JavaScript and picks up the client-applied title, so the indexing half is already handled.

What it actually needs

A render cache keyed by URL rather than by source file — one cached render per trail, which is still a real cache, unlike renderVary: 'request' (that keys on the whole request context, cookies and IP included, so it caches per visitor and stops being a cache).

That option does not exist today, and adding it spans three packages:

  1. bun-plugin-stx — serve.d.ts:336 declares renderCacheVary?: 'request' | 'source'. A URL-keyed mode has to start here.
  2. stacks — storage/framework/core/types/src/server.ts:162 declares the same two values, and core/buddy/src/production-server.ts:407 passes it through.
  3. wildloop — set it in config/server.ts, then move the trail's metadata into a <script server> block and render the <h1>.

Worth checking before building it: whether one render per trail is acceptable for a 600k-row catalogue, or whether the cache needs a bound. That is the question that decides the design, and it is not answerable from this repo.

What changed here

  • /trails serves a rendered heading. It is the neutral one — cache.renderVary means one render answers every query string, so ?country=DE cannot have its own — with x-text specialising it on hydration. Verified with Chromium: the three trail-filters.pw.ts cases that assert the heading changing still pass.
  • tests/browser/served-markup.test.ts now asserts both static routes serve exactly one <h1> containing no {{, so the fix cannot regress.
  • The remaining skipped case names the render-cache constraint rather than implying the view is at fault.

Acceptance, revised

The original list stands for /trail/{id}, with one addition: the render cache has to be keyed per URL first, or the rest of it cannot be done correctly at all.

9 days ago

Second correction: no framework change is needed, and the metadata half is done

ef943b53. I was wrong twice above, and the second time I nearly built the wrong thing — a URL-keyed render cache across three packages. It is not needed.

packages/bun-plugin/src/serve.ts already skips the shell cache for a dynamic route whose server script reads its params:

const cacheShell = ENABLE_HTML_CACHE
  && RENDER_CACHE_VARY === 'source'
  && paramNames.length > 0
  ...
  && !serverScriptsReadParams(dynServerScripts, paramNames)

Not when a server script reads its params: that page genuinely renders per record, and one record's output must never be served for another.

serverScriptsReadParams matches a <script server> block referencing params or a route param name, and stx covers it in render-cache.test.ts:209 — "renders a page whose server script reads its params per record".

So usePageSeo.ts's premise — that a server block "does not work for a detail page" — was never true of serve.ts as it stands. Each time I repeated it above I was taking a comment at face value instead of reading the code it describes.

What landed

/trail/{id} reads its trail with Trail.find() at render time and serves its own <title>, description, canonical, og: set and Twitter card. The description leads with location, distance and difficulty. A trail that does not exist is noindex, so a crawler trying bad ids does not fill the index with generic pages competing against real ones.

The cost, stated plainly: this route no longer serves a cached shell, so every trail page renders. That is the trade this buys the og: half with — the half no client-side helper can reach, because social scrapers run no JavaScript.

Verified, since this is the first detail page here to render per record: two trails serve different titles, a missing trail serves the generic one with noindex, the four Playwright specs that visit a trail page still pass, and three new assertions in served-markup.test.ts fail if the server block is removed.

What is still open, and why

The <h1>. It sits inside <div :if="trail">, gated on the client signal, and the first 120 lines of that block hold ten more interpolations. Opening the gate server-side would render all of them as literal — worse than the heading being absent, which is the same trap as writing one trail's name for all of them.

Fixing it means the template's trail resolving against a server-supplied value as well as the client store, which is a rework of the page's data flow rather than a change to a condition. Its case stays skipped in served-markup.test.ts and now says this.

The other six detail pages — club, activity, effort, territory, athlete, event — have the same generic-metadata problem and none has a server block. The pattern in trail/[id].stx transfers to all of them.

9 days ago

Fixed. The same four values this issue tabulated, fetched from the QA app at /trail/9:

beforenow
<title>Trail Details - WildloopEifel Forest Way — Monschau, Nordrhein-Westfalen | Wildloop
og:titlesite defaultEifel Forest Way — Monschau, Nordrhein-Westfalen | Wildloop
meta[name=description]site-wide defaultEifel Forest Way in Monschau, Nordrhein-Westfalen. Moderate · 11.4 mi · 400 ft elevation gain. Route map, records and recent conditions on Wildloop.
<h1>, display: noneEifel Forest Way, in the accessibility tree

The premise recorded next to the skipped test was out of date, and worth correcting for anyone who reads it: a per-record heading was thought impossible because cache.renderVary: 'source' keys the render by file path, so one render of trail/[id].stx would answer for all ~600k trails. A server script that reads its params opts the route out of that cache, which is what made this tractable.

Two defects were in the way, and only the first was known. The heading held a literal , because trail is a client signal with nothing to resolve against server-side. And it was absent from the accessibility tree whatever it said: :if is evaluated on the client, so stx serves that branch with x-cloak — display: none !important — and the whole trail header sits inside one. Writing the name into that <h1> still left getByRole('heading', { level: 1 }) finding nothing, which is what a crawler and a screen reader find too. The heading now sits outside that container as sr-only, with the painted title demoted to a paragraph.

Scope went beyond this issue: the same two defects were on all six other detail pages — club, activity, athlete, event, territory, effort — each serving a cloaked literal, and effort serving no heading at all on a loaded attempt. All seven now serve one exposed heading naming their record, and each page's server-rendered title and description are the same strings the page sets for itself, so a shared link previews as what it opens as.

Against the acceptance list: 1, 2, 3 and 5 are met. 4 is not — a nonexistent trail still serves HTTP 200 with both branches cloaked, so a crawler sees no not-found state. That was a deliberate choice rather than an oversight: the server cannot distinguish "no such trail" from "the lookup failed", and the page is noindex either way. It is filed separately rather than left implied here.

The five :if-gated headers this issue suggested checking are all fine — /feed, /clubs, /records, /challenges and /settings each serve one visible rendered <h1>.

Coverage, since the suite was green throughout while six of the seven pages were broken: ROUTES in served-markup.test.ts was / and /trails, so the detail pages had none. Both previously skipped cases are unskipped, the trail metadata assertion now requires the name rather than merely differing from the home page, and new cases assert every detail route serves exactly one heading that names its record. A Playwright test compares each page's served metadata against what it sets after hydration, so the two halves cannot drift apart again unnoticed — it caught the client advertising --/mi on its first run.

Landed in 2b095ab, e79406d, 65e542f, 89f9f35, 624e073, c59694f, 939c5af, e98e3b0, 90550f2.

Sign in to comment on this issue.