ReviewOS

stacks/stx

stx-router fragment-swap path silently drops scope IIFEs on revisit — dedup check is missing the `isAlreadyScoped` escape

#1700
Closed glennmichael123 opened this 10 days ago ·
10 days ago

Summary

After the recent run of router/signals fixes, navigating away from a page and back to it (fragment-swap SPA nav) leaves the page's scope IIFEs un-executed on the return visit. The scope element is in the DOM but absent from window.stx._scopes, so the scope walk in signals.js skips it, bindIf/@event/x-class never bind, and mutually-exclusive :if branches all render at once.

Visible symptom: all three :if branches of a reviews-feed-style component (loading, empty, list) are rendered simultaneously when the user returns to the page via SPA nav after visiting another page.

Reproduction

  1. Visit /reviews (page with a scope IIFE: <script data-stx-scoped>(function(){ ... })()</script>).
  2. Click an in-app <StxLink> to a different same-layout page — e.g. /review. (In my repro /review auth-gates to /login, but the auth redirect is incidental — any same-layout fragment-swap nav reproduces it.)
  3. Click an <StxLink> back to /reviews.
  4. Page renders with every :if branch visible at once. Console shows no useStore: reviews / no bindIf logs for the ReviewsFeed scope on the return visit, and _latestSetup keys still references the previous page's setup.

window.stx._scopes['stx_scope_ReviewsFeed_<n>'] is undefined after step 3; the corresponding DOM element exists with data-stx-scope set.

Root cause

Two interacting pieces:

(a) signals.js's cleanupContainer (called by the router before each swap) iterates the departing container's [data-stx-scope] elements and deletes their entries from window.stx._scopes (signals.js:3970-3983). So leaving /reviews for /login clears stx_scope_ReviewsFeed_<n>.

(b) stx-router/src/client.ts:354 (fragment-swap path) skips re-executing any non-setup script whose hash was previously seen:

var h=hashScript(code);
var isSetup=code.indexOf('__stx_setup_')!==-1;
if(!isSetup&&executedScriptHashes[h]){
  log('[router] skipping already-executed script (hash dedup)');
  return;
}

executedScriptHashes is seeded with every script's hash at router-init time (src/client.ts:128-132). On the return visit, the ReviewsFeed IIFE hash is already present → script is skipped → scope is never re-registered.

The full-doc swap path further down (src/client.ts:592) gets this right:

if(!runAlways&&!isSetup&&executedScriptHashes[h])return;

The fragment path lost the runAlways escape. The full-doc path's addScript(text, true) always sets runAlways=true for inside-container scripts, which is the equivalent of saying "self-contained IIFE — always rerun." The fragment path doesn't have a runAlways concept at all.

The dedup's purpose (per the inline comment on line 347-351) is to avoid re-running layout-level partials that declare top-level const/let. But the isAlreadyScoped check at line 370 already wraps non-IIFE non-import scripts in { ... } block scope to neutralise that problem. IIFE scripts are inherently safe to re-run. So the dedup is over-broad in this branch.

Suggested fix

Hoist isAlreadyScoped computation above the dedup gate and exempt IIFE scripts from dedup. Self-contained IIFEs (the canonical scope-script shape) own their own scope and re-running them is always safe.

           var h=hashScript(code);
           var isSetup=code.indexOf('__stx_setup_')!==-1;
-          if(!isSetup&&executedScriptHashes[h]){
+          // Compute isAlreadyScoped first so the dedup check can
+          // exempt self-contained IIFE scripts. data-stx-scoped scope
+          // IIFEs ((function(){ ... })() shape) must re-execute on
+          // every nav — they own their scope, won't collide with
+          // prior runs, AND _cleanupContainer just deleted their
+          // entry from window.stx._scopes on the way out. Without
+          // this exemption, navigating away and back leaves the page
+          // leaf with no scope registered: bindIf/@event never
+          // re-bind and all :if branches stay visible at once.
+          // Mirrors the full-doc swap path's runAlways escape below.
+          var isAlreadyScoped=code.trimStart().charAt(0)==='('||code.trimStart().charAt(0)===';'||code.indexOf('window.stx.mount')>-1;
+          if(!isSetup&&!isAlreadyScoped&&executedScriptHashes[h]){
             log('[router] skipping already-executed script (hash dedup)');
             return;
           }
           executedScriptHashes[h]=1;
           var ns=document.createElement('script');
           var hasImport=code.indexOf('import ')!==-1;
           if(hasImport){
             ns.type='module';
           }
-          // Wrap in block scope to prevent const/let collisions ...
-          var isAlreadyScoped=code.trimStart().charAt(0)==='('||...;

(The isAlreadyScoped definition just gets moved above the dedup check.)

Why it matters

This is a regression that affects every page using the scope-IIFE pattern as soon as the user revisits it via SPA nav. The page renders garbled (multiple :if branches at once), state stays stale, fetches don't fire. The bug is invisible on hard reload — only SPA nav triggers it.

In my project this manifested as a /reviews list page hung on "Loading reviews…" stacked on top of "No reviews yet" stacked on top of an actual review card, after the user navigated /reviews → /login → /reviews.

Patch I'm running locally

pantry/stx-router/dist/index.js and the matching src/client.ts — same diff as above. Will drop the local patch as soon as upstream lands.

Environment

  • stx-router (built from main at 0a71c7c1d5 + your 1899048 issue-fix batch)
  • Pages: /reviews (extends layouts/default) → /login (extends layouts/default) → /reviews
  • Both pages use <script data-stx-scoped>(function(){ ... })()</script> scope IIFEs

🤖 Filed via Claude Code

9 days ago

Fixed in f3483904a4 — fix(router): exempt scope IIFEs from fragment-swap hash dedup (#1700).

The patch is the same as the suggested diff in the issue: isAlreadyScoped is computed before the dedup gate, and the gate now exempts self-contained IIFE scripts. dist/index.js is up to date with the source.

Closing — should have happened when the commit landed, missed it.

Sign in to comment on this issue.