Bug: Each Bindings Lose External Reactivity#175
Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
1 Skipped Deployment
|
❌ Regression for
|
| metric | Regression |
|---|---|
hydrate-helper-100-state-change |
+18% (1ms) ❗ |
hydrate-helper-100-mount |
+12% (5ms) |
hydrate-each-100 |
+10% (8ms) |
hydrate-each-100-mount |
+5% (3ms) |
📜 Regressions from peak (31)
These metrics were better on a prior commit than they are now. The peak CI dominates current CI — not attributable to per-sample noise. Bisect candidates are the commits between the peak and HEAD; nearest-to-peak is usually the best bet.
⚪ No Change (21)
Metrics where this PR measured within ±2% of main — no meaningful performance change detected.
| metric | Change |
|---|---|
add-20 |
-0.1% – +0.1% |
bulk-add-500 |
-0.5% – +0.7% |
clear-completed-250 |
-0.9% – +0.8% |
create-10k |
-0.3% – +0.4% |
create-1k |
-0.9% – +0.8% |
edit-cycle-5 |
-0.2% – +0.5% |
edit-start-10 |
-0.3% – +1.9% |
filter-cycle-20 |
-0.4% – +0.7% |
remove-10-middle |
-0.8% – +0.6% |
remove-first-10 |
-0.9% – +0.9% |
remove-last-10 |
-0.1% – +0.1% |
remove-middle-10 |
-0.4% – +0.2% |
remove-row-back-10 |
-1.8% – +0.6% |
remove-row-front-20 |
-1.3% – +0.6% |
select-40 |
-0.6% – +0.9% |
swap-rows-20 |
-1.0% – +0.9% |
toggle-10 |
-0.5% – +0.3% |
toggle-all-20 |
-0.7% – +0.4% |
toggle-first-10 |
0.0% – +0.2% |
toggle-last-10 |
-0.4% – +0.2% |
toggle-middle-10 |
-0.3% – +0.4% |
🔍 Unsure (7)
Inconclusive (6)
The measured difference is small, and our sampling couldn't confidently place it above or below zero. Running more samples in a future run might settle these metrics.
| metric | Change | Expected Noise |
|---|---|---|
append-1k |
-1.8% – +3.0% | ±1% |
clear-10k |
-4.4% – +1.5% | ±1% |
remove-5-back |
-4.3% – +1.6% | ±2% |
remove-5-front |
-3.6% – +0.5% | ±2% |
remove-row-middle-20 |
-2.5% – +1.1% | ±0% |
update-10th-10 |
-2.2% – +2.2% | ±1% |
Too Fast to Measure Precisely (1)
On benches this short, system jitter (scheduling, GC, JIT) masks sub-4% changes; larger deltas still resolve cleanly.
| metric | Change | Test Time | Expected Noise |
|---|---|---|---|
replace-1k |
-2.4% – +0.3% | ~99ms | ±2% |
Sample size: 50 · Resolution floor: ±2% · Timeout: 3min · Wall-clock: 17m15s
each.hydrate registers a dep on the items collection and defers per-item
Reaction wiring to the first signal change — a perf optimization that
assumes per-item bindings only read item-local data. When a binding
helper closes over external state (e.g. {classMap getItemClasses item}
where getItemClasses reads state.activeID) and items never mutates after
hydrate, those Reactions never wire and external mutations silently lose
reactivity. The inpage-menu in the docs site reproduces this on every
page that hydrates with menu items from props.
Adds a static classifier (each-content-classifier.js) that walks the
each's content AST once per AST identity (cached on a WeakMap) and
classifies every binding's identifier set against the iteration scope
plus the framework's pure-helper registry. Self-contained → keep
current lazy hydrate. Anything else → adopt eagerly so per-item
Reactions register their external deps now.
Lives in the renderer (only invoked from each.hydrate) so the 90% of
users who runtime-compile in the browser without SSR pay nothing.
SSR users pay one walk per unique each-content shape.
Conservative bails: no-`as` each (item keys spread into local scope,
indistinguishable from external names), snippet/template/rerender/async
invocations (cross-AST flow). False positives pay the eager-wire cost
where it wasn't strictly needed; false negatives silently lose
reactivity (the bug). Conservative is the safe direction.
Unskips snippet-with-named-args-inside-each test — same fix family.
jlukic
changed the title
jlukic
changed the title
jlukic
changed the title
Embeds the non-obvious findings from the each-hydrate-external-state investigation into the ssr-hydration skill so future agents land on them without redoing the archaeology: - Critical gotchas section near top: innerHTML doesn't process DSD (use setHTMLUnsafe), renderToString empty in browser env unless Template.isServer = true (try/finally), bench noise floor scales inversely with bench duration, cross-session "regressed from peak" verdicts are phantoms. - New "skipFirstWrite contract" subsection — names the flag as the grep-able mechanism behind evaluation-as-witness reactivity. - New "Each-block hydration: lazy by default, eager when content reads external state" section — covers the each-content-classifier, conservative bails (no-`as`, snippet/template/rerender/async), the inpage-menu canonical repro, per-AST cache. - hydrateMarkers Pass 1 updated from legacy ref-DOM walker to Plan 04 data-sui-bind fast path. - Quick Reference + Key Files updated. Plan archive (ai/plans/archive/hydrate-each-external-state.md) records the full investigation arc, fresh-take debate, and empirical bench verdict for posterity. Eval workspace (ai/skills/authoring/ssr-hydration-workspace/) holds 3 iterations of subagent runs scored by an opus judge against a 27-assertion rubric. Final: with-skill 27/27 (100%) vs no-skill baseline 21/27 (78%) — +22pp.
The skill update has shipped; the iteration evidence has served its purpose. Per ai_folder_layout, /ai/skills/ is for MCP-served content, not eval workspace state.
Public entry point only walked eachNode.content; the inner each handler
already checked both branches. When SSR rendered only the {:else} (items
empty at hydrate time) and that branch read external state, bindings
stayed unwired forever — same silent-reactivity-loss class this PR is
fixing for item content.
Also refreshes the each.js block comment that still claimed "no per-item
Reactions are wired at hydrate time" — true before this PR, misleading
now that the eager path exists.
Five fixes from the second-pass review:
- each.hydrate's eager path early-returned on empty items, so an each
block whose server output was the {else} branch never wired its
bindings — the classifier verdict was correct but no code acted on
it. Restore the pre-perf-pass empty-items + elseContent hydrate
branch (mirrors renderElse's record shape).
- isEachContentSelfContained passed the iteration-inclusive scope to
the elseContent check. The {:else} branch runs outside the iteration,
so a name collision (`{#each foo in items}...{else}{foo}{/each}`
where `foo` is also a parent signal) was silently classified
in-scope. Use parent scope, matching the inner each handler.
- Classifier's "identifier followed by `:`" object-key skip didn't
distinguish ternary then-values (`{a: cond ? externalThen : x}`).
Track pending-ternary count per brace level; only skip when no
ternary `?` is open at this level. Optional chaining and nullish
coalescing don't open ternaries.
- STRING_LITERAL_RE greedy-matched backtick contents including
${interpolation}, hiding external reads. Bail conservative on any
backtick — template literals as expression bodies are unusual in
this DSL.
- each.hydrate called lookupExpression(node.over) once explicitly,
then again via resolveItems in the eager branch. Move the standalone
call into the lazy branch where it's the only thing that runs.
Tests: 9 new unit tests (ternary disambiguation, optional chaining,
template literal bail, elseContent scope collision) + browser
regression test for elseContent reactivity after hydrate.
- each.hydrate's elseContent path created `elseScope` and stored it on
the isElse record but never registered it on `region.childScopes`.
On the next else→items transition, `region.clear()` only disposes
scopes registered there, and `self.records.length = 0` orphaned the
record. Result: a one-shot scope leak per hydrated-then-transitioned
each block. renderElse picks up registration via `region.setContent`;
the hydrate path bypasses setContent (DOM is already in place), so
register explicitly.
- Reframe the inpage-menu regression test comment to past tense ("Pre-fix,
each.hydrate only registered ...") so future readers don't think the
bug is still active. Drop the stale `each.js:645` line ref.
- Update the skill doc's hydrate pseudocode to match the post-PR
structure (lookupExpression now lives inside the lazy branch; eager
branch picks up the dep via resolveItems).
- Remove `this` from RESERVED_NAMES — it's already in IMPLICIT_LOCALS
(which adds it via buildLocalScope). The duplicate was harmless but
contradicted RESERVED_NAMES's "don't read data context" contract.
- Clarify the parentScope comment in isEachContentSelfContained:
empty Set captures BOTH `eachNode.as` and IMPLICIT_LOCALS being
absent in the :else context, not just `as`.
Inside `{#each item in items}`, `this` is NOT bound to the item — only
the explicit `as` name is. `this` resolves to the parent data context,
which is external state from the each's perspective. Treating it as
iteration-local meant `{this.x}` inside an as-each silently lost
reactivity to parent mutations.
The no-as form `{#each items}` does set `eachData.this = item`, but
those eaches bail at the public-entry no-`as` check before scope
construction runs. So removing `this` from IMPLICIT_LOCALS only
affects the as-form, where the change is the bugfix.
Rip out the static-AST classifier introduced earlier in this PR. each.hydrate now honors the same "register Reactions on hydrate" contract every other block hook honors: call adoptServerItems immediately, wire per-item Reactions in place against the server- rendered DOM. No lazy/eager gate, no shadow lexer, no hardcoded block-name table. Why the classifier had to go: - Abstraction-breaking: lived in blocks/ but wasn't a block; re-derived per-block knowledge via hardcoded `case` statements duplicating the registry. - Extensibility hole: user-registered custom blocks always hit the default bail; framework-shipped block types were privileged. - Shadow lexer: hardcoded RESERVED_NAMES, IMPLICIT_LOCALS, char-by-char expression scanning with brace-depth + ternary tracking — re-implementing what the compiler does. - Predicting what runtime tells us for free: Dependency.depend() is a no-op outside an active Reaction; the runtime per-binding Reaction IS the optimal analyzer. Empirical justification: the 1000-item bench showed eager wiring is flat vs main at the mount window. The lazy optimization the classifier was protecting wasn't paying for itself at any scale we measured. The +19%/+1ms on state-change is the work the framework should be doing on a state mutation; main's "fast" baseline was the silent-failure shape this PR repairs. Net: -513 lines (260 classifier + 240 unit tests + skill/plan/each.js simplifications). Bug fix and regression tests intact: helper-call attribute reactivity, snippet-args-in-each, elseContent reactivity all pass under the simpler shape.
After the classifier removal, `self.hasHydrated = true` only happens
when `each.hydrate`'s `adoptServerItems` already failed (legacy SSR
without per-item markers). Server markers don't appear retroactively,
so the retry in `update` would always fail and fall through to
nuke-and-rebuild — making the entire retry block dead code.
Inline the legacy fallback directly: clear region, drop records,
proceed to fresh reconcile.
Also refresh two stale comments the classifier removal exposed:
- adoptServerItems header described its old timing ("first-data-change
adoption path") — now describes what the function does.
- elseContent regression test comment referenced "each.hydrate's eager
path" — classifier-era concept that no longer exists.
Pre-1.0, no tagged release has shipped SSR. There's no scenario where a client of this version hydrates server output from an older version. The server always emits `<!--sui-item:v1:KEY-->` markers for non-empty items (server.js:422); hydrate can trust them. Changes: - adoptServerItems no longer returns boolean. Throws on missing markers (build/version mismatch — surface loudly, not silently fall through). - each.hydrate drops the `if (!adopted) self.hasHydrated = true` line. - each.update drops the entire `if (self.hasHydrated)` legacy-rebuild block. - `hasHydrated` field removed from create() initial state. Skill doc and plan archive updated.
Pre-1.0, no tagged release has shipped, so there's no scenario where the client hydrates SSR output that lacks `data-sui-bind`. The fast path is the only path. Removed: - The 70-line parallel ref-DOM walker in `hydrateAttributes` - The `refRoot` lazy getter on the buildString cache and its supporting scaffolding - The `ATTR_MARKER_PATTERN` constant and `ATTR_MARKER_SUFFIX` import (only the walker referenced them) - The `ast` parameter threaded through `hydrateMarkers` and `hydrateInnerContent` (only the walker needed it) - `hydrateAttributesViaDataBind` renamed to plain `hydrateAttributes` since the dispatcher is gone Two follow-on simplifications: - Inlined `parseAttributeParts` instance wrapper at its sole remaining caller (`bindMarkers`) - Dropped the `hasAttrEntries` pre-walk gate in `hydrateMarkers` — `hydrateAttributes` is a no-op when no element carries the marker Net -127 / +14 lines. All 921 tests pass.
Five small comment touchups surfaced by the round-7 Opus review pass. Code unchanged; just the comment surface catching up to the deletions in a08d8ba and earlier. - renderer.js buildStringCache header described the entry shape as `{ htmlString, entries }`; the actual shape post-refRoot-removal is `{ html, svg }` (HTML + SVG buildHTMLString variants). - each.js block-level docstring used "deferred to a future items mutation that may never come" as motivation — defines-against-a- ghost now that the lazy/classifier path is gone. Trimmed. - each.js adoptServerItems comment referenced "the data-sui-bind fast path" — the legacy walker is gone, so there's no slow path to be fast against. Just describe what hydrateMarkers does. - renderer.js hydrateMarkers comment used "pre-classification gate" to refer to the dropped `hasAttrEntries` pre-walk — "classification" overlaps awkwardly with the deleted each-content classifier concept. Renamed to "pre-walk gate". - Plan archive `Final architecture` pseudocode had a duplicate `adoptServerItems` call — editing artifact from when I rewrote the snippet during the rip-out.
Leftover from when update could call adoptServerItems on first mutation (the legacy-fallback retry ripped out in 1dfc4f0). The parameter was kept in the destructure but never referenced in the body. Surfaced by the round-8 review pass.
Falsifiable regression coverage for `region.childScopes.push(elseScope)` in each.hydrate's empty-items + elseContent branch. Without it, the elseScope is orphaned at transition time — the elseContent's bindings keep their subscriptions on whatever signals they read until the isConnected guard self-stops them on the next stale fire (a bounded leak invisible to behavioral assertions because the guard short- circuits before the callback runs). Strategy: control-baseline subscriber-count comparison. A control component renders with items already populated (elseContent never runs, baseline = framework-internal subscribers only). A second component renders empty, picks up extra subscribers per elseContent binding, then transitions to non-empty. After the transition, the subscriber count must return to the control baseline — equality proves no orphans remain. Three bindings on `emptyLabel` in the elseContent give a 1-vs-N delta robust to framework-internal counts. Verified empirically: - With the line: count returns to baseline → test passes - Without the line: count stays elevated → test fails
jlukic
changed the title
Encodes patterns surfaced during the PR #175 author-review iteration. Adds punctuation pass (em-dashes, colons, semicolons), word target, voice/tense consistency, bug-PR discovery framing, risk score anchoring.
jlukic
added a commit
that referenced
this pull request
May 4, 2026
…omment trim
- attributeBinding shape narrows to { parts } — rawAttrName had only one
reader (reconstructed inline at the consumer from the data-sui-bind
payload itself), markerIDs had no readers post-firstMarkerID revert.
- Marker ID parsers use `+str` instead of `parseInt(str)`. Compiler-emitted
inputs are digit-only after a fixed prefix, so semantics match. ~2-3×
faster, drops the no-radix linter concern.
- Comment trims at four clear sites (formatBlockClose, hydrateInto trailing
default sentence, server.js per-item key marker, server.js plain-AST
branch). Bench-hydrate file header refreshed to describe the eager-mount
shape (was still describing pre-#175 lazy adoption).
- author-pull-requests skill: don't leak iteration history into commit
subjects — branch commits feed code-review subagents and round-numbering
biases their read.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This bug was discovered while debugging hydration issues with
inpage-menuin docs.Per-item bindings inside each blocks silently lost reactivity to external state when items came from static props (how astro handles hydration).
Changes
Risk
6/10
This is the renderer hot path for hydration. Also currently 5-15% performance tax over baseline that needs to be confirmed necessary.
How to Test