serve icons w doctype header [SLT-547] #3500
Conversation
trajan0x
requested review from
bigboydiamonds,
abtestingalpha and
Defi-Moses
as code owners
WalkthroughThis pull request introduces a new dependency Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (3)
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
Documentation and Community
|
Deploying sanguine-fe with
|
| Latest commit: |
4a92515
|
| Status: | ⚡️ Build in progress... |
![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/rest-api/src/utils/svgUtils.ts (1)
1-8: Consider using TypeScript's built-in Buffer typeInstead of importing Buffer from 'buffer', consider using TypeScript's built-in Node.js types which include Buffer.
-import { Buffer } from 'buffer'packages/rest-api/src/routes/chainIconRoute.ts (1)
42-42: Consider adding content length headerFor proper client-side handling, consider adding the Content-Length header to the response.
res.set({ 'Cache-Control': 'public, max-age=604800', 'Content-Type': contentType, + 'Content-Length': processedBuffer.length, })Also applies to: 45-45
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (4)
packages/rest-api/package.json(1 hunks)packages/rest-api/src/routes/addressIconRoute.ts(2 hunks)packages/rest-api/src/routes/chainIconRoute.ts(2 hunks)packages/rest-api/src/utils/svgUtils.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: lint
- GitHub Check: changesets-integrity-checker
- GitHub Check: lint
🔇 Additional comments (3)
packages/rest-api/src/routes/addressIconRoute.ts (2)
40-46: Validate content type before processingSimilar to chainIconRoute.ts, the default content type assumption might lead to incorrect processing of non-SVG files.
Apply the same fix as suggested for chainIconRoute.ts.
51-51: Consider adding content length headerFor consistency with chainIconRoute.ts, add the Content-Length header here as well.
Apply the same fix as suggested for chainIconRoute.ts.
Also applies to: 54-54
packages/rest-api/package.json (1)
35-35:⚠️ Potential issueVerify jsdom version compatibility
The specified jsdom version ^26.0.0 is not yet released (as of January 2025). The latest stable version is 22.1.0. Please update to use a released version.
- "jsdom": "^26.0.0", + "jsdom": "^22.1.0",
| export const addSvgHeaderIfMissing = (buffer: ArrayBuffer): Buffer => { | ||
| const content = Buffer.from(buffer).toString('utf-8') | ||
|
|
||
| // Create a DOM with the SVG content | ||
| const dom = new JSDOM(content, { contentType: 'image/svg+xml' }) | ||
| const document = dom.window.document | ||
|
|
||
| // Get the serialized content to check if we need to add headers | ||
| const serialized = dom.serialize() | ||
| const needsXmlHeader = !serialized.includes('<?xml') | ||
| const needsDoctype = !serialized.includes('<!DOCTYPE') | ||
|
|
||
| if (!needsXmlHeader && !needsDoctype) { | ||
| return Buffer.from(content) | ||
| } | ||
|
|
||
| // Build the final SVG with required headers | ||
| let finalSvg = '' | ||
| if (needsXmlHeader) { | ||
| finalSvg += XML_HEADER + '\n' | ||
| } | ||
| if (needsDoctype) { | ||
| finalSvg += DOCTYPE + '\n' | ||
| } | ||
|
|
||
| // Add the SVG content | ||
| const svgElement = document.querySelector('svg') | ||
| if (svgElement) { | ||
| finalSvg += svgElement.outerHTML | ||
| } else { | ||
| // If no SVG element found, return original content | ||
| return Buffer.from(content) | ||
| } | ||
|
|
||
| return Buffer.from(finalSvg) | ||
| } |
There was a problem hiding this comment.
Add error handling for malformed SVG content
The current implementation might throw if the input is not valid UTF-8 or if JSDOM fails to parse the content. Consider adding try-catch blocks to handle these cases gracefully.
export const addSvgHeaderIfMissing = (buffer: ArrayBuffer): Buffer => {
+ try {
const content = Buffer.from(buffer).toString('utf-8')
// Create a DOM with the SVG content
const dom = new JSDOM(content, { contentType: 'image/svg+xml' })
const document = dom.window.document
// Get the serialized content to check if we need to add headers
const serialized = dom.serialize()
const needsXmlHeader = !serialized.includes('<?xml')
const needsDoctype = !serialized.includes('<!DOCTYPE')
if (!needsXmlHeader && !needsDoctype) {
return Buffer.from(content)
}
// Build the final SVG with required headers
let finalSvg = ''
if (needsXmlHeader) {
finalSvg += XML_HEADER + '\n'
}
if (needsDoctype) {
finalSvg += DOCTYPE + '\n'
}
// Add the SVG content
const svgElement = document.querySelector('svg')
if (svgElement) {
finalSvg += svgElement.outerHTML
} else {
// If no SVG element found, return original content
return Buffer.from(content)
}
return Buffer.from(finalSvg)
+ } catch (error) {
+ console.warn('Failed to process SVG, returning original content:', error)
+ return Buffer.from(buffer)
+ }
}📝 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.
| export const addSvgHeaderIfMissing = (buffer: ArrayBuffer): Buffer => { | |
| const content = Buffer.from(buffer).toString('utf-8') | |
| // Create a DOM with the SVG content | |
| const dom = new JSDOM(content, { contentType: 'image/svg+xml' }) | |
| const document = dom.window.document | |
| // Get the serialized content to check if we need to add headers | |
| const serialized = dom.serialize() | |
| const needsXmlHeader = !serialized.includes('<?xml') | |
| const needsDoctype = !serialized.includes('<!DOCTYPE') | |
| if (!needsXmlHeader && !needsDoctype) { | |
| return Buffer.from(content) | |
| } | |
| // Build the final SVG with required headers | |
| let finalSvg = '' | |
| if (needsXmlHeader) { | |
| finalSvg += XML_HEADER + '\n' | |
| } | |
| if (needsDoctype) { | |
| finalSvg += DOCTYPE + '\n' | |
| } | |
| // Add the SVG content | |
| const svgElement = document.querySelector('svg') | |
| if (svgElement) { | |
| finalSvg += svgElement.outerHTML | |
| } else { | |
| // If no SVG element found, return original content | |
| return Buffer.from(content) | |
| } | |
| return Buffer.from(finalSvg) | |
| } | |
| export const addSvgHeaderIfMissing = (buffer: ArrayBuffer): Buffer => { | |
| try { | |
| const content = Buffer.from(buffer).toString('utf-8') | |
| // Create a DOM with the SVG content | |
| const dom = new JSDOM(content, { contentType: 'image/svg+xml' }) | |
| const document = dom.window.document | |
| // Get the serialized content to check if we need to add headers | |
| const serialized = dom.serialize() | |
| const needsXmlHeader = !serialized.includes('<?xml') | |
| const needsDoctype = !serialized.includes('<!DOCTYPE') | |
| if (!needsXmlHeader && !needsDoctype) { | |
| return Buffer.from(content) | |
| } | |
| // Build the final SVG with required headers | |
| let finalSvg = '' | |
| if (needsXmlHeader) { | |
| finalSvg += XML_HEADER + '\n' | |
| } | |
| if (needsDoctype) { | |
| finalSvg += DOCTYPE + '\n' | |
| } | |
| // Add the SVG content | |
| const svgElement = document.querySelector('svg') | |
| if (svgElement) { | |
| finalSvg += svgElement.outerHTML | |
| } else { | |
| // If no SVG element found, return original content | |
| return Buffer.from(content) | |
| } | |
| return Buffer.from(finalSvg) | |
| } catch (error) { | |
| console.warn('Failed to process SVG, returning original content:', error) | |
| return Buffer.from(buffer) | |
| } | |
| } |
| const contentType = response.headers.get('content-type') || 'image/svg+xml' | ||
|
|
||
| // Only process SVG files | ||
| const processedBuffer = | ||
| contentType === 'image/svg+xml' | ||
| ? addSvgHeaderIfMissing(buffer) | ||
| : Buffer.from(buffer) |
There was a problem hiding this comment.
Validate content type before processing
The default content type of 'image/svg+xml' might lead to incorrect processing of non-SVG files. Consider using the actual content type from the response headers.
- const contentType = response.headers.get('content-type') || 'image/svg+xml'
+ const contentType = response.headers.get('content-type')
+ if (!contentType) {
+ throw new Error('Content-Type header missing from response')
+ }📝 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 contentType = response.headers.get('content-type') || 'image/svg+xml' | |
| // Only process SVG files | |
| const processedBuffer = | |
| contentType === 'image/svg+xml' | |
| ? addSvgHeaderIfMissing(buffer) | |
| : Buffer.from(buffer) | |
| const contentType = response.headers.get('content-type') | |
| if (!contentType) { | |
| throw new Error('Content-Type header missing from response') | |
| } | |
| // Only process SVG files | |
| const processedBuffer = | |
| contentType === 'image/svg+xml' | |
| ? addSvgHeaderIfMissing(buffer) | |
| : Buffer.from(buffer) |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
packages/rest-api/src/routes/chainIconRoute.ts (1)
11-11: Enhance query parameter validation and documentation.The query parameter handling could be more robust:
- Add JSDoc to document the
headersparameter- Validate against unexpected values
- Use a more explicit boolean conversion
+/** + * @param {string} [req.query.headers] - When 'true', adds XML headers to SVG files + */ - const addHeaders = req.query.headers === 'true' + const addHeaders = ['true', '1', 'yes'].includes(req.query.headers?.toLowerCase() ?? '')packages/rest-api/src/routes/addressIconRoute.ts (1)
12-12: Consider validating the headers query parameter.The current implementation only checks for
'true', but other values will silently default tofalse. Consider explicit validation to handle invalid inputs.- const addHeaders = req.query.headers === 'true' + const headersParam = req.query.headers?.toString().toLowerCase() + if (headersParam && !['true', 'false'].includes(headersParam)) { + res.status(400).json({ error: 'Invalid headers parameter. Use "true" or "false".' }) + return + } + const addHeaders = headersParam === 'true'
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (2)
packages/rest-api/src/routes/addressIconRoute.ts(2 hunks)packages/rest-api/src/routes/chainIconRoute.ts(2 hunks)
🧰 Additional context used
🪛 GitHub Check: lint
packages/rest-api/src/routes/addressIconRoute.ts
[warning] 44-44:
Insert ⏎·····
[warning] 45-45:
Insert ··
[warning] 46-46:
Insert ··
packages/rest-api/src/routes/chainIconRoute.ts
[warning] 35-35:
Insert ⏎·····
[warning] 36-36:
Insert ··
[warning] 37-37:
Insert ··
🪛 GitHub Actions: Typescript Packages
packages/rest-api/src/routes/addressIconRoute.ts
[warning] 44-46: Formatting issues with whitespace and line breaks
packages/rest-api/src/routes/chainIconRoute.ts
[warning] 35-37: Formatting issues with whitespace and line breaks
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: test
- GitHub Check: lint
🔇 Additional comments (6)
packages/rest-api/src/routes/chainIconRoute.ts (3)
5-5: LGTM! Clean import addition.The new utility import is properly placed and follows the project's import conventions.
32-33: Validate content type before processing.The default content type of 'image/svg+xml' might lead to incorrect processing of non-SVG files.
42-42: LGTM! Proper response handling.The Content-Type header is correctly set and the processed buffer is properly sent in the response.
Also applies to: 45-45
packages/rest-api/src/routes/addressIconRoute.ts (3)
5-5: LGTM! Clear and well-placed import statement.
51-54: LGTM! Proper response handling with dynamic content type.The response headers and buffer sending are correctly implemented, maintaining the caching behavior while supporting different content types.
41-42: Reconsider the default content type.Defaulting to 'image/svg+xml' might be incorrect if the actual image is not an SVG. Consider using a more generic image type or responding with an error if the content type is missing.
| // Only process SVG files if headers are requested | ||
| const processedBuffer = contentType === 'image/svg+xml' && addHeaders | ||
| ? addSvgHeaderIfMissing(buffer) | ||
| : Buffer.from(buffer) |
There was a problem hiding this comment.
Fix formatting issues.
The conditional block needs proper indentation and line breaks as flagged by the linter.
- const processedBuffer = contentType === 'image/svg+xml' && addHeaders
- ? addSvgHeaderIfMissing(buffer)
- : Buffer.from(buffer)
+ const processedBuffer = contentType === 'image/svg+xml' && addHeaders
+ ? addSvgHeaderIfMissing(buffer)
+ : Buffer.from(buffer)📝 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.
| // Only process SVG files if headers are requested | |
| const processedBuffer = contentType === 'image/svg+xml' && addHeaders | |
| ? addSvgHeaderIfMissing(buffer) | |
| : Buffer.from(buffer) | |
| // Only process SVG files if headers are requested | |
| const processedBuffer = contentType === 'image/svg+xml' && addHeaders | |
| ? addSvgHeaderIfMissing(buffer) | |
| : Buffer.from(buffer) |
🧰 Tools
🪛 GitHub Check: lint
[warning] 35-35:
Insert ⏎·····
[warning] 36-36:
Insert ··
[warning] 37-37:
Insert ··
🪛 GitHub Actions: Typescript Packages
[warning] 35-37: Formatting issues with whitespace and line breaks
| // Only process SVG files if headers are requested | ||
| const processedBuffer = contentType === 'image/svg+xml' && addHeaders | ||
| ? addSvgHeaderIfMissing(buffer) | ||
| : Buffer.from(buffer) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fix indentation as per linting rules.
The conditional expression needs proper indentation.
// Only process SVG files if headers are requested
- const processedBuffer = contentType === 'image/svg+xml' && addHeaders
- ? addSvgHeaderIfMissing(buffer)
- : Buffer.from(buffer)
+ const processedBuffer = contentType === 'image/svg+xml' && addHeaders
+ ? addSvgHeaderIfMissing(buffer)
+ : Buffer.from(buffer)📝 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.
| // Only process SVG files if headers are requested | |
| const processedBuffer = contentType === 'image/svg+xml' && addHeaders | |
| ? addSvgHeaderIfMissing(buffer) | |
| : Buffer.from(buffer) | |
| // Only process SVG files if headers are requested | |
| const processedBuffer = contentType === 'image/svg+xml' && addHeaders | |
| ? addSvgHeaderIfMissing(buffer) | |
| : Buffer.from(buffer) |
🧰 Tools
🪛 GitHub Check: lint
[warning] 44-44:
Insert ⏎·····
[warning] 45-45:
Insert ··
[warning] 46-46:
Insert ··
🪛 GitHub Actions: Typescript Packages
[warning] 44-46: Formatting issues with whitespace and line breaks
|
maybe less relevant |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #3500 +/- ##
===================================================
- Coverage 98.89241% 90.59166% -8.30075%
===================================================
Files 11 54 +43
Lines 632 1031 +399
Branches 0 82 +82
===================================================
+ Hits 625 934 +309
- Misses 7 95 +88
- Partials 0 2 +2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Description
Modification to add doctype header
Summary by CodeRabbit
Release Notes
New Features
Dependencies
jsdomlibrary to support SVG processing.Bug Fixes
The update improves icon rendering and content type management for token and chain icons.