ReviewOS

stacks/bench-review

stx-standards §9.10: 15 route params re-parsed from location.pathname instead of useRoute().params

#87
Closed glennmichael123 opened this 9 days ago ·
9 days ago

Conformance gap against stx-standards §9.10, found while fixing the courthouse tab links.

The rule

9.10 MUST — read route params from params, never re-parse the URL. In a <script server> block on a dynamic route, use the injected params object or the bare param name. In a client script, use useRoute().params. Never reconstruct a param from requestContext.url(), location.pathname or a query-string fallback.

What bench does instead

Every dynamic-route component resolves its id by hand, in the same shape:

function resolveCourtId(): number {
  if (typeof window === 'undefined') return 0
  const fromRouter = Number((window as any).stx?._rp?.id)
  if (Number.isFinite(fromRouter) && fromRouter > 0) return fromRouter
  const m = window.location.pathname.match(/^\/court-houses\/(\d+)/)
  return m ? Number(m[1]) : 0
}

Measured:

.stx files reading location.pathname18
call sites re-parsing a route param (.match)15
files using the _rp + pathname fallback14
files using useRoute()0

Affected components include Bench/Judge/{JudgeTabs,Profile,ProfileHeader,Reviews,RecentRulings}, Bench/Court/{CourtTabs,CourtHeader,CourtProfile}, Bench/ArticleView, Bench/Comments, Bench/User/ReviewerProfile, Bench/Admin/AdminShell.

Why it has persisted

Two reasons, and both look like good reasons until you check them.

1. A project memory told us to. The note says window.stx._rp is unset on a hard reload, so dynamic-route components must parse location.pathname instead or the fetch never fires. That observation was real. The conclusion was wrong: §9.10 documents useRoute().params as the reactive wrapper over the same _rp object (signals.js:990-1002), so it resolves when _rp arrives — which is precisely the hard-reload case the workaround was invented for. Verified present in bench's installed stx 0.2.176.

2. The strict guard is on, but this category is allowlisted. config/ui.ts sets strict: { enabled: true, failOnViolation: false } — ahead of where the standards found bughq — but allowPatterns contains 'window.location', and allowPatterns is a rule-global substring match. The entry is justified in its own comment by a different problem (bare location gets shadowed by store signals, so bench qualifies with window.), which is a genuine footgun. But one entry suppresses every window.location finding project-wide, including the route-param reads that have a proper replacement. The last build emitted 0 strict warnings against 28 occurrences.

What to do

  • Replace the hand-rolled resolvers with useRoute().params (auto-imported; never hand-import it — rule 9)
  • Confirm reactivity on a hard reload, not just SPA nav — that is the case the workaround existed for, and the one that must not regress
  • Narrow the window.location allowPattern so genuine non-route uses stay allowed and route-param reads start warning again
  • Correct the project memory so the anti-pattern stops being re-applied

Risk

The hard-reload path is the whole point. Each component must be verified with a real reload, not just in-app navigation — the judge tab shells in particular resolve the id from the pathname by design today, because one shell serves all 2,740 judges (see #83).

9 days ago

Correcting this issue: it cannot be a blanket migration, and filing it as one was wrong. Two things turned up on starting the work.

1. Shell-served routes must keep parsing the path

bench serves the four judge tab routes from a single shared shell — one built file for all 2,740 judges (judgeShellPath()). The build injects route params per file, so the shell gets the literal id it was built with:

built pageinjected _rp
dist/judges/1803.html (per-id){"id":"1803"}
dist/judges/_shell/profile.html (shared shell){"id":"_shell"}

So useRoute().params.id returns the string "_shell" on every judge tab route. Reading the path is load-bearing by design there, not sloppiness — it is the mechanism that lets one file serve 2,740 judges. Migrating those components would break them.

That rules out six components outright: Bench/Judge/{ProfileHeader,JudgeTabs,Profile,Reviews,RecentRulings,Cases}. ProfileHeader is doubly excluded — it renders on both the shells and the canonical /judges/:id.

And the two cleanest remaining candidates are unverifiable today: ArticleView (/article/:id) and ReviewerProfile (/user/:id) both render on routes with 0 built pages, because no reviews are published yet. Migrating them would be a change nobody could observe.

2. The hard-reload hazard does not apply to static builds

The memory note behind this pattern recorded a real incident — _rp undefined during hydration, so the fetch never fired. On the static build that cannot happen. The id is injected in <head>, at byte 520 of dist/court-houses/1/profile.html:

<script>if(window.stx)window.stx._rp={"id":"1"};else window.__stx_rp={"id":"1"};</script>

before the signals runtime loads. The runtime's init then does _routeParams.set(window.stx?._rp || window.__stx_rp) (signals.js:1274-1276), so params are populated before any component scope script runs. The original incident was on the dev server, where the injection order differs — not a property of the API.

What actually landed

The three courthouse components — CourtHeader, CourtProfile, CourtTabs — which render on 808 per-id pages carrying real ids. They now read useRoute().params.id first and keep a narrow, documented path fallback for the shell case, so #83 will not have to reopen them.

useRoute() went from 0 files to 3.

Revised remaining scope

  • ArticleView, ReviewerProfile — migrate when /article/:id and /user/:id have built pages to verify against
  • Judge components — do not migrate while the shared shell exists; revisit only if the shell design changes
  • Narrow the window.location allowPattern in config/ui.ts once the above settles, so genuine re-parsing warns while the shell exception stays allowed

Sign in to comment on this issue.