From 13f9bddced0d2faa298c909d8c3964fd0f6d05e3 Mon Sep 17 00:00:00 2001
From: Kevin Ansfield <kevin@lookingsideways.co.uk>
Date: Wed, 18 Oct 2023 13:24:45 +0100
Subject: [PATCH] update comments

---
 packages/kg-default-transforms/package.json   |  3 +-
 .../src/transforms/denest.ts                  | 36 +++++++++++++------
 2 files changed, 28 insertions(+), 11 deletions(-)

diff --git a/packages/kg-default-transforms/package.json b/packages/kg-default-transforms/package.json
index 0802ff5402..f280ce1bfc 100644
--- a/packages/kg-default-transforms/package.json
+++ b/packages/kg-default-transforms/package.json
@@ -11,7 +11,8 @@
     "dev": "tsc --watch --preserveWatchOutput --sourceMap",
     "build": "tsc",
     "prepare": "tsc",
-    "test:unit": "NODE_ENV=testing c8 --src src --all --check-coverage --100 --reporter text --reporter cobertura mocha -r ts-node/register './test/**/*.test.ts'",
+    "pretest": "tsc",
+    "test:unit": "NODE_ENV=testing c8 --src build --all --check-coverage --100 --reporter text --reporter cobertura mocha -r ts-node/register './test/**/*.test.ts'",
     "test": "yarn test:types && yarn test:unit",
     "test:types": "tsc --noEmit",
     "lint:code": "eslint src/ --ext .ts --cache",
diff --git a/packages/kg-default-transforms/src/transforms/denest.ts b/packages/kg-default-transforms/src/transforms/denest.ts
index bbc7424fdf..2c3bd19bb5 100644
--- a/packages/kg-default-transforms/src/transforms/denest.ts
+++ b/packages/kg-default-transforms/src/transforms/denest.ts
@@ -3,11 +3,28 @@ import {ElementNode, $createParagraphNode, LexicalEditor, LexicalNode, Klass, $g
 
 export type CreateNodeFn<T extends LexicalNode> = (originalNode: T) => T;
 
+// Through pasting content or converting from HTML in the API (ultimately the same path as pasting)
+// it's possible for the editor to end up with nested element nodes. Situations we've seen:
+// - headers inside paragraphs
+// - image decorator nodes inside paragraphs
+// - image decorator nodes inside lists/list items
+// - large swathes of content nested inside a paragraph/heading node
+// - etc.
+//
+// This invalid nesting causes numerous issues inside the editor as we only support our "card"
+// decorator nodes at the top-level meaning selection, deletion, etc gets very confused when nested.
+// Our renderer is also not designed to handle nested element nodes meaning things may _look_ fine
+// in the editor but not render correctly or not render at all.
+//
+// With this transform we attempt to detect and fix invalid nesting by pulling out any non-inline
+// children from the passed-in node and inserting them after the node's top-level parent. We need
+// to move the nested nodes rather than remove them so we don't lose any pasted/imported content.
+
 export function denestTransform<T extends ElementNode>(node: T, createNode: CreateNodeFn<T>) {
     const children = node.getChildren();
 
     const hasInvalidChild = children.some((child: LexicalNode) => {
-        return !$isListNode(child) && !$isListItemNode(child) && child.isInline && !child.isInline();
+        return child.isInline && !child.isInline() && !$isListNode(child) && !$isListItemNode(child);
     });
 
     if (!hasInvalidChild) {
@@ -21,11 +38,11 @@ export function denestTransform<T extends ElementNode>(node: T, createNode: Crea
 
     // we need a new node of the current node type to collect inline
     // children so we can maintain order when moving the non-inline children
-    // out. Will be appended and replaced with a new node each time we
-    // find a non-inline child
+    // out. Will be appended to the temp paragraph and the var replaced with a
+    // new node each time we find a non-inline child
     let currentElementNode = createNode(node);
 
-    // pull out any non-inline children as we don't support nested element nodes
+    // pull any non-inline children out into the temp paragraph
     children.forEach((child: LexicalNode) => {
         if (!$isListNode(child) && !$isListItemNode(child) && child.isInline && !child.isInline()) {
             if (currentElementNode.getChildrenSize() > 0) {
@@ -38,7 +55,7 @@ export function denestTransform<T extends ElementNode>(node: T, createNode: Crea
         }
     });
 
-    // append any remaining text nodes
+    // append any remaining nodes from the current element node holder
     if (currentElementNode.getChildrenSize() > 0) {
         tempParagraph.append(currentElementNode);
     }
@@ -50,17 +67,16 @@ export function denestTransform<T extends ElementNode>(node: T, createNode: Crea
         parent = parent.getParentOrThrow();
     }
 
-    // reverse because we can only insertAfter the current paragraph
-    // so we need to insert the first child last to maintain order.
+    // reverse order because we can only insertAfter the parent node
+    // meaning first child needs to be inserted last to maintain order.
     tempParagraph.getChildren().reverse().forEach((child) => {
         parent.insertAfter(child);
     });
 
-    // remove the original paragraph
+    // remove the original node - it's now empty
     node.remove();
 
-    // clean up the temporary paragraph immediately, although it should be
-    // cleaned up by the reconciler's garbage collection of detached nodes
+    // clean up the temporary detached paragraph immediately as we know it's no longer needed
     tempParagraph.remove();
 }