fix(query-core): clear stale select error when observer switches to a query without data - #11161
Conversation
… query without data
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
Included review availability: Your plan includes up to 10 reviews per rolling hour; 1 remains after this review. 📝 WalkthroughWalkthrough
ChangesSelect error reset
Estimated code review effort: 2 (Simple) | ~10 minutes Merge Risk: ⚪ Minimal · up to The change clears stale select errors when an observer switches queries or resets, preventing incorrect error states during refetching; no actionable merge-blocking risk remains after normal checks and review. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
MILLERMARRU
left a comment
There was a problem hiding this comment.
Traced #selectError through createResult to confirm this. It's an instance field that only gets touched inside the options.select && data !== undefined && !skipSelect block, so when the observer switches to a query whose current data is undefined (a fresh key, or right after a reset), that block is skipped entirely and whatever #selectError was left over from the previous query keeps sitting there. Then the unconditional if (this.#selectError) a few lines down turns that stale error into status: 'error' on a query that hasn't even started fetching yet. The fix closes exactly that hole by clearing it in the one case the original condition didn't cover.
One combination I don't see a test for: skipSelect gets set to true in the placeholderData memoization path (when prevResult.isPlaceholderData and the same placeholderData option is being reused), and in that path data ends up defined again (it's the placeholder), so neither branch of the if/else if runs. If a query switch lands on that exact memoized-placeholder path while a #selectError from the prior query is still set, I think it could still leak through. Narrow edge case, probably not worth blocking on, but might be worth a third branch or a comment confirming it's out of scope.
|
Good catch. I spent some time trying to reproduce this, and you're right that the memoized-placeholder path is the one exit my But I couldn't reach that state without going through the bug #11011 fixes. The memoization only runs when The one repro I found on current
With #11011 applied step 3 is unreachable, so a clearing branch here turns into dead code the moment that merges. My preference is to leave this branch alone and let #11011 close the hole |
|
Traced this against current Where I'd push back a little: "leave this branch alone and let #11011 close the hole" assumes #11011 actually lands, and it's been open since June 30 with zero reviews. This PR is moving, that one isn't. If this merges and releases before #11011 does (which looks likely at this rate), the gap you found is a known, filed, reachable bug shipping in a release, on purpose, because it's waiting on a dependency that has no timeline. Given that, I'd just fold #11011's one-line fix into this PR instead of leaving the two coupled: if (this.#selectError) {
error = this.#selectError
data = this.#selectResult
errorUpdatedAt = Date.now()
status = 'error'
isPlaceholderData = false
}That's not dead code the way a guard on the memoization branch would be, it's the actual fix for the type-contract violation, and it happens to also close the memoization hole as a side effect, since |
|
Fair enough. The types.ts angle settles it for me: if the error result variants declare I folded the line into the error block here, plus a regression test for the exact chain from this thread (stable Credit where due: that line is #11011 by @chatman-media, which also comes with its own test for the placeholder case. If that lands first I'll rebase; the collision is trivial in either order. |
|
#11011 was just merged |
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
|
View your CI Pipeline Execution ↗ for commit a650397
☁️ Nx Cloud last updated this comment at |
Fixes #11160
Problem
When a
selectfunction throws, the error is stored on the observer (#selectError) and surfaced on every subsequent result. It is only cleared inside the select block itself, which is skipped entirely whiledataisundefined. Switching the observer to a differentqueryKey, or resetting the query, therefore leaves the old error in place: the new pending query reportsstatus: 'error'with the previous query's select error, and withthrowOnError/ suspense the stale error is thrown into the error boundary while the new query is still fetching.Solution
In
createResult, clear the stored select error whenever a result is built without data to select:A select error can only be produced by running
selecton some data — if the result being built has no data (fresh query after a key switch, or a reset query), that data is gone and the error cannot describe the current result.The "keep the error while the same data and selector are in place" memoization is untouched: in that case the query's own
datais defined (a select error implies the queryFn succeeded), so that path still goes through the select block as before. If the selector genuinely throws again for new data, the error is re-created — only stale errors are dropped.Clearing is done at result-creation time rather than on query switch (
#updateQuery) becauseresetQueriesreplaces the state of the same Query instance — a switch-based clear would miss it.Tests
Two regression tests in
queryObserver.test.tsx, written first and failing onmainwith the exact buggy behavior (status: 'error'where'pending'is expected):setOptionsto key B → result ispendingwitherror: null, then succeedsresetQueries→ same expectationFull
query-core(509) andreact-query(417) suites pass.Related but distinct from #11011, which handles
isPlaceholderDatawhen select throws on placeholder data; the two changes touch different branches and compose.Summary by CodeRabbit
Bug Fixes
Tests
Update
After the review discussion below, this PR also resets
isPlaceholderDatain the select-error block. That is the same one-line fix as #11011 by @chatman-media (credit to that PR); folding it in aligns the runtime with the error-result types intypes.ts(which declareisPlaceholderData: false) and closes the memoized-placeholderData leak found in review, so this PR no longer depends on #11011's merge order. A regression test covers the full leak chain.