Uh oh!
There was an error while loading. Please reload this page.
fix(core): observing "promise" needs to implicitly observe "data" - #9772
Conversation
🦋 Changeset detectedLatest commit: e6fce97 The changes in this PR will be included in the next version bump. This PR includes changesets to release 19 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 |
WalkthroughAccessing a proxied query result's Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Component as React component
participant Proxy as QueryObserver Proxy
participant Observer as QueryObserver internals
participant Thenable as Query.thenable
Component->>Proxy: access `query.promise` (e.g. use(query.promise))
note right of Proxy #D6EAF8: proxy.get('promise') updated
Proxy->>Observer: track('data') ## explicit tracking added
alt thenable pending and !experimental_prefetchInRender
Observer->>Thenable: reject/throw pending thenable
note right of Thenable #FDEBD0: triggers Suspense / error path
else
Observer->>Thenable: return thenable (normal path)
note right of Thenable #E8F8F5: component can await/use data
end
Thenable-->>Component: resolved/rejected result
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
View your CI Pipeline Execution ↗ for commit e6fce97
☁️ Nx Cloud last updated this comment at |
Sizes for commit e6fce97:
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/react-query/src/__tests__/useQuery.promise.test.tsx (1)
1388-1433: Well-crafted test that validates the fix for issue #9727.The test correctly verifies that accessing
query.promiseviaReact.use()now implicitly tracksdata, ensuring thathasNextPage(which depends on the tracked data) reflects the correct value after suspending. The query setup, suspense handling, and assertions are all appropriate.Optional suggestions for enhanced clarity:
- Consider adding a brief inline comment before line 1405 explaining that this test specifically validates implicit data observation when using the promise (e.g.,
// Accessing promise should implicitly track data, ensuring hasNextPage is correctly computed).- For completeness, you could add a complementary test case that verifies
hasNextPage: falsewhengetNextPageParamreturnsnullorundefined.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/react-query/src/__tests__/useQuery.promise.test.tsx(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
packages/react-query/src/__tests__/useQuery.promise.test.tsx (2)
packages/query-core/src/queryObserver.ts (1)
query(704-721)packages/query-core/src/infiniteQueryBehavior.ts (1)
hasNextPage(162-168)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Preview
- GitHub Check: Test
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@## main #9772 +/- ##
===========================================
+ Coverage 45.75% 59.77% +14.02%
===========================================
Files 200 129 -71 Lines 8403 5619 -2784 Branches 1930 1537 -393 ===========================================
- Hits 3845 3359 -486 + Misses 4111 1955 -2156 + Partials 447 305 -142
🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/react-query/src/__tests__/useQuery.promise.test.tsx (2)
420-423: Brittle renderedComponents ordering assertionsAdjustments to expect(renderedComponents).toEqual([Page, MyComponent]) look correct functionally, but strict array equality on render order can be flaky across React versions/runtimes. Consider relaxing:
- Assert presence instead of exact order: expect(renderedComponents).toEqual(expect.arrayContaining([Page, MyComponent])) and assert length
- Or assert DOM output only (already done in most tests)
Also applies to: 488-491, 682-685, 1097-1100, 1111-1114
1287-1294: Make background-update assertion deterministic under fake timersThe new waitFor is reasonable, but with vi.useFakeTimers shouldAdvanceTime: true, polling can still be timing-sensitive. Prefer explicit timer advancement + stream take to reduce flakiness.
Apply this diff:
- {- const { snapshot } = await renderStream.takeRender()- expect(snapshot).toMatchObject({ data: 'test3' })- }+ {+ const { snapshot } = await renderStream.takeRender()+ expect(snapshot).toMatchObject({ data: 'test3' })+ }- await waitFor(() => rendered.getByText('test0new'))+ await vi.advanceTimersByTimeAsync(10)+ {+ const { withinDOM } = await renderStream.takeRender()+ withinDOM().getByText('test0new')+ }If you prefer keeping waitFor, consider jest/rtl timer config to ensure it integrates with vitest fake timers in CI.
Also applies to: 1302-1303
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/react-query/src/__tests__/useQuery.promise.test.tsx(9 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
packages/react-query/src/__tests__/useQuery.promise.test.tsx (2)
packages/query-core/src/queryObserver.ts (1)
query(704-721)packages/query-core/src/infiniteQueryBehavior.ts (1)
hasNextPage(162-168)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Preview
- GitHub Check: Test
🔇 Additional comments (3)
packages/react-query/src/__tests__/useQuery.promise.test.tsx (3)
9-9: Confirm peer dependency for waitForUsing waitFor from @testing-library/react 16.x requires @testing-library/dom as a peer. Ensure it’s installed to avoid runtime/typings issues in CI.
Based on learnings
15-17: Public export of useInfiniteQueryImporting useInfiniteQuery from the package barrel implies it’s exported publicly. Please confirm the export is present in the public module to keep tests (and consumers) aligned.
1385-1430: Great coverage: implicit data observation via promiseThis precisely captures the regression for infinite queries (hasNextPage becomes true after suspension). LGTM.
Uh oh!
There was an error while loading. Please reload this page.
fixes#9727
Summary by CodeRabbit
Bug Fixes
New Features
Tests
Chores