Sync editor settings in layout effect (fixes autosave e2e)#78799
Open
jsnajdr wants to merge 2 commits into
Open
Sync editor settings in layout effect (fixes autosave e2e)#78799jsnajdr wants to merge 2 commits into
jsnajdr wants to merge 2 commits into
Conversation
|
Size Change: +16 B (0%) Total Size: 8.21 MB 📦 View Changed
ℹ️ View Unchanged
|
Member
|
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. |
|
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. |
Member
Author
Haha you remember everything 🙂 At the end of the comment I wrote:
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 |
Member
|
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. |
Member
Author
Yes, that's expected, because I removed the If the test succeeds at setting up both autosaves, it checks the notices. And there was the 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. |
Contributor
@jsnajdr Your change in 5dfbf96 looks fine to me! The purpose of the @chubes4 only recently fixed this pathway to save the CRDT doc alone instead of the whole post which was even more disruptive. |
Member
Author
Patching the The reason is that In
With a separate endpoint we would avoid a lot of confusion. What do you think? |
Member
Author
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 This is triggered by a Taxonomies have a What is the solution for this? Should categories have a persisted CRDT doc at all? |
Member
Author
|
Setting |
Contributor
@jsnajdr This sounds right to me, especially considering all of the side effects of |
Member
Author
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 If we don't fix this, I think people will soon discover that their I also wanted to clarify the empty |
Contributor
Okay, thanks for the clarification. I'll look into what's involved with changing our persistence endpoint.
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. |
Contributor
Member
Author
Thanks, it works very well in my testing, and I posted a few review comments already. |
6 tasks
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
3 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.
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:
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
But this doesn't work because of the following combination of reasons:
hasRemoteAutosavevalue is read inuseEffect, on mount, fromselect( editorStore ).getEditorSettings().autosave, i.e., the editor settings stored in data storeuseEffect, inExperimentalEditorProvider. 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
LocalAutosaveMonitoralways sees a false value of theautosavesetting, the check doesn't work.I'm fixing this by doing the
updateEditorSettingscall 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
stillHasRemoteAutosavecheck, 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
crdtDocumentto post meta, triggered by a supposedly read-onlygetEntityRecord. This request happens after the autosave. And the very bad thing about it is that it updates the "modified" timestamp of the post. When theedit-form-blocks.phppage 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