ReviewOS

stacks/stx

codemod: false positives inside comments and on component props make --fix unsafe

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

Ran stx codemod over a real app (analyticshq, 39 .stx files) on v0.2.171. It reported 30 sites; 1 was genuine. Two distinct false-positive classes, and the first makes --fix actively destructive.

1. Component props named title are treated as tooltips — --fix would corrupt them

23 of the 24 "would rewrite" hits were the tooltip rule matching every title="…". About ten of them are not tooltips at all:

dashboard.stx:1404  [tooltip]  <BreakdownPanel title="Top sources" :rows="referrers" … />
dashboard.stx:1450  [tooltip]  <BreakdownPanel title="Top countries" :rows="countries" … />
dashboard.stx:1618  [tooltip]  <BreakdownPanel title="Campaign sources" … />

title there is a prop on an app component — it renders as the panel's <h2>. Rewriting it to a tooltip primitive changes the component's public API and silently drops the heading.

Suggested: skip elements whose tag is a component (capitalised, or resolvable as one). A native title on <span>/<a> is a fair hit; a prop on <BreakdownPanel> is not.

2. Rules match inside comments

Three of the six "an stx primitive already covers this" hits are prose, not code:

dashboard.stx:809   [navigate]  // Site switcher. Was an inline onchange="location.href = ..." on the <select>, i.e.
account.stx:215     [navigate]  // window.location.replace(url), so the emitted behaviour is byte-for-byte what it
dashboard.stx:928   [confirm]   // Split from one condition into two: the old `shareBusy() || !confirm(...)` relied on

All three describe code that was already migrated to the primitive — the comments exist to explain the migration. So the rule fires hardest on exactly the code that already complied, which is close to the worst possible signal.

stx already has stripCommentsAndLiterals in strip-literals.ts (the handoff calls out keeping one implementation of that); running rules over the stripped text would fix this class.

3. Minor: a client-only rule fires inside <script server>

dashboard.stx:142  [search-params]  const q = new URLSearchParams()
    use useSearchParams() — reactive, and writes back through the router

Line 142 is inside <script server>. A reactive client composable is not the fix there. The block kind is already known — extractScriptBlocks returns it — so rules could declare which kinds they apply to.

Net

bucketcount
title on a component~10
matched inside a comment3
client rule in a server block1
duplicate reports of one site2
genuine (hand-rolled navigator.clipboard with fallback → useClipboard())1

The clipboard hit is a good find and exactly what the tool is for. But at a ~3% true-positive rate the output gets muted on first read, which is the failure mode the sweep handoff itself warns about: "a gate that invents errors gets muted, and a muted gate catches nothing."

Happy to take this — comment-stripping and the component-tag check both look self-contained.

9 days ago

Fixed in 630c2aaae1. On your repro shape the output goes 8 findings → 4, and all 4 are correct.

Comments and strings — rules now match a view with both blanked, positions preserved. Your framing was the sharpest part of the report: the comments existed to explain the migration, so the rule fired hardest on exactly the code that had already complied.

Server blocks — blanked too. Every primitive these rules suggest is client-side, so new URLSearchParams() in <script server> was correct code being told to adopt a composable it cannot use. I did this by blanking the block rather than threading block kinds through, which gets the same result without a per-rule declaration to keep in sync.

Component props — already fixed in 1ea9808b44, from #1902. You were on v0.2.171, which predates it.

One thing the fix had to be talked out of

Blanking string literals broke click-outside, whose pattern names 'click' — caught by its own test. A rule that needs its literals is now detected from /['"]/.test(pattern.source), i.e. from the rule itself, rather than a hand-kept exception list. A list of rules that must agree with another list of rules is precisely how this codebase keeps getting bitten, and the handoff you quoted says so.

The tooltip rule needed a third view, comments-only: stripCommentsAndLiterals is JavaScript-aware and would blank title="Top sources" — the value the rule has to copy.

reject hooks still read the original text, so location.href = 'mailto:…' is still skipped. Pinned, because the code view blanks the very literal that decision reads.

Duplicates are gone too: one line, one finding per rule.

10 tests; removing the blanking fails 3.

The clipboard hit being the one genuine finding is a fair summary of where the tool was — thanks for measuring it rather than just reporting the noise.

Sign in to comment on this issue.