Size Change: +43 B (0%)

Total Size: 8.75 MB

_{compressed-size-action}

Flaky tests detected in 5bf3804.
Some tests passed with failed attempts. The failures may not be related to this commit but are still reported for visibility. See the documentation for more information.

🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/23227274347
📝 Reported issues:

• [Flaky Test] post title and content are editable and blocks can be inserted #76524 in /test/e2e/specs/editor/various/post-content-focus-mode.spec.js
• [Flaky Test] should move focus back to the Publish panel toggle button when canceling #76179 in /test/e2e/specs/editor/various/publish-panel.spec.js

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 props-bot label.

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Co-authored-by: andrewserong <andrewserong@git.wordpress.org>
Co-authored-by: ramonjd <ramonopoly@git.wordpress.org>

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

Thanks for the quick review @ramonjd! 🙇

I've added an extra test for that case where the offset matches the totalItems, and I've moved where the number validation happens (and added a tiny test). I think that makes things a little tidier, but let me know if it's not what you had in mind.

LGTM - nice one!

Good catch and thanks, @andrewserong!

I totally forgot about the offset pagination argument; we don't really use it in the app. I think the original bug didn't affect this case because it will have a different stableKey cache key for every request. Which brings me to my next point: Should theoffset, like other pagination arguments, be excluded from stableKey?

Looking folks for feedback, @jsnajdr, @youknowriad, @ntsekouras.

P.S. I think infinite scroll logic should use page/per_page combo and let the internals calculate the offset.

Thanks George!

Which brings me to my next point: Should the offset, like other pagination arguments, be excluded from stableKey?

Good question. As far as I could tell, to do that we'd need to update more of the logic to handle offset properly (e.g. in getMergedItemIds) so it seemed a simpler (and pragmatic) fix for now to keep it included in the stableKey. That said, I think you're right — ideally, offset would be factored in so that it's still the same sequence of items (with undefined entries where holes exist).

Could be a good code quality / performance thing to follow-up on at some point (I likely won't get a chance to look at in in the next couple of weeks as I'll be travelling).

P.S. I think infinite scroll logic should use page/per_page combo and let the internals calculate the offset.

Worth taking a look at the latest infinite scroll PR if you haven't already (it is a big one, though!): #74378

Agree, that's something we can improve in follow-up. I can give it a try if I have time this week.

One last question: Have you tried using meta?.totalPages instead of meta?.totalItems? If yes, were there any downsides in your tests?

I think that approach would require fewer changes and would mostly be contained in getQueriedItemsUncached.

Have you tried using meta?.totalPages instead of meta?.totalItems? If yes, were there any downsides in your tests?

Oh no, I didn't try that! I have to say, I went from hearing about the bug to trying to fix it fairly quickly, so didn't consider much how else we could go about it 🙂

Not a problem. I'll do more testing and see if it works.

Update: Just finished testing my theory, and it doesn't work, mostly because the server also ignores the offset parameter when calculating pagination meta. Which is probably intentional, but I couldn't find any Trac ticket.

We should definitely follow up with an improvement where offset is not a part of stable key and where startOffset in getQueriedItems is calculated also from offset, not only from page and per_page. Then page and offset would be two equivalent ways how to specify startOffset, and queries with offset would contribute to one big array of results, instead of having their own private results array.

We should definitely follow up with an improvement where offset is not a part of stable key

Trying to follow up on this in #76808.