Editor: Decouple local and remote autosave monitors - #79801
Conversation
|
Size Change: +203 B (0%) Total Size: 7.68 MB 📦 View Changed
|
|
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 3436c62. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/28920037212
|
tyxla
left a comment
There was a problem hiding this comment.
This is looking mostly good to me, left just a few minor questions.
How do we test this thoroughly to ensure there are no regressions? Do you feel the test steps (and the existing tests) are enough?
| // remote (REST) save state. | ||
| describe( 'independence from the remote monitor', () => { | ||
| it( 'should save while a remote autosave is in flight', () => { | ||
| setState( { isSaveable: true, isDirty: true, isAutosaving: true } ); |
There was a problem hiding this comment.
This test mocks isEditedPostSaveable to return true, but in a real use it returns false whenever a save is running (isSavingPost = state.saving.pending), which is exactly the case during a remote autosave. So in production, the local tick's if ( ! isEditedPostSaveable() ... ) return; guard would actually skip the backup here, which is the opposite of what the test claims. The test passes only because the mock hides that coupling. So the local monitor isn't really independent of remote saves. Either the test should use the real saveable logic, or the guard needs to stop depending on isSavingPost if full independence is what we want to ensure.
There was a problem hiding this comment.
Good catch, @tyxla!
The local autosave is still dependent on remove saving; the isSavingPost will be true for both manual and autosave triggers.
I guess the main question here is whether we want a local backup autosave while the remote one is in-flight, which later will be purged if the save request is successful.
There was a problem hiding this comment.
I guess the main question here is whether we want a local backup autosave while the remote one is in-flight, which later will be purged if the save request is successful.
Maybe we want one to capture the scenario when the remote one wasn't successful?
There was a problem hiding this comment.
Updated, tried to capture new behavior in should create local autosave if remote autosave fails e2e test. It's not perfect, but I couldn't come up with anything better.
9d38a69 to
3436c62
Compare
3436c62 to
d91e31d
Compare
|
@tyxla, added a section for testing local autosaves. |
| ( key ) => backup[ key ] === getEditedPostAttribute( key ) | ||
| ); | ||
| if ( isBackupCurrent ) { | ||
| return; |
There was a problem hiding this comment.
This will never update the lastEditsReferenceRef ref, is that expected? Should we still set it to editsReference as done below?
And if yes, we might want to add test coverage for this scenario.
What?
Follow-up to #79043.
PR reformulates the autosave decision logic so remote and local autosave are independent — each has its own clock, its own "anything to save?" check, and its own notion of busy. Here's what's changed:
AutosaveMonitoris now remote-only, andLocalAutosaveMonitorcomposes its own timer tick instead of wrapping the remote component. The only shared code is the smalluseIntervalhelper (now built onuseEvent).isEditedPostAutosaveable()orisAutosavingPost().<PostTypeSupportCheck supportKeys="autosave">, so nothing mounts (no timers, no hooks) for post types without autosave support.autosave-existsnotice fromEditorProviderintoAutosaveMonitor, so both autosave notices live beside the code that triggers them.localAutosaveGet()now returns parsed{ title, content, excerpt }edits instead of a raw string; both call sites dropped their parse/compare boilerplate.local-autosave-monitor/merged intoautosave-monitor/aslocal.js. Public exports are unchanged.LocalAutosaveMonitor, including its independence from remote save state.Why?
LocalAutosaveMonitorreused the remote engine and silently inherited remote-only checks: local backups compared edits against the server autosave, waited for the server autosave to load, and paused while a REST save was in flight. None of that is relevant to writing sessionStorage.Open question: #79043 dropped the old 1s fast-retry while the post is ineligible, so the first autosave after a post becomes eligible can lag up to one interval. This PR keeps it out; flagging for reviewers whether to reintroduce it.
Behavior changes:
autosave-existsnotice no longer skips re-creation on error recovery (same ID, so no duplicates).Testing Instructions
Testing local autosaves.
User-facing behavior is also covered via
test/e2e/specs/editor/various/autosave.spec.js.Testing Instructions for Keyboard
Same.
Use of AI Tools
Assisted by Claude