Refactored CTA card button rendering #1517
Conversation
|
""" WalkthroughThe Possibly related PRs
Suggested reviewers
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/call-to-action/calltoaction-renderer.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 (2)
🚧 Files skipped from review as they are similar to previous changes (1)
🧰 Additional context used🧬 Code Graph Analysis (1)packages/kg-default-nodes/test/nodes/call-to-action.test.js (1)
⏰ Context from checks skipped due to timeout of 90000ms (2)
🔇 Additional comments (1)
✨ 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: 2
🧹 Nitpick comments (2)
packages/kg-default-nodes/lib/nodes/call-to-action/calltoaction-renderer.js (2)
112-121: Redundant style/class duplication on both<td>and inner<a>The
kg-style-accentclass and inlinebuttonStyleare applied to both the<td>and its descendant<a>.
Apart from inflating the markup size, some email clients will only honour the first occurrence, leading to inconsistent colours.A leaner variant keeps the styling on the
<a>alone:- <td class="${dataset.buttonColor === 'accent' ? 'kg-style-accent' : ''}" style="${buttonStyle}"> - <a href="${dataset.buttonUrl}" - class="${dataset.buttonColor === 'accent' ? 'kg-style-accent' : ''}" - style="${buttonStyle}"> + <td> + <a href="${dataset.buttonUrl}" + class="${dataset.buttonColor === 'accent' ? 'kg-style-accent' : ''}" + style="${buttonStyle}">
96-99: Hard-codedalttext reduces accessibilityThe
altattribute is always"CTA Image". Screen-reader users receive no meaningful description, and repeated identical alt text is announced for every CTA.Consider accepting
dataset.imageAlt(falling back to an empty string if not supplied) and apply it consistently:- ${wrapWithLink(dataset, `<img src="${dataset.imageUrl}" alt="CTA Image" …>`)} + const alt = (dataset.imageAlt ?? '').replace(/"/g, '"'); + ${wrapWithLink(dataset, `<img src="${dataset.imageUrl}" alt="${alt}" …>`)}Also applies to: 140-152, 253-265
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/kg-default-nodes/lib/nodes/call-to-action/calltoaction-renderer.js(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Node 20.11.1
- GitHub Check: Node 22.13.1
| if (options.feature?.emailCustomizationAlpha) { | ||
| const renderContent = () => { | ||
| if (dataset.layout === 'minimal') { | ||
| return ` | ||
| <tr> | ||
| <td class="kg-cta-content"> | ||
| <table border="0" cellpadding="0" cellspacing="0" width="100%" class="kg-cta-content-wrapper"> | ||
| <tr> | ||
| ${dataset.imageUrl ? ` | ||
| <td class="kg-cta-image-container" width="64"> | ||
| ${wrapWithLink(dataset, `<img src="${dataset.imageUrl}" alt="CTA Image" class="kg-cta-image" width="64" height="64">`)} | ||
| </td> | ||
| ` : ''} | ||
| <td class="kg-cta-content-inner"> | ||
| <table border="0" cellpadding="0" cellspacing="0" width="100%"> | ||
| ${dataset.textValue ? ` | ||
| <tr> | ||
| <td class="kg-cta-text"> | ||
| ${dataset.textValue} | ||
| </td> | ||
| </tr> | ||
| ` : ''} | ||
| ${showButton(dataset) ? ` | ||
| <tr> | ||
| <td class="kg-cta-button-container"> | ||
| <table border="0" cellpadding="0" cellspacing="0" class="btn"> | ||
| <tr> | ||
| <td class="${dataset.buttonColor === 'accent' ? 'kg-style-accent' : ''}" style="${buttonStyle}"> | ||
| <a href="${dataset.buttonUrl}" | ||
| class="${dataset.buttonColor === 'accent' ? 'kg-style-accent' : ''}" | ||
| style="${buttonStyle}" | ||
| > | ||
| ${dataset.buttonText} | ||
| </a> | ||
| </td> | ||
| </tr> | ||
| </table> | ||
| </td> | ||
| </tr> | ||
| ` : ''} | ||
| </table> | ||
| </td> | ||
| </tr> | ||
| </table> | ||
| </td> | ||
| </tr> | ||
| `; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Large duplicated template blocks – refactor for maintainability
The two renderContent lambdas (feature-flag branch vs fallback) are ~90 % identical. Future tweaks (e.g. button markup, class names) will have to be patched twice, increasing the chance of divergence.
Extract the shared pieces into helper functions or build the HTML using small, composable template builders. A quick win is to compute const isAlpha = options.feature?.emailCustomizationAlpha; and parametrize only the differences (e.g. wrapper class names).
[ Code-level idea, not prescriptive ]
function buttonTd(innerClass) {
return `<td class="${innerClass}" style="${buttonStyle}">`;
}Reducing duplication will aid reviewability and cut e-mail payload size.
Also applies to: 200-313
🤖 Prompt for AI Agents
In packages/kg-default-nodes/lib/nodes/call-to-action/calltoaction-renderer.js
between lines 87 and 134, the renderContent function is duplicated with about
90% identical code in the feature-flag branch and fallback. To fix this, extract
the common HTML template parts into reusable helper functions that generate
smaller pieces like the button markup or image container. Compute a single
boolean for the feature flag (e.g., isAlpha) and use it to conditionally adjust
only the differing parts such as class names or wrapper elements. This refactor
will reduce duplication, improve maintainability, and simplify future updates.
Validate buttonUrl to prevent javascript: or other dangerous schemes
dataset.buttonUrl is interpolated directly into multiple <a href=""> attributes without any sanitisation or protocol checks. A malicious URL (javascript:alert(1), data:text/html;base64,…, etc.) could slip into outgoing emails or the web front-end.
Guard the URL early in the renderer (or, even better, in the card model) and fall back to a safe no-op if the scheme isn’t http, https, or mailto.
+const ALLOWED_PROTOCOLS = /^(https?|mailto):/i;
+if (!ALLOWED_PROTOCOLS.test(dataset.buttonUrl)) {
+ dataset.buttonUrl = '#';
+}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (options.feature?.emailCustomizationAlpha) { | |
| const renderContent = () => { | |
| if (dataset.layout === 'minimal') { | |
| return ` | |
| <tr> | |
| <td class="kg-cta-content"> | |
| <table border="0" cellpadding="0" cellspacing="0" width="100%" class="kg-cta-content-wrapper"> | |
| <tr> | |
| ${dataset.imageUrl ? ` | |
| <td class="kg-cta-image-container" width="64"> | |
| ${wrapWithLink(dataset, `<img src="${dataset.imageUrl}" alt="CTA Image" class="kg-cta-image" width="64" height="64">`)} | |
| </td> | |
| ` : ''} | |
| <td class="kg-cta-content-inner"> | |
| <table border="0" cellpadding="0" cellspacing="0" width="100%"> | |
| ${dataset.textValue ? ` | |
| <tr> | |
| <td class="kg-cta-text"> | |
| ${dataset.textValue} | |
| </td> | |
| </tr> | |
| ` : ''} | |
| ${showButton(dataset) ? ` | |
| <tr> | |
| <td class="kg-cta-button-container"> | |
| <table border="0" cellpadding="0" cellspacing="0" class="btn"> | |
| <tr> | |
| <td class="${dataset.buttonColor === 'accent' ? 'kg-style-accent' : ''}" style="${buttonStyle}"> | |
| <a href="${dataset.buttonUrl}" | |
| class="${dataset.buttonColor === 'accent' ? 'kg-style-accent' : ''}" | |
| style="${buttonStyle}" | |
| > | |
| ${dataset.buttonText} | |
| </a> | |
| </td> | |
| </tr> | |
| </table> | |
| </td> | |
| </tr> | |
| ` : ''} | |
| </table> | |
| </td> | |
| </tr> | |
| </table> | |
| </td> | |
| </tr> | |
| `; | |
| } | |
| if (options.feature?.emailCustomizationAlpha) { | |
| const renderContent = () => { | |
| // only allow http(s) or mailto schemes for buttonUrl | |
| const ALLOWED_PROTOCOLS = /^(https?|mailto):/i; | |
| if (!ALLOWED_PROTOCOLS.test(dataset.buttonUrl)) { | |
| dataset.buttonUrl = '#'; | |
| } | |
| if (dataset.layout === 'minimal') { | |
| return ` | |
| <tr> | |
| <td class="kg-cta-content"> | |
| <table border="0" cellpadding="0" cellspacing="0" width="100%" class="kg-cta-content-wrapper"> | |
| <tr> | |
| ${dataset.imageUrl ? ` | |
| <td class="kg-cta-image-container" width="64"> | |
| ${wrapWithLink(dataset, `<img src="${dataset.imageUrl}" alt="CTA Image" class="kg-cta-image" width="64" height="64">`)} | |
| </td> | |
| ` : ''} | |
| <td class="kg-cta-content-inner"> | |
| <table border="0" cellpadding="0" cellspacing="0" width="100%"> | |
| ${dataset.textValue ? ` | |
| <tr> | |
| <td class="kg-cta-text"> | |
| ${dataset.textValue} | |
| </td> | |
| </tr> | |
| ` : ''} | |
| ${showButton(dataset) ? ` | |
| <tr> | |
| <td class="kg-cta-button-container"> | |
| <table border="0" cellpadding="0" cellspacing="0" class="btn"> | |
| <tr> | |
| <td class="${dataset.buttonColor === 'accent' ? 'kg-style-accent' : ''}" style="${buttonStyle}"> | |
| <a href="${dataset.buttonUrl}" | |
| class="${dataset.buttonColor === 'accent' ? 'kg-style-accent' : ''}" | |
| style="${buttonStyle}" | |
| > | |
| ${dataset.buttonText} | |
| </a> | |
| </td> | |
| </tr> | |
| </table> | |
| </td> | |
| </tr> | |
| ` : ''} | |
| </table> | |
| </td> | |
| </tr> | |
| </table> | |
| </td> | |
| </tr> | |
| `; | |
| } |
🤖 Prompt for AI Agents
In packages/kg-default-nodes/lib/nodes/call-to-action/calltoaction-renderer.js
between lines 87 and 134, the dataset.buttonUrl is used directly in href
attributes without validation, which risks injection of dangerous URL schemes
like javascript:. To fix this, add a validation step before rendering that
checks if buttonUrl starts with http, https, or mailto; if not, replace it with
a safe fallback such as '#' or an empty string. This validation should be done
early in the renderContent function to ensure only safe URLs are used in the
anchor tags.
ref https://linear.app/ghost/issue/PROD-1688/implement-button-corners-setting - The button markup and styling are now consistent across the email template - Added emailCustomizationAlpha feature flag to enable new CTA card button rendering
kevinansfield
force-pushed
the
cta-card-button
branch
from
e3866ad to
1bdf186
Compare
2 participants
Ref https://linear.app/ghost/issue/PROD-1688/implement-button-corners-setting