fix: optimistic updates for nested queries should work then optional relationships have null values #1884
Conversation
📝 WalkthroughWalkthroughThe pull request introduces modifications to the Tanstack Query React Hooks V5 test suite, enhancing test coverage for optimistic updates and nested relationships. It defines a new Changes
Possibly related issues
Possibly related PRs
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (5)
packages/plugins/tanstack-query/tests/test-model-meta.ts (1)
57-73: LGTM! Consider removing redundant isOptional flag.The relationship between Post and Category is well-defined with proper foreign key mapping and backlink. The optional nature of the relationship aligns with the PR objectives.
The
isOptional: trueon line 62 is redundant since it's already set infieldDefaults. Consider removing it for consistency:category: { ...fieldDefaults, type: 'Category', name: 'category', isDataModel: true, - isOptional: true, isRelationOwner: true, backLink: 'posts', foreignKeyMapping: { id: 'categoryId' }, },packages/runtime/src/cross/mutator.ts (1)
168-168: Consider optimizing object updatesThe changes correctly handle nested field updates and prevent invalid assignments. However, there's room for optimization in how objects are cloned.
Consider batching multiple field updates before creating a new object to reduce object spread operations:
- if (r && typeof r === 'object') { - resultData = { ...resultData, [key]: r }; - updated = true; - } + if (r && typeof r === 'object') { + if (!updated) { + resultData = { ...resultData }; + updated = true; + } + resultData[key] = r; + }This optimization would prevent creating a new object for each field update while maintaining immutability.
Also applies to: 184-187
packages/plugins/tanstack-query/tests/react-hooks-v5.test.tsx (3)
390-390: Fix typographical errors in comments
- Line 390: The comment has a typo:
'pupulate'should be'populate'.- Line 504: The word
'relatonship'should be'relationship'.Also applies to: 504-504
495-496: Address the TODO regarding category inclusionIn lines 495-496, there's a TODO comment questioning whether the
categoryobject should be included in the post's data. Consider updating the assertion to include thecategoryobject if it's part of the expected behavior.Would you like assistance in updating the test assertion to include the
categoryobject?
355-356: Consider using explicit TypeScript interfaces instead ofanyDefining specific interfaces for
userData,categoryData, andpostDatacan improve type safety and code readability in your tests.Also applies to: 391-392, 505-506, 597-598
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
packages/plugins/tanstack-query/tests/react-hooks-v5.test.tsx(3 hunks)packages/plugins/tanstack-query/tests/test-model-meta.ts(1 hunks)packages/runtime/src/cross/mutator.ts(3 hunks)
🔇 Additional comments (8)
packages/plugins/tanstack-query/tests/test-model-meta.ts (1)
77-96: LGTM! Consider adding deleteCascade rule for Category.
The Category model is well-structured with proper bidirectional relationship to Post.
Let's verify if we need a deleteCascade rule for Category:
packages/runtime/src/cross/mutator.ts (2)
Line range hint 154-163: LGTM: Improved type safety for array mutations
The added type check ensures that only valid object values are used when updating array items, preventing potential issues with null values in optional relationships.
163-166: LGTM: Essential fixes for data integrity
Two important improvements:
- The null check properly handles optional relationships
- Cloning prevents mutations of the original data during iteration, which could lead to bugs
This change directly addresses the PR's objective of handling null values in optional relationships.
packages/plugins/tanstack-query/tests/react-hooks-v5.test.tsx (5)
14-15: Good use of a BASE_URL constant
Defining BASE_URL enhances maintainability by avoiding hardcoded URLs and making the base URL easily configurable.
30-30: Updated makeUrl function to use BASE_URL
The makeUrl function now utilizes the BASE_URL constant, improving consistency and maintainability.
350-499: Added test for optimistic create with deeply nested relationships
The new test optimistic create updating deeply nested query effectively tests the optimistic update functionality when creating a post connected to both a user and a category. The test correctly populates the cache and asserts that the optimistic updates are reflected in deeply nested queries.
501-591: Added test for optimistic update with optional one-to-many relationship
The test optimistic update with optional one-to-many relationship correctly verifies that optimistic updates work as expected when updating a post with a nullable category relationship. The implementation accurately reflects the intended functionality.
593-692: Added test for optimistic update with nested optional relationships
The test optimistic update with nested optional one-to-many relationship effectively ensures that optimistic updates propagate correctly through nested data structures, even when optional relationships have null values.
This PR addresses the issue raised in #1878. It adds guards to
doApplyMutationto ensure thatresultDatais an array or an object before it is processed.I had a bit of trouble getting nock to play nicely in the test suite and needed to be more explicit about which endpoints were being intercepted. I added a constant for
BASE_URLand added that intomakeUrltoo, for consistency. I hope that's okay?Either way, the tests are passing and I hope this is a useful contribution to your epic project! 🚀