RTC: Fix empty revision from peer autosave - #79911
Conversation
alecgeatches
added
[Type] Bug
|
Size Change: 0 B Total Size: 7.72 MB |
|
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. |
alecgeatches
added
the
No Core Sync Required
|
Flaky tests detected in 88eb4af. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/28817030704
|
|
Put back into draft. I'm not 100% sold on the fix video: revision-diff-fixed.movIt's a bit weird to say "there's newer content" and show an autosave with the same content as the current page (~:35 in the video), even if the autosave contains a diff newer than the saved post content. It's more correct than showing an empty autosave on the revisions page, but I want to think about this a bit more. |
|
Okay, put this back up for review. I think the comment above is correct, but we should still merge this PR anyway. The code in this PR still correctly removes the server-side logic causing "blank" (exactly equal) autosaves. Separately, the autosave notice should probably be changed in RTC to use a different metric for showing itself. Even though a newer autosave might exist when collaboratively editing a document, the CRDT usually will already contain the new content too, so the terminology and display is confusing. We should likely gate the notice under something more robust than a more recent autosave timestamp and compare content. This PR addresses the server-side fix and unnecessary autosave, let's address the header in another PR. |
ingeniumed
left a comment
ingeniumed
left a comment
There was a problem hiding this comment.
The code itself is good, just need to add 2 more tests and tweak a doc.
| * the correct baseline; otherwise (no revisions yet, or the parent was updated | ||
| * directly) the parent post is used. | ||
| * | ||
| * @since 7.0.0 |
| return $latest_revision; | ||
| } | ||
|
|
||
| return $post; |
There was a problem hiding this comment.
There's currently a test missing for the case where the parent post is newer than the revision that's found. This case exists when collaboration isn't enabled but not for when it is. Worth adding that in?
| // and an empty string as equivalent so the editor's empty default for | ||
| // an unset key is not mistaken for a change. | ||
| $old_meta = get_metadata_raw( 'post', $post->ID, $meta_key, true ) ?? ''; | ||
| $old_meta = get_metadata_raw( 'post', $baseline->ID, $meta_key, true ) ?? ''; |
There was a problem hiding this comment.
Currently the existing post meta test only compares the changed post meta with the parent, not with the latest revision so this code isn't being tested for that case. Would add another test for that case
|
@ingeniumed Thank you for the review! I added 2 tests, updated versions (to |
2 participants
What?
Follow-up to #79591, which fixed single-user "empty" autosaves. There's another workflow from multiple users (see
collaboration-peer-blank-revision.spec.ts) that can result in the same outcome:multi-revision-reproduction.mov
(middle sped up to wait for autosaves)
The expected behavior should be to show the
- editcontent diff, but instead the revisions page shows an empty save.This PR changes the autosave controller to compare an incoming autosave against the most recent revision rather than the parent post.
Why?
#79591 fixed the single-user case, but autosaves from multiple users in collaborative sessions can still result in no-op revisions. That PR guarded the first autosave for a user and delegated every subsequent autosave to core with the assumption that core already de-duplicates. However, that's a bad assumption.
Both #79591 and core's
create_post_autosave()compare the incoming autosave against the parent post. But with RTC, the parent draft is intentionally never updated as part of auto-saving. Peer edits are stored as revisions, regardless of whether the user is the original post author. Peers can share the same CRDT content but that content differs from the parent post until there's a real save, so each peer's autosave "looks different from the parent". Those revisions are still identical to one another, and look "blank" as described in #79591:How?
In short, don't create an autosave if there's already an autosave with the same content.
The autosave controller now determines a comparison baseline instead of always using the parent post before persisting a new revision:
get_autosave_comparison_baseline()returns the most recent revision when it is newer than the parent, where RTC autosaves are stored. This falls back to the parent when no revision exists yet or in non-RTC.is_redundant_autosave()compares the revisioned fields (title, content, excerpt) and revisioned meta (e.g.footnotes) against that baseline.The baseline can be another peer's autosave revision, and that is intentional. Under RTC all peers converge, so the most recent revision is the best snapshot of the shared state regardless of author. A match means the content is already persisted, so there's no point in creating a second revision with exactly the same content.
Testing Instructions
First, try a reproduction in
trunk:- edited) and DO NOT SAVERepeat on this branch (
fix/rtc-multi-user-empty-autosave) and the only autosave shown should be the actual diff from the unsaved content, not a subsequent empty diff:revision-diff-fixed.mov
Automated testing
See test failure for
collaboration-peer-blank-revision.spec.tsin this run, and test passing after the fix was added in this branch.Use of AI Tools
Claude (Opus 4.8) for reproduction and drafting the fix.