Core Data: Optimize revision selectors#76043
Merged
Conversation
Mamaduka
added
[Type] Performance
|
Size Change: +123 B (0%) Total Size: 6.89 MB
ℹ️ View Unchanged
|
|
Flaky tests detected in f1c2cbd. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/22945690849
|
Mamaduka
force-pushed
the
try/optimize-revision-selectors
branch
from
8710dc2 to
6a64695
Compare
|
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. |
Mamaduka
commented
Mar 2, 2026
Mamaduka
requested review from
ellatrix,
jsnajdr,
ramonjd,
tyxla and
youknowriad
ramonjd
reviewed
Mar 3, 2026
| } | ||
|
|
||
| if ( record ) { | ||
| dispatch.receiveRevisions( |
Member
There was a problem hiding this comment.
Just a note to say I tested using getRevision in the `getCurrentRevision' selector on this branch, and the revisions UI was working fine in Chrome and Safari.
Context:
ellatrix
approved these changes
Mar 3, 2026
Member
ellatrix
left a comment
ellatrix
left a comment
There was a problem hiding this comment.
Thanks! Revisions UI still works. Worth noting that we changed #74771 in the end to always fetch all revision, we should try again after this to load only the current and prev revision and only have the slider depend on all revisions. The slider will then still take longer to load, but at least you can see the current revision diff quicker. I've also started a branch somewhere to only fetch revisions when you slide, but it's not the best UX tbh. Maybe we need some better heuristics.
Mamaduka
force-pushed
the
try/optimize-revision-selectors
branch
from
e528b87 to
6753968
Compare
Mamaduka
requested review from
ajitbohra,
fabiankaegy and
juanmaguitar
as code owners
Member
Author
|
Unfortunately, I'm still able to reproduce the race condition issue when restoring the selector in Which makes me wonder if I have the same problem with Here's the problem: I've pushed failing, skipped the test to showcase the issue. Testing patch
diff --git a/packages/editor/src/store/private-selectors.js b/packages/editor/src/store/private-selectors.js
index 0802d51d4ca..03f1c40f1cb 100644
--- a/packages/editor/src/store/private-selectors.js
+++ b/packages/editor/src/store/private-selectors.js
@@ -335,28 +335,15 @@ export const getCurrentRevision = createRegistrySelector(
}
const { type: postType, id: postId } = getCurrentPost( state );
- // - Use getRevisions (plural) instead of getRevision (singular) to
- // avoid a race condition where both API calls complete around the
- // same time and the single revision fetch overwrites the list in the
- // store.
- // - getRevision also needs to be updated to check if there's any
- // received revisions from the collection API call to avoid unnecessary
- // API calls.
- const revisions = select( coreStore ).getRevisions(
+ const revision = select( coreStore ).getRevision(
'postType',
postType,
postId,
- { per_page: -1, context: 'edit' }
+ revisionId,
+ { context: 'edit' }
);
- if ( ! revisions ) {
- return null;
- }
- const entityConfig = select( coreStore ).getEntityConfig(
- 'postType',
- postType
- );
- const revKey = entityConfig?.revisionKey || 'id';
- return revisions.find( ( r ) => r[ revKey ] === revisionId ) ?? null;
+
+ return revision || null;
}
);Screenshot
|
Merged
7 tasks
Member
Yes, I agree this is a bug. When we're receiving a single item, the And I agree that |
Member
|
@Mamaduka I pushed a first attempt as a fix (654197c). The trick is to treat single-item and multiple-item receives as very different. Passing What is incomplete about the fix:
|
Mamaduka
commented
Mar 9, 2026
Member
Author
|
Thanks, @jsnajdr!
I can check the codebase and fix similar conversions.
Can you elaborate a bit more on the nature of these new unit tests? Should they be similar to e15a444? |
Member
I also saw something like
Very similar to what we already have for revisions, just a more generic test for all entities in general. |
Member
Author
|
Decide to cherry-pick the general fix into a separate PR - #76318. I've also applied changes based on our latest discussion. |
Mamaduka
force-pushed
the
try/optimize-revision-selectors
branch
from
654197c to
93b68fe
Compare
Member
Author
|
Rebased on top of the general fix #76318. I've also confirmed These general improvements are good to merge. I would appreciate final approval. Test diffdiff --git a/packages/editor/src/store/private-selectors.js b/packages/editor/src/store/private-selectors.js
index 0802d51d4ca..af21d160b6a 100644
--- a/packages/editor/src/store/private-selectors.js
+++ b/packages/editor/src/store/private-selectors.js
@@ -335,28 +335,14 @@ export const getCurrentRevision = createRegistrySelector(
}
const { type: postType, id: postId } = getCurrentPost( state );
- // - Use getRevisions (plural) instead of getRevision (singular) to
- // avoid a race condition where both API calls complete around the
- // same time and the single revision fetch overwrites the list in the
- // store.
- // - getRevision also needs to be updated to check if there's any
- // received revisions from the collection API call to avoid unnecessary
- // API calls.
- const revisions = select( coreStore ).getRevisions(
+ const revision = select( coreStore ).getRevision(
'postType',
postType,
postId,
- { per_page: -1, context: 'edit' }
- );
- if ( ! revisions ) {
- return null;
- }
- const entityConfig = select( coreStore ).getEntityConfig(
- 'postType',
- postType
+ revisionId,
+ { context: 'edit' }
);
- const revKey = entityConfig?.revisionKey || 'id';
- return revisions.find( ( r ) => r[ revKey ] === revisionId ) ?? null;
+ return revision || null;
}
); |
jsnajdr
approved these changes
Mar 10, 2026
Member
jsnajdr
left a comment
jsnajdr
left a comment
There was a problem hiding this comment.
Looks good, just a few nits as usual 🙂
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
5 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.
What?
Restore revisions optimizations from 11439e3#diff-fec6390e24332dec4a0c7c74f9f7b9a015ce191e0e6348361ead793ceda3991dL1143-L1145.
PR applies context-based resolution optimizations and fixes potential race conditions, which could wipe out previously fetched collections.
It's easier to review changes with white spaces hidden.
Why?
In #74771, we noticed that a later call to
getRevisioncould wipe out already fetched collection for the samerecordKey.Testing Instructions
I've included an integration test, happy to add more as needed.
Testing Instructions for Keyboard
Same.
AI Usage
Had an initial idea, used Claude to iterate. Reviewed results.