Removed need for exportDOM overrides in each node
#1531
Conversation
|
Caution Review failedThe pull request is closed. """ WalkthroughThis change refactors the node class definitions in the Possibly related PRs
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
packages/kg-default-nodes/lib/nodes/bookmark/BookmarkNode.jsOops! Something went wrong! :( ESLint: 8.57.1 Error: Failed to load parser '@babel/eslint-parser' declared in 'packages/kg-default-nodes/.eslintrc.js': Cannot find module '@babel/eslint-parser'
packages/kg-default-nodes/lib/nodes/codeblock/CodeBlockNode.jsOops! Something went wrong! :( ESLint: 8.57.1 Error: Failed to load parser '@babel/eslint-parser' declared in 'packages/kg-default-nodes/.eslintrc.js': Cannot find module '@babel/eslint-parser'
packages/kg-default-nodes/lib/nodes/audio/AudioNode.jsOops! Something went wrong! :( ESLint: 8.57.1 Error: Failed to load parser '@babel/eslint-parser' declared in 'packages/kg-default-nodes/.eslintrc.js': Cannot find module '@babel/eslint-parser'
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (23)
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
packages/kg-default-nodes/lib/nodes/collection/CollectionNode.js (1)
29-29: Remove the unnecessary blank line.This extra blank line doesn't add value to code readability.
const collection = this.__collection; const postCount = this.__postCount; - if (!options?.getCollectionPosts) {packages/kg-default-nodes/lib/generate-decorator-node.js (1)
40-40: Fix JSDoc typo - missing closing brace.The example in the JSDoc is incomplete.
- * @param {Function} defaultRenderFn - A function that returns a @tryghost/kg-lexical-html-renderer compatible object, e.g. {element: Div, type: 'inner} + * @param {Function} defaultRenderFn - A function that returns a @tryghost/kg-lexical-html-renderer compatible object, e.g. {element: 'div', type: 'inner'}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (23)
packages/kg-default-nodes/lib/generate-decorator-node.js(2 hunks)packages/kg-default-nodes/lib/nodes/audio/AudioNode.js(1 hunks)packages/kg-default-nodes/lib/nodes/bookmark/BookmarkNode.js(2 hunks)packages/kg-default-nodes/lib/nodes/button/ButtonNode.js(1 hunks)packages/kg-default-nodes/lib/nodes/call-to-action/CallToActionNode.js(1 hunks)packages/kg-default-nodes/lib/nodes/callout/CalloutNode.js(1 hunks)packages/kg-default-nodes/lib/nodes/codeblock/CodeBlockNode.js(1 hunks)packages/kg-default-nodes/lib/nodes/collection/CollectionNode.js(2 hunks)packages/kg-default-nodes/lib/nodes/email-cta/EmailCtaNode.js(2 hunks)packages/kg-default-nodes/lib/nodes/email/EmailNode.js(1 hunks)packages/kg-default-nodes/lib/nodes/embed/EmbedNode.js(1 hunks)packages/kg-default-nodes/lib/nodes/file/FileNode.js(1 hunks)packages/kg-default-nodes/lib/nodes/gallery/GalleryNode.js(1 hunks)packages/kg-default-nodes/lib/nodes/header/HeaderNode.js(2 hunks)packages/kg-default-nodes/lib/nodes/horizontalrule/HorizontalRuleNode.js(1 hunks)packages/kg-default-nodes/lib/nodes/html/HtmlNode.js(1 hunks)packages/kg-default-nodes/lib/nodes/image/ImageNode.js(2 hunks)packages/kg-default-nodes/lib/nodes/markdown/MarkdownNode.js(1 hunks)packages/kg-default-nodes/lib/nodes/paywall/PaywallNode.js(1 hunks)packages/kg-default-nodes/lib/nodes/product/ProductNode.js(2 hunks)packages/kg-default-nodes/lib/nodes/signup/SignupNode.js(2 hunks)packages/kg-default-nodes/lib/nodes/toggle/ToggleNode.js(1 hunks)packages/kg-default-nodes/lib/nodes/video/VideoNode.js(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Node 20.11.1
- GitHub Check: Node 22.13.1
🔇 Additional comments (22)
packages/kg-default-nodes/lib/nodes/collection/CollectionNode.js (1)
6-16: LGTM! Refactoring correctly implements the centralized rendering pattern.The changes properly consolidate the decorator configuration with
nodeTypeas the first property anddefaultRenderFnset to the imported render function, aligning with the PR objectives.packages/kg-default-nodes/lib/nodes/audio/AudioNode.js (1)
6-16: LGTM! Consistent implementation of the centralized rendering pattern.The refactoring correctly follows the established pattern with
nodeTypefirst anddefaultRenderFnproperly configured.packages/kg-default-nodes/lib/nodes/image/ImageNode.js (1)
5-18: LGTM! Clean implementation of the centralized rendering pattern.The refactoring maintains consistency with other nodes, properly configuring
nodeTypeanddefaultRenderFn.packages/kg-default-nodes/lib/nodes/file/FileNode.js (1)
7-17: LGTM! Refactoring correctly implemented.The changes properly implement the new pattern:
nodeType: 'file'is correctly placed as the first propertydefaultRenderFnis properly assigned to the importedrenderFileNode- The removal of
exportDOM()aligns with the PR's goal of centralizing this functionalitypackages/kg-default-nodes/lib/nodes/toggle/ToggleNode.js (1)
6-13: LGTM! Consistent with the refactoring pattern.The changes correctly follow the established pattern:
nodeType: 'toggle'as the first propertydefaultRenderFnproperly references the importedrenderToggleNode- Clean implementation without the
exportDOM()methodpackages/kg-default-nodes/lib/nodes/bookmark/BookmarkNode.js (1)
6-19: LGTM! Clean refactoring with commented code removed.The changes correctly implement the pattern:
nodeType: 'bookmark'properly positioned firstdefaultRenderFncorrectly assigned to the importedrenderBookmarkNode- Good cleanup by removing the previously commented-out
exportDOM()methodpackages/kg-default-nodes/lib/nodes/call-to-action/CallToActionNode.js (1)
26-27: Changes correctly implement the centralized rendering pattern.The addition of
defaultRenderFn: renderCallToActionNodeproperly integrates with the refactoredgenerateDecoratorNodefactory, successfully removing the need for individualexportDOMoverrides.packages/kg-default-nodes/lib/nodes/button/ButtonNode.js (1)
6-14: Refactoring successfully implements the centralized rendering approach.The changes properly:
- Place
nodeTypeon the first line of properties as per standardization goals- Add the required
defaultRenderFnproperty with the correct render function- Maintain clean multi-line formatting for better readability
packages/kg-default-nodes/lib/nodes/product/ProductNode.js (1)
6-21: Excellent implementation of the centralized rendering pattern.The refactoring successfully:
- Removes the explicit
exportDOMoverride as intended- Adds the
defaultRenderFnproperty pointing to the appropriate render function- Maintains consistency with the standardized
nodeTypeplacement- Preserves other class methods (
exportJSON,isEmpty) that are still neededpackages/kg-default-nodes/lib/nodes/codeblock/CodeBlockNode.js (1)
6-14: LGTM! Refactoring correctly implements the new pattern.The changes properly refactor the
CodeBlockNodeto use the newdefaultRenderFnpattern, removing the need for anexportDOMoverride. ThenodeTypeproperty is correctly positioned on the first line as specified in the PR objectives.packages/kg-default-nodes/lib/nodes/markdown/MarkdownNode.js (1)
5-11: LGTM! Clean refactoring to the new pattern.The
MarkdownNodehas been correctly refactored to usedefaultRenderFninstead of anexportDOMmethod, withnodeTypeproperly positioned on the first line.packages/kg-default-nodes/lib/nodes/header/HeaderNode.js (1)
9-41: Excellent implementation of versioned rendering pattern!The
HeaderNodecorrectly demonstrates the versioneddefaultRenderFnpattern using an object keyed by version numbers. This cleanly replaces the previousexportDOMmethod that contained version-specific logic, centralizing the rendering configuration as intended.packages/kg-default-nodes/lib/nodes/email/EmailNode.js (1)
5-11: Refactoring looks good!The changes correctly implement the centralized rendering pattern by:
- Moving to a multi-line format for better readability
- Placing
nodeTypeas the first property (standardization)- Using
defaultRenderFninstead of anexportDOMoverridepackages/kg-default-nodes/lib/nodes/signup/SignupNode.js (1)
6-26: Consistent refactoring implementation!The SignupNode correctly follows the new pattern with:
nodeTypeas the first property- Extensive properties array maintaining all defaults
defaultRenderFnproperly set torenderSignupCardToDOMThe custom constructor and label management methods remain properly intact.
packages/kg-default-nodes/lib/nodes/horizontalrule/HorizontalRuleNode.js (1)
6-9: Clean and minimal refactoring!The HorizontalRuleNode correctly implements the new pattern with the minimal required properties:
nodeTypeas the first propertydefaultRenderFnpointing to the appropriate rendererThe remaining methods (
importDOM,getTextContent,hasEditMode) are properly preserved.packages/kg-default-nodes/lib/nodes/callout/CalloutNode.js (1)
6-14: LGTM! Clean refactoring of the CalloutNode class.The changes correctly implement the new pattern:
nodeTypeis properly placed on the first linedefaultRenderFncorrectly references the importedrenderCalloutNode- The removal of the
exportDOM()method aligns with the PR's goal of centralizing rendering logicpackages/kg-default-nodes/lib/nodes/html/HtmlNode.js (1)
6-13: LGTM! Consistent refactoring of the HtmlNode class.The changes correctly follow the established pattern:
nodeTypeis on the first line as requiredhasVisibilityproperty is properly preserveddefaultRenderFncorrectly references the importedrenderHtmlNodepackages/kg-default-nodes/lib/nodes/email-cta/EmailCtaNode.js (1)
5-18: LGTM! EmailCtaNode successfully refactored.The changes are consistent with the refactoring pattern:
nodeTypeis properly placed on the first linedefaultRenderFncorrectly references the importedrenderEmailCtaNode- The empty class body is appropriate since no methods need to be overridden
packages/kg-default-nodes/lib/nodes/embed/EmbedNode.js (1)
6-16: LGTM! Clean refactoring to use centralized rendering logic.The changes correctly implement the new pattern:
nodeTypeis placed on the first line of properties for better searchabilitydefaultRenderFnproperly delegates rendering torenderEmbedNode- Removal of
exportDOMaligns with centralizing this logic ingenerateDecoratorNodepackages/kg-default-nodes/lib/nodes/gallery/GalleryNode.js (1)
5-12: LGTM! Consistent implementation of the centralized rendering pattern.The refactoring follows the same pattern as other nodes, properly delegating rendering responsibility to the factory function.
packages/kg-default-nodes/lib/nodes/paywall/PaywallNode.js (1)
6-9: LGTM! Refactoring aligns with PR objectives.The changes correctly implement the centralized rendering pattern by:
- Adding the required
defaultRenderFnparameter withrenderPaywallNode- Placing
nodeTypeon the first line for consistency- Removing the
exportDOMmethod (now handled by the factory)packages/kg-default-nodes/lib/nodes/video/VideoNode.js (1)
5-23: LGTM! Consistent implementation of the rendering refactor.The changes correctly implement the centralized rendering pattern:
defaultRenderFnis properly set torenderVideoNodenodeTypeplacement follows the new standard (first line)- All existing properties are preserved
exportDOMmethod removal aligns with the factory-based approach
kevinansfield
force-pushed
the
centralize-export-dom
branch
from
64b395f to
3e4ae59
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
packages/kg-default-nodes/lib/nodes/collection/CollectionNode.js (1)
29-29: Remove unnecessary blank line.const postCount = this.__postCount; -
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (23)
packages/kg-default-nodes/lib/generate-decorator-node.js(2 hunks)packages/kg-default-nodes/lib/nodes/audio/AudioNode.js(1 hunks)packages/kg-default-nodes/lib/nodes/bookmark/BookmarkNode.js(2 hunks)packages/kg-default-nodes/lib/nodes/button/ButtonNode.js(1 hunks)packages/kg-default-nodes/lib/nodes/call-to-action/CallToActionNode.js(1 hunks)packages/kg-default-nodes/lib/nodes/callout/CalloutNode.js(1 hunks)packages/kg-default-nodes/lib/nodes/codeblock/CodeBlockNode.js(1 hunks)packages/kg-default-nodes/lib/nodes/collection/CollectionNode.js(2 hunks)packages/kg-default-nodes/lib/nodes/email-cta/EmailCtaNode.js(2 hunks)packages/kg-default-nodes/lib/nodes/email/EmailNode.js(1 hunks)packages/kg-default-nodes/lib/nodes/embed/EmbedNode.js(1 hunks)packages/kg-default-nodes/lib/nodes/file/FileNode.js(1 hunks)packages/kg-default-nodes/lib/nodes/gallery/GalleryNode.js(1 hunks)packages/kg-default-nodes/lib/nodes/header/HeaderNode.js(2 hunks)packages/kg-default-nodes/lib/nodes/horizontalrule/HorizontalRuleNode.js(1 hunks)packages/kg-default-nodes/lib/nodes/html/HtmlNode.js(1 hunks)packages/kg-default-nodes/lib/nodes/image/ImageNode.js(2 hunks)packages/kg-default-nodes/lib/nodes/markdown/MarkdownNode.js(1 hunks)packages/kg-default-nodes/lib/nodes/paywall/PaywallNode.js(1 hunks)packages/kg-default-nodes/lib/nodes/product/ProductNode.js(2 hunks)packages/kg-default-nodes/lib/nodes/signup/SignupNode.js(2 hunks)packages/kg-default-nodes/lib/nodes/toggle/ToggleNode.js(1 hunks)packages/kg-default-nodes/lib/nodes/video/VideoNode.js(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/kg-default-nodes/lib/nodes/markdown/MarkdownNode.js
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Node 20.11.1
- GitHub Check: Node 22.13.1
🔇 Additional comments (27)
packages/kg-default-nodes/lib/generate-decorator-node.js (2)
40-40: LGTM: Clear documentation for the new parameter.The JSDoc documentation clearly explains the
defaultRenderFnparameter and its expected return type.
43-43: LGTM: Function signature correctly updated.The function signature properly includes the new
defaultRenderFnparameter as required by the refactor.packages/kg-default-nodes/lib/nodes/call-to-action/CallToActionNode.js (1)
26-27: LGTM: Correctly implements the centralized rendering pattern.The addition of
defaultRenderFn: renderCallToActionNodeproperly centralizes the rendering logic as intended by the refactor. The nodeType is correctly positioned on the first line as per the standardization requirements.packages/kg-default-nodes/lib/nodes/toggle/ToggleNode.js (1)
6-13: LGTM: Clean implementation of the centralized rendering pattern.The refactor correctly:
- Positions
nodeTypeon the first line as per standardization requirements- Adds
defaultRenderFn: renderToggleNodeto centralize rendering logic- Uses multiline formatting for improved readability
This aligns perfectly with the PR objectives.
packages/kg-default-nodes/lib/nodes/image/ImageNode.js (1)
5-18: LGTM: Consistent implementation of the rendering refactor.The changes properly implement the centralized rendering pattern:
nodeTypeis correctly positioned on the first linedefaultRenderFn: renderImageNodecentralizes the rendering logic- Multiline formatting enhances readability
The refactor maintains all existing functionality while following the established pattern.
packages/kg-default-nodes/lib/nodes/email-cta/EmailCtaNode.js (1)
5-17: LGTM! Refactor correctly implemented.The refactor successfully centralizes the rendering logic by:
- Moving
nodeTypeto the first line for consistency- Adding
defaultRenderFnconfiguration pointing to the imported renderer- Maintaining clean, declarative node configuration
This follows the intended pattern described in the PR objectives.
packages/kg-default-nodes/lib/nodes/paywall/PaywallNode.js (1)
6-9: LGTM! Consistent refactor implementation.The configuration correctly:
- Places
nodeTypeon the first line for standardization- Assigns the imported
renderPaywallNodetodefaultRenderFn- Maintains the same clean pattern across node types
The preservation of the static
importDOM()method is appropriate since this refactor only targetsexportDOMcentralization.packages/kg-default-nodes/lib/nodes/audio/AudioNode.js (1)
6-16: LGTM! Refactor pattern consistently applied.The AudioNode configuration correctly implements the centralized rendering approach:
nodeTypestandardized on first line- Audio-specific properties maintained in the properties array
defaultRenderFnproperly assigned to the imported renderer functionThe implementation maintains consistency with the other refactored nodes.
packages/kg-default-nodes/lib/nodes/embed/EmbedNode.js (1)
6-16: LGTM! Refactor completed successfully.The EmbedNode configuration correctly implements the centralized rendering pattern:
nodeTypeplaced on first line for consistency- Embed-specific properties preserved in the configuration
defaultRenderFnproperly assigned to imported renderer functionThis completes the consistent application of the refactor pattern across all reviewed node types.
packages/kg-default-nodes/lib/nodes/header/HeaderNode.js (2)
9-11: LGTM: Clean refactor implementation.The addition of
nodeTypeparameter togenerateDecoratorNode()follows the standardization objective and maintains consistency across the codebase.
37-40:✅ Verification successful
LGTM: Elegant version-based rendering solution.
The version mapping approach for
defaultRenderFnis well-designed, allowing different render functions for v1 and v2 headers. This centralizes the version logic that was previously handled in theexportDOMmethod.Please verify that the
generateDecoratorNodefunction has been updated to support the newdefaultRenderFnparameter:
🏁 Script executed:
#!/bin/bash # Description: Verify generateDecoratorNode supports defaultRenderFn parameter # Expected: Function should handle defaultRenderFn and create appropriate exportDOM method # Search for generateDecoratorNode implementation ast-grep --pattern 'function generateDecoratorNode($PARAMS) { $$$ }' # Also search for the function as arrow function or export rg -A 20 "generateDecoratorNode.*="Length of output: 26970
Confirmed:
generateDecoratorNodesupports thedefaultRenderFnparameterVerified that in
packages/kg-default-nodes/lib/generate-decorator-node.jsthe function signature includesdefaultRenderFn, and itsexportDOMmethod correctly handles both object‐based (versioned) and single‐function renderers:
- Signature at line 43:
export function generateDecoratorNode({nodeType, properties = [], defaultRenderFn, version = 1, hasVisibility = false}) {exportDOMimplementation at lines 178–184 and 187–193 usesdefaultRenderFn[this.__version]when it’s an object and falls back to a direct function call otherwise.No further changes required.
packages/kg-default-nodes/lib/nodes/product/ProductNode.js (2)
6-7: LGTM: Consistent with refactor pattern.The
nodeTypestandardization and positioning aligns with the PR objectives for easier regex operations and AI agent compatibility.
19-21: LGTM: Proper rendering function assignment.The
defaultRenderFnimplementation correctly replaces the previousexportDOMmethod with centralized rendering logic.packages/kg-default-nodes/lib/nodes/signup/SignupNode.js (2)
6-7: LGTM: Standardized node type declaration.The positioning of
nodeTypeas the first property in thegenerateDecoratorNodecall follows the standardization pattern across all nodes.
24-26: LGTM: Centralized rendering approach.The
defaultRenderFnassignment properly replaces the previous rendering logic, maintaining functionality while simplifying the node definition.packages/kg-default-nodes/lib/nodes/file/FileNode.js (2)
7-8: LGTM: Consistent standardization.The
nodeTypeplacement follows the established pattern, contributing to the overall consistency of the refactor.
15-17: LGTM: Successful rendering centralization.The
defaultRenderFnimplementation completes the refactor pattern, removing the need for individualexportDOMmethod overrides while maintaining rendering functionality.packages/kg-default-nodes/lib/nodes/button/ButtonNode.js (1)
6-14: LGTM! The refactoring correctly implements the centralized rendering pattern.The changes properly:
- Place
nodeTypeas the first property for consistency- Add
defaultRenderFn: renderButtonNodeto centralize the rendering logic- Maintain clean multi-line formatting for better readability
packages/kg-default-nodes/lib/nodes/callout/CalloutNode.js (1)
6-14: LGTM! The CalloutNode refactoring follows the new pattern correctly.The implementation properly:
- Standardizes
nodeTypeplacement as the first property- Adds
defaultRenderFn: renderCalloutNodefor centralized rendering- Maintains consistency with the custom constructor's default values
packages/kg-default-nodes/lib/nodes/bookmark/BookmarkNode.js (1)
6-19: LGTM! The BookmarkNode refactoring successfully implements the centralized rendering pattern.The changes correctly:
- Place
nodeTypeas the first property following the standardization- Add
defaultRenderFn: renderBookmarkNodeto delegate rendering to the centralized handler- Preserve the complex metadata structure with
urlPathpropertiespackages/kg-default-nodes/lib/nodes/codeblock/CodeBlockNode.js (1)
6-14: LGTM! Clean refactoring that aligns with PR objectives.The refactoring correctly moves the rendering logic to
defaultRenderFnand placesnodeTypeon the first line for better consistency and searchability.packages/kg-default-nodes/lib/nodes/email/EmailNode.js (1)
5-11: LGTM! Consistent implementation of the refactoring pattern.The changes correctly implement the centralized rendering approach using
defaultRenderFn.packages/kg-default-nodes/lib/nodes/horizontalrule/HorizontalRuleNode.js (1)
6-9: LGTM! Correct implementation for a node without properties.The refactoring properly handles the case where a node doesn't require properties, while still implementing the
defaultRenderFnpattern.packages/kg-default-nodes/lib/nodes/collection/CollectionNode.js (1)
6-16: LGTM! Refactoring correctly implemented.The changes properly implement the centralized rendering pattern with
defaultRenderFn.packages/kg-default-nodes/lib/nodes/html/HtmlNode.js (1)
6-13: Refactoring looks good!The changes correctly implement the centralized rendering pattern by:
- Moving
nodeTypeto the first property position for consistency- Adding
defaultRenderFn: renderHtmlNodeto replace the removedexportDOMmethod- Maintaining all existing functionality
packages/kg-default-nodes/lib/nodes/gallery/GalleryNode.js (1)
5-12: Consistent implementation of the refactoring pattern.The changes follow the same pattern as other nodes:
nodeTypeproperty standardized to first positiondefaultRenderFn: renderGalleryNodereplaces the removedexportDOMmethod- All functionality preserved
packages/kg-default-nodes/lib/nodes/video/VideoNode.js (1)
5-23: Well-executed refactoring with proper handling of complex properties.The implementation correctly applies the refactoring pattern while maintaining the node's complexity:
nodeTypeproperly positioned firstdefaultRenderFn: renderVideoNodecorrectly replaces the removedexportDOMmethod- All properties and custom methods (like
exportJSON) remain intact
| export class EmbedNode extends generateDecoratorNode({ | ||
| nodeType: 'embed', | ||
| properties: [ | ||
| {name: 'url', default: '', urlType: 'url'}, | ||
| {name: 'embedType', default: ''}, | ||
| {name: 'html', default: ''}, | ||
| {name: 'metadata', default: {}}, | ||
| {name: 'caption', default: '', wordCount: true} | ||
| ]} | ||
| ) { | ||
| ], | ||
| defaultRenderFn: renderEmbedNode | ||
| }) { |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify refactor completion across the entire codebase.
Since this is a systematic refactor affecting multiple node types, let's ensure all nodes have been updated consistently and that no exportDOM methods remain.
🏁 Script executed:
#!/bin/bash
# Description: Verify that the refactor has been applied consistently across all node files
# Expected: All node files should have defaultRenderFn and no exportDOM methods
echo "=== Checking for remaining exportDOM methods ==="
rg -n "exportDOM" packages/kg-default-nodes/lib/nodes/ || echo "✓ No exportDOM methods found"
echo -e "\n=== Checking for defaultRenderFn usage ==="
rg -n "defaultRenderFn:" packages/kg-default-nodes/lib/nodes/ | head -10
echo -e "\n=== Checking generateDecoratorNode usage pattern ==="
rg -A 5 "generateDecoratorNode\(\{" packages/kg-default-nodes/lib/nodes/ | grep -E "(nodeType|defaultRenderFn)" | head -20
echo -e "\n=== Verifying that generateDecoratorNode supports defaultRenderFn ==="
rg -A 10 -B 5 "defaultRenderFn" packages/kg-default-nodes/lib/generate-decorator-node.jsLength of output: 5820
Remove remaining exportDOM overrides from AtLink nodes
We’ve confirmed that two node files still define custom exportDOM methods instead of using defaultRenderFn. Please update them to pass a render function into generateDecoratorNode and remove the overrides:
• packages/kg-default-nodes/lib/nodes/at-link/AtLinkNode.js (line 60)
• packages/kg-default-nodes/lib/nodes/at-link/AtLinkSearchNode.js (line 67)
Suggested steps:
- Remove the
exportDOMmethod in each class. - Add the appropriate
defaultRenderFn(e.g.renderAtLinkNode/renderAtLinkSearchNode) to the decorator config. - Migrate any custom export logic into those render functions so the built-in
exportDOMimplementation can be used.
Once done, confirm no other exportDOM overrides remain across packages/kg-default-nodes/lib/nodes.
🤖 Prompt for AI Agents
In packages/kg-default-nodes/lib/nodes/embed/EmbedNode.js lines 6 to 16, the
class EmbedNode uses generateDecoratorNode with a defaultRenderFn but does not
include any exportDOM method override. The review comment suggests removing
custom exportDOM methods from other nodes and ensuring they use
generateDecoratorNode with the appropriate render function. To fix this, verify
that EmbedNode correctly passes the renderEmbedNode function as defaultRenderFn,
remove any existing exportDOM override, and ensure the class relies solely on
generateDecoratorNode for rendering and exporting. Confirm that no exportDOM
methods remain in this file after the update.
There was a problem hiding this comment.
Neither of those nodes are decorator nodes and so don't use generateDecoratorNode()
There was a problem hiding this comment.
🧩 Analysis chain
@kevinansfield Thank you for the clarification! Let me verify the structure of those nodes to better understand the distinction between decorator nodes and other node types.
🏁 Script executed:
#!/bin/bash
# Check the structure of AtLinkNode and AtLinkSearchNode to confirm they don't use generateDecoratorNode
echo "=== AtLinkNode.js structure ==="
head -20 packages/kg-default-nodes/lib/nodes/at-link/AtLinkNode.js
echo -e "\n=== AtLinkSearchNode.js structure ==="
head -20 packages/kg-default-nodes/lib/nodes/at-link/AtLinkSearchNode.js
echo -e "\n=== Checking what these nodes extend instead ==="
rg -A 3 "class.*Node extends" packages/kg-default-nodes/lib/nodes/at-link/
echo -e "\n=== Double-checking no generateDecoratorNode usage in at-link nodes ==="
rg "generateDecoratorNode" packages/kg-default-nodes/lib/nodes/at-link/ || echo "✓ No generateDecoratorNode usage found in at-link nodes"
Length of output: 3016
@kevinansfield You're absolutely right! I can see from the code structure that:
- AtLinkNode extends
ElementNode(from Lexical) - AtLinkSearchNode extends
TextNode(from Lexical)
These are standard Lexical nodes, not decorator nodes created by generateDecoratorNode(). The refactor is specifically for decorator nodes that extend generateDecoratorNode() (which creates classes extending KoenigDecoratorNode).
The AtLink nodes legitimately need their own exportDOM methods and should not be part of this refactor. Thank you for the clarification!
🧬 Code Graph Analysis Results
Relevant snippet from packages/kg-default-nodes/lib/generate-decorator-node.js (lines 43-304)
export function generateDecoratorNode({nodeType, properties = [], defaultRenderFn, version = 1, hasVisibility = false}) {
validateArguments(nodeType, properties);
// Adds a `privateName` field to the properties for convenience (e.g. `__name`):
// properties: [{name: 'name', privateName: '__name', type: 'string', default: 'hello'}, {...}]
properties = properties.map((prop) => {
return {...prop, privateName: `__${prop.name}`};
});
// Adds `visibility` property to the properties array if `hasVisibility` is true
// uses a getter for `default` to avoid problems with mutation of nested objects
if (hasVisibility) {
properties.push({
name: 'visibility',
get default() {
return buildDefaultVisibility();
},
privateName: '__visibility'
});
}
class GeneratedDecoratorNode extends KoenigDecoratorNode {
constructor(data = {}, key) {
super(key);
properties.forEach((prop) => {
if (typeof prop.default === 'boolean') {
this[prop.privateName] = data[prop.name] ?? prop.default;
} else {
this[prop.privateName] = data[prop.name] || prop.default;
}
});
}
static getType() {
return nodeType;
}
static clone(node) {
return new this(node.getDataset(), node.__key);
}
static getPropertyDefaults() {
return properties.reduce((obj, prop) => {
obj[prop.name] = prop.default;
return obj;
}, {});
}
static get urlTransformMap() {
let map = {};
properties.forEach((prop) => {
if (prop.urlType) {
if (prop.urlPath) {
map[prop.urlPath] = prop.urlType;
} else {
map[prop.name] = prop.urlType;
}
}
});
return map;
}
getDataset() {
const self = this.getLatest();
let dataset = {};
properties.forEach((prop) => {
dataset[prop.name] = self[prop.privateName];
});
return dataset;
}
static importJSON(serializedNode) {
const data = {};
// migrate older nodes that were saved with an earlier version of the visibility format
serializedNode.visibility = migrateOldVisibilityFormat(serializedNode.visibility);
properties.forEach((prop) => {
data[prop.name] = serializedNode[prop.name];
});
return new this(data);
}
exportJSON() {
const dataset = {
type: nodeType,
version: version,
...properties.reduce((obj, prop) => {
obj[prop.name] = this[prop.name];
return obj;
}, {})
};
return dataset;
}
exportDOM(options = {}) {
if (typeof defaultRenderFn === 'object') {
const render = defaultRenderFn[this.__version];
if (!render) {
throw new Error(`[generateDecoratorNode] ${nodeType}: "defaultRenderFn" for version ${this.__version} is required`);
}
return render(this, options);
}
if (!defaultRenderFn) {
throw new Error(`[generateDecoratorNode] ${nodeType}: "defaultRenderFn" is required`);
}
const render = defaultRenderFn;
return render(this, options);
}
createDOM() {
return document.createElement('div');
}
updateDOM() {
return false;
}
isInline() {
// All our cards are top-level blocks. Override if needed.
return false;
}
hasDynamicData() {
return false;
}
hasEditMode() {
// Most of our cards have an edit mode. Override if needed.
return true;
}
getTextContent() {
const self = this.getLatest();
const propertiesWithText = properties.filter(prop => !!prop.wordCount);
const text = propertiesWithText.map(
prop => readTextContent(self, prop.name)
).filter(Boolean).join('\n');
return text ? `${text}\n\n` : '';
}
getIsVisibilityActive() {
if (!properties.some(prop => prop.name === 'visibility')) {
return false;
}
const self = this.getLatest();
const visibility = self.__visibility;
return isVisibilityRestricted(visibility);
}
}
properties.forEach((prop) => {
Object.defineProperty(GeneratedDecoratorNode.prototype, prop.name, {
get: function () {
const self = this.getLatest();
return self[prop.privateName];
},
set: function (newVal) {
const writable = this.getWritable();
writable[prop.privateName] = newVal;
}
});
});
return GeneratedDecoratorNode;
}Relevant snippet from packages/kg-default-nodes/lib/nodes/embed/EmbedNode.js (full file)
/* eslint-disable ghost/filenames/match-exported-class */
import {generateDecoratorNode} from '../../generate-decorator-node';
import {parseEmbedNode} from './embed-parser';
import {renderEmbedNode} from './embed-renderer';
export class EmbedNode extends generateDecoratorNode({
nodeType: 'embed',
properties: [
{name: 'url', default: '', urlType: 'url'},
{name: 'embedType', default: ''},
{name: 'html', default: ''},
{name: 'metadata', default: {}},
{name: 'caption', default: '', wordCount: true}
],
defaultRenderFn: renderEmbedNode
}) {
static importDOM() {
return parseEmbedNode(this);
}
isEmpty() {
return !this.__url && !this.__html;
}
}
export const $createEmbedNode = (dataset) => {
return new EmbedNode(dataset);
};
export function $isEmbedNode(node) {
return node instanceof EmbedNode;
}Relevant snippet from packages/kg-default-nodes/lib/nodes/embed/embed-parser.js (lines 4-79)
export function parseEmbedNode(EmbedNode) {
return {
figure: (nodeElem) => {
if (nodeElem.nodeType === 1 && nodeElem.tagName === 'FIGURE') {
const iframe = nodeElem.querySelector('iframe');
if (iframe) {
return {
conversion(domNode) {
const payload = _createPayloadForIframe(iframe);
if (!payload) {
return null;
}
payload.caption = readCaptionFromElement(domNode);
const node = new EmbedNode(payload);
return {node};
},
priority: 1
};
}
const blockquote = nodeElem.querySelector('blockquote');
if (blockquote) {
return {
conversion(domNode) {
const link = domNode.querySelector('a');
if (!link) {
return null;
}
let url = link.getAttribute('href');
// If we don't have a url, or it's not an absolute URL, we can't handle this
if (!url || !url.match(/^https?:\/\//i)) {
return null;
}
let payload = {url: url};
// append caption, remove element from blockquote
payload.caption = readCaptionFromElement(domNode);
let figcaption = domNode.querySelector('figcaption');
figcaption?.remove();
payload.html = domNode.innerHTML;
const node = new EmbedNode(payload);
return {node};
},
priority: 1
};
}
}
return null;
},
iframe: (nodeElem) => {
if (nodeElem.nodeType === 1 && nodeElem.tagName === 'IFRAME') {
return {
conversion(domNode) {
const payload = _createPayloadForIframe(domNode);
if (!payload) {
return null;
}
const node = new EmbedNode(payload);
return {node};
},
priority: 1
};
}
return null;
}
};
}Relevant snippet from packages/kg-default-nodes/lib/nodes/embed/embed-renderer.js (lines 5-16)
export function renderEmbedNode(node, options = {}) {
addCreateDocumentOption(options);
const document = options.createDocument();
const embedType = node.embedType;
if (embedType === 'twitter') {
return twitterRenderer(node, document, options);
}
return renderTemplate(node, document, options);
}These snippets provide context on:
- The
EmbedNodeclass extending a node generated bygenerateDecoratorNode(). - The implementation and behavior of
generateDecoratorNode(), which creates a subclass ofKoenigDecoratorNode. - The parsing and rendering logic for embed nodes.
- The user comment about nodes not being decorator nodes and not using
generateDecoratorNode()relates directly to the fact thatEmbedNodeis created bygenerateDecoratorNode().
✏️ Learnings added
Learnt from: kevinansfield
PR: TryGhost/Koenig#1531
File: packages/kg-default-nodes/lib/nodes/embed/EmbedNode.js:6-16
Timestamp: 2025-05-26T14:57:48.106Z
Learning: AtLinkNode and AtLinkSearchNode in packages/kg-default-nodes/lib/nodes/at-link/ are not decorator nodes and don't use generateDecoratorNode(). They are a different type of node that legitimately maintains their own exportDOM methods and should not be part of the decorator node refactor.
Learnt from: kevinansfield
PR: TryGhost/Koenig#1531
File: packages/kg-default-nodes/lib/nodes/embed/EmbedNode.js:6-16
Timestamp: 2025-05-26T14:57:48.106Z
Learning: The decorator node refactor in packages/kg-default-nodes only applies to nodes that extend generateDecoratorNode() (which creates KoenigDecoratorNode subclasses). Nodes like AtLinkNode and AtLinkSearchNode that extend standard Lexical nodes (ElementNode, TextNode) are not decorator nodes and legitimately maintain their own exportDOM methods outside of this refactor scope.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
no issue First part of a refactor to allow node renderers to be provided by the consumer. This change centralizes our `exportDOM()` definition in our `generateDecoratorNode()` factory which means if needed we can select a different renderer function in one place rather than needing to spread knowledge across every node. - added a required `defaultRenderFn` option to `generateDecoratorNode()` - takes a function or an object, the object version should be keyed by version numbers with the appropriate render function as the value - adds default `exportDOM()` method that calls `defaultRenderFn` or the appropriate versioned render function - removed `exportDOM()` overrides from all of our decorator nodes - standardized on `nodeType:` property being defined on the first line of the properties list in each node which helps with regex search & replace and AI agents
kevinansfield
force-pushed
the
centralize-export-dom
branch
from
3e4ae59 to
6991c15
Compare
1 participant
no issue
First part of a refactor to allow node renderers to be provided by the consumer. This change centralizes our
exportDOM()definition in ourgenerateDecoratorNode()factory which means if needed we can select a different renderer function in one place rather than needing to spread knowledge across every node.defaultRenderFnoption togenerateDecoratorNode()exportDOM()method that callsdefaultRenderFnor the appropriate versioned render functionexportDOM()overrides from all of our decorator nodesnodeType:property being defined on the first line of the properties list in each node which helps with regex search & replace and AI agents