Nc feat/canvas formula url #10541
Nc feat/canvas formula url #10541
Conversation
📝 WalkthroughWalkthroughThis update enhances URL handling in the formula cell rendering functionality. The Changes
Suggested labels
Suggested reviewers
✨ 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 (
|
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
packages/nc-gui/components/smartsheet/grid/canvas/cells/Formula.ts (1)
67-85: Consider accessibility improvements for URL rendering.While the URL rendering is visually enhanced, it lacks accessibility considerations for screen readers.
Consider adding ARIA attributes and keyboard navigation support. You can achieve this by maintaining a separate DOM element for accessibility:
+const createAccessibleLink = (label: string, url: string) => { + const link = document.createElement('a') + link.href = url + link.textContent = label + link.setAttribute('aria-label', `${label} - Link`) + link.setAttribute('role', 'link') + return link +} if (typeof urls === 'string') { const { label, suffixText, prefixText } = getFormulaLabelAndUrl(urls) + // Create accessible link in the DOM + const accessibleLink = createAccessibleLink(label, url) ctx.font = `${pv ? 600 : 500} 13px Manrope` ctx.fillStyle = pv ? '#3366FF' : textColorpackages/nc-gui/components/smartsheet/grid/canvas/utils/canvas.ts (1)
1160-1232: Optimize performance with text measurement caching.The function performs repeated text measurements which can be expensive. Consider caching these measurements to improve performance.
Apply this diff to add text measurement caching:
+const measurementCache = new LRUCache<string, number>({ + max: 1000, +}) +function getCachedTextWidth( + ctx: CanvasRenderingContext2D | OffscreenCanvasRenderingContext2D, + text: string, +): number { + const cacheKey = `${text}-${ctx.font}` + let width = measurementCache.get(cacheKey) + if (width === undefined) { + width = ctx.measureText(text).width + measurementCache.set(cacheKey, width) + } + return width +} const renderText = (text: string, isUrl: boolean): void => { const words = text.split(' ') let wordCount = 0 for (const word of words) { wordCount++ const separator = wordCount === words.length ? '' : ' ' - const wordWidth = ctx.measureText(word + separator).width + const wordWidth = getCachedTextWidth(ctx, word + separator)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/nc-gui/components/smartsheet/grid/canvas/cells/Formula.ts(4 hunks)packages/nc-gui/components/smartsheet/grid/canvas/utils/canvas.ts(1 hunks)packages/nc-gui/utils/urlUtils.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: release-docker / buildx
- GitHub Check: pre-build-for-playwright / playwright
- GitHub Check: unit-tests
- GitHub Check: unit-tests-pg
packages/nc-gui/utils/urlUtils.ts
Outdated
| export function getFormulaLabelAndUrl(anchorLinkHTML: string) { | ||
| const parser = new DOMParser() | ||
| const dom = parser.parseFromString(anchorLinkHTML, 'text/html') | ||
| const anchor = dom.querySelector('a') | ||
| return { | ||
| label: anchor?.textContent ?? '', | ||
| url: anchor?.href ?? '', | ||
| suffixText: anchor?.nextSibling?.nodeValue ?? '', | ||
| prefixText: anchor?.previousSibling?.nodeValue ?? '', | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for malformed HTML and missing elements.
The function assumes well-formed HTML and presence of anchor elements. Consider adding error handling to make it more robust.
Apply this diff to improve error handling:
export function getFormulaLabelAndUrl(anchorLinkHTML: string) {
+ if (!anchorLinkHTML) {
+ return {
+ label: '',
+ url: '',
+ suffixText: '',
+ prefixText: '',
+ }
+ }
+
const parser = new DOMParser()
const dom = parser.parseFromString(anchorLinkHTML, 'text/html')
const anchor = dom.querySelector('a')
+
+ if (!anchor) {
+ console.warn('No anchor element found in the provided HTML')
+ return {
+ label: '',
+ url: '',
+ suffixText: '',
+ prefixText: '',
+ }
+ }
+
return {
label: anchor?.textContent ?? '',
url: anchor?.href ?? '',
suffixText: anchor?.nextSibling?.nodeValue ?? '',
prefixText: anchor?.previousSibling?.nodeValue ?? '',
}
}📝 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 function getFormulaLabelAndUrl(anchorLinkHTML: string) { | |
| const parser = new DOMParser() | |
| const dom = parser.parseFromString(anchorLinkHTML, 'text/html') | |
| const anchor = dom.querySelector('a') | |
| return { | |
| label: anchor?.textContent ?? '', | |
| url: anchor?.href ?? '', | |
| suffixText: anchor?.nextSibling?.nodeValue ?? '', | |
| prefixText: anchor?.previousSibling?.nodeValue ?? '', | |
| } | |
| } | |
| export function getFormulaLabelAndUrl(anchorLinkHTML: string) { | |
| if (!anchorLinkHTML) { | |
| return { | |
| label: '', | |
| url: '', | |
| suffixText: '', | |
| prefixText: '', | |
| } | |
| } | |
| const parser = new DOMParser() | |
| const dom = parser.parseFromString(anchorLinkHTML, 'text/html') | |
| const anchor = dom.querySelector('a') | |
| if (!anchor) { | |
| console.warn('No anchor element found in the provided HTML') | |
| return { | |
| label: '', | |
| url: '', | |
| suffixText: '', | |
| prefixText: '', | |
| } | |
| } | |
| return { | |
| label: anchor?.textContent ?? '', | |
| url: anchor?.href ?? '', | |
| suffixText: anchor?.nextSibling?.nodeValue ?? '', | |
| prefixText: anchor?.previousSibling?.nodeValue ?? '', | |
| } | |
| } |
| if (typeof urls === 'string') { | ||
| const { label, suffixText, prefixText, url } = getFormulaLabelAndUrl(urls) | ||
| const ctx = defaultOffscreen2DContext | ||
| ctx.font = `${pv ? 600 : 500} 13px Manrope` | ||
| ctx.fillStyle = pv ? '#3366FF' : textColor | ||
| const boxes = renderMultiLineURLWithPrefixAndSuffix(ctx, { | ||
| urlText: label, | ||
| height, | ||
| maxWidth, | ||
| prefixText, | ||
| suffixText, | ||
| x: x + padding, | ||
| y: y + 3, | ||
| lineHeight: 16, | ||
| underlineOffset: y < 36 ? 0 : 3, | ||
| }) | ||
| if (boxes.some((box) => isBoxHovered(box, props.mousePosition))) { | ||
| window.open(url, '_blank') | ||
| } | ||
| return true | ||
| } |
There was a problem hiding this comment.
Enhance URL security handling.
The current implementation opens URLs in a new window without proper security checks.
Apply this diff to improve URL security:
if (typeof urls === 'string') {
const { label, suffixText, prefixText, url } = getFormulaLabelAndUrl(urls)
+ // Validate URL protocol
+ const safeUrl = (() => {
+ try {
+ const urlObj = new URL(url)
+ // Only allow http/https protocols
+ if (!['http:', 'https:'].includes(urlObj.protocol)) {
+ console.warn('Blocked potentially unsafe URL protocol:', urlObj.protocol)
+ return null
+ }
+ return url
+ } catch {
+ console.warn('Invalid URL:', url)
+ return null
+ }
+ })()
+
const ctx = defaultOffscreen2DContext
ctx.font = `${pv ? 600 : 500} 13px Manrope`
ctx.fillStyle = pv ? '#3366FF' : textColor
const boxes = renderMultiLineURLWithPrefixAndSuffix(ctx, {
urlText: label,
height,
maxWidth,
prefixText,
suffixText,
x: x + padding,
y: y + 3,
lineHeight: 16,
underlineOffset: y < 36 ? 0 : 3,
})
- if (boxes.some((box) => isBoxHovered(box, props.mousePosition))) {
+ if (safeUrl && boxes.some((box) => isBoxHovered(box, props.mousePosition))) {
- window.open(url, '_blank')
+ window.open(safeUrl, '_blank', 'noopener,noreferrer')
}
return true
}📝 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 (typeof urls === 'string') { | |
| const { label, suffixText, prefixText, url } = getFormulaLabelAndUrl(urls) | |
| const ctx = defaultOffscreen2DContext | |
| ctx.font = `${pv ? 600 : 500} 13px Manrope` | |
| ctx.fillStyle = pv ? '#3366FF' : textColor | |
| const boxes = renderMultiLineURLWithPrefixAndSuffix(ctx, { | |
| urlText: label, | |
| height, | |
| maxWidth, | |
| prefixText, | |
| suffixText, | |
| x: x + padding, | |
| y: y + 3, | |
| lineHeight: 16, | |
| underlineOffset: y < 36 ? 0 : 3, | |
| }) | |
| if (boxes.some((box) => isBoxHovered(box, props.mousePosition))) { | |
| window.open(url, '_blank') | |
| } | |
| return true | |
| } | |
| if (typeof urls === 'string') { | |
| const { label, suffixText, prefixText, url } = getFormulaLabelAndUrl(urls) | |
| // Validate URL protocol | |
| const safeUrl = (() => { | |
| try { | |
| const urlObj = new URL(url) | |
| // Only allow http/https protocols | |
| if (!['http:', 'https:'].includes(urlObj.protocol)) { | |
| console.warn('Blocked potentially unsafe URL protocol:', urlObj.protocol) | |
| return null | |
| } | |
| return url | |
| } catch { | |
| console.warn('Invalid URL:', url) | |
| return null | |
| } | |
| })() | |
| const ctx = defaultOffscreen2DContext | |
| ctx.font = `${pv ? 600 : 500} 13px Manrope` | |
| ctx.fillStyle = pv ? '#3366FF' : textColor | |
| const boxes = renderMultiLineURLWithPrefixAndSuffix(ctx, { | |
| urlText: label, | |
| height, | |
| maxWidth, | |
| prefixText, | |
| suffixText, | |
| x: x + padding, | |
| y: y + 3, | |
| lineHeight: 16, | |
| underlineOffset: y < 36 ? 0 : 3, | |
| }) | |
| if (safeUrl && boxes.some((box) => isBoxHovered(box, props.mousePosition))) { | |
| window.open(safeUrl, '_blank', 'noopener,noreferrer') | |
| } | |
| return true | |
| } |
|
Uffizzi Preview |
0d1da96 to
6be6981
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
packages/nc-gui/components/smartsheet/grid/canvas/cells/Formula.ts (1)
97-116:⚠️ Potential issueEnhance URL security handling.
The current implementation opens URLs in a new window without proper security checks.
This is a duplicate of a previous review comment. The same security concerns about URL validation and safe window opening options still apply. Please refer to the previous review comment for the recommended fix.
🧹 Nitpick comments (1)
packages/nc-gui/components/smartsheet/grid/canvas/cells/Formula.ts (1)
62-78: Add error handling for URL processing.The URL rendering logic could benefit from error handling to gracefully handle malformed URLs or text segments.
const urls = replaceUrlsWithLink(result) const maxWidth = width - padding * 2 if (typeof urls === 'string') { + try { const texts = getFormulaTextSegments(urls) ctx.font = `${pv ? 600 : 500} 13px Manrope` ctx.fillStyle = pv ? '#3366FF' : textColor renderFormulaURL(ctx, { texts, height, maxWidth, x: x + padding, y: y + 3, lineHeight: 16, underlineOffset: y < 36 ? 0 : 3, }) return + } catch (error) { + console.warn('Error processing URL:', error) + // Fallback to single line text rendering + SingleLineTextCellRenderer.render(ctx, { + ...props, + value: urls, + formula: true, + }) + return + } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/nc-gui/components/smartsheet/grid/canvas/cells/Formula.ts(4 hunks)packages/nc-gui/components/smartsheet/grid/canvas/utils/canvas.ts(1 hunks)packages/nc-gui/utils/urlUtils.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/nc-gui/utils/urlUtils.ts
- packages/nc-gui/components/smartsheet/grid/canvas/utils/canvas.ts
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: release-docker / buildx
- GitHub Check: pre-build-for-playwright / playwright
- GitHub Check: unit-tests
- GitHub Check: unit-tests-pg
🔇 Additional comments (1)
packages/nc-gui/components/smartsheet/grid/canvas/cells/Formula.ts (1)
1-2: LGTM! Appropriate imports added for formula handling.The new imports for
FormulaDataTypes,handleTZ, and canvas utilities are well-aligned with the URL handling enhancements.
6be6981 to
5ddece6
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
packages/nc-gui/components/smartsheet/grid/canvas/cells/Formula.ts (1)
97-116:⚠️ Potential issueEnhance URL security handling.
The current implementation opens URLs in a new window without proper security checks.
The security concern raised in the past review comments is still valid. Please implement the suggested security checks to protect against malicious URLs.
🧹 Nitpick comments (1)
packages/nc-gui/components/smartsheet/grid/canvas/cells/Formula.ts (1)
117-118: Address TODO comments for inline warnings.The TODO comments indicate that inline warning functionality is incomplete. Consider implementing a more descriptive warning message or a proper error handling mechanism.
Would you like me to help implement a proper warning system for these scenarios?
Also applies to: 126-127
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/nc-gui/components/smartsheet/grid/canvas/cells/Formula.ts(4 hunks)packages/nc-gui/components/smartsheet/grid/canvas/utils/canvas.ts(1 hunks)packages/nc-gui/utils/urlUtils.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/nc-gui/utils/urlUtils.ts
- packages/nc-gui/components/smartsheet/grid/canvas/utils/canvas.ts
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: pre-build-for-playwright / playwright
- GitHub Check: unit-tests-pg
- GitHub Check: unit-tests
- GitHub Check: release-docker / buildx
| @@ -57,10 +58,62 @@ export const FormulaCellRenderer: CellRenderer = { | |||
| }) | |||
| return | |||
| } | |||
| SingleLineTextCellRenderer.render(ctx, props) | |||
|
|
|||
| const urls = replaceUrlsWithLink(result) | |||
There was a problem hiding this comment.
Fix undefined variable reference.
The result variable is used but not defined. This could cause runtime errors.
Add the missing variable definition before line 62:
+ const result = isPg(column.columnObj.source_id) ? renderValue(handleTZ(value)) : renderValue(value)
const urls = replaceUrlsWithLink(result)📝 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 urls = replaceUrlsWithLink(result) | |
| const result = isPg(column.columnObj.source_id) ? renderValue(handleTZ(value)) : renderValue(value) | |
| const urls = replaceUrlsWithLink(result) |
5ddece6 to
5b59c4b
Compare
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 (3)
packages/nc-gui/components/smartsheet/grid/canvas/cells/Formula.ts(4 hunks)packages/nc-gui/components/smartsheet/grid/canvas/utils/canvas.ts(1 hunks)packages/nc-gui/utils/urlUtils.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/nc-gui/components/smartsheet/grid/canvas/utils/canvas.ts
- packages/nc-gui/utils/urlUtils.ts
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: release-docker / buildx
- GitHub Check: pre-build-for-playwright / playwright
- GitHub Check: unit-tests
- GitHub Check: unit-tests-pg
🔇 Additional comments (2)
packages/nc-gui/components/smartsheet/grid/canvas/cells/Formula.ts (2)
62-62: Fix undefined variable reference.The
resultvariable is used but not defined.Add the missing variable definition before line 62:
+ const result = isPg(column.columnObj.source_id) ? renderValue(handleTZ(value)) : renderValue(value) const urls = replaceUrlsWithLink(result)
64-82: Enhance URL security handling.The current implementation processes URLs without proper security validation.
Apply the previously suggested security improvements to validate URL protocols.
| if (hoveredBox) { | ||
| window.open(hoveredBox.url, '_blank') | ||
| } |
There was a problem hiding this comment.
Improve URL click handling security.
The current implementation opens URLs without proper security checks and parameters.
Apply this diff to improve security:
if (hoveredBox) {
- window.open(hoveredBox.url, '_blank')
+ const safeUrl = (() => {
+ try {
+ const urlObj = new URL(hoveredBox.url)
+ if (!['http:', 'https:'].includes(urlObj.protocol)) {
+ console.warn('Blocked unsafe URL protocol:', urlObj.protocol)
+ return null
+ }
+ return hoveredBox.url
+ } catch {
+ console.warn('Invalid URL:', hoveredBox.url)
+ return null
+ }
+ })()
+ if (safeUrl) {
+ window.open(safeUrl, '_blank', 'noopener,noreferrer')
+ }
}📝 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 (hoveredBox) { | |
| window.open(hoveredBox.url, '_blank') | |
| } | |
| if (hoveredBox) { | |
| const safeUrl = (() => { | |
| try { | |
| const urlObj = new URL(hoveredBox.url) | |
| if (!['http:', 'https:'].includes(urlObj.protocol)) { | |
| console.warn('Blocked unsafe URL protocol:', urlObj.protocol) | |
| return null | |
| } | |
| return hoveredBox.url | |
| } catch { | |
| console.warn('Invalid URL:', hoveredBox.url) | |
| return null | |
| } | |
| })() | |
| if (safeUrl) { | |
| window.open(safeUrl, '_blank', 'noopener,noreferrer') | |
| } | |
| } |
5b59c4b to
bc63cd5
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
packages/nc-gui/components/smartsheet/grid/canvas/cells/Formula.ts (1)
91-120:⚠️ Potential issueEnhance URL click handling security.
The URL click handling needs additional security measures.
The current implementation opens URLs without proper security checks. This issue was previously identified in past reviews. Please refer to the existing comment for the recommended fix.
🧹 Nitpick comments (3)
packages/nc-gui/utils/urlUtils.ts (1)
3-44: Improve caching implementation with error handling and cache size monitoring.The caching implementation is good for performance optimization, but could be enhanced with error handling and monitoring.
Consider these improvements:
export const replaceUrlsWithLink = (text: string) => { + if (!text) return false + if (replaceUrlsWithLinkCache.has(text)) { + try { return replaceUrlsWithLinkCache.get(text)! + } catch (error) { + console.warn('Cache retrieval failed:', error) + replaceUrlsWithLinkCache.delete(text) + } } + const result = _replaceUrlsWithLink(text) - replaceUrlsWithLinkCache.set(text, result) + try { + replaceUrlsWithLinkCache.set(text, result) + } catch (error) { + console.warn('Cache set failed:', error) + } return result }packages/nc-gui/components/smartsheet/grid/canvas/utils/canvas.ts (2)
29-35: Consider adjusting cache size based on usage patterns.The cache size is hardcoded to 1000 entries which might not be optimal for all use cases.
Consider making the cache size configurable:
+const DEFAULT_CACHE_SIZE = 1000 + export const replaceUrlsWithLinkCache: LRUCache<string, boolean | string> = new LRUCache({ - max: 1000, + max: process.env.URL_CACHE_SIZE ? parseInt(process.env.URL_CACHE_SIZE) : DEFAULT_CACHE_SIZE, }) export const formulaTextSegmentsCache: LRUCache<string, Array<{ text: string; url?: string }>> = new LRUCache({ - max: 1000, + max: process.env.SEGMENTS_CACHE_SIZE ? parseInt(process.env.SEGMENTS_CACHE_SIZE) : DEFAULT_CACHE_SIZE, })
1170-1237: Optimize text rendering performance and memory usage.The text rendering implementation could be optimized for better performance.
Consider these optimizations:
export function renderFormulaURL( ctx: CanvasRenderingContext2D | OffscreenCanvasRenderingContext2D, params: { texts: Array<{ text: string; url?: string }> x: number y: number maxWidth: number height: number lineHeight: number underlineOffset: number }, ): { x: number; y: number; width: number; height: number; url?: string }[] { const { texts, x, y, maxWidth, height, lineHeight, underlineOffset } = params + // Pre-calculate text metrics for better performance + const textMetrics = new Map<string, number>() let currentX = x let currentY = y let remainingHeight = height const urlRects: { x: number; y: number; width: number; height: number; url: string }[] = [] const renderText = (text: string, url?: string): void => { const words = text.split(' ') let wordCount = 0 for (const word of words) { wordCount++ const separator = wordCount === words.length ? '' : ' ' - const wordWidth = ctx.measureText(word + separator).width + const key = word + separator + let wordWidth = textMetrics.get(key) + if (wordWidth === undefined) { + wordWidth = ctx.measureText(key).width + textMetrics.set(key, wordWidth) + } if (currentX + wordWidth > x + maxWidth) { currentX = x currentY += lineHeight remainingHeight -= lineHeight if (remainingHeight < lineHeight) { return // Stop rendering if out of height } } ctx.fillText(word + separator, currentX, currentY + lineHeight * 0.8) if (url) { urlRects.push({ x: currentX, y: currentY, width: wordWidth, height: lineHeight, url: url, }) + // Batch underline drawing for better performance + if (!ctx.batch) ctx.batch = [] + ctx.batch.push({ + x: currentX, + y: currentY + lineHeight + underlineOffset, + width: wordWidth + }) - const underlineY = currentY + lineHeight + underlineOffset - ctx.strokeStyle = 'black' - ctx.beginPath() - ctx.moveTo(currentX, underlineY) - ctx.lineTo(currentX + wordWidth, underlineY) - ctx.stroke() } currentX += wordWidth } } for (const item of texts) { renderText(item.text, item.url) } + // Batch draw underlines + if (ctx.batch) { + ctx.strokeStyle = 'black' + ctx.beginPath() + for (const {x, y, width} of ctx.batch) { + ctx.moveTo(x, y) + ctx.lineTo(x + width, y) + } + ctx.stroke() + delete ctx.batch + } return urlRects }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/nc-gui/components/smartsheet/grid/canvas/cells/Formula.ts(4 hunks)packages/nc-gui/components/smartsheet/grid/canvas/utils/canvas.ts(3 hunks)packages/nc-gui/utils/urlUtils.ts(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: release-docker / buildx
- GitHub Check: unit-tests
- GitHub Check: unit-tests-pg
🔇 Additional comments (1)
packages/nc-gui/components/smartsheet/grid/canvas/utils/canvas.ts (1)
46-47: LGTM!The cache clearing implementation is correct and comprehensive.
| export function getFormulaTextSegments(anchorLinkHTML: string) { | ||
| if (formulaTextSegmentsCache.has(anchorLinkHTML)) { | ||
| return formulaTextSegmentsCache.get(anchorLinkHTML)! | ||
| } | ||
| const container = document.createElement('div') | ||
| container.innerHTML = anchorLinkHTML | ||
|
|
||
| const result: Array<{ text: string; url?: string }> = [] | ||
|
|
||
| function traverseNodes(node: ChildNode) { | ||
| if (node.nodeType === Node.TEXT_NODE) { | ||
| const text = node.textContent | ||
| if (text) { | ||
| result.push({ text }) | ||
| } | ||
| } else if (node.nodeType === Node.ELEMENT_NODE) { | ||
| if ((node as Element).tagName === 'A') { | ||
| const anchor = node as HTMLAnchorElement | ||
| result.push({ text: node.textContent ?? '', url: anchor.href }) | ||
| } else { | ||
| node.childNodes.forEach(traverseNodes) | ||
| } | ||
| } | ||
| } | ||
|
|
||
| container.childNodes.forEach(traverseNodes) | ||
| formulaTextSegmentsCache.set(anchorLinkHTML, result) | ||
| return result | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance DOM manipulation security and performance.
The function creates DOM elements for HTML parsing which could be optimized and secured.
Apply these improvements:
export function getFormulaTextSegments(anchorLinkHTML: string) {
+ if (!anchorLinkHTML) {
+ return []
+ }
+
if (formulaTextSegmentsCache.has(anchorLinkHTML)) {
return formulaTextSegmentsCache.get(anchorLinkHTML)!
}
+
+ // Sanitize input HTML
+ const sanitizedHTML = anchorLinkHTML.replace(/<script\b[^<]*(?:(?!<\/script>)<[^<]*)*<\/script>/gi, '')
+
const container = document.createElement('div')
- container.innerHTML = anchorLinkHTML
+ container.innerHTML = sanitizedHTML
+
+ // Use DocumentFragment for better performance
+ const fragment = document.createDocumentFragment()
+ fragment.appendChild(container)
const result: Array<{ text: string; url?: string }> = []
function traverseNodes(node: ChildNode) {
if (node.nodeType === Node.TEXT_NODE) {
const text = node.textContent
if (text) {
result.push({ text })
}
} else if (node.nodeType === Node.ELEMENT_NODE) {
if ((node as Element).tagName === 'A') {
const anchor = node as HTMLAnchorElement
+ // Validate URL
+ try {
+ new URL(anchor.href)
+ } catch {
+ console.warn('Invalid URL:', anchor.href)
+ return
+ }
result.push({ text: node.textContent ?? '', url: anchor.href })
} else {
node.childNodes.forEach(traverseNodes)
}
}
}
container.childNodes.forEach(traverseNodes)
formulaTextSegmentsCache.set(anchorLinkHTML, result)
return result
}📝 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 function getFormulaTextSegments(anchorLinkHTML: string) { | |
| if (formulaTextSegmentsCache.has(anchorLinkHTML)) { | |
| return formulaTextSegmentsCache.get(anchorLinkHTML)! | |
| } | |
| const container = document.createElement('div') | |
| container.innerHTML = anchorLinkHTML | |
| const result: Array<{ text: string; url?: string }> = [] | |
| function traverseNodes(node: ChildNode) { | |
| if (node.nodeType === Node.TEXT_NODE) { | |
| const text = node.textContent | |
| if (text) { | |
| result.push({ text }) | |
| } | |
| } else if (node.nodeType === Node.ELEMENT_NODE) { | |
| if ((node as Element).tagName === 'A') { | |
| const anchor = node as HTMLAnchorElement | |
| result.push({ text: node.textContent ?? '', url: anchor.href }) | |
| } else { | |
| node.childNodes.forEach(traverseNodes) | |
| } | |
| } | |
| } | |
| container.childNodes.forEach(traverseNodes) | |
| formulaTextSegmentsCache.set(anchorLinkHTML, result) | |
| return result | |
| } | |
| export function getFormulaTextSegments(anchorLinkHTML: string) { | |
| + if (!anchorLinkHTML) { | |
| + return [] | |
| + } | |
| + | |
| if (formulaTextSegmentsCache.has(anchorLinkHTML)) { | |
| return formulaTextSegmentsCache.get(anchorLinkHTML)! | |
| } | |
| + | |
| + // Sanitize input HTML | |
| + const sanitizedHTML = anchorLinkHTML.replace(/<script\b[^<]*(?:(?!<\/script>)<[^<]*)*<\/script>/gi, '') | |
| + | |
| const container = document.createElement('div') | |
| - container.innerHTML = anchorLinkHTML | |
| + container.innerHTML = sanitizedHTML | |
| + | |
| + // Use DocumentFragment for better performance | |
| + const fragment = document.createDocumentFragment() | |
| + fragment.appendChild(container) | |
| const result: Array<{ text: string; url?: string }> = [] | |
| function traverseNodes(node: ChildNode) { | |
| if (node.nodeType === Node.TEXT_NODE) { | |
| const text = node.textContent | |
| if (text) { | |
| result.push({ text }) | |
| } | |
| } else if (node.nodeType === Node.ELEMENT_NODE) { | |
| if ((node as Element).tagName === 'A') { | |
| const anchor = node as HTMLAnchorElement | |
| + // Validate URL | |
| + try { | |
| + new URL(anchor.href) | |
| + } catch { | |
| + console.warn('Invalid URL:', anchor.href) | |
| + return | |
| + } | |
| result.push({ text: node.textContent ?? '', url: anchor.href }) | |
| } else { | |
| node.childNodes.forEach(traverseNodes) | |
| } | |
| } | |
| } | |
| container.childNodes.forEach(traverseNodes) | |
| formulaTextSegmentsCache.set(anchorLinkHTML, result) | |
| return result | |
| } |
| const urls = replaceUrlsWithLink(result) | ||
| const maxWidth = width - padding * 2 | ||
| if (typeof urls === 'string') { | ||
| const texts = getFormulaTextSegments(urls) | ||
| ctx.font = `${pv ? 600 : 500} 13px Manrope` | ||
| ctx.fillStyle = pv ? '#3366FF' : textColor | ||
| const boxes = renderFormulaURL(ctx, { | ||
| texts, | ||
| height, | ||
| maxWidth, | ||
| x: x + padding, | ||
| y: y + 3, | ||
| lineHeight: 16, | ||
| underlineOffset: y < 36 ? 0 : 3, | ||
| }) | ||
| const hoveredBox = boxes.find((box) => isBoxHovered(box, mousePosition)) | ||
| if (hoveredBox) { | ||
| setCursor('pointer') | ||
| } | ||
| return | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add missing variable declaration and improve URL rendering.
The code has a missing variable declaration and could benefit from improved URL handling.
Apply these fixes:
+ const result = isPg(column.columnObj.source_id) ? renderValue(handleTZ(value)) : renderValue(value)
const urls = replaceUrlsWithLink(result)
const maxWidth = width - padding * 2
if (typeof urls === 'string') {
const texts = getFormulaTextSegments(urls)
+ // Save current context state
+ const originalFont = ctx.font
+ const originalFillStyle = ctx.fillStyle
+
ctx.font = `${pv ? 600 : 500} 13px Manrope`
ctx.fillStyle = pv ? '#3366FF' : textColor
const boxes = renderFormulaURL(ctx, {
texts,
height,
maxWidth,
x: x + padding,
y: y + 3,
lineHeight: 16,
underlineOffset: y < 36 ? 0 : 3,
})
const hoveredBox = boxes.find((box) => isBoxHovered(box, mousePosition))
if (hoveredBox) {
setCursor('pointer')
}
+ // Restore context state
+ ctx.font = originalFont
+ ctx.fillStyle = originalFillStyle
return
}📝 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 urls = replaceUrlsWithLink(result) | |
| const maxWidth = width - padding * 2 | |
| if (typeof urls === 'string') { | |
| const texts = getFormulaTextSegments(urls) | |
| ctx.font = `${pv ? 600 : 500} 13px Manrope` | |
| ctx.fillStyle = pv ? '#3366FF' : textColor | |
| const boxes = renderFormulaURL(ctx, { | |
| texts, | |
| height, | |
| maxWidth, | |
| x: x + padding, | |
| y: y + 3, | |
| lineHeight: 16, | |
| underlineOffset: y < 36 ? 0 : 3, | |
| }) | |
| const hoveredBox = boxes.find((box) => isBoxHovered(box, mousePosition)) | |
| if (hoveredBox) { | |
| setCursor('pointer') | |
| } | |
| return | |
| } | |
| const result = isPg(column.columnObj.source_id) ? renderValue(handleTZ(value)) : renderValue(value) | |
| const urls = replaceUrlsWithLink(result) | |
| const maxWidth = width - padding * 2 | |
| if (typeof urls === 'string') { | |
| const texts = getFormulaTextSegments(urls) | |
| // Save current context state | |
| const originalFont = ctx.font | |
| const originalFillStyle = ctx.fillStyle | |
| ctx.font = `${pv ? 600 : 500} 13px Manrope` | |
| ctx.fillStyle = pv ? '#3366FF' : textColor | |
| const boxes = renderFormulaURL(ctx, { | |
| texts, | |
| height, | |
| maxWidth, | |
| x: x + padding, | |
| y: y + 3, | |
| lineHeight: 16, | |
| underlineOffset: y < 36 ? 0 : 3, | |
| }) | |
| const hoveredBox = boxes.find((box) => isBoxHovered(box, mousePosition)) | |
| if (hoveredBox) { | |
| setCursor('pointer') | |
| } | |
| // Restore context state | |
| ctx.font = originalFont | |
| ctx.fillStyle = originalFillStyle | |
| return | |
| } |
Change Summary
Handle formula URLs in Canvas
Change type
Test/ Verification
Provide summary of changes.
Additional information / screenshots (optional)
Anything for maintainers to be made aware of