ReviewOS

stacks/bench-review

stx-standards: full 12-rule conformance triage

#88
Closed glennmichael123 opened this 10 days ago ·
10 days ago

Conformance triage of bench against stx-standards — the 12 rules, measured rather than estimated. Companion to #87, which carries the one finding big enough to need its own issue.

Scorecard

#Rulebench
1No <!DOCTYPE>/<html>/<head>/<body> in pages or components1exemption, see below
2No vanilla DOM code18 files / 15 call sites#87
3Crosswind only, no vanilla CSS8 files with <style>see below
4StxLink for internal navigation1 (was 2)effectively clean — 177 StxLink
5useHead/useSeoMeta, no hand-written <title>0clean
6@stacksjs/components before hand-rollingnot measuredneeds a per-primitive audit
7Cross-page state in a storenot measuredbench already routes data through resources/stores/
8Directives, not JS that builds markup5RichTextEditor, see below
9Never hand-import an auto-imported symbol0clean
10Annotate every parameter and returnnot measurednothing verifies it — see note
11≤2 script blocks; client/lang="ts" redundant66 + 4 + 1cosmetic
12No unencoded user data in script blocksnot measuredneeds review alongside the XSS work

For scale, the standards' reference audit of bughq found 263 plain <a> against 27 StxLink. bench is 1 against 177, and already runs the strict guard bughq had never enabled. The gaps below are real but this is not a codebase that ignored the rules.

Rule 1 — one file, and it is deliberate

resources/views/500.stx is a full standalone document. That is correct: a 500 page must render when the app shell is the thing that broke, so it cannot depend on layouts or the router. It also accounts for one of the two rule-4 hits (<a class="btn" href="/">), for the same reason.

resources/components/Bench/ArticleView.stx:531 matched the grep but is <head> inside a prose comment — a false positive, not a violation.

Rule 4 — fixed in passing

The one real violation was resources/views/court-houses/[id].stx:77, a plain <a href="/court-houses"> written earlier in this same session. Now <StxLink to="…">. The remaining hit is 500.stx, which is the exemption above.

Rule 3 — the 8 <style> blocks are a documented deviation, not drift

A project memory records why: @push-ed styles do not survive SPA navigation (the router swaps only <main>, and @push hoists to <head>), so component CSS is deliberately placed in a plain <style> inside the component body. Worth re-testing against current stx before treating it as permanent, but it is a reasoned position rather than neglect.

Rule 8 — 5 hits, all one component

createElement is confined to the rich-text editor, which wraps ts-medium-editor. config/ui.ts already allowlists it with that justification: DOM building is inherent to the third-party editor and cannot be expressed as directives. Accurate, and not worth chasing.

Rule 11 — 66 files, cosmetic

<script client> where client is redundant (66), more than two script blocks (4), lang="ts" (1). None of this is broken; it is noise that makes the rule harder to enforce later. Low priority, mechanical, and a good candidate for a codemod rather than hand edits.

Not measured, and why

Rules 6, 7, 10 and 12 need judgement per call site rather than a grep, so counting them would have produced a number with no meaning. Rule 10 is worth flagging specifically: the standards point out that nothing verifies annotations — tsc --noEmit cannot see inside .stx files, so a wrong type in a script block is silent. Any claim of rule-10 conformance here would be unfalsifiable with current tooling.

Suggested order

  1. #87 — rule 2, the only finding with a correctness consequence
  2. Rule 11 cleanup, mechanically
  3. Rules 6/7/10/12 audits, when there is a reason to open those files anyway
9 days ago

Replacing the original scorecard: I have now read all thirteen chapters rather than measuring against the rule list. The first triage was written having read only the README, 09-navigation.md and part of 12-enforcement.md, and it missed things that reading turns up immediately.

Corrected scorecard

#Rulebench
1No document tags in pages/components1500.stx, deliberate
2No vanilla DOM18 files / 15 call sites#87
3Crosswind only8 <style>documented SPA-nav deviation
4StxLink for internal nav1 vs 171clean
5useHead/useSeoMeta, no hand-written <title>0clean — but see #89
6@stacksjs/components before hand-rollingButton/Badge/Switch/Dialog/Avatar/Breadcrumb/Tabs: 0 each§10.4–10.8, all hand-rolled
7Cross-page state in a store16 storeslooks healthy
8Directives, not JS building markup5editor only, allowlisted
9Never hand-import an auto-import0clean
10Annotate everythingunverifiabletsc cannot see inside .stx
11≤2 script blocks; client/lang="ts" redundant66 + 4 + 1cosmetic
12No unencoded user data in script blocksnot auditedbelongs with the XSS work

Plus rules the chapters add beyond the twelve-line summary:

Rulebench
2.8 / 3.5exactly one <main>, owned by the layout2,942 pages had two — fixed in 918af33
7.1every :attr needs a static counterpart0 violations — clean
7.3toggle with :show, never a "hidden" class116 in 27 files
6.5useCookie(), never document.cookiedocument.cookie 5, useCookie 0
6.6useQuery/useMutation, never raw fetch0 used
6.7every listener cancelled by onDestroy10 listeners, 2 onDestroy

Fixed while reading

§2.8/§3.5 — two <main> landmarks on 2,942 pages (918af33). The layout opens one; judges/[id].stx and court-houses/[id].stx each opened another. Invalid HTML, two landmarks announced to a screen reader, and config/ui.ts sets router.container: 'main' — so the SPA router had two candidates. 365 of 400 sampled pages → 0 of 500.

Worth noting the courthouse one was written in this repo yesterday, by mirroring the judge page. The same propagation that produced #87.

§7.3 and the :if memory note

The project memory says to show/hide with x-class="flag() ? '' : 'hidden'" because :if is dropped when false at first render. §7.3 says use :show.

Those are different binders — bindShow writes el.style.display and keeps the node mounted; bindIf mounts and unmounts. The recorded bug was bindIf. bench already uses :show successfully 17 times, so the 116 hidden-class toggles look like an avoidable workaround rather than a necessary one.

§7.3's pairing — :show plus a static style="display:none" — is explicitly the answer for elements that start hidden, which is the exact case the memory was written about. Worth testing one migration before committing to 116.

What reading changed, and the general lesson

#89 is the clearest case: §4.0.1 marks useSeoMeta in <script server> as "MUST — the default", which reads as an indictment of bench's build-time SEO injection. Testing bench's actual view shape showed @extends discards the server-scope head entirely, so the injection layer is a justified workaround and the API table applies only to layout-less pages.

So: the chapters are verified against bughq, which uses autoShell. Every rule that touches the head, the shell, or the layout needs re-testing against an @extends page before adoption here. Measuring conformance is not the same as reading the rule, and neither is the same as testing it.

Suggested order

  1. #87 — rule 2, the only one with a correctness consequence
  2. §7.3 — test :show on one site, then migrate the 116 if it holds
  3. §6.7 — 8 uncancelled listeners is a real leak, small to fix
  4. §6.5 / §6.6 — mechanical but touches auth and every data path
  5. §10 — a large UI migration against the Gavel Blue design system; real, not urgent
  6. Rule 11 — cosmetic, codemod territory
9 days ago

§7.3 done — and it was not a style rule

Tested one site first, as planned. The test passed, so I carried it through the whole app. What the test actually revealed changes the framing in the triage above.

§7.3 is not about which binder is tidier. x-class is evaluated in the browser only, so a pre-rendered page ships the element with no hidden in its static class list and paints it until the runtime boots. bench is a 2,990-page static build, so every one of these was a visible defect on every load:

pagepainted before JS
every pageheader search dropdown hung open
/verify-emailall four mutually exclusive outcomes at once — "Verifying…", "Email verified ✓", "Verification failed", "Check your inbox"
/reviewsthe full-screen "Report this review" dialog + scrim, over the page
/admin/usersthe full-screen "Edit user" modal + scrim
/login, /admin/login"Your session expired. Please sign in again."
/judges/signupall three auth states stacked in one column
/searchall three empty states at once
/homethe trending + active-reviewer bands the guards exist to hide
/judges/:id profileempty education list and "Background not yet available"
/settings"Claim submitted — a moderator will review it"
/notificationsempty state with unresolved
admin tablesevery empty state over its own list

~40 elements across 15 page shapes. Several of these sat under comments stating the opposite intent — "Hidden until the fetch lands, so nothing flashes", "empty trending bands read worse than a missing section".

Not 116. The triage counted x-class toggles in source. Most are nested under a parent that :show now cloaks, and a cloaked parent does not paint its children, so converting the outer element fixed three or four at a time. Entity pages were clean. Four toggles are deliberately left alone because their condition is true on arrival — converting those would trade a flash-out for a flash-in and make real content JS-dependent.

The method mattered more than the rule. I first measured with a browser probe: load a JS-free copy of each built page, list [x-class] elements with offsetParent !== null. It called /reviews clean. offsetParent is null for position: fixed — so the probe was blind to exactly the full-screen overlays that matter most. gate.ts check 19 walks the tag stack in the built HTML instead; it caught the /reviews dialog, the /admin/users modal, and the whole admin surface the browser sweep never visited. Falsified before trusting it.

Commits: 5bf5477, d3fa3eb, 73f296c, 44abaff, 93417471, c22bc6d, fb75565, d990daf. Gate 19/19.

Remaining from the triage: §6.7 listener cleanup (10 addEventListener vs 2 onDestroy), §6.5 useCookie(), §6.6 useQuery/useMutation, §10 components.

9 days ago

§6.7 done — and the rule's premise does not hold here

Measured before fixing, which changed the answer.

bench's built artifact ships no SPA page router. Four independent confirmations:

  1. dist/_stx/runtime.*.js registers no document-level click handler and contains no anchor interception.
  2. It handles stx:load but nothing ever dispatches one — that is the router's job.
  3. A measured StxLink click on the built site discards window state and reports performance navigation type 'navigate' — a real document load.
  4. The router is a separate asset. buildRouterAsset() lives in @stacksjs/stx/dist/build-assets.js and is injected by production-builder.js as /__stx/router.<hash>.js. bench's build.ts calls buildApp — the SSG path — which contains zero router/spa references. Note /__stx/ (SSR) vs our /_stx/ (SSG): different pipelines.

This is how stx's SSG works, not a misconfiguration. StxLink degrades to a plain anchor, and the _shell pages still work because they read their route param client-side on a hard load.

So §6.7's leak class — component destroyed while its timers keep firing — cannot occur across navigation in production. Each document binds its listeners once and takes them away again. Fixed anyway, because it is correct in both environments and the dev server does navigate client-side:

  • ConfirmHost — document keydown (Escape-dismiss + Tab trap), was unguarded
  • AdminShell — window stx:navigate + popstate, were unguarded
  • VerifyEmail / reset-password — the two redirect timers, which were neither tracked nor cancellable despite config/ui.ts's allowPatterns entry describing bench's setTimeout use as "all tracked and cancellable (audited: no leaks)"

The remaining six addEventListener calls bind to elements inside their own component and die with the node. BenchHeader was already guarded by a window flag — and its comment ("BenchHeader's IIFE re-runs on every SPA nav") directly contradicted AdminShell's ("the top-level script body only runs once"). Both were right about different environments and neither said which. They do now.

Also corrected a comment from this morning's <main> fix. It claimed config/ui.ts sets router.container: 'main'. There is no router key in that file — I read it off the standards chapter and did not check. The a11y reason stands, and the real stx reason is better: the runtime's scope-root fallback is document.querySelector('main'), which returns the first one, so two landmarks means a script block can bind its scope to the wrong element.

Open product question, not a bug: whether bench wants SPA navigation in production. Today it does not have it. Enabling it means the ssr build path, not a flag on the SSG one. Worth a decision before #85 (deploy), since it changes what the host has to serve.

Commits: 7282dbb, 9640aec. Gate 19/19.

9 days ago

§6.5 and §6.6 — one migrated, two documented deviations

§6.6 — fetch is now 0 in templates

grep -E 'fetch\(' resources --include='*.stx' was 4, now 0. All four were form POSTs sitting in views, which broke bench's own standing rule (components do not fetch) as well as §6.6:

wasnow
/api/contact in contact.stxnew contact store, mirroring subscribe
/api/judges/submit in judges/submit.stxjudges.submitJudge()
/api/auth/password/forgot in forgot-password.stxauth.requestPasswordReset()
/api/auth/password/reset in reset-password.stxauth.resetPassword()

The password one was worth moving for a security reason, not a tidiness one. The forgot-password endpoint answers 404 for an unknown address, and something has to decide that a 4xx still means "if an account exists, we sent you a link". That decision was in the view. A second caller handling the status itself would have reintroduced the enumeration oracle. In the store it cannot — the store never reports the difference. Verified with the API down: an unknown address takes a real 404 and still renders the neutral confirmation, no error, no toast.

§6.6's other half is a deviation, and stays one

The rule also wants useQuery/useMutation. bench does not adopt them, for three reasons checked against @stacksjs/stx@0.2.176 rather than assumed:

  1. useQuery is component-scoped by construction — it starts with onMount(fetchData) (signals.js:1537) and cleans up via onDestroy (:1549, :1565, :1569). A store is built at defineStore time, outside any mount.
  2. Its documented shape puts fetch back in the component. §6.6's own example declares useMutation('/login', …) inside the view — which would undo exactly what the migration above just finished.
  3. Its headline benefit does not apply here. _queryCache earns its keep by surviving fragment swaps; bench's built site has no client router, so it would be discarded on every navigation. Within a page the stores already deduplicate.

Written up in resources/stores/README.md, which was previously three lines and the word "wip".

§6.5 — useCookie() — also a deviation

bench has 3 document.cookie statements, all in the auth store. The serialisation is not the problem: useCookie (signals.js:5344) builds the identical string. The problem is when it writes — inside an effect (:5370), and effect runs immediately on creation with no option to suppress it (:781). A store-scope declaration would therefore rewrite the cookie on every page load and refresh max-age: a sliding expiry.

auth-user is the UI mirror of the HttpOnly auth-token, and the two are deliberately given the same fixed 30-day life from the same login. Slide one and it outlives the other — isAuthenticated() stays true after the session is dead, so instead of a clean logged-out render the user gets a page that thinks it is signed in until the first 401 bounces them. Recorded in resources/stores/auth.ts; revisit if useCookie gains a non-eager write.

Commits: e3fd8fc, a0e8020, cfe8efe. Gate 19/19, tsc clean.

Remaining: §10 components (Button/Badge/Switch/Dialog/Avatar all 0, StxLink 171 and Icon 61 already in use) and rule 11 cosmetic.

9 days ago

§10 — and the bug the audit walked into

§10.1 was already satisfied (bench registers @stacksjs/components through a local shim in plugins/stx-components.ts, with its reason documented). §10.13's reference audit found seven unreferenced components. Six are deleted — ReviewSection, PaywallPage, ArticleHeader, Article, Article/PaywallCta, Modal/BaseModal. PaywallCta only surfaced once Article.stx went, so the audit was re-run until it stopped finding things.

The seventh, MobileMenu.stx, is kept and annotated instead — checking whether it was safe to delete turned up the gap it was written for. BenchHeader puts hidden lg:flex on all three of its regions and offers no hamburger, so below lg the site header is the logo and nothing else. The iOS app is a WebView over this same site. Filed as #90.

The real find: the whole site rendered dark-on-dark

Screenshotting /judges at phone width to check the nav gap showed the hero heading nearly invisible — dark blue on near-black. It is not a hero bug. Every page of the site is unreadable in a dark-mode browser.

bench renders light-only — there is not one dark: variant in any template, and every text colour is picked for a light surface. Nothing in the built page ever painted a background, so the canvas was whatever the user agent chose, and a browser in dark mode chooses black.

Three things had to be missing at once, and all three were:

  1. stx.config.ts sets app.bodyClass, but buildApp()'s SSG ignores app.* — the same reason the webfont tags need a post-build splice. Every built page ships <body> with no attributes at all.
  2. That bodyClass asks for bg-off-white, and off-white is not defined in config/ui.ts's theme.colors (which holds only the scaffolding leftovers veryCool and brand.primary). So even where the class is written — resources/layouts/default.stx puts it on <main> — crosswind emits no rule. grep -c off-white on the built CSS is 0.
  3. No color-scheme meta, so canvas, scrollbars and form controls all followed the OS.

Fixed in efce30e2: app/Helpers/colorScheme.ts declares color-scheme: light and pins html, body to white, spliced post-build like the webfont and CSP tags, and added to the dev shell's headRaw. White rather than a tinted off-white, because white is what the site already rendered as in light mode — this fixes dark mode without touching the light-mode design.

Verified on the built /judges with prefers-color-scheme: dark emulated: before, the h1 computes to oklch(0.21 …) with no background anywhere up the ancestor chain; after, html and body are both rgb(255,255,255) and the page is identical to light mode.

Gate check 20 — "every page declares a colour scheme" — guards it, falsified by stripping the meta from one built page and restoring it. Checked against dist rather than the template, because the declaration is a post-build splice and a template-side test would pass while the artifact shipped without it.

Still open

bg-off-white remains a no-op wherever it appears. Choosing the intended colour is a design decision, not one I should make silently — right now those surfaces are white.

Commits: efce30e2, d746f3ea. Gate 20/20, tsc clean.

9 days ago

Re-triage — every rule measured, including the four that never were

Re-ran the whole scorecard rather than trusting it. Numbers moved in both directions, and two of the original counts were measurement artefacts.

#Rulethennow
1No doctype/html/body13all exempt — see below
2No vanilla DOM18 files—#87 closed
3Crosswind only89documented deviation
4StxLink131 exempt, 2 argued below
5No hand-written <title>04all false positives
6@stacksjs/components firstnot measuredmeasuredbelow
7Cross-page state in a storenot measuredmeasuredbelow
8Directives not JS markup55allowlisted
9No hand-imported auto-imports00clean
10Annotate params and returnsunfalsifiableenforcedgate check 26
11≤2 script blocks66+4+168+4+0not doing it — below
12No unencoded user data in scriptsnot measured0clean

Rule 10 — no longer unfalsifiable (91266080)

This was the one worth the time. tsc --noEmit cannot see inside a .stx file, so a missing annotation in a script block is invisible to every tool in the repo — the rule could be claimed but never checked.

316 functions live in .stx script blocks. 37 had no return type. Seven were plain omissions and are now annotated. The rest are exempt for reasons that belong to stx rather than to taste:

  • getStaticPaths (13) is extracted by regex and run through new Function, so its signature is not ordinary TypeScript.
  • The pagination helpers (15) return arrays of inline object literals, which stx's type-stripping mangles — JudgeDirectory already carried a comment saying so. A named type dodges the inline literal but puts the same colons back in a declaration, so the omission stands until the stripper is fixed upstream.

Gate check 26 enforces the rest. It took three attempts to become trustworthy, which is worth recording:

  1. A regex ending the parameter list at the first ) mis-read closeOnFocusOut(event, close: () => void) and reported an already-annotated function. It walks the parens now.
  2. Matching const x = ( without requiring => reported five parenthesised expressions as unannotated functions — const value = (e.target as HTMLInputElement).value is not a function.

A check that cries wolf is worse than no check, so both were fixed before it went in. Falsified both ways: removing an annotation flags that function; adding an un-annotated arrow flags that one.

Rule 12 — measured, and clean

Zero. Every apparent hit was discussed inside a comment — a naive grep reports 5. Stripping JS comments before matching gives 0 real interpolations inside script blocks.

Rule 5 — the 4 are all false positives

An email template's <title> (required in HTML email), two inside a regex in BenchComingSoon that parses an error page's title, and 500.stx. The original "0" was right about the thing that matters.

Rule 1 — 3 files, all correct

500.stx (must render when the app shell is what broke) plus two email templates, which have to be standalone documents. The original count missed the emails rather than being wrong about the rule.

Rule 6 — measured

StxLink 179 and Icon 51 are in use. Button, Badge, Switch, Dialog, Avatar, Tooltip, Dropdown, Modal, Toast are all 0, against 178 hand-rolled <button>.

Not adopting them. The package has at least one open defect of exactly the kind that bites silently — Notification emits escaped quotes into its own script block (stx#1757) and dies with Unexpected token & — and swapping 178 working, styled buttons for a dependency with that track record trades a lint count for real risk.

Rule 7 — measured

21 stores; 49 of 126 .stx files consume one. state< appears 109 times in stores and 251 times in components — but that is not a violation count: a dropdown's open flag and an unsent draft are local UI state and belong where they are. The cross-page half is the part the rule cares about, and the §6.6 work already moved every fetch into a store.

Rule 11 — measured, and deliberately not done

68 files, 81 <script client> blocks. lang="ts" is now 0. Four files still carry more than two script blocks.

The triage called the client keyword cosmetic. It is not quite: stx's build-mode-detector matches on it directly (CLIENT_SCRIPT_RE), and while the build mode keys off <script server> so stripping client would not change it, this is a codebase where script-block handling has repeatedly failed silently — empty scope registration, the variable extractor dropping multi-line consts, :if branches vanishing. None of those surface in tests, because there are no DOM tests for most of these 68 components.

So: rewriting 81 script tags across the app's entire interactive surface, to delete a redundant keyword, with verification that would mean a manual pass over 68 components. The cost is real, the benefit is zero functional change, and the failure mode is invisible. Recommend closing rule 11 as "won't do" unless a codemod lands upstream with the framework's own guarantee that bare <script> is identical.

Rule 4 — two hits I am leaving

500.stx is the standing exemption. The other two are /terms and /privacy in register.stx, both target="_blank" rel="noopener" so a half-filled signup form is not lost.

StxLink does forward arbitrary attributes — verified in the built output, aria-describedby survives — so converting them would render. But a target="_blank" link is not router-intercepted navigation, which is what rule 4 is about, and whether the dev router would swallow the new tab cannot be checked from the built artifact, which ships no router. Leaving them as <a> keeps the behaviour that matters and is honest about what they are.


Remaining: rule 11 (recommend won't-do), and the rule 3 <style> deviation, which is worth re-testing against current stx since it was recorded when @push styles did not survive SPA navigation — and bench's built site has no client router at all now.

9 days ago

Closing the triage. All 12 rules are now measured — the four that were never measured (6, 7, 10, 12) have numbers, and rule 10 is enforced rather than claimed (gate check 26, 91266080).

Two decisions recorded as won't-do rather than left implied:

  • Rule 11 — 81 <script client> blocks across 68 files stay as they are. Deleting a redundant keyword across the app's entire interactive surface, in a codebase where script-block handling fails silently and those components have no DOM tests, is real risk for zero functional gain. Revisit only if a codemod lands upstream with a guarantee that bare <script> is identical.
  • Rule 6 — the 178 hand-rolled buttons stay. @stacksjs/components has an open defect of exactly the silent kind (stx#1757, Notification emitting escaped quotes into its own script block).

One thing genuinely worth revisiting later: the rule 3 <style> deviation. It was recorded because @push styles did not survive SPA navigation — and bench's built site now ships no client router at all, so the premise may no longer hold. Worth re-testing whenever those components are open anyway.

Scorecard, measurements and the full reasoning are in the comment above.

Sign in to comment on this issue.