feat: implement table widget infinite scroll with dynamic height#39646
feat: implement table widget infinite scroll with dynamic height#39646jacquesikot merged 18 commits intoreleasefrom
Conversation
…o feat/implement-infinite-scroll-with-dynamic-height
WalkthroughThis pull request introduces a new Cypress test suite to validate the dynamic row height behavior of the Table Widget. In addition, it adds a new Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant BaseVirtualList
participant useRowHeightMeasurement
participant useColumnVariableHeight
participant DOM
User->>BaseVirtualList: Initiates scroll event
BaseVirtualList->>useRowHeightMeasurement: Measure row height for visible rows
BaseVirtualList->>useColumnVariableHeight: Identify columns with variable height
useRowHeightMeasurement->>DOM: Query for cell dimensions and computed styles
DOM-->>useRowHeightMeasurement: Return measurement values
useRowHeightMeasurement->>BaseVirtualList: Return calculated row height
BaseVirtualList-->>User: Render updated row with dynamic height
Assessment against linked issues
Possibly related PRs
Suggested reviewers
Poem
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
app/client/src/widgets/TableWidgetV2/component/TableBody/Row.tsx (1)
48-125: Refinement on error handling & optional chaining
The measuring logic is well-structured but silently catches all errors, which can obscure unexpected issues. Consider logging or handling them more specifically.
Meanwhile, applying an optional chain at line 124 aligns with safer property access:- rowNeedsMeasurement.current && (rowNeedsMeasurement.current[index] = false); + rowNeedsMeasurement.current?.[index] = false;🧰 Tools
🪛 Biome (1.9.4)
[error] 124-124: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
app/client/src/widgets/TableWidgetV2/component/TableBody/Row.tsx(3 hunks)app/client/src/widgets/TableWidgetV2/component/TableBody/VirtualList.tsx(4 hunks)app/client/src/widgets/TableWidgetV2/component/TableBody/index.tsx(5 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
app/client/src/widgets/TableWidgetV2/component/TableBody/Row.tsx
[error] 124-124: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: client-unit-tests / client-unit-tests
- GitHub Check: client-lint / client-lint
- GitHub Check: client-check-cyclic-deps / check-cyclic-dependencies
- GitHub Check: client-build / client-build
- GitHub Check: client-prettier / prettier-check
🔇 Additional comments (20)
app/client/src/widgets/TableWidgetV2/component/TableBody/Row.tsx (7)
2-2: Looks good.
This import is appropriate for the new hooks usage and poses no issues.
8-12: Constants import
Neat import of constants for table sizing and styling.
21-27: Clear interface definition
This interface addition keeps the cell props explicit, aiding readability.
31-31: Prop destructuring
Extracting props here promotes clarity.
37-37: Retrieving listRef
Grabbing the list reference from context is a clean approach for virtualization.
41-42: Context-based row measurements
Acquiring rowHeights and rowNeedsMeasurement from context effectively enables dynamic row resizing.
160-160: Ref assignment
Binding rowRef to the row DOM element is crucial for accurate height measurement.app/client/src/widgets/TableWidgetV2/component/TableBody/VirtualList.tsx (6)
2-9: Necessary imports
Bringing in the relevant types and hooks for variable-size virtualization.
11-13: BodyContext usage
Importing BodyContext aligns the virtual list with the dynamic row measurement paradigm.
42-42: Ref type update
Switching to React.Ref is consistent with the variable sizing approach.
57-57: Destructuring from context
Extracting listRef and rowHeights sets up the foundation for dynamic sizing logic.
59-89: Combining refs & computing item size
Merging the infiniteLoaderListRef and listRef is convenient, and the getItemSize callback nicely accommodates variable row heights.
92-110: VariableSizeList integration
Leveraging dynamic row sizing with an estimated item size is a solid choice for better rendering performance.app/client/src/widgets/TableWidgetV2/component/TableBody/index.tsx (7)
1-1: Imports
No issues; this suits the new hooks and effect usage.
7-7: Importing VariableSizeList
Needed to enable variable row height logic.
36-38: Enhancement to BodyContextType
Adding rowHeights, rowNeedsMeasurement, and listRef defines the backbone for measuring rows.
53-55: Default context initialization
Providing default ref objects ensures consistent behavior throughout.
142-145: Creating ref state
The useRef hooks for listRef, rowHeights, and rowNeedsMeasurement help centralize row dimension tracking.
147-149: Row measurement reset
Reinitializing rowNeedsMeasurement on row changes is a clean way to handle newly rendered rows.
182-185: Context provider additions
Populating rowHeights, rowNeedsMeasurement, and listRef in BodyContext completes the dynamic resizing infrastructure.
|
🔴🔴🔴 Cyclic Dependency Check: This PR has increased the number of cyclic dependencies by 1, when compared with the release branch. Refer this document to identify the cyclic dependencies introduced by this PR. You can view the dependency diff in the run log. Look for the check-cyclic-dependencies job in the run. |
…asurement - Added a new hook `useColumnVariableHeight` to manage dynamic row heights - Introduced a force update mechanism to trigger row height recalculation - Renamed `FixedInfiniteVirtualList` to `VariableHeightInfiniteVirtualList` for clarity - Improved error handling and type safety in row height measurement - Simplified cell wrapping detection logic
|
🔴🔴🔴 Cyclic Dependency Check: This PR has increased the number of cyclic dependencies by 1, when compared with the release branch. Refer this document to identify the cyclic dependencies introduced by this PR. You can view the dependency diff in the run log. Look for the check-cyclic-dependencies job in the run. |
- Enhanced `useColumnVariableHeight` hook to track row data changes - Added a helper function `hasRowDataChanged` to detect data modifications - Updated `Row` component to support HTML column type detection - Introduced more robust row height recalculation mechanism
|
🔴🔴🔴 Cyclic Dependency Check: This PR has increased the number of cyclic dependencies by 1, when compared with the release branch. Refer this document to identify the cyclic dependencies introduced by this PR. You can view the dependency diff in the run log. Look for the check-cyclic-dependencies job in the run. |
…ation to fix cyclic dependency
|
🔴🔴🔴 Cyclic Dependency Check: This PR has increased the number of cyclic dependencies by 1, when compared with the release branch. Refer this document to identify the cyclic dependencies introduced by this PR. You can view the dependency diff in the run log. Look for the check-cyclic-dependencies job in the run. |
jacquesikot
changed the title
github-actions
bot
added
Query Widgets & IDE Pod
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (11)
app/client/src/widgets/TableWidgetV2/component/TableBody/BaseVirtualList.tsx (2)
78-90: Consider updating useCallback dependency arrayThe
useCallbackhook dependency array includesrowHeights.currentwhich is a mutable ref value and won't trigger proper dependency updates.- [rowHeights.current, tableSizes.ROW_HEIGHT], + [tableSizes.ROW_HEIGHT], // rowHeights is a ref, so we don't need it in the deps array
80-87: Add error logging for better debuggingYour error handling silently falls back to default row height when an error occurs. Consider adding error logging to make debugging easier.
try { // Add a minimum height threshold to prevent rows from being too small const rowHeight = rowHeights.current?.[index] || tableSizes.ROW_HEIGHT; return Math.max(rowHeight, tableSizes.ROW_HEIGHT); } catch (error) { + console.error("Error calculating row height:", error); return tableSizes.ROW_HEIGHT; }app/client/src/widgets/TableWidgetV2/component/TableBody/useColumnVariableHeight.tsx (3)
55-75: Improve comparison for nested objectsThe current implementation of
hasRowDataChangedperforms shallow comparison, which won't detect changes in nested objects.function hasRowDataChanged( currentData: Record<string, unknown>, previousData: Record<string, unknown> | null, ): boolean { if (!previousData) return true; // Check if the keys are different const currentKeys = Object.keys(currentData); const previousKeys = Object.keys(previousData); if (currentKeys.length !== previousKeys.length) return true; // Check if any values have changed for the variable height columns for (const key of currentKeys) { - if (currentData[key] !== previousData[key]) { + if (JSON.stringify(currentData[key]) !== JSON.stringify(previousData[key])) { return true; } } return false; }
6-52: Add null/undefined check for columnsThe hook doesn't handle the case where columns might be null or undefined, which could cause runtime errors.
function useColumnVariableHeight( columns: ReactTableColumnProps[], row: ReactTableRowType<Record<string, unknown>>, ) { const variableHeightColumnsRef = useRef<number[]>([]); const previousRowDataRef = useRef<Record<string, unknown> | null>(null); + // Return empty array if columns are not defined + if (!columns || !Array.isArray(columns)) { + return []; + } // Identify columns that need variable height handling (wrapping or HTML) const columnsNeedingVariableHeight = columns .map((col, index) => { // Check for columns with wrapping enabled if (col.columnProperties?.allowCellWrapping) { return index; } // Check for with HTML type if (col.columnProperties?.columnType === ColumnTypes.HTML) { return index; } return -1; }) .filter((index) => index !== -1);
6-52: Optimize row data change detection for large datasetsThe hook checks all keys in row data rather than just those for variable height columns, which could be inefficient for large datasets.
Consider optimizing to only check relevant columns:
// Check if row data has changed const currentRowData = row?.original || {}; + + // Only check columns that need variable height + const relevantKeys = Object.keys(currentRowData).filter((key, idx) => + columnsNeedingVariableHeight.includes(idx)); + + const relevantCurrentData = relevantKeys.reduce((obj, key) => { + obj[key] = currentRowData[key]; + return obj; + }, {} as Record<string, unknown>); + + const relevantPreviousData = previousRowDataRef.current ? relevantKeys.reduce((obj, key) => { + obj[key] = previousRowDataRef.current?.[key]; + return obj; + }, {} as Record<string, unknown>) : null; const rowDataChanged = hasRowDataChanged( - currentRowData, - previousRowDataRef.current, + relevantCurrentData, + relevantPreviousData, );app/client/cypress/e2e/Regression/ClientSide/Widgets/TableV2/InfiniteScrollVariableHeightRows_spec.ts (2)
72-116: Avoid nested async operations with better structureThe test uses nested async operations which can lead to flaky tests. Consider restructuring to avoid unnecessary nesting.
- let currentRowHeight = 0; - cy.get(".t--widget-tablewidgetv2 .tbody .tr").each(($row) => { - cy.wrap($row) - .invoke("outerHeight") - .then((height) => { - currentRowHeight = Math.ceil(height!); - }); - }); + // Get the height of the first row before update + cy.get(".t--widget-tablewidgetv2 .tbody .tr") + .first() + .invoke("outerHeight") + .then((height) => { + const currentRowHeight = Math.ceil(height!); + // Update the table data + propPane.EnterJSContext("Table data", JSON.stringify(updatedTestData)); + // Verify height change + cy.get(".t--widget-tablewidgetv2 .tbody .tr") + .invoke("outerHeight") + .should("be.greaterThan", currentRowHeight); + });
103-115: Replace manual max height calculation with Cypress commandsThe manual calculation of max height can be replaced with Cypress commands for better readability and reliability.
- let maxHeight = 0; - cy.get(".t--widget-tablewidgetv2 .tbody .tr") - .each(($row, index) => { - cy.wrap($row) - .invoke("outerHeight") - .then((height) => { - if (height! > maxHeight) { - maxHeight = height!; - } - }); - }) - .then(() => { - expect(maxHeight).to.be.greaterThan(currentRowHeight); - }); + // Use Cypress to directly find the tallest row + cy.get(".t--widget-tablewidgetv2 .tbody .tr") + .then(($rows) => { + let maxHeight = 0; + $rows.each((_, row) => { + const height = Cypress.$(row).outerHeight(); + maxHeight = Math.max(maxHeight, height || 0); + }); + expect(maxHeight).to.be.greaterThan(currentRowHeight); + });app/client/src/widgets/TableWidgetV2/component/TableBody/Row.tsx (4)
53-56: Consider memoizing dependency to prevent unnecessary renders.The current implementation will trigger re-renders whenever
wrappingColumnschanges. Consider memoizing this value or implementing shouldComponentUpdate logic to prevent unnecessary re-renders.
57-87: Extract cell type identification logic into a separate function.The logic for identifying cells with wrapping and HTML content could be extracted into a separate function to improve readability and maintainability.
- if (row?.cells && Array.isArray(row.cells)) { - row.cells.forEach((cell, index: number) => { - const typedCell = cell as unknown as CellWithColumnProps; - - if (typedCell?.column?.columnProperties?.allowCellWrapping) { - cellIndexesWithAllowCellWrapping.push(index); - } - - if ( - typedCell?.column?.columnProperties?.columnType === ColumnTypes.HTML - ) { - cellIndexesWithHTMLCell.push(index); - } - }); - } + if (row?.cells && Array.isArray(row.cells)) { + const identifyCellTypes = (cell: any, index: number) => { + const typedCell = cell as unknown as CellWithColumnProps; + + if (typedCell?.column?.columnProperties?.allowCellWrapping) { + cellIndexesWithAllowCellWrapping.push(index); + } + + if (typedCell?.column?.columnProperties?.columnType === ColumnTypes.HTML) { + cellIndexesWithHTMLCell.push(index); + } + }; + + row.cells.forEach(identifyCellTypes); + }
88-132: Extract height calculation logic into separate functions.The height calculation logic is complex and would benefit from being extracted into separate functions for better readability and maintainability.
- // Get all child elements - const children = element.children; - let normalCellHeight = 0; - let htmlCellHeight = 0; - - cellIndexesWithAllowCellWrapping.forEach((index: number) => { - const child = children[index] as HTMLElement; - const dynamicContent = child.querySelector( - ".t--table-cell-tooltip-target", - ); - - if (dynamicContent) { - const styles = window.getComputedStyle(dynamicContent); - - normalCellHeight += - (dynamicContent as HTMLElement).offsetHeight + - parseFloat(styles.marginTop) + - parseFloat(styles.marginBottom) + - parseFloat(styles.paddingTop) + - parseFloat(styles.paddingBottom); - } - }); - - cellIndexesWithHTMLCell.forEach((index: number) => { - const child = children[index] as HTMLElement; - const dynamicContent = child.querySelector( - '[data-testid="t--table-widget-v2-html-cell"]>span', - ); - - if (dynamicContent) { - const styles = window.getComputedStyle(dynamicContent); - - htmlCellHeight += - (dynamicContent as HTMLElement).offsetHeight + - parseFloat(styles.marginTop) + - parseFloat(styles.marginBottom) + - parseFloat(styles.paddingTop) * 2 + - parseFloat(styles.paddingBottom) * 2; - } - }); + // Get all child elements + const children = element.children; + + const calculateWrappingCellsHeight = () => { + let height = 0; + cellIndexesWithAllowCellWrapping.forEach((index: number) => { + const child = children[index] as HTMLElement; + const dynamicContent = child.querySelector( + ".t--table-cell-tooltip-target", + ); + + if (dynamicContent) { + const styles = window.getComputedStyle(dynamicContent); + + height += + (dynamicContent as HTMLElement).offsetHeight + + parseFloat(styles.marginTop) + + parseFloat(styles.marginBottom) + + parseFloat(styles.paddingTop) + + parseFloat(styles.paddingBottom); + } + }); + return height; + }; + + const calculateHtmlCellsHeight = () => { + let height = 0; + cellIndexesWithHTMLCell.forEach((index: number) => { + const child = children[index] as HTMLElement; + const dynamicContent = child.querySelector( + '[data-testid="t--table-widget-v2-html-cell"]>span', + ); + + if (dynamicContent) { + const styles = window.getComputedStyle(dynamicContent); + + height += + (dynamicContent as HTMLElement).offsetHeight + + parseFloat(styles.marginTop) + + parseFloat(styles.marginBottom) + + parseFloat(styles.paddingTop) * 2 + + parseFloat(styles.paddingBottom) * 2; + } + }); + return height; + }; + + const normalCellHeight = calculateWrappingCellsHeight(); + const htmlCellHeight = calculateHtmlCellsHeight();
133-136: Use optional chaining as suggested by static analysis.Implement optional chaining for safer property access on line 135.
- listRef && listRef.current?.resetAfterIndex(index); + listRef?.current?.resetAfterIndex(index);🧰 Tools
🪛 Biome (1.9.4)
[error] 135-135: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (11)
app/client/cypress/e2e/Regression/ClientSide/Widgets/TableV2/InfiniteScrollVariableHeightRows_spec.ts(1 hunks)app/client/src/widgets/TableWidgetV2/component/StaticTable.tsx(1 hunks)app/client/src/widgets/TableWidgetV2/component/TableBody/BaseVirtualList.tsx(1 hunks)app/client/src/widgets/TableWidgetV2/component/TableBody/BodyContext.tsx(1 hunks)app/client/src/widgets/TableWidgetV2/component/TableBody/InifiniteScrollBody/index.tsx(2 hunks)app/client/src/widgets/TableWidgetV2/component/TableBody/Row.tsx(3 hunks)app/client/src/widgets/TableWidgetV2/component/TableBody/VirtualList.tsx(1 hunks)app/client/src/widgets/TableWidgetV2/component/TableBody/index.tsx(3 hunks)app/client/src/widgets/TableWidgetV2/component/TableBody/useColumnVariableHeight.tsx(1 hunks)app/client/src/widgets/TableWidgetV2/component/VirtualTable.tsx(1 hunks)app/client/src/widgets/TableWidgetV2/component/header/VirtualTableInnerElement.tsx(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- app/client/src/widgets/TableWidgetV2/component/header/VirtualTableInnerElement.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- app/client/src/widgets/TableWidgetV2/component/TableBody/index.tsx
🧰 Additional context used
📓 Path-based instructions (1)
`app/client/cypress/**/**.*`: Review the following e2e test ...
app/client/cypress/**/**.*: Review the following e2e test code written using the Cypress test library. Ensure that:
- Follow best practices for Cypress code and e2e automation.
- Avoid using cy.wait in code.
- Avoid using cy.pause in code.
- Avoid using agHelper.sleep().
- Use locator variables for locators and do not use plain strings.
- Use data-* attributes for selectors.
- Avoid Xpaths, Attributes and CSS path.
- Avoid selectors like .btn.submit or button[type=submit].
- Perform logins via API with LoginFromAPI.
- Perform logout via API with LogOutviaAPI.
- Perform signup via API with SignupFromAPI.
- Avoid using it.only.
- Avoid using after and aftereach in test cases.
- Use multiple assertions for expect statements.
- Avoid using strings for assertions.
- Do not use duplicate filenames even with different paths.
- Avoid using agHelper.Sleep, this.Sleep in any file in code.
app/client/cypress/e2e/Regression/ClientSide/Widgets/TableV2/InfiniteScrollVariableHeightRows_spec.ts
🪛 Biome (1.9.4)
app/client/src/widgets/TableWidgetV2/component/TableBody/Row.tsx
[error] 135-135: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: client-lint / client-lint
- GitHub Check: client-unit-tests / client-unit-tests
- GitHub Check: client-build / client-build
- GitHub Check: client-check-cyclic-deps / check-cyclic-dependencies
- GitHub Check: client-prettier / prettier-check
🔇 Additional comments (15)
app/client/src/widgets/TableWidgetV2/component/VirtualTable.tsx (1)
70-70: Added new props for dynamic row height supportThe new props
listRef,rowHeights, androwNeedsMeasurementprovide the necessary infrastructure for variable row height functionality in the TableBody component. These props will allow the table to measure and track row heights dynamically based on content.Also applies to: 77-78
app/client/src/widgets/TableWidgetV2/component/TableBody/BodyContext.tsx (1)
1-52: Well-structured context implementation for variable height managementThis new context properly encapsulates the state needed for variable height rows. The use of RefObject types for tracking row heights and measurement flags is appropriate, and importing VariableSizeList from react-window shows the intention to support dynamic row sizing.
app/client/src/widgets/TableWidgetV2/component/StaticTable.tsx (1)
87-87: Consistent implementation of row height props in StaticTableThe addition of
listRef,rowHeights, androwNeedsMeasurementprops maintains consistency with the VirtualTable implementation, ensuring that both table types support the variable height functionality.Also applies to: 93-94
app/client/src/widgets/TableWidgetV2/component/TableBody/InifiniteScrollBody/index.tsx (2)
8-8: Replaced fixed height list with variable height implementationSwapping
FixedInfiniteVirtualListforVariableHeightInfiniteVirtualListis the key change enabling dynamic row heights. This modification aligns perfectly with the PR objective of supporting variable height rows in the Table Widget.Also applies to: 49-49
49-59:Details
✅ Verification successful
Verify prop compatibility with the new component
The props being passed to
VariableHeightInfiniteVirtualListappear unchanged from those previously passed toFixedInfiniteVirtualList. Ensure that the new component handles these props correctly, especially considering its enhanced capability to manage variable heights.
🏁 Script executed:
#!/bin/bash # Check the implementation of VariableHeightInfiniteVirtualList to verify prop compatibility echo "Checking VirtualList implementation to verify prop types and handling..." cat app/client/src/widgets/TableWidgetV2/component/TableBody/VirtualList.tsxLength of output: 886
Prop Compatibility Verified
The implementation ofVariableHeightInfiniteVirtualListshows that it forwards all props—includingonItemsRenderedandinfiniteLoaderListRef—toBaseVirtualListwithout alteration. This confirms compatibility with the props previously passed toFixedInfiniteVirtualList.
- The new component is a simple wrapper around
BaseVirtualList, ensuring that any variable height handling is seamlessly integrated.- No additional alterations are required based on the verification.
app/client/src/widgets/TableWidgetV2/component/TableBody/BaseVirtualList.tsx (1)
47-113: Implementation looks goodThe BaseVirtualList component is well-structured with proper ref handling and memoization for performance optimization. The approach for calculating variable row heights is solid.
app/client/src/widgets/TableWidgetV2/component/TableBody/VirtualList.tsx (2)
7-11: Component renaming reflects functionality changeGood job renaming
FixedInfiniteVirtualListtoVariableHeightInfiniteVirtualListto better reflect its purpose with the new variable height functionality.
13-21: Clean separation of fixed vs variable list componentsThe separation of concerns between
FixedVirtualListandVariableHeightInfiniteVirtualListis well implemented, making the code more maintainable.app/client/cypress/e2e/Regression/ClientSide/Widgets/TableV2/InfiniteScrollVariableHeightRows_spec.ts (2)
10-44: Tests setup looks goodThe test setup using the feature flag intercept and appropriate test data with varying text lengths is well structured.
46-161: Comprehensive test casesThe tests cover all the key scenarios for variable height rows: fixed height baseline, dynamic height with wrapping, height updates on content change, reversion to fixed height, and HTML content handling.
app/client/src/widgets/TableWidgetV2/component/TableBody/Row.tsx (5)
2-14: Clean import organization for new functionality.The imports have been properly organized to support the new dynamic height measurement functionality. Good addition of the necessary hooks and constants.
23-30: Well-defined interface for column properties.The
CellWithColumnPropsinterface clearly defines the structure needed for handling cell wrapping and column types, which is essential for the dynamic height calculations.
33-52: Good setup of references and state for dynamic measurements.The component now properly uses refs and state to track row elements and handle dynamic updates based on wrapping columns.
171-171: Good addition of the ref attribute to the row element.The ref attribute is properly added to the row element, which is essential for the height measurement logic.
69-71:Details
❓ Verification inconclusive
Review the hardcoded index in cellIndexesWithHTMLCell array.
The array is initialized with
[0]which assumes the first cell is always an HTML cell. Is this intentional? If not, consider initializing as an empty array.
🏁 Script executed:
#!/bin/bash # Check if the first cell is always HTML in the table ast-grep --pattern 'cellIndexesWithHTMLCell: number[] = [0]'Length of output: 62
Action Required: Confirm the Intent Behind the Hardcoded Index
It’s not entirely clear from the code whether hardcoding
[0]forcellIndexesWithHTMLCellis intentional. Please verify if the first cell should always be an HTML cell; if so, consider adding a clarifying comment to document this design decision. Otherwise, initialize the array as empty to avoid accidental assumptions.
- Location:
app/client/src/widgets/TableWidgetV2/component/TableBody/Row.tsx(Lines 69-71)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
app/client/src/widgets/TableWidgetV2/component/VirtualTable/BaseVirtualList.tsx (1)
60-77: Consider simplifying ref handling logicThe combined ref handling is complex but necessary. However, it could be improved by abstracting the ref assignment logic into a separate utility function to make the code more maintainable.
- const combinedRef = (list: VariableSizeList | null) => { - // Handle infiniteLoaderListRef - if (infiniteLoaderListRef) { - if (typeof infiniteLoaderListRef === "function") { - infiniteLoaderListRef(list); - } else { - ( - infiniteLoaderListRef as React.MutableRefObject<VariableSizeList | null> - ).current = list; - } - } - - // Handle listRef - only if it's a mutable ref - if (listRef && "current" in listRef) { - (listRef as React.MutableRefObject<VariableSizeList | null>).current = - list; - } - }; + const combinedRef = (list: VariableSizeList | null) => { + // Utility function to update a ref + const updateRef = (ref: React.Ref<VariableSizeList> | null | undefined) => { + if (!ref) return; + if (typeof ref === "function") { + ref(list); + } else if ("current" in ref) { + (ref as React.MutableRefObject<VariableSizeList | null>).current = list; + } + }; + + updateRef(infiniteLoaderListRef); + updateRef(listRef); + };app/client/src/widgets/TableWidgetV2/component/VirtualTable/useRowHeightMeasurement.tsx (2)
76-116: DOM measurement could be more robustThe DOM measurement logic works but should be more defensive against potential layout changes. Consider adding null checks for each child and dynamicContent access.
cellIndexesWithAllowCellWrapping.forEach((index: number) => { const child = children[index] as HTMLElement; + if (!child) return; const dynamicContent = child.querySelector( ".t--table-cell-tooltip-target", ); - if (dynamicContent) { + if (dynamicContent instanceof HTMLElement) { const styles = window.getComputedStyle(dynamicContent); normalCellHeight += - (dynamicContent as HTMLElement).offsetHeight + + dynamicContent.offsetHeight + parseFloat(styles.marginTop) + parseFloat(styles.marginBottom) + parseFloat(styles.paddingTop) + parseFloat(styles.paddingBottom); } });
117-123: Use optional chaining for list resetFix the optional chaining issue flagged by static analysis.
- listRef && listRef.current?.resetAfterIndex(index); + listRef?.current?.resetAfterIndex(index);🧰 Tools
🪛 Biome (1.9.4)
[error] 123-123: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
app/client/src/widgets/TableWidgetV2/component/VirtualTable/useColumnVariableHeight.tsx (1)
48-66: hasRowDataChanged could be optimized for large datasetsThe implementation is correct but performs a shallow comparison on each property. For large datasets, consider implementing a more efficient equality check or using a memoization library.
function hasRowDataChanged( currentData: Record<string, unknown>, previousData: Record<string, unknown> | null, ): boolean { if (!previousData) return true; const currentKeys = Object.keys(currentData); const previousKeys = Object.keys(previousData); if (currentKeys.length !== previousKeys.length) return true; + // Fast path: Check if references are equal first + if (currentData === previousData) return false; + + // For large datasets, consider using a faster equality check or memoization for (const key of currentKeys) { if (currentData[key] !== previousData[key]) { return true; } } return false; }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (3)
app/client/src/widgets/TableWidgetV2/component/VirtualTable/BaseVirtualList.tsx(1 hunks)app/client/src/widgets/TableWidgetV2/component/VirtualTable/useColumnVariableHeight.tsx(1 hunks)app/client/src/widgets/TableWidgetV2/component/VirtualTable/useRowHeightMeasurement.tsx(1 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
app/client/src/widgets/TableWidgetV2/component/VirtualTable/useRowHeightMeasurement.tsx
[error] 123-123: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🔇 Additional comments (6)
app/client/src/widgets/TableWidgetV2/component/VirtualTable/BaseVirtualList.tsx (3)
16-34: Efficient implementation of rowRenderer with memoizationThe memoized rowRenderer implementation is efficient and correctly prevents unnecessary re-renders through the areEqual comparison function.
79-91: Robust row height calculation with fallbackThe getItemSize implementation correctly handles fallback to default row height when measurements aren't available, ensuring the UI doesn't break.
93-113: Well-structured VariableSizeList configurationThe VariableSizeList configuration properly accounts for headers and padding in height calculations, which is essential for accurate virtual rendering.
app/client/src/widgets/TableWidgetV2/component/VirtualTable/useRowHeightMeasurement.tsx (2)
37-56: Effective early returns for performance optimizationThe implementation properly uses early returns to avoid unnecessary calculations, which is good for performance.
57-75: Comprehensive cell type detectionThe detection of cells needing variable height is well-implemented, checking both for wrapping and HTML content types.
app/client/src/widgets/TableWidgetV2/component/VirtualTable/useColumnVariableHeight.tsx (1)
6-46: Efficient column variable height detectionThe useColumnVariableHeight hook efficiently identifies and tracks columns needing variable height handling. The implementation correctly caches results and only updates when necessary.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
app/client/src/widgets/TableWidgetV2/component/VirtualTable/useRowHeightMeasurement.tsx (1)
27-133: Consider optional chaining for safer assignments
At line 123, adopting optional chaining may simplify and clarify the index assignment:- rowNeedsMeasurement.current && (rowNeedsMeasurement.current[index] = false); + rowNeedsMeasurement.current?.[index] = false;🧰 Tools
🪛 Biome (1.9.4)
[error] 123-123: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (5)
app/client/src/widgets/TableWidgetV2/component/TableBodyCoreComponents/Row.tsx(2 hunks)app/client/src/widgets/TableWidgetV2/component/TableBodyCoreComponents/VirtualList.tsx(2 hunks)app/client/src/widgets/TableWidgetV2/component/VirtualTable/BaseVirtualList.tsx(1 hunks)app/client/src/widgets/TableWidgetV2/component/VirtualTable/InifiniteScrollBody/index.tsx(2 hunks)app/client/src/widgets/TableWidgetV2/component/VirtualTable/useRowHeightMeasurement.tsx(1 hunks)
🧰 Additional context used
🧬 Code Definitions (4)
app/client/src/widgets/TableWidgetV2/component/VirtualTable/InifiniteScrollBody/index.tsx (1)
app/client/src/widgets/TableWidgetV2/component/TableBodyCoreComponents/VirtualList.tsx (1) (1)
VariableInfiniteVirtualList(24:28)
app/client/src/widgets/TableWidgetV2/component/VirtualTable/BaseVirtualList.tsx (1)
app/client/src/widgets/TableWidgetV2/component/TableBodyCoreComponents/Row.tsx (1) (1)
Row(21:131)
app/client/src/widgets/TableWidgetV2/component/TableBodyCoreComponents/VirtualList.tsx (1)
app/client/src/widgets/TableWidgetV2/component/VirtualTable/BaseVirtualList.tsx (1) (1)
BaseVirtualListProps(45:55)
app/client/src/widgets/TableWidgetV2/component/TableBodyCoreComponents/Row.tsx (1)
app/client/src/widgets/TableWidgetV2/component/VirtualTable/useRowHeightMeasurement.tsx (1) (1)
useRowHeightMeasurement(27:133)
🪛 Biome (1.9.4)
app/client/src/widgets/TableWidgetV2/component/VirtualTable/useRowHeightMeasurement.tsx
[error] 123-123: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: perform-test / client-build / client-build
- GitHub Check: perform-test / server-build / server-unit-tests
- GitHub Check: perform-test / rts-build / build
- GitHub Check: client-prettier / prettier-check
- GitHub Check: client-lint / client-lint
- GitHub Check: client-check-cyclic-deps / check-cyclic-dependencies
- GitHub Check: client-build / client-build
- GitHub Check: client-unit-tests / client-unit-tests
🔇 Additional comments (14)
app/client/src/widgets/TableWidgetV2/component/VirtualTable/InifiniteScrollBody/index.tsx (2)
5-5: Introduce VariableInfiniteVirtualList for dynamic heights
Swapping fromFixedInfiniteVirtualListtoVariableInfiniteVirtualListaligns perfectly with the goal of supporting variable row heights.
43-53:Details
❓ Verification inconclusive
Confirm correct prop passing in VariableInfiniteVirtualList
Everything appears consistent with the dynamic row-height approach. Verify that props likeonItemsRenderedandinfiniteLoaderListRefare fully tested with infinite scrolling.
🏁 Script executed:
#!/bin/bash # Verify occurrences of VariableInfiniteVirtualList usage across the codebase rg "VariableInfiniteVirtualList"Length of output: 641
Action: Verify Infinite Scroll Prop Passing and Testing Coverage
The usage of
VariableInfiniteVirtualListin this file appears to align with the dynamic row-height approach. However, please ensure that key props such asonItemsRenderedandinfiniteLoaderListRefare thoroughly covered in your infinite scrolling test cases. In particular:
- Confirm that tests effectively simulate infinite scroll scenarios to validate that
onItemsRenderedis invoked at the proper times.- Verify that
infiniteLoaderListRefis properly handled within the component, as observed in its usage across the codebase (see import fromTableBodyCoreComponents/VirtualList).app/client/src/widgets/TableWidgetV2/component/VirtualTable/BaseVirtualList.tsx (4)
1-19: Imports and extended props look good
Imports correctly support variable-sized virtualization with react-window. No issues found.
21-43: rowRenderer function is well-structured
Appropriately handles both valid row data and empty rows. Meshes nicely with the Row component.
45-55: BaseVirtualListProps definition is comprehensive
Thoroughly covers references, props, and item details needed for variable-size rendering.
57-130: BaseVirtualList component is properly implemented
Smart use ofuseCallbackfor dynamic row sizing and a careful default fallback enhance clarity and performance.app/client/src/widgets/TableWidgetV2/component/VirtualTable/useRowHeightMeasurement.tsx (1)
1-25: Initial interfaces are well-defined
They neatly outline the data structures and references needed for measuring row heights.app/client/src/widgets/TableWidgetV2/component/TableBodyCoreComponents/VirtualList.tsx (2)
3-6: Correct type imports for variable-size usage
Switching toVariableSizeListtypes is correct for dynamic row heights. No issues here.Also applies to: 16-16
24-25: VariableInfiniteVirtualList meets dynamic row needs
Renaming from fixed to variable infinite list is well-structured. The memoized component usage looks good.app/client/src/widgets/TableWidgetV2/component/TableBodyCoreComponents/Row.tsx (5)
2-4: Imports properly updated for variable row height functionality.The new imports support the implementation of dynamic row height calculations for the Table Widget's infinite scroll functionality.
Also applies to: 8-9
16-18: Added well-typed props for row height management.These new properties provide the necessary references for the variable height implementation, allowing row heights to be measured, stored, and used by the virtual list component.
22-24: Added ref and state management for row measurement.The rowRef provides access to the DOM element for measurement, while forceUpdate creates a mechanism to trigger re-renders when needed. The isInfiniteScrollEnabled flag is appropriately extracted from context.
Also applies to: 29-29
38-58: Implemented row height measurement with appropriate hooks and effects.The implementation correctly:
- Uses useColumnVariableHeight to identify columns needing variable height
- Uses useRowHeightMeasurement to calculate actual row heights
- Implements an effect to mark rows for remeasurement when wrapping columns change
The dependency array in the useEffect correctly includes wrappingColumns, ensuring updates happen when needed.
66-66: Connected ref to row DOM element for measurement.Adding the ref to rowProps ensures the DOM element can be accessed for height measurement.
jacquesikot
added
ok-to-test
jacquesikot
added
ok-to-test
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
app/client/src/widgets/TableWidgetV2/component/VirtualTable/useRowHeightMeasurement.tsx (3)
58-58: Consider clarifying why index 0 is pre-populated.The array initialization with
[0]isn't immediately obvious. Consider adding a comment explaining why index 0 is included by default incellIndexesWithHTMLCell.- const cellIndexesWithHTMLCell: number[] = [0]; + // Include index 0 by default as it often contains HTML content (e.g., row number or selection cell) + const cellIndexesWithHTMLCell: number[] = [0];
81-97: Extract repeated height calculation logic into a helper function.Both the wrapping cell and HTML cell height calculations follow a similar pattern. Consider extracting this to reduce duplication.
+ const calculateElementHeight = (element: HTMLElement, selector: string) => { + const dynamicContent = element.querySelector(selector); + if (!dynamicContent) return 0; + + const styles = window.getComputedStyle(dynamicContent); + return (dynamicContent as HTMLElement).offsetHeight + + parseFloat(styles.marginTop) + + parseFloat(styles.marginBottom) + + parseFloat(styles.paddingTop) + + parseFloat(styles.paddingBottom); + }; + cellIndexesWithAllowCellWrapping.forEach((index: number) => { const child = children[index] as HTMLElement; - const dynamicContent = child.querySelector( - ".t--table-cell-tooltip-target", - ); - - if (dynamicContent) { - const styles = window.getComputedStyle(dynamicContent); - - normalCellHeight += - (dynamicContent as HTMLElement).offsetHeight + - parseFloat(styles.marginTop) + - parseFloat(styles.marginBottom) + - parseFloat(styles.paddingTop) + - parseFloat(styles.paddingBottom); - } + normalCellHeight += calculateElementHeight(child, ".t--table-cell-tooltip-target"); }); cellIndexesWithHTMLCell.forEach((index: number) => { const child = children[index] as HTMLElement; - const dynamicContent = child.querySelector( - '[data-testid="t--table-widget-v2-html-cell"]>span', - ); - - if (dynamicContent) { - const styles = window.getComputedStyle(dynamicContent); - - htmlCellHeight += - (dynamicContent as HTMLElement).offsetHeight + - parseFloat(styles.marginTop) + - parseFloat(styles.marginBottom) + - parseFloat(styles.paddingTop) * 2 + - parseFloat(styles.paddingBottom) * 2; - } + // Note: HTML cells need padding multiplied by 2 + const height = calculateElementHeight(child, '[data-testid="t--table-widget-v2-html-cell"]>span'); + if (height > 0) { + const styles = window.getComputedStyle(child.querySelector('[data-testid="t--table-widget-v2-html-cell"]>span')); + htmlCellHeight += height + parseFloat(styles.paddingTop) + parseFloat(styles.paddingBottom); + } });Also applies to: 99-115
123-123: Use optional chaining for listRef access.As suggested by the static analysis tool, using optional chaining can make the code more concise and safer.
- listRef && listRef.current?.resetAfterIndex(index); + listRef?.current?.resetAfterIndex(index);🧰 Tools
🪛 Biome (1.9.4)
[error] 123-123: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
app/client/src/widgets/TableWidgetV2/component/TableBodyCoreComponents/Row.tsx (3)
51-57: Use optional chaining for safer property access.The current check could be simplified with optional chaining for safer property access.
- if (props.rowNeedsMeasurement && props.rowNeedsMeasurement.current) { + if (props.rowNeedsMeasurement?.current) { props.rowNeedsMeasurement.current[props.index] = true; }🧰 Tools
🪛 Biome (1.9.4)
[error] 52-52: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
51-57: Consider memoizing wrappingColumns to prevent unnecessary re-renders.The
useEffectis triggered on every change towrappingColumns, which could lead to frequent state updates and potentially unnecessary re-renders.- const wrappingColumns = useColumnVariableHeight(columns, props.row); + const wrappingColumns = React.useMemo( + () => useColumnVariableHeight(columns, props.row), + [columns, props.row] + );This might not be necessary if
useColumnVariableHeightis already memoized internally, but it's a good practice to consider.🧰 Tools
🪛 Biome (1.9.4)
[error] 52-52: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
22-23: Consider initializing forceUpdate with a callback function.Using a callback in the useState initialization is generally preferred for consistency.
- const [forceUpdate, setForceUpdate] = useState(0); + const [forceUpdate, setForceUpdate] = useState(() => 0);
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (2)
app/client/src/widgets/TableWidgetV2/component/TableBodyCoreComponents/Row.tsx(2 hunks)app/client/src/widgets/TableWidgetV2/component/VirtualTable/useRowHeightMeasurement.tsx(1 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
app/client/src/widgets/TableWidgetV2/component/TableBodyCoreComponents/Row.tsx
[error] 52-52: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
app/client/src/widgets/TableWidgetV2/component/VirtualTable/useRowHeightMeasurement.tsx
[error] 123-123: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: perform-test / rts-build / build
- GitHub Check: perform-test / client-build / client-build
- GitHub Check: perform-test / server-build / server-unit-tests
- GitHub Check: client-unit-tests / client-unit-tests
- GitHub Check: client-lint / client-lint
- GitHub Check: client-check-cyclic-deps / check-cyclic-dependencies
- GitHub Check: client-build / client-build
- GitHub Check: client-prettier / prettier-check
🔇 Additional comments (2)
app/client/src/widgets/TableWidgetV2/component/VirtualTable/useRowHeightMeasurement.tsx (1)
27-133: Well-implemented hook with good separation of concerns.The implementation effectively handles dynamic row height measurement for both wrapped text and HTML content. The early return conditions are well-structured, and the effect dependencies are correctly specified.
🧰 Tools
🪛 Biome (1.9.4)
[error] 123-123: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
app/client/src/widgets/TableWidgetV2/component/TableBodyCoreComponents/Row.tsx (1)
38-49: Good integration of the height measurement hook.The integration of
useRowHeightMeasurementwith the appropriate props looks good. This cleanly separates the measurement logic from the rendering component.
…o feat/implement-infinite-scroll-with-dynamic-height
rahulbarwal
left a comment
rahulbarwal
left a comment
There was a problem hiding this comment.
Does this PR also account for the possibility of multiple columns allowing cell wrapping and HTML content? In such cases, we should ensure that the maximum height is taken into consideration. I haven't explored this in detail, but please ensure that we handle this scenario appropriately.
app/client/src/widgets/TableWidgetV2/component/VirtualTable/useRowHeightMeasurement.tsx
Outdated
Show resolved
Hide resolved
app/client/src/widgets/TableWidgetV2/component/VirtualTable/useRowHeightMeasurement.tsx
Show resolved
Hide resolved
…o feat/implement-infinite-scroll-with-dynamic-height
Yes, this PR check for the highest height for all rows and uses that in the implementation |
- Introduced useMemo for prerequisite checks to optimize performance. - Added comprehensive documentation for the useRowHeightMeasurement hook, detailing its functionality and measurement process.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
app/client/src/widgets/TableWidgetV2/component/VirtualTable/useRowHeightMeasurement.tsx (5)
54-65: Use consistent memoization to improve performanceGood decomposition of logical conditions into readable variables. However,
isAlreadyCalculatedcould also benefit from memoization likehasPreRequisitesForInfiniteScroll.- const isAlreadyCalculated = - rowNeedsMeasurement && - rowNeedsMeasurement.current && - rowNeedsMeasurement.current[index] === false; + const isAlreadyCalculated = useMemo(() => { + return ( + rowNeedsMeasurement && + rowNeedsMeasurement.current && + rowNeedsMeasurement.current[index] === false + ); + }, [rowNeedsMeasurement, index]);🧰 Tools
🪛 Biome (1.9.4)
[error] 61-62: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
77-77: Remove redundant checkThis check is redundant since
hasRowDataAndUIElementis already checked at line 68.- if (!hasRowDataAndUIElement) return;
61-64: Apply optional chaining to simplify null checksThe static analysis tool correctly identified that optional chaining can simplify these null checks.
- const isAlreadyCalculated = - rowNeedsMeasurement && - rowNeedsMeasurement.current && - rowNeedsMeasurement.current[index] === false; + const isAlreadyCalculated = + rowNeedsMeasurement?.current?.[index] === false;🧰 Tools
🪛 Biome (1.9.4)
[error] 61-62: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
149-149: Apply optional chaining to simplify redundant checksUse optional chaining to simplify this line based on the static analysis suggestion.
- listRef?.current && listRef.current?.resetAfterIndex(index); + listRef?.current?.resetAfterIndex(index);🧰 Tools
🪛 Biome (1.9.4)
[error] 149-149: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
104-121: Extract cell height measurement into a helper functionThe logic for measuring cell heights is duplicated between normal cells and HTML cells. Consider extracting this logic into a reusable helper function.
+ const measureCellHeight = ( + element: HTMLElement, + selector: string, + extraPaddingMultiplier = 1 + ): number => { + const dynamicContent = element.querySelector(selector); + if (!dynamicContent) return 0; + + const styles = window.getComputedStyle(dynamicContent); + return ( + (dynamicContent as HTMLElement).offsetHeight + + parseFloat(styles.marginTop) + + parseFloat(styles.marginBottom) + + parseFloat(styles.paddingTop) * extraPaddingMultiplier + + parseFloat(styles.paddingBottom) * extraPaddingMultiplier + ); + }; cellIndexesWithAllowCellWrapping.forEach((index: number) => { const child = children[index] as HTMLElement; - const dynamicContent = child.querySelector( - ".t--table-cell-tooltip-target", - ); - - if (dynamicContent) { - const styles = window.getComputedStyle(dynamicContent); - - // Calculate total height including margins and padding - normalCellHeight += - (dynamicContent as HTMLElement).offsetHeight + - parseFloat(styles.marginTop) + - parseFloat(styles.marginBottom) + - parseFloat(styles.paddingTop) + - parseFloat(styles.paddingBottom); - } + normalCellHeight += measureCellHeight( + child, + ".t--table-cell-tooltip-target" + ); }); cellIndexesWithHTMLCell.forEach((index: number) => { const child = children[index] as HTMLElement; - const dynamicContent = child.querySelector( - '[data-testid="t--table-widget-v2-html-cell"]>span', - ); - - if (dynamicContent) { - const styles = window.getComputedStyle(dynamicContent); - - htmlCellHeight += - (dynamicContent as HTMLElement).offsetHeight + - parseFloat(styles.marginTop) + - parseFloat(styles.marginBottom) + - parseFloat(styles.paddingTop) * 2 + - parseFloat(styles.paddingBottom) * 2; - } + htmlCellHeight += measureCellHeight( + child, + '[data-testid="t--table-widget-v2-html-cell"]>span', + 2 + ); });Also applies to: 123-139
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (1)
app/client/src/widgets/TableWidgetV2/component/VirtualTable/useRowHeightMeasurement.tsx(1 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
app/client/src/widgets/TableWidgetV2/component/VirtualTable/useRowHeightMeasurement.tsx
[error] 61-62: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 149-149: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
⏰ Context from checks skipped due to timeout of 90000ms (20)
- GitHub Check: perform-test / ci-test / ci-test (19)
- GitHub Check: perform-test / ci-test / ci-test (18)
- GitHub Check: perform-test / ci-test / ci-test (17)
- GitHub Check: perform-test / ci-test / ci-test (16)
- GitHub Check: perform-test / ci-test / ci-test (15)
- GitHub Check: perform-test / ci-test / ci-test (14)
- GitHub Check: perform-test / ci-test / ci-test (13)
- GitHub Check: perform-test / ci-test / ci-test (12)
- GitHub Check: perform-test / ci-test / ci-test (11)
- GitHub Check: perform-test / ci-test / ci-test (10)
- GitHub Check: perform-test / ci-test / ci-test (9)
- GitHub Check: perform-test / ci-test / ci-test (8)
- GitHub Check: perform-test / ci-test / ci-test (7)
- GitHub Check: perform-test / ci-test / ci-test (6)
- GitHub Check: perform-test / ci-test / ci-test (5)
- GitHub Check: perform-test / ci-test / ci-test (4)
- GitHub Check: perform-test / ci-test / ci-test (3)
- GitHub Check: perform-test / ci-test / ci-test (2)
- GitHub Check: perform-test / ci-test / ci-test (1)
- GitHub Check: perform-test / ci-test / ci-test (0)
🔇 Additional comments (3)
app/client/src/widgets/TableWidgetV2/component/VirtualTable/useRowHeightMeasurement.tsx (3)
27-43: Good documentation implementationExcellent JSDoc comment that clearly explains the hook's purpose, when it should be used, and the measurement process. This addresses the previous reviewer's request for detailed comments.
44-53: Good implementation of the hook interfaceThe hook's parameter destructuring is well-organized and matches the interface definition. This makes the code more maintainable and self-documenting.
141-149:Details
✅ Verification successful
Consider adding a minimum height fallback
The current implementation calculates the total height based on measured elements, but doesn't include a fallback minimum height if measurements fail or return very small values.
const totalHeight = - Math.max(normalCellHeight, htmlCellHeight) + + Math.max(normalCellHeight, htmlCellHeight, TABLE_SIZES.DEFAULT.ROW_HEIGHT) + TABLE_SIZES.DEFAULT.VERTICAL_PADDING * 2;
🏁 Script executed:
#!/bin/bash # Check if TABLE_SIZES.DEFAULT.ROW_HEIGHT exists grep -r "ROW_HEIGHT" app/client/src/widgets/TableWidgetV2/component/Constants.tsLength of output: 162
Minimum Height Fallback Verified and Recommended
We've confirmed that the constant
TABLE_SIZES.DEFAULT.ROW_HEIGHTexists in the constants file (evidenced by the grep search). To ensure a fallback for cases where measured values are too small or fail, please update the Math.max computation as follows:
- File:
app/client/src/widgets/TableWidgetV2/component/VirtualTable/useRowHeightMeasurement.tsx(Lines 141-149)- Change: Replace
withMath.max(normalCellHeight, htmlCellHeight) +Math.max(normalCellHeight, htmlCellHeight, TABLE_SIZES.DEFAULT.ROW_HEIGHT) +This change ensures that the calculated row height will always be at least the defined default, improving stability when measurements are inadequate.
🧰 Tools
🪛 Biome (1.9.4)
[error] 149-149: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
app/client/src/widgets/TableWidgetV2/component/VirtualTable/useRowHeightMeasurement.tsx
Outdated
Show resolved
Hide resolved
app/client/src/widgets/TableWidgetV2/component/VirtualTable/useRowHeightMeasurement.tsx
Show resolved
Hide resolved
…rity and prevent unnecessary data handling
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
app/client/src/widgets/TableWidgetV2/component/VirtualTable/useRowHeightMeasurement.tsx (4)
60-63: Simplify with optional chaining.Use optional chaining to simplify the condition and avoid potential null/undefined errors.
- const isAlreadyCalculated = - rowNeedsMeasurement && - rowNeedsMeasurement.current && - rowNeedsMeasurement.current[index] === false; + const isAlreadyCalculated = rowNeedsMeasurement?.current?.[index] === false;🧰 Tools
🪛 Biome (1.9.4)
[error] 61-62: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
83-97: Consider optimizing the cell identification process.The current implementation potentially processes the same cell twice if it both has HTML content and allows wrapping. Consider tracking these attributes together in a single pass.
- const cellIndexesWithAllowCellWrapping: number[] = []; - const cellIndexesWithHTMLCell: number[] = []; + interface CellMeasurementInfo { + index: number; + hasWrapping: boolean; + isHtmlCell: boolean; + } + const cellsToMeasure: CellMeasurementInfo[] = []; if (row?.cells && Array.isArray(row.cells)) { row.cells.forEach((cell, index: number) => { const typedCell = cell as unknown as CellWithColumnProps; + const hasWrapping = !!typedCell?.column?.columnProperties?.allowCellWrapping; + const isHtmlCell = typedCell?.column?.columnProperties?.columnType === ColumnTypes.HTML; - if (typedCell?.column?.columnProperties?.allowCellWrapping) { - cellIndexesWithAllowCellWrapping.push(index); - } - - if ( - typedCell?.column?.columnProperties?.columnType === ColumnTypes.HTML - ) { - cellIndexesWithHTMLCell.push(index); + if (hasWrapping || isHtmlCell) { + cellsToMeasure.push({ index, hasWrapping, isHtmlCell }); } }); }
104-139: Combine height calculation logic for better maintainability.With the refactored cell identification process, you could also simplify the height calculation for better maintainability.
- cellIndexesWithAllowCellWrapping.forEach((index: number) => { - const child = children[index] as HTMLElement; - const dynamicContent = child.querySelector( - ".t--table-cell-tooltip-target", - ); - - if (dynamicContent) { - const styles = window.getComputedStyle(dynamicContent); - - // Calculate total height including margins and padding - normalCellHeight += - (dynamicContent as HTMLElement).offsetHeight + - parseFloat(styles.marginTop) + - parseFloat(styles.marginBottom) + - parseFloat(styles.paddingTop) + - parseFloat(styles.paddingBottom); - } - }); - - cellIndexesWithHTMLCell.forEach((index: number) => { - const child = children[index] as HTMLElement; - const dynamicContent = child.querySelector( - '[data-testid="t--table-widget-v2-html-cell"]>span', - ); - - if (dynamicContent) { - const styles = window.getComputedStyle(dynamicContent); - - htmlCellHeight += - (dynamicContent as HTMLElement).offsetHeight + - parseFloat(styles.marginTop) + - parseFloat(styles.marginBottom) + - parseFloat(styles.paddingTop) * 2 + - parseFloat(styles.paddingBottom) * 2; - } - }); + let maxCellHeight = 0; + + cellsToMeasure.forEach(({ index, hasWrapping, isHtmlCell }) => { + const child = children[index] as HTMLElement; + let cellHeight = 0; + + if (hasWrapping) { + const dynamicContent = child.querySelector(".t--table-cell-tooltip-target"); + if (dynamicContent) { + const styles = window.getComputedStyle(dynamicContent); + cellHeight = Math.max(cellHeight, + (dynamicContent as HTMLElement).offsetHeight + + parseFloat(styles.marginTop) + + parseFloat(styles.marginBottom) + + parseFloat(styles.paddingTop) + + parseFloat(styles.paddingBottom)); + } + } + + if (isHtmlCell) { + const dynamicContent = child.querySelector('[data-testid="t--table-widget-v2-html-cell"]>span'); + if (dynamicContent) { + const styles = window.getComputedStyle(dynamicContent); + cellHeight = Math.max(cellHeight, + (dynamicContent as HTMLElement).offsetHeight + + parseFloat(styles.marginTop) + + parseFloat(styles.marginBottom) + + parseFloat(styles.paddingTop) * 2 + + parseFloat(styles.paddingBottom) * 2); + } + } + + maxCellHeight = Math.max(maxCellHeight, cellHeight); + }); + + const totalHeight = maxCellHeight + TABLE_SIZES.DEFAULT.VERTICAL_PADDING * 2;
149-149: Simplify optional chaining.Use consistent optional chaining to improve readability.
- listRef?.current && listRef.current?.resetAfterIndex(index); + listRef?.current?.resetAfterIndex(index);🧰 Tools
🪛 Biome (1.9.4)
[error] 149-149: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (1)
app/client/src/widgets/TableWidgetV2/component/VirtualTable/useRowHeightMeasurement.tsx(1 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
app/client/src/widgets/TableWidgetV2/component/VirtualTable/useRowHeightMeasurement.tsx
[error] 61-62: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 149-149: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: perform-test / client-build / client-build
- GitHub Check: perform-test / rts-build / build
- GitHub Check: perform-test / server-build / server-unit-tests
- GitHub Check: client-check-cyclic-deps / check-cyclic-dependencies
- GitHub Check: client-build / client-build
- GitHub Check: client-lint / client-lint
- GitHub Check: client-unit-tests / client-unit-tests
- GitHub Check: client-prettier / prettier-check
🔇 Additional comments (4)
app/client/src/widgets/TableWidgetV2/component/VirtualTable/useRowHeightMeasurement.tsx (4)
16-25: Well-structured props interface with clear type annotations.The interface is well-defined with proper TypeScript types and clear documentation comments explaining each property.
27-43: Excellent JSDoc documentation.The documentation clearly explains the purpose, use cases, and measurement process of the hook. This will help other developers understand and maintain the code.
54-58: Good use of useMemo for derived values.Using useMemo for the prerequisites calculation prevents unnecessary recalculations on re-renders.
67-158: Thorough implementation with clear logic.The overall implementation of the useEffect hook is robust and handles edge cases well. It correctly identifies cells that might affect row height, measures them appropriately, and updates the virtualized list to reflect these measurements.
🧰 Tools
🪛 Biome (1.9.4)
[error] 149-149: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
jacquesikot
deleted the
feat/implement-infinite-scroll-with-dynamic-height
branch
…smithorg#39646) ## Description This PR adds support for variable height rows in the Table Widget when using infinite scroll. The implementation dynamically adjusts row heights based on content, particularly for wrapped text and HTML content. ## Key Features 1. **Dynamic Row Height Calculation**: Rows automatically resize based on content length when cell wrapping is enabled or HTML content is present 2. **Smooth Infinite Scrolling**: Maintains smooth scrolling performance while supporting variable height rows 3. **Responsive Layout**: Rows adjust in real-time when table data changes or when cell wrapping is toggled ## Implementation Details The implementation replaces the fixed-size virtual list with a variable-size virtual list that can handle rows of different heights: 1. Created a new `BaseVirtualList` component that uses `VariableSizeList` from react-window 2. Added row height measurement logic in the `Row` component to calculate optimal heights 3. Implemented a context-based system to store and update row heights 4. Created a utility hook `useColumnVariableHeight` to track columns that need variable height handling ## Testing Added comprehensive Cypress tests that verify: 1. Fixed height behavior when cell wrapping is disabled 2. Increased row heights when cell wrapping is enabled 3. Dynamic height updates when content changes 4. Proper handling of HTML content that have extended heights 5. Reverting to fixed height when wrapping is disabled Fixes appsmithorg#39089 ## Automation /ok-to-test tags="@tag.Widget, @tag.Sanity, @tag.Binding" ### 🔍 Cypress test results <!-- This is an auto-generated comment: Cypress test results --> > [!TIP] > 🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉 > Workflow run: <https://github.com/appsmithorg/appsmith/actions/runs/13946877628> > Commit: 9f50f22 > <a href="https://internal.appsmith.com/app/cypress-dashboard/rundetails-65890b3c81d7400d08fa9ee5?branch=master&workflowId=13946877628&attempt=1" target="_blank">Cypress dashboard</a>. > Tags: `@tag.Widget, @tag.Sanity, @tag.Binding` > Spec: > <hr>Wed, 19 Mar 2025 14:39:17 UTC <!-- end of auto-generated comment: Cypress test results --> ## Communication Should the DevRel and Marketing teams inform users about this change? - [ ] Yes - [ ] No <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **New Features** - Enhanced the table widget to automatically adjust row heights based on varying content lengths, wrapping settings, and HTML content for a smoother user experience. - Improved performance with optimized virtualization of table rows, ensuring efficient rendering and smooth infinite scrolling. - **Tests** - Added a comprehensive test suite validating the table’s behavior under fixed, dynamic, and updated content conditions, ensuring consistent row height adjustments during user interactions. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Co-authored-by: Rahul Barwal <rahul.barwal@appsmith.com>
2 participants
Description
This PR adds support for variable height rows in the Table Widget when using infinite scroll. The implementation dynamically adjusts row heights based on content, particularly for wrapped text and HTML content.\
Key Features
Implementation Details
The implementation replaces the fixed-size virtual list with a variable-size virtual list that can handle rows of different heights:
BaseVirtualListcomponent that usesVariableSizeListfrom react-windowRowcomponent to calculate optimal heightsuseColumnVariableHeightto track columns that need variable height handlingTesting
Added comprehensive Cypress tests that verify:
Fixes #39089, #39091
Automation
/ok-to-test tags="@tag.Widget, @tag.Sanity, @tag.Binding"
🔍 Cypress test results
Tip
🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/13946877628
Commit: 9f50f22
Cypress dashboard.
Tags:
@tag.Widget, @tag.Sanity, @tag.BindingSpec:
Wed, 19 Mar 2025 14:39:17 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
New Features
Tests