Updated html markup for CTA card frontend rendering #1442
Conversation
|
Warning Rate limit exceeded@ronaldlangeveld has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 5 minutes and 8 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (2)
WalkthroughThis pull request introduces updates to the call-to-action rendering logic. It adds validation for the Possibly related PRs
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
packages/kg-default-nodes/lib/nodes/call-to-action/calltoaction-renderer.js (1)
6-9: Consider expanding color validation to support more formats.The current regex pattern only allows letters, numbers, and hyphens, which might reject valid CSS color values like hex colors (#FF5733), RGB/RGBA, or HSL/HSLA formats. Consider using a more comprehensive color validation approach.
- if (!dataset.buttonColor || !dataset.buttonColor.match(/^[a-zA-Z\d-]+$/)) { + const isValidColor = (color) => { + // Allow named colors, hex, rgb(a), hsl(a) + const colorRegex = /^(#[0-9A-Fa-f]{3,8}|rgb\(.*\)|rgba\(.*\)|hsl\(.*\)|hsla\(.*\)|[a-zA-Z\d-]+)$/; + return color && colorRegex.test(color); + }; + if (!isValidColor(dataset.buttonColor)) {
📜 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(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Node 20.11.1
| const buttonStyle = dataset.buttonColor === 'accent' | ||
| ? `style="color: ${dataset.buttonTextColor};"` | ||
| : `style="background-color: ${dataset.buttonColor}; color: ${dataset.buttonTextColor};"`; |
There was a problem hiding this comment.
Security: Sanitize color values before interpolation.
Direct interpolation of user input (buttonColor, buttonTextColor) into style attributes could lead to XSS vulnerabilities. Ensure these values are properly sanitized.
+ const sanitizeColor = (color) => {
+ // Implement color value sanitization
+ return color.replace(/[^#,().\d\w%-]/g, '');
+ };
+
const buttonStyle = dataset.buttonColor === 'accent'
- ? `style="color: ${dataset.buttonTextColor};"`
- : `style="background-color: ${dataset.buttonColor}; color: ${dataset.buttonTextColor};"`;
+ ? `style="color: ${sanitizeColor(dataset.buttonTextColor)};"`
+ : `style="background-color: ${sanitizeColor(dataset.buttonColor)}; color: ${sanitizeColor(dataset.buttonTextColor)};"`;📝 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.
| const buttonStyle = dataset.buttonColor === 'accent' | |
| ? `style="color: ${dataset.buttonTextColor};"` | |
| : `style="background-color: ${dataset.buttonColor}; color: ${dataset.buttonTextColor};"`; | |
| const sanitizeColor = (color) => { | |
| // Implement color value sanitization | |
| return color.replace(/[^#,().\d\w%-]/g, ''); | |
| }; | |
| const buttonStyle = dataset.buttonColor === 'accent' | |
| ? `style="color: ${sanitizeColor(dataset.buttonTextColor)};"` | |
| : `style="background-color: ${sanitizeColor(dataset.buttonColor)}; color: ${sanitizeColor(dataset.buttonTextColor)};"`; |
| <div class="kg-card kg-cta-card kg-cta-bg-${dataset.backgroundColor} kg-cta-${dataset.layout}" data-layout="${dataset.layout}"> | ||
| ${dataset.hasSponsorLabel ? ` | ||
| <div class="kg-sponsor-label"> | ||
| <div class="kg-cta-sponsor-label"> | ||
| Sponsored | ||
| </div> | ||
| ` : ''} | ||
| <div class="kg-cta-content"> | ||
| ${dataset.hasImage ? ` | ||
| <div class="kg-cta-image-container"> | ||
| <img src="${dataset.imageUrl}" alt="CTA Image"> | ||
| </div> | ||
| ` : ''} | ||
| <div class="kg-cta-content-inner"> | ||
| <div class="kg-cta-text"> | ||
| ${dataset.textValue} | ||
| </div> | ||
| ${dataset.showButton ? ` | ||
| <a href="${dataset.buttonUrl}" class="kg-cta-button ${buttonAccent}" | ||
| ${buttonStyle}> | ||
| ${dataset.buttonText} | ||
| </a> | ||
| ` : ''} | ||
| </div> | ||
| </div> | ||
| </div> |
There was a problem hiding this comment.
Enhance security and accessibility.
- URLs are directly interpolated without sanitization, which could lead to XSS vulnerabilities.
- The image alt text is generic and not customizable, which impacts accessibility.
+ const sanitizeUrl = (url) => {
+ try {
+ const parsed = new URL(url);
+ return parsed.toString();
+ } catch {
+ return '#';
+ }
+ };
+
return `
<div class="kg-card kg-cta-card kg-cta-bg-${dataset.backgroundColor} kg-cta-${dataset.layout}" data-layout="${dataset.layout}">
${dataset.hasSponsorLabel ? `
<div class="kg-cta-sponsor-label">
Sponsored
</div>
` : ''}
<div class="kg-cta-content">
${dataset.hasImage ? `
<div class="kg-cta-image-container">
- <img src="${dataset.imageUrl}" alt="CTA Image">
+ <img src="${sanitizeUrl(dataset.imageUrl)}" alt="${dataset.imageAltText || 'CTA Image'}">
</div>
` : ''}
<div class="kg-cta-content-inner">
<div class="kg-cta-text">
${dataset.textValue}
</div>
${dataset.showButton ? `
- <a href="${dataset.buttonUrl}" class="kg-cta-button ${buttonAccent}"
+ <a href="${sanitizeUrl(dataset.buttonUrl)}" class="kg-cta-button ${buttonAccent}"📝 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.
| <div class="kg-card kg-cta-card kg-cta-bg-${dataset.backgroundColor} kg-cta-${dataset.layout}" data-layout="${dataset.layout}"> | |
| ${dataset.hasSponsorLabel ? ` | |
| <div class="kg-sponsor-label"> | |
| <div class="kg-cta-sponsor-label"> | |
| Sponsored | |
| </div> | |
| ` : ''} | |
| <div class="kg-cta-content"> | |
| ${dataset.hasImage ? ` | |
| <div class="kg-cta-image-container"> | |
| <img src="${dataset.imageUrl}" alt="CTA Image"> | |
| </div> | |
| ` : ''} | |
| <div class="kg-cta-content-inner"> | |
| <div class="kg-cta-text"> | |
| ${dataset.textValue} | |
| </div> | |
| ${dataset.showButton ? ` | |
| <a href="${dataset.buttonUrl}" class="kg-cta-button ${buttonAccent}" | |
| ${buttonStyle}> | |
| ${dataset.buttonText} | |
| </a> | |
| ` : ''} | |
| </div> | |
| </div> | |
| </div> | |
| const sanitizeUrl = (url) => { | |
| try { | |
| const parsed = new URL(url); | |
| return parsed.toString(); | |
| } catch { | |
| return '#'; | |
| } | |
| }; | |
| return ` | |
| <div class="kg-card kg-cta-card kg-cta-bg-${dataset.backgroundColor} kg-cta-${dataset.layout}" data-layout="${dataset.layout}"> | |
| ${dataset.hasSponsorLabel ? ` | |
| <div class="kg-cta-sponsor-label"> | |
| Sponsored | |
| </div> | |
| ` : ''} | |
| <div class="kg-cta-content"> | |
| ${dataset.hasImage ? ` | |
| <div class="kg-cta-image-container"> | |
| <img src="${sanitizeUrl(dataset.imageUrl)}" alt="${dataset.imageAltText || 'CTA Image'}"> | |
| </div> | |
| ` : ''} | |
| <div class="kg-cta-content-inner"> | |
| <div class="kg-cta-text"> | |
| ${dataset.textValue} | |
| </div> | |
| ${dataset.showButton ? ` | |
| <a href="${sanitizeUrl(dataset.buttonUrl)}" class="kg-cta-button ${buttonAccent}" | |
| ${buttonStyle}> | |
| ${dataset.buttonText} | |
| </a> | |
| ` : ''} | |
| </div> | |
| </div> | |
| </div> | |
| `; |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
packages/kg-default-nodes/lib/nodes/call-to-action/calltoaction-renderer.js (4)
6-9:⚠️ Potential issueEnhance color validation and sanitization.
The current color validation is too restrictive and may reject valid color formats (hex, rgb, hsl). Additionally, the validation logic is duplicated with backgroundColor.
As suggested in previous reviews, implement a comprehensive color validation and sanitization solution:
+// At the top of the file +const isValidColor = (color) => { + const colorRegex = /^(#[0-9A-Fa-f]{3,8}|rgb\(.*\)|rgba\(.*\)|hsl\(.*\)|hsla\(.*\)|[a-zA-Z\d-]+)$/; + return color && colorRegex.test(color); +}; + +const sanitizeColor = (color) => { + return color.replace(/[^#,().\d\w%-]/g, ''); +}; + // In ctaCardTemplate -if (!dataset.buttonColor || !dataset.buttonColor.match(/^[a-zA-Z\d-]+$/)) { +if (!isValidColor(dataset.buttonColor)) { dataset.buttonColor = 'accent'; }
12-14:⚠️ Potential issueSecurity: Sanitize color values before interpolation.
Direct interpolation of user input (buttonColor, buttonTextColor) into style attributes could lead to XSS vulnerabilities.
Apply color sanitization before interpolation:
const buttonStyle = dataset.buttonColor === 'accent' - ? `style="color: ${dataset.buttonTextColor};"` - : `style="background-color: ${dataset.buttonColor}; color: ${dataset.buttonTextColor};"`; + ? `style="color: ${sanitizeColor(dataset.buttonTextColor)};"` + : `style="background-color: ${sanitizeColor(dataset.buttonColor)}; color: ${sanitizeColor(dataset.buttonTextColor)};"`;
17-41:⚠️ Potential issueEnhance security and accessibility in web template.
The template has security and accessibility concerns:
- URLs are directly interpolated without sanitization
- Image alt text is generic and not customizable
Apply URL sanitization and improve accessibility:
+const sanitizeUrl = (url) => { + try { + const parsed = new URL(url); + return parsed.toString(); + } catch { + return '#'; + } +}; -<img src="${dataset.imageUrl}" alt="CTA Image"> +<img src="${sanitizeUrl(dataset.imageUrl)}" alt="${dataset.imageAltText || 'CTA Image'}"> -<a href="${dataset.buttonUrl}" class="kg-cta-button ${buttonAccent}" +<a href="${sanitizeUrl(dataset.buttonUrl)}" class="kg-cta-button ${buttonAccent}"
161-164: 🛠️ Refactor suggestionRefactor duplicated color validation.
The color validation logic is duplicated from the buttonColor validation.
Use the shared color validation function:
-if (!dataset.backgroundColor || !dataset.backgroundColor.match(/^[a-zA-Z\d-]+$/)) { +if (!isValidColor(dataset.backgroundColor)) { dataset.backgroundColor = 'white'; }
📜 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(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Node 20.11.1
| 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.hasImage ? ` | ||
| <td class="kg-cta-image-container" width="64"> | ||
| <img src="${dataset.imageUrl}" alt="CTA Image" class="kg-cta-image" width="64"> | ||
| </td> | ||
| ` : ''} | ||
| <td class="kg-cta-content-inner"> | ||
| <div class="kg-cta-text"> | ||
| ${dataset.textValue} | ||
| </div> | ||
| ${dataset.showButton ? ` | ||
| <a href="${dataset.buttonUrl}" | ||
| class="kg-cta-button ${dataset.buttonColor === 'accent' ? 'kg-style-accent' : ''}" | ||
| style="${buttonStyle}"> | ||
| ${dataset.buttonText} | ||
| </a> | ||
| ` : ''} | ||
| </td> | ||
| </tr> | ||
| </table> | ||
| </td> | ||
| </tr> | ||
| `; | ||
| } | ||
|
|
||
| return ` | ||
| <tr> | ||
| <td class="kg-cta-content"> | ||
| <table border="0" cellpadding="0" cellspacing="0" width="100%" class="kg-cta-content-wrapper"> | ||
| ${dataset.hasImage ? ` | ||
| <tr> | ||
| <td class="kg-cta-image-container"> | ||
| <table border="0" cellpadding="0" cellspacing="0" width="100%"> | ||
| <tr> | ||
| <td> | ||
| <img src="${dataset.imageUrl}" alt="CTA Image" class="kg-cta-image"> | ||
| </td> | ||
| </tr> | ||
| </table> | ||
| </td> | ||
| </tr> | ||
| ` : ''} | ||
| <tr> | ||
| <td class="kg-cta-text"> | ||
| ${dataset.textValue} | ||
| </td> | ||
| </tr> | ||
| ${dataset.showButton ? ` | ||
| <tr> | ||
| <td> | ||
| <table border="0" cellpadding="0" cellspacing="0" width="100%"> | ||
| <tr> | ||
| <td> | ||
| <a href="${dataset.buttonUrl}" | ||
| class="kg-cta-button ${dataset.buttonColor === 'accent' ? 'kg-style-accent' : ''}" | ||
| style="${buttonStyle}"> | ||
| ${dataset.buttonText} | ||
| </a> | ||
| </td> | ||
| </tr> | ||
| </table> | ||
| </td> | ||
| </tr> | ||
| ` : ''} | ||
| </table> | ||
| </td> | ||
| </tr> | ||
| `; | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor email template for maintainability and security.
The email template has several issues:
- Same security vulnerabilities as the web template (unsanitized URLs and colors)
- Complex nested table structure with duplicated code between layouts
Consider these improvements:
- Extract common table structures into reusable functions:
const renderImageSection = (imageUrl, altText, minimal = false) => `
<tr>
<td class="kg-cta-image-container"${minimal ? ' width="64"' : ''}>
<img src="${sanitizeUrl(imageUrl)}"
alt="${altText || 'CTA Image'}"
class="kg-cta-image"
${minimal ? ' width="64"' : ''}>
</td>
</tr>
`;- Apply the same security measures as the web template:
-<img src="${dataset.imageUrl}" alt="CTA Image" class="kg-cta-image">
+<img src="${sanitizeUrl(dataset.imageUrl)}" alt="${dataset.imageAltText || 'CTA Image'}" class="kg-cta-image">
-<a href="${dataset.buttonUrl}"
+<a href="${sanitizeUrl(dataset.buttonUrl)}" 
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)
24-26: Enhance image accessibility with descriptive alt text.The generic alt text "CTA Image" is not descriptive enough for screen readers. Consider adding a property to allow customized alt text.
-<img src="${dataset.imageUrl}" alt="CTA Image"> +<img src="${dataset.imageUrl}" alt="${dataset.imageAltText || 'Call to action promotional image'}">
51-125: Refactor duplicated button rendering logic.The button rendering logic is duplicated between minimal and default layouts. Consider extracting it into a helper function.
+const renderButton = (dataset, buttonStyle) => ` + <a href="${dataset.buttonUrl}" + class="kg-cta-button ${dataset.buttonColor === 'accent' ? 'kg-style-accent' : ''}" + style="${buttonStyle}"> + ${dataset.buttonText} + </a> +`; const renderContent = () => { if (dataset.layout === 'minimal') { // ... minimal layout code ... - <a href="${dataset.buttonUrl}" - class="kg-cta-button ${dataset.buttonColor === 'accent' ? 'kg-style-accent' : ''}" - style="${buttonStyle}"> - ${dataset.buttonText} - </a> + ${renderButton(dataset, buttonStyle)} // ... rest of minimal layout code ... } // ... default layout code ... - <a href="${dataset.buttonUrl}" - class="kg-cta-button ${dataset.buttonColor === 'accent' ? 'kg-style-accent' : ''}" - style="${buttonStyle}"> - ${dataset.buttonText} - </a> + ${renderButton(dataset, buttonStyle)} // ... rest of default layout code ... };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/kg-default-nodes/lib/nodes/call-to-action/calltoaction-renderer.js(2 hunks)packages/kg-default-nodes/test/nodes/call-to-action.test.js(1 hunks)
🔇 Additional comments (1)
packages/kg-default-nodes/lib/nodes/call-to-action/calltoaction-renderer.js (1)
6-14: Security: Sanitize color values before interpolation.Direct interpolation of user input (buttonColor, buttonTextColor) into style attributes could lead to XSS vulnerabilities.
| // Add validation for backgroundColor | ||
| if (!dataset.backgroundColor || !dataset.backgroundColor.match(/^[a-zA-Z\d-]+$/)) { | ||
| dataset.backgroundColor = 'white'; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Align backgroundColor validation with buttonColor validation.
The validation pattern for backgroundColor is more restrictive than buttonColor. Consider using the same pattern for consistency.
-if (!dataset.backgroundColor || !dataset.backgroundColor.match(/^[a-zA-Z\d-]+$/)) {
+if (!dataset.backgroundColor || !dataset.backgroundColor.match(/^[a-zA-Z\d-]+|#([a-fA-F\d]{3}|[a-fA-F\d]{6})$/)) {📝 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.
| // Add validation for backgroundColor | |
| if (!dataset.backgroundColor || !dataset.backgroundColor.match(/^[a-zA-Z\d-]+$/)) { | |
| dataset.backgroundColor = 'white'; | |
| } | |
| // Add validation for backgroundColor | |
| if (!dataset.backgroundColor || !dataset.backgroundColor.match(/^[a-zA-Z\d-]+|#([a-fA-F\d]{3}|[a-fA-F\d]{6})$/)) { | |
| dataset.backgroundColor = 'white'; | |
| } |
| const html = element.outerHTML.toString(); | ||
| html.should.containEql('data-layout="minimal"'); | ||
| html.should.containEql('background-color: green'); | ||
| html.should.containEql('kg-cta-bg-green'); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add test coverage for color validation.
The test suite should include cases for color validation, including:
- Valid hex colors
- Invalid color formats
- Default color fallbacks
it('validates buttonColor format', editorTest(function () {
const testCases = [
{input: '#123', expected: '#123'},
{input: '#123456', expected: '#123456'},
{input: 'invalid<script>', expected: 'accent'},
{input: undefined, expected: 'accent'}
];
testCases.forEach(({input, expected}) => {
const node = new CallToActionNode({...dataset, buttonColor: input});
const {element} = node.exportDOM(exportOptions);
if (expected === 'accent') {
element.outerHTML.should.containEql('kg-style-accent');
} else {
element.outerHTML.should.containEql(`background-color: ${expected}`);
}
});
}));- In order for us to resize the images properly, we need to set the dimentions of the images in order for us to ratio it correctly when it gets scaled for different disply types, eg email
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/kg-default-nodes/lib/nodes/call-to-action/CallToActionNode.js (1)
21-22: Consider adding type validation and constraints for image dimensions.To ensure data integrity and prevent rendering issues:
- Add type validation to ensure numeric values
- Consider adding min/max constraints for valid dimensions
- Document the expected format (pixels, percentage, etc.)
Example enhancement:
- {name: 'imageWidth', default: null}, - {name: 'imageHeight', default: null}, + {name: 'imageWidth', default: null, validate: value => value === null || (typeof value === 'number' && value > 0)}, + {name: 'imageHeight', default: null, validate: value => value === null || (typeof value === 'number' && value > 0)},
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/kg-default-nodes/lib/nodes/call-to-action/CallToActionNode.js(1 hunks)packages/kg-default-nodes/test/nodes/call-to-action.test.js(7 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/kg-default-nodes/test/nodes/call-to-action.test.js
🔇 Additional comments (1)
packages/kg-default-nodes/lib/nodes/call-to-action/CallToActionNode.js (1)
21-22: LGTM! The new image dimension properties are a valuable addition.The
imageWidthandimageHeightproperties will help with proper image rendering and layout in the CTA card.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/koenig-lexical/src/nodes/CallToActionNodeComponent.jsx(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Node 20.11.1
🔇 Additional comments (1)
packages/koenig-lexical/src/nodes/CallToActionNodeComponent.jsx (1)
10-10: LGTM!The import is correctly placed and properly utilized in the
handleImageChangefunction.
| const handleImageChange = async (files) => { | ||
| const imgPreviewUrl = URL.createObjectURL(files[0]); | ||
| const {width, height} = await getImageDimensions(imgPreviewUrl); | ||
| const result = await imageUploader.upload(files); | ||
| // reset original src so it can be replaced with preview and upload progress | ||
| editor.update(() => { | ||
| const node = $getNodeByKey(nodeKey); | ||
| node.imageUrl = result?.[0].url; | ||
| node.hasImage = true; | ||
| node.imageWidth = width; | ||
| node.imageHeight = height; | ||
| }); | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add cleanup for the preview URL to prevent memory leaks.
The preview URL created with URL.createObjectURL should be revoked after use to free up memory.
Apply this diff to add cleanup:
const handleImageChange = async (files) => {
const imgPreviewUrl = URL.createObjectURL(files[0]);
- const {width, height} = await getImageDimensions(imgPreviewUrl);
- const result = await imageUploader.upload(files);
- // reset original src so it can be replaced with preview and upload progress
- editor.update(() => {
- const node = $getNodeByKey(nodeKey);
- node.imageUrl = result?.[0].url;
- node.hasImage = true;
- node.imageWidth = width;
- node.imageHeight = height;
- });
+ try {
+ const {width, height} = await getImageDimensions(imgPreviewUrl);
+ const result = await imageUploader.upload(files);
+ // reset original src so it can be replaced with preview and upload progress
+ editor.update(() => {
+ const node = $getNodeByKey(nodeKey);
+ node.imageUrl = result?.[0].url;
+ node.hasImage = true;
+ node.imageWidth = width;
+ node.imageHeight = height;
+ });
+ } finally {
+ URL.revokeObjectURL(imgPreviewUrl);
+ }
};📝 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.
| const handleImageChange = async (files) => { | |
| const imgPreviewUrl = URL.createObjectURL(files[0]); | |
| const {width, height} = await getImageDimensions(imgPreviewUrl); | |
| const result = await imageUploader.upload(files); | |
| // reset original src so it can be replaced with preview and upload progress | |
| editor.update(() => { | |
| const node = $getNodeByKey(nodeKey); | |
| node.imageUrl = result?.[0].url; | |
| node.hasImage = true; | |
| node.imageWidth = width; | |
| node.imageHeight = height; | |
| }); | |
| }; | |
| const handleImageChange = async (files) => { | |
| const imgPreviewUrl = URL.createObjectURL(files[0]); | |
| try { | |
| const {width, height} = await getImageDimensions(imgPreviewUrl); | |
| const result = await imageUploader.upload(files); | |
| // reset original src so it can be replaced with preview and upload progress | |
| editor.update(() => { | |
| const node = $getNodeByKey(nodeKey); | |
| node.imageUrl = result?.[0].url; | |
| node.hasImage = true; | |
| node.imageWidth = width; | |
| node.imageHeight = height; | |
| }); | |
| } finally { | |
| URL.revokeObjectURL(imgPreviewUrl); | |
| } | |
| }; |
There was a problem hiding this comment.
Would be good to add this in, ObjectURLs will stick around in memory and can use quite a bit when storing large images
There was a problem hiding this comment.
@kevinansfield Thanks for confirming! I'll make sure to flag this in future reviews where ObjectURLs are used without proper cleanup.
✏️ Learnings added
Learnt from: kevinansfield
PR: TryGhost/Koenig#1442
File: packages/koenig-lexical/src/nodes/CallToActionNodeComponent.jsx:94-106
Timestamp: 2025-02-13T09:53:46.951Z
Learning: When using URL.createObjectURL() to create preview URLs for images, always clean up using URL.revokeObjectURL() to prevent memory leaks, as ObjectURLs can consume significant memory especially with large images. Use try/finally to ensure cleanup happens even if an error occurs.
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.
If you found this review helpful, would you consider giving us a shout-out on X?
Thank you for using CodeRabbit!
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
packages/kg-default-nodes/lib/nodes/call-to-action/calltoaction-renderer.js (3)
7-14:⚠️ Potential issueSecurity: Color values need sanitization.
Direct interpolation of user input (
buttonColor,buttonTextColor) into style attributes could lead to XSS vulnerabilities.+ const sanitizeColor = (color) => { + return color.replace(/[^#,().\d\w%-]/g, ''); + }; const buttonStyle = dataset.buttonColor === 'accent' - ? `style="color: ${dataset.buttonTextColor};"` - : `style="background-color: ${dataset.buttonColor}; color: ${dataset.buttonTextColor};"`; + ? `style="color: ${sanitizeColor(dataset.buttonTextColor)};"` + : `style="background-color: ${sanitizeColor(dataset.buttonColor)}; color: ${sanitizeColor(dataset.buttonTextColor)};"`;
16-40:⚠️ Potential issueSecurity and accessibility improvements needed.
- URLs are directly interpolated without sanitization
- Image alt text is not customizable
+ const sanitizeUrl = (url) => { + try { + const parsed = new URL(url); + return parsed.toString(); + } catch { + return '#'; + } + }; - <img src="${dataset.imageUrl}" alt="CTA Image"> + <img src="${sanitizeUrl(dataset.imageUrl)}" alt="${dataset.imageAltText || 'CTA Image'}"> - <a href="${dataset.buttonUrl}" class="kg-cta-button ${buttonAccent}" + <a href="${sanitizeUrl(dataset.buttonUrl)}" class="kg-cta-button ${buttonAccent}"
63-137: 🛠️ Refactor suggestionRefactor email template for maintainability and security.
The email template has several issues:
- Same security vulnerabilities as the web template
- Complex nested table structure with duplicated code
Consider extracting common table structures and applying security measures as suggested in the web template.
🧹 Nitpick comments (1)
packages/kg-default-nodes/lib/nodes/call-to-action/calltoaction-renderer.js (1)
50-61: Extract image dimension logic to a separate function.The image dimension calculation logic could be extracted to improve readability and reusability.
+ const calculateImageDimensions = (width, height) => { + if (!width || !height) { + return null; + } + + const dimensions = { width, height }; + return width >= 560 ? resizeImage(dimensions, { width: 560 }) : dimensions; + }; - let imageDimensions; - if (dataset.imageWidth && dataset.imageHeight) { - imageDimensions = { - width: dataset.imageWidth, - height: dataset.imageHeight - }; - - if (dataset.imageWidth >= 560) { - imageDimensions = resizeImage(imageDimensions, {width: 560}); - } - } + const imageDimensions = calculateImageDimensions(dataset.imageWidth, dataset.imageHeight);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/kg-default-nodes/lib/nodes/call-to-action/calltoaction-renderer.js(2 hunks)packages/kg-default-nodes/test/nodes/call-to-action.test.js(8 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/kg-default-nodes/test/nodes/call-to-action.test.js
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Node 20.11.1
🔇 Additional comments (2)
packages/kg-default-nodes/lib/nodes/call-to-action/calltoaction-renderer.js (2)
140-149: LGTM! Clean and semantic table structure.The outer table structure follows email HTML best practices with proper semantic classes and good separation of concerns.
173-176: Align backgroundColor validation with buttonColor validation.The validation pattern for backgroundColor is more restrictive than buttonColor.
-if (!dataset.backgroundColor || !dataset.backgroundColor.match(/^[a-zA-Z\d-]+$/)) { +if (!dataset.backgroundColor || !dataset.backgroundColor.match(/^[a-zA-Z\d-]+|#([a-fA-F\d]{3}|[a-fA-F\d]{6})$/)) {
| import {renderWithVisibility} from '../../utils/visibility'; | ||
| import {resizeImage} from '../../utils/resize-image'; | ||
|
|
||
| // TODO - this is a placeholder for the cta card web template |
There was a problem hiding this comment.
I think this can be removed now 😄
* Updated html markup for CTA card frontend rendering Ref https://linear.app/ghost/issue/PLG-337/create-cta-card-web-design * Updated CTA card email rendering Ref https://linear.app/ghost/issue/PLG-336/create-cta-card-email-design * Fixed regex not taking hex color into account * Added sponsorLabel to email * Cleanup * Updated email CTA card rendering Ref https://linear.app/ghost/issue/PLG-336/create-cta-card-email-design * Added image sizes to CTA Card Node - In order for us to resize the images properly, we need to set the dimentions of the images in order for us to ratio it correctly when it gets scaled for different disply types, eg email * Added width / height loading to uploader * Added height and width to email rendering * Added cleanup to image change ref TryGhost/Koenig#1442 * Added imageWidth and imageHeight to remove media no issue * Fixed test * Cleanup todo comments --------- Co-authored-by: Ronald Langeveld <[email protected]>
* Updated html markup for CTA card frontend rendering Ref https://linear.app/ghost/issue/PLG-337/create-cta-card-web-design * Updated CTA card email rendering Ref https://linear.app/ghost/issue/PLG-336/create-cta-card-email-design * Fixed regex not taking hex color into account * Added sponsorLabel to email * Cleanup * Updated email CTA card rendering Ref https://linear.app/ghost/issue/PLG-336/create-cta-card-email-design * Added image sizes to CTA Card Node - In order for us to resize the images properly, we need to set the dimentions of the images in order for us to ratio it correctly when it gets scaled for different disply types, eg email * Added width / height loading to uploader * Added height and width to email rendering * Added cleanup to image change ref TryGhost/Koenig#1442 * Added imageWidth and imageHeight to remove media no issue * Fixed test * Cleanup todo comments --------- Co-authored-by: Ronald Langeveld <[email protected]>
3 participants
Ref https://linear.app/ghost/issue/PLG-337/create-cta-card-web-design