ReviewOS

stacks/stx

DOM API violation names the layout, not the view the script was authored in (points at a file with no match)

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

A buddy build of a Stacks app prints:

[STX] DOM API violation in default.stx:
  ✗ window.localStorage is prohibited (line: 8)
    → Use useLocalStorage() from composables

default.stx does not contain localStorage. Neither does any other default.stx in the project — I checked all nine, including the framework defaults:

resources/layouts/default.stx                                        0 matches
storage/framework/defaults/resources/layouts/default.stx             0
storage/framework/defaults/resources/layouts/{system-tray,dashboard}/default.stx  0
… (9 files, all 0)

The code is in resources/views/login.stx:14, and resources/layouts/default.stx is just the layout that view extends (login.stx:1 → @extends('layouts/default')).

Cause

script-validation.ts:215 reports whatever it is handed:

const fileName = filePath.split('/').pop() || filePath

and process.ts:1781 hands it the file being processed at that point:

for (const { match, attrs, content } of clientScriptMatches) {
  validateClientScript(content, filePath, options.strict)

When a view's <script> lives inside @section('content') and the view is composed into a layout, that filePath is the layout's. So the diagnostic names a file that does not contain the offending code, and the reported line number indexes neither file — line: 8 against login.stx:14, and default.stx is 5 lines long in total.

For a project driving a standards cleanup off these warnings, this costs real time: I went looking through every default.stx on disk before finding the actual source.

Suggested fix

Thread the authoring file (and its line offset) through to validateClientScript rather than the file currently being processed, so the message points at login.stx:14. If the origin isn't available at that call site, saying in <layout> (script from a composed view) would still beat naming a file that provably doesn't contain the match.

Second question, possibly by design

The block it flagged is a plain <script>, not <script client> — a deliberate pre-paint theme bootstrap:

<script>
  // Login uses a different layout, so it carries its own theme bootstrap.
  // Stays a plain pre-paint script on purpose: it must run before first paint,
  // and a hydrated directive would flash the wrong theme. See
  // stacksjs/stx#1788 for why useColorMode cannot express this yet.
  ;(() => {
    try {
      const s = localStorage.getItem('theme') || 'system'

Pre-paint is the one place useLocalStorage() cannot be the answer — the runtime does not exist yet, which is the whole reason the block is written this way. Suggesting it there is advice that cannot be taken.

If bare <script> is meant to be server/pre-paint (as script-classifier.js treats a signal-free bare block), should the DOM validator run on it at all? If it should, the suggestion text could acknowledge the pre-paint case rather than pointing at a composable that isn't loaded yet.

9 days ago

Both halves are now done.

The attribution half — already fixed

ba9381e3d9 landed before I got here. It uses context.__originalFilePath, which the layout composition already stashes, so nothing needed threading — the call site just never passed it:

[STX] DOM API violation in resources/views/login.stx (composed into resources/layouts/default.stx):

It also fixed the two related things you flagged: the bare basename became a cwd-relative path (your nine default.stx files), and (line: 8) — which indexed neither file — is now labelled script line 8 with the offending line quoted, so it is findable by searching rather than counting.

Your second question — fixed in 8cc5154

should the DOM validator run on it at all?

Yes, and I did not change that. A bare <script> is client-side in stx — only <script server> runs on the server — so it belongs in the validator. There is a test asserting the rule still fires, because naming an escape hatch must not quietly turn a rule off.

the suggestion text could acknowledge the pre-paint case rather than pointing at a composable that isn't loaded yet

Agreed, and this was the real defect: advice that cannot be followed reads as the rule not understanding the code, which costs more trust than the rule earns.

✗ window.localStorage is prohibited (script line 1)
  → Use useLocalStorage() from composables — or @appearanceBootstrap if this
    must run before first paint, where the runtime does not exist yet

@appearanceBootstrap is very likely the actual fix for your login page. It does exactly what your block does — reads the stored theme, resolves appearance and colour mode, sets the attributes and the dark class, all before paint — and it is emitted as data-stx-scoped, so it is never validated. That makes your comment about #1788 unnecessary for the theme case specifically.

Only the rule with a genuine pre-paint story gets the extra clause; sessionStorage is untouched, with a test pinning that.

What I deliberately did not add

A per-script opt-out. allowPatterns exists but is project-wide and matches on the message, so silencing this one file would lose real localStorage violations everywhere else — too blunt for your case.

A per-script marker (an attribute or an ignore comment) would fix that properly, but it is new public API and your issue framed this as a question rather than a request, so it seemed wrong for me to invent the spelling unilaterally. If @appearanceBootstrap does not cover a pre-paint script of yours that is not a theme bootstrap, say so and I will add one — that is the remaining gap.

Full suite 10776 pass in a clean worktree; the two failures are the pre-existing crosswind-arbitrary-values case and a package-exports timeout that passes in isolation.

Sign in to comment on this issue.