Core Data: fetch one author-scoped autosave in getAutosave - #30
Core Data: fetch one author-scoped autosave in getAutosave#30whyisjake wants to merge 5 commits into
Conversation
Delegating to getAutosaves with a third argument records resolution under [ postType, postId, 1 ], while hasFetchedAutosaves looks up [ postType, postId ]. The keys never match, so hasFetchedAutosaves stays false and isEditedPostAutosaveable never enables autosaving. The failure is silent. Have getAutosave fetch its own record instead, scoped to the author whose autosave the selector actually returns, and mark getAutosaves resolution finished so hasFetchedAutosaves keeps working. getAutosaves itself is left unchanged, so calling it directly still returns the full collection. Adds packages/core-data/src/test/autosaves.js, which drives a real registry rather than a mocked dispatch so the resolution path is exercised end to end. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
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. |
The completion dispatch sat on the success path only, so an early return (unknown author, post type without autosave support) or a rejected request skipped it. hasFetchedAutosaves then stayed false and isEditedPostAutosaveable never enabled autosaving for the session. This was a regression against the delegation it replaced: that completed getAutosaves resolution unconditionally, and hasFinishedResolution treats an errored resolution as complete, so even a failed fetch counted as fetched. Move the dispatch into a finally, and correct the docblock, which claimed getAutosaves was unaffected. Reporting against its key also suppresses that resolver via hasStartedResolution, so a later direct call returns the stored single-author array rather than refetching. Two existing tests asserted the dispatch was NOT made on the early-return paths, encoding the bug as intended behavior. They now assert the opposite, and cover the failed-request path. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
2ca18d0 to
dc7f449
Compare
|
Merged these changes into WordPress#71367 - work continues there. |
|
Thanks @whyisjake - i merged these changes into the other PR. |
What
Targets
adamsilverstein:fix/autosave-loadingrather thantrunk, so it stacks on WordPress#71367.Reworks
getAutosaveto fetch its own author-scoped record instead of narrowinggetAutosaves, which is closer to what @Mamaduka suggested — with one wrinkle that turns out to be load-bearing.Trac ticket: https://core.trac.wordpress.org/ticket/62057
Why the current approach breaks autosaving
getAutosavecurrently delegates with a third argument:That records resolution under the key
[ postType, postId, 1 ]. ButhasFetchedAutosaveslooks up a two-argument key:EquivalentKeyMapnever matches the two, sohasFetchedAutosavesstaysfalseforever.isEditedPostAutosaveablein@wordpress/editorreturnsfalsewhile that is false, so autosaving never enables. There is no error and nothing in the current test suite catches it — the failure is silent.packages/core-data/src/test/autosaves.jsin this PR reproduces it against a real registry. Against the current branch those tests fail withhasFetchedAutosavesreturningfalse; with this change they pass.The multi-author question
@Mamaduka also asked what happens when the current author did not make the latest autosave. It is a real problem, and it is the reason
per_page=1alone is not enough.Every consumer of
getAutosavepasses the current user's ID, and the selector filters on it:So "the most recent autosave by anyone" is the wrong record whenever a post has more than one editor — the selector filters it out and returns
undefined. Scoping the request by author fixes this, and it needsauthorsupport on the endpoint, which is being added in the companion core patch on the Trac ticket. WordPress stores exactly one autosave per author, soper_page=1combined withauthoris well defined.Changes
getAutosavefetches?context=edit&per_page=1&author=<id>directly.getAutosavesresolver is restored to its original behavior, so its fetch is unchanged for anyone who reaches it first. See the trade-off below for the case wheregetAutosaveruns first — that one is a real behavior change.getAutosavedispatchesfinishResolution( 'getAutosaves', [ postType, postId ] )from afinally, so every exit path reports completion andhasFetchedAutosavescontinues to work.per_page=1from a two-argumentgetAutosavescall. Those currently fail on this branch — the revised resolver only appendsper_pagewhen the third argument is passed, so a two-argument call produces the original path.Trade-off worth reviewing
Reporting completion against the
getAutosaveskey also suppresses that resolver.fulfillSelectorbails whenhasStartedResolutionis true, and that is simply "resolution state exists" — which this dispatch creates. So oncegetAutosavehas run for a post, a later direct call to thegetAutosavesselector returns the single-author array this resolver stored and never refetches the full collection.In this repo nothing reads
state.autosavesexceptgetAutosaveandhasFetchedAutosaves, so there is no in-tree impact. It is a genuine behavior change for third parties callinggetAutosavesafter the editor has mounted, and it is the part of this PR I would most like a second opinion on.The alternative is changing
hasFetchedAutosavesto observegetAutosaveinstead, which means giving it an implicit current-user dependency it does not have today. That seemed worse, but I do not feel strongly.A related consequence:
receiveAutosavesreplaces rather than mergesstate.autosaves[ postId ], so a single-author response overwrites a previously fetched full collection. Same reasoning — no in-tree consumer today, worth knowing.Why
finallyThe completion dispatch is in a
finallyrather than on the success path. The delegation it replaces completedgetAutosavesresolution unconditionally, andhasFinishedResolutiontreats an errored resolution as complete, so a failed request still counted as fetched. Reporting only on success would leaveisEditedPostAutosaveablefalse for the rest of the session after one network failure or early return — the same class of silent breakage this PR is fixing.packages/core-data/src/test/autosaves.jscovers the failed-request and unknown-author paths.Ordering constraint
The
authorparameter does not exist on the endpoint yet. Until the core patch lands, this request returns the same thing it does today — WordPress ignores unknown query parameters — so nothing breaks, but the author scoping is inert.Separately, the core-side preload path must not be narrowed until this change syncs into core. The preload cache is keyed on the normalized path, so a core preload of
?context=edit&per_page=1&author=Nagainst a client requesting?context=editmisses on every editor load: the preload is computed and discarded and the request goes to the network anyway. The core patch is split in two on the Trac ticket for this reason.Testing instructions
packages/core-data/src/test/autosaves.jsis the meaningful one — it drives a real registry rather than a mocked dispatch, so it exercisesfinishResolution→hasFinishedResolution→hasFetchedAutosavesend to end.To see the regression this fixes, revert
getAutosaveto the version on this branch and re-runautosaves.js: the first two tests fail withhasFetchedAutosavesreturningfalse.Disclosure
This change was developed with AI assistance (Claude Code), per the WordPress AI guidelines. The commit carries a
Co-Authored-Bytrailer for the same reason.I have reviewed every line and can speak to the approach and its trade-offs — in particular the
finishResolutioncall, which is the one decision here I would most like a second opinion on. The unit and integration tests above were run locally against this branch, along with the widerpackages/core-dataandpackages/editor/src/storesuites (585 tests, all passing) and ESLint.