Enhance serialization with optional chaining and checks#2880
Conversation
Added optional chaining to safely check for class presence and attribute setting. Improved handling of nesting level attribute assignment.
|
@Bredo is attempting to deploy a commit to the TypeCell Team on Vercel. A member of the Team first needs to authorize it. |
📝 WalkthroughWalkthroughThis change adds defensive guards to the ChangesDefensive DOM Guards in Serialization
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~5 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/core/src/api/exporters/html/util/serializeBlocksExternalHTML.ts (1)
246-256: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winGuard only covers the last mutation of
firstChild; earlier lines still crash on non-elements.The new check at Line 251 protects only the
data-nesting-levelassignment, but lines 247 and 250 access/mutate the sameret.dom.firstChildwithout any guard:
- Line 247 calls
.setAttributeunconditionally in the loop.- Line 250 passes
firstChildtoaddAttributesAndRemoveClasses, which doesArray.from(element.classList)with no optional chaining — this throws iffirstChildlacksclassList(e.g. a Text node).Since
ret.dompassing theclassList.contains("bn-block-content")check (Line 231) says nothing about what typeret.dom.firstChildis, the same "not an HTMLElement" crash this PR is meant to prevent remains reachable a few lines earlier in the same branch.🛡️ Proposed fix: guard all firstChild mutations consistently
+ const firstChild = ret.dom.firstChild as HTMLElement | null; for (const attr of blockContentDataAttributes) { - (ret.dom.firstChild! as HTMLElement).setAttribute(attr.name, attr.value); + firstChild?.setAttribute?.(attr.name, attr.value); } - addAttributesAndRemoveClasses(ret.dom.firstChild! as HTMLElement); - if (nestingLevel > 0 && typeof (ret.dom.firstChild as HTMLElement)?.setAttribute === 'function') { - (ret.dom.firstChild! as HTMLElement).setAttribute( + if (firstChild?.classList) { + addAttributesAndRemoveClasses(firstChild); + } + if (nestingLevel > 0 && typeof firstChild?.setAttribute === "function") { + firstChild.setAttribute( "data-nesting-level", nestingLevel.toString(), ); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/core/src/api/exporters/html/util/serializeBlocksExternalHTML.ts` around lines 246 - 256, The firstChild safety check in serializeBlocksExternalHTML only protects the final data-nesting-level mutation, but the earlier uses of ret.dom.firstChild still assume an HTMLElement. Update the branch in serializeBlocksExternalHTML to consistently guard or narrow firstChild before any mutation, including the blockContentDataAttributes loop and the call to addAttributesAndRemoveClasses, so those paths only run when firstChild is an element. Use the existing ret.dom.firstChild references and addAttributesAndRemoveClasses helper as the key locations to adjust.
🧹 Nitpick comments (1)
packages/core/src/api/exporters/html/util/serializeBlocksExternalHTML.ts (1)
230-266: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winNo test coverage added for the new non-HTMLElement guard paths.
This defensive fix guards against
classList/setAttributebeing absent (e.g.DocumentFragment/Textnodes), but no test exercises a block whoserender/toExternalHTMLreturns such a node. Given this is fixing a real crash scenario, a regression test would help confirm the fix and prevent future regressions.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/core/src/api/exporters/html/util/serializeBlocksExternalHTML.ts` around lines 230 - 266, Add regression coverage for the new non-HTMLElement guard paths in serializeBlocksExternalHTML. Create a test that exercises a block whose render/toExternalHTML returns a DocumentFragment or Text node so the classList/setAttribute checks are skipped safely. Verify the export no longer crashes and still appends the node correctly, using the serializeBlocksExternalHTML flow and the block-content handling branch as the target location.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@packages/core/src/api/exporters/html/util/serializeBlocksExternalHTML.ts`:
- Around line 246-256: The firstChild safety check in
serializeBlocksExternalHTML only protects the final data-nesting-level mutation,
but the earlier uses of ret.dom.firstChild still assume an HTMLElement. Update
the branch in serializeBlocksExternalHTML to consistently guard or narrow
firstChild before any mutation, including the blockContentDataAttributes loop
and the call to addAttributesAndRemoveClasses, so those paths only run when
firstChild is an element. Use the existing ret.dom.firstChild references and
addAttributesAndRemoveClasses helper as the key locations to adjust.
---
Nitpick comments:
In `@packages/core/src/api/exporters/html/util/serializeBlocksExternalHTML.ts`:
- Around line 230-266: Add regression coverage for the new non-HTMLElement guard
paths in serializeBlocksExternalHTML. Create a test that exercises a block whose
render/toExternalHTML returns a DocumentFragment or Text node so the
classList/setAttribute checks are skipped safely. Verify the export no longer
crashes and still appends the node correctly, using the
serializeBlocksExternalHTML flow and the block-content handling branch as the
target location.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 86855951-5657-4e2e-9b9f-873052eb0e67
📒 Files selected for processing (1)
packages/core/src/api/exporters/html/util/serializeBlocksExternalHTML.ts
1 participant
Summary
Added optional chaining to safely check for class presence and attribute setting. Improved handling of nesting level attribute assignment.
Rationale
When serializing blocks to external HTML (via
blocksToHTMLLossy), the serializer evaluates(ret.dom as HTMLElement).classList.contains("bn-block-content"). However, certain blocktoExternalHTMLor render implementations might return node types that aren't strictly HTMLElements (such as DocumentFragment or Text nodes). These nodes do not have a.classListproperty, which causes the serializer to crash withTypeError: Cannot read properties of undefined (reading 'contains').I ran in to this specifically using @twentyhq/twenty with numbered lists
<ol>Additionally, if these node types are evaluated in the else branch and nestingLevel > 0, the exporter tries to call
.setAttribute("data-nesting-level", ...)on them, causing a secondary crash (TypeError: ...setAttribute is not a function).This PR guards both of these DOM operations to ensure the HTML exporter fails gracefully and doesn't crash when encountering non-element DOM nodes.
Changes
.classListexists before calling.contains("bn-block-content").data-nesting-levelassignments by checking if the.setAttributemethod exists viatypeof === 'function'before invoking it.Impact
This PR increases the stability of the HTML exporters. It prevents hard crashes during
blocksToHTMLLossyandblocksToFullHTMLwhen the editor encounters complex fragment or text structures. Existing functionalities will remain completely unaffected as this merely guards operations against undefined errors.Testing
Manual testing was performed by executing
blocksToHTMLLossyon a document containing block structures that resolve to nodes lacking.classListproperties. Previously, this would crash the frontend application; with this fix, the serialization completes successfully.Checklist
Summary by CodeRabbit