Editor: Avoid scheduling autosave if same edit reference as last completion

[aduth]

Fixes #12318

This pull request seeks to resolve an issue where autosaves will repeatedly occur when an edit is made to a published post. As explained in #12318 (comment) , this is a consequence of a published post's autosave occurring as a revision, not a proper save. The post is still in an "unsaved" state, in the sense that the user has not yet Updated the post.

With these changes, an autosave will not be scheduled after a prior autosave had completed until an explicit edit occurs, even if the post is dirty or autosaveable.

Implementation notes:

For posterity's sake, there are some specific intricacies of the autosave flow which may appear to be redundant with the logic included here, but which still serve an independent role. Namely, the re-dirtying of an autosaved published post performed as a consequence of:

gutenberg/packages/editor/src/store/effects/posts.js

Lines 143 to 150 in 8743217

	optimist: {
	// Note: REVERT is not a failure case here. Rather, it
	// is simply reversing the assumption that the updates
	// were applied to the post proper, such that the post
	// treated as having unsaved changes.
	type: isRevision ? REVERT : COMMIT,
	id: POST_UPDATE_TRANSACTION_ID,
	},

gutenberg/packages/editor/src/store/selectors.js

Lines 124 to 150 in 8743217

	/**
	* Returns true if there are unsaved values for the current edit session, or
	* false if the editing state matches the saved or new post.
	*
	* @param {Object} state Global application state.
	*
	* @return {boolean} Whether unsaved values exist.
	*/
	export function isEditedPostDirty( state ) {
	if ( hasChangedContent( state ) ) {
	return true;
	}
	// Edits should contain only fields which differ from the saved post (reset
	// at initial load and save complete). Thus, a non-empty edits state can be
	// inferred to contain unsaved values.
	if ( Object.keys( state.editor.present.edits ).length > 0 ) {
	return true;
	}
	// Edits and change detectiona are reset at the start of a save, but a post
	// is still considered dirty until the point at which the save completes.
	// Because the save is performed optimistically, the prior states are held
	// until committed. These can be referenced to determine whether there's a
	// chance that state may be reverted into one considered dirty.
	return inSomeHistory( state, isEditedPostDirty );
	}

To reiterate, it is correct that a post is considered dirty after a published post is autosaved. The user should be prompted when attempting to navigate away. It's because of this distinction that an additional "has autosaved for edits reference" is included here.

It may, however, be worth considering for future refactoring to remove the isEditedPostAutosaveable selector in favor of dirtiness conditions, or reflecting the last autosave which had occurred. Currently, the recurring autosave occurs because hasChangedContent will return true due to the re-dirtying behavior described above.

Testing instructions:

Repeat steps to reproduce from #12318, verifying by Network DevTools activity that only a single autosave occurs for a single edit to a published post.

Verify that after an autosave occurs, continued edits will still autosave.

Pending:

This is worth testing via an end-to-end test.

[tofumatt]

This is kinda confusing logic to follow and I don't quite understand the test, but maybe if explained a bit more I would. The code seems fine but I don't grok the test.

[tofumatt]

Maybe I'm mis-reading the test, but I would expect this to test for something different. Now the post is dirty, isn't autosaving, is autosavable, and has a different editsReference, but the expectation is the same.

I don't quite follow 🤷‍♂️

[aduth]

Maybe I'm mis-reading the test, but I would expect this to test for something different.

You're totally right. We expect toggleTimer to have been called with true, which it is, but as written toHaveBeenCalledWith would consider both this and the prior invocation (since we didn't clear mock calls between). So while it's true that it had been called with false, this was not what we're testing.

I've updated the test so that it explicitly checks that the second time around, it's called with true.

[aduth]

In case it has an impact on how people prioritize reviewing this: Without this change, published posts will currently autosave infinitely after an initial change is made to a block.

[youknowriad]

It definitely does feel like this selector is useless or doesn't have the same meaning as its name.

[youknowriad]

Thanks for the fix.