fix(signals,solid): preserve shallow leaves and sequence hybrid hydration

[Monkeylordz]

Summary

Fixes #3498.

Computed shallow stores currently proxy raw leaves and JSON-clone loading/SSR/hydration snapshots. This changes identity and values (for example, a Date becomes a string), and SSR can throw a Proxy invariant error on a frozen leaf. Hybrid store hydration can also discard a delayed first server answer when a loading seed is present, suppress the first yield of later computations, or replace a server error at hydration completion.

This change:

• Selects shallow draft wrapping once and copies only the root container for shallow loading and SSR snapshots.
• Preserves shallow leaves in hydration replay shadows.
• Waits for both hydration completion and the first server answer before hybrid store takeover. Only the initial handoff suppresses a duplicate first yield; later computations commit normally. A rejected server answer stays visible until refresh, which can start a fresh client computation.
• Adds standalone regressions for object/array roots, ordinary/optimistic stores, loading, SSR, hybrid handoff, later updates, and error recovery. The tree-shaking test normalizes Windows path separators when comparing source and built module names.

Bundle-size cost — needs additional inspection

This correctness fix increases retained bundle size. @ryansolid, please review the cost and the hybrid handoff semantics before merge; passing the adjusted gates alone should not be treated as approval of the increase.

Measured against upstream next at 59c4192a, using the same Rollup/property-mangled artifacts and the pinned scripts/size tooling on Windows/Node 24.18.0:

Scenario	Upstream	This PR	Increase	Proposed cap change
Signals + createStore	16,366 B	16,470 B	+104 B	16.45 → 16.47 KB
Hydrating app without stores	19,782 B	19,853 B	+71 B	19.80 → 19.86 KB
Hydrating app with every store family	29,854 B	29,952 B	+98 B	29.90 → 29.96 KB
CSR app	15,140 B	15,180 B	+40 B	15.15 → 15.18 KB

These are compressed artifact sizes from size-limit (Brotli), not minified or gzip byte counts. The other six scenarios are unchanged, including the 9,303 B signal-only floor. The patch proposes only the minimum rounded caps needed for these local measurements; CI/platform differences and whether a smaller implementation is preferable deserve inspection.

A separate isolated source-bundle comparison finds byte-identical signal-only output and gzip increases of 35 B for stores, 45 B for optimistic stores, 124 B for hydration, and 51 B for the server. Local alternating benchmarks measured ordinary store write/read at 0.673 → 0.663 µs and deep projection recompute/read at 1.287 → 1.305 µs. These small local timing differences are not a substitute for the project's performance checks.

How did you test this change?

• Built signals in all three tiers, ran the shared property-mangling pass for prod/observe, and built the Solid and web artifacts. The pure-annotation guard passes.
• Ran the 20 new regression cases against unmodified upstream sources: 18 fail, 2 pass. All 20 pass with this fix.
• packages/solid: node ../../node_modules/vitest/vitest.mjs run — 632 passed.
• packages/signals: the same command — 2,744 passed, 2 expected failures, 1 skipped, with 3 failures in rules-index.test.ts. Those documentation-index failures also reproduce with the upstream projection source restored in this Windows checkout; they report unresolved documentation citations. The full signals run is therefore not entirely green locally.
• Signals and Solid tsconfig.build.json checks pass. Solid tsconfig.test.json and tsconfig.no-dom.json checks pass.
• scripts/size: node node_modules/size-limit/bin.js --json — all 10 scenarios pass with the proposed caps; four fail with the unchanged upstream caps.
• Prettier and git diff --check pass.

CodSpeed investigation (current result remains unresolved)

The bot reports one regressed benchmark and 174 unchanged benchmarks. The affected case is projection derive: delete + set one ROOT key (#3352) in projection-root-write.bench.ts: 1.3493 ms → 1.7177 ms, a 21.45% efficiency loss (about 27.3% more simulated time).

I inspected the public comparison metadata and downloaded both call graphs. The environment warning is a real CPU change: the baseline ran on an Intel Xeon Platinum 8573C; the PR ran on an AMD EPYC 9V74. Both workflow logs show Node 24.20.0 and CodSpeed simulation instruments 4.19.1.

The reported self CPU cost for runProjectionComputedNext grows from about 13.5 µs to 371.8 µs, while the projection draft getter falls from about 163.0 µs to 16.5 µs. Costs also shift in unchanged flush and ensurePB paths. This is compatible with runtime compilation/allocation or cache effects being attributed to the active functions, but does not establish that the regression is harmless.

Four local runs of the exact benchmark, in baseline/PR/PR/baseline order with matching dev/observe flags and test instrumentation disabled, measured root-write batches at 73.3/68.5/67.1/68.6 µs respectively. These are Windows/Node 24.18.0 wall-clock measurements, not a reproduction of Linux CodSpeed simulation; they do not clear the failed performance check.

A same-runtime/CPU baseline-versus-head rerun is needed before accepting the performance result or attributing it to the patch. Attempting to rerun the unchanged GitHub benchmark job was denied because this account lacks repository permissions. No performance regression has been acknowledged or waived, and the benchmark or threshold has not been changed.

[changeset-bot]

🦋 Changeset detected

Latest commit: c4ba6c7

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 11 packages

Name	Type
@solidjs/signals	Patch
solid-js	Patch
test-integration	Patch
@solidjs/web	Patch
@solidjs/element	Patch
@solidjs/h	Patch
@solidjs/html	Patch
@solidjs/universal	Patch
@solidjs/babel-plugin	Patch
@solidjs/compiler	Patch
@solidjs/diagnostics	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

[codspeed]

Merging this PR will degrade performance by 21.45%

⚠️ Different runtime environments detected

Some benchmarks with significant performance changes were compared across different runtime environments,
which may affect the accuracy of the results.

Open the report in CodSpeed to investigate

❌ 1 regressed benchmark
✅ 174 untouched benchmarks

Warning

Please fix the performance issues or acknowledge them on CodSpeed.

Performance Changes

	Benchmark	BASE	HEAD	Efficiency
❌	projection derive: delete + set one ROOT key (#3352)	1.3 ms	1.7 ms	-21.45%

Tip

Investigate this regression by commenting @codspeedbot fix this regression on this PR, or directly use the CodSpeed MCP with your agent.

---

_{Comparing Monkeylordz:codex/fix-shallow-store-hydration (c4ba6c7) with next (59c4192)}

[brenelz]

Review

Intent: Fix #3498. Shallow computed stores were deep-proxying leaves and JSON.parse(JSON.stringify(...))-cloning shadows (Date→string, NaN→null, frozen Proxy invariant on SSR). Hybrid hydration could drop a delayed first server answer when a loading seed was present, mute later first yields, or overwrite a server error at hydration end.

What changed (correct shape):

• wrapDraft(..., wrap) — shallow picks Object once so nested leaves stay raw; deep still recurses.
• Shared cloneProjection (client projection + SSR) — shallow root slice/{...}, deep still JSON.
• SSR createDeepProxy(..., shallow) skips child proxies when shallow.
• Hydration shadows honor options.shallow.
• Hybrid store path leaves withHydrationGate: peek serialized id, wait for hydration end ∧ first server answer before setHydrated(true); live only after initial handoff; reject sets live without flipping hydrated so the error sticks until refresh.

Verdict: Correctness direction looks good and well regression-tested. Don’t merge until CodSpeed is resolved (or explicitly waived) and Ryan signs the size/hybrid semantics. Author already flags both — agree.

Blockers / merge gates

1. CodSpeed red (−21.45% on projection derive: delete + set one ROOT key). Different CPUs (Xeon vs EPYC) + call-graph cost flipping between draft getter and runProjectionComputedNext smells like attribution/noise, and local wall-clock doesn’t clear it. Need a same-runtime baseline-vs-head rerun or a maintainer ACK — not a quiet merge.
2. Bundle caps up on four scenarios (store +104 B brotli, hydrating-no-stores +71 B, etc.). Signal-only floor flat is good; hydrating without stores still grew — worth a 30-second “why is that in the non-store chunk?” pass before treating the ratchet as free. Author correctly pinged @ryansolid.

Should confirm

• Hybrid reject path: ready.then(..., () => { live = true }) never setHydrated(true). Matches the “error until refresh” test; just confirm no other wake (navigation / dependency) can re-enter if (live) return fn(draft) and accidentally clear a still-visible server error without an explicit refresh.
• peekNextChildId before coreFn: same pattern as other hydrate helpers — OK if id allocation order is stable; any future owner-id change upstream will break hybrid arming the same way as those paths.

Nits

• cloneProjection duplicated in projection.ts and server/signals.ts — fine for layering, or one shared util later.
• Deep path still JSON-clones (Dates etc.) — out of scope for Shallow computed stores lose leaf identity; hybrid hydration can lose the first answer #3498; don’t let reviewers re-litigate it here.
• Windows rules-index.test.ts noise on the author’s machine; CI job is green — ignore unless CI flakes.
• Treeshake path separator fix is a welcome drive-by.

What’s strong

• 20 new cases with “18 fail on next, 20 pass here” is the right evidence bar.
• Client loading + SSR shallow + hybrid handoff/later-yield/reject covered for store and optimistic, object and array roots.
• PR writeup is unusually honest about size and CodSpeed — keep that standard.

[ryansolid]

Thanks for this — the shallow analysis was exactly right, and the tests were the evidence that made it easy to act on. Sorry it sat while next moved so much underneath it.

We split it. The shallow half landed as #3533 with you as co-author: your wrapDraft / createDeepProxy / shadow changes and your tests (minus the hybrid describes), reduced a little — a shallow boolean short-circuits the draft recursion instead of the wrap function parameter, and the clone helper is mirrored in the server runtime rather than exported. It is a strict narrowing, so it needed no semantic call, and it measures as a perf win for shallow stores (no per-read proxy allocation in the draft).

The hybrid hydration half stays here. Waiting for hydration end ∧ first server answer before takeover, committing later runs' first yields, and keeping a rejected server answer visible until refresh are rules about what a hybrid store shows during handoff, not just bug fixes — Ryan needs to rule on those before this can be reviewed as a fix, and your tests for that path are the right pins if it's ratified. Of the three, the first-yield suppression on later runs looks like a plain bug and may be able to go on its own; the other two are the design questions. Leaving this open for that.

The Windows path normalization in treeshake.test.ts is a welcome standalone — happy to take it as its own tiny PR.

— Claude via Cursor

[ryansolid]

Thanks for this — the diagnosis in #3498 was right, and your pins for the lost server answer (item 1) and the rejection-then-refresh() path (item 3) were the correct tests. We've adapted them, with credit, into #3551, which supersedes this PR, so closing here.

Where we landed differently is the gate. Waiting on onHydrationEnd couples the store to the whole page's hydration, which is the wrong thing to wait for: hydration end isn't a fact about this store's answer, and it holds the handoff open longer than the data needs. The ruling was:

1. The handoff waits for the first server answer to land — synchronous for a settled value (as today), on landing for a pending loadingValue placeholder. Never on hydration end; nothing holds hydration.
2. Only the handoff run's first yield is the duplicate. Later runs (dependency change, refresh()) commit theirs — that was a real bug alongside the lost answer.
3. A rejected server answer is the adopted answer: the error stands until refresh().
4. A dependency change before the pending answer lands supersedes it, like any new pending change — the client run takes over and the server's late landing is dropped.

Mechanically, #3551 hands the pending thenable to the engine as a one-yield stream, and the pull after that step commits is the "landed" signal, so the gate flips exactly when the answer has been applied — no counting, no timers.

Appreciate you working through this one; it exposed two bugs we'd have shipped otherwise.

— Claude via Cursor