-
Notifications
You must be signed in to change notification settings - Fork 13
chore: validate html descriptions, warn if invalid #196
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from 2 commits
6b1d689
3a5e53d
c036adb
0250f6b
b572809
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,77 @@ | ||
| /** | ||
| * Validates HTML syntax by checking for balanced opening and closing tags | ||
| * @param {string} html - The HTML string to validate | ||
| * @returns {Object} - { valid: boolean, reason: string } | ||
| */ | ||
| function validateHtml(html) { | ||
| if (typeof html !== 'string') { | ||
| return { valid: false, reason: 'Input must be a string' }; | ||
| } | ||
|
|
||
| if (html.trim() === '') { | ||
| return { valid: true, reason: 'Empty string is valid' }; | ||
| } | ||
|
|
||
| const stack = []; | ||
| const selfClosingTags = new Set([ | ||
| 'area', 'base', 'br', 'col', 'embed', 'hr', 'img', 'input', | ||
| 'link', 'meta', 'param', 'source', 'track', 'wbr' | ||
| ]); | ||
|
|
||
| // Regular expression to match HTML tags | ||
| const tagRegex = /<\/?([a-zA-Z][a-zA-Z0-9]*)\b[^>]*>/g; | ||
| let match; | ||
| let lineNumber = 1; | ||
| let charPosition = 0; | ||
|
|
||
| while ((match = tagRegex.exec(html)) !== null) { | ||
| const fullTag = match[0]; | ||
| const tagName = match[1].toLowerCase(); | ||
| const isClosingTag = fullTag.startsWith('</'); | ||
| const isSelfClosing = fullTag.endsWith('/>') || selfClosingTags.has(tagName); | ||
|
|
||
| // Calculate position for error reporting | ||
| const beforeMatch = html.substring(0, match.index); | ||
| lineNumber = beforeMatch.split('\n').length; | ||
| charPosition = match.index - beforeMatch.lastIndexOf('\n') - 1; | ||
|
|
||
| if (isSelfClosing) { | ||
| // Self-closing tags don't need to be balanced | ||
| continue; | ||
| } | ||
|
|
||
| if (isClosingTag) { | ||
| // Check if we have a matching opening tag | ||
| if (stack.length === 0) { | ||
| return { | ||
| valid: false, | ||
| reason: `Unexpected closing tag </${tagName}> at line ${lineNumber}, position ${charPosition}` | ||
| }; | ||
| } | ||
|
|
||
| const lastOpenTag = stack.pop(); | ||
| if (lastOpenTag !== tagName) { | ||
| return { | ||
| valid: false, | ||
| reason: `Mismatched tags: expected </${lastOpenTag}> but found </${tagName}> at line ${lineNumber}, position ${charPosition}` | ||
| }; | ||
| } | ||
| } else { | ||
| // Opening tag - push onto stack | ||
| stack.push(tagName); | ||
| } | ||
| } | ||
|
|
||
| // Check if any opening tags weren't closed | ||
| if (stack.length > 0) { | ||
| const unclosedTags = stack.reverse().join(', '); | ||
| return { | ||
| valid: false, | ||
| reason: `Unclosed tags: ${unclosedTags}` | ||
| }; | ||
| } | ||
|
|
||
| return { valid: true, reason: 'HTML is valid' }; | ||
| } | ||
|
|
||
| module.exports = { validateHtml } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -5,10 +5,11 @@ const { findDescription, prepareBaseTemplate, getPrimaryImage, generatePriceStri | |
| const { generateLdJson } = require('./ldJson'); | ||
| const { requestSaaS, getProductUrl } = require('../utils'); | ||
| const { ProductQuery, ProductByUrlKeyQuery } = require('../queries'); | ||
| const { validateHtml } = require('../lib/validateHtml'); | ||
|
|
||
| const productTemplateCache = {}; | ||
|
|
||
| function toTemplateProductData(baseProduct) { | ||
| function toTemplateProductData(baseProduct, context) { | ||
| const templateProductData = { ...baseProduct }; | ||
| const primaryImage = getPrimaryImage(baseProduct)?.url; | ||
|
|
||
|
|
@@ -20,6 +21,18 @@ function toTemplateProductData(baseProduct) { | |
| templateProductData.primaryImage = primaryImage; | ||
| templateProductData.metaTitle = baseProduct.metaTitle || baseProduct.name || 'Product Details'; | ||
|
|
||
| const fieldValidations = { | ||
| metaDescription: validateHtml(templateProductData.metaDescription), | ||
| shortDescription: validateHtml(templateProductData.shortDescription), | ||
| description: validateHtml(templateProductData.description) | ||
| } | ||
|
|
||
| Object.entries(fieldValidations).forEach(([field, validation]) => { | ||
| if (!validation.valid) { | ||
| context.logger.warn(`Validation failed for "${field}" field: ${validation.reason}`); | ||
|
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this log sufficient to inform the user? Hopefully users do not have to experience what I did - tracing backwards from malformed UX through EDS CDN, Azure, and finally realizing it is the data itself that is the problem. @duynguyen let me know if there's a better way to alert users to bad data.
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. the log message is good but it will potentially get lost among other logs. |
||
| } | ||
| }) | ||
|
|
||
| return templateProductData; | ||
| } | ||
|
|
||
|
|
@@ -65,7 +78,7 @@ async function generateProductHtml(sku, urlKey, context) { | |
| } | ||
|
|
||
| // Assign meta tag data for template | ||
| const templateProductData = toTemplateProductData(baseProduct); | ||
| const templateProductData = toTemplateProductData(baseProduct, context); | ||
|
|
||
| // Generate LD-JSON | ||
| const ldJson = await generateLdJson(baseProduct, context); | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.