From ae3ee4bb9ae2e95a3620551ac3356000b0926d13 Mon Sep 17 00:00:00 2001 From: Nate Higgins Date: Wed, 9 Feb 2022 00:19:22 +0000 Subject: [PATCH 01/13] feat: re-enable failing test for inserting / updating text blocks rapidly --- .../playwright/tests/page-creation.spec.ts | 36 +++++++++---------- 1 file changed, 17 insertions(+), 19 deletions(-) diff --git a/packages/hash/playwright/tests/page-creation.spec.ts b/packages/hash/playwright/tests/page-creation.spec.ts index 95c09138d76..4f4d8e48a76 100644 --- a/packages/hash/playwright/tests/page-creation.spec.ts +++ b/packages/hash/playwright/tests/page-creation.spec.ts @@ -76,20 +76,19 @@ test("user can create page", async ({ page }) => { // TODO: Move the cursor below the new divider and update the test? - // TODO: Uncomment after fixing flaky ProseMirror behavior - // // Insert a paragraph creation with newlines - // await sleep(100); // TODO: investigate flakiness in FF and Webkit - // await page.keyboard.type("Second paragraph"); - // await sleep(100); // TODO: investigate flakiness in FF and Webkit - // await page.keyboard.press("Shift+Enter"); - // await sleep(100); // TODO: investigate flakiness in FF and Webkit - // await page.keyboard.press("Shift+Enter"); - // await sleep(100); // TODO: investigate flakiness in FF and Webkit - // await page.keyboard.type("with"); - // await page.keyboard.press("Shift+Enter"); - // await sleep(100); // TODO: investigate flakiness in FF and Webkit - // await page.keyboard.type("line breaks"); - // await sleep(100); // TODO: investigate flakiness in FF and Webkit + // Insert a paragraph creation with newlines + await sleep(100); // TODO: investigate flakiness in FF and Webkit + await page.keyboard.type("Second paragraph"); + await sleep(100); // TODO: investigate flakiness in FF and Webkit + await page.keyboard.press("Shift+Enter"); + await sleep(100); // TODO: investigate flakiness in FF and Webkit + await page.keyboard.press("Shift+Enter"); + await sleep(100); // TODO: investigate flakiness in FF and Webkit + await page.keyboard.type("with"); + await page.keyboard.press("Shift+Enter"); + await sleep(100); // TODO: investigate flakiness in FF and Webkit + await page.keyboard.type("line breaks"); + await sleep(100); // TODO: investigate flakiness in FF and Webkit // Expect just inserted content to be present on the page await expect(blockRegionLocator).toContainText( @@ -124,11 +123,10 @@ test("user can create page", async ({ page }) => { blockRegionLocator.locator("p").nth(0).locator("em"), ).toContainText("italics"); - // TODO: Uncomment after fixing flaky ProseMirror behavior - // await expect(blockRegionLocator.locator("p").nth(1)).toContainText( - // "Second paragraph\n\nwith\nline breaks", - // { useInnerText: true }, - // ); + await expect(blockRegionLocator.locator("p").nth(1)).toContainText( + "Second paragraph\n\nwith\nline breaks", + { useInnerText: true }, + ); await expect(blockRegionLocator.locator("hr")).toBeVisible(); From 0660c37ef24d2cf378d2b256ce5c021e359e1b6d Mon Sep 17 00:00:00 2001 From: Nate Higgins Date: Wed, 9 Feb 2022 12:20:45 +0000 Subject: [PATCH 02/13] refactor: don't assume the shape of componentNodeAttrs when checking if they need updating post-save --- packages/hash/api/src/collab/Instance.ts | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/packages/hash/api/src/collab/Instance.ts b/packages/hash/api/src/collab/Instance.ts index ee662be4cd4..ee884284c2d 100644 --- a/packages/hash/api/src/collab/Instance.ts +++ b/packages/hash/api/src/collab/Instance.ts @@ -430,12 +430,12 @@ export class Instance { }, }); } - } else if (isComponentNode(node) && !node.attrs.blockEntityId) { - transform.setNodeMarkup( - mapping.map(pos), - undefined, - getComponentNodeAttrs(blockEntity), - ); + } else if (isComponentNode(node)) { + const nodeAttrs = getComponentNodeAttrs(blockEntity); + + if (!isEqual(node.attrs, nodeAttrs)) { + transform.setNodeMarkup(mapping.map(pos), undefined, nodeAttrs); + } } }); From 5ce5fdf9b6d9c391097298141da35e65fbd30d10 Mon Sep 17 00:00:00 2001 From: Nate Higgins Date: Wed, 9 Feb 2022 12:21:16 +0000 Subject: [PATCH 03/13] refactor: ComponentView get entity id from entity store instead of prosemirror --- packages/hash/frontend/src/blocks/page/ComponentView.tsx | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/packages/hash/frontend/src/blocks/page/ComponentView.tsx b/packages/hash/frontend/src/blocks/page/ComponentView.tsx index e164925a710..072e608bfe2 100644 --- a/packages/hash/frontend/src/blocks/page/ComponentView.tsx +++ b/packages/hash/frontend/src/blocks/page/ComponentView.tsx @@ -127,10 +127,11 @@ export class ComponentView implements NodeView { .resolve(this.getPos()) .node(2).attrs.draftId; - // @todo handle entity id not being defined - const entityId = node.attrs.blockEntityId ?? ""; - const entity = this.store.draft[blockDraftId]; const mappedUrl = componentIdToUrl(this.componentId); + const entity = this.store.draft[blockDraftId]; + + // @todo handle entity id not being defined + const entityId = entity.entityId ?? ""; /** used by collaborative editing feature `FocusTracker` */ this.target.setAttribute("data-entity-id", entityId); From a16d5541edcb01093032f5b14737395b0977a743 Mon Sep 17 00:00:00 2001 From: Nate Higgins Date: Wed, 9 Feb 2022 12:21:43 +0000 Subject: [PATCH 04/13] refactor: share types for getComponentNodeAttrs and isComponentNode --- packages/hash/shared/src/prosemirror.ts | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/packages/hash/shared/src/prosemirror.ts b/packages/hash/shared/src/prosemirror.ts index 80dba6cc46c..850033e7b4a 100644 --- a/packages/hash/shared/src/prosemirror.ts +++ b/packages/hash/shared/src/prosemirror.ts @@ -91,9 +91,10 @@ type NodeWithAttrs = Omit< "attrs" > & { attrs: Attrs }; -export type ComponentNode = NodeWithAttrs<{ +type ComponentNodeAttrs = { blockEntityId: string | null; -}>; +}; +export type ComponentNode = NodeWithAttrs; export type EntityNode = NodeWithAttrs<{ draftId: string | null; @@ -127,7 +128,7 @@ export const findComponentNodes = (doc: ProsemirrorNode) => { export const getComponentNodeAttrs = ( entity?: { entityId?: string | null } | null, -) => ({ +): ComponentNodeAttrs => ({ blockEntityId: entity?.entityId ?? "", }); From c00f681d9cb922bb63a0c87c76c61ce8dfe3f62e Mon Sep 17 00:00:00 2001 From: Nate Higgins Date: Wed, 9 Feb 2022 12:22:19 +0000 Subject: [PATCH 05/13] refactor: BlockView get entity id from entity store instead of prosemirror --- .../frontend/src/blocks/page/BlockView.tsx | 35 +++++++++++-------- 1 file changed, 20 insertions(+), 15 deletions(-) diff --git a/packages/hash/frontend/src/blocks/page/BlockView.tsx b/packages/hash/frontend/src/blocks/page/BlockView.tsx index f675cd3edb7..24303c90d3b 100644 --- a/packages/hash/frontend/src/blocks/page/BlockView.tsx +++ b/packages/hash/frontend/src/blocks/page/BlockView.tsx @@ -1,25 +1,25 @@ -import { BlockVariant } from "blockprotocol"; import { BlockMeta } from "@hashintel/hash-shared/blockMeta"; +import { EntityStore } from "@hashintel/hash-shared/entityStore"; +import { + entityStorePluginState, + subscribeToEntityStore, +} from "@hashintel/hash-shared/entityStorePlugin"; +import { isEntityNode } from "@hashintel/hash-shared/prosemirror"; import { ProsemirrorSchemaManager } from "@hashintel/hash-shared/ProsemirrorSchemaManager"; -import { findComponentNodes } from "@hashintel/hash-shared/prosemirror"; +import { BlockVariant } from "blockprotocol"; import { ProsemirrorNode, Schema } from "prosemirror-model"; import { NodeSelection } from "prosemirror-state"; import { EditorView, NodeView } from "prosemirror-view"; import { createRef, forwardRef, RefObject, useMemo, useState } from "react"; import { useOutsideClick } from "rooks"; import { tw } from "twind"; -import { EntityStore } from "@hashintel/hash-shared/entityStore"; -import { - entityStorePluginState, - subscribeToEntityStore, -} from "@hashintel/hash-shared/entityStorePlugin"; import { BlockContextMenu } from "../../components/BlockContextMenu/BlockContextMenu"; -import { BlockSuggesterProps } from "./createSuggester/BlockSuggester"; import { DragVerticalIcon } from "../../components/icons"; +import { BlockViewContext } from "./BlockViewContext"; +import { CollabPositionIndicators } from "./CollabPositionIndicators"; +import { BlockSuggesterProps } from "./createSuggester/BlockSuggester"; import styles from "./style.module.css"; import { RenderPortal } from "./usePortals"; -import { CollabPositionIndicators } from "./CollabPositionIndicators"; -import { BlockViewContext } from "./BlockViewContext"; type BlockHandleProps = { entityId: string | null; @@ -85,14 +85,19 @@ export class BlockView implements NodeView { private unsubscribe: Function; getBlockEntityIdFromNode = (node: ProsemirrorNode) => { - const componentNodes = findComponentNodes(node); - if (componentNodes.length !== 1) { + const blockEntityNode = node.firstChild; + + if (!blockEntityNode || !isEntityNode(blockEntityNode)) { + throw new Error("Unexpected prosemirror structure"); + } + + if (!blockEntityNode.attrs.draftId) { return null; } - const { blockEntityId } = componentNodes[0][0].attrs; - // The current default for this is an empty string, not null. - return blockEntityId === "" ? null : blockEntityId; + const draftEntity = this.store.draft[blockEntityNode.attrs.draftId]; + + return draftEntity?.entityId ?? null; }; constructor( From 97618f4a576fce9ef31951b4febbc7421741b279 Mon Sep 17 00:00:00 2001 From: Nate Higgins Date: Wed, 9 Feb 2022 12:27:37 +0000 Subject: [PATCH 06/13] feat: use entity nodes when working out blocks that have been removed This changes behavior slightly as deleting everything will be considered a block update in some cases instead of a block removal --- packages/hash/shared/src/save.ts | 29 ++++++++++++++++++++++++----- 1 file changed, 24 insertions(+), 5 deletions(-) diff --git a/packages/hash/shared/src/save.ts b/packages/hash/shared/src/save.ts index 21f96c74bb7..62d98378a8d 100644 --- a/packages/hash/shared/src/save.ts +++ b/packages/hash/shared/src/save.ts @@ -66,10 +66,29 @@ const defineOperation = }; const removeBlocks = defineOperation( - (entities: BlockEntity[], nodes: [ComponentNode, number][]) => { - const draftBlockEntityIds = new Set( - nodes.map(([node]) => node.attrs.blockEntityId), - ); + ( + entities: BlockEntity[], + doc: ProsemirrorNode, + entityStore: EntityStore, + ) => { + const draftBlockEntityIds = new Set(); + + doc.descendants((node) => { + if (isEntityNode(node)) { + if (node.attrs.draftId) { + const draftEntity = entityStore.draft[node.attrs.draftId]; + + if (!draftEntity || !isDraftBlockEntity(draftEntity)) { + throw new Error("Unexpected prosemirror structure"); + } + + // @todo handle entityId not being set + draftBlockEntityIds.add(draftEntity.entityId); + } + + return false; + } + }); const removedBlockEntities = entities .map((block, position) => [block, position] as const) @@ -330,7 +349,7 @@ const calculateSaveActions = ( let actions: UpdatePageAction[] = []; blocks = [...blocks]; - [actions, blocks] = removeBlocks(actions, blocks, componentNodes); + [actions, blocks] = removeBlocks(actions, blocks, doc, entityStore); [actions, blocks] = moveBlocks(actions, blocks, componentNodes); [actions, blocks] = insertBlocks( actions, From 73aa01ddacc7526ea09e31a86aa63cd2117e08c7 Mon Sep 17 00:00:00 2001 From: Nate Higgins Date: Wed, 9 Feb 2022 12:37:49 +0000 Subject: [PATCH 07/13] refactor: use entity nodes when calculating moved blocks --- packages/hash/shared/src/save.ts | 67 +++++++++++++++++--------------- 1 file changed, 35 insertions(+), 32 deletions(-) diff --git a/packages/hash/shared/src/save.ts b/packages/hash/shared/src/save.ts index 62d98378a8d..0d312d693a3 100644 --- a/packages/hash/shared/src/save.ts +++ b/packages/hash/shared/src/save.ts @@ -19,7 +19,12 @@ import { getTextEntityFromSavedBlock, isDraftTextContainingEntityProperties, } from "./entity"; -import { EntityStore, isBlockEntity, isDraftBlockEntity } from "./entityStore"; +import { + DraftEntity, + EntityStore, + isBlockEntity, + isDraftBlockEntity, +} from "./entityStore"; import { CreateEntityMutation, CreateEntityMutationVariables, @@ -66,33 +71,12 @@ const defineOperation = }; const removeBlocks = defineOperation( - ( - entities: BlockEntity[], - doc: ProsemirrorNode, - entityStore: EntityStore, - ) => { - const draftBlockEntityIds = new Set(); - - doc.descendants((node) => { - if (isEntityNode(node)) { - if (node.attrs.draftId) { - const draftEntity = entityStore.draft[node.attrs.draftId]; - - if (!draftEntity || !isDraftBlockEntity(draftEntity)) { - throw new Error("Unexpected prosemirror structure"); - } - - // @todo handle entityId not being set - draftBlockEntityIds.add(draftEntity.entityId); - } - - return false; - } - }); + (entities: BlockEntity[], draftBlockEntityIds: DraftEntity["entityId"][]) => { + const draftBlockEntityIdsSet = new Set(draftBlockEntityIds); const removedBlockEntities = entities .map((block, position) => [block, position] as const) - .filter(([block]) => !draftBlockEntityIds.has(block.entityId)); + .filter(([block]) => !draftBlockEntityIdsSet.has(block.entityId)); const updatedEntities = entities.filter( (_, position) => @@ -117,9 +101,9 @@ const removeBlocks = defineOperation( ); const moveBlocks = defineOperation( - (entities: BlockEntity[], nodes: [ComponentNode, number][]) => { - const entitiesWithoutNewBlocks = nodes.filter( - ([node]) => !!node.attrs.blockEntityId, + (entities: BlockEntity[], draftBlockEntityIds: DraftEntity["entityId"][]) => { + const otherExistingBlockEntityIds = draftBlockEntityIds.filter( + (blockEntityId) => !!blockEntityId, ); const actions: UpdatePageAction[] = []; @@ -127,8 +111,8 @@ const moveBlocks = defineOperation( for (let position = 0; position < entities.length; position++) { const block = entities[position]; - const positionInDoc = entitiesWithoutNewBlocks.findIndex( - ([node]) => node.attrs.blockEntityId === block.entityId, + const positionInDoc = otherExistingBlockEntityIds.findIndex( + (blockEntityId) => blockEntityId === block.entityId, ); if (positionInDoc < 0) { @@ -348,9 +332,28 @@ const calculateSaveActions = ( const componentNodes = findComponentNodes(doc); let actions: UpdatePageAction[] = []; + const draftBlockEntityIds: DraftEntity["entityId"][] = []; + + doc.descendants((node) => { + if (isEntityNode(node)) { + if (node.attrs.draftId) { + const draftEntity = entityStore.draft[node.attrs.draftId]; + + if (!draftEntity || !isDraftBlockEntity(draftEntity)) { + throw new Error("Unexpected prosemirror structure"); + } + + // @todo handle entityId not being set + draftBlockEntityIds.push(draftEntity.entityId); + } + + return false; + } + }); + blocks = [...blocks]; - [actions, blocks] = removeBlocks(actions, blocks, doc, entityStore); - [actions, blocks] = moveBlocks(actions, blocks, componentNodes); + [actions, blocks] = removeBlocks(actions, blocks, draftBlockEntityIds); + [actions, blocks] = moveBlocks(actions, blocks, draftBlockEntityIds); [actions, blocks] = insertBlocks( actions, blocks, From 48c002e9911024727a412b5fa60196f19bbcf968 Mon Sep 17 00:00:00 2001 From: Nate Higgins Date: Wed, 9 Feb 2022 13:52:22 +0000 Subject: [PATCH 08/13] refactor: use entity nodes when calculating inserted blocks --- packages/hash/shared/src/entityStorePlugin.ts | 2 +- packages/hash/shared/src/prosemirror.ts | 42 +++++++++++++++- packages/hash/shared/src/save.ts | 49 +++++++++++++------ 3 files changed, 77 insertions(+), 16 deletions(-) diff --git a/packages/hash/shared/src/entityStorePlugin.ts b/packages/hash/shared/src/entityStorePlugin.ts index 75fbc855533..eaf9e444ca0 100644 --- a/packages/hash/shared/src/entityStorePlugin.ts +++ b/packages/hash/shared/src/entityStorePlugin.ts @@ -448,7 +448,7 @@ class ProsemirrorStateChangeHandler { // @todo in what circumstances does this occur if (!isDraftBlockEntity(parentEntity)) { - const componentNodeChild = findComponentNodes(node)[0][0]; + const componentNodeChild = findComponentNodes(node)[0]; addEntityStoreAction(this.state, this.tr, { type: "updateEntityProperties", diff --git a/packages/hash/shared/src/prosemirror.ts b/packages/hash/shared/src/prosemirror.ts index 850033e7b4a..5b3b4a07754 100644 --- a/packages/hash/shared/src/prosemirror.ts +++ b/packages/hash/shared/src/prosemirror.ts @@ -112,7 +112,47 @@ export const isComponentNode = ( ): node is ComponentNode => !!node.type.spec.attrs && "blockEntityId" in node.type.spec.attrs; -export const findComponentNodes = (doc: ProsemirrorNode) => { +export const findComponentNodes = ( + containingNode: ProsemirrorNode, +): ComponentNode[] => { + const componentNodes: ComponentNode[] = []; + + containingNode.descendants((node) => { + if (isComponentNode(node)) { + componentNodes.push(node); + } + + return true; + }); + + return componentNodes; +}; + +export const findComponentNode = ( + containingNode: ProsemirrorNode, + containingNodePosition: number, +): [ComponentNode, number] | null => { + let result: [ComponentNode, number] | null = null; + + containingNode.descendants((node, pos) => { + if (isComponentNode(node)) { + result = [node, containingNodePosition + 1 + pos]; + + return false; + } + + return true; + }); + + return result; +}; + +/** + * @deprecated + * @todo remove this + * @warning this can only be used on a doc + */ +export const findDocComponentNodes = (doc: ProsemirrorNode) => { const componentNodes: [ComponentNode, number][] = []; doc.descendants((node, pos) => { diff --git a/packages/hash/shared/src/save.ts b/packages/hash/shared/src/save.ts index 0d312d693a3..655f303c674 100644 --- a/packages/hash/shared/src/save.ts +++ b/packages/hash/shared/src/save.ts @@ -40,7 +40,9 @@ import { import { ComponentNode, componentNodeToId, - findComponentNodes, + EntityNode, + findComponentNode, + findDocComponentNodes, isComponentNode, isEntityNode, textBlockNodeToEntityProperties, @@ -151,18 +153,32 @@ type CreatedEntities = Map; const insertBlocks = defineOperation( ( entities: BlockEntity[], - nodes: [ComponentNode, number][], + blockEntityNodes: [EntityNode, number, string | null][], accountId: string, createdEntities: CreatedEntities, ) => { const actions: UpdatePageAction[] = []; const exists = blockEntityIdExists(entities); - for (const [position, [node, nodePosition]] of Object.entries(nodes)) { - if (exists(node.attrs.blockEntityId)) { + for (const [ + position, + [blockNode, blockNodePosition, blockEntityId], + ] of Object.entries(blockEntityNodes)) { + if (exists(blockEntityId)) { continue; } + const componentNodeResult = findComponentNode( + blockNode, + blockNodePosition, + ); + + if (!componentNodeResult) { + throw new Error("Unexpected prosemirror structure"); + } + + const [node, nodePosition] = componentNodeResult; + if (createdEntities.has(nodePosition)) { const createdEntity = createdEntities.get(nodePosition)!; @@ -329,24 +345,29 @@ const calculateSaveActions = ( entityStore: EntityStore, createdEntities: CreatedEntities, ) => { - const componentNodes = findComponentNodes(doc); + const componentNodes = findDocComponentNodes(doc); let actions: UpdatePageAction[] = []; const draftBlockEntityIds: DraftEntity["entityId"][] = []; + const draftBlockEntityNodes: [EntityNode, number, string | null][] = []; - doc.descendants((node) => { + doc.descendants((node, pos) => { if (isEntityNode(node)) { - if (node.attrs.draftId) { - const draftEntity = entityStore.draft[node.attrs.draftId]; + if (!node.attrs.draftId) { + throw new Error("Unexpected prosemirror structure"); + } - if (!draftEntity || !isDraftBlockEntity(draftEntity)) { - throw new Error("Unexpected prosemirror structure"); - } + const draftEntity = entityStore.draft[node.attrs.draftId]; - // @todo handle entityId not being set - draftBlockEntityIds.push(draftEntity.entityId); + if (!draftEntity || !isDraftBlockEntity(draftEntity)) { + throw new Error("Unexpected prosemirror structure"); } + // @todo handle entityId not being set + + draftBlockEntityNodes.push([node, pos, draftEntity.entityId]); + draftBlockEntityIds.push(draftEntity.entityId); + return false; } }); @@ -357,7 +378,7 @@ const calculateSaveActions = ( [actions, blocks] = insertBlocks( actions, blocks, - componentNodes, + draftBlockEntityNodes, accountId, createdEntities, ); From 2257325e627695381fa114d48ddc2f122ce6314f Mon Sep 17 00:00:00 2001 From: Nate Higgins Date: Wed, 9 Feb 2022 14:03:40 +0000 Subject: [PATCH 09/13] refactor: use entity nodes when calculating updated blocks --- packages/hash/shared/src/prosemirror.ts | 19 ------------------- packages/hash/shared/src/save.ts | 21 ++++++++++++--------- 2 files changed, 12 insertions(+), 28 deletions(-) diff --git a/packages/hash/shared/src/prosemirror.ts b/packages/hash/shared/src/prosemirror.ts index 5b3b4a07754..9dd2dfeb398 100644 --- a/packages/hash/shared/src/prosemirror.ts +++ b/packages/hash/shared/src/prosemirror.ts @@ -147,25 +147,6 @@ export const findComponentNode = ( return result; }; -/** - * @deprecated - * @todo remove this - * @warning this can only be used on a doc - */ -export const findDocComponentNodes = (doc: ProsemirrorNode) => { - const componentNodes: [ComponentNode, number][] = []; - - doc.descendants((node, pos) => { - if (isComponentNode(node)) { - componentNodes.push([node, pos]); - } - - return true; - }); - - return componentNodes; -}; - export const getComponentNodeAttrs = ( entity?: { entityId?: string | null } | null, ): ComponentNodeAttrs => ({ diff --git a/packages/hash/shared/src/save.ts b/packages/hash/shared/src/save.ts index 655f303c674..dcbee768008 100644 --- a/packages/hash/shared/src/save.ts +++ b/packages/hash/shared/src/save.ts @@ -38,11 +38,9 @@ import { UpdatePageContentsMutationVariables, } from "./graphql/apiTypes.gen"; import { - ComponentNode, componentNodeToId, EntityNode, findComponentNode, - findDocComponentNodes, isComponentNode, isEntityNode, textBlockNodeToEntityProperties, @@ -146,6 +144,7 @@ const moveBlocks = defineOperation( type CreatedEntities = Map; +type BlockEntityNodeDescriptor = [EntityNode, number, string | null]; /** * @warning this does not apply its actions to the entities it returns as it is * not necessary for the pipeline of calculations. Be wary of this. @@ -153,7 +152,7 @@ type CreatedEntities = Map; const insertBlocks = defineOperation( ( entities: BlockEntity[], - blockEntityNodes: [EntityNode, number, string | null][], + blockEntityNodes: BlockEntityNodeDescriptor[], accountId: string, createdEntities: CreatedEntities, ) => { @@ -227,7 +226,7 @@ const insertBlocks = defineOperation( const updateBlocks = defineOperation( ( entities: BlockEntity[], - nodes: [ComponentNode, number][], + nodes: BlockEntityNodeDescriptor[], entityStore: EntityStore, ) => { const exists = blockEntityIdExists(entities); @@ -249,12 +248,17 @@ const updateBlocks = defineOperation( * create a list of entities that we need to post updates to via * GraphQL */ - .flatMap(([node]) => { - const { blockEntityId } = node.attrs; + .flatMap(([blockNode, blockNodePos, blockEntityId]) => { if (!exists(blockEntityId)) { return []; } + const node = findComponentNode(blockNode, blockNodePos)?.[0]; + + if (!node) { + throw new Error("Unexpected prosemirror structure"); + } + const savedEntity = entityStore.saved[blockEntityId]; if (!savedEntity) { @@ -345,11 +349,10 @@ const calculateSaveActions = ( entityStore: EntityStore, createdEntities: CreatedEntities, ) => { - const componentNodes = findDocComponentNodes(doc); let actions: UpdatePageAction[] = []; const draftBlockEntityIds: DraftEntity["entityId"][] = []; - const draftBlockEntityNodes: [EntityNode, number, string | null][] = []; + const draftBlockEntityNodes: BlockEntityNodeDescriptor[] = []; doc.descendants((node, pos) => { if (isEntityNode(node)) { @@ -382,7 +385,7 @@ const calculateSaveActions = ( accountId, createdEntities, ); - [actions] = updateBlocks(actions, blocks, componentNodes, entityStore); + [actions] = updateBlocks(actions, blocks, draftBlockEntityNodes, entityStore); return actions; }; From 6c653b5382128ba5ca0fb8d97cdc0b0db496cfba Mon Sep 17 00:00:00 2001 From: Nate Higgins Date: Wed, 9 Feb 2022 14:47:11 +0000 Subject: [PATCH 10/13] fix: remove blockEntityId from componentNodes, fixing bug clearing content --- packages/hash/api/src/collab/Instance.ts | 14 +-- .../shared/src/ProsemirrorSchemaManager.ts | 88 +++++++++---------- packages/hash/shared/src/prosemirror.ts | 23 ++--- packages/hash/shared/src/schema.ts | 5 +- .../hash/shared/src/wrapEntitiesPlugin.ts | 10 --- patches/@types+prosemirror-model+1.16.0.patch | 13 ++- 6 files changed, 66 insertions(+), 87 deletions(-) diff --git a/packages/hash/api/src/collab/Instance.ts b/packages/hash/api/src/collab/Instance.ts index ee884284c2d..464da1c9e7f 100644 --- a/packages/hash/api/src/collab/Instance.ts +++ b/packages/hash/api/src/collab/Instance.ts @@ -15,17 +15,13 @@ import { entityStorePluginState, } from "@hashintel/hash-shared/entityStorePlugin"; import { - LatestEntityRef, GetBlocksQuery, GetBlocksQueryVariables, GetPageQuery, GetPageQueryVariables, + LatestEntityRef, } from "@hashintel/hash-shared/graphql/apiTypes.gen"; -import { - getComponentNodeAttrs, - isComponentNode, - isEntityNode, -} from "@hashintel/hash-shared/prosemirror"; +import { isEntityNode } from "@hashintel/hash-shared/prosemirror"; import { ProsemirrorSchemaManager } from "@hashintel/hash-shared/ProsemirrorSchemaManager"; import { getBlocksQuery, @@ -430,12 +426,6 @@ export class Instance { }, }); } - } else if (isComponentNode(node)) { - const nodeAttrs = getComponentNodeAttrs(blockEntity); - - if (!isEqual(node.attrs, nodeAttrs)) { - transform.setNodeMarkup(mapping.map(pos), undefined, nodeAttrs); - } } }); diff --git a/packages/hash/shared/src/ProsemirrorSchemaManager.ts b/packages/hash/shared/src/ProsemirrorSchemaManager.ts index 0f8cfd76f0d..34344044a08 100644 --- a/packages/hash/shared/src/ProsemirrorSchemaManager.ts +++ b/packages/hash/shared/src/ProsemirrorSchemaManager.ts @@ -1,6 +1,6 @@ import { BlockVariant } from "blockprotocol"; import { isString } from "lodash"; -import { ProsemirrorNode, NodeSpec, Schema } from "prosemirror-model"; +import { NodeSpec, ProsemirrorNode, Schema } from "prosemirror-model"; import { EditorState, Transaction } from "prosemirror-state"; import { EditorProps, EditorView } from "prosemirror-view"; @@ -28,21 +28,35 @@ import { entityStorePluginStateFromTransaction, newDraftId, } from "./entityStorePlugin"; -import { childrenForTextEntity, getComponentNodeAttrs } from "./prosemirror"; +import { + childrenForTextEntity, + componentNodeGroupName, + isComponentNode, + isComponentNodeType, +} from "./prosemirror"; declare interface OrderedMapPrivateInterface { content: (string | T)[]; } -const createComponentNodeSpec = (spec: Partial): NodeSpec => ({ - ...spec, - selectable: false, - group: "componentNode", - attrs: { - ...(spec.attrs ?? {}), - // @todo remove this - blockEntityId: { default: "" }, +const createComponentNodeSpec = ( + spec: Omit, "group">, +): NodeSpec => ({ + /** + * @todo consider if we should encode the component id here / any other + * information + */ + toDOM: (node) => { + if (node.type.isTextblock) { + return ["span", { "data-hash-type": "component" }, 0]; + } else { + return ["span", { "data-hash-type": "component" }]; + } }, + selectable: false, + + ...spec, + group: componentNodeGroupName, }); type NodeViewFactory = NonNullable["nodeViews"]>[string]; @@ -122,18 +136,6 @@ export class ProsemirrorSchemaManager { this.prepareToDisableBlankDefaultComponentNode(); const spec = createComponentNodeSpec({ - /** - * @todo consider if we should encode the component id here / any other - * information - */ - toDOM: (node) => { - if (node.type.isTextblock) { - return ["span", { "data-hash-type": "component" }, 0]; - } else { - return ["span", { "data-hash-type": "component" }]; - } - }, - /** * Currently we detect whether a block takes editable text by detecting if * it has an editableRef prop in its schema – we need a more sophisticated @@ -167,16 +169,21 @@ export class ProsemirrorSchemaManager { private prepareToDisableBlankDefaultComponentNode() { const blankType = this.schema.nodes.blank; - if (blankType.spec.group === "componentNode") { - delete blankType.spec.group; - } + if (isComponentNodeType(blankType)) { + if (blankType.spec.group?.includes(componentNodeGroupName)) { + if (blankType.spec.group !== componentNodeGroupName) { + throw new Error( + "Blank node type has group expression more complicated than we can handle", + ); + } - // Accessing private API here, hence the casting - const groups = (blankType as any).groups as string[]; + delete blankType.spec.group; + } - const idx = groups.indexOf("componentNode"); - if (idx > -1) { - groups.splice(idx, 1); + blankType.groups!.splice( + blankType.groups!.indexOf(componentNodeGroupName), + 1, + ); } } @@ -239,10 +246,7 @@ export class ProsemirrorSchemaManager { async ensureDocDefined(doc: ProsemirrorNode) { const componentIds = new Set(); doc.descendants((node) => { - if ( - node.type.spec.group === "componentNode" && - node.type.name.startsWith("http") - ) { + if (isComponentNode(node) && node.type.name.startsWith("http")) { componentIds.add(node.type.name); } @@ -289,8 +293,6 @@ export class ProsemirrorSchemaManager { } } - const componentNodeAttributes = getComponentNodeAttrs(blockEntity); - if (requiresText) { const draftTextEntity = draftBlockId && entityStore @@ -317,12 +319,7 @@ export class ProsemirrorSchemaManager { { draftId: draftTextEntity?.draftId, }, - [ - this.schema.nodes[targetComponentId].create( - componentNodeAttributes, - content, - ), - ], + [this.schema.nodes[targetComponentId].create({}, content)], ), ]), ]); @@ -340,12 +337,7 @@ export class ProsemirrorSchemaManager { ? blockEntity.properties.entity.draftId : null, }, - [ - this.schema.nodes[targetComponentId].create( - componentNodeAttributes, - [], - ), - ], + [this.schema.nodes[targetComponentId].create({}, [])], ), ), ]); diff --git a/packages/hash/shared/src/prosemirror.ts b/packages/hash/shared/src/prosemirror.ts index 9dd2dfeb398..55a160fc9ce 100644 --- a/packages/hash/shared/src/prosemirror.ts +++ b/packages/hash/shared/src/prosemirror.ts @@ -1,4 +1,4 @@ -import { ProsemirrorNode, Schema } from "prosemirror-model"; +import { NodeType, ProsemirrorNode, Schema } from "prosemirror-model"; import { Text } from "./graphql/apiTypes.gen"; import { TextToken } from "./graphql/types"; @@ -91,9 +91,7 @@ type NodeWithAttrs = Omit< "attrs" > & { attrs: Attrs }; -type ComponentNodeAttrs = { - blockEntityId: string | null; -}; +type ComponentNodeAttrs = {}; export type ComponentNode = NodeWithAttrs; export type EntityNode = NodeWithAttrs<{ @@ -104,13 +102,14 @@ export const isEntityNode = ( node: ProsemirrorNode | null, ): node is EntityNode => !!node && node.type === node.type.schema.nodes.entity; -/** - * @todo use group name for this - */ +export const componentNodeGroupName = "componentNode"; + +export const isComponentNodeType = (nodeType: NodeType) => + nodeType.groups?.includes(componentNodeGroupName) ?? false; + export const isComponentNode = ( node: ProsemirrorNode, -): node is ComponentNode => - !!node.type.spec.attrs && "blockEntityId" in node.type.spec.attrs; +): node is ComponentNode => isComponentNodeType(node.type); export const findComponentNodes = ( containingNode: ProsemirrorNode, @@ -147,10 +146,4 @@ export const findComponentNode = ( return result; }; -export const getComponentNodeAttrs = ( - entity?: { entityId?: string | null } | null, -): ComponentNodeAttrs => ({ - blockEntityId: entity?.entityId ?? "", -}); - export const componentNodeToId = (node: ComponentNode) => node.type.name; diff --git a/packages/hash/shared/src/schema.ts b/packages/hash/shared/src/schema.ts index c23b3167484..63cab4a2adf 100644 --- a/packages/hash/shared/src/schema.ts +++ b/packages/hash/shared/src/schema.ts @@ -1,3 +1,4 @@ +import { componentNodeGroupName } from "@hashintel/hash-shared/prosemirror"; import { Schema } from "prosemirror-model"; export const createSchema = () => @@ -15,8 +16,10 @@ export const createSchema = () => * node from the componentNode group, which ensures that when * Prosemirror attempts to instantiate a componentNode it uses that * node instead of the blank one + * + * @see import("./ProsemirrorSchemaManager").ProsemirrorSchemaManager#prepareToDisableBlankDefaultComponentNode */ - group: "componentNode", + group: componentNodeGroupName, toDOM: () => ["div", 0] as const, }, block: { diff --git a/packages/hash/shared/src/wrapEntitiesPlugin.ts b/packages/hash/shared/src/wrapEntitiesPlugin.ts index e4409ccab44..b3de8459160 100644 --- a/packages/hash/shared/src/wrapEntitiesPlugin.ts +++ b/packages/hash/shared/src/wrapEntitiesPlugin.ts @@ -59,16 +59,6 @@ const ensureEntitiesAreWrapped = ( throw new Error("Cannot rewrap"); } - /** - * @todo we won't need this once we remove blockEntityId from the - * component node - */ - if (wrappers && !wrapperNodes) { - tr.setNodeMarkup(tr.mapping.map(position), undefined, { - blockEntityId: null, - }); - } - /** * In the event that a block is not fully wrapped (i.e. is _not_ a block node), we provide a fallback * in case wrapperNodes were not provided. diff --git a/patches/@types+prosemirror-model+1.16.0.patch b/patches/@types+prosemirror-model+1.16.0.patch index 13720333cd6..95d3cb5515b 100644 --- a/patches/@types+prosemirror-model+1.16.0.patch +++ b/patches/@types+prosemirror-model+1.16.0.patch @@ -1,5 +1,5 @@ diff --git a/node_modules/@types/prosemirror-model/index.d.ts b/node_modules/@types/prosemirror-model/index.d.ts -index 3e5aa92..4040b63 100755 +index 3e5aa92..243b5d6 100755 --- a/node_modules/@types/prosemirror-model/index.d.ts +++ b/node_modules/@types/prosemirror-model/index.d.ts @@ -103,8 +103,14 @@ export class Fragment { @@ -33,3 +33,14 @@ index 3e5aa92..4040b63 100755 /** * Error type raised by [`Node.replace`](#model.Node.replace) when * given an invalid replacement. +@@ -984,6 +998,10 @@ export class NodeType { + * A link back to the `Schema` the node type belongs to. + */ + schema: S; ++ /** ++ * Parsed set of groups from spec.group ++ */ ++ groups?: string[]; + /** + * The spec that this type is based on + */ From f0d254a5d74698807dfffffde79eac0a443d0b5a Mon Sep 17 00:00:00 2001 From: Nate Higgins Date: Sat, 19 Feb 2022 01:28:50 +0000 Subject: [PATCH 11/13] refactor: remove unused save mapping --- packages/hash/api/src/collab/Instance.ts | 13 ------------- 1 file changed, 13 deletions(-) diff --git a/packages/hash/api/src/collab/Instance.ts b/packages/hash/api/src/collab/Instance.ts index 464da1c9e7f..1ec96a57ed4 100644 --- a/packages/hash/api/src/collab/Instance.ts +++ b/packages/hash/api/src/collab/Instance.ts @@ -79,7 +79,6 @@ export class Instance { userCount = 0; waiting: Waiting[] = []; saveChain = Promise.resolve(); - saveMapping: Mapping | null = null; collecting: ReturnType | null = null; clientIds = new WeakMap(); @@ -288,9 +287,6 @@ export class Instance { const result = tr.maybeStep(steps[i]); if (!result.doc) return false; - if (this.saveMapping) { - this.saveMapping.appendMap(steps[i].getMap()); - } } for (const action of actions) { @@ -317,8 +313,6 @@ export class Instance { }; save = (apolloClient: ApolloClient) => (clientID: string) => { - const mapping = new Mapping(); - this.saveChain = this.saveChain .catch() .then(async () => { @@ -340,7 +334,6 @@ export class Instance { return createdEntities; }) .then((createdEntities) => { - this.saveMapping = mapping; const { doc } = this.state; const store = entityStorePluginState(this.state); @@ -462,12 +455,6 @@ export class Instance { }) .catch((err) => { logger.error("could not save", err); - }) - - .finally(() => { - if (this.saveMapping === mapping) { - this.saveMapping = null; - } }); }; From 214c858ca45d47640baea265f7aa14575a07ef3a Mon Sep 17 00:00:00 2001 From: Nate Higgins Date: Mon, 21 Feb 2022 02:19:42 +0000 Subject: [PATCH 12/13] Remove unused transform --- packages/hash/api/src/collab/Instance.ts | 27 +----------------------- 1 file changed, 1 insertion(+), 26 deletions(-) diff --git a/packages/hash/api/src/collab/Instance.ts b/packages/hash/api/src/collab/Instance.ts index 1ec96a57ed4..1c7a847266c 100644 --- a/packages/hash/api/src/collab/Instance.ts +++ b/packages/hash/api/src/collab/Instance.ts @@ -35,7 +35,7 @@ import { Response } from "express"; import { isEqual, memoize, pick } from "lodash"; import { Schema } from "prosemirror-model"; import { EditorState } from "prosemirror-state"; -import { Mapping, Step, Transform } from "prosemirror-transform"; +import { Step } from "prosemirror-transform"; import { logger } from "../logger"; import { EntityWatcher } from "./EntityWatcher"; import { InvalidVersionError } from "./errors"; @@ -346,13 +346,6 @@ export class Instance { apolloClient, createdEntities, ).then((newPage) => { - /** - * This is purposefully based on the current doc, not the doc at - * the time of save, because we need to apply transforms to the - * current doc based on the result of the save query (in order to - * insert entity ids for new blocks) - */ - const transform = new Transform(this.state.doc); const actions: EntityStorePluginAction[] = []; /** @@ -422,14 +415,6 @@ export class Instance { } }); - /** - * We're posting actions and steps from the post-save - * transformation process for maximum safety – as we need to - * ensure the actions are processed before the steps, and that's - * not easy to do in one message right now - * - * @todo combine the two calls to addEvents - */ if (actions.length) { this.addEvents(apolloClient)( this.version, @@ -440,16 +425,6 @@ export class Instance { ); } - if (transform.docChanged) { - this.addEvents(apolloClient)( - this.version, - transform.steps, - `${clientID}-server`, - [], - false, - ); - } - this.updateSavedContents(newPage.properties.contents); }); }) From bb0e0e86610d25d6c0fd6567ad2a678e6e5fc716 Mon Sep 17 00:00:00 2001 From: Alexander Kachkaev Date: Mon, 21 Feb 2022 19:23:20 +0000 Subject: [PATCH 13/13] Delete collecting and saveMapping --- packages/hash/api/src/collab/Instance.ts | 2 -- 1 file changed, 2 deletions(-) diff --git a/packages/hash/api/src/collab/Instance.ts b/packages/hash/api/src/collab/Instance.ts index 70f17d06531..647eb19b4d3 100644 --- a/packages/hash/api/src/collab/Instance.ts +++ b/packages/hash/api/src/collab/Instance.ts @@ -77,8 +77,6 @@ export class Instance { lastActive = Date.now(); waiting: Waiting[] = []; saveChain = Promise.resolve(); - collecting: ReturnType | null = null; - saveMapping: Mapping | null = null; clientIds = new WeakMap(); positionPollers: CollabPositionPoller[] = [];