fix(tanstack-query): non-zenstack queries are accidentally visited during optimistic update #1775
Conversation
…ring optimistic update Fixes #1774
📝 WalkthroughWalkthroughThe changes in this pull request enhance the functionality of optimistic updates and query invalidation in the Tanstack Query library. A new function, Changes
Assessment against linked issues
Possibly related PRs
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 (1)
packages/plugins/tanstack-query/tests/react-hooks-v5.test.tsx (1)
867-930: LGTM: New test case for mixed zenstack and non-zenstack queriesThe new test case "optimistic update mixed with non-zenstack queries" is a valuable addition. It correctly tests the behavior of optimistic updates in the presence of both zenstack and non-zenstack queries.
However, consider the following suggestions to enhance the test:
After the optimistic update, verify that the non-zenstack query data remains unchanged. This would ensure that the optimistic update doesn't affect unrelated queries.
Consider adding a comment explaining the purpose of this test case, highlighting the importance of isolating zenstack and non-zenstack query behaviors.
Here's a suggested addition to the test case:
// After line 929, add: // Verify that the non-zenstack query remains unaffected expect(myQueryResult.current.data).toEqual({ data: 'myData' }); // Consider adding this comment at the beginning of the test case (line 867): // This test ensures that optimistic updates for zenstack queries // do not interfere with non-zenstack queries
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- packages/plugins/tanstack-query/src/runtime/common.ts (2 hunks)
- packages/plugins/tanstack-query/tests/react-hooks-v5.test.tsx (2 hunks)
🧰 Additional context used
🔇 Additional comments (4)
packages/plugins/tanstack-query/tests/react-hooks-v5.test.tsx (1)
6-6: LGTM: New import foruseQueryThe addition of
useQueryimport from '@tanstack/react-query-v5' is correct and necessary for the new test case.packages/plugins/tanstack-query/src/runtime/common.ts (3)
388-391: Correctly Skipping Non-ZenStack QueriesThe addition of the
isZenStackQueryKeycheck ensures that non-ZenStack queries are appropriately skipped during optimistic updates. This prevents unintended side effects on unrelated queries.
Line range hint
400-406: Respecting Query's Optimistic Update ConfigurationThe check for the
optimisticUpdateflag inqueryOptionscorrectly skips queries that have opted out of optimistic updates. This honors the query's configuration and enhances control over the optimistic update behavior.
459-469: Implementation ofisZenStackQueryKeyFunctionThe
isZenStackQueryKeyfunction accurately determines whether a query key corresponds to a ZenStack query by checking its length and prefix. This effectively filters out non-ZenStack queries during the optimistic update process.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
tests/integration/tests/cli/plugins.test.ts (1)
Line range hint
1-391: Summary and Next StepsThe update to the Tanstack Query version is a good step towards addressing the optimistic update issue mentioned in the PR objectives. The extensive test suite in this file provides robust coverage for various plugin scenarios, which is crucial for maintaining system integrity.
To fully address the PR objectives:
- Ensure that the optimistic update functionality is specifically tested, either in this file or in a separate test file dedicated to Tanstack Query behavior.
- Consider adding a test case that reproduces the issue mentioned in Tanstack-query optimistic update occasionally fails silently #1774 (Tanstack-query optimistic update occasionally fails silently) to prevent regression.
- If the optimistic update fix requires changes to the plugin implementation, make sure to update the relevant plugin tests in this file.
These steps will help ensure that the PR fully addresses the reported issue while maintaining the overall integrity of the plugin system.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (2)
packages/plugins/tanstack-query/package.jsonis excluded by!**/*.jsonpnpm-lock.yamlis excluded by!**/pnpm-lock.yaml,!**/*.yaml
📒 Files selected for processing (2)
- packages/plugins/tanstack-query/tests/plugin.test.ts (1 hunks)
- tests/integration/tests/cli/plugins.test.ts (1 hunks)
🧰 Additional context used
🔇 Additional comments (3)
tests/integration/tests/cli/plugins.test.ts (2)
Line range hint
1-391: Verify test suite integrity after version update.While the change to the Tanstack Query version is localized, it's crucial to ensure that it doesn't inadvertently affect other parts of the test suite.
Please run the entire test suite to confirm that all tests still pass with the updated Tanstack Query version. You can use the following command:
This will help catch any potential regressions caused by the version update.
76-76: Approve version update with a compatibility check suggestion.The update to
@tanstack/react-query@5.56.xis appropriate for pinning the major version while allowing minor updates. This change aligns with the PR objective of addressing issues with Tanstack-query.To ensure this change doesn't introduce any compatibility issues, please run the following command to check for any breaking changes or deprecations in the Tanstack Query changelog between the previous version and 5.56.x:
This will help identify any potential issues that might affect the optimistic update functionality or other areas of the codebase.
packages/plugins/tanstack-query/tests/plugin.test.ts (1)
Line range hint
1-359: Verify test suite integrity after @tanstack/react-query version update.The change to the @tanstack/react-query version in the 'react-query run plugin v5' test case is the only modification in this file. While this change is isolated, it's crucial to ensure that it doesn't inadvertently affect other tests or the overall behavior of the plugin.
To maintain the integrity of the test suite:
- Run the entire test suite to confirm that all tests still pass with the updated dependency version.
- Pay special attention to any tests related to optimistic updates, as mentioned in the PR objectives.
- Consider adding specific tests for the optimistic update scenario that this PR aims to fix.
Please run the following script to execute the test suite and check for any failures or unexpected behaviors:
#!/bin/bash # Description: Run the test suite and highlight any failures or warnings # Run the test suite npm test -- packages/plugins/tanstack-query/tests/plugin.test.ts > test_output.txt 2>&1 # Check for any test failures if grep -q "FAIL" test_output.txt; then echo "Test failures detected:" grep -A 5 "FAIL" test_output.txt else echo "All tests passed successfully." fi # Check for any warnings if grep -q "Warning" test_output.txt; then echo "Warnings detected:" grep -A 2 "Warning" test_output.txt fi # Clean up rm test_output.txtThis script will help ensure that the version update hasn't introduced any regressions in the test suite.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
tests/integration/tests/enhancements/with-delegate/plugin-interaction.test.ts (2)
22-22: Approve the version update with a minor suggestion.The update to '@tanstack/react-query@5.56.x' is a good change as it helps ensure consistency in testing environments. This aligns well with the PR objective of addressing issues with Tanstack-query optimistic updates.
Consider adding a comment explaining why this specific version was chosen. For example:
extraDependencies: [ '@tanstack/react-query@5.56.x', // Specific version to reproduce and fix optimistic update issue ],This would provide context for future maintainers about the reasoning behind this version constraint.
Line range hint
1-156: Consider reviewing other test cases for version consistency.While the update to the Tanstack Query test case is appropriate, it might be beneficial to review the other test cases (swr, trpc, zod) to ensure they are also using specific versions of their respective dependencies. This could help maintain consistency across all integration tests and potentially prevent future issues related to dependency versioning.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- tests/integration/tests/enhancements/with-delegate/plugin-interaction.test.ts (1 hunks)
🧰 Additional context used
Fixes #1774