fix(base-data-service): replace cached page in place on re-fetch - #9915
Open
szupzj18 wants to merge 2 commits into
Open
fix(base-data-service): replace cached page in place on re-fetch#9915szupzj18 wants to merge 2 commits into
szupzj18 wants to merge 2 commits into
Conversation
fetchInfiniteQuery appended or prepended a fresh copy when re-fetching a page param already present in the cache, leaving duplicate, out-of-order pages that accumulated on every refetch. Detect the existing page and replace it in place instead. Fixes MetaMask#9900 Co-Authored-By: Claude <noreply@anthropic.com>
Co-Authored-By: Claude <noreply@anthropic.com>
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Reviewed by Cursor Bugbot for commit 742c9b1. Configure here.
| pageParams: nextPageParams, | ||
| }); | ||
| return freshPage as TData; | ||
| } |
There was a problem hiding this comment.
Refetch corrupts cache without duplicate
High Severity
The in-place repair always shifts or pops after a re-fetch, assuming query.fetch added a duplicate at the edge. query-core skips the request when getPreviousPageParam or getNextPageParam is null, which is typical when re-fetching a cached page that is not the true next page. The cache is then rewritten from the original pages, so callers can receive the wrong page and observers see dropped or reordered data.
Reviewed by Cursor Bugbot for commit 742c9b1. Configure here.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.


Explanation
Fixes #9900
BaseDataService.fetchInfiniteQueryforwarded re-fetches of an already-cached page param toquery.fetchwith afetchMoredirection. query-core's infinite-query behavior merges a fetched page by appending (forward) or prepending (backward) it, so re-requesting a page that was already in the cache left a duplicate, out-of-order copy in the cache. As the issue reports, the service cache grew on every refetch (e.g.[1, 2]→[2, 1, 2]→[2, 1, 2, 3]), and a newly hydrating observer could briefly receive the corrupted pages.The fix detects that the requested page param is already present before the fetch, and after the fetch completes, collapses the edge-merged duplicate and places the fresh page at the original index via
setQueryData. The fresh page is returned to the caller.Changes
fetchInfiniteQuery: record the existing page index before fetching; when present, normalize the merged result so the cache holds each page exactly once, in order, with fresh data.fetchMoredirections, asserting the cache holds each page once after a re-fetch.Checklist
yarn workspace @metamask/base-data-service run testpasses (100% statements/lines/functions, branches above threshold)yarn lint:tscand ESLint on the changed files passNote
Medium Risk
Changes core pagination cache behavior in BaseDataService; incorrect normalization could still corrupt infinite-query state for activity-style consumers.
Overview
Fixes infinite-query cache corruption when
fetchInfiniteQueryre-requests a page param that is already in the cache. TanStack Query’sfetchMoremerge appended or prepended a second copy instead of updating the existing slot, so the cache could grow with duplicate, out-of-order pages on every refetch.Before fetching, the service now records whether that page param is already cached. After the fetch, if it was, it strips the edge-merged duplicate, writes the fresh page back at the original index via
setQueryData, and returns that page. New tests cover backward and forward refetch directions and assert the cache holds each page exactly once.Reviewed by Cursor Bugbot for commit 742c9b1. Bugbot is set up for automated code reviews on this repo. Configure here.