Repository navigation
fix(signals): an optimistic view reads a property its inner store removed as undefined (#3883) - #3885
Merged
Merged
Conversation
…oved as undefined (#3883) A chained store (an optimistic store over a store) served an absent key from the outer node's cached value instead of reading through to the inner store, so a property reconciliation removed kept its last value. Absent keys now take the same read-through path as present ones: the outer node is a subscription point unless it carries a guess. Co-authored-by: GabbeV <gabriel.valfridsson@gmail.com> Co-authored-by: Cursor <cursoragent@cursor.com>
🦋 Changeset detectedLatest commit: a609f2c The changes in this PR will be included in the next version bump. This PR includes changesets to release 12 packages
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 |
Size (brotli, eager entry chunk)
|
Coverage Report for CI Build 37705969691Coverage remained the same at 76.43%Details
Uncovered ChangesNo uncovered changes found. Coverage RegressionsNo coverage regressions found. Coverage Stats
💛 - Coveralls |
Merging this PR will improve performance by 6.99%
|
| Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|
| ⚡ | memo + sync render effect only (reference) |
30.1 ms | 28.1 ms | +6.99% |
Tip
Curious why performance improved? Comment @codspeedbot explain why performance improved on this PR, or directly use the CodSpeed MCP with your agent.
Comparing fix/3883-optimistic-removed-prop (a609f2c) with next (3086f1b)
Co-authored-by: Cursor <cursoragent@cursor.com>
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
2 participants
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.
Fixes #3883
Thanks to @GabbeV for bisecting this to the store rewrite in #3774 and for the fix, which is taken as-is (credited with a
Co-authored-bytrailer), plus tests.Root cause
In a chained store (an optimistic store over a store), the outer store's per-key node is a subscription point; the inner store is the truth unless the outer node carries an optimistic guess (§7b). Present keys follow that rule through
serveDataKey. The absent-key branch of thegettrap did not: it returned the outer node's cached value directly. When reconciliation removed a property from the inner store, the view kept serving the last cached value (createFailed: truein the report).The fix routes an absent key on a chained target reading its committed backing (
target.ch && src === target.v) throughserveDataKey, the same read-through present keys take: the inner store answers, a guess on the outer node still wins, and the link's committed value is synced to the inner store's.Paths checked: optimistic overrides (a guess on the removed key shows during the action,
undefinedafter); derived/projection stores (same treatment as present keys); the #3767 alias / #3859resolveChainedRawpath (only for non-chained reads, not reached here); deleted-key tracking (checked earlier, against the outer staging only, unchanged); in-draft reads (skip the node and serve the inner value, as before).Tests
packages/signals/tests/store/optimistic-removed-prop-3883.test.ts:undefinedthrough the optimistic view (fails onnext)next)createFailed: true, settles, then reconcile removes it (fails onnext)in/Object.keysthrough the view (control)undefinedafterFull signals suite passes.
docs/RULES-INDEX.mdregenerated (one more §7b citation).Behavior note
An equal-value guess (
truewritten overtrue) is not held as an override. Before this fix, if the inner store then removed the key mid-action, the view kept showingtrue, during the action and after it (the bug). Now it readsundefinedduring the action. This matches present keys: the same equal guess followed by the inner store changing the value tofalsealready showsfalse.Size
Local measurement, base = this branch without the fix:
signals: + createStoreapp: hydrating + every store primitive familyapp: compiled CSRapp: compiled hydratingapp: render + one signalSize-Exception: maintainer accepted on 2026-10-07: +40 B minified for the #3883 fix, store scenarios only; hello world (
app: render + one signal) unchanged.Public API changes
None.