Core Data: remove offset param from stableKey, use pagination logic#76808
Merged
Core Data: remove offset param from stableKey, use pagination logic#76808
Conversation
jsnajdr
added
[Type] Enhancement
|
Size Change: +2 B (0%) Total Size: 7.73 MB
ℹ️ View Unchanged
|
Mamaduka
reviewed
Mar 25, 2026
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
|
Flaky tests detected in 4ca4323. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/23744640133
|
Contributor
andrewserong
left a comment
andrewserong
left a comment
There was a problem hiding this comment.
Thanks for the ping and follow-up! The issue was encountered while #74378 was being reviewed and tested, which updating infinite scroll behaviour, but the feature isn't actively being used in Gutenberg yet (the behaviour in Storybook is working well, but there was still some bugginess when used in the experimental media modal, so it was removed before landing).
However, we can hack it in locally using the following (borrowed from that PR) to verify that the problem where the last "page" of results wasn't loaded, is still resolved with this change:
diff --git a/packages/media-utils/src/components/media-upload-modal/index.tsx b/packages/media-utils/src/components/media-upload-modal/index.tsx
index 6b571964209..726857536e0 100644
--- a/packages/media-utils/src/components/media-upload-modal/index.tsx
+++ b/packages/media-utils/src/components/media-upload-modal/index.tsx
@@ -196,13 +196,14 @@ export function MediaUploadModal( {
titleField: 'title',
mediaField: 'media_thumbnail',
search: '',
- page: 1,
+ startPosition: 1,
perPage: 50,
filters: [],
layout: {
previewSize: 170,
density: 'compact',
},
+ infiniteScrollEnabled: true,
} ) );
// Build query args based on view properties, similar to PostList
@@ -243,6 +244,21 @@ export function MediaUploadModal( {
: allowedTypes;
}
+ // For infinite scroll, use offset-based pagination
+ // For regular pagination, use page-based pagination
+ if ( view.infiniteScrollEnabled && view.startPosition !== undefined ) {
+ return {
+ offset: view.startPosition - 1,
+ per_page: view.perPage || 20,
+ status: 'inherit',
+ order: view.sort?.direction,
+ orderby: view.sort?.field,
+ search: view.search,
+ ...filters,
+ };
+ }
+
+ // Regular page-based pagination
return {
per_page: view.perPage || 20,
page: view.page || 1,
@@ -443,13 +459,16 @@ export function MediaUploadModal( {
[ allowedTypes, handleUpload, registerBatch ]
);
- const paginationInfo = useMemo(
- () => ( {
- totalItems,
- totalPages,
- } ),
- [ totalItems, totalPages ]
- );
+ const prevPaginationInfoRef = useRef( { totalItems: 0, totalPages: 0 } );
+
+ const paginationInfo = useMemo( () => {
+ // Only update when we have valid values (not both 0)
+ // to avoid showing 0 values during data fetching
+ if ( totalItems > 0 || totalPages > 0 ) {
+ prevPaginationInfoRef.current = { totalItems, totalPages };
+ }
+ return prevPaginationInfoRef.current;
+ }, [ totalItems, totalPages ] );
const defaultLayouts = useMemo(
() => ( {
Then, we can enable the media modal experiment in Gutenberg and try it out when selecting a featured image block:
With all that, I'm seeing the same behaviour in trunk as in this PR, and the last "page" of results is correctly loaded with this PR applied. To test, I'm comparing the classic media library at http://localhost:8888/wp-admin/upload.php and making sure that the last items are appearing for me in that view, and in the modal view in the editor.
So, this is looking like a good direction to me, and the logic change makes good sense to me, with offset being used to set the range to update in getMergedItemItds 👍
It sounds like you wanted to do a bit more refactoring before landing? I'm travelling at the moment, so apologies if I don't get back to you quickly once you've done further updates, but feel free to land this if/when you're happy with it.
Mamaduka
reviewed
Mar 27, 2026
Mamaduka
reviewed
Mar 27, 2026
Comment on lines
+50
to
+51
| const startOffset = | ||
| perPage === -1 ? 0 : queryOffset ?? ( page - 1 ) * perPage; |
Member
There was a problem hiding this comment.
The page argument is now ignored when offset is provided. Is this something we want to document for our consumers?
Member
Author
There was a problem hiding this comment.
Yes, offset takes precedence over page. That's also how the server endpoint behaves. Query like this:
// can be ran in console
await wp.apiFetch({ path:'/wp/v2/media?offset=4&page=2&per_page=2' })will return items at offset=4 and the page=2 param will be ignored.
Where would be the best place to document this? The getEntityRecords selector doesn't document its query parameters.
Member
There was a problem hiding this comment.
Since we're dealing with general pagination params for the REST API, maybe it should be clarified in its docs: https://developer.wordpress.org/rest-api/using-the-rest-api/pagination/#pagination-parameters.
It's not a blocker here, just wanted to leave a note as I was reviewing PR.
Member
Author
I applied the diff locally, and yes, it sort of works now. At least the queries and the merged items in Core Data state look right. However, the All in all, the fetching seems OK, but the scroll behavior is looping and jumping a lot. |
Mamaduka
reviewed
Mar 27, 2026
Contributor
Yes, that's what I'm seeing too (either with it applied on trunk or applied on top of this PR). That'll definitely need to be looked into (and fixed) before we opt-in to the support properly! |
jsnajdr
force-pushed
the
update/offset-pagination-logic
branch
from
6b83ff2 to
9975ba5
Compare
Mamaduka
reviewed
Mar 30, 2026
Member
There was a problem hiding this comment.
Maybe we should add a unit test for the new offset option, currently we're testing it indirectly in a different test suite.
| } | ||
| const nextItemIdsStartIndex = ( page - 1 ) * perPage; | ||
|
|
||
| const nextItemIdsStartIndex = offset ?? ( ( page ?? 1 ) - 1 ) * perPage; |
Member
There was a problem hiding this comment.
Should we guard against possible NaN when the options parameter is omitted?
This isn't really a case at the moment, and even with #76406, we can make this work, but it might be worth hardening.
Member
Author
There was a problem hiding this comment.
If options are omitted, then the only thing we are missing is perPage, is that right? And the best default seems to be -1. getMergedItemIds without any options will simply replace itemIds with nextItemIds, without any merging.
The equivalent REST request (to /wp/v2/media, for example) will return only the first page of results, with per_page defaulting to 10. But here it's getQueryParts who will provide the defaults.
Member
There was a problem hiding this comment.
If options are omitted, then the only thing we are missing is perPage, is that right?
That's correct, and nextItemIdsStartIndex will be NaN.
And the best default seems to be -1. getMergedItemIds with no options simply replaces itemIds with nextItemIds, without any merging.
Do you think that could introduce any side effects?
P.S. If we decide to provide a default during destructions, we could do the same for page and simplify this - ( ( page ?? 1 ) - 1 ).
Member
Author
There was a problem hiding this comment.
We can apply the same defaults as getQueryParts does: page = 1 and perPage = 10. Then they will be duplicated, not beautiful, but OK.
The unbounded query with perPage = -1 has some edge cases that are buggy across the entire stack.
What if I fetch with both page and perPage=-1 specified?
wp.apiFetch( { path: '/wp/v2/media?page=2&per_page=-1' } )Then the fetchAllMiddleware helper will do a series of per_page=100 fetches and will concat them together. The result is an array of items from index 100 to the end. page=3 will start at 200, etc.
The getQueriedItems( { page: 2, perPage: -1 } ) selector will ignore the page argument and will return the entire set. That's different from apiFetch.
The getMergedItems( current, next, { page: 2, perPage: -1 } ) helper? Before this PR, it would set nextItemIdsStartIndex = -1 and continue calculating some nonsense result. After this PR, it ignores the page parameter and returns the entire set. Same as getQueriedItems, different from apiFetch.
Things will outright break when the query is { offset: 10, perPage: -1 }. apiFetch will be sending requests like:
/wp/v2/media?offset=10&per_page=100
/wp/v2/media?offset=10&per_page=100&page=2
/wp/v2/media?offset=10&per_page=100&page=3
/wp/v2/media?offset=10&per_page=100&page=4
Because the offset parameter takes precedence over page, the fetch result will be 4 times the same array, concatenated together.
getQueriedItems and getMergedItemIds, will return the entire set for this query, ignoring the offset parameter.
Nothing of this is really new.
Member
There was a problem hiding this comment.
We can apply the same defaults as getQueryParts does: page = 1 and perPage = 10. Then they will be duplicated, not beautiful, but OK.
Sounds good. I'll continue work on my pagination PR after this is merged.
Nothing of this is really new.
Definitely, but it's worth documenting and addressing separately if we can. I think, with DataViews being used more in core and by 3rd-party consumers, folks might run into these edge cases more often.
Member
Author
There was a problem hiding this comment.
@Mamaduka I added defaults to getMergedItemIds options, as discussed, and also added one new unit test for the queries reducer.
Definitely, but it's worth documenting and addressing separately if we can.
I'll start a separate PR for this, adding warnings at various places about conflicting page, offset and perPage values.
I think I addressed all feedback, let me know if anything is still missing.
adamsilverstein
pushed a commit
that referenced
this pull request
Mar 31, 2026
…76808) * Core Data: remove offset param from stableKey, use pagination logic * getMergedItemIds: refactor to options object * getQueryParts: update offset jsdoc and type * getQueryParts: improve jsdoc * getMergedItemIds: defaults for page and perPage * Reducer unit test for sparse items at given offsets Co-authored-by: jsnajdr <jsnajdr@git.wordpress.org> Co-authored-by: Mamaduka <mamaduka@git.wordpress.org> Co-authored-by: andrewserong <andrewserong@git.wordpress.org>
ObliviousHarmony
pushed a commit
that referenced
this pull request
Apr 1, 2026
…76808) * Core Data: remove offset param from stableKey, use pagination logic * getMergedItemIds: refactor to options object * getQueryParts: update offset jsdoc and type * getQueryParts: improve jsdoc * getMergedItemIds: defaults for page and perPage * Reducer unit test for sparse items at given offsets Co-authored-by: jsnajdr <jsnajdr@git.wordpress.org> Co-authored-by: Mamaduka <mamaduka@git.wordpress.org> Co-authored-by: andrewserong <andrewserong@git.wordpress.org>
adamsilverstein
pushed a commit
that referenced
this pull request
Apr 7, 2026
…76808) * Core Data: remove offset param from stableKey, use pagination logic * getMergedItemIds: refactor to options object * getQueryParts: update offset jsdoc and type * getQueryParts: improve jsdoc * getMergedItemIds: defaults for page and perPage * Reducer unit test for sparse items at given offsets Co-authored-by: jsnajdr <jsnajdr@git.wordpress.org> Co-authored-by: Mamaduka <mamaduka@git.wordpress.org> Co-authored-by: andrewserong <andrewserong@git.wordpress.org>
1 task
Mamaduka
reviewed
Apr 13, 2026
Comment on lines
+45
to
+49
| // The defaults for `page` and `perPage` are the same as in `getQueryParts`. | ||
| { page = 1, offset, perPage = 10 } = {} | ||
| ) { | ||
| // If the query is unbounded, then `nextItemIds` is a complete replacement. | ||
| if ( perPage === -1 ) { |
Member
There was a problem hiding this comment.
Cross-posting for future reference. It looks like we accidentally introduced regression here. See - #76406 (comment).
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
3 participants
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.
Followup to #76613 that removes
offsetfromstableKeyand treats it as a new pagination parameter, on par withpageandper_page. The query results for different offsets are merged into one big array withgetMergedItemIds.I'm not sure how to test it. There are unit tests, but they test isolated parts of the entire process (the selectors, the reducer). @andrewserong Do we already have a UI feature that uses the infinite scroll with
offset, so that it could be tested there, end to end?