Fix change tracking for array items accessed via iteration methods (find, forEach, for...of, etc.) #910
+673
−250
Conversation
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
…n-demand Add comprehensive tests to verify that createOptimisticAction works correctly with syncMode "on-demand" collections. These tests were created while investigating a reported bug where mutationFn was not being called when using on-demand collections. Tests added: - Basic on-demand collection with insert - Collection not started (idle state) - Collection in loading state (not ready) - On-demand collection with live query filter - UPDATE operation on on-demand collection - Debug test verifying mutations array is populated All tests pass, suggesting the reported issue may have been fixed by recent scheduler bug fixes or is specific to a configuration not covered here.
These tests reproduce the bug where modifications to array items retrieved via iteration methods (find(), forEach, for...of) are not tracked by the change proxy. Root cause: When using array iteration methods like find(), the proxy returns raw array elements instead of proxied versions. This causes mutations to these elements to not be tracked, resulting in getChanges() returning an empty object. This directly causes the createOptimisticAction bug where mutationFn is never called when modifying nested array items via: draft.nested.array.find(...).property = value Failing tests: - find() returns unproxied items - forEach() iterates over unproxied items - for...of iterates over unproxied items Working test: - Direct index access [0] works because the proxy's get handler intercepts numeric index access and returns proxied items The fix requires handling array iteration methods similar to how Map/Set iteration is already handled in the proxy.
This fixes a bug where modifications to array items retrieved via iteration methods like find(), forEach(), and for...of were not being tracked by the change proxy. The root cause was that array iteration methods were returning raw array elements instead of proxied versions. When users modified these raw elements, the changes were not tracked, causing getChanges() to return an empty object. This directly caused the createOptimisticAction bug where mutationFn was never called when using patterns like: draft.nested.array.find(...).property = value The fix adds handling for array iteration methods similar to how Map/Set iteration is already handled: - find(), findLast(): Return proxied element when found - filter(): Return array of proxied elements - forEach(), map(), flatMap(), some(), every(): Pass proxied elements to callbacks so mutations are tracked - reduce(), reduceRight(): Pass proxied elements to reducer callback - Symbol.iterator (for...of): Return iterator that yields proxied elements Fixes the Discord-reported bug where syncMode "on-demand" collections with createOptimisticAction would have onMutate run but mutationFn never called.
- Remove "bug" references from proxy test comments - Remove on-demand syncMode tests from optimistic-action.test.ts that were always passing (the actual fix was in proxy.ts array iteration handling)
🦋 Changeset detectedLatest commit: d52f46b The changes in this PR will be included in the next version bump. This PR includes changesets to release 11 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 |
Cast through 'unknown' first when converting changeTracker.copy_ to Array<unknown> to satisfy TypeScript's type checking.
More templates
@tanstack/angular-db
@tanstack/db
@tanstack/db-ivm
@tanstack/electric-db-collection
@tanstack/offline-transactions
@tanstack/powersync-db-collection
@tanstack/query-db-collection
@tanstack/react-db
@tanstack/rxdb-db-collection
@tanstack/solid-db
@tanstack/svelte-db
@tanstack/trailbase-db-collection
@tanstack/vue-db
commit: |
Contributor
|
Size Change: +471 B (+0.54%) Total Size: 86.9 kB
ℹ️ View Unchanged
|
Contributor
|
Size Change: 0 B Total Size: 3.34 kB ℹ️ View Unchanged
|
- Hoist CALLBACK_ITERATION_METHODS Set to module scope for better perf - Extract isProxiableObject helper to reduce code duplication - Add tests for filter(), findLast(), some(), and reduce() change tracking
samwillis
approved these changes
Nov 26, 2025
Collaborator
samwillis
left a comment
samwillis
left a comment
There was a problem hiding this comment.
I think this looks good, the test pass (and look good) and validate the implementation.
One observation is that the main get method on the proxy is now humungous, we need to break it up into helpers.
Approving, but like good to pull out at least this branch as a helper.
packages/db/src/proxy.ts
Outdated
|
|
||
| // For Array methods that iterate with callbacks and may return elements | ||
| // These need to pass proxied elements to callbacks and return proxied results | ||
| if (CALLBACK_ITERATION_METHODS.has(methodName)) { |
Collaborator
There was a problem hiding this comment.
The parent get method that this branch is part of has got very big. With this change it's a 450 line function. Can we move this branch out into a helper function for handling iteration.
I think we should also consider further breaking up this function.
Collaborator
Author
There was a problem hiding this comment.
Yeah good time for a refactor yeah — Claude did it pretty and the get method is now ~100 lines of code
packages/db/tests/proxy.test.ts
Outdated
| expect(obj.date).toEqual(originalDate) | ||
| }) | ||
|
|
||
| // Test that array iteration methods return proxied elements for change tracking |
Collaborator
There was a problem hiding this comment.
Maybe we should group these tests in a describe
- Extract createArrayIterationHandler() for find/filter/forEach/etc. - Extract createArrayIteratorHandler() for Symbol.iterator - Reduces main get() handler by ~120 lines - Group array iteration tests in describe block per review feedback
- Hoist ARRAY_MODIFYING_METHODS Set to module scope - Hoist MAP_SET_MODIFYING_METHODS Set to module scope - Hoist MAP_SET_ITERATOR_METHODS Set to module scope - Extract createModifyingMethodHandler() for unified collection mutation wrapping - Extract createMapSetIteratorHandler() for Map/Set iteration (~200 lines) - Main get() handler reduced from ~450 lines to ~100 lines
KyleAMathews
deleted the
claude/fix-createOptimisticAction-01A3N3BTNq8jMtZdBkc2kCUg
branch
Merged
Contributor
|
🎉 This PR has been released! Thank you for your contribution! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Previously, modifications to array items retrieved via iteration methods were not tracked by the change proxy because these methods returned raw array elements instead of proxied versions. This caused
getChanges()to return an empty object, which in turn causedcreateOptimisticAction'smutationFnto never be called when using patterns like:The fix adds proxy handling for array iteration methods similar to how Map/Set iteration is already handled, ensuring that callbacks receive proxied elements and returned elements are properly proxied.