Open
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
1 Skipped Deployment
|
⚪ No Meaningful Change for
|
| metric | Change |
|---|---|
add-20 |
-0.0% – +0.1% |
bulk-add-500 |
-0.6% – +0.9% |
clear-completed-250 |
-1.0% – +0.2% |
create-10k |
-0.4% – +0.7% |
create-1k |
-0.5% – +1.3% |
edit-cycle-5 |
-0.7% – +0.1% |
edit-start-10 |
-1.0% – +1.1% |
filter-cycle-20 |
-0.6% – +0.5% |
hydrate-each-100 |
-0.9% – +0.8% |
hydrate-each-100-mount |
-0.5% – +1.1% |
hydrate-helper-100-mount |
-1.0% – +0.3% |
remove-10-middle |
-0.8% – +0.4% |
remove-5-front |
-1.3% – +1.3% |
remove-first-10 |
-0.3% – +0.6% |
remove-middle-10 |
-0.4% – +0.7% |
remove-row-back-10 |
-0.6% – +0.3% |
remove-row-front-20 |
-0.7% – +0.7% |
remove-row-middle-20 |
-1.5% – +0.3% |
replace-1k |
-0.4% – +1.4% |
select-40 |
-0.3% – +0.7% |
swap-rows-20 |
-0.7% – +0.5% |
toggle-10 |
-0.2% – +0.1% |
toggle-all-20 |
-0.6% – +0.7% |
toggle-first-10 |
-0.4% – +1.2% |
toggle-last-10 |
-0.2% – +0.3% |
toggle-middle-10 |
-1.4% – +1.0% |
update-10th-10 |
-0.8% – +1.3% |
🔍 Unsure (5)
Inconclusive (4)
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 |
-3.0% – +2.8% | ±1% |
clear-10k |
-4.9% – +1.7% | ±1% |
remove-5-back |
-2.5% – +1.9% | ±2% |
remove-last-10 |
-0.7% – +2.5% | ±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 |
|---|---|---|---|
hydrate-helper-100-state-change |
-10.6% – +12.7% | ~5ms | ±32% |
Sample size: 50 · Resolution floor: ±2% · Timeout: 3min · Wall-clock: 16m16s
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.
Member
Author
Code reviewFound 1 issue:
Public entry point (missing the check): Inner handler (correct shape, for reference): Suggested fix at line 218: Generated with Claude Code If this code review was useful, please react with thumbs up. Otherwise, react with thumbs down. |
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.
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.
Each blocks deferred per-item Reaction wiring on hydrate, assuming bindings only read iteration-local data; bindings reading external state silently lost reactivity. Now those bindings hydrate eagerly.
Changes
Risk
6/10 — renderer hot path; gates the 425ms 1000-item hydrate budget.
Failure modes:
each.hydrateis the only hydrate hook opting out ofskipFirstWrite: true. An independent reviewer proposed reverting to the canonical pattern with an opt-instaticflag. The classifier keeps the perf win at the cost of architectural inconsistency.How to Test
hydrate-each-100-mount(item-only, expect WIN/TIED) andhydrate-helper-100-mount(helper-call, expect REGRESSED but correct; main's "fast" is no-op silent failure)