Skip to content

Real-time collaboration: Add more robust mergeCrdtBlocks() testing #74304

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.

Sign up for GitHub

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

Merged
chriszarate merged 5 commits into from
Jan 5, 2026
Merged

Real-time collaboration: Add more robust mergeCrdtBlocks() testing #74304

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
1e9d100
Add tests for mergeCrdtBlocks() edge cases
alecgeatches Dec 31, 2025
edb9b8e
Fix potential bug if merged attribute has type change
alecgeatches Dec 31, 2025
9a652b0
Fix clientId deduping across block nesting levels
alecgeatches Dec 31, 2025
cdfe518
Move type checking in block attribute update to separate more-flexibl…
alecgeatches Jan 5, 2026
b283ed9
Separate type-specific attribute logic into updateYBlockAttribute()
alecgeatches Jan 5, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
192 changes: 139 additions & 53 deletions packages/core-data/src/utils/crdt-blocks.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -193,6 +193,26 @@ export function mergeCrdtBlocks(
yblocks: YBlocks,
incomingBlocks: Block[],
cursorPosition: number | null
): void {
// Perform the merge
mergeCrdtBlocksInternal( yblocks, incomingBlocks, cursorPosition );

// Remove duplicate clientIds across all nesting levels (at top level)
removeDuplicateClientIds( yblocks );
}

/**
* Internal function to merge blocks without any top-level checks.
* Called recursively for innerBlocks.
*
* @param yblocks The blocks in the local Y.Doc.
* @param incomingBlocks Gutenberg blocks being synced.
* @param cursorPosition The position of the cursor after the change occurs.
*/
function mergeCrdtBlocksInternal(
yblocks: YBlocks,
incomingBlocks: Block[],
cursorPosition: number | null
): void {
// Ensure we are working with serializable block data.
if ( ! serializableBlocksCache.has( incomingBlocks ) ) {
Expand Down Expand Up @@ -282,43 +302,29 @@ export function mergeCrdtBlocks(

Object.entries( value ).forEach(
( [ attributeName, attributeValue ] ) => {
if (
fastDeepEqual(
currentAttributes?.get( attributeName ),
attributeValue
)
) {
return;
}

const currentAttribute =
currentAttributes.get( attributeName );
const isRichText = isRichTextAttribute(

const isExpectedType = isExpectedAttributeType(
block.name,
attributeName
attributeName,
currentAttribute
);
Comment on lines +305 to +312

@chriszarate chriszarate Jan 2, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we conceivably have other attribute type transitions that we need to expect? Is there another more flexible approach that would allow us to add them in the future (and mark the location accordingly)?

@alecgeatches alecgeatches Jan 5, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed in cdfe518, let me know what you think! This adds isExpectedAttributeType() which is a less brittle way of doing the same check, and modifies our block attribute cache to store all expected block types, not just those for rich text.

@alecgeatches alecgeatches Jan 5, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, let me add one more change to do the same for the isRichText if statement.

@alecgeatches alecgeatches Jan 5, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added some more changes in b283ed9 to move type-specific logic into another function.


if (
isRichText &&
'string' === typeof attributeValue &&
currentAttributes.has( attributeName ) &&
currentAttribute instanceof Y.Text
) {
// Rich text values are stored as persistent Y.Text instances.
// Update the value with a delta in place.
mergeRichTextUpdate(
const isAttributeChanged =
! isExpectedType ||
! fastDeepEqual(
currentAttribute,
attributeValue,
cursorPosition
attributeValue
);
} else {
currentAttributes.set(

if ( isAttributeChanged ) {
updateYBlockAttribute(
block.name,
attributeName,
createNewYAttributeValue(
block.name,
attributeName,
attributeValue
)
attributeValue,
currentAttributes,
cursorPosition
);
}
}
Expand All @@ -345,7 +351,7 @@ export function mergeCrdtBlocks(
yblock.set( key, yInnerBlocks );
}

mergeCrdtBlocks(
mergeCrdtBlocksInternal(
yInnerBlocks,
value ?? [],
cursorPosition
Expand Down Expand Up @@ -375,23 +381,36 @@ export function mergeCrdtBlocks(

yblocks.insert( left, newBlock );
}
}

// remove duplicate clientids
const knownClientIds = new Set< string >();
/**
* Recursively remove duplicate clientIds from blocks and their innerBlocks.
*
* @param yblocks The blocks to check.
* @param knownClientIds Set of clientIds seen so far.
*/
function removeDuplicateClientIds(
yblocks: YBlocks,
knownClientIds: Set< string > = new Set()
): void {
for ( let j = 0; j < yblocks.length; j++ ) {
const yblock: YBlock = yblocks.get( j );

let clientId = yblock.get( 'clientId' );

if ( ! clientId ) {
continue;
if ( clientId ) {
Comment on lines -386 to +401

@chriszarate chriszarate Dec 31, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this logic inverted for any particular reason?

@alecgeatches alecgeatches Dec 31, 2025 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This just allows the recursion on innerBlocks to run even if the current block is missing a client ID. I'm not clear on when a clientId will be missing from a block or if that's even a real possibility, but it was accounted for in the original code. Do you have any insight on when this happens? Should we also bail on innerBlocks in this unusual case?

@chriszarate chriszarate Jan 1, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No particular insight. IIRC it was copied over from Kevin's initial implementation.

@alecgeatches alecgeatches Jan 2, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This bit of code had to change to accommodate client ID deduping of the whole tree, and I think this change makes sense in that context. I can also remove this small overhaul if you want to keep it the same, but it made more sense to me to dedupe across more than one level at a time.

if ( knownClientIds.has( clientId ) ) {
clientId = uuidv4();
yblock.set( 'clientId', clientId );
}
knownClientIds.add( clientId );
}

if ( knownClientIds.has( clientId ) ) {
clientId = uuidv4();
yblock.set( 'clientId', clientId );
// Recursively check innerBlocks
const yInnerBlocks = yblock.get( 'innerBlocks' );
if ( yInnerBlocks instanceof Y.Array ) {
removeDuplicateClientIds( yInnerBlocks, knownClientIds );
}
knownClientIds.add( clientId );
}
}

Expand Down Expand Up @@ -428,45 +447,112 @@ function shouldBlockBeSynced( block: Block ): boolean {
return true;
}

function updateYBlockAttribute(
blockName: string,
attributeName: string,
attributeValue: unknown,
currentAttributes: YBlockAttributes,
cursorPosition: number | null
): void {
const isRichText = isRichTextAttribute( blockName, attributeName );
const currentAttribute = currentAttributes.get( attributeName );

if (
isRichText &&
'string' === typeof attributeValue &&
currentAttributes.has( attributeName ) &&
currentAttribute instanceof Y.Text
) {
// Rich text values are stored as persistent Y.Text instances.
// Update the value with a delta in place.
mergeRichTextUpdate( currentAttribute, attributeValue, cursorPosition );
} else {
currentAttributes.set(
attributeName,
createNewYAttributeValue( blockName, attributeName, attributeValue )
);
}
}

// Cache rich-text attributes for all block types.
let cachedRichTextAttributes: Map< string, Map< string, true > >;
let cachedBlockAttributeTypes: Map< string, Map< string, string > >;

/**
* Given a block name and attribute key, return true if the attribute is rich-text typed.
* Get the defined attribute type for a block attribute.
*
* @param blockName The name of the block, e.g. 'core/paragraph'.
* @param attributeName The name of the attribute to check, e.g. 'content'.
* @return True if the attribute is rich-text typed, false otherwise.
* @param attributeName The name of the attribute, e.g. 'content'.
* @return The type of the attribute, e.g. 'rich-text' or 'string'.
*/
function isRichTextAttribute(
function getBlockAttributeType(
blockName: string,
attributeName: string
): boolean {
if ( ! cachedRichTextAttributes ) {
): string | undefined {
if ( ! cachedBlockAttributeTypes ) {
// Parse the attributes for all blocks once.
cachedRichTextAttributes = new Map< string, Map< string, true > >();
cachedBlockAttributeTypes = new Map< string, Map< string, string > >();

for ( const blockType of getBlockTypes() as BlockType[] ) {
const richTextAttributeMap = new Map< string, true >();
const blockAttributeTypeMap = new Map< string, string >();

for ( const [ name, definition ] of Object.entries(
blockType.attributes ?? {}
) ) {
if ( 'rich-text' === definition.type ) {
richTextAttributeMap.set( name, true );
if ( definition.type ) {
blockAttributeTypeMap.set( name, definition.type );
}
}

cachedRichTextAttributes.set(
cachedBlockAttributeTypes.set(
blockType.name,
richTextAttributeMap
blockAttributeTypeMap
);
}
}

return (
cachedRichTextAttributes.get( blockName )?.has( attributeName ) ?? false
return cachedBlockAttributeTypes.get( blockName )?.get( attributeName );
}

/**
* Check if an attribute value is the expected type.
*
* @param blockName The name of the block, e.g. 'core/paragraph'.
* @param attributeName The name of the attribute, e.g. 'content'.
* @param attributeValue The current attribute value.
* @return True if the attribute type is expected, false otherwise.
*/
function isExpectedAttributeType(
blockName: string,
attributeName: string,
attributeValue: unknown
): boolean {
const expectedAttributeType = getBlockAttributeType(
blockName,
attributeName
);

if ( expectedAttributeType === 'rich-text' ) {
return attributeValue instanceof Y.Text;
} else if ( expectedAttributeType === 'string' ) {
return typeof attributeValue === 'string';
}

// No other types comparisons use special logic.
return true;
}

/**
* Given a block name and attribute key, return true if the attribute is rich-text typed.
*
* @param blockName The name of the block, e.g. 'core/paragraph'.
* @param attributeName The name of the attribute to check, e.g. 'content'.
* @return True if the attribute is rich-text typed, false otherwise.
*/
function isRichTextAttribute(
blockName: string,
attributeName: string
): boolean {
return 'rich-text' === getBlockAttributeType( blockName, attributeName );
}

let localDoc: Y.Doc;
Expand Down
Loading
Loading