fix(hooks): support optimistic update for "upsert" #1767
Conversation
📝 Walkthrough📝 WalkthroughWalkthroughThe changes in this pull request introduce a new Jest configuration file for the TanStack Query plugin, establishing various testing parameters. Additionally, new test cases are added to the React Hooks testing suite to validate optimistic updates and upserts. Furthermore, a new Changes
Assessment against linked issues
Possibly related PRs
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
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: 3
🧹 Outside diff range and nitpick comments (4)
jest.config.ts (1)
36-36: LGTM! Consider adding a comment for clarity.The addition of
testEnvironment: 'jsdom'is appropriate for testing React hooks and components that interact with the DOM. This aligns well with the PR's focus on enhancing hook behavior, particularly for optimistic updates in upsert operations.Consider adding a brief comment explaining the reason for using JSDOM, like this:
+ // Use JSDOM for testing React hooks and components that interact with the DOM testEnvironment: 'jsdom',This will help future contributors understand the rationale behind this configuration choice.
packages/plugins/tanstack-query/tests/react-hooks-v5.test.tsx (1)
535-585: LGTM with a minor suggestion: Optimistic upsert - update test caseThis test case effectively verifies the optimistic update behavior for an upsert operation that updates an existing record. The test structure, setup, and assertions are well-implemented.
One minor suggestion for improvement:
In the mutation call (line 577), thecreateobject uses a different name ('zee') than theupdateobject ('bar'). For consistency and to better reflect a real-world upsert scenario, consider using the same name in both objects:- act(() => mutationResult.current.mutate({ ...queryArgs, update: { name: 'bar' }, create: { name: 'zee' } })); + act(() => mutationResult.current.mutate({ ...queryArgs, update: { name: 'bar' }, create: { name: 'bar' } }));This change would make the test case more realistic, as typically in an upsert operation, you'd want to set the same data for both the update and create scenarios.
packages/plugins/tanstack-query/tests/react-hooks.test.tsx (2)
431-433: Remove unnecessaryundefinedarguments inuseModelMutationcallsIn the
useModelMutationcalls, the last argument is explicitly set toundefined, which is unnecessary. Omitting it enhances code readability without affecting functionality.Update the code to remove the
undefinedarguments:useModelMutation( 'User', 'POST', makeUrl('User', 'upsert'), modelMeta, { optimisticUpdate: true, invalidateQueries: false }, - undefined ),Also applies to: 494-496
420-422: Clarify console log messages in mock responsesThe console log message
'Not mutating data'may be misleading, as the test simulates an 'upsert' operation. Consider updating the log messages to accurately reflect the simulated operation.Update the console logs for clarity:
- console.log('Not mutating data'); + console.log('Simulating upsert operation without server-side mutation');Also applies to: 482-485
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
- jest.config.ts (1 hunks)
- packages/plugins/tanstack-query/tests/react-hooks-v5.test.tsx (2 hunks)
- packages/plugins/tanstack-query/tests/react-hooks.test.tsx (2 hunks)
- packages/runtime/src/cross/mutator.ts (1 hunks)
🧰 Additional context used
🔇 Additional comments (4)
packages/plugins/tanstack-query/tests/react-hooks-v5.test.tsx (2)
475-533: LGTM: Comprehensive test for optimistic upsert - create scenarioThis test case effectively verifies the optimistic update behavior for an upsert operation that creates a new record. The test setup, execution, and assertions are well-structured and cover the expected behavior.
A few observations:
- The test correctly mocks the initial empty state and the upsert API call.
- It uses the
useModelMutationhook with appropriate options for optimistic updates.- The assertions verify that the cache is updated optimistically with the new user data.
475-585: Excellent addition: Comprehensive coverage for optimistic upsert operationsThe introduction of these two test cases for optimistic upsert operations significantly enhances the test suite's coverage. By testing both the create and update scenarios, these additions ensure that the optimistic update functionality for upsert operations is thoroughly validated.
These test cases:
- Align perfectly with the PR objectives of supporting optimistic updates for upsert hooks.
- Follow the established testing patterns, maintaining consistency within the test suite.
- Cover edge cases and verify the correct behavior of the cache updates.
This improvement in test coverage will help maintain the reliability and correctness of the optimistic update feature for upsert operations.
packages/plugins/tanstack-query/tests/react-hooks.test.tsx (2)
441-444: Verify optimistic upsert handles both create and update pathsEnsure that the optimistic upsert operation correctly handles both creating a new record and updating an existing one. Currently, the test focuses on creation; consider adding assertions to cover the update scenario as well.
Add assertions to confirm the cache state after an update operation:
// After mutation expect(cacheData[0].name).toBe('bar'); // Verify update path
502-505: Ensure cache reflects updated data after optimistic upsertIn the 'optimistic upsert - update' test, verify that the cache correctly reflects the updated data (
name: 'bar') after the optimistic upsert operation.Confirm the cache data matches the expected updated values:
expect(cacheData).toMatchObject({ name: 'bar', $optimistic: true });
There was a problem hiding this comment.
Caution
Inline review comments failed to post
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
packages/plugins/tanstack-query/jest.config.ts (2)
12-14: LGTM: Proper setup of global and test environment scripts.The use of
path.join()with__dirnameensures cross-platform compatibility, which is a good practice. The global setup and environment setup scripts are correctly referenced.Consider extracting the common path segment
'../../../script/'into a variable for better maintainability:const scriptDir = path.join(__dirname, '../../../script'); export default { // ... globalSetup: path.join(scriptDir, 'test-global-setup.ts'), setupFiles: [path.join(scriptDir, 'set-test-env.ts')], // ... };
16-29: LGTM: Comprehensive coverage configuration.The coverage settings are well-configured:
- Enabling coverage collection is crucial for maintaining code quality.
- The coverage directory is appropriately set.
- Ignoring
node_modulesandtestsdirectories is standard practice.- The 'v8' coverage provider is suitable for Node.js environments.
- The selection of coverage reporters provides a good mix of formats.
Consider adding a
coverageThresholdsetting to enforce minimum coverage requirements:coverageThreshold: { global: { branches: 80, functions: 80, lines: 80, statements: 80 } },This helps maintain a high standard of test coverage as the project evolves.
🛑 Comments failed to post (1)
packages/plugins/tanstack-query/jest.config.ts (1)
34-34: 💡 Codebase verification
Reduce
testTimeoutto Enhance Test PerformanceThe current
testTimeoutof 300,000 milliseconds (5 minutes) is significantly long for individual tests. Analysis reveals that some tests inprisma-pulse.test.tsutilizesetTimeoutwith durations up to 10,000 milliseconds (10 seconds). While these delays may be necessary for certain operations, a 5-minute timeout could mask performance issues or lead to unnecessarily prolonged test runs.Consider the following actions:
- Lower the
testTimeoutto a more reasonable value, such as 60,000 milliseconds (1 minute), unless a longer duration is explicitly justified.- Refactor tests to minimize the use of fixed delays where possible, potentially leveraging async utilities or mocking timers.
- Separate long-running tests into a distinct test suite with adjusted timeout settings to maintain overall test efficiency.
🔗 Analysis chain
Consider reducing the test timeout.
The current
testTimeoutof 300,000 milliseconds (5 minutes) is quite long for individual tests. While this might be necessary for complex integration tests or tests involving external services, it could potentially hide performance issues or infinite loops in tests.Consider reducing this timeout unless there's a specific reason for such a long duration. If long-running tests are necessary, it might be beneficial to separate them into a different test suite with a longer timeout.
To help identify potentially slow tests, you can run the following command:
This command searches for test files that might contain long-running asynchronous tests or explicitly set timeouts.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Find test files with potentially long-running tests rg --type typescript -g '*.test.ts' -g '*.spec.ts' -i '(timeout|done.*\).*=>)' -C 2Length of output: 148
Script:
#!/bin/bash # Find test files with potentially long-running tests without using --type rg -g '*.test.ts' -g '*.spec.ts' -i '(timeout|done.*\).*=>)' -C 2Length of output: 2957
Fixes #1750