Repository navigation
Editor: Make autosave restorations non-destructive - #80504
sidhant-tomar-003 wants to merge 8 commits into
Conversation
|
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 Unlinked AccountsThe following contributors have not linked their GitHub and WordPress.org accounts: @gracie. Contributors, please read how to link your accounts to ensure your work is properly credited in WordPress releases. 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. |
|
👋 Thanks for your first Pull Request and for helping build the future of Gutenberg and WordPress, @sidhant-tomar-003! In case you missed it, we'd love to have you join us in our Slack community. If you want to learn more about WordPress development in general, check out the Core Handbook full of helpful information. |
|
The remote autosave is treated as a revision (#79947), and it has a less destructive UI for restoration, unless the user chooses to override the content. That's why I don't think changes in packages/editor/src/store/private-actions.js are required. The batching and undo for local autosaves are a nice touch; this could improve the UX. The e2e test failures are related to changes in this PR. |
…rwrite-fix # Conflicts: # packages/editor/src/store/private-actions.js
…ocal autosave UX improvements
e98d095 to
d24a585
Compare
|
@Mamaduka Hey just pushed changes to remove the remote autosave stuff. Now the only changes remaining are the batching and undo for local autosave 👍 |
priethor
left a comment
There was a problem hiding this comment.
Left a couple of comments. Small note: the PR description still mentions the remote autosave changes that were removed in the latest update.
| resetEditorBlocks( parse( edits.content ) ); | ||
| removeNotice( id ); | ||
|
|
||
| registry.batch( () => { |
There was a problem hiding this comment.
editPost and resetEditorBlocks seem to create separate core-data undo entries, even inside the batch, am I understanding it correctly? So if the backup includes changes to both the title and content, I think one click on Undo would only restore the content. If that’s right, could all the restored fields be grouped into a single undo step? It might also be worth covering title in the E2E test.
There was a problem hiding this comment.
Clubbed them together into one batch, and also added coverage for this in the E2E test.
Also tested and confirmed that the undo would revert both changes to post body and post title.
| onClick: () => { | ||
| registry | ||
| .dispatch( coreStore ) | ||
| .undo(); |
There was a problem hiding this comment.
Since this calls the global undo() when the button is clicked, could an edit made after the restore be undone by mistake? For example, restore the backup, insert a block, then click Undo in the snackbar.
If so, would it make sense to limit the action to the restore itself, or at least hide it once a newer edit is added to the undo history?
There was a problem hiding this comment.
Right, this was indeed an issue. Fixed this by dismissing the snackbar undo button if the user makes any changes to the post. The autosave changes will still be in the Undo queue so the user can still choose to unapply them.
Hope this fix works 👍
This commit addresses reviewer feedback by grouping `editPost` and `resetEditorBlocks` into a single `editPost` call in `local-autosave-monitor/index.js`. This ensures that both the content blocks and the title are grouped in a single core-data undo level, rather than being handled as separate steps. The E2E tests have been updated to cover title restoration as well.
When restoring an autosave, a snackbar with an 'Undo' action appears. Previously, if a user made a new edit while the snackbar was still visible, clicking 'Undo' would incorrectly revert the new edit (since it just fired the global undo action). This automatically dismisses the snackbar as soon as new edits are detected to prevent unintended undo actions. This also fixes an infinite dispatch loop that occurred when dismissing the snackbar.
| const postType = registry | ||
| .select( editorStore ) | ||
| .getCurrentPostType(); | ||
|
|
||
| const initialEdits = registry | ||
| .select( coreStore ) | ||
| .getEntityRecordNonTransientEdits( | ||
| 'postType', | ||
| postType, | ||
| postId | ||
| ); | ||
| const unsubscribe = registry.subscribe( () => { | ||
| const currentEdits = registry | ||
| .select( coreStore ) | ||
| .getEntityRecordNonTransientEdits( | ||
| 'postType', | ||
| postType, | ||
| postId | ||
| ); | ||
| if ( currentEdits !== initialEdits ) { | ||
| // Safely unsubscribe first to prevent loops | ||
| unsubscribe(); | ||
| // Defer dispatch to avoid nested dispatch issues | ||
| setTimeout( () => { | ||
| registry | ||
| .dispatch( noticesStore ) | ||
| .removeNotice( | ||
| 'wpEditorAutosaveRestored' | ||
| ); | ||
| }, 0 ); | ||
| } | ||
| } ); |
There was a problem hiding this comment.
I don't really love all this complexity (like usage of registry.subscribe). There should be a simpler way. Maybe we should go back to UX and see if communications can be improved in other ways.
There was a problem hiding this comment.
We could just leave it as it was, because if a user decided to make some changes within the 10 seconds of the undo button being on the snackbar, they probably weren't planning on using the button to begin with. And the changes do remain in queue for the session so theoretically the changes can still be reverted.
Other than that, I don't really see any way to have the Undo button on the Toast message only undo the autosave changes that would be simpler than this.
There was a problem hiding this comment.
Thanks, @sidhant-tomar-003!
I left a more general comment on issue #79777. I think there are other areas we can improve. No action required at the moment, just discussion and decision.
There was a problem hiding this comment.
Glad to help if needed regarding this 😄
This reverts the complex registry.subscribe and setTimeout logic introduced in the previous commit. Based on maintainer feedback, the complexity of manually intercepting undo actions and dismissing the snackbar was deemed too high. We are returning to the standard, simple global undo behavior.
What?
Closes #79777
Makes restoring both local (not remote autosaves) a non-destructive action. Users can now restore an autosave inline, view the changes in the editor, and undo them if needed, all before making a hard save to the server.
Why?
Previously, restoring an autosave was a destructive action. For local autosaves, it would replace the editor content silently with no way to undo. This caused users to accidentally lose active work.
How?
LocalAutosaveMonitorto group edits in aregistry.batch()and trigger a success snackbar with anUndobutton (coreStore.undo()).await dispatch.savePost()call fromrestoreRevisioninprivate-actions.js, allowing users to preview applied revisions before manually hitting "Update".Undofunctionality inautosave.spec.js.Testing Instructions
Testing Local Autosave Restore:
wp.data.dispatch( 'core/editor' ).autosave( { local: true } )in the console).Testing Instructions for Keyboard
Tabto navigate to the "Restore..." button and pressEnter.Tabto navigate to the "Undo" button inside the resulting snackbar and pressEnter. Verify the content reverts correctly.Screenshots or screencast
Use of AI Tools
This PR was authored with the assistance of an AI coding agent (Google Antigravity), and manually reviewed and tested by me.