Sync editor settings in layout effect (fixes autosave e2e)

[jsnajdr]

Closes #58959.
Supersedes and closes #77829.

Attempt to fix #78706, a flaky autosave test that started failing 2 days ago. The most likely suspect is the React 19 migration which changed the precise scheduling of effects, state updates and similar things.

The problem is that when we have both remote (server) and local (storage) autosaves, we show notices about both of them:

Locator: locator('.components-notice__content')
Expected substring: "There is an autosave of this post that is more recent than the version below."
Error: strict mode violation: locator('.components-notice__content') resolved to 2 elements:
    1) <div class="components-notice__content">The backup of this post in your browser is differ…</div> aka getByText('The backup of this post in')
    2) <div class="components-notice__content">There is an autosave of this post that is more re…</div> aka getByLabel('Editor content').getByText('There is an autosave of this')

although we have explicit code (added 7 years ago in #17501) that disables the local notice when the remote one is shown:

gutenberg/packages/editor/src/components/local-autosave-monitor/index.js

Lines 100 to 102 in 86c5139

	if ( hasRemoteAutosave ) {
	return;
	}

But this doesn't work because of the following combination of reasons:

• the hasRemoteAutosave value is read in useEffect, on mount, from select( editorStore ).getEditorSettings().autosave, i.e., the editor settings stored in data store
• at the moment of the mount, the data store doesn't contain the real settings yet, it only has the default values. The real settings are saved in another useEffect, in ExperimentalEditorProvider. And because effects on parent components run only after effects on child components, the sync happens too late.

The result is that the effect in LocalAutosaveMonitor always sees a false value of the autosave setting, the check doesn't work.

I'm fixing this by doing the updateEditorSettings call in a layout effect instead. Then it happens early enough. Doing this is generally a good idea, it's not a mere hack. Because the initial render doesn't yet have the real settings, only the default ones, we shouldn't paint the result. We should trigger a second render as soon as possible, and paint only after the second, correct render finishes.

In this PR I'm also removing the stillHasRemoteAutosave check, because it hides another bug. This check was added by @mcsf in #17679, also 7 years ago. The problem was that sometimes the autosave is not there after reload, although it was reliably saved. I don't know why this was happening in 2019, but today it's happening because of RTC 🙂

There is this POST request that saves the crdtDocument to post meta, triggered by a supposedly read-only getEntityRecord. This request happens after the autosave. And the very bad thing about it is that it updates the "modified" timestamp of the post. When the edit-form-blocks.php page decides whether to send the autosave, it checks the timestamps:

https://github.com/WordPress/wordpress-develop/blob/75fc4e9f0eb6ec211b1d95e2bd80683fc36c5090/src/wp-admin/edit-form-blocks.php#L291-L300

In my testing, the autosave and meta-save happen either in the same second, and then the test succeeds. Or the meta-save happens in the next second, and then the modified timestamp is bigger, the remote autosave is deleted and the test fails.

For this reason, the e2e test will still fail. What can we do about it? The meta-save request probably shouldn't update any timestamps at all. FYI @alecgeatches

[github-actions]

Size Change: -14 B (0%)

Total Size: 8.6 MB

📦 View Changed

Filename	Size	Change
build/scripts/editor/index.min.js	473 kB	-14 B (0%)

_{compressed-size-action}

[Mamaduka]

The change reminded me of our previous conversation - #49409 (comment). Do you think that's still doable? It should also resolve the related issue #15993.

[github-actions]

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: jsnajdr <jsnajdr@git.wordpress.org>
Co-authored-by: Mamaduka <mamaduka@git.wordpress.org>
Co-authored-by: alecgeatches <alecgeatches@git.wordpress.org>

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

[jsnajdr]

The change reminded me of our previous conversation

Haha you remember everything 🙂 At the end of the comment I wrote:

In practice it's not so simple and there's some amount of complexity to untangle, but in principle it should work.

I don't remember what I meant by that, but today there is at least one caveat: the settings we send to the data store is not the raw object that was passed to initializeEditor, but it's enhanced inside EditorProvider with styles and callbacks like onNavigateToEntityRecord. So it's not so easy to move the updateEditorSettings call from the React component (EditorProvider) to the initializeEditor function.

[Mamaduka]

I think it’s doable to untangle. Styles should already be available on server and passed as raw settings. Navigation callback will need more tinkering.

Mostly want to close old issue with a fix when possible or with comment why change can’t be made.

Anyway, it’s not a blocker here, just something I remembered.

Unfortunately, the tests seem to be failing on this branch as well.

[jsnajdr]

Unfortunately, the tests seem to be failing on this branch as well.

Yes, that's expected, because I removed the stillHasRemoteAutosave check. Its purpose is to check whether we managed to set up the initial condition: there are both remote and local autosaves. Sometimes setting up the remote autosave fails, because of the modified race condition. Then the test silently gave up, acted as noop and succeeded. Without the check, it now fails.

If the test succeeds at setting up both autosaves, it checks the notices. And there was the updateEditorSettings bug that caused the notices to be displayed incorrectly.

If we don't manage to fix the setup problem, I'll add the check back, although it's a bad thing to have it there.

[alecgeatches]

The meta-save request probably shouldn't update any timestamps at all.

@jsnajdr Your change in 5dfbf96 looks fine to me! The purpose of the persistCRDTDoc call is to fix the server-persisted CRDT doc when we load a post and see that the content has changed, e.g. a WP-CLI command has added content to the post or a taxonomy/meta value has changed on server but not in the CRDT doc. We want both to match, so when a user loads in and determines there's a mismatch, the saveEntityRecord call fires with the intended side-effect of just updating CRDT meta. I don't think there would be any problems patching the timestamp to keep it stable. Another more invasive solution could be to detect CRDT meta updates server-side and then not update the post_modified flag to avoid overwriting an actual change, but that might be too case-specific.

@chubes4 only recently fixed this pathway to save the CRDT doc alone instead of the whole post which was even more disruptive.

[jsnajdr]

the saveEntityRecord call fires with the intended side-effect of just updating CRDT meta. I don't think there would be any problems patching the timestamp to keep it stable.

Patching the modified timestamp works for the moment, but I think an ideal solution will be a separate endpoint to save the CRTD document. Maybe in the wp-sync namespace, something like

POST /wp-json/wp-sync/save { "room": "postType/post:5", "doc": ... }

The reason is that saveEntityRecord, the REST endpoint, and the server-side PHP wp_update_post have the semantics that the user really saved a new version of the post, and modified it in some meaningful way. But saving the CRDT document is just a RTC bookkeeping, the user is not really saving anything. It's triggered even by purely read-only actions.

In wp-data, saveEntityRecord makes the UI think that the post is really saving, and it disables the "Publish" button for a moment, for example.

wp_update_post calls many popular filters and actions that plugins hook into, and the assumption is that we're saving a post.

With a separate endpoint we would avoid a lot of confusion. What do you think?

[jsnajdr]

@chubes4 only recently fixed this pathway to save the CRDT doc alone instead of the whole post

It seems there is a case where the payload is too small and doesn't even include the CRDT doc. Sometimes when creating a new post I see a request to update a category, where the payload includes only the id:

POST /wp/v2/categories/1 { id: 1 }

This is triggered by a getEntityRecord( 'taxonomy', 'category' ) read.

Taxonomies have a syncConfig, but it's the defaultSyncConfig one that doesn't have a getPersistedCRDTDoc, and also taxonomies don't have the __unstablePrePersist hook that would add the CRDT document to the empty payload.

What is the solution for this? Should categories have a persisted CRDT doc at all?

[jsnajdr]

Setting modified explicitly doesn't work, the REST API considers the modified field as read-only and that attempt to set it is ignored.

[alecgeatches]

Patching the modified timestamp works for the moment, but I think an ideal solution will be a separate endpoint to save the CRTD document. Maybe in the wp-sync namespace, something like

POST /wp-json/wp-sync/save { "room": "postType/post:5", "doc": ... }

The reason is that saveEntityRecord, the REST endpoint, and the server-side PHP wp_update_post have the semantics that the user really saved a new version of the post, and modified it in some meaningful way. But saving the CRDT document is just a RTC bookkeeping, the user is not really saving anything. It's triggered even by purely read-only actions.

In wp-data, saveEntityRecord makes the UI think that the post is really saving, and it disables the "Publish" button for a moment, for example.

@jsnajdr This sounds right to me, especially considering all of the side effects of saveEntityRecord() you mentioned. I opened a new issue in #78867 to track this. Does this seem necessary to fix the problems you're seeing in this PR, i.e. should we get this done pretty quickly?

[jsnajdr]

Does this seem necessary to fix the problems you're seeing in this PR, i.e. should we get this done pretty quickly?

Yes, this is causing at least two flaky e2e tests, this one, and also the Cancel button failures in #77721. The mechanism is similar in each case: the POST request with CRDT meta is an unexpected side effect that is observable in one way or another: changes the modified date, signals that the post is already saving, etc. I think there will be more flaky tests like this, it's just that so far I know about two.

If we don't fix this, I think people will soon discover that their modified dates are off, it's a very visible bug. Maybe it's already reported somewhere.

I also wanted to clarify the empty { id } payload when saving a category. Should categories (and other taxonomies) also have a CRDT document? They don't have a content attribute to edit.

[alecgeatches]

Does this seem necessary to fix the problems you're seeing in this PR, i.e. should we get this done pretty quickly?

Yes, this is causing at least two flaky e2e tests, this one, and also the Cancel button failures in #77721. The mechanism is similar in each case: the POST request with CRDT meta is an unexpected side effect that is observable in one way or another: changes the modified date, signals that the post is already saving, etc. I think there will be more flaky tests like this, it's just that so far I know about two.

If we don't fix this, I think people will soon discover that their modified dates are off, it's a very visible bug. Maybe it's already reported somewhere.

Okay, thanks for the clarification. I'll look into what's involved with changing our persistence endpoint.

I also wanted to clarify the empty { id } payload when saving a category. Should categories (and other taxonomies) also have a CRDT document? They don't have a content attribute to edit.

Categories/taxonomies are synced over Yjs, but they're never persisted as stored CRDT docs, so I think you've identified a bug. We're supposed to bail when an entity doesn't support meta, but it appears categories do have a meta property and are incorrectly getting the "persist CRDT" side-effect with no doc to save for no good reason. That should also be addressed in this work.

[alecgeatches]

@jsnajdr I started on a PR in #78891. There are a couple of snags with other side effects we relied on from saving, but overall it seems to be working well. I'll ping you on that PR when it's ready for review.

[jsnajdr]

I started on a PR in #78891. There are a couple of snags with other side effects we relied on from saving, but overall it seems to be working well.

Thanks, it works very well in my testing, and I posted a few review comments already.

[github-actions]

Flaky tests detected in 46005d7.
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/27752236766
📝 Reported issues:

• [Flaky Test] does not dirty related template parts or navigation menus when rendered in the post editor #79294 in /test/e2e/specs/editor/blocks/navigation-passive-rendering.spec.js

[jsnajdr]

Now after the new RTC save endpoint was merged in #78891, this PR finishes the fix for the flaky autosave test (#78706). There are four changes that fix various bugs in both UI code and the test itself:

1. In editor provider, update editor settings (including the settings.autosave value) in layout effect instead of regular effect. This ensures that child components see the correct value early, before the first paint. And also ensures that useEffect in LocalAutosaveMonitor runs after the settings update.
2. In useAutosaveNotice, read the hasRemoteAutosave value directly inside the useEffect callback. Don't use the potentially stale value that was read during render. This is a second part of fixing the bug where both remote and local autosave notices were shown. This change also fixes all warnings about useAutosaveNotice effect dependencies.
3. In the e2e test itself, make sure that autosave is triggered more than 1s after previous publish. I do it by making typing slower. It ensures that this condition on the server is always true: autosave modified date strictly bigger than the post modified date. Otherwise the autosave is not sent.
4. In the e2e test, remove a special guard about missing remote autosave. There is a comment that says "there is no server-provided autosave value available, despite our having previously explicitly autosaved. The reasons for this are still unknown." Now the reason is known, it's the timestamp mismatch described in the previous point.

[Mamaduka]

Looks great! Thank you, @jsnajdr!

I've linked the issue and PR, which should also be resolved and closed after this is merged.