diff --git a/packages/editor/src/components/collab-sidebar/suggestion-actions.js b/packages/editor/src/components/collab-sidebar/suggestion-actions.js index 0baaf20e2107dd..1d5435612fc589 100644 --- a/packages/editor/src/components/collab-sidebar/suggestion-actions.js +++ b/packages/editor/src/components/collab-sidebar/suggestion-actions.js @@ -109,7 +109,11 @@ function useSuggestionDecision( thread ) { const onReject = async () => { setBusy( true ); try { - await rejectSuggestion( { commentId: thread.id } ); + await rejectSuggestion( { + commentId: thread.id, + clientId: thread.blockClientId, + payload, + } ); } catch { // Notice surfaced by the provider. } finally { diff --git a/packages/editor/src/components/suggestion-mode/auto-save.js b/packages/editor/src/components/suggestion-mode/auto-save.js index c023452ef18782..d271b2649e1c68 100644 --- a/packages/editor/src/components/suggestion-mode/auto-save.js +++ b/packages/editor/src/components/suggestion-mode/auto-save.js @@ -66,6 +66,35 @@ export function fingerprintOperations( operations ) { } } +/** + * Derive the operation list a given overlay entry should persist. + * + * Structural entries (block-remove, block-insert-after, block-move) carry + * a single pre-built op in `entry.structuralOp`; the interceptor wrote it + * after detecting the corresponding tree mutation. Attribute-set entries + * derive their ops from the baseline-vs-overlay diff. An entry can have + * both — a user can edit attributes on a block that was suggested for + * removal — in which case the structural op leads and any attribute ops + * follow. + * + * @param {Object} entry Overlay entry. + * @return {Array} Ops describing the entry's pending suggestion. + */ +export function operationsForEntry( entry ) { + const ops = []; + if ( entry.structuralOp ) { + ops.push( entry.structuralOp ); + } + const attrOps = operationsFromOverlay( + entry.baselineAttributes, + entry.overlayAttributes + ); + for ( const op of attrOps ) { + ops.push( op ); + } + return ops; +} + /** * Invisible component that auto-commits pending overlay edits to the server * as note comments. Replaces the manual "Submit suggestion" button — in @@ -122,10 +151,7 @@ export default function SuggestionAutoSave() { if ( ! entry ) { return; } - const operations = operationsFromOverlay( - entry.baselineAttributes, - entry.overlayAttributes - ); + const operations = operationsForEntry( entry ); const fingerprint = fingerprintOperations( operations ); if ( fingerprint === entry.syncedOpsKey ) { return; @@ -208,10 +234,7 @@ export default function SuggestionAutoSave() { const timers = timersRef.current; for ( const [ clientId, entry ] of Object.entries( entries ) ) { - const operations = operationsFromOverlay( - entry.baselineAttributes, - entry.overlayAttributes - ); + const operations = operationsForEntry( entry ); const fingerprint = fingerprintOperations( operations ); if ( fingerprint === entry.syncedOpsKey ) { continue; diff --git a/packages/editor/src/components/suggestion-mode/index.js b/packages/editor/src/components/suggestion-mode/index.js index 19eaec10652496..7bdf54b7339f48 100644 --- a/packages/editor/src/components/suggestion-mode/index.js +++ b/packages/editor/src/components/suggestion-mode/index.js @@ -16,6 +16,8 @@ export { hasAttributeConflict, parseSuggestionPayload, payloadByteLength, + findStructuralOp, + clearSuggestionMarkerAttributes, PAYLOAD_MAX_BYTES, SCHEMA_VERSION, } from './provider'; diff --git a/packages/editor/src/components/suggestion-mode/overlay-context.js b/packages/editor/src/components/suggestion-mode/overlay-context.js index 43d560fa671236..35eda69aeaa064 100644 --- a/packages/editor/src/components/suggestion-mode/overlay-context.js +++ b/packages/editor/src/components/suggestion-mode/overlay-context.js @@ -83,6 +83,7 @@ const OverlayContext = createContext( { clearOverlay: () => {}, setCommentId: () => {}, setSyncedOpsKey: () => {}, + setStructuralOp: () => {}, hasOverlay: () => false, requestInterceptorBypass: () => {}, consumeInterceptorBypass: () => false, @@ -161,6 +162,25 @@ export function overlayReducer( state, action ) { }, }; } + case 'SET_STRUCTURAL_OP': { + // Structural ops (block-remove, block-insert-after, block-move) + // don't have a baseline-vs-overlay attribute diff; the operation + // itself describes the change. Auto-save reads `structuralOp` + // straight through. Replaces any existing op for the same block + // — only one structural marker can be pending at a time. + const existing = state[ action.clientId ]; + return { + ...state, + [ action.clientId ]: { + blockName: action.blockName, + baselineAttributes: existing?.baselineAttributes ?? {}, + overlayAttributes: existing?.overlayAttributes ?? {}, + commentId: existing?.commentId ?? null, + syncedOpsKey: existing?.syncedOpsKey ?? null, + structuralOp: action.op, + }, + }; + } case 'PRUNE_ORPHANS': { // Action carries a serializable array; the reducer materializes a // Set internally for the lookup. Keeps actions Redux-DevTools- @@ -236,6 +256,17 @@ export function SuggestionOverlayProvider( { children } ) { [] ); + const setStructuralOp = useCallback( + ( clientId, blockName, op ) => + dispatch( { + type: 'SET_STRUCTURAL_OP', + clientId, + blockName, + op, + } ), + [] + ); + const hasEntries = Object.keys( entries ).length > 0; const hasOverlay = useCallback( @@ -315,6 +346,7 @@ export function SuggestionOverlayProvider( { children } ) { clearOverlay, setCommentId, setSyncedOpsKey, + setStructuralOp, hasOverlay, requestInterceptorBypass, consumeInterceptorBypass, @@ -326,6 +358,7 @@ export function SuggestionOverlayProvider( { children } ) { clearOverlay, setCommentId, setSyncedOpsKey, + setStructuralOp, hasOverlay, requestInterceptorBypass, consumeInterceptorBypass, diff --git a/packages/editor/src/components/suggestion-mode/provider.js b/packages/editor/src/components/suggestion-mode/provider.js index 6a945c857c0d9d..1825a99f239894 100644 --- a/packages/editor/src/components/suggestion-mode/provider.js +++ b/packages/editor/src/components/suggestion-mode/provider.js @@ -181,6 +181,57 @@ function isAttributeEqual( a, b ) { return true; } +/** + * Operation types that mutate the block tree's structure rather than a + * single block's attributes. These flow through a different apply/reject + * path than `attribute-set`: Apply dispatches the corresponding block- + * editor action (`removeBlock`, `insertBlock`, `moveBlockToPosition`), + * Reject just clears the `metadata.suggestion` marker. + */ +const STRUCTURAL_OP_TYPES = new Set( [ + 'block-remove', + 'block-insert-after', + 'block-move', +] ); + +/** + * Locate the structural operation in a suggestion payload. v2 payloads carry + * at most one structural op per suggestion (the auto-save loop persists each + * structural mutation as its own note); attribute-set ops can ride along + * inside the same payload but the structural op leads. + * + * @param {SuggestionOperation[]} operations Payload operations. + * @return {SuggestionOperation|null} Structural op, or null when none. + */ +export function findStructuralOp( operations ) { + if ( ! Array.isArray( operations ) ) { + return null; + } + for ( const op of operations ) { + if ( op && STRUCTURAL_OP_TYPES.has( op.type ) ) { + return op; + } + } + return null; +} + +/** + * Build attributes that clear the `metadata.suggestion` marker on a block + * while preserving every other metadata field. Used by Apply (after the + * mutation lands) and by Reject (to drop the pending state). + * + * @param {Object} currentAttributes Block's current attributes. + * @return {Object} Partial attributes payload safe for `updateBlockAttributes`. + */ +export function clearSuggestionMarkerAttributes( currentAttributes ) { + const meta = currentAttributes?.metadata; + if ( ! meta || meta.suggestion === undefined ) { + return null; + } + const { suggestion: _drop, ...rest } = meta; + return { metadata: rest }; +} + /** * Apply a suggestion payload's operations to a block's current attributes * to produce the new attributes. Pure function — no side effects. @@ -328,7 +379,8 @@ export function useSuggestionsProvider() { const { saveEntityRecord } = useDispatch( coreStore ); const { createNotice } = useDispatch( noticesStore ); - const { updateBlockAttributes } = useDispatch( blockEditorStore ); + const { updateBlockAttributes, removeBlock } = + useDispatch( blockEditorStore ); const { getBlockAttributes: selectBlockAttributes, getClientIdsWithDescendants: selectClientIdsWithDescendants, @@ -575,6 +627,58 @@ export function useSuggestionsProvider() { return; } + // Structural ops (block-remove for now, block-insert-after and + // block-move in follow-up PRs) can't ride the + // updateBlockAttributes path: their apply mutates the tree + // rather than a single block's attributes. Branch out, run the + // matching block-editor action, and short-circuit before the + // attribute-set rollback machinery below. + const structuralOp = findStructuralOp( payload.operations ); + if ( structuralOp ) { + try { + if ( structuralOp.type === 'block-remove' ) { + // Bypass twice: the marker-clear dispatch lands + // first (so the live block ends without the + // pending-remove flag should the removeBlock fail), + // then the actual removal. + const clearAttrs = clearSuggestionMarkerAttributes( + selectBlockAttributes( targetClientId ) + ); + if ( clearAttrs ) { + requestInterceptorBypass( targetClientId ); + updateBlockAttributes( targetClientId, clearAttrs ); + } + requestInterceptorBypass( targetClientId ); + clearOverlay( targetClientId ); + removeBlock( targetClientId ); + } + + await saveEntityRecord( + 'root', + 'comment', + { + id: commentId, + status: 'approved', + meta: { _wp_suggestion_status: 'applied' }, + }, + { throwOnError: true } + ); + + createNotice( 'snackbar', __( 'Suggestion applied.' ), { + type: 'snackbar', + isDismissible: true, + } ); + } catch ( error ) { + createNotice( + 'error', + error?.message || + __( 'Failed to save suggestion status.' ), + { type: 'snackbar', isDismissible: true } + ); + } + return; + } + const currentAttributes = selectBlockAttributes( targetClientId ); const newAttributes = applyOperations( currentAttributes, @@ -645,6 +749,7 @@ export function useSuggestionsProvider() { [ saveEntityRecord, updateBlockAttributes, + removeBlock, selectBlockAttributes, selectClientIdsWithDescendants, createNotice, @@ -657,14 +762,39 @@ export function useSuggestionsProvider() { * Reject a suggestion by setting the comment's lifecycle status. The * comment itself stays as a thread (status `approved`) so the * conversation persists as evidence that the suggestion was reviewed. + * For structural suggestions (e.g. `block-remove`), also clears the + * `metadata.suggestion` marker on the live block so the dimmed/struck + * visual treatment goes away. * - * @param {Object} args Reject arguments. - * @param {number|string} args.commentId Comment id of the rejected - * suggestion. + * @param {Object} args Reject arguments. + * @param {number|string} args.commentId Comment id of the rejected + * suggestion. + * @param {string} [args.clientId] Target block clientId, if + * known. + * @param {SuggestionPayload} [args.payload] Parsed suggestion payload — + * inspected to detect a + * structural op so the marker + * can be cleared on the live + * block. * @return {Promise} */ const rejectSuggestion = useCallback( - async ( { commentId } ) => { + async ( { commentId, clientId, payload } ) => { + // Clear the live block's suggestion marker for structural + // rejects. Attribute-set suggestions don't carry a marker, so + // nothing to clear. + const structuralOp = findStructuralOp( payload?.operations ); + if ( structuralOp && clientId ) { + const clearAttrs = clearSuggestionMarkerAttributes( + selectBlockAttributes( clientId ) + ); + if ( clearAttrs ) { + requestInterceptorBypass( clientId ); + updateBlockAttributes( clientId, clearAttrs ); + } + clearOverlay( clientId ); + } + try { await saveEntityRecord( 'root', @@ -689,7 +819,14 @@ export function useSuggestionsProvider() { ); } }, - [ saveEntityRecord, createNotice ] + [ + saveEntityRecord, + createNotice, + selectBlockAttributes, + updateBlockAttributes, + requestInterceptorBypass, + clearOverlay, + ] ); return { diff --git a/packages/editor/src/components/suggestion-mode/store-interceptor.js b/packages/editor/src/components/suggestion-mode/store-interceptor.js index 459fc1551133a7..b5f35a1d0b2200 100644 --- a/packages/editor/src/components/suggestion-mode/store-interceptor.js +++ b/packages/editor/src/components/suggestion-mode/store-interceptor.js @@ -78,7 +78,7 @@ function readNoteIds( metadata ) { * folds changes to these keys into its snapshot before diffing so they are * never reverted and never routed into the user-pending overlay. */ -const SYSTEM_METADATA_KEYS = new Set( [ 'noteId' ] ); +const SYSTEM_METADATA_KEYS = new Set( [ 'noteId', 'suggestion' ] ); /** * Compare two attribute values structurally. Mirrors `isAttributeEqual` in @@ -336,6 +336,95 @@ function isAcceptedSuggestionChange( coreSelect, currentAttributes, delta ) { return changedKeys.every( ( key ) => matched.has( key ) ); } +/** + * Marker shape stored at `metadata.suggestion` on a block to indicate a + * pending structural suggestion. The block stays in the live tree; the + * marker drives the visual treatment and tells the auto-save loop to + * persist the corresponding structural operation. Cleared on apply or + * reject. See `docs/explanations/architecture/suggestions.md` for the + * "apply-and-tag" rationale. + * + * @typedef {Object} SuggestionMarker + * @property {'pending-remove'|'pending-insert'|'pending-move'} type Op type + * the marker represents. + * @property {number} [commentId] Filled in by auto-save once a note comment + * exists for this marker. + */ + +/** + * Walk the live block-editor tree and capture the parent + index of every + * block. Used by the removal-detection branch to re-insert a block at its + * previous position when the live tree drops it. + * + * @param {Object} blockEditor Block-editor selectors (`registry.select( + * 'core/block-editor' )`). + * @return {{ + * blocksByClientId: Map, + * parentByClientId: Map, + * indexByClientId: Map, + * }} Tree snapshot. + */ +function captureTreeSnapshot( blockEditor ) { + const blocksByClientId = new Map(); + const parentByClientId = new Map(); + const indexByClientId = new Map(); + + const walk = ( clientIds, parentClientId ) => { + for ( let index = 0; index < clientIds.length; index++ ) { + const clientId = clientIds[ index ]; + const block = blockEditor.getBlock?.( clientId ); + if ( ! block ) { + continue; + } + blocksByClientId.set( clientId, block ); + parentByClientId.set( clientId, parentClientId ); + indexByClientId.set( clientId, index ); + const childIds = blockEditor.getBlockOrder?.( clientId ) ?? []; + if ( childIds.length > 0 ) { + walk( childIds, clientId ); + } + } + }; + walk( blockEditor.getBlockOrder?.() ?? [], null ); + + return { blocksByClientId, parentByClientId, indexByClientId }; +} + +/** + * Add or replace the `metadata.suggestion` marker on an attributes object, + * leaving every other field untouched. Returns a new object — the caller + * passes it to `updateBlockAttributes`, which performs its own merge. + * + * @param {Object} currentMetadata Current block metadata. + * @param {SuggestionMarker} marker Marker to write. + * @return {Object} New metadata with the marker applied. + */ +function withSuggestionMarker( currentMetadata, marker ) { + return { + ...( currentMetadata || {} ), + suggestion: marker, + }; +} + +/** + * Filter a list of removed clientIds down to the topmost ancestors — the + * parents whose subtree contains the rest. Re-inserting a top-level removed + * block restores its descendants automatically; re-inserting both a parent + * and its child would duplicate the child. + * + * @param {string[]} removedIds All clientIds missing + * from the live tree. + * @param {Map} parentByClientId Snapshot parents. + * @return {string[]} Top-level removed clientIds. + */ +function topLevelRemoved( removedIds, parentByClientId ) { + const removedSet = new Set( removedIds ); + return removedIds.filter( ( id ) => { + const parent = parentByClientId.get( id ); + return parent === null || ! removedSet.has( parent ); + } ); +} + /** * Invisible component that catches block-attribute mutations dispatched * directly to the block-editor store while the editor is in Suggest intent. @@ -362,6 +451,7 @@ export default function SuggestionStoreInterceptor() { entries, captureBaseline, setOverlayAttributes, + setStructuralOp, consumeInterceptorBypass, } = useSuggestionOverlay(); const registry = useRegistry(); @@ -383,6 +473,9 @@ export default function SuggestionStoreInterceptor() { const setOverlayAttributesRef = useRef( setOverlayAttributes ); setOverlayAttributesRef.current = setOverlayAttributes; + const setStructuralOpRef = useRef( setStructuralOp ); + setStructuralOpRef.current = setStructuralOp; + const consumeInterceptorBypassRef = useRef( consumeInterceptorBypass ); consumeInterceptorBypassRef.current = consumeInterceptorBypass; @@ -415,6 +508,13 @@ export default function SuggestionStoreInterceptor() { ); } + // Tree snapshot from the previous tick. Used by the removal- + // detection branch to recover a block's parent + index + content + // after the block-editor store has dropped it. Refreshed at the end + // of every fire so the next fire can compare against the most + // recently-stable state. + let tree = captureTreeSnapshot( blockEditor ); + // Set true while we're calling `updateBlockAttributes` to revert a // detected mutation, so the resulting subscribe fire doesn't loop. let isReverting = false; @@ -523,14 +623,119 @@ export default function SuggestionStoreInterceptor() { // block; do NOT update it to `current` here. } - // Drop snapshot entries for blocks that were removed. - if ( snapshot.size > liveClientIds.length ) { - for ( const clientId of snapshot.keys() ) { - if ( ! live.has( clientId ) ) { + // Detect blocks that disappeared from the live tree and route + // them through the Suggest-mode "apply-and-tag" flow: re-insert + // the subtree from the previous-tick snapshot at its previous + // position, then tag the re-inserted block with a + // `metadata.suggestion = { type: 'pending-remove' }` marker. + // Auto-save reads the marker and persists it as a `block-remove` + // operation; Apply later runs the real `removeBlock`, Reject + // just clears the marker. See suggestions.md for the rationale. + const removedIds = []; + for ( const clientId of tree.blocksByClientId.keys() ) { + if ( ! live.has( clientId ) ) { + // "Apply landing": when another client accepts a + // `pending-remove` suggestion, the resulting + // `removeBlock` arrives here through sync — + // typically batched with the marker-clearing + // `updateBlockAttributes` into a single block- + // editor update, which fires this subscriber + // once. The previous-tick tree snapshot still + // carries the pending marker, so we can recognize + // the disappearance as the apply landing rather + // than a fresh user delete. Adopt the removal: + // drop the snapshot entry and skip the re-insert + // + re-tag path. Without this, the apply bounces + // back through sync and undoes the removal on the + // accepting client a moment after they clicked. + const trackedMarker = + tree.blocksByClientId.get( clientId )?.attributes + ?.metadata?.suggestion?.type; + if ( trackedMarker === 'pending-remove' ) { snapshot.delete( clientId ); + continue; + } + removedIds.push( clientId ); + } + } + + if ( removedIds.length > 0 ) { + const tops = topLevelRemoved( + removedIds, + tree.parentByClientId + ); + + // Phase 1: re-insert each top-level removed subtree at its + // previous position. Done synchronously inside `isReverting` + // so the resulting subscribe fires don't loop. The inserted + // blocks reuse their original clientIds (preserved by + // `getBlock`), so the marker write below targets the same + // IDs the caller saw. + isReverting = true; + try { + for ( const clientId of tops ) { + const block = tree.blocksByClientId.get( clientId ); + const parent = tree.parentByClientId.get( clientId ); + const index = tree.indexByClientId.get( clientId ); + if ( ! block ) { + continue; + } + blockEditorDispatch.insertBlock( + block, + index, + parent ?? undefined, + false + ); } + } finally { + isReverting = false; + } + + // Phase 2: tag each re-inserted block with the pending- + // remove marker AND record the structural operation in the + // overlay so auto-save can persist it as a `block-remove` + // op. `metadata.suggestion` is in SYSTEM_METADATA_KEYS, so + // subsequent fires fold the marker into the snapshot and + // don't route it to the user overlay. + for ( const clientId of tops ) { + const currentAttrs = + blockEditor.getBlockAttributes?.( clientId ); + if ( ! currentAttrs ) { + continue; + } + const block = tree.blocksByClientId.get( clientId ); + isReverting = true; + try { + blockEditorDispatch.updateBlockAttributes( clientId, { + metadata: withSuggestionMarker( + currentAttrs.metadata, + { type: 'pending-remove' } + ), + } ); + } finally { + isReverting = false; + } + setStructuralOpRef.current?.( clientId, block?.name ?? '', { + type: 'block-remove', + clientId, + blockName: block?.name ?? '', + block, + } ); + } + + // Drop snapshot entries for any remaining (descendant) + // removed blocks — they came back as part of their parent's + // re-inserted subtree, but we still want a fresh re-seed. + for ( const clientId of removedIds ) { + snapshot.delete( clientId ); } } + + // Refresh the tree snapshot so the next fire compares against + // the new stable state. Done at the end of every fire whether + // or not a removal was detected — captures inserts and moves + // the live tree settled on during this tick. + tree = captureTreeSnapshot( blockEditor ); }, BLOCK_EDITOR_STORE_NAME ); return unsubscribe; @@ -545,4 +750,7 @@ export { adoptSystemMetadata, stripSystemMetadata, isAcceptedSuggestionChange, + captureTreeSnapshot, + topLevelRemoved, + withSuggestionMarker, }; diff --git a/packages/editor/src/components/suggestion-mode/test/auto-save.js b/packages/editor/src/components/suggestion-mode/test/auto-save.js index 9d09f0e910f837..5808145234a526 100644 --- a/packages/editor/src/components/suggestion-mode/test/auto-save.js +++ b/packages/editor/src/components/suggestion-mode/test/auto-save.js @@ -13,7 +13,7 @@ import { store as noticesStore } from '@wordpress/notices'; /** * Internal dependencies */ -import SuggestionAutoSave from '../auto-save'; +import SuggestionAutoSave, { operationsForEntry } from '../auto-save'; import { SuggestionOverlayProvider, useSuggestionOverlay, @@ -442,3 +442,58 @@ describe( 'SuggestionAutoSave', () => { expect( createSuggestion ).not.toHaveBeenCalled(); } ); } ); + +describe( 'operationsForEntry', () => { + it( 'derives attribute-set ops from baseline + overlay when no structural op is set', () => { + expect( + operationsForEntry( { + blockName: 'core/paragraph', + baselineAttributes: { content: 'a' }, + overlayAttributes: { content: 'b' }, + } ) + ).toEqual( [ + { + type: 'attribute-set', + attribute: 'content', + before: 'a', + after: 'b', + }, + ] ); + } ); + + it( 'returns the structural op as a single-element array when present', () => { + const op = { + type: 'block-remove', + clientId: 'x', + blockName: 'core/paragraph', + }; + expect( + operationsForEntry( { + blockName: 'core/paragraph', + baselineAttributes: {}, + overlayAttributes: {}, + structuralOp: op, + } ) + ).toEqual( [ op ] ); + } ); + + it( 'emits structural op first then attribute-set ops when both are present', () => { + const op = { type: 'block-remove', clientId: 'x' }; + expect( + operationsForEntry( { + blockName: 'core/paragraph', + baselineAttributes: { content: 'a' }, + overlayAttributes: { content: 'b' }, + structuralOp: op, + } ) + ).toEqual( [ + op, + { + type: 'attribute-set', + attribute: 'content', + before: 'a', + after: 'b', + }, + ] ); + } ); +} ); diff --git a/packages/editor/src/components/suggestion-mode/test/overlay-context.js b/packages/editor/src/components/suggestion-mode/test/overlay-context.js index d0a49e8a697dd4..11e0abff2b7e28 100644 --- a/packages/editor/src/components/suggestion-mode/test/overlay-context.js +++ b/packages/editor/src/components/suggestion-mode/test/overlay-context.js @@ -215,4 +215,56 @@ describe( 'overlayReducer', () => { expect( afterClear[ 'block-a' ] ).toBeUndefined(); expect( afterClear[ 'block-b' ] ).toEqual( state[ 'block-b' ] ); } ); + + it( 'creates an entry on SET_STRUCTURAL_OP when none existed', () => { + const op = { + type: 'block-remove', + clientId: CLIENT_ID, + blockName: 'core/paragraph', + }; + const next = overlayReducer( INITIAL, { + type: 'SET_STRUCTURAL_OP', + clientId: CLIENT_ID, + blockName: 'core/paragraph', + op, + } ); + expect( next[ CLIENT_ID ] ).toMatchObject( { + blockName: 'core/paragraph', + structuralOp: op, + baselineAttributes: {}, + overlayAttributes: {}, + commentId: null, + syncedOpsKey: null, + } ); + } ); + + it( 'preserves attribute-overlay state when adding a structural op to an existing entry', () => { + let state = overlayReducer( INITIAL, { + type: 'CAPTURE_BASELINE', + clientId: CLIENT_ID, + blockName: 'core/paragraph', + attributes: { content: 'baseline' }, + } ); + state = overlayReducer( state, { + type: 'SET_OVERLAY_ATTRIBUTES', + clientId: CLIENT_ID, + attributes: { content: 'edited' }, + } ); + state = overlayReducer( state, { + type: 'SET_STRUCTURAL_OP', + clientId: CLIENT_ID, + blockName: 'core/paragraph', + op: { type: 'block-remove', clientId: CLIENT_ID }, + } ); + expect( state[ CLIENT_ID ].baselineAttributes ).toEqual( { + content: 'baseline', + } ); + expect( state[ CLIENT_ID ].overlayAttributes ).toEqual( { + content: 'edited', + } ); + expect( state[ CLIENT_ID ].structuralOp ).toEqual( { + type: 'block-remove', + clientId: CLIENT_ID, + } ); + } ); } ); diff --git a/packages/editor/src/components/suggestion-mode/test/provider.js b/packages/editor/src/components/suggestion-mode/test/provider.js index 4a938a0adbf8b2..b79d59600553a2 100644 --- a/packages/editor/src/components/suggestion-mode/test/provider.js +++ b/packages/editor/src/components/suggestion-mode/test/provider.js @@ -8,6 +8,8 @@ import { parseSuggestionPayload, payloadByteLength, PAYLOAD_MAX_BYTES, + findStructuralOp, + clearSuggestionMarkerAttributes, } from '../provider'; describe( 'operationsFromOverlay', () => { @@ -333,3 +335,59 @@ describe( 'parseSuggestionPayload', () => { ).toBeNull(); } ); } ); + +describe( 'findStructuralOp', () => { + it( 'returns null for a payload of attribute-set ops only', () => { + expect( + findStructuralOp( [ + { type: 'attribute-set', attribute: 'content', after: 'x' }, + ] ) + ).toBeNull(); + } ); + + it( 'returns the block-remove op when present', () => { + const op = { + type: 'block-remove', + clientId: 'abc', + blockName: 'core/paragraph', + }; + expect( + findStructuralOp( [ + { type: 'attribute-set', attribute: 'x', after: 1 }, + op, + ] ) + ).toBe( op ); + } ); + + it( 'recognizes block-insert-after and block-move op types', () => { + expect( + findStructuralOp( [ { type: 'block-insert-after' } ] )?.type + ).toBe( 'block-insert-after' ); + expect( findStructuralOp( [ { type: 'block-move' } ] )?.type ).toBe( + 'block-move' + ); + } ); + + it( 'returns null for non-array input', () => { + expect( findStructuralOp( null ) ).toBeNull(); + expect( findStructuralOp( undefined ) ).toBeNull(); + } ); +} ); + +describe( 'clearSuggestionMarkerAttributes', () => { + it( 'returns null when there is no marker to clear', () => { + expect( clearSuggestionMarkerAttributes( {} ) ).toBeNull(); + expect( + clearSuggestionMarkerAttributes( { metadata: { noteId: 1 } } ) + ).toBeNull(); + } ); + + it( 'strips the suggestion field while preserving other metadata', () => { + expect( + clearSuggestionMarkerAttributes( { + content: 'hi', + metadata: { noteId: 7, suggestion: { type: 'pending-remove' } }, + } ) + ).toEqual( { metadata: { noteId: 7 } } ); + } ); +} ); diff --git a/packages/editor/src/components/suggestion-mode/test/store-interceptor.js b/packages/editor/src/components/suggestion-mode/test/store-interceptor.js index 32972ce5790084..1177bc89c998f5 100644 --- a/packages/editor/src/components/suggestion-mode/test/store-interceptor.js +++ b/packages/editor/src/components/suggestion-mode/test/store-interceptor.js @@ -26,6 +26,8 @@ import SuggestionStoreInterceptor, { adoptSystemMetadata, stripSystemMetadata, isAcceptedSuggestionChange, + topLevelRemoved, + withSuggestionMarker, } from '../store-interceptor'; import { SuggestionOverlayProvider, @@ -171,7 +173,7 @@ describe( 'SuggestionStoreInterceptor (integration)', () => { ); } ); - function setup() { + function setup( { initialBlocks } = {} ) { const registry = createRegistry(); registry.register( noticesStore ); // `preferencesStore` is required by `setEditorIntent` on branches @@ -183,8 +185,11 @@ describe( 'SuggestionStoreInterceptor (integration)', () => { registry.register( editorStore ); registry.dispatch( editorStore ).setEditorIntent( 'suggest' ); - const block = createBlock( TEST_BLOCK_NAME, { content: 'Hello' } ); - registry.dispatch( blockEditorStore ).resetBlocks( [ block ] ); + const block = + initialBlocks?.[ 0 ] ?? + createBlock( TEST_BLOCK_NAME, { content: 'Hello' } ); + const blocks = initialBlocks ?? [ block ]; + registry.dispatch( blockEditorStore ).resetBlocks( blocks ); let overlayHandle; function CaptureOverlay() { @@ -383,6 +388,123 @@ describe( 'SuggestionStoreInterceptor (integration)', () => { getOverlay().entries[ clientId ]?.overlayAttributes?.content ).toBe( 'After apply' ); } ); + + it( 'tags a removed block with metadata.suggestion = pending-remove and writes a block-remove op into the overlay', async () => { + // In Suggest mode a `removeBlock` dispatch must not actually remove + // the block — the apply-and-tag flow re-inserts the subtree at its + // previous position and marks it pending-remove. Auto-save reads + // the structural op from the overlay and persists it as a comment; + // Apply later runs the real removeBlock, Reject just clears the + // marker. + const { registry, clientId, getOverlay } = setup(); + + await act( async () => { + registry.dispatch( blockEditorStore ).removeBlock( clientId ); + } ); + await flushSubscribers(); + + const liveBlocks = registry.select( blockEditorStore ).getBlocks(); + expect( liveBlocks ).toHaveLength( 1 ); + expect( liveBlocks[ 0 ].clientId ).toBe( clientId ); + expect( liveBlocks[ 0 ].attributes?.metadata?.suggestion ).toEqual( { + type: 'pending-remove', + } ); + + // The marker is system metadata — it must NOT leak into the user + // overlay (the overlay represents user-pending edits, not provider- + // internal linkage). + expect( + getOverlay().entries[ clientId ]?.overlayAttributes?.metadata + ).toBeUndefined(); + + // The structural op slot drives auto-save persistence. + expect( getOverlay().entries[ clientId ]?.structuralOp ).toMatchObject( + { + type: 'block-remove', + clientId, + blockName: TEST_BLOCK_NAME, + } + ); + } ); + + it( 'adopts a removal that arrives bundled with its marker-clear (apply of pending-remove via sync)', async () => { + // Regression: when another client clicks "Apply" on a + // pending-remove suggestion, the resulting marker-clear and + // removeBlock land here through sync as a single batched + // block-editor update. Without the apply-landing check the + // removal-detection branch would re-insert the block and tag + // it as a fresh pending-remove — which then bounces back + // through sync, undoing the apply on the accepting client a + // moment after they clicked. + const a = createBlock( TEST_BLOCK_NAME, { content: 'A' } ); + const b = createBlock( TEST_BLOCK_NAME, { content: 'B' } ); + const { registry, getOverlay } = setup( { + initialBlocks: [ a, b ], + } ); + + // First, this client creates the pending-remove suggestion + // (just like the suggester would). + await act( async () => { + registry.dispatch( blockEditorStore ).removeBlock( b.clientId ); + } ); + await flushSubscribers(); + + expect( + registry.select( blockEditorStore ).getBlockAttributes( b.clientId ) + ?.metadata?.suggestion?.type + ).toBe( 'pending-remove' ); + + // Simulate the apply landing: marker-clear + removeBlock + // batched into a single store update (matching how YJS + // applies multi-op transactions on a peer client). + await act( async () => { + registry.batch( () => { + registry + .dispatch( blockEditorStore ) + .updateBlockAttributes( b.clientId, { metadata: {} } ); + registry.dispatch( blockEditorStore ).removeBlock( b.clientId ); + } ); + } ); + await flushSubscribers(); + + // The block must stay removed — the interceptor adopts the + // apply landing rather than re-inserting and re-tagging. + const liveBlocks = registry.select( blockEditorStore ).getBlocks(); + expect( liveBlocks ).toHaveLength( 1 ); + expect( liveBlocks[ 0 ].clientId ).toBe( a.clientId ); + + // The orphan overlay entry is cleaned up by the PRUNE_ORPHANS + // effect once the block leaves the live tree. + expect( getOverlay().entries[ b.clientId ] ).toBeUndefined(); + } ); + + it( 're-inserts only the top-level removed block when a parent and its child are removed together', async () => { + // `removeBlock` on a parent removes the whole subtree atomically. + // We must re-insert just the parent — its child rides along. + // Re-inserting both would duplicate the child. + const parent = createBlock( TEST_BLOCK_NAME, { content: 'Parent' }, [ + createBlock( TEST_BLOCK_NAME, { content: 'Child' } ), + ] ); + const { registry } = setup( { initialBlocks: [ parent ] } ); + + await act( async () => { + registry + .dispatch( blockEditorStore ) + .removeBlock( parent.clientId ); + } ); + await flushSubscribers(); + + const liveBlocks = registry.select( blockEditorStore ).getBlocks(); + expect( liveBlocks ).toHaveLength( 1 ); + expect( liveBlocks[ 0 ].clientId ).toBe( parent.clientId ); + expect( liveBlocks[ 0 ].innerBlocks ).toHaveLength( 1 ); + expect( liveBlocks[ 0 ].innerBlocks[ 0 ].attributes?.content ).toBe( + 'Child' + ); + expect( liveBlocks[ 0 ].attributes?.metadata?.suggestion?.type ).toBe( + 'pending-remove' + ); + } ); } ); describe( 'isAcceptedSuggestionChange', () => { @@ -580,3 +702,71 @@ describe( 'stripSystemMetadata', () => { } ); } ); } ); + +describe( 'topLevelRemoved', () => { + it( 'returns the input when every removed block has a live parent', () => { + const parentByClientId = new Map( [ + [ 'a', null ], + [ 'b', null ], + ] ); + expect( topLevelRemoved( [ 'a', 'b' ], parentByClientId ) ).toEqual( [ + 'a', + 'b', + ] ); + } ); + + it( 'filters out descendants of other removed blocks', () => { + const parentByClientId = new Map( [ + [ 'parent', null ], + [ 'child', 'parent' ], + [ 'grandchild', 'child' ], + ] ); + expect( + topLevelRemoved( + [ 'parent', 'child', 'grandchild' ], + parentByClientId + ) + ).toEqual( [ 'parent' ] ); + } ); + + it( 'keeps siblings whose parent is still live', () => { + const parentByClientId = new Map( [ + [ 'liveParent', null ], + [ 'sibling-a', 'liveParent' ], + [ 'sibling-b', 'liveParent' ], + ] ); + expect( + topLevelRemoved( [ 'sibling-a', 'sibling-b' ], parentByClientId ) + ).toEqual( [ 'sibling-a', 'sibling-b' ] ); + } ); +} ); + +describe( 'withSuggestionMarker', () => { + it( 'attaches the marker to a fresh metadata object', () => { + expect( + withSuggestionMarker( undefined, { type: 'pending-remove' } ) + ).toEqual( { suggestion: { type: 'pending-remove' } } ); + } ); + + it( 'preserves existing metadata fields when adding the marker', () => { + expect( + withSuggestionMarker( + { noteId: 7, name: 'Hero' }, + { type: 'pending-remove' } + ) + ).toEqual( { + noteId: 7, + name: 'Hero', + suggestion: { type: 'pending-remove' }, + } ); + } ); + + it( 'replaces an existing marker rather than merging', () => { + expect( + withSuggestionMarker( + { suggestion: { type: 'pending-insert', commentId: 1 } }, + { type: 'pending-remove' } + ).suggestion + ).toEqual( { type: 'pending-remove' } ); + } ); +} ); diff --git a/tools/eslint/suppressions.json b/tools/eslint/suppressions.json index 3e89d014c4743a..e157b71c607c8d 100644 --- a/tools/eslint/suppressions.json +++ b/tools/eslint/suppressions.json @@ -1096,7 +1096,7 @@ }, "packages/editor/src/components/suggestion-mode/store-interceptor.js": { "react-hooks/refs": { - "count": 4 + "count": 5 } }, "packages/editor/src/components/suggestion-mode/suggestion-diff.js": {