From 497028901e99fb558bb033e7d587f932530e52a9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Christian=20Hamburger=20Gr=C3=B8ngaard?= Date: Fri, 16 Aug 2024 11:56:19 +0200 Subject: [PATCH] fix(editor): guard against apply side effects when undoing/redoing --- .../editor/src/editor/hooks/useSyncValue.ts | 25 +++++--------- .../src/editor/plugins/createWithMaxBlocks.ts | 10 ++++++ .../editor/plugins/createWithObjectKeys.ts | 23 +++++++------ .../src/editor/plugins/createWithPatches.ts | 9 ++--- .../plugins/createWithPlaceholderBlock.ts | 10 ++++++ .../createWithPortableTextMarkModel.ts | 10 ++++++ .../src/editor/plugins/createWithUndoRedo.ts | 10 +++--- packages/editor/src/utils/withPreserveKeys.ts | 14 -------- packages/editor/src/utils/withUndoRedo.ts | 34 +++++++++++++++++++ 9 files changed, 92 insertions(+), 53 deletions(-) delete mode 100644 packages/editor/src/utils/withPreserveKeys.ts create mode 100644 packages/editor/src/utils/withUndoRedo.ts diff --git a/packages/editor/src/editor/hooks/useSyncValue.ts b/packages/editor/src/editor/hooks/useSyncValue.ts index 186adf982..b3494cca6 100644 --- a/packages/editor/src/editor/hooks/useSyncValue.ts +++ b/packages/editor/src/editor/hooks/useSyncValue.ts @@ -11,7 +11,6 @@ import {validateValue} from '../../utils/validateValue' import {toSlateValue, VOID_CHILD_KEY} from '../../utils/values' import {isChangingLocally, isChangingRemotely, withRemoteChanges} from '../../utils/withChanges' import {withoutPatching} from '../../utils/withoutPatching' -import {withPreserveKeys} from '../../utils/withPreserveKeys' import {withoutSaving} from '../plugins/createWithUndoRedo' import {type PortableTextEditor} from '../PortableTextEditor' @@ -184,10 +183,8 @@ export function useSyncValue( currentBlock, ) if (validation.valid || validation.resolution?.autoResolve) { - withPreserveKeys(slateEditor, () => { - Transforms.insertNodes(slateEditor, currentBlock, { - at: [currentBlockIndex], - }) + Transforms.insertNodes(slateEditor, currentBlock, { + at: [currentBlockIndex], }) } else { debug('Invalid', validation) @@ -267,9 +264,7 @@ function _replaceBlock( Transforms.deselect(slateEditor) } Transforms.removeNodes(slateEditor, {at: [currentBlockIndex]}) - withPreserveKeys(slateEditor, () => { - Transforms.insertNodes(slateEditor, currentBlock, {at: [currentBlockIndex]}) - }) + Transforms.insertNodes(slateEditor, currentBlock, {at: [currentBlockIndex]}) slateEditor.onChange() if (selectionFocusOnBlock) { Transforms.select(slateEditor, currentSelection) @@ -350,21 +345,17 @@ function _updateBlock( Transforms.removeNodes(slateEditor, { at: [currentBlockIndex, currentBlockChildIndex], }) - withPreserveKeys(slateEditor, () => { - Transforms.insertNodes(slateEditor, currentBlockChild as Node, { - at: [currentBlockIndex, currentBlockChildIndex], - }) + Transforms.insertNodes(slateEditor, currentBlockChild as Node, { + at: [currentBlockIndex, currentBlockChildIndex], }) slateEditor.onChange() // Insert it if it didn't exist before } else if (!oldBlockChild) { debug('Inserting new child', currentBlockChild) - withPreserveKeys(slateEditor, () => { - Transforms.insertNodes(slateEditor, currentBlockChild as Node, { - at: [currentBlockIndex, currentBlockChildIndex], - }) - slateEditor.onChange() + Transforms.insertNodes(slateEditor, currentBlockChild as Node, { + at: [currentBlockIndex, currentBlockChildIndex], }) + slateEditor.onChange() } } }) diff --git a/packages/editor/src/editor/plugins/createWithMaxBlocks.ts b/packages/editor/src/editor/plugins/createWithMaxBlocks.ts index ada7e3ba3..1ed29d482 100644 --- a/packages/editor/src/editor/plugins/createWithMaxBlocks.ts +++ b/packages/editor/src/editor/plugins/createWithMaxBlocks.ts @@ -1,5 +1,6 @@ import {type PortableTextSlateEditor} from '../../types/editor' import {isChangingRemotely} from '../../utils/withChanges' +import {isRedoing, isUndoing} from '../../utils/withUndoRedo' /** * This plugin makes sure that the PTE maxBlocks prop is respected @@ -18,6 +19,15 @@ export function createWithMaxBlocks(maxBlocks: number) { return } + /** + * We don't want to run any side effects when the editor is undoing or + * redoing operations. + */ + if (isUndoing(editor) || isRedoing(editor)) { + apply(operation) + return + } + const rows = maxBlocks if (rows > 0 && editor.children.length >= rows) { if ( diff --git a/packages/editor/src/editor/plugins/createWithObjectKeys.ts b/packages/editor/src/editor/plugins/createWithObjectKeys.ts index 2b3ca319e..0adc1ef70 100644 --- a/packages/editor/src/editor/plugins/createWithObjectKeys.ts +++ b/packages/editor/src/editor/plugins/createWithObjectKeys.ts @@ -2,7 +2,7 @@ import {Editor, Element, Node, Transforms} from 'slate' import {type PortableTextMemberSchemaTypes, type PortableTextSlateEditor} from '../../types/editor' import {isChangingRemotely} from '../../utils/withChanges' -import {isPreservingKeys, PRESERVE_KEYS} from '../../utils/withPreserveKeys' +import {isRedoing, isUndoing} from '../../utils/withUndoRedo' /** * This plugin makes sure that every new node in the editor get a new _key prop when created @@ -13,11 +13,8 @@ export function createWithObjectKeys( keyGenerator: () => string, ) { return function withKeys(editor: PortableTextSlateEditor): PortableTextSlateEditor { - PRESERVE_KEYS.set(editor, false) const {apply, normalizeNode} = editor - // The apply function can be called with a scope (withPreserveKeys) that will - // preserve keys for the produced nodes if they have a _key property set already. // The default behavior is to always generate a new key here. // For example, when undoing and redoing we want to retain the keys, but // when we create a new bold span by splitting a non-bold-span we want the produced node to get a new key. @@ -31,14 +28,21 @@ export function createWithObjectKeys( return } - if (operation.type === 'split_node') { - const withNewKey = !isPreservingKeys(editor) || !('_key' in operation.properties) + /** + * We don't want to run any side effects when the editor is undoing or + * redoing operations. + */ + if (isUndoing(editor) || isRedoing(editor)) { + apply(operation) + return + } + if (operation.type === 'split_node') { apply({ ...operation, properties: { ...operation.properties, - ...(withNewKey ? {_key: keyGenerator()} : {}), + _key: keyGenerator(), }, }) @@ -46,15 +50,12 @@ export function createWithObjectKeys( } if (operation.type === 'insert_node') { - // Must be given a new key or adding/removing marks while typing gets in trouble (duped keys)! - const withNewKey = !isPreservingKeys(editor) || !('_key' in operation.node) - if (!Editor.isEditor(operation.node)) { apply({ ...operation, node: { ...operation.node, - ...(withNewKey ? {_key: keyGenerator()} : {}), + _key: keyGenerator(), }, }) diff --git a/packages/editor/src/editor/plugins/createWithPatches.ts b/packages/editor/src/editor/plugins/createWithPatches.ts index 84dfe3fb9..5ae74dab0 100644 --- a/packages/editor/src/editor/plugins/createWithPatches.ts +++ b/packages/editor/src/editor/plugins/createWithPatches.ts @@ -27,7 +27,6 @@ import {fromSlateValue, isEqualToEmptyEditor} from '../../utils/values' import {IS_PROCESSING_REMOTE_CHANGES, KEY_TO_VALUE_ELEMENT} from '../../utils/weakMaps' import {withRemoteChanges} from '../../utils/withChanges' import {isPatching, PATCHING, withoutPatching} from '../../utils/withoutPatching' -import {withPreserveKeys} from '../../utils/withPreserveKeys' import {withoutSaving} from './createWithUndoRedo' const debug = debugWithName('plugin:withPatches') @@ -117,11 +116,9 @@ export function createWithPatches({ Editor.withoutNormalizing(editor, () => { withoutPatching(editor, () => { withoutSaving(editor, () => { - withPreserveKeys(editor, () => { - patches.forEach((patch) => { - if (debug.enabled) debug(`Handling remote patch ${JSON.stringify(patch)}`) - changed = applyPatch(editor, patch) - }) + patches.forEach((patch) => { + if (debug.enabled) debug(`Handling remote patch ${JSON.stringify(patch)}`) + changed = applyPatch(editor, patch) }) }) }) diff --git a/packages/editor/src/editor/plugins/createWithPlaceholderBlock.ts b/packages/editor/src/editor/plugins/createWithPlaceholderBlock.ts index 4f1b933bb..e078ef6f1 100644 --- a/packages/editor/src/editor/plugins/createWithPlaceholderBlock.ts +++ b/packages/editor/src/editor/plugins/createWithPlaceholderBlock.ts @@ -4,6 +4,7 @@ import {type PortableTextSlateEditor} from '../../types/editor' import {type SlateTextBlock, type VoidElement} from '../../types/slate' import {debugWithName} from '../../utils/debug' import {isChangingRemotely} from '../../utils/withChanges' +import {isRedoing, isUndoing} from '../../utils/withUndoRedo' const debug = debugWithName('plugin:withPlaceholderBlock') @@ -27,6 +28,15 @@ export function createWithPlaceholderBlock(): ( return } + /** + * We don't want to run any side effects when the editor is undoing or + * redoing operations. + */ + if (isUndoing(editor) || isRedoing(editor)) { + apply(op) + return + } + if (op.type === 'remove_node') { const node = op.node as SlateTextBlock | VoidElement if (op.path[0] === 0 && Editor.isVoid(editor, node)) { diff --git a/packages/editor/src/editor/plugins/createWithPortableTextMarkModel.ts b/packages/editor/src/editor/plugins/createWithPortableTextMarkModel.ts index 7261dd07e..158d22bcc 100644 --- a/packages/editor/src/editor/plugins/createWithPortableTextMarkModel.ts +++ b/packages/editor/src/editor/plugins/createWithPortableTextMarkModel.ts @@ -20,6 +20,7 @@ import {debugWithName} from '../../utils/debug' import {toPortableTextRange} from '../../utils/ranges' import {EMPTY_MARKS} from '../../utils/values' import {isChangingRemotely} from '../../utils/withChanges' +import {isRedoing, isUndoing} from '../../utils/withUndoRedo' const debug = debugWithName('plugin:withPortableTextMarkModel') @@ -243,6 +244,15 @@ export function createWithPortableTextMarkModel( return } + /** + * We don't want to run any side effects when the editor is undoing or + * redoing operations. + */ + if (isUndoing(editor) || isRedoing(editor)) { + apply(op) + return + } + // Special hook before inserting text at the end of an annotation. if (op.type === 'insert_text') { const {selection} = editor diff --git a/packages/editor/src/editor/plugins/createWithUndoRedo.ts b/packages/editor/src/editor/plugins/createWithUndoRedo.ts index 13f04a32d..9e614ff76 100644 --- a/packages/editor/src/editor/plugins/createWithUndoRedo.ts +++ b/packages/editor/src/editor/plugins/createWithUndoRedo.ts @@ -12,7 +12,7 @@ import {type Descendant, Editor, Operation, Path, type SelectionOperation, Trans import {type PatchObservable, type PortableTextSlateEditor} from '../../types/editor' import {debugWithName} from '../../utils/debug' import {fromSlateValue} from '../../utils/values' -import {PRESERVE_KEYS, withPreserveKeys} from '../../utils/withPreserveKeys' +import {setIsRedoing, setIsUndoing, withRedoing, withUndoing} from '../../utils/withUndoRedo' const debug = debugWithName('plugin:withUndoRedo') const debugVerbose = debug.enabled && false @@ -150,7 +150,7 @@ export function createWithUndoRedo( try { Editor.withoutNormalizing(editor, () => { - withPreserveKeys(editor, () => { + withUndoing(editor, () => { withoutSaving(editor, () => { reversedOperations.forEach((op) => { editor.apply(op) @@ -166,7 +166,7 @@ export function createWithUndoRedo( Transforms.deselect(editor) editor.history = {undos: [], redos: []} SAVING.set(editor, true) - PRESERVE_KEYS.set(editor, false) + setIsUndoing(editor, false) editor.onChange() return } @@ -196,7 +196,7 @@ export function createWithUndoRedo( }) try { Editor.withoutNormalizing(editor, () => { - withPreserveKeys(editor, () => { + withRedoing(editor, () => { withoutSaving(editor, () => { // eslint-disable-next-line max-nested-callbacks transformedOperations.forEach((op) => { @@ -213,7 +213,7 @@ export function createWithUndoRedo( Transforms.deselect(editor) editor.history = {undos: [], redos: []} SAVING.set(editor, true) - PRESERVE_KEYS.set(editor, false) + setIsRedoing(editor, false) editor.onChange() return } diff --git a/packages/editor/src/utils/withPreserveKeys.ts b/packages/editor/src/utils/withPreserveKeys.ts deleted file mode 100644 index f0328f537..000000000 --- a/packages/editor/src/utils/withPreserveKeys.ts +++ /dev/null @@ -1,14 +0,0 @@ -import {type Editor} from 'slate' - -export const PRESERVE_KEYS: WeakMap = new WeakMap() - -export function withPreserveKeys(editor: Editor, fn: () => void): void { - const prev = isPreservingKeys(editor) - PRESERVE_KEYS.set(editor, true) - fn() - PRESERVE_KEYS.set(editor, prev) -} - -export function isPreservingKeys(editor: Editor): boolean | undefined { - return PRESERVE_KEYS.get(editor) -} diff --git a/packages/editor/src/utils/withUndoRedo.ts b/packages/editor/src/utils/withUndoRedo.ts new file mode 100644 index 000000000..1a976f752 --- /dev/null +++ b/packages/editor/src/utils/withUndoRedo.ts @@ -0,0 +1,34 @@ +import {type Editor} from 'slate' + +const IS_UDOING: WeakMap = new WeakMap() +const IS_REDOING: WeakMap = new WeakMap() + +export function withUndoing(editor: Editor, fn: () => void) { + const prev = isUndoing(editor) + IS_UDOING.set(editor, true) + fn() + IS_UDOING.set(editor, prev) +} + +export function isUndoing(editor: Editor) { + return IS_UDOING.get(editor) ?? false +} + +export function setIsUndoing(editor: Editor, isUndoing: boolean) { + IS_UDOING.set(editor, isUndoing) +} + +export function withRedoing(editor: Editor, fn: () => void) { + const prev = isRedoing(editor) + IS_REDOING.set(editor, true) + fn() + IS_REDOING.set(editor, prev) +} + +export function isRedoing(editor: Editor) { + return IS_REDOING.get(editor) ?? false +} + +export function setIsRedoing(editor: Editor, isRedoing: boolean) { + IS_REDOING.set(editor, isRedoing) +}