-
-
Notifications
You must be signed in to change notification settings - Fork 91
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
refactor: correct minor mistakes picked up by coderabbitai in nitpick comments #1887
Conversation
📝 WalkthroughWalkthroughThe changes in this pull request enhance the testing coverage for the Tanstack Query React Hooks V5, focusing on various query and mutation scenarios. New tests have been added for optimistic updates and nested relationships, ensuring correct cache behavior after mutations. Additionally, modifications to the Changes
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 (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
packages/plugins/tanstack-query/tests/react-hooks-v5.test.tsx (1)
Line range hint
390-503
: Consider enhancing the test coverage for deeply nested relationships.The test case effectively validates optimistic updates for deeply nested queries. However, there's an open TODO comment about including the category object in optimistic updates that should be addressed.
Consider these improvements:
- Add assertions to verify the complete object structure including the category relationship
- Test the rollback scenario when the mutation fails
- Add test cases for concurrent updates to the same nested relationship
Example assertion to include category object:
expect(posts[0]).toMatchObject({ $optimistic: true, id: expect.any(String), title: 'post1', ownerId: '1', categoryId: '1', + category: { + $optimistic: true, + id: '1', + name: 'category1' + } });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
packages/plugins/tanstack-query/tests/react-hooks-v5.test.tsx
(2 hunks)packages/plugins/tanstack-query/tests/test-model-meta.ts
(0 hunks)
💤 Files with no reviewable changes (1)
- packages/plugins/tanstack-query/tests/test-model-meta.ts
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the improvements. I merged the last one too fast haha
@ymc9 very minor changes based on feedback from coderabbitai, based on my previous PR. Sorry I didn't catch these before you merged.