ReviewOS

stacks/stx

codemod: no `alert` rule, so 30 native alert() calls in one app go unreported while stxAlert ships beside the detected stxConfirm

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

stxAlert sits directly beside stxConfirm in the runtime — same signature, same _createDialog call, both on window — but the codemod has a confirm rule and no alert rule. So the native call it replaces is invisible.

function stxAlert(message, options) {
  return _createDialog(message, options, false);
}

function stxConfirm(message, options) {
  return _createDialog(message, options, true);
}

Scale

One app: 30 alert() calls in a single file, none reported. Same app, same run, the confirm rule correctly flagged all 3 of its confirm() sites. The adoption gap #1843 describes is identical for both primitives; only one of them is detected.

The calls are the ordinary kind — validation feedback and success notices:

alert('Please enter both email and password')
alert('Please enter a valid email address')
alert('Failed to subscribe. Please try again later.')
alert('Link copied to clipboard!')

Rewritable, but not blindly — and differently from confirm

Worth being precise, because the caveat is not the same one:

  • confirm is dangerous because the return value flips. if (confirm(m)) → if (stxConfirm(m)) is always true. Hence "needs a human", which is right.

  • alert has no return value to get wrong. The difference is that native alert() blocks and stxAlert() returns a Promise. A bare statement is safe:

    alert('Saved')            // →  stxAlert('Saved')

    What changes is code that leaned on the block:

    alert('About to reload')
    location.reload()          // used to run after dismissal; now runs immediately

So the honest classification is somewhere between the two existing buckets: safe to rewrite when the call is an expression statement and nothing follows it in the same block, needs a human when it does.

Suggested fix

Add an alert rule. Conservative version: report-only, matching the ten existing report rules — that alone closes the detection gap, and per the module docblock "reporting is most of the value anyway".

If it should rewrite, the guard is a local one: rewrite when the call is an expression statement that is the last statement in its block, report otherwise with the blocking-semantics reason. That mirrors how the confirm rule already explains itself rather than guessing.

14 days ago

Fixed in 1ea9808b44. There is an alert rule now, on by default beside confirm.

Report-only, which is the conservative option you offered — and your analysis of why the caveat differs is what the implementation and its docblock are built on, because it is the part someone would otherwise get wrong later:

  • confirm is dangerous because the return value flips. if (stxConfirm(m)) is always true.
  • alert has no return value to get wrong. The difference is that native alert() blocks, so anything after it in the same block ran after dismissal and would now run immediately.

Your alert('About to reload'); location.reload() example is in the finding's own message, so the reader gets the caveat rather than just the suggestion.

I did not take the conditional-rewrite option. "Expression statement that is the last statement in its block" is a materially harder thing to prove than the async check confirm uses, and the payoff is one line of typing — for a rule whose entire value per #1843 is detection, since the primitive ships and gets used zero times.

window.alert and a file declaring its own alert are both skipped, the same guard #1898 added for confirm.

Added to the --rule help and to the adoption table in docs/guide/adopting-primitives.md, with the blocking-semantics caveat written out beside the confirm one.

5 tests; removing the rule fails 3.

Sign in to comment on this issue.