ReviewOS

stacks/stx

codemod: confirm/alert rules miss `window.`-qualified calls — found 1 of 3 confirm sites in a real app, and the 2 missed were the destructive ones

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

The confirm and alert rules match a bare call only. window.confirm(…) and window.alert(…) are not detected, and they are the form a linted codebase is most likely to contain — several style guides require the explicit global.

Repro

<script client>
  const a = async () => {
    if (!confirm('bare form')) return
    if (!window.confirm('window form')) return
    alert('bare alert')
    window.alert('window alert')
  }
</script>
$ stx codemod "dialogs.stx"

Would rewrite 1 site(s) (pass --fix):
  dialogs.stx:3  [confirm]  if (!confirm('bare form')) return

An stx primitive already covers this — 1 site(s):
  dialogs.stx:5  [alert]  alert('bare alert')

2 of 4 found. Both window.-qualified calls are invisible.

Where it bit

Auditing a real app, stx codemod "resources/**/*.stx" reported one confirm site. The app actually had three:

resources/views/queue.stx:373      if (!confirm('Delete this post? This cannot be undone.'))          ← found
resources/views/accounts.stx:775   if (!window.confirm(`Permanently delete ${label}? …`))             ← missed
resources/views/listening.stx:263  if (!draftId() || !window.confirm('Delete this listening rule…'))  ← missed

The two misses are the destructive ones — a permanent purge and a rule deletion. Reporting one of three reads as "almost clean" when the opposite is true, and that is worse than not running the rule, because the clean-looking report is what gets trusted.

Fix

Allow an optional window. (and globalThis.) qualifier in both patterns. For confirm, the existing "needs a human" reasoning is unchanged — the Promise caveat applies identically. For alert, so does the blocking-semantics note.

Worth checking the ten report-only rules for the same assumption: navigate already handles location.assign and window.location.href (its pattern is unanchored), but e.g. clipboard matches navigator.clipboard and would miss window.navigator.clipboard.

14 days ago

Fixed in 2e56a20964. Your repro now reports all 4 sites, and --fix rewrites window.confirm(x) to await stxConfirm(x) with no stray receiver.

This corrects reasoning I wrote, and your framing is what overturned it. The rule skipped the qualified form on the grounds that naming the global is "an explicit choice of the native dialog". That is wrong for exactly the reason you gave: several style guides require the explicit global, so it is the form a linted codebase contains — meaning the rule was quietest precisely where a house style is enforced. "The 2 missed were the destructive ones" is the part that makes it worth fixing rather than noting.

Other receivers stay excluded: this.confirm(...) and dialog.confirm(...) are somebody else's method. The rewrite now replaces the whole matched call head rather than a fixed confirm( width, so the receiver goes with it, and that is pinned.

The test that encoded the old behaviour is updated rather than deleted, with the measurement that overturned it written beside it — the next person to read that rule should see why it changed.

Sign in to comment on this issue.