ReviewOS

stacks/stx

stx codemod: false positives from unscoped matching — plain <script> blocks, <script server> blocks, URLSearchParams construction, and server-generated HTML strings

#1909
Closed glennmichael123 opened this 14 days ago · 1 comment
14 days ago

stx codemod is genuinely useful — it found real dead code and a real redundancy in this app. This is about the signal-to-noise ratio of what remains, so the list stays worth reading.

After acting on every true finding, 12 reports remain in app code and roughly half are structurally impossible to act on. Four distinct causes:

1. Plain <script> blocks are scanned for [navigate]

{{-- Pre-paint auth bounce. Runs before the runtime exists. --}}
<script>
(function () {
  if (!/(^|;)\s*loghq_token=/.test(document.cookie)) {
    window.location.replace('/login')
    return
  }
})()
</script>
AuthGuard.stx:29  [navigate]  window.location.replace('/login')
    use navigate(path) / goBack() — a full document load throws away every store…

This is a plain <script>, not <script client>. It runs before the client runtime is loaded — that is its whole purpose — so navigate is not defined at that point. The suggestion cannot be applied.

The rule looks correct for <script client>; it should skip blocks that are not client blocks.

2. <script server> blocks are reported for client-only composables

dashboard.stx:10  [search-params]  const query = new URLSearchParams(globalThis.__stxServeSearch || '')
    use useSearchParams() — reactive, and writes back through the router…

That line is in a <script server> block, reading the serve context. useSearchParams is a browser composable — reactive and router-backed are both meaningless server-side. Same shape in settings.stx:16.

3. [search-params] fires on constructing a query string, not reading one

dashboard.stx:129  [search-params]  const p = new URLSearchParams()

Empty constructor, then p.set(...) to build a canonical URL for a link. Nothing is being read from location, so there is no search-params state to adopt. Matching new URLSearchParams unconditionally is what does it; requiring an argument derived from location/search would fix this one.

4. Server-generated HTML strings are scanned as client code

SocialCallbackAction.ts:114  [navigate]  + `location.replace('/dashboard')</script>Signing you in...`,

This is a .ts server Action building an HTML bootstrap page as a string. The location.replace inside it is the generated page's code, delivered to a browser that has no stx runtime yet. navigate() does not exist on either side of that boundary.


Two more where the advice does not fit, for a different reason

Not false positives — the code matches — but the suggested primitive does not fit, and both took a while to establish:

  • [clipboard] → useClipboard(). It returns a plain object (copied: boolean plus subscribe()), not a signal. This app drives three independent copy buttons from three state(false) flags bound in the template. Adopting it means three instances plus a mirror signal per button and a subscribe/unsubscribe — more code than it replaces, and the reactive part still has to be hand-written. A signal-returning variant would make this rule directly actionable.

  • useForm. Two different useForms exist — this one and @stacksjs/composables — and use-form.ts:18-36 documents them as not interchangeable. An app told to "use useForm" has to work out which, and the one with submit orchestration (isSubmitting, setErrors for 422, submitButtonProps) is the other one. Whichever rule eventually points here should name the package.


Suggestion

The four causes above are all "which block/file is this?" rather than "is this pattern present?" — a scope filter (client blocks only, .stx only for client-side rules) would remove most of them.

Also worth considering: the report counts sites in storage/framework/defaults/, which is git-ignored vendored scaffold. In this repo that is 102 of 114 reported sites, so the headline number is dominated by files the app cannot edit. Respecting .gitignore, or a flag to, would make the total mean something.

14 days ago

Fixed in 544698e567 and d13b34459d. Measured against your four causes:

causebeforenow
1. plain <script> scanned1 finding0
2. <script server> scanned00 (already fixed in #1905)
3. empty new URLSearchParams()1 finding0
4. HTML string in a .ts Action00 (already fixed in #1905)
a genuine new URLSearchParams(location.search)11

Cause 1 is the one your report sharpened for me. I had already skipped <script server> on the grounds that the primitives are client-side. A plain <script> is a better argument: it runs before the runtime loads, which is usually the entire reason it was written that way, so navigate() does not exist at that moment. The advice was impossible to apply, not just unhelpful. Only <script client> is guaranteed the runtime.

Cause 3 now requires a constructor argument that reads a query. Building a link with p.set(…) is not adopting search-param state.

And the .gitignore suggestion is in — that was the highest-leverage item in the report. 102 of 114 sites being vendored scaffold means the total meant nothing. It asks git rather than carrying a list of vendor directories, since the project already declares what it treats as vendored. Best effort: no git, no repo, nothing ignored.

The two "advice does not fit" notes

Both are fair and I have not acted on either, deliberately:

  • useClipboard() returning a plain object rather than a signal is a real API gap, and your three-copy-buttons case is the right test of it. That is a change to the composable, not to the rule, and it deserves its own issue.
  • Two useForms — agreed the rule should name the package when it eventually points there. use-form.ts:18-36 documenting them as not interchangeable is exactly the trap.

Worth filing those two separately if you want them tracked; they are not codemod bugs.

Sign in to comment on this issue.