Extracted HTML card visibility rendering to utility function #1433
Conversation
|
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/html/html-renderer.jsOops! Something went wrong! :( ESLint: 8.57.0 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/utils/visibility.jsOops! Something went wrong! :( ESLint: 8.57.0 Error: Failed to load parser '@babel/eslint-parser' declared in 'packages/kg-default-nodes/.eslintrc.js': Cannot find module '@babel/eslint-parser'
WalkthroughThis pull request refactors visibility rendering in HTML node processing. The Changes
Sequence Diagram(s)sequenceDiagram
participant H as renderHtmlNode
participant V as renderWithVisibility
participant E as EmailVisibilityHelper
participant W as WebVisibilityHelper
H->>V: Call renderWithVisibility(renderOutput, node.visibility, options)
alt Visibility condition active
V->>E: Process email-specific visibility
V->>W: Process web-specific visibility
else No visibility condition
V-->>H: Return original renderOutput
end
H-->>Caller: Return final HTML element
Possibly related PRs
Suggested reviewers
Poem
Tip 🌐 Web search-backed reviews and chat
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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 (
|
ref https://linear.app/ghost/issue/PLG-332 - extracted visibility rendering logic and adapted to be more generalised for use in other cards
kevinansfield
force-pushed
the
extract-visibility-rendering
branch
from
7232267 to
8cbc28f
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
packages/kg-default-nodes/lib/utils/visibility.js (1)
40-77: Promote fallback handling for unknown rendering targets.The function currently handles
'email'and defaults to'web'for any other value without explicit checks or error messages. Consider either throwing an error for unknown targets or defining a documented fallback to prevent unintentional behavior if targets like'pdf'are used.packages/kg-default-nodes/lib/nodes/html/html-renderer.js (1)
25-25: Keep inline comments consistent.The inline comment clarifies intent. For consistency, consider adding a short explanation whenever returning an object with
{element, type}for newly introduced blocks.packages/kg-default-nodes/test/utils/visibility.test.js (1)
62-200: Impressive coverage for various visibility scenarios.These tests comprehensively cover old vs. new formats, multiple render types, and membership segments. One extension could be testing unexpected targets to ensure graceful handling or errors, mirroring any fallback logic in
renderWithVisibility.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/kg-default-nodes/lib/nodes/html/html-renderer.js(2 hunks)packages/kg-default-nodes/lib/utils/visibility.js(2 hunks)packages/kg-default-nodes/test/utils/visibility.test.js(2 hunks)
🔇 Additional comments (4)
packages/kg-default-nodes/lib/utils/visibility.js (3)
81-92: Validate the DOM element before extracting content.If
elementis unexpectedly null/undefined, this block will throw an error. Consider adding a small guard to ensureelementis valid, or at least document that a valid DOM element is required.
94-101: Watch for potential HTML injection incontainer.innerHTML.When assigning
contenttoinnerHTML, it's possible that untrusted markup could be injected. If the input is user-generated, confirm that sanitization is performed upstream to avoid XSS risks.
102-108: Consider sanitizing or validating wrapped comments.Wrapping
contentinside HTML comments is effective for gating, but ifcontentis user-supplied, ensure you’ve properly handled or sanitized any malicious scripts injected into the comment block.packages/kg-default-nodes/lib/nodes/html/html-renderer.js (1)
3-3: Delegate to newrenderWithVisibilitylogic looks good.Replacing the inline visibility handling with
renderWithVisibilitycentralizes the gating logic and simplifies maintainability. This is a solid refactor.Also applies to: 20-22
2 participants
ref https://linear.app/ghost/issue/PLG-332