fix(comments): don't drop DOCX comments containing bookmark nodes (#3828)

[gpardhivvarma]

Summary

Fixes #3828. DOCX comments containing w:bookmarkStart / w:bookmarkEnd nodes were silently dropped from the loaded comments list, with RangeError: Unknown node type: bookmarkStart / Failed to convert comment logged to the console.

Root cause

A comment's imported ProseMirror JSON (comment.elements) is produced against the full DOCX schema, which registers bookmarkStart/bookmarkEnd. But getHtmlFromComment renders the comment's display HTML with a throwaway headless Editor built from the reduced rich-text schema (getRichTextExtensions()), which does not register those nodes.

normalizeCommentForEditor forwarded the bookmark nodes verbatim, so ProseMirror's nodeFromJSON threw Unknown node type: bookmarkStart. The try/catch in getHtmlFromComment swallowed the error and returned undefined, and the guard if (!htmlContent && !comment.trackedChange) return; in processLoadedDocxComments then discarded the entire comment.

Fix

Normalize defensively: drop or unwrap any node type the rich-text schema does not register — not just bookmarks — so this whole class of crash is prevented (stray lineBreak, tab, etc. would fail the same way). The supported-node set is derived directly from the extension objects' .type/.name (ProseMirror adds no implicit nodes, so no schema construction is needed).

• Invisible leaf nodes (e.g. bookmark boundaries) are dropped.
• Nodes wrapping inline content are unwrapped so the visible text survives.
• The original docxCommentJSON is untouched, so exported bookmark metadata is preserved on round-trip.

The normalizer is extracted into a pure comment-normalize.js module so it can be unit-tested directly.

Tests

• comment-normalize.test.js (new) — fast, pure unit tests: bookmark stripping, unknown-node unwrap/drop, supported-node passthrough, textStyle font-attr stripping, and the empty-set fallback (strip bookmarks only).
• comments-store.bookmark.test.js (new) — end-to-end regression test using the real headless Editor (the existing comments-store.test.js mocks Editor, so it could not reproduce the schema throw). Verified to fail before the fix (comment dropped) and pass after.
• All 136 existing comments-store tests still pass.

Files

• packages/superdoc/src/stores/comment-normalize.js (new) — defensive normalizer + getRichTextSupportedNodeNames.
• packages/superdoc/src/stores/comments-store.js — import the helpers, remove the inline normalizer, thread the supported-node set through getHtmlFromComment.
• packages/superdoc/src/stores/comment-normalize.test.js, packages/superdoc/src/stores/comments-store.bookmark.test.js (new tests).

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 258a63949a

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

[qodo-code-review]

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0) 📜 Skill insights (0)

Context used

✅ Tickets: 🎫 DOCX comment bookmarks cause `Unknown node type: bookmarkStart` and drop comments

1. Fake timers not restored ✓ Resolved 🐞 Bug ☼ Reliability

Description

comments-store.bookmark.test.js enables Vitest fake timers in beforeEach but never restores real
timers, which can leak fake timers into later tests in the same worker and cause order-dependent
hangs/timeouts.

Code

packages/superdoc/src/stores/comments-store.bookmark.test.js[R100-106]

+  beforeEach(() => {
+    vi.useFakeTimers();
+    vi.clearAllMocks();
+    setActivePinia(createPinia());
+    store = useCommentsStore();
+    __mockSuperdoc.documents.value = [{ id: 'doc-1', type: 'docx' }];
+  });

Relevance

⭐⭐⭐ High

Test contamination cleanup patterns accepted before; restoring globals in teardown (analogous to
vi.useRealTimers) was accepted in PR #3181.

PR-#3181

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The new test turns on fake timers but never turns them off; a nearby test file shows the established
cleanup pattern used in this repo.

packages/superdoc/src/stores/comments-store.bookmark.test.js[1-128]
packages/superdoc/src/stores/comments-store.test.js[143-191]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`comments-store.bookmark.test.js` calls `vi.useFakeTimers()` in `beforeEach` but does not call `vi.useRealTimers()` afterward. This can contaminate other tests executed in the same Vitest worker.

### Issue Context
The existing `comments-store.test.js` file already follows the safe pattern of restoring real timers in `afterEach`.

### Fix Focus Areas
- packages/superdoc/src/stores/comments-store.bookmark.test.js[1-110]

### Suggested fix
- Import `afterEach` from `vitest`.
- Add:
 ```js
 afterEach(() => {
   vi.useRealTimers();
 });
 ```
- (Optional) If this test doesn’t rely on timers at all, remove `vi.useFakeTimers()` entirely.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

2. Tabs/lineBreaks dropped ✓ Resolved 🐞 Bug ≡ Correctness

Description

normalizeCommentForEditor drops any unsupported leaf node by returning null, so imported DOCX
comments containing leaf nodes like tab/lineBreak/hardBreak will lose that visible
spacing/formatting in the rendered comment HTML.

Code

packages/superdoc/src/stores/comment-normalize.js[R43-56]

+  // Drop invisible bookmark boundary nodes and any node absent from the rich-text
+  // schema used to render comment HTML, preserving inline content they wrap so the
+  // visible text survives. Falls back to stripping only bookmark boundary nodes when
+  // the supported-node set can't be determined (size 0). (#3828)
+  const isBoundaryNode = node.type === 'bookmarkStart' || node.type === 'bookmarkEnd';
+  const isUnsupported = supportedNodeNames && supportedNodeNames.size > 0 && !supportedNodeNames.has(node.type);
+  if (isBoundaryNode || isUnsupported) {
+    return Array.isArray(node.content)
+      ? node.content
+          .map((child) => normalizeCommentForEditor(child, supportedNodeNames))
+          .flat()
+          .filter(Boolean)
+      : null;
+  }

Relevance

⭐⭐ Medium

No historical evidence for preserving tab/lineBreak nodes in comment normalization; likely product
tradeoff vs crash prevention.

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The normalizer explicitly returns null for unsupported leaf nodes, and the reduced rich-text
extension set excludes line-break/tab nodes that exist in the full DOCX schema, so those nodes would
be removed from comment display rendering when present.

packages/superdoc/src/stores/comment-normalize.js[33-56]
packages/super-editor/src/editors/v1/extensions/index.js[98-141]
packages/super-editor/src/editors/v1/extensions/index.js[144-169]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
The normalizer currently drops *all* unsupported leaf nodes (`return null`), which removes visible separators like tabs/line breaks when they appear in imported comment JSON.

### Issue Context
The reduced rich-text extension set (`getRichTextExtensions`) omits `LineBreak`/`HardBreak`/`TabNode`, while the full DOCX schema includes them; comments are imported against the full schema, so these node types are plausible in `comment.elements`.

### Fix Focus Areas
- packages/superdoc/src/stores/comment-normalize.js[43-56]
- packages/super-editor/src/editors/v1/extensions/index.js[98-141]
- packages/super-editor/src/editors/v1/extensions/index.js[144-169]

### Suggested fix
Implement an explicit preservation/mapping for known visible unsupported leaf nodes before the generic `isUnsupported` drop logic. For example:
- Map `tab` to a text node (e.g., `{ type: 'text', text: '\t' }` or spaces).
- Map `lineBreak`/`hardBreak` to a representation that survives HTML rendering (e.g., split paragraphs, or map to text with `\n` if the consumer preserves whitespace).

Alternatively (if acceptable for the comment-rendering editor), extend the headless comment Editor’s extension list to include just the missing leaf-node extensions so `nodeFromJSON` can load them without stripping.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

Previous review results

Review updated until commit 547d560

Results up to commit 258a639

🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0) 🎨 UX issues (0) 🔗 Cross-repo conflicts (0) 📜 Skill insights (0)

1. Fake timers not restored ✓ Resolved 🐞 Bug ☼ Reliability

Description

comments-store.bookmark.test.js enables Vitest fake timers in beforeEach but never restores real
timers, which can leak fake timers into later tests in the same worker and cause order-dependent
hangs/timeouts.

Code

packages/superdoc/src/stores/comments-store.bookmark.test.js[R100-106]

+  beforeEach(() => {
+    vi.useFakeTimers();
+    vi.clearAllMocks();
+    setActivePinia(createPinia());
+    store = useCommentsStore();
+    __mockSuperdoc.documents.value = [{ id: 'doc-1', type: 'docx' }];
+  });

Relevance

⭐⭐⭐ High

Test contamination cleanup patterns accepted before; restoring globals in teardown (analogous to
vi.useRealTimers) was accepted in PR #3181.

PR-#3181

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The new test turns on fake timers but never turns them off; a nearby test file shows the established
cleanup pattern used in this repo.

packages/superdoc/src/stores/comments-store.bookmark.test.js[1-128]
packages/superdoc/src/stores/comments-store.test.js[143-191]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`comments-store.bookmark.test.js` calls `vi.useFakeTimers()` in `beforeEach` but does not call `vi.useRealTimers()` afterward. This can contaminate other tests executed in the same Vitest worker.

### Issue Context
The existing `comments-store.test.js` file already follows the safe pattern of restoring real timers in `afterEach`.

### Fix Focus Areas
- packages/superdoc/src/stores/comments-store.bookmark.test.js[1-110]

### Suggested fix
- Import `afterEach` from `vitest`.
- Add:
 ```js
 afterEach(() => {
   vi.useRealTimers();
 });
 ```
- (Optional) If this test doesn’t rely on timers at all, remove `vi.useFakeTimers()` entirely.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

2. Tabs/lineBreaks dropped ✓ Resolved 🐞 Bug ≡ Correctness

Description

normalizeCommentForEditor drops any unsupported leaf node by returning null, so imported DOCX
comments containing leaf nodes like tab/lineBreak/hardBreak will lose that visible
spacing/formatting in the rendered comment HTML.

Code

packages/superdoc/src/stores/comment-normalize.js[R43-56]

+  // Drop invisible bookmark boundary nodes and any node absent from the rich-text
+  // schema used to render comment HTML, preserving inline content they wrap so the
+  // visible text survives. Falls back to stripping only bookmark boundary nodes when
+  // the supported-node set can't be determined (size 0). (#3828)
+  const isBoundaryNode = node.type === 'bookmarkStart' || node.type === 'bookmarkEnd';
+  const isUnsupported = supportedNodeNames && supportedNodeNames.size > 0 && !supportedNodeNames.has(node.type);
+  if (isBoundaryNode || isUnsupported) {
+    return Array.isArray(node.content)
+      ? node.content
+          .map((child) => normalizeCommentForEditor(child, supportedNodeNames))
+          .flat()
+          .filter(Boolean)
+      : null;
+  }

Relevance

⭐⭐ Medium

No historical evidence for preserving tab/lineBreak nodes in comment normalization; likely product
tradeoff vs crash prevention.

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The normalizer explicitly returns null for unsupported leaf nodes, and the reduced rich-text
extension set excludes line-break/tab nodes that exist in the full DOCX schema, so those nodes would
be removed from comment display rendering when present.

packages/superdoc/src/stores/comment-normalize.js[33-56]
packages/super-editor/src/editors/v1/extensions/index.js[98-141]
packages/super-editor/src/editors/v1/extensions/index.js[144-169]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
The normalizer currently drops *all* unsupported leaf nodes (`return null`), which removes visible separators like tabs/line breaks when they appear in imported comment JSON.

### Issue Context
The reduced rich-text extension set (`getRichTextExtensions`) omits `LineBreak`/`HardBreak`/`TabNode`, while the full DOCX schema includes them; comments are imported against the full schema, so these node types are plausible in `comment.elements`.

### Fix Focus Areas
- packages/superdoc/src/stores/comment-normalize.js[43-56]
- packages/super-editor/src/editors/v1/extensions/index.js[98-141]
- packages/super-editor/src/editors/v1/extensions/index.js[144-169]

### Suggested fix
Implement an explicit preservation/mapping for known visible unsupported leaf nodes before the generic `isUnsupported` drop logic. For example:
- Map `tab` to a text node (e.g., `{ type: 'text', text: '\t' }` or spaces).
- Map `lineBreak`/`hardBreak` to a representation that survives HTML rendering (e.g., split paragraphs, or map to text with `\n` if the consumer preserves whitespace).

Alternatively (if acceptable for the comment-rendering editor), extend the headless comment Editor’s extension list to include just the missing leaf-node extensions so `nodeFromJSON` can load them without stripping.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

[codecov-commenter]

Codecov Report

❌ Patch coverage is 99.09910% with 1 line in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
packages/superdoc/src/stores/comment-normalize.js	99.07%	1 Missing ⚠️

📢 Thoughts on this report? Let us know!

[gpardhivvarma]

hey @caio-pizzol . Can you please take a look at this. Coudn't contribute for some time because of some personal reasons. Good to be back

[gpardhivvarma]

Addressed the automated review feedback in 547d560:

• Tabs / line breaks dropped (Codex & Qodo): normalizeCommentForEditor no longer maps every unsupported leaf to null. Visible whitespace leaves (lineBreak/hardBreak → \n, tab → \t) are now mapped to text so word separation survives in the rendered HTML; truly invisible nodes (bookmark boundaries, etc.) are still dropped. Added a unit test covering One<br/>Two<tab>Three → One\nTwo\tThree. (Note: before this PR, such comments crashed and were dropped entirely, so this is strictly an improvement either way.)
• Fake timers not restored (Qodo): added afterEach(() => vi.useRealTimers()) to comments-store.bookmark.test.js, matching the pattern in comments-store.test.js.

All 145 local tests pass (8 unit + 1 e2e + 136 existing).

Re the two red CI checks (collaboration (liveblocks) / CI Examples / validate): those fail at the "Create .env for cloud providers" step because VITE_LIVEBLOCKS_PUBLIC_KEY isn't available to fork PRs — unrelated to this change.

[qodo-code-review]

Code review by qodo was updated up to the latest commit 547d560

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 547d560065

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

[chatgpt-codex-connector]

Preserve no-break hyphens in comment text

When a DOCX comment contains <w:noBreakHyphen/>, the importer emits a noBreakHyphen leaf whose visible text is U+2011, but getRichTextExtensions() does not register NoBreakHyphenNode, so this new unsupported-leaf path returns null because the leaf is absent from this map. That makes sidebar text like co‑author render as coauthor rather than preserving the visible hyphen; include noBreakHyphen: '‑' before dropping unsupported leaves.

Useful? React with 👍 / 👎.