Core Data: Treat single-item responses specially#76318
Merged
Conversation
Mamaduka
added
[Type] Bug
|
Size Change: +32 B (0%) Total Size: 6.89 MB
ℹ️ View Unchanged
|
|
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
requested review from
ellatrix,
jsnajdr,
ramonjd,
tyxla and
youknowriad
|
Flaky tests detected in 6b68e83. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/22895526360
|
jsnajdr
approved these changes
Mar 9, 2026
Member
jsnajdr
left a comment
jsnajdr
left a comment
There was a problem hiding this comment.
Looks good, just a few code nitpicks 🙂
| // reducer tracks only a single query object. | ||
| onSubKey( 'stableKey' ), | ||
| ] )( ( state = {}, action ) => { | ||
| const { type, page, perPage, key = DEFAULT_ENTITY_KEY } = action; |
Member
There was a problem hiding this comment.
I wanted to avoid this destructuring because it's not strictly type-safe and after a potential TypeScript rewrite it would be an error. It accesses fields like page on actions of all types, even these that don't have the field at all.
| 'postType', | ||
| template.type, | ||
| template | ||
| ); |
Member
There was a problem hiding this comment.
Yes, this is the one, thanks for finding it 😆
Member
There was a problem hiding this comment.
I added this, what did I do? 🙊
Member
Author
|
Thanks, @jsnajdr! Pushed changes based your feedback ✅ |
|
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. |
jsnajdr
reviewed
Mar 10, 2026
| } | ||
|
|
||
| if ( type !== 'RECEIVE_ITEMS' ) { | ||
| if ( ! Array.isArray( action.items ) ) { |
Member
There was a problem hiding this comment.
This maybe deserves a comment: when items is only an individual item, then the action doesn't have any information for updating the item list.
ellatrix
reviewed
Mar 10, 2026
| ); | ||
| records = Array.isArray( records ) | ||
| ? records.map( addTitleToAutoDraft ) | ||
| : addTitleToAutoDraft( records ); |
Member
There was a problem hiding this comment.
It's not possible to receive multiple auto-drafts?
Member
Author
There was a problem hiding this comment.
Maybe not, but this case was here, we just adjusted it to work with this concept.
Member
There was a problem hiding this comment.
You mean that there can ever be only one auto-draft in any list of received entities? Then addTitleToAutoDraft will find and update only max one, no problem. This code merely aims to preserve single-item records parameter, without converting it to one-element array.
Member
Author
There was a problem hiding this comment.
The collection can also have more auto-draft records, if created programmatically. Here's one case that comes to mind - #56881. I know it's rare, but technically still possible.
tyxla
reviewed
Mar 10, 2026
| return state; | ||
| } | ||
|
|
||
| const key = action.key || DEFAULT_ENTITY_KEY; |
Member
There was a problem hiding this comment.
Isn't this subtly different from the prior destructuring? Technically, right now, if action.key is 0, this will fall back to the default key due to the boolean check. Previously, this would only happen if key is not defined.
Maybe that's alright and even safer than before, but just wanted to point it out.
Member
Author
There was a problem hiding this comment.
Good point. I'll change it to action.key ?? DEFAULT_ENTITY_KEY.
Member
Author
|
I'm going to merge this if there's no further feedback. Then I can land the remaining improvements for revisions - #76043. |
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
4 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?
Extracted from #76043. Props to @jsnajdr!
PR fixes a bug where receiving a single-entity item could override or corrupt the entire collection.
queries.itemIdsunless we're dealing with collections.Why?
See #76043 (comment).
Testing Instructions
I've included basic integration tests, happy to include more.
Testing Instructions for Keyboard
Same.