feat: Improved Grid View #10520
feat: Improved Grid View #10520
Conversation
…lar to old implementation
📝 WalkthroughWalkthroughThis pull request introduces significant modifications across the application’s UI and grid functionality. Global style updates enhance dropdown visuals and conditional heading formatting in rich text. Several existing cell editor components have been refactored, including the removal of unused dependencies, updated prop access, and refined focus logic. Additionally, a large suite of new files has been added for the smart sheet grid’s canvas layer, including numerous cell renderers, composable functions for grid operations (e.g., column/row reordering, copy/paste, data fetching, fill handling, keyboard and mouse navigation), and supporting loaders and context components. Minor component adjustments and file removals are also included. Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant GC as Grid Canvas
participant GCH as Grid Cell Handler
participant CR as Specific Cell Renderer
participant AM as ActionManager
U->>GC: Interact (click/keydown)
GC->>GCH: Process event & get cell position
GCH->>CR: Delegate rendering based on column type
CR-->>GCH: Render cell content (text/tag/icon)
GCH->>AM: If action required, trigger update/save
AM-->>GC: Update cell state and refresh canvas
GC->>U: Render updated grid view
Possibly related PRs
Suggested labels
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.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments.
🛑 Comments failed to post (57)
packages/nc-gui/components/smartsheet/grid/canvas/cells/Year.ts (3)
73-78: 🛠️ Refactor suggestion
Extract hardcoded dimensions and improve error handling.
The clickable area uses a hardcoded height and lacks error handling for invalid row/column combinations.
Consider these improvements:
+const CELL_HEIGHT = 33 +if (!bound) { + console.warn('Invalid cell position') + return false +} const clickableArea = { x: bound.x + padding, y: bound.y, width: textWidth, - height: 33, + height: CELL_HEIGHT, }📝 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 CELL_HEIGHT = 33 if (!bound) { console.warn('Invalid cell position') return false } const clickableArea = { x: bound.x + padding, y: bound.y, width: textWidth, height: CELL_HEIGHT, }
1-4:
⚠️ Potential issueAdd missing CellRenderer interface import.
The
CellRendererinterface is used but not imported. This could lead to type checking issues.Add the missing import:
import dayjs from 'dayjs' import { defaultOffscreen2DContext, isBoxHovered, renderSingleLineText, renderTagLabel, truncateText } from '../utils/canvas' +import type { CellRenderer } from '../types'📝 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.import dayjs from 'dayjs' import { defaultOffscreen2DContext, isBoxHovered, renderSingleLineText, renderTagLabel, truncateText } from '../utils/canvas' import type { CellRenderer } from '../types' export const YearCellRenderer: CellRenderer = {
11-14: 🛠️ Refactor suggestion
Strengthen year validation.
The current year parsing is not strict enough. It will accept any string that starts with numbers and parse them as valid years.
Add strict parsing to ensure only valid year formats are accepted:
-const year = dayjs(value.toString(), 'YYYY') +const year = dayjs(value.toString(), 'YYYY', 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.const year = dayjs(value.toString(), 'YYYY', true) if (year.isValid()) { text = year.format('YYYY') }packages/nc-gui/components/smartsheet/grid/canvas/cells/QRCode.ts (1)
32-54:
⚠️ Potential issueFix inconsistent y-coordinate calculations in return values.
The y-coordinate calculation is inconsistent between success and failure cases:
- Success case:
y: yPos * 2(where yPos is centered)- Failure case:
y: y + padding + sizeThis inconsistency could cause layout issues when switching between states.
return { x: x + padding + size, - y: yPos * 2, + y: y + padding + size, }📝 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 qrCanvas = imageLoader.loadOrGetQR(qrValue, size, { dark: meta.dark, light: meta.light, }) if (qrCanvas) { const xPos = renderAsTag ? x + padding : x + (width - size) / 2 const yPos = y + (height - size) / 2 imageLoader.renderQRCode(ctx, qrCanvas, xPos, yPos, size) return { x: x + padding + size, y: y + padding + size, } } else { imageLoader.renderPlaceholder(ctx, x + padding, y + padding, size, 'qr_code') return { x: x + padding + size, y: y + padding + size, } } },packages/nc-gui/components/smartsheet/grid/canvas/composables/useRowReOrder.ts (3)
56-72: 🛠️ Refactor suggestion
Optimize drag performance and edge scrolling.
The drag handler could benefit from performance optimizations and better configuration.
Consider implementing throttling and debouncing:
import { throttle, debounce } from 'lodash-es' // Add to configuration interface ScrollConfig { edgeThreshold: number scrollStep: number } // Throttle drag handler const handleDrag = throttle((e: MouseEvent) => { currentDragY.value = e.clientY const rect = canvasRef.value?.getBoundingClientRect() if (!rect) return targetRowIndex.value = findRowFromPosition(e.clientY - rect.top) // Debounce canvas refresh for better performance debouncedRefreshCanvas() const mouseY = e.clientY - rect.top handleEdgeScroll(mouseY, rect.height) }, 16) // ~60fps // Separate edge scrolling logic const handleEdgeScroll = (mouseY: number, rectHeight: number) => { const { edgeThreshold, scrollStep } = config.scroll if (mouseY < edgeThreshold) { scrollToCell(Math.max(0, targetRowIndex.value - scrollStep), 0) } else if (mouseY > rectHeight - edgeThreshold) { scrollToCell(Math.min(totalRows.value - 1, targetRowIndex.value + scrollStep), 0) } } const debouncedRefreshCanvas = debounce(triggerRefreshCanvas, 16)
36-54: 🛠️ Refactor suggestion
Optimize event listeners and add error handling.
The drag start handler could benefit from performance optimizations and better error handling.
Consider these improvements:
const handleDragStart = (e: MouseEvent) => { const rect = canvasRef.value?.getBoundingClientRect() if (!rect) return const rowIndex = findRowFromPosition(e.clientY - rect.top) - 1 const row = cachedRows.value.get(rowIndex) - if (!row) return + if (!row) { + console.warn(`Row ${rowIndex} not found in cache`) + return + } - row.rowMeta.isDragging = true - cachedRows.value.set(rowIndex, row) + try { + row.rowMeta.isDragging = true + cachedRows.value.set(rowIndex, row) + } catch (error) { + console.error('Failed to update row metadata:', error) + return + } isDragging.value = true draggedRowIndex.value = rowIndex targetRowIndex.value = rowIndex + 1 dragStartY.value = e.clientY currentDragY.value = e.clientY - window.addEventListener('mousemove', handleDrag) - window.addEventListener('mouseup', handleDragEnd) + window.addEventListener('mousemove', handleDrag, { passive: true }) + window.addEventListener('mouseup', handleDragEnd, { passive: 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.const handleDragStart = (e: MouseEvent) => { const rect = canvasRef.value?.getBoundingClientRect() if (!rect) return const rowIndex = findRowFromPosition(e.clientY - rect.top) - 1 const row = cachedRows.value.get(rowIndex) if (!row) { console.warn(`Row ${rowIndex} not found in cache`) return } try { row.rowMeta.isDragging = true cachedRows.value.set(rowIndex, row) } catch (error) { console.error('Failed to update row metadata:', error) return } isDragging.value = true draggedRowIndex.value = rowIndex targetRowIndex.value = rowIndex + 1 dragStartY.value = e.clientY currentDragY.value = e.clientY window.addEventListener('mousemove', handleDrag, { passive: true }) window.addEventListener('mouseup', handleDragEnd, { passive: true }) }
73-96: 🛠️ Refactor suggestion
Enhance error handling and cleanup robustness.
The drag end and cleanup functions could benefit from better error handling and more robust cleanup.
Consider these improvements:
async function handleDragEnd() { if (draggedRowIndex.value !== null && draggedRowIndex.value + 1 !== targetRowIndex.value) { - await updateRecordOrder(draggedRowIndex.value, targetRowIndex.value === totalRows.value ? null : targetRowIndex.value) + try { + await updateRecordOrder( + draggedRowIndex.value, + targetRowIndex.value === totalRows.value ? null : targetRowIndex.value + ) + } catch (error) { + console.error('Failed to update record order:', error) + // Consider showing user feedback + } } cleanup() } function cleanup() { + // Reset all dragging related state isDragging.value = false draggedRowIndex.value = null targetRowIndex.value = null - window.removeEventListener('mousemove', handleDrag) - window.removeEventListener('mouseup', handleDragEnd) + + // Clean up throttled/debounced functions + handleDrag.cancel?.() + debouncedRefreshCanvas.cancel?.() + + // Remove event listeners with same options as add + window.removeEventListener('mousemove', handleDrag, { passive: true }) + window.removeEventListener('mouseup', handleDragEnd, { passive: true }) + triggerRefreshCanvas() }📝 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.async function handleDragEnd() { if (draggedRowIndex.value !== null && draggedRowIndex.value + 1 !== targetRowIndex.value) { try { await updateRecordOrder( draggedRowIndex.value, targetRowIndex.value === totalRows.value ? null : targetRowIndex.value ) } catch (error) { console.error('Failed to update record order:', error) // Consider showing user feedback } } cleanup() } function cleanup() { // Reset all dragging related state isDragging.value = false draggedRowIndex.value = null targetRowIndex.value = null // Clean up throttled/debounced functions handleDrag.cancel?.() debouncedRefreshCanvas.cancel?.() // Remove event listeners with same options as add window.removeEventListener('mousemove', handleDrag, { passive: true }) window.removeEventListener('mouseup', handleDragEnd, { passive: true }) triggerRefreshCanvas() } onBeforeUnmount(() => { cleanup() }) return { handleDragStart, } }packages/nc-gui/components/smartsheet/grid/canvas/cells/Duration.ts (2)
1-3:
⚠️ Potential issueAdd missing imports for types and utility functions.
Several types and utility functions are used but not imported:
CellRenderertype/interfaceisValidValue,parseProp, andconvertMS2Durationutility functionsAdd the following imports:
+import type { CellRenderer } from '../types' import { renderSingleLineText, renderTagLabel } from '../utils/canvas' +import { isValidValue, parseProp, convertMS2Duration } from '../utils/cell'📝 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.import type { CellRenderer } from '../types' import { renderSingleLineText, renderTagLabel } from '../utils/canvas' import { isValidValue, parseProp, convertMS2Duration } from '../utils/cell' export const DurationCellRenderer: CellRenderer = {
41-45: 🛠️ Refactor suggestion
Avoid direct row mutation and add error handling.
The current implementation directly mutates the row data and lacks error handling for
makeCellEditable.- if (/^[0-9]$/.test(e.key) && columnObj.title) { - row.row[columnObj.title] = '' - makeCellEditable(row, column) - return true - } + if (/^[0-9]$/.test(e.key) && columnObj.title) { + try { + // Use a proper update method instead of direct mutation + await updateRowValue(row, columnObj.title, '') + await makeCellEditable(row, column) + return true + } catch (error) { + console.error('Failed to make cell editable:', error) + return false + } + }📝 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 (/^[0-9]$/.test(e.key) && columnObj.title) { try { // Use a proper update method instead of direct mutation await updateRowValue(row, columnObj.title, '') await makeCellEditable(row, column) return true } catch (error) { console.error('Failed to make cell editable:', error) return false } }packages/nc-gui/components/smartsheet/grid/canvas/cells/Checkbox.ts (6)
1-2:
⚠️ Potential issueAdd missing imports.
The following types/constants are used but not imported:
CellRenderertyperowHeightInPxconstantAdd the missing imports at the top of the file:
import { isBoxHovered, renderTag } from '../utils/canvas' +import type { CellRenderer } from '../types' +import { rowHeightInPx } from '../constants'📝 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.import { isBoxHovered, renderTag } from '../utils/canvas' import type { CellRenderer } from '../types' import { rowHeightInPx } from '../constants' export const CheckboxCellRenderer: CellRenderer = { // rest of the implementation... }
70-82: 🛠️ Refactor suggestion
Improve error handling and type safety in keyboard handler.
The keyboard handler has several areas for improvement:
- Inconsistent variable naming (
columnObjvscolumn)- Direct property access without type checking
- Missing error handling for async operation
async handleKeyDown(ctx) { const { e, row, column, updateOrSaveRow, readonly } = ctx - const columnObj = column.columnObj + const { columnObj } = column if (column.readonly || readonly) return if (e.key === 'Enter') { - row.row[columnObj.title!] = !row.row[columnObj.title!] - await updateOrSaveRow(row, columnObj.title) + const title = columnObj.title + if (!title) return false + try { + row.row[title] = !row.row[title] + await updateOrSaveRow(row, title) + } catch (error) { + console.error('Failed to update checkbox value:', error) + } return true } return false }📝 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.async handleKeyDown(ctx) { const { e, row, column, updateOrSaveRow, readonly } = ctx const { columnObj } = column if (column.readonly || readonly) return if (e.key === 'Enter') { const title = columnObj.title if (!title) return false try { row.row[title] = !row.row[title] await updateOrSaveRow(row, title) } catch (error) { console.error('Failed to update checkbox value:', error) } return true } return false },
24-24: 🛠️ Refactor suggestion
Return coordinates on early exit.
Early return without coordinates could affect layout calculations in the grid. Consider returning default coordinates.
-if (readonly && !checked && !renderAsTag) return +if (readonly && !checked && !renderAsTag) return { x, y: y + height }📝 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 (readonly && !checked && !renderAsTag) return { x, y: y + height }
18-22:
⚠️ Potential issueAdd missing imports for utility functions.
The following utility functions are used but not imported:
parsePropextractCheckboxIconAdd the missing imports at the top of the file:
import { isBoxHovered, renderTag } from '../utils/canvas' +import { parseProp, extractCheckboxIcon } from '../utils/helpers'📝 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.import { isBoxHovered, renderTag } from '../utils/canvas' import { parseProp, extractCheckboxIcon } from '../utils/helpers' // ... other imports and file content const columnMeta = { color: 'primary', ...parseProp(column.meta), icon: extractCheckboxIcon(column?.meta ?? {}), }
4-4:
⚠️ Potential issueUse safe array access.
Direct array access with the non-null assertion operator (
!) is unsafe. Consider using a default value or runtime check.-height = rowHeightInPx[1]! +height = rowHeightInPx[1] ?? rowHeightInPx[0] ?? 32📝 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.height = rowHeightInPx[1] ?? rowHeightInPx[0] ?? 32
83-109: 🛠️ Refactor suggestion
Improve error handling and maintainability in click handler.
The click handler has several areas for improvement:
- Missing type checking for column title
- Missing error handling for async operation
- Magic numbers in checkbox bounds calculation
async handleClick(ctx) { const { row, column, updateOrSaveRow, getCellPosition, mousePosition, selected, readonly } = ctx if (column.readonly || readonly) return false if (selected) { - row.row[column.title!] = !row.row[column.title!] - await updateOrSaveRow(row, column.title) + const title = column.title + if (!title) return false + try { + row.row[title] = !row.row[title] + await updateOrSaveRow(row, title) + } catch (error) { + console.error('Failed to update checkbox value:', error) + } return true } const bounds = getCellPosition(column, row.rowMeta.rowIndex!) + const CHECKBOX_SIZE = 14 + const VERTICAL_OFFSET = 8 const checkboxBounds = { - x: bounds.x + bounds.width / 2 - 7, - y: bounds.y + bounds.height / 2 - 8, - width: 14, - height: 14, + x: bounds.x + bounds.width / 2 - CHECKBOX_SIZE / 2, + y: bounds.y + bounds.height / 2 - VERTICAL_OFFSET, + width: CHECKBOX_SIZE, + height: CHECKBOX_SIZE, } if (isBoxHovered(checkboxBounds, mousePosition)) { - row.row[column.title!] = !row.row[column.title!] - await updateOrSaveRow(row, column.title) + const title = column.title + if (!title) return false + try { + row.row[title] = !row.row[title] + await updateOrSaveRow(row, title) + } catch (error) { + console.error('Failed to update checkbox value:', error) + } return true } return false }📝 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.async handleClick(ctx) { const { row, column, updateOrSaveRow, getCellPosit F438 ion, mousePosition, selected, readonly } = ctx if (column.readonly || readonly) return false if (selected) { const title = column.title if (!title) return false try { row.row[title] = !row.row[title] await updateOrSaveRow(row, title) } catch (error) { console.error('Failed to update checkbox value:', error) } return true } const bounds = getCellPosition(column, row.rowMeta.rowIndex!) const CHECKBOX_SIZE = 14 const VERTICAL_OFFSET = 8 const checkboxBounds = { x: bounds.x + bounds.width / 2 - CHECKBOX_SIZE / 2, y: bounds.y + bounds.height / 2 - VERTICAL_OFFSET, width: CHECKBOX_SIZE, height: CHECKBOX_SIZE, } if (isBoxHovered(checkboxBounds, mousePosition)) { const title = column.title if (!title) return false try { row.row[title] = !row.row[title] await updateOrSaveRow(row, title) } catch (error) { console.error('Failed to update checkbox value:', error) } return true } return false },packages/nc-gui/components/smartsheet/grid/canvas/composables/useDataFetch.ts (1)
24-37: 🛠️ Refactor suggestion
Handle out-of-range
chunkId + 1for safetyWhen
isInitialLoadis true, the code sets the state ofchunkId + 1to'loading'or'loaded'. IfchunkIdhappens to be the last possible chunk, this can go out of range. Consider guarding against that scenario to avoid potential index errors or undefined behavior.A minimal fix could be:
if (isInitialLoad && chunkId + 1 < chunkStates.value.length) { chunkStates.value[chunkId + 1] = 'loading' }📝 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 fetchChunk = async (chunkId: number, isInitialLoad = false) => { if (chunkStates.value[chunkId]) return const offset = chunkId * CHUNK_SIZE const limit = isInitialLoad ? INITIAL_LOAD_SIZE : CHUNK_SIZE if (offset >= totalRows.value) { return } chunkStates.value[chunkId] = 'loading' if (isInitialLoad && chunkId + 1 < chunkStates.value.length) { chunkStates.value[chunkId + 1] = 'loading' }packages/nc-gui/components/smartsheet/grid/canvas/composables/useColumnResize.ts (1)
75-77: 🛠️ Refactor suggestion
Unify minimum-width enforcement with
normalizeWidth.
Currently, a hardcoded50minimum width is used in multiple places (lines 76, 126) whilenormalizeWidth()applies different fallback minimum widths (80) for unknown column types. Consider consistently usingnormalizeWidth()to ensure that all columns respect the same constraints.--- a/packages/nc-gui/components/smartsheet/grid/canvas/composables/useColumnResize.ts +++ b/packages/nc-gui/components/smartsheet/grid/canvas/composables/useColumnResize.ts @@ -74,7 +74,7 @@ const handleMouseMove = (e: MouseEvent) => { if (isResizing.value && activeColumn.value) { const delta = mousePosition.value.x - activeColumn.value.startX - const newWidth = Math.max(50, activeColumn.value.initialWidth + delta) + const newWidth = normalizeWidth({ uidt: undefined }, activeColumn.value.initialWidth + delta) onResize?.(activeColumn.value.id, newWidth) } @@ -125,7 +125,7 @@ if (shouldTriggerResize && activeColumn.value && mousePosition.value) { const delta = mousePosition.value.x - activeColumn.value.startX - const newWidth = Math.max(50, activeColumn.value.initialWidth + delta) + const newWidth = normalizeWidth({ uidt: undefined }, activeColumn.value.initialWidth + delta) onResizeEnd?.(activeColumn.value.id, newWidth) }Also applies to: 126-127, 165-170
packages/nc-gui/components/smartsheet/grid/canvas/cells/LTAR/ManyToMany.ts (1)
77-79:
⚠️ Potential issueBug: Unused
countvariable and missing iteration logic.
The variablecountis never incremented, yet it’s used in conditions like(count < cells.length). This likely prevents correct handling of multiple cells and the rendering of the ellipsis when items overflow.- let flag = false - const count = 1 - for (const cell of cells) { + let flag = false + let index = 0 + for (const cell of cells) { ... if (point?.x >= x + initialWidth - padding * 2 - (index < cells.length - 1 ? 50 - ellipsisWidth : 0)) { ... } index++Also applies to: 88-94, 117-117
packages/nc-gui/components/smartsheet/grid/canvas/cells/LTAR/HasMany.ts (1)
77-79:
⚠️ Potential issueBug: Unused
countvariable and missing iteration logic.
Just likeManyToMany.ts,countis never incremented, but it’s used to decide if ellipses should be displayed. This will break overflow handling when multiple items exist.- let flag = false - const count = 1 - for (const cell of cells) { + let flag = false + let index = 0 + for (const cell of cells) { ... if (point?.x >= x + initialWidth - padding * 2 - (index < cells.length - 1 ? 50 - ellipsisWidth : 0)) { ... } index++Also applies to: 88-94, 121-121
packages/nc-gui/components/smartsheet/grid/canvas/cells/LongText.ts (1)
97-124: 🛠️ Refactor suggestion
Open new windows securely
When callingwindow.open(link.url, '_blank'), consider addingnoopenerornoreferrerto protect the app from reverse tabnabbing or external manipulations. For example:- window.open(link.url, '_blank') + window.open(link.url, '_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.handleClick: async (props) => { const { column, getCellPosition, row, mousePosition, makeCellEditable, cellRenderStore } = props const isRichMode = column.columnObj?.meta?.richMode if (isRichMode) { const links: { x: number; y: number; width: number; height: number; url: string }[] = cellRenderStore?.links || [] for (const link of links) { if (isBoxHovered(link, mousePosition)) { - window.open(link.url, '_blank') + window.open(link.url, '_blank', 'noopener,noreferrer') return true } } } if (isAIPromptCol(column?.columnObj)) { return AILongTextCellRenderer.handleClick!(props) } else { const { x, y, width } = getCellPosition(column, row.rowMeta.rowIndex!) if (isBoxHovered({ x: x + width - 28, y: y + 7, width: 18, height: 18 }, mousePosition)) { makeCellEditable(row.rowMeta.rowIndex!, column) return true } return false } },packages/nc-gui/components/smartsheet/grid/canvas/loaders/ActionManager.ts (1)
65-101: 🛠️ Refactor suggestion
Optimize or evaluate performance overhead in animation loop
startAnimationLoop()callsrequestAnimationFramerepeatedly while actions are loading, plus a cooldown. This approach may become expensive if many actions are triggered in quick succession. It might be worth implementing a more throttled or debounced rendering approach to avoid excessive UI updates.packages/nc-gui/components/smartsheet/grid/canvas/loaders/ImageLoader.ts (2)
84-110: 🛠️ Refactor suggestion
Harmonize calling
this.onSettled?.()with theloadImageflow.
generateQRcallsonSettledeach time a QR is done, whereasloadImageonly callsonSettledwhen all images finish (pendingSprites === 0). Consider aligning these behaviors—if you only want a callback once all loads are complete, do so in both places consistently.
18-63: 🛠️ Refactor suggestion
Return a consistent placeholder or in-progress reference for multiple calls.
If a subsequent call toloadOrGetImage()arrives while the first is still loading, the function currently returnsundefined. To improve usability, you could return the same promise or a placeholder so that multiple calls can share a single loading flow instead of proceeding blindly.- if (isAnyLoading) { - return undefined - } + if (isAnyLoading) { + // Return the same promise or a custom handle to represent an in-flight image + return undefined // or a reference to the existing loading promise + }Committable suggestion skipped: line range outside the PR's diff.
packages/nc-gui/components/smartsheet/grid/canvas/cells/User.ts (1)
127-151: 🛠️ Refactor suggestion
Handle missing images gracefully.
WhenuserIcon.iconType === IconType.IMAGE, if loading the image fails orimgis not retrieved, a fallback is still needed. Consider an error callback or a placeholder icon if an image can’t be rendered.packages/nc-gui/components/smartsheet/grid/canvas/composables/useKeyboardNavigation.ts (1)
57-72: 🛠️ Refactor suggestion
Evaluate breaking down
handleKeyDownfor maintainability.This block begins a large conditional chain. While it’s logically correct, the length and nested checks suggest that splitting out distinct logic paths (e.g., handling open dropdowns, expansions, or modals) into helper functions would improve clarity and testability.
packages/nc-gui/components/smartsheet/grid/canvas/cells/index.ts (1)
39-53: 🛠️ Refactor suggestion
Extract typed definitions for the handler’s parameter object.
useGridCellHandlerhas a complex parameter object with multiple function references and typed data. Define an interface or type alias to clarify each field’s usage (e.g.,SetCursorType) and to standardize the usage across the codebase.packages/nc-gui/components/smartsheet/grid/canvas/context/Cell.vue (1)
101-117:
⚠️ Potential issueAwait async function calls for proper error handling.
deleteRow?.(row)is asynchronous, but it’s invoked in a synchronous try/catch. The error may never be caught if the promise rejects later. Useawaitinside an async function or handle the promise explicitly.- deleteRow?.(row) + await deleteRow?.(row)Committable suggestion skipped: line range outside the PR's diff.
packages/nc-gui/components/smartsheet/grid/canvas/composables/useCanvasTable.ts (1)
890-936: 🛠️ Refactor suggestion
Extract or streamline the
makeCellEditablelogic.The logic has many conditional checks for read/write states, system columns, PK columns, etc., making it lengthy and harder to maintain. Consider extracting subroutines (e.g., PK checks, AI checks, system column checks) or using a helper composable to handle different column constraints more cleanly.
packages/nc-gui/components/smartsheet/grid/canvas/composables/useCanvasRender.ts (1)
797-1225: 🛠️ Refactor suggestion
Extract row drawing logic into smaller functions.
renderRowshandles hovering, selection, row ordering, and partial row rendering. This monolithic approach can become difficult to maintain. Splitting out specialized helpers—e.g., one for drawing row backgrounds, one for rendering cells, and one for row metadata—would reduce complexity.packages/nc-gui/components/smartsheet/grid/canvas/index.vue (1)
828-849: 🛠️ Refactor suggestion
Use structured approaches for new column creation logic.
Within
handleMouseUp, there's a conditional block to open the “Add New Column” dropdown (ADD_NEW_COLUMN_WIDTH, etc.). Much of this logic is repeated inaddEmptyColumnandopenColumnCreate. It might be beneficial to unify them in a single composable or helper function to avoid potential maintenance overhead.packages/nc-gui/components/smartsheet/grid/Table.vue (1)
5-5:
⚠️ Potential issuePossible undefined reference to removed
isReadonlylogicLine 443 in this file still references
isReadonlyin thecolMetacomputed property, but the removedisReadonlyfunction is no longer imported. This can cause a runtime error and break the read-only logic for columns. Reintroduce or replaceisReadonlylogic to avoid undefined references and maintain the expected read-only behavior.- import { UITypes, ViewTypes, isAIPromptCol, isLinksOrLTAR, isSystemColumn, isVirtualCol } from 'nocodb-sdk' + import { UITypes, ViewTypes, isAIPromptCol, isLinksOrLTAR, isSystemColumn, isVirtualCol } from 'nocodb-sdk' // restore or re-implement isReadonly here, for example: + function isReadonly(col: ColumnType) { + // reintroduce or replicate the original read-only checks + return false // placeholder + }📝 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.import { UITypes, ViewTypes, isAIPromptCol, isLinksOrLTAR, isSystemColumn, isVirtualCol } from 'nocodb-sdk' // restore or re-implement isReadonly here, for example: function isReadonly(col: ColumnType) { // reintroduce or replicate the original read-only checks return false // placeholder }packages/nc-gui/components/smartsheet/grid/canvas/cells/LTAR/index.ts (2)
21-26:
⚠️ Potential issueFix type inconsistency in handleClick method.
The
handleClickmethod usesprops.column.columnObjwhile therendermethod usesprops.columndirectly. This inconsistency could lead to type errors.Ensure consistent type usage:
handleClick: async (props) => { - const cellRenderer = getLtarCellRenderer(props.column.columnObj) + const cellRenderer = getLtarCellRenderer(props.column) if (cellRenderer) { - return cellRenderer?.handleClick?.(props) + return cellRenderer.handleClick?.(props) } },📝 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.handleClick: async (props) => { const cellRenderer = getLtarCellRenderer(props.column) if (cellRenderer) { return cellRenderer.handleClick?.(props) } },
1-6:
⚠️ Potential issueAdd missing type imports.
The file is missing imports for several types and helper functions:
CellRenderertype- Helper functions:
isHm,isMm,isBt,isOoAdd the missing imports:
import type { ColumnType } from 'nocodb-sdk' +import type { CellRenderer } from '../types' +import { isHm, isMm, isBt, isOo } from '../utils/helpers' import { BelongsToCellRenderer } from './BelongsTo'📝 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.import type { ColumnType } from 'nocodb-sdk' import type { CellRenderer } from '../types' import { isHm, isMm, isBt, isOo } from '../utils/helpers' import { BelongsToCellRenderer } from './BelongsTo' import { HasManyCellRenderer } from './HasMany' import { ManyToManyCellRenderer } from './ManyToMany' import { OneToOneCellRenderer } from './OneToOne'packages/nc-gui/components/smartsheet/grid/canvas/loaders/TableMetaLoader.ts (1)
16-26:
⚠️ Potential issueFix incorrect return behavior and add error handling in getTableMeta.
The current implementation has several issues:
- Returns undefined when loading is in progress instead of waiting for the result
- Doesn't handle errors from getMeta
- Doesn't return the loaded metadata
Implement proper async handling:
async getTableMeta(tableIdOrTitle: string): Promise<TableType | undefined> { if (this.loadingCache.get(tableIdOrTitle)) return this.loadingCache.set(tableIdOrTitle, tableIdOrTitle) try { - await this.getMeta(tableIdOrTitle) + const meta = await this.getMeta(tableIdOrTitle) this.onSettled?.() + return meta || undefined + } catch (error) { + console.error('Failed to load table metadata:', error) + return undefined } finally { this.loadingCache.delete(tableIdOrTitle) } }📝 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.async getTableMeta(tableIdOrTitle: string): Promise<TableType | undefined> { if (this.loadingCache.get(tableIdOrTitle)) return this.loadingCache.set(tableIdOrTitle, tableIdOrTitle) try { const meta = await this.getMeta(tableIdOrTitle) this.onSettled?.() return meta || undefined } catch (error) { console.error('Failed to load table metadata:', error) return undefined } finally { this.loadingCache.delete(tableIdOrTitle) } }packages/nc-gui/components/smartsheet/grid/canvas/context/Aggregation.vue (2)
16-17:
⚠️ Potential issueFix potential null reference in computed properties.
The computed properties don't handle the case when column.value is null.
Add null checks:
-const gridCol = computed(() => gridViewCols.value[column.value.id]) -const aggregations = computed(() => getAggregations(column.value.columnObj)) +const gridCol = computed(() => column.value ? gridViewCols.value[column.value.id] : undefined) +const aggregations = computed(() => column.value ? getAggregations(column.value.columnObj) : [])📝 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 gridCol = computed(() => column.value ? gridViewCols.value[column.value.id] : undefined) const aggregations = computed(() => column.value ? getAggregations(column.value.columnObj) : [])
1-4:
⚠️ Potential issueAdd missing type imports and definitions.
The component is missing several type imports and has potential type safety issues.
Add the required types:
<script setup lang="ts"> +import type { CanvasGridColumn } from '../types' +import type { AggregationType } from 'nocodb-sdk' const props = defineProps<{ column: CanvasGridColumn | null📝 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.<script setup lang="ts"> import type { CanvasGridColumn } from '../types' import type { AggregationType } from 'nocodb-sdk' const props = defineProps<{ column: CanvasGridColumn | null }>()packages/nc-gui/components/smartsheet/grid/canvas/cells/SingleLineText.ts (2)
1-6:
⚠️ Potential issueAdd missing type imports and interfaces.
The file is missing necessary type definitions.
Add required types:
+import type { CellRenderer, RenderProps, KeyboardEventContext } from '../types' import { renderMultiLineText, renderTagLabel } from '../utils/canvas'📝 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.import type { CellRenderer, RenderProps, KeyboardEventContext } from '../types' import { renderMultiLineText, renderTagLabel } from '../utils/canvas' export const SingleLineTextCellRenderer: CellRenderer = { render: (ctx, props) => { const { value, x, y, width, height, pv, padding, textColor = '#4a5268' } = props // ... rest of the implementation } }
35-47: 🛠️ Refactor suggestion
Add input validation in handleKeyDown.
The keyboard event handler should validate the input more thoroughly.
Add input validation:
- async handleKeyDown(ctx) { + async handleKeyDown(ctx: KeyboardEventContext) { const { e, row, column, makeCellEditable } = ctx if (column.readonly) return const columnObj = column.columnObj - if (e.key.length === 1 && columnObj.title) { + // Validate that the key is printable and not a control character + if (e.key.length === 1 && /^[\x20-\x7E]$/.test(e.key) && columnObj.title) { row.row[columnObj.title] = '' makeCellEditable(row, column) return true } return false },📝 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.async handleKeyDown(ctx: KeyboardEventContext) { const { e, row, column, makeCellEditable } = ctx if (column.readonly) return const columnObj = column.columnObj // Validate that the key is printable and not a control character if (e.key.length === 1 && /^[\x20-\x7E]$/.test(e.key) && columnObj.title) { row.row[columnObj.title] = '' makeCellEditable(row, column) return true } return false },packages/nc-gui/components/smartsheet/grid/canvas/cells/Currency.ts (1)
7-35: 🛠️ Refactor suggestion
Optimize value validation and remove redundant checks.
The code has multiple redundant value checks.
Simplify the validation:
- if (!isValidValue(value) || isNaN(value)) { + if (!isValidValue(value) || isNaN(value) || !value) { return { x, y, } } - const currencyMeta = { currency_locale: 'en-US', currency_code: 'USD', ...(parseProp(column?.meta) || {}), } - let formattedValue = '' try { formattedValue = new Intl.NumberFormat(currencyMeta.currency_locale || 'en-US', { style: 'currency', currency: currencyMeta.currency_code || 'USD', }).format(value) } catch (e) { formattedValue = value.toString() } - - if (!value) { - return { - x, - y, - } - }📝 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 (!isValidValue(value) || isNaN(value) || !value) { return { x, y, } } const currencyMeta = { currency_locale: 'en-US', currency_code: 'USD', ...(parseProp(column?.meta) || {}), } let formattedValue = '' try { formattedValue = new Intl.NumberFormat(currencyMeta.currency_locale || 'en-US', { style: 'currency', currency: currencyMeta.currency_code || 'USD', }).format(value) } catch (e) { formattedValue = value.toString() }packages/nc-gui/components/smartsheet/grid/canvas/cells/Decimal.ts (1)
11-20: 💡 Verification agent
🧩 Analysis chain
Consider adding validation for extreme decimal values.
The number validation and formatting logic should handle edge cases like Infinity, -Infinity, and very large/small decimals that might affect precision.
🌐 Web query:
What is the maximum safe decimal precision in JavaScript?💡 Result:
JavaScript's maximum safe decimal precision is approximately 15 to 17 significant digits due to its use of double-precision floating-point numbers (IEEE 754 standard). Here’s a breakdown:
Key Points
Mantissa Precision:
- JavaScript stores numbers in a 64-bit binary format with:
- 1 sign bit
- 11 exponent bits
- 52 mantissa bits (for fractional precision)[8][9].
- The mantissa provides ~15–17 significant decimal digits of precision[8][9]. For example:
console.log(0.123456789012345); // Accurate (15 digits) console.log(0.12345678901234567); // Rounded (17+ digits)Safe Integer Range:
- Integers between
-(2^53 - 1)(-9007199254740991) and2^53 - 1(9007199254740991) are "safe" (Number.MAX_SAFE_INTEGER)[1][4][6]. Beyond this range, integers lose precision:console.log(9007199254740992 === 9007199254740993); // true[1][4]Decimal Limitations:
- Decimals with more than 16–17 significant digits (integer + fractional parts) lose accuracy[2][5][8]:
console.log(0.1 + 0.2); // 0.30000000000000004[5] console.log(99999999999.0123 + 0.00231432423); // Rounded[5]Workarounds:
- Use
BigIntfor integers beyondMAX_SAFE_INTEGER[7].- For high-precision decimals, libraries like
decimal.jsorbig.jsare recommended[5].Summary
JavaScript’s number type reliably handles up to 15–17 significant digits for both integers and decimals due to its binary storage limitations. Beyond this range, rounding errors occur, necessitating specialized libraries for precise calculations[5][8][9].
Citations:
- 1: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Number/MAX_SAFE_INTEGER
- 2: https://www.outsystems.com/forums/discussion/27345/unable-to-convert-to-the-javascript-number/
- 3: https://esdiscuss.org/topic/number-max-safe-integer-incorrect
- 4: https://www.tektutorialshub.com/javascript/javascript-number-min-max-safe-values/
- 5: https://avioconsulting.com/blog/overcoming-javascript-numeric-precision-issues/
- 6: https://www.progress.com/blogs/limitations-number-type-javascript
- 7: https://javascript.plainenglish.io/going-big-with-javascript-numbers-71616cac8e44
- 8: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Number
- 9: https://codemia.io/knowledge-hub/path/what_is_javascripts_highest_integer_value_that_a_number_can_go_to_without_losing_precision
Action Required: Add Finite-Value Validation for Extreme Decimals
The current logic only checks for non-null and non-NaN values, meaning edge cases like Infinity, -Infinity, or numbers with precision beyond JavaScript’s ~15–17 significant digits aren’t explicitly handled. Consider updating the validation to use
Number.isFinite(Number(value))(or an equivalent check) to ensure that only finite numbers are processed, and potentially add guard clauses for values exceeding the safe precision range.
- File:
packages/nc-gui/components/smartsheet/grid/canvas/cells/Decimal.ts- Location: Lines 11–20
- Action: Modify the validation logic to catch extreme values (e.g., by replacing or supplementing the current
!isNaN(Number(value))check withNumber.isFinite(Number(value))).packages/nc-gui/components/cell/PhoneNumber/Editor.vue (1)
43-50:
⚠️ Potential issuePrevent potential data loss during unmount.
The validation in
onBeforeUnmountcould lead to data loss if the phone number is invalid. Consider validating before unmount but preserving the invalid value.onBeforeUnmount(() => { - if (parseProp(column.value.meta)?.validate && localState.value && !isMobilePhone(localState.value)) { + if (parseProp(column.value.meta)?.validate && localState.value) { + if (!isMobilePhone(localState.value)) { message.error(t('msg.invalidPhoneNumber')) - localState.value = undefined return + } } localState.value = props.modelValue })📝 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.onBeforeUnmount(() => { if (parseProp(column.value.meta)?.validate && localState.value) { if (!isMobilePhone(localState.value)) { message.error(t('msg.invalidPhoneNumber')) return } } localState.value = props.modelValue })packages/nc-gui/components/smartsheet/grid/canvas/cells/Email.ts (1)
75-78:
⚠️ Potential issueAdd security measures for opening email links.
Using
window.opendirectly with user input could pose security risks. Consider sanitizing the email address and addingnoopenerfor security.- window.open(`mailto:${text}`, '_blank') + const sanitizedEmail = encodeURIComponent(text.trim()) 10000 + window.open(`mailto:${sanitizedEmail}`, '_blank', 'noopener')📝 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 (isBoxHovered({ x, y, width: xOffset - x, height: yOffset - y }, mousePosition)) { const sanitizedEmail = encodeURIComponent(text.trim()) window.open(`mailto:${sanitizedEmail}`, '_blank', 'noopener') return true }packages/nc-gui/components/smartsheet/grid/canvas/cells/PhoneNumber.ts (1)
75-78:
⚠️ Potential issueAdd security measures for opening phone links.
Similar to email links, phone numbers should be sanitized before opening.
- window.open(`tel:${text}`, '_blank') + const sanitizedPhone = encodeURIComponent(text.replace(/\s+/g, '')) + window.open(`tel:${sanitizedPhone}`, '_blank', 'noopener')📝 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 (isBoxHovered({ x, y, width: xOffset - x, height: yOffset - y }, mousePosition)) { const sanitizedPhone = encodeURIComponent(text.replace(/\s+/g, '')) window.open(`tel:${sanitizedPhone}`, '_blank', 'noopener') return true }packages/nc-gui/components/smartsheet/grid/canvas/cells/Links.ts (1)
21-42: 🛠️ Refactor suggestion
Cache text measurements for better performance.
Text measurements are performed twice. Consider caching the measurements to avoid redundant calculations.
+ const textMeasurements = renderSingleLineText(ctx, { + x: x + padding, + y, + text, + maxWidth: width - padding * 2 - 20, + fontFamily: '500 13px Manrope', + fillStyle: 'rgb(67, 81, 232)', + height, + }) - const { y: textYOffset, width: textWidth } = renderSingleLineText(ctx, {...}) + const { y: textYOffset, width: textWidth } = textMeasurements const isHoverOverText = isBoxHovered({...}, mousePosition) - const { x: xOffset, y: yOffset } = renderSingleLineText(ctx, {...}) + const { x: xOffset, y: yOffset } = renderSingleLineText(ctx, { + ...textMeasurements, + underline: isHoverOverText, + })Committable suggestion skipped: line range outside the PR's diff.
packages/nc-gui/components/smartsheet/grid/canvas/cells/GeoData.ts (1)
10-12: 🛠️ Refactor suggestion
Add input validation for geographical coordinates.
The latitude/longitude parsing lacks validation. Invalid coordinate values could pass through silently.
- const [latitude, longitude] = (value || '').split(';') - const isLocationSet = !!(latitude && longitude) + const [latitude, longitude] = (value || '').split(';') + const isValidCoordinate = (coord: string) => { + const num = parseFloat(coord) + return !isNaN(num) && isFinite(num) + } + const isLocationSet = !!(latitude && longitude && isValidCoordinate(latitude) && isValidCoordinate(longitude))📝 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 [latitude, longitude] = (value || '').split(';') const isValidCoordinate = (coord: string) => { const num = parseFloat(coord) return !isNaN(num) && isFinite(num) } const isLocationSet = !!(latitude && longitude && isValidCoordinate(latitude) && isValidCoordinate(longitude)) const displayText = isLocationSet ? `${latitude}; ${longitude}` : 'Set location'packages/nc-gui/components/smartsheet/grid/canvas/cells/SingleSelect.ts (1)
53-74: 🛠️ Refactor suggestion
Optimize canvas context creation in handleHover.
Creating a new canvas context on every hover is inefficient. Consider reusing a shared context.
+const sharedContext = document.createElement('canvas').getContext('2d') + async handleHover({ getCellPosition, column, row, value, mousePosition }) { const { x, y, width } = getCellPosition(column, row.rowMeta.rowIndex!) const { tryShowTooltip, hideTooltip } = useTooltipStore() hideTooltip() const padding = 10 const maxWidth = width - padding * 2 - tagPadding * 2 - const ctx = document.createElement('canvas').getContext('2d') - if (!ctx) return - ctx.font = `${column.pv ? 600 : 500} 13px Manrope` + if (!sharedContext) return + sharedContext.font = `${column.pv ? 600 : 500} 13px Manrope`Committable suggestion skipped: line range outside the PR's diff.
packages/nc-gui/components/smartsheet/grid/canvas/cells/Time.ts (1)
59-70: 🛠️ Refactor suggestion
Optimize time parsing logic to reduce unnecessary operations.
The current implementation attempts multiple time parsing formats sequentially. This can be optimized by using a more targeted approach.
- let time = dayjs(value) - if (!time.isValid()) { - time = dayjs(value, 'HH:mm:ss') - } - if (!time.isValid()) { - time = dayjs(`1999-01-01 ${value}`) - } + const formats = ['HH:mm:ss', 'HH:mm'] + let time + for (const format of formats) { + time = dayjs(value, format) + if (time.isValid()) break + } + if (!time?.isValid()) { + time = dayjs(`1999-01-01 ${value}`) + }📝 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 (value) { canvasContext.font = '500 13px Manrope' - let time = dayjs(value) - if (!time.isValid()) { - time = dayjs(value, 'HH:mm:ss') - } - if (!time.isValid()) { - time = dayjs(`1999-01-01 ${value}`) - } + const formats = ['HH:mm:ss', 'HH:mm'] + let time + for (const format of formats) { + time = dayjs(value, format) + if (time.isValid()) break + } + if (!time?.isValid()) { + time = dayjs(`1999-01-01 ${value}`) + } if (time.isValid()) { text = time.format(timeFormat) } }packages/nc-gui/components/smartsheet/grid/canvas/cells/Date.ts (1)
23-27:
⚠️ Potential issueAdd validation for date string format.
The date parsing logic assumes the input string is in a valid format. Consider adding explicit format validation to prevent invalid date displays.
- const date = dayjs(/^\d+$/.test(value) ? +value : value, defaultDateFormat) + const date = dayjs( + /^\d+$/.test(value) + ? +value + : (typeof value === 'string' && value.match(/^\d{4}-\d{2}-\d{2}$/) + ? value + : null), + defaultDateFormat + )📝 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 date = dayjs( /^\d+$/.test(value) ? +value : (typeof value === 'string' && value.match(/^\d{4}-\d{2}-\d{2}$/) ? value : null), defaultDateFormat ) if (date.isValid()) { formattedDate = date.format(dateFormat) } }packages/nc-gui/components/smartsheet/grid/canvas/cells/Json.ts (1)
34-35: 🛠️ Refactor suggestion
Add JSON validation and formatting.
The JSON stringification is done without validation or formatting. Consider adding proper validation and formatting for better readability.
- const text = typeof value === 'string' ? value : JSON.stringify(value) + const text = typeof value === 'string' + ? (isValidJson(value) ? JSON.stringify(JSON.parse(value), null, 2) : value) + : JSON.stringify(value, null, 2) + function isValidJson(str: string): boolean { + try { + JSON.parse(str) + return true + } catch (e) { + return false + } + }📝 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 text = typeof value === 'string' ? (isValidJson(value) ? JSON.stringify(JSON.parse(value), null, 2) : value) : JSON.stringify(value, null, 2) function isValidJson(str: string): boolean { try { JSON.parse(str) return true } catch (e) { return false } }packages/nc-gui/components/cell/Decimal/Editor.vue (1)
72-76: 🛠️ Refactor suggestion
Add error handling for focus operations.
The focus operation might fail in certain browser conditions. Consider adding error handling.
onMounted(() => { if (isCanvasInjected && !isExpandedFormOpen.value && !isEditColumn.value && !isForm.value) { - inputRef.value?.focus() + try { + inputRef.value?.focus() + } catch (e) { + console.warn('Failed to focus input:', e) + } } })📝 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.onMounted(() => { if (isCanvasInjected && !isExpandedFormOpen.value && !isEditColumn.value && !isForm.value) { try { inputRef.value?.focus() } catch (e) { console.warn('Failed to focus input:', e) } } })packages/nc-gui/components/smartsheet/grid/canvas/cells/LTAR/BelongsTo.ts (1)
112-128: 🛠️ Refactor suggestion
Add error handling to click handler.
The click handler should include error handling for edge cases where
cellRenderStoreorrowIndexmight be undefined.async handleClick({ row, column, getCellPosition, mousePosition, makeCellEditable, cellRenderStore, selected }) { + if (!row?.rowMeta?.rowIndex) { + console.warn('Invalid row data in BelongsToCellRenderer click handler') + return false + } + const rowIndex = row.rowMeta.rowIndex! const { x, y, width } = getCellPosition(column, rowIndex) const size = 14 try { if (isBoxHovered({ x: x + width - 26, y: y + 8, height: size, width: size }, mousePosition)) { makeCellEditable(rowIndex, column) return true } if (!cellRenderStore?.x || !selected) return false if (isBoxHovered({ x: cellRenderStore.x + 2, y: y + 8, height: size, width: size }, mousePosition)) { makeCellEditable(rowIndex, column) return true } return false + } catch (error) { + console.error('Error in BelongsToCellRenderer click handler:', error) + return false + } }📝 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.async handleClick({ row, column, getCellPosition, mousePosition, makeCellEditable, cellRenderStore, selected }) { if (!row?.rowMeta?.rowIndex) { console.warn('Invalid row data in BelongsToCellRenderer click handler') return false } const rowIndex = row.rowMeta.rowIndex! const { x, y, width } = getCellPosition(column, rowIndex) const size = 14 try { if (isBoxHovered({ x: x + width - 26, y: y + 8, height: size, width: size }, mousePosition)) { makeCellEditable(rowIndex, column) return true } if (!cellRenderStore?.x || !selected) return false if (isBoxHovered({ x: cellRenderStore.x + 2, y: y + 8, height: size, width: size }, mousePosition)) { makeCellEditable(rowIndex, column) return true } return false } catch (error) { console.error('Error in BelongsToCellRenderer click handler:', error) return false } },packages/nc-gui/components/smartsheet/grid/canvas/cells/Url.ts (1)
17-17:
⚠️ Potential issueEnhance URL validation and sanitization.
The current URL handling has potential security issues:
- URL validation could be more robust
- URLs are opened without proper sanitization
+const sanitizeUrl = (url: string): string => { + try { + const parsed = new URL(url) + return ['http:', 'https:'].includes(parsed.protocol) ? url : '' + } catch { + return '' + } +} + -const isValid = text && isValidURL(text) +const isValid = text && sanitizeUrl(text) // In handleClick: -window.open(/^https?:\/\//.test(text) ? text : `https://${text}`, '_blank') +const sanitized = sanitizeUrl(/^https?:\/\//.test(text) ? text : `https://${text}`) +if (sanitized) { + window.open(sanitized, '_blank', 'noopener,noreferrer') +}Also applies to: 133-133
packages/nc-gui/components/smartsheet/grid/canvas/cells/LTAR/OneToOne.ts (2)
54-77: 🛠️ Refactor suggestion
Improve error handling for cell rendering.
The code silently returns if
returnData?.xis undefined, which might hide rendering issues.Add error handling and logging:
returnData = cellRenderer({ ...props, value: cellValue, column: ooColumn, width: cellWidth, relatedColObj: undefined, relatedTableMeta: undefined, readonly: true, height: rowHeightInPx['1']!, padding: 10, textColor: themeV3Colors.brand['500'], tag: { renderAsTag: true, tagBgColor: themeV3Colors.brand['50'], tagHeight: 24, }, meta: relatedTableMeta, x: x + 4, y: y + (rowHeightInPx['1'] === height ? 0 : 2), }) Object.assign(cellRenderStore, returnData) -if (!returnData?.x) return +if (!returnData?.x) { + console.warn('Cell rendering failed: returnData.x is undefined', { cellValue, ooColumn }) + 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.returnData = cellRenderer({ ...props, value: cellValue, column: ooColumn, width: cellWidth, relatedColObj: undefined, relatedTableMeta: undefined, readonly: true, height: rowHeightInPx['1']!, padding: 10, textColor: themeV3Colors.brand['500'], tag: { renderAsTag: true, tagBgColor: themeV3Colors.brand['50'], tagHeight: 24, }, meta: relatedTableMeta, x: x + 4, y: y + (rowHeightInPx['1'] === height ? 0 : 2), }) Object.assign(cellRenderStore, returnData) if (!returnData?.x) { console.warn('Cell rendering failed: returnData.x is undefined', { cellValue, ooColumn }) return }
40-47:
⚠️ Potential issueAdd type checking for value object.
The code assumes
valueis an object with specific properties but lacks type checking, which could lead to runtime errors.Add type checking before accessing object properties:
if (isValidValue(value)) { const cellWidth = width - (isBoxHovered({ x, y, width, height }, mousePosition) ? (hasValue ? 16 : 14) : 0) const cellValue = - value && !Array.isArray(value) && typeof value === 'object' + value && !Array.isArray(value) && typeof value === 'object' && (relatedTableDisplayValueProp in value || relatedTableDisplayValuePropId in value) ? value[relatedTableDisplayValueProp] ?? value[relatedTableDisplayValuePropId] : value📝 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 (isValidValue(value)) { const cellWidth = width - (isBoxHovered({ x, y, width, height }, mousePosition) ? (hasValue ? 16 : 14) : 0) const cellValue = value && !Array.isArray(value) && typeof value === 'object' && (relatedTableDisplayValueProp in value || relatedTableDisplayValuePropId in value) ? value[relatedTableDisplayValueProp] ?? value[relatedTableDisplayValuePropId] : valuepackages/nc-gui/components/cell/Percent/index.vue (1)
57-62: 🛠️ Refactor suggestion
Improve type safety for column meta parsing.
The code assumes parseProp returns an object with specific properties but lacks type checking.
Add type checking and interface:
+interface ColumnMeta { + is_progress?: boolean + [key: string]: any +} const percentMeta = computed(() => { + const meta = parseProp(column.value?.meta) as ColumnMeta return { - ...parseProp(column.value?.meta), - is_progress: isUnderLookup ? false : parseProp(column.value?.meta).is_progress ?? false, + ...meta, + is_progress: isUnderLookup.value ? false : meta.is_progress ?? false, } })📝 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.interface ColumnMeta { is_progress?: boolean [key: string]: any } const percentMeta = computed(() => { const meta = parseProp(column.value?.meta) as ColumnMeta return { ...meta, is_progress: isUnderLookup.value ? false : meta.is_progress ?? false, } })packages/nc-gui/components/cell/GeoData.vue (1)
52-72: 🛠️ Refactor suggestion
Consider adding error messages for geolocation failures.
The geolocation error handling only logs to console, which doesn't provide feedback to users when location access fails.
Add user-friendly error notifications:
const onError: PositionErrorCallback = (err: GeolocationPositionError) => { - console.error(`ERROR(${err.code}): ${err.message}`) + const errorMessages = { + 1: 'Location access denied. Please enable location services.', + 2: 'Location unavailable. Please try again.', + 3: 'Location request timed out. Please try again.' + } + message.error(errorMessages[err.code] || err.message) isLoading.value = false }📝 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 => { isLoading.value = true const onSuccess: PositionCallback = (position: GeolocationPosition) => { const crd = position.coords formState.latitude = `${crd.latitude}` formState.longitude = `${crd.longitude}` isLoading.value = false } const onError: PositionErrorCallback = (err: GeolocationPositionError) => { const errorMessages = { 1: 'Location access denied. Please enable location services.', 2: 'Location unavailable. Please try again.', 3: 'Location request timed out. Please try again.' } message.error(errorMessages[err.code] || err.message) isLoading.value = false } const options = { enableHighAccuracy: true, timeout: 20000, maximumAge: 2000, } navigator.geolocation.getCurrentPosition(onSuccess, onError, options) }packages/nc-gui/components/smartsheet/Cell.vue (1)
91-98: 🛠️ Refactor suggestion
Consider adding cleanup for save timer.
The save timer should be cleaned up when the component is unmounted.
onBeforeUnmount(() => { if (!isCanvasInjected) return if (currentRow.value.oldRow?.[column.value.title] === currentRow.value.row?.[column.value.title]) return currentRow.value.rowMeta.changed = false emitSave() + if (saveTimer) { + clearTimeout(saveTimer) + } })📝 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 isCanvasInjected = inject(IsCanvasInjectionInj, false) onBeforeUnmount(() => { if (!isCanvasInjected) return if (currentRow.value.oldRow?.[column.value.title] === currentRow.value.row?.[column.value.title]) return currentRow.value.rowMeta.changed = false emitSave() if (saveTimer) { clearTimeout(saveTimer) } })
|
Uffizzi Preview |
Change Summary
Fixes Associated Performance issues with Grid View
Change type