diff --git a/docs/validation-table-scroll-issue.md b/docs/validation-table-scroll-issue.md index c8969f2..8177a61 100644 --- a/docs/validation-table-scroll-issue.md +++ b/docs/validation-table-scroll-issue.md @@ -203,6 +203,313 @@ We removed scroll position management code from: Result: This did not fix the issue either. +### 8. Simple Scroll Position Management with Event Listeners + +```typescript +// Create a ref to store scroll position +const scrollPosition = useRef({ left: 0, top: 0 }); +const tableContainerRef = useRef(null); + +// Save scroll position when scrolling +const handleScroll = useCallback(() => { + if (tableContainerRef.current) { + scrollPosition.current = { + left: tableContainerRef.current.scrollLeft, + top: tableContainerRef.current.scrollTop + }; + } +}, []); + +// Add scroll listener +useEffect(() => { + const container = tableContainerRef.current; + if (container) { + container.addEventListener('scroll', handleScroll); + return () => container.removeEventListener('scroll', handleScroll); + } +}, [handleScroll]); + +// Restore scroll position after data changes +useLayoutEffect(() => { + const container = tableContainerRef.current; + if (container) { + const { left, top } = scrollPosition.current; + if (left > 0 || top > 0) { + container.scrollLeft = left; + container.scrollTop = top; + } + } +}, [data]); +``` + +Result: Still did not maintain scroll position during updates. + +### 9. Memoized Scroll Container Component + +```typescript +// Create a stable scroll container that won't re-render with the table +const ScrollContainer = React.memo(({ children }: { children: React.ReactNode }) => { + const containerRef = useRef(null); + const scrollPosition = useRef({ left: 0, top: 0 }); + + const handleScroll = useCallback(() => { + if (containerRef.current) { + scrollPosition.current = { + left: containerRef.current.scrollLeft, + top: containerRef.current.scrollTop + }; + } + }, []); + + useEffect(() => { + const container = containerRef.current; + if (container) { + // Set initial scroll position if it exists + if (scrollPosition.current.left > 0 || scrollPosition.current.top > 0) { + container.scrollLeft = scrollPosition.current.left; + container.scrollTop = scrollPosition.current.top; + } + container.addEventListener('scroll', handleScroll); + return () => container.removeEventListener('scroll', handleScroll); + } + }, [handleScroll]); + + return ( +
+ {children} +
+ ); +}); +``` + +Result: Still did not maintain scroll position during updates, even with a memoized container. + +### 10. Using TanStack Table State Management + +```typescript +// Track scroll state in the table instance +const [scrollState, setScrollState] = useState({ scrollLeft: 0, scrollTop: 0 }); + +const table = useReactTable({ + data, + columns, + getCoreRowModel: getCoreRowModel(), + state: { + rowSelection, + // Include scroll position in table state + scrollLeft: scrollState.scrollLeft, + scrollTop: scrollState.scrollTop + }, + onStateChange: (updater) => { + if (typeof updater === 'function') { + const newState = updater({ + rowSelection, + scrollLeft: scrollState.scrollLeft, + scrollTop: scrollState.scrollTop + }); + if ('scrollLeft' in newState || 'scrollTop' in newState) { + setScrollState({ + scrollLeft: newState.scrollLeft ?? scrollState.scrollLeft, + scrollTop: newState.scrollTop ?? scrollState.scrollTop + }); + } + } + } +}); + +// Handle scroll events +const handleScroll = useCallback((event: React.UIEvent) => { + const target = event.target as HTMLDivElement; + setScrollState({ + scrollLeft: target.scrollLeft, + scrollTop: target.scrollTop + }); +}, []); + +// Restore scroll position after updates +useLayoutEffect(() => { + if (tableContainerRef.current) { + tableContainerRef.current.scrollLeft = scrollState.scrollLeft; + tableContainerRef.current.scrollTop = scrollState.scrollTop; + } +}, [data, scrollState]); +``` + +Result: Still did not maintain scroll position during updates, even with table state management. + +### 11. Using CSS Sticky Positioning + +```typescript +return ( +
+ + + + {table.getFlatHeaders().map((header) => ( + + {/* Header content */} + + ))} + + + + {/* Table body content */} + +
+
+); +``` + +Result: Still did not maintain scroll position during updates, even with native CSS scrolling. + +### 12. Optimized Memoization with Object.is + +```typescript +// Memoize data structures to prevent unnecessary re-renders +const memoizedData = useMemo(() => data, [data]); +const memoizedValidationErrors = useMemo(() => validationErrors, [validationErrors]); +const memoizedValidatingCells = useMemo(() => validatingCells, [validatingCells]); +const memoizedItemNumbers = useMemo(() => itemNumbers, [itemNumbers]); + +// Use Object.is for more efficient comparisons +export default React.memo(ValidationTable, (prev, next) => { + if (!Object.is(prev.data.length, next.data.length)) return false; + + if (prev.validationErrors.size !== next.validationErrors.size) return false; + for (const [key, value] of prev.validationErrors) { + if (!next.validationErrors.has(key)) return false; + if (!Object.is(value, next.validationErrors.get(key))) return false; + } + + // ... more optimized comparisons ... +}); +``` + +Result: Caused the page to crash with "TypeError: undefined has no properties" in the MemoizedCell component. + +### 13. Simplified Component Structure + +```typescript +const ValidationTable = ({ + data, + fields, + rowSelection, + setRowSelection, + updateRow, + validationErrors, + // ... other props +}) => { + const tableContainerRef = useRef(null); + const lastScrollPosition = useRef({ left: 0, top: 0 }); + + // Simple scroll position management + const handleScroll = useCallback(() => { + if (tableContainerRef.current) { + lastScrollPosition.current = { + left: tableContainerRef.current.scrollLeft, + top: tableContainerRef.current.scrollTop + }; + } + }, []); + + useEffect(() => { + const container = tableContainerRef.current; + if (container) { + container.addEventListener('scroll', handleScroll); + return () => container.removeEventListener('scroll', handleScroll); + } + }, [handleScroll]); + + useLayoutEffect(() => { + const container = tableContainerRef.current; + if (container) { + const { left, top } = lastScrollPosition.current; + if (left > 0 || top > 0) { + requestAnimationFrame(() => { + if (container) { + container.scrollLeft = left; + container.scrollTop = top; + } + }); + } + } + }, [data]); + + return ( +
+ + {/* ... table content ... */} + + {table.getRowModel().rows.map((row) => ( + + {/* ... row content ... */} + + ))} + +
+
+ ); +}; +``` + +Result: Still did not maintain scroll position during updates. However, this implementation restored the subtle red highlight on rows with validation errors, which is a useful visual indicator that should be preserved in future attempts. + +### 14. Portal-Based Scroll Container + +```typescript +// Create a stable container outside of React's control +const createStableContainer = () => { + const containerId = 'validation-table-container'; + let container = document.getElementById(containerId); + + if (!container) { + container = document.createElement('div'); + container.id = containerId; + container.className = 'overflow-auto'; + container.style.maxHeight = 'calc(100vh - 300px)'; + document.body.appendChild(container); + } + + return container; +}; + +const ValidationTable = ({...props}) => { + const [container] = useState(createStableContainer); + const [mounted, setMounted] = useState(false); + + useEffect(() => { + setMounted(true); + return () => { + if (container && container.parentNode) { + container.parentNode.removeChild(container); + } + }; + }, [container]); + + // ... table configuration ... + + return createPortal(content, container); +}; +``` + +Result: The table contents failed to render at all. The portal-based approach to maintain scroll position by moving the scroll container outside of React's control was unsuccessful. + ## Current Understanding The scroll position issue appears to be complex and likely stems from multiple factors: @@ -214,14 +521,18 @@ The scroll position issue appears to be complex and likely stems from multiple f ## Next Steps to Consider -Potential approaches that haven't been tried yet: +At this point, we have tried multiple approaches without success: +1. Various scroll position management techniques +2. Memoization and optimization strategies +3. Different component structures +4. Portal-based rendering -1. Implement a completely separate scroll container that exists outside of React's rendering cycle -2. Use a third-party virtualized table library that handles scroll position natively -3. Restructure the component hierarchy to minimize re-renders -4. Use the React DevTools profiler to identify which components are causing re-renders -5. Consider simplifying the data structure to reduce the complexity of renders +Given that none of these approaches have fully resolved the issue, it may be worth: +1. Investigating if there are any parent component updates forcing re-renders +2. Profiling the application to identify the exact timing of scroll position resets +3. Considering if the current table implementation could be simplified +4. Exploring if the data update patterns could be optimized to reduce re-renders ## Conclusion -This issue has proven particularly challenging to resolve. The current ValidationTable implementation struggles with scroll position preservation despite multiple different approaches. A more fundamental restructuring of the component or its rendering approach may be necessary. \ No newline at end of file +The scroll position issue has proven resistant to multiple solution attempts. Each approach has either failed to maintain scroll position, introduced new issues, or in some cases (like the portal-based approach) prevented the table from rendering entirely. A deeper investigation into the component lifecycle and data flow may be necessary to identify the root cause. \ No newline at end of file diff --git a/inventory/src/lib/react-spreadsheet-import/src/steps/ValidationStepNew/components/ValidationContainer.tsx b/inventory/src/lib/react-spreadsheet-import/src/steps/ValidationStepNew/components/ValidationContainer.tsx index 7149e6d..2a0babe 100644 --- a/inventory/src/lib/react-spreadsheet-import/src/steps/ValidationStepNew/components/ValidationContainer.tsx +++ b/inventory/src/lib/react-spreadsheet-import/src/steps/ValidationStepNew/components/ValidationContainer.tsx @@ -1,4 +1,4 @@ -import React, { useState, useEffect, useRef, useCallback, useMemo } from 'react' +import React, { useState, useEffect, useRef, useCallback, useMemo, useLayoutEffect } from 'react' import { useValidationState, Props } from '../hooks/useValidationState' import ValidationTable from './ValidationTable' import { Button } from '@/components/ui/button' @@ -878,6 +878,53 @@ const ValidationContainer = ({ copyDown ]); + // Add scroll container ref at the container level + const scrollContainerRef = useRef(null); + const lastScrollPosition = useRef({ left: 0, top: 0 }); + const isScrolling = useRef(false); + + // Save scroll position when scrolling + const handleScroll = useCallback(() => { + if (!isScrolling.current && scrollContainerRef.current) { + isScrolling.current = true; + lastScrollPosition.current = { + left: scrollContainerRef.current.scrollLeft, + top: scrollContainerRef.current.scrollTop + }; + requestAnimationFrame(() => { + isScrolling.current = false; + }); + } + }, []); + + // Add scroll event listener + useEffect(() => { + const container = scrollContainerRef.current; + if (container) { + container.addEventListener('scroll', handleScroll, { passive: true }); + return () => container.removeEventListener('scroll', handleScroll); + } + }, [handleScroll]); + + // Restore scroll position after data updates + useLayoutEffect(() => { + const container = scrollContainerRef.current; + if (container) { + const { left, top } = lastScrollPosition.current; + if (left > 0 || top > 0) { + requestAnimationFrame(() => { + if (container) { + container.scrollTo({ + left, + top, + behavior: 'auto' + }); + } + }); + } + } + }, [filteredData]); + return (
@@ -937,8 +984,20 @@ const ValidationContainer = ({ {/* Main table section */}
-
- {renderValidationTable} +
+
+
{/* Force container to be at least as wide as content */} + {renderValidationTable} +
+
diff --git a/inventory/src/lib/react-spreadsheet-import/src/steps/ValidationStepNew/components/ValidationTable.tsx b/inventory/src/lib/react-spreadsheet-import/src/steps/ValidationStepNew/components/ValidationTable.tsx index b1a9e48..9e0a441 100644 --- a/inventory/src/lib/react-spreadsheet-import/src/steps/ValidationStepNew/components/ValidationTable.tsx +++ b/inventory/src/lib/react-spreadsheet-import/src/steps/ValidationStepNew/components/ValidationTable.tsx @@ -1,4 +1,4 @@ -import React, { useMemo, useRef, useEffect, useLayoutEffect, useCallback } from 'react' +import React, { useMemo } from 'react' import { useReactTable, getCoreRowModel, @@ -49,233 +49,6 @@ interface ValidationTableProps { [key: string]: any } -// Make Field type mutable for internal use -type MutableField = { - -readonly [K in keyof Field]: Field[K] extends readonly (infer U)[] ? U[] : Field[K] -} - -interface MemoizedCellProps { - field: MutableField - value: any - rowIndex: number - updateRow: (rowIndex: number, key: string, value: any) => void - validationErrors: Map> - validatingCells: Set - itemNumbers: Map - width: number - copyDown: (rowIndex: number, key: string) => void -} - -// Memoized cell component that only updates when its specific data changes -const MemoizedCell = React.memo(({ - field, - value, - rowIndex, - updateRow, - validationErrors, - validatingCells, - itemNumbers, - width, - copyDown -}: MemoizedCellProps) => { - const rowErrors = validationErrors.get(rowIndex) || {}; - const fieldErrors = rowErrors[String(field.key)] || []; - const isValidating = validatingCells.has(`${rowIndex}-${field.key}`); - - // Only compute options when needed for select/multi-select fields - const options = useMemo(() => { - if (field.fieldType.type === 'select' || field.fieldType.type === 'multi-select') { - return Array.from((field.fieldType as any).options || []); - } - return []; - }, [field.fieldType]); - - // Memoize the onChange handler to prevent unnecessary re-renders - const handleChange = useCallback((newValue: any) => { - updateRow(rowIndex, field.key, newValue); - }, [updateRow, rowIndex, field.key]); - - // Memoize the copyDown handler - const handleCopyDown = useCallback(() => { - copyDown(rowIndex, field.key); - }, [copyDown, rowIndex, field.key]); - - return ( - - ); -}, (prev, next) => { - const fieldKey = String(prev.field.key); - - // For item_number fields, only update if the item number or validation state changes - if (fieldKey === 'item_number') { - const prevItemNumber = prev.itemNumbers.get(prev.rowIndex); - const nextItemNumber = next.itemNumbers.get(next.rowIndex); - const prevValidating = prev.validatingCells.has(`${prev.rowIndex}-item_number`); - const nextValidating = next.validatingCells.has(`${next.rowIndex}-item_number`); - - return ( - prevItemNumber === nextItemNumber && - prevValidating === nextValidating && - prev.value === next.value - ); - } - - // For UPC fields, only update if the value or validation state changes - if (fieldKey === 'upc' || fieldKey === 'barcode') { - const prevValidating = prev.validatingCells.has(`${prev.rowIndex}-${fieldKey}`); - const nextValidating = next.validatingCells.has(`${next.rowIndex}-${fieldKey}`); - - return ( - prev.value === next.value && - prevValidating === nextValidating - ); - } - - // For other fields, only update if the value or errors change - const prevErrors = prev.validationErrors.get(prev.rowIndex)?.[fieldKey]; - const nextErrors = next.validationErrors.get(next.rowIndex)?.[fieldKey]; - - // For select/multi-select fields, also check if options changed - if (prev.field.fieldType.type === 'select' || prev.field.fieldType.type === 'multi-select') { - const prevOptions = (prev.field.fieldType as any).options; - const nextOptions = (next.field.fieldType as any).options; - - // If options length changed, we need to re-render - if (prevOptions?.length !== nextOptions?.length) { - return false; - } - } - - return ( - prev.value === next.value && - JSON.stringify(prevErrors) === JSON.stringify(nextErrors) - ); -}); - -MemoizedCell.displayName = 'MemoizedCell'; - -interface MemoizedRowProps { - row: RowData; - fields: readonly { - readonly label: string; - readonly key: string; - readonly description?: string; - readonly alternateMatches?: readonly string[]; - readonly validations?: readonly any[]; - readonly fieldType: any; - readonly example?: string; - readonly width?: number; - readonly disabled?: boolean; - }[]; - updateRow: (rowIndex: number, key: string, value: any) => void; - validationErrors: Map>; - validatingCells: Set; - itemNumbers: Map; - options?: { [key: string]: any[] }; - rowIndex: number; - isSelected: boolean; - copyDown: (rowIndex: number, key: string) => void; -} - -const MemoizedRow = React.memo(({ - row, - fields, - updateRow, - validationErrors, - validatingCells, - itemNumbers, - options = {}, - rowIndex, - isSelected, - copyDown -}) => { - return ( - - {fields.map((field) => { - if (field.disabled) return null; - - const fieldWidth = field.width || ( - field.fieldType.type === "checkbox" ? 80 : - field.fieldType.type === "select" ? 150 : - field.fieldType.type === "multi-select" ? 200 : - (field.fieldType.type === "input" || field.fieldType.type === "multi-input") && - (field.fieldType as any).multiline ? 300 : - 150 - ); - - const isValidating = validatingCells.has(`${rowIndex}-${field.key}`); - - // Memoize the copyDown handler - const handleCopyDown = () => { - copyDown(rowIndex, field.key); - }; - - return ( - } - value={row[field.key]} - onChange={(value) => updateRow(rowIndex, field.key, value)} - errors={validationErrors.get(rowIndex)?.[String(field.key)] || []} - isValidating={isValidating} - fieldKey={String(field.key)} - options={options[String(field.key)] || []} - width={fieldWidth} - rowIndex={rowIndex} - itemNumber={itemNumbers.get(rowIndex)} - copyDown={handleCopyDown} - /> - ); - })} - - ); -}, (prev, next) => { - // Compare row data - const prevRowStr = JSON.stringify(prev.row); - const nextRowStr = JSON.stringify(next.row); - if (prevRowStr !== nextRowStr) return false; - - // Compare validation errors for this row - const prevErrors = prev.validationErrors.get(prev.rowIndex); - const nextErrors = next.validationErrors.get(next.rowIndex); - if (JSON.stringify(prevErrors) !== JSON.stringify(nextErrors)) return false; - - // Compare validation state for this row's cells - const prevValidatingCells = Array.from(prev.validatingCells) - .filter(key => key.startsWith(`${prev.rowIndex}-`)); - const nextValidatingCells = Array.from(next.validatingCells) - .filter(key => key.startsWith(`${next.rowIndex}-`)); - if (JSON.stringify(prevValidatingCells) !== JSON.stringify(nextValidatingCells)) return false; - - // Compare item numbers for this row - const prevItemNumber = prev.itemNumbers.get(prev.rowIndex); - const nextItemNumber = next.itemNumbers.get(next.rowIndex); - if (prevItemNumber !== nextItemNumber) return false; - - // Compare selection state - if (prev.isSelected !== next.isSelected) return false; - - return true; -}); - -MemoizedRow.displayName = 'MemoizedRow'; - const ValidationTable = ({ data, fields, @@ -293,67 +66,6 @@ const ValidationTable = ({ copyDown }: ValidationTableProps) => { const { translations } = useRsi(); - - // Create a global scroll position manager - const scrollManager = useRef({ - windowX: 0, - windowY: 0, - containerLeft: 0, - containerTop: 0, - isScrolling: false, - - // Save current scroll positions - save: function() { - this.windowX = window.scrollX; - this.windowY = window.scrollY; - if (tableContainerRef.current) { - this.containerLeft = tableContainerRef.current.scrollLeft; - this.containerTop = tableContainerRef.current.scrollTop; - } - }, - - // Restore saved scroll positions - restore: function() { - if (this.isScrolling) return; - this.isScrolling = true; - - // Restore window scroll - window.scrollTo(this.windowX, this.windowY); - - // Restore container scroll - if (tableContainerRef.current) { - tableContainerRef.current.scrollLeft = this.containerLeft; - tableContainerRef.current.scrollTop = this.containerTop; - } - - // Reset flag after a short delay - setTimeout(() => { - this.isScrolling = false; - }, 50); - } - }); - - // Table container ref - const tableContainerRef = useRef(null); - - // Save scroll position before any potential re-render - useLayoutEffect(() => { - scrollManager.current.save(); - - // Restore after render - return () => { - requestAnimationFrame(() => { - scrollManager.current.restore(); - }); - }; - }); - - // Also restore on data changes - useEffect(() => { - requestAnimationFrame(() => { - scrollManager.current.restore(); - }); - }, [data]); // Memoize the selection column const selectionColumn = useMemo((): ColumnDef, any> => ({ @@ -391,8 +103,6 @@ const ValidationTable = ({ const defaultBrand = row.original.company || undefined; const rowIndex = data.findIndex(r => r === row.original); - console.log(`Template cell for row ${row.id}, index ${rowIndex}`); - return ( {isLoadingTemplates ? ( @@ -433,25 +143,21 @@ const ValidationTable = ({ accessorKey: String(field.key), header: field.label || String(field.key), size: fieldWidth, - cell: ({ row }) => { - const cellUpdateRow = (rowIndex: number, key: string, value: any) => { - updateRow(rowIndex, key as T, value); - }; - - return ( - } - value={row.original[field.key as keyof typeof row.original]} - rowIndex={row.index} - updateRow={cellUpdateRow} - validationErrors={validationErrors} - validatingCells={validatingCells} - itemNumbers={itemNumbers} - width={fieldWidth} - copyDown={(rowIndex, key) => copyDown(rowIndex, key as T)} - /> - ); - } + cell: ({ row }) => ( + updateRow(row.index, field.key, value)} + errors={validationErrors.get(row.index)?.[String(field.key)] || []} + isValidating={validatingCells.has(`${row.index}-${field.key}`)} + fieldKey={String(field.key)} + options={(field.fieldType as any).options || []} + itemNumber={itemNumbers.get(row.index)} + width={fieldWidth} + rowIndex={row.index} + copyDown={() => copyDown(row.index, field.key)} + /> + ) }; }).filter((col): col is ColumnDef, any> => col !== null), [fields, validationErrors, validatingCells, itemNumbers, updateRow, copyDown]); @@ -466,24 +172,16 @@ const ValidationTable = ({ enableRowSelection: true, onRowSelectionChange: setRowSelection, getRowId: (row) => { - // Prefer __index if available (likely a UUID) if (row.__index) return row.__index; - - // Fall back to position in array const index = data.indexOf(row); return String(index); } }); - // Log selection changes for debugging - useEffect(() => { - const selectedCount = Object.values(rowSelection).filter(v => v === true).length; - const selectedIds = Object.entries(rowSelection) - .filter(([_, selected]) => selected === true) - .map(([id, _]) => id); - - console.log(`Row selection updated: ${selectedCount} rows selected, IDs:`, selectedIds); - }, [rowSelection]); + // Calculate total table width for stable horizontal scrolling + const totalWidth = useMemo(() => { + return columns.reduce((total, col) => total + (col.size || 0), 0); + }, [columns]); // Don't render if no data if (data.length === 0) { @@ -499,69 +197,63 @@ const ValidationTable = ({ } return ( -
- - - - {table.getFlatHeaders().map((header) => ( - + + + {table.getFlatHeaders().map((header) => ( + + {flexRender(header.column.columnDef.header, header.getContext())} + + ))} + + + + {table.getRowModel().rows.map((row) => ( + + {row.getVisibleCells().map((cell) => ( + - {header.id === 'select' ? ( -
- table.toggleAllPageRowsSelected(!!value)} - aria-label="Select all" - /> -
- ) : ( - flexRender(header.column.columnDef.header, header.getContext()) - )} -
+ {flexRender(cell.column.columnDef.cell, cell.getContext())} + ))}
-
- - {table.getRowModel().rows.map((row) => ( - - {row.getVisibleCells().map((cell) => ( - - {flexRender(cell.column.columnDef.cell, cell.getContext())} - - ))} - - ))} - -
-
+ ))} + + ); }; export default React.memo(ValidationTable, (prev, next) => { - // Deep compare data - if (JSON.stringify(prev.data) !== JSON.stringify(next.data)) return false; - - // Compare validation errors - if (JSON.stringify(Array.from(prev.validationErrors.entries())) !== - JSON.stringify(Array.from(next.validationErrors.entries()))) return false; - - // Compare filters - if (JSON.stringify(prev.filters) !== JSON.stringify(next.filters)) return false; - - // Compare row selection - if (JSON.stringify(prev.rowSelection) !== JSON.stringify(next.rowSelection)) return false; - + // Add more specific checks to prevent unnecessary re-renders + if (prev.data.length !== next.data.length) return false; + if (prev.validationErrors.size !== next.validationErrors.size) return false; + if (prev.filters?.showErrorsOnly !== next.filters?.showErrorsOnly) return false; + if (prev.validatingCells.size !== next.validatingCells.size) return false; + if (prev.itemNumbers.size !== next.itemNumbers.size) return false; + if (prev.templates.length !== next.templates.length) return false; return true; }); \ No newline at end of file