Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Investigate content loss issues after connection loss/long period of time. #23781

Closed
youknowriad opened this issue Jul 8, 2020 · 15 comments
Closed
Assignees
Labels
[Feature] History History, undo, redo, revisions, autosave. [Feature] Saving Related to saving functionality [Type] Bug An existing feature does not function as intended

Comments

@youknowriad
Copy link
Contributor

I've had an interaction with a Gutenberg user that experienced content loss while using the editor. Here are the potential steps to reproduce the issue.

  • Write a long post (and scroll to the bottom to potentially hide notices that appear at the top of the page);
  • Cut the internet connection or let it sit for a while.
  • A notice shows up but not visible enough.

At this point, the content loss happens. It's not really clear what caused it. The notice might have been dismissed, switching to writing other posts/tabs could be a factor too.

Expected Result

  • Ideally, the content loss doesn't happen as we have local storage saving.
  • Also, the notice is not visible or clear enough, it should be more prominent and indicate the potential content loss. Maybe it should be non-dismissible.
@youknowriad youknowriad added [Type] Bug An existing feature does not function as intended Needs Testing Needs further testing to be confirmed. [Feature] Saving Related to saving functionality labels Jul 8, 2020
@adamziel adamziel self-assigned this Jul 9, 2020
@adamziel
Copy link
Contributor

What I managed to reproduce:

  1. I created a very long post and ran wp-env stop. The notice is there regardless of the scroll position, although it is dismissable:

Zrzut ekranu 2020-07-14 o 13 23 04

  1. I tried refreshing the page, there was another notice:

Zrzut ekranu 2020-07-14 o 13 23 33

  1. Upon refreshing, my post was no longer there - apparently local storage saving did not work:

Zrzut ekranu 2020-07-14 o 13 24 15

@adamziel
Copy link
Contributor

adamziel commented Jul 14, 2020

The autosave worked and the content is present in window.sessionStorage under some ID. The issue is that WordPress creates a new draft post every time the post editor is opened, which means a new ID, which means the autosave code uses a different cache key after refreshing the page:

Zrzut ekranu 2020-07-14 o 13 33 17

Post 375 is not available for me from wp-admin, but If I manually navigate to post.php?post=375&action=edit then I am notified about backup being available:

Zrzut ekranu 2020-07-14 o 13 36 14

Clicking restore brings my post back:

Zrzut ekranu 2020-07-14 o 13 36 25

It seems like sessionStorage autosave should take wasSaved flag into account and then offer to restore a backup even if the post id doesn't match.

@adamziel adamziel added the [Feature] History History, undo, redo, revisions, autosave. label Jul 14, 2020
@youknowriad
Copy link
Contributor Author

cc @mcsf

Good debugging, thanks @adamziel It does seem like a single auto-draft local save is the way to go regardless of the id.

That said, I'm not sure if it's the only issue because unless I'm mistaken the "auto-save" triggers after a minute which means it's unlikely that the long post is still an auto-draft when the error happens.

@adamziel
Copy link
Contributor

adamziel commented Jul 14, 2020

@youknowriad I kept digging and it also seems like the AutosaveMonitor isn't overly reliable:

toggleTimer( isPendingSave ) {
const { interval, shouldThrottle = false } = this.props;
// By default, AutosaveMonitor will wait for a pause in editing before
// autosaving. In other words, its action is "debounced".
//
// The `shouldThrottle` props allows overriding this behaviour, thus
// making the autosave action "throttled".
if ( ! shouldThrottle && this.pendingSave ) {
clearTimeout( this.pendingSave );
delete this.pendingSave;
}
if ( isPendingSave && ! ( shouldThrottle && this.pendingSave ) ) {
this.pendingSave = setTimeout( () => {
this.props.autosave();
delete this.pendingSave;
}, interval * 1000 );
}
}

If I'm constantly typing, this fragment will kick in every now and then and reschedule the autosave +1 minute each time it runs. I also noticed it tends to be non-deterministic: there are times where I keep typing and the initial autosave kicks in after the first minute, and there are times where I keep typing and no autosave would kick in for 10 or more minutes.

I noticed the debounce mode was added in this PR by @aduth and @mcsf:

#16490

Is there any specific reason we need that debounce? I can't think of a use case where I'd like to delay the autosave indefinitely as long as I keep typing - quite the opposite, I'd like it to preserve as much of my typing as possible. I would get rid of it and just let it run every interval seconds. cc @gziolo who reviewed that other PR

@youknowriad
Copy link
Contributor Author

@adamziel I think at the time the debounce was added, the threshold was 10 seconds and not one minute and running auto-saves each 10 seconds is too much and it can lead to a high number of revisions which is bad for WP mysql performance I think.

One option could be to have two timeouts, a debounced one (1 minute) and non-debounced one (like 10mn or so).

Any thoughts @mtias @mcsf

@adamziel
Copy link
Contributor

It would also be good to display the "you are offline" notice as soon as either an autosave OR a heartbeat fails, currently it only shows in response to an autosave failure.

@adamziel
Copy link
Contributor

adamziel commented Jul 14, 2020

@youknowriad What is the approach to the post history? Is it supposed to be immutable? I wonder if it would make sense to have a non-debounced 1-minute autosave that would create a new revision once every 10 runs and amend the last autosaved revision the other 9 times.

@mcsf
Copy link
Contributor

mcsf commented Jul 14, 2020

Great debugging, @adamziel!

What is the approach to the post history? Is it supposed to be immutable? I wonder if it would make sense to have a non-debounced 1-minute autosave that would create a new revision once every 10 runs and amend the last autosaved revision the other 9 times.

I'm speaking from memory and intuition, but I believe that traditionally consecutive autosaves are squashed together, while explicit user-triggered saves always create a new revision.

So a flow of

save draft | autosave | autosave | autosave | save draft | autosave

will result in the revision history:

rev | rev (autosave) | rev | rev (autosave)

One option could be to have two timeouts, a debounced one (1 minute) and non-debounced one (like 10mn or so).

This might work. We should also look to make sure that the debounce fuse is good, i.e. should we shorten the idle time required to force an autosave?

Would it help to consider the first (server-side) autosave more important than the following ones? The first autosave is crucial to obtain a reliable post ID with which to save/restore saves. So maybe we aggressively autosave a draft after one minute even if the editor is not idle. From that point, we know that we'll have sessionStorage autosaves at the very least. WDYT?

@adamziel
Copy link
Contributor

adamziel commented Jul 14, 2020

Would it help to consider the first (server-side) autosave more important than the following ones? The first autosave is crucial to obtain a reliable post ID with which to save/restore saves. So maybe we aggressively autosave a draft after one minute even if the editor is not idle. From that point, we know that we'll have sessionStorage autosaves at the very least. WDYT?

#23928 would solve the sessionStorage issue for posts without a "reliable" post ID.

I'm speaking from memory and intuition, but I believe that traditionally consecutive autosaves are squashed together, while explicit user-triggered saves always create a new revision.

In that case, what's the advantage of debounced autosave over the "always run every 60 seconds" model? It seems like the number of revisions would be pretty reasonable in both cases.

@mcsf
Copy link
Contributor

mcsf commented Jul 14, 2020

In that case, what's the advantage of debounced autosave over the "always run every 60 seconds" model? It seems like the number of revisions would be pretty reasonable in both cases.

Yes, I agree. The poor answer is that the debouncing mechanism was already around before I touched Autosave. I'm tempted to say that there's currently no good reason to keep it that way, but of course I may be missing some history. Ultimately, I think it's worth reimplementing.

#23928 would solve the sessionStorage issue for posts without a "reliable" post ID.

I think it's a fine start, but it's worth pointing out that it breaks down as soon as a user has two new drafts sitting side by side. So we're left with a couple of options:

  • Strive to get an ID from the server ASAP.
  • Build some much more involved user interactions and underlying logic allowing the editor to keep concurrent auto-draft backups in session storage, and afterwards presenting the user with a UI allowing them to recover the right auto-draft.

@mtias
Copy link
Member

mtias commented Jul 14, 2020

Apart from the improvements to the logic, this seems like a case of poor UI where we could just surface any local-drafts whenever you start a new post, with the ability to discard them. This would mean that even if the connection between the IDs gets lost, a user would still be able to browse local copies and resurrect a text.

@adamziel
Copy link
Contributor

Apart from the improvements to the logic, this seems like a case of poor UI where we could just surface any local-drafts whenever you start a new post, with the ability to discard them. This would mean that even if the connection between the IDs gets lost, a user would still be able to browse local copies and resurrect a text.

This could use some design input so I created a new issue here #23955

@adamziel
Copy link
Contributor

Early progress on AutosaveMonitor refactor: #23962

@getdave getdave removed the Needs Testing Needs further testing to be confirmed. label Sep 7, 2020
@adamziel
Copy link
Contributor

adamziel commented Nov 3, 2020

Documentation for the AutosaveMonitor proposed in #26663

@adamziel
Copy link
Contributor

adamziel commented Nov 3, 2020

Closing this one since #23955 is a separate issue and I haven't seen any more reports of the problem described here.

@adamziel adamziel closed this as completed Nov 3, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] History History, undo, redo, revisions, autosave. [Feature] Saving Related to saving functionality [Type] Bug An existing feature does not function as intended
Projects
None yet
Development

No branches or pull requests

5 participants