Fix post dirty-on-load in yjs-server (#18)

Author: alecgeatches

src/engines/yjs-server/engine.ts

@@ -10,6 +10,7 @@ import * as Y from 'yjs';
 import type {
 	EngineCollection,
 	EngineEntity,
+	ObjectData,
 	SyncEngine,
 } from '@wordpress/sync';
 
@@ -52,6 +53,9 @@ import {
  * - `getEditorChanges` reports nothing until bootstrap, so an empty
  *   pre-sync document can never be dispatched into the editor as a
  *   mass deletion.
+ * - After bootstrap, the dirtying `content` edit is withheld while the
+ *   document still serializes byte-identical to the loaded record, so
+ *   merely opening a post does not mark the editor dirty.
  *
  * After bootstrap the editor's blocks originate from this document's own
  * JSON, so steady-state diffs (`mergeCrdtBlocks`) are no-ops for
@@ -78,6 +82,22 @@ export function createYjsServerEngine(): SyncEngine {
 			const isBootstrapped = () =>
 				undefined !== stateMap.get( VERSION_KEY );
 
+			// Because hydrate() is a no-op, the server's genesis snapshot
+			// arrives as a REMOTE change whose content typically matches the
+			// loaded record byte-for-byte. The post sync config still reports
+			// it: its `blocks` case cannot compare document blocks to editor
+			// blocks (the two sides mint different block identities), so it
+			// reports `blocks` on every remote change and injects a fresh
+			// `content` serializer alongside. `blocks` is a transient edit,
+			// but `content` is not, so merely opening a post marked the
+			// editor dirty, activated the Save button, and scheduled
+			// autosaves of unchanged content. Until the document and the
+			// record first genuinely diverge, withhold that `content` edit
+			// whenever the reported blocks serialize byte-identical to the
+			// record's raw content. The `blocks` edit still dispatches so the
+			// editor adopts the document's block identities at bootstrap.
+			let docMayStillMatchRecord = true;
+
 			// Edits made before the server snapshot arrives, replayed in
 			// order once it does.
 			let pendingLocalChanges: Array< {
@@ -159,10 +179,37 @@ export function createYjsServerEngine(): SyncEngine {
 					applyChanges( changes, origin, Boolean( options.isSave ) );
 				},
 
-				getEditorChanges: ( editedRecord ) =>
-					isBootstrapped()
-						? syncConfig.getChangesFromCRDTDoc( ydoc, editedRecord )
-						: {},
+				getEditorChanges: ( editedRecord ) => {
+					if ( ! isBootstrapped() ) {
+						return {};
+					}
+
+					const changes = syncConfig.getChangesFromCRDTDoc(
+						ydoc,
+						editedRecord
+					);
+
+					if ( ! docMayStillMatchRecord ) {
+						return changes;
+					}
+
+					// An empty change set neither confirms nor refutes a
+					// match; leave the guard armed for the next dispatch.
+					if ( 0 === Object.keys( changes ).length ) {
+						return changes;
+					}
+
+					if (
+						isRedundantBootstrapDispatch( changes, editedRecord )
+					) {
+						const nonDirtyingChanges = { ...changes };
+						delete nonDirtyingChanges.content;
+						return nonDirtyingChanges;
+					}
+
+					docMayStillMatchRecord = false;
+					return changes;
+				},
 
 				encodeSnapshot: () => encodeDocSnapshot( ydoc ),
 
@@ -284,3 +331,83 @@ export function createYjsServerEngine(): SyncEngine {
 		},
 	};
 }
+
+/**
+ * Change-set keys that may appear in a redundant bootstrap dispatch. `blocks`
+ * and `selection` are transient (non-dirtying) entity edits; `content` is the
+ * injected serializer the bootstrap guard withholds. Any other key means the
+ * document genuinely diverges from the record.
+ */
+const REDUNDANT_DISPATCH_KEYS = new Set( [ 'blocks', 'content', 'selection' ] );
+
+/**
+ * Extract the raw content string from an edited record's `content` property,
+ * which is represented either as a plain string or as an object with a `raw`
+ * property. Returns undefined for any other shape, notably the lazy serializer
+ * function that replaces it once the editor has registered its own content
+ * edit.
+ *
+ * @param value The edited record's `content` property.
+ */
+function getRawContentString( value: unknown ): string | undefined {
+	if ( 'string' === typeof value ) {
+		return value;
+	}
+
+	if (
+		value &&
+		'object' === typeof value &&
+		'raw' in value &&
+		'string' === typeof value.raw
+	) {
+		return value.raw;
+	}
+
+	return undefined;
+}
+
+/**
+ * Determine whether a reported change set merely re-states what the editor
+ * already shows: the document's blocks serialize byte-identical to the
+ * record's raw content, and nothing besides blocks, the injected content
+ * serializer, and selection is reported. Such a dispatch carries no
+ * information the editor lacks except the document's block identities, which
+ * ride on the transient `blocks` edit alone.
+ *
+ * @param changes      Changes reported by the sync config.
+ * @param editedRecord The edited record the changes were computed against.
+ */
+function isRedundantBootstrapDispatch(
+	changes: ObjectData,
+	editedRecord: ObjectData
+): boolean {
+	const contentEdit = changes.content;
+	const recordContent = getRawContentString( editedRecord.content );
+
+	if (
+		! changes.blocks ||
+		'function' !== typeof contentEdit ||
+		'string' !== typeof recordContent
+	) {
+		return false;
+	}
+
+	const hasOnlyRedundantKeys = Object.keys( changes ).every( ( key ) =>
+		REDUNDANT_DISPATCH_KEYS.has( key )
+	);
+
+	if ( ! hasOnlyRedundantKeys ) {
+		return false;
+	}
+
+	// The injected serializer captures the reported blocks; invoking it here
+	// trades one serialization for the comparison the sync config cannot make
+	// itself (the document and the editor mint different block identities).
+	// The trim mirrors the sync config's own persisted-document comparison.
+	const serializedDocContent = contentEdit();
+
+	return (
+		'string' === typeof serializedDocContent &&
+		serializedDocContent.trim() === recordContent
+	);
+}

tests/e2e/specs/collaboration-yjs-server-engine.spec.ts

@@ -329,6 +329,67 @@ test.describe( 'Collaboration - yjs-server engine', () => {
 		] );
 	} );
 
+	test( 'merely opening a post does not mark it dirty', async ( {
+		collaborationUtils,
+		requestUtils,
+		page,
+	} ) => {
+		// Regression test for the bootstrap dirty state: the server's genesis
+		// snapshot arrives as a remote change, and the dispatch that swapped
+		// in the document's blocks used to register a dirtying `content` edit
+		// even though nothing changed, activating the Save button and the
+		// autosave timer on a post nobody touched.
+		const post = await requestUtils.createPost( {
+			title: 'Yjs Server Dirty On Open Test',
+			status: 'draft',
+			content:
+				'<!-- wp:paragraph -->\n<p>Untouched content</p>\n<!-- /wp:paragraph -->',
+			date_gmt: new Date().toISOString(),
+		} );
+
+		await collaborationUtils.openPost( post.id );
+
+		const getEditKeys = () =>
+			page.evaluate( ( postId ) => {
+				const edits = window.wp.data
+					.select( 'core' )
+					.getEntityRecordEdits( 'postType', 'post', postId );
+				return Object.keys( edits ?? {} );
+			}, post.id );
+
+		// Wait for the server's genesis snapshot to bootstrap the session:
+		// its dispatch registers the (transient, non-dirtying) `blocks` edit
+		// that swaps in the document's block identities.
+		await expect
+			.poll( getEditKeys, { timeout: 20000 } )
+			.toContain( 'blocks' );
+
+		// The bootstrap dispatch is exactly the moment the regression fired.
+		// Keep watching across further poll cycles to catch a delayed flip.
+		const becameDirty = await page.evaluate(
+			() =>
+				new Promise( ( resolve ) => {
+					const started = Date.now();
+					const interval = setInterval( () => {
+						const isDirty = window.wp.data
+							.select( 'core/editor' )
+							.isEditedPostDirty();
+						if ( isDirty ) {
+							clearInterval( interval );
+							resolve( true );
+						} else if ( Date.now() - started > 8000 ) {
+							clearInterval( interval );
+							resolve( false );
+						}
+					}, 200 );
+				} )
+		);
+		expect( becameDirty ).toBe( false );
+
+		// The dirtying half of the old dispatch must be gone for good.
+		expect( await getEditKeys() ).not.toContain( 'content' );
+	} );
+
 	test( 'title edits sync between users in both directions', async ( {
 		collaborationUtils,
 		requestUtils,

tests/js/engines/yjs-server/engine.test.ts

@@ -142,4 +142,149 @@ describe( 'createYjsServerEngine › createEntity', () => {
 
 		expect( onRemoteChange ).toHaveBeenCalled();
 	} );
+
+	describe( 'bootstrap dirty guard', () => {
+		const BLOCK_MARKUP =
+			'<!-- wp:paragraph -->\n<p>Hello</p>\n<!-- /wp:paragraph -->';
+
+		function makeBootstrappedEntity() {
+			const entity = makeEntity();
+			entity.hydrate( {} as any, jest.fn() );
+			entity.createSession().receiveUpdate( genesisRow() );
+			return entity;
+		}
+
+		/**
+		 * The change shape the framework's post sync config reports for a
+		 * live-document remote change: doc blocks plus an injected lazy
+		 * content serializer capturing them.
+		 *
+		 * @param serialized The string the injected serializer returns.
+		 */
+		function postShapedChanges( serialized: string ) {
+			return {
+				blocks: [ { name: 'core/paragraph' } ],
+				content: () => serialized,
+			};
+		}
+
+		it( 'withholds the injected content edit when doc blocks serialize identically to the record content', () => {
+			const entity = makeBootstrappedEntity();
+			syncConfig.getChangesFromCRDTDoc.mockReturnValue(
+				postShapedChanges( BLOCK_MARKUP )
+			);
+
+			const changes = entity.getEditorChanges( {
+				content: { raw: BLOCK_MARKUP },
+			} as any );
+
+			// The blocks still dispatch (transient; the editor adopts the
+			// document's block identities), but the dirtying content edit
+			// does not.
+			expect( changes.blocks ).toBeDefined();
+			expect( changes ).not.toHaveProperty( 'content' );
+		} );
+
+		it( 'accepts plain-string record content and tolerates trailing whitespace in the serialization', () => {
+			const entity = makeBootstrappedEntity();
+			syncConfig.getChangesFromCRDTDoc.mockReturnValue(
+				postShapedChanges( `${ BLOCK_MARKUP }\n` )
+			);
+
+			const changes = entity.getEditorChanges( {
+				content: BLOCK_MARKUP,
+			} as any );
+
+			expect( changes ).not.toHaveProperty( 'content' );
+		} );
+
+		it( 'keeps withholding across repeated redundant dispatches and empty change sets', () => {
+			const entity = makeBootstrappedEntity();
+			const editedRecord = { content: { raw: BLOCK_MARKUP } } as any;
+
+			syncConfig.getChangesFromCRDTDoc.mockReturnValue(
+				postShapedChanges( BLOCK_MARKUP )
+			);
+			expect(
+				entity.getEditorChanges( editedRecord )
+			).not.toHaveProperty( 'content' );
+
+			// An empty change set must not disarm the guard.
+			syncConfig.getChangesFromCRDTDoc.mockReturnValue( {} );
+			expect( entity.getEditorChanges( editedRecord ) ).toEqual( {} );
+
+			syncConfig.getChangesFromCRDTDoc.mockReturnValue(
+				postShapedChanges( BLOCK_MARKUP )
+			);
+			expect(
+				entity.getEditorChanges( editedRecord )
+			).not.toHaveProperty( 'content' );
+		} );
+
+		it( 'passes genuine divergence through and disarms the guard permanently', () => {
+			const entity = makeBootstrappedEntity();
+			const editedRecord = { content: { raw: BLOCK_MARKUP } } as any;
+
+			// A remote edit produced content the record does not have.
+			syncConfig.getChangesFromCRDTDoc.mockReturnValue(
+				postShapedChanges( `${ BLOCK_MARKUP }\n<!-- wp:more -->` )
+			);
+			const diverged = entity.getEditorChanges( editedRecord );
+			expect( typeof diverged.content ).toBe( 'function' );
+
+			// Even a later identical-looking dispatch is no longer filtered:
+			// steady-state behavior is restored for the rest of the session.
+			syncConfig.getChangesFromCRDTDoc.mockReturnValue(
+				postShapedChanges( BLOCK_MARKUP )
+			);
+			const settled = entity.getEditorChanges( editedRecord );
+			expect( typeof settled.content ).toBe( 'function' );
+		} );
+
+		it( 'does not withhold when the edited record already carries its own content edit', () => {
+			const entity = makeBootstrappedEntity();
+			syncConfig.getChangesFromCRDTDoc.mockReturnValue(
+				postShapedChanges( BLOCK_MARKUP )
+			);
+
+			// Once the user edits, the record's content is a lazy serializer
+			// function, not a raw string; the guard must stand aside.
+			const changes = entity.getEditorChanges( {
+				content: () => BLOCK_MARKUP,
+			} as any );
+
+			expect( typeof changes.content ).toBe( 'function' );
+		} );
+
+		it( 'does not withhold when other properties changed alongside blocks', () => {
+			const entity = makeBootstrappedEntity();
+			syncConfig.getChangesFromCRDTDoc.mockReturnValue( {
+				...postShapedChanges( BLOCK_MARKUP ),
+				title: 'Remote title',
+			} );
+
+			const changes = entity.getEditorChanges( {
+				content: { raw: BLOCK_MARKUP },
+			} as any );
+
+			expect( typeof changes.content ).toBe( 'function' );
+			expect( changes.title ).toBe( 'Remote title' );
+		} );
+
+		it( 'preserves a shifted selection when withholding the content edit', () => {
+			const entity = makeBootstrappedEntity();
+			const selection = { selectionStart: {}, selectionEnd: {} };
+			syncConfig.getChangesFromCRDTDoc.mockReturnValue( {
+				...postShapedChanges( BLOCK_MARKUP ),
+				selection,
+			} );
+
+			const changes = entity.getEditorChanges( {
+				content: { raw: BLOCK_MARKUP },
+			} as any );
+
+			expect( changes.selection ).toBe( selection );
+			expect( changes ).not.toHaveProperty( 'content' );
+		} );
+	} );
 } );