RFC 108: Autosave MVP

[gasman]

Rendered

This RFC describes a minimum viable product for automatic background saving of pages and snippets, following preliminary work on concurrent editing notifications (RFC 95) and deferring validation when saving drafts (RFC 104).

[SebCorbin]

Have we excluded the ability to auto-save custom models too? Should it be possible to implement it rather easily through model/viewset mixins?

[gasman]

@SebCorbin Similarly to deferred validation (#12923), this functionality depends on the model extending DraftStateMixin, and the view code would most likely be implemented in wagtail.admin.generic.mixins.CreateEditViewOptionalFeaturesMixin, so technically it ought to work for any models / viewsets making use of those. However, we currently only formally support and document those features in the context of snippets, so I'm essentially referring to "snippets" in this RFC as a shorthand for "models extending DraftStateMixin and being edited through views extending CreateEditViewOptionalFeaturesMixin".

I know there's been requests to support optional features such as draft state on non-snippet models, and I'm not sure if @laymonage plans on acting on those - but FWIW I'm not really sold on the idea. Access to these optional features is the one remaining defining feature of "a snippet" as opposed to "a model" - asking for those features to be available outside of snippets is essentially asking for the concept of a snippet without it being called a snippet. And however much you might hate the name snippet, removing a name from a concept is surely a backward step.

[gasman]

We should outline what changes need to be made to the "you have unsaved changes" messaging. Even with autosave in place, we need to account for the window of time between saves, and the round-trip time from submitting a background POST to getting a response, and keep the "are you sure you want to navigate away" prompt if the user leaves the page before this completes.

On the other hand, we can probably remove the "you have unsaved changes" message in the footer - if autosave is active, then the unsaved changes will be short enough duration that there's no need for a notification, and if autosave is suspended due to a validation error or edit conflict, those will have their own notifications.

[laymonage]

I think it would be more useful to have something like "Last saved xx seconds/minutes ago". This gives you some idea that there might be changes that haven't been saved (if you made some changes within the given duration), without necessarily becoming a "warning" that the user needs to worry much about.

It's also still useful when there's a validation error or edit conflict. Although, with that in mind, such information might be better placed at the header rather than the footer. Not sure how busy the header will be if that were the case, though.

[laymonage]

I wonder if it's worth adding the backend code that allows rendering the form errors as <template data-controller="w-teleport"> elements, so that we could render them without a full-page reload. Might be a bit tricky for accessibility if the user is currently interacting with the form input that has an error, though. We did a similar trick for updating the editing sessions list in the header, so maybe some learnings from that could be applied.

Although this would probably need a different handling for StreamField as it uses client-side rendering.

[laymonage]

I think it would be more useful to have something like "Last saved xx seconds/minutes ago". This gives you some idea that there might be changes that haven't been saved (if you made some changes within the given duration), without necessarily becoming a "warning" that the user needs to worry much about.

It's also still useful when there's a validation error or edit conflict. Although, with that in mind, such information might be better placed at the header rather than the footer. Not sure how busy the header will be if that were the case, though.

[laymonage]

I think it would be great if we could, especially for live previews. Having both autosave and live previews would double the work on both the client and server.

The PreviewController already does a lot of similar logic: serialising the form, keeping the last state for comparison, and periodically sending the data to a backend endpoint. On the backend side, the preview view also performs validation on the form.

Not sure how feasible it is to do it as part of the MVP, though.

[laymonage]

Not sure about this and how the form state will be updated. I assume Telepath will be involved?

I'm still wondering whether we should lean more heavily towards server-side logic and rendering, i.e. adding HTMX-style swapping of form elements with the updated state from the server (perhaps with TeleportController), vs. leaning towards client-side with Telepath & co.

The current situation isn't ideal as we duplicate the markup in both Django templates and our homegrown JS components like https://github.com/wagtail/wagtail/blob/034f71c43887e614802a9f643ace3b4976923df3/client/src/components/StreamField/blocks/FieldBlock.js#L19-L32

[laymonage]

Just thinking out loud here: maybe there's value in storing the last timestamp (or counter that's always incremented in the client) of a successful autosave on the server and returning it to the client, to be used by the client for the next submission, so the server knows whether there was a blip since the last submission?

Might also be something that we can fit into/rework from the existing EditingSession model, as it has similar fields.

[laymonage]

Since overwrite_revision_id is only populated after the first successful autosave, this means there's still a chance of conflict if someone has saved a new revision since the editor was loaded, but before the autosave request is sent.

With the approach described in the current RFC, the server will happily create a new revision, which the client will use as overwrite_revision_id, ignoring the new revision made by the other session.

The "concurrent editing notifications" feature #95 can help mitigate this, but only if the "ping" request has been sent to detect the new revision before the first autosave. However, given that the feature works at a configurable interval (and can be turned off), we cannot rely on it.

If the desired behaviour is to prevent conflict in such cases, we need to include an optional POST value loaded_revision_id for the revision ID that was loaded when the form was rendered.

Then, assuming overwrite_revision_id is always newer than loaded_revision_id, we detect conflicts with the following logic:

submitted_revision_id = request.POST.get("overwrite_revision_id") or request.POST.get("loaded_revision_id")
has_conflict = submitted_revision_id and (submitted_revision_id != str(page.latest_revision_id))

In the non-autosaving case, the loaded_revision_id is still useful to fix wagtail/wagtail#2565.

[laymonage]

The above change has now been implemented in wagtail/wagtail#13684.

However, the above isn't completely bulletproof:

We only check for the loaded_revision_id, but we don't check whether that revision hasn't been updated at the time of saving. Consider the following scenario:

1. User A loaded the editor and made a successful autosave, setting their overwrite_revision_id to 2.
2. User B loaded the editor afterwards, so their loaded_revision_id is 2.
3. User A made further edits, so revision with ID 2 has now been updated (thus becomes different from what user B loaded).
4. User B made some changes, triggering an autosave request with loaded_revision_id = 2 and overwrite_revision_id = null.

In this case, the server will happily create revision with ID 3 as User B's overwrite_revision_id. User A will be prevented from making further autosave (as their overwrite_revision_id = 2 is older than the latest_revision_id = 3). However, in step 4, User B would have unknowingly overwritten User A's changes in step 3.

Concurrent editing notifications won't help in this case, as it suffers from the same issue – it assumes the initially loaded revision 2 hasn't changed.

To help mitigate this, we need something else other than the ID to discriminate between an untouched revision vs an overwritten one. We could make use of the created_at timestamp (which gets updated when the revision is overwritten), or introduce a new field (e.g. an overwrite counter, or a UUID).