Prevent collab improperly clearing text blocks on save

[nathggns]

🌟 What is the purpose of this PR?

This fixes a bug where the content of new text blocks is being improperly cleared on save. It does that by removing the blockEntityId attribute for Prosemirror nodes, which removes the need to update the Prosemirror document when creating blocks.

It also re-enables the playwright test which was failing due to this bug.

You can see this bug occurring in https://app.replay.io/recording/59943358-a173-48d3-ac76-5fc778467eda, which was generated using a replay-enabled fork of Playwright. See replayio/playwright#9 (comment) for details.

⚠️ Known issues

Updating blockEntityId on save should not have cleared the content of the block, and it indicates something is going wrong with the application of actions. This is tracked by https://app.asana.com/0/1200211978612931/1201720874843710/f (internal).

🐾 Next steps

We'll look at fixing the above in the future.

🔍 What does this change?

• page-creation.spec.ts has been updated to re-enable the previous flaky test of rapid text insertion in new blocks which were disabled in Remove a few steps from Playwright tests to reduce CI flakiness #232
• isComponentNode now detects component nodes by whether their node type belongs to the componentNode group, rather than by checking for a blockEntityId attribute.
  • The name for the component node group is now available as the componentNodeGroupName variable instead of duplicated all over the place.
  • @types/prosemirror-model has been patched to make the groups field on NodeType available for use. This is incorrectly left out of the community types. [prosemirror-model]: NodeType is missing its groups property DefinitelyTyped/DefinitelyTyped#58892
  • New isComponentNodeType helper to check if a node type belongs to the component node group. Used by isComponentNode
• BlockView has been updated to retrieve the entity id of the block (for the "link to block" functionality) by using the entity store and the draft id of the block entity node, instead of by using the blockEntityId attribute of the component node
• ComponentView has been update to get the entity id of the block from the entity store, instead of by using the blockEntityId attribute of the component node
• New findComponentNode API which is used for finding a single component node within a containing node. The signature for this requires knowing the position of the containing node, so we can be sure the positions returned are absolute positions, and not relative to the containing node
  • The save logic now uses this instead of findComponentNodes. It doesn't need to know the position of the component node, but it did always only need to find a single component node within the block node, so it makes sense to use this instead of the one which returns all component nodes.
  • findComponentNodes no longer returns the position of the nodes it finds
    • This was no longer necessary as the save logic no longer uses it, and only that usage of findComponentNodes required the position
    • The positions reported by findComponentNodes were always relative to the top level node given to it, rather than their absolute position in the document. If you pass a doc node to findComponentNodes, they're the same thing, but it could lead to confusion so it was sensible to simplify the API
• The save logic no longer uses blockEntityId from component nodes. Instead, it references block entity nodes and the entity store to calculate save operations
• ProsemirrorSchemaManager now creates component nodes with no props, as no uses of blockEntityId remain
  • This means getComponentNodeAttrs has been removed
  • Also means the wrapEntitiesPlugin has been updated to not have to remove blockEntityId when text blocks are split in half
• More static properties of the spec for component nodes has been moved into the definition of createComponentNodeSpec instead of at its call site – specifically toDOM.
• Instance has been updated to no longer send Prosemirror steps setting the blockEntityId of component nodes post-save, as it isn't used anymore
  • This is the main purpose of the above changes
  • These steps are written in a way that include their content. This means the draft content is getting reset on the client when applying these steps. Rebasing is supposed to handle this, but that's where the flakiness / fragility is coming it.
  • This allows removing having to keep track of changes made to the document in between a save being initiated and its response returning, which was done with saveMapping

📜 Does this require a change to the docs?

No

🔗 Related links

• Asana task Prevent unstable application of actions in collab leading to text being improperly cleared from blocks (internal)

🛡 What tests cover this?

• page-creation.spec.ts playwright tests, "user can create page".

❓ How to test this?

Run the playwright tests. This was never a bug we saw in person as it depended on speed.

You should also just ensure the general stability of the system in creating blocks, making changes and seeing them be persisted, and in linking to blocks.

📹 Demo

N/A

[semgrep-app]

Detected user input used in bracket notation accessor. This could lead to object injection through node.attrs.draftId, which could grant access to every property available in the object and therefore sensitive information. Instead, avoid the use of user input in property name fields or create a whitelist of allowed input.

Click here if this comment is not useful

^{This comment is advisory. You do not need to address it before merging this pull request.}
(javascript.lang.security.audit.detect-bracket-object-injection.detect-bracket-object-injection in Security Policy)

[semgrep-app]

Detected user input used in bracket notation accessor. This could lead to object injection through blockEntityNode.attrs.draftId, which could grant access to every property available in the object and therefore sensitive information. Instead, avoid the use of user input in property name fields or create a whitelist of allowed input.

Click here if this comment is not useful

^{This comment is advisory. You do not need to address it before merging this pull request.}
(javascript.lang.security.audit.detect-bracket-object-injection.detect-bracket-object-injection in Security Policy)

[lgtm-com]

This pull request introduces 1 alert when merging f0d254a into 232e3ac - view on LGTM.com

new alerts:

• 1 for Unused variable, import, function or class