feat: implement table widget infinite scroll with dynamic height #39646
Conversation
…o feat/implement-infinite-scroll-with-dynamic-height
WalkthroughThis change set enhances the Changes
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (8)
🪧 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 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:✅ 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:❓ 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)
jacquesikot
added
ok-to-test
|
/build-deploy-preview skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/13811463188. |
|
Deploy-Preview-URL: https://ce-39646.dp.appsmith.com |
| @@ -84,11 +84,14 @@ const StaticTable = (props: StaticTableProps, ref: React.Ref<SimpleBar>) => { | |||
| height={props.height} | |||
| isAddRowInProgress={props.isAddRowInProgress} | |||
| isLoading={props.isLoading} | |||
| listRef={null} | |||
There was a problem hiding this comment.
These props should be optional at the source.
| import type { ReactTableColumnProps } from "../Constants"; | ||
| import type { HeaderComponentProps } from "../Table"; | ||
|
|
||
| export type BodyContextType = { |
There was a problem hiding this comment.
Changes to this will become moot after my refactor PR goes in.
| setForceUpdate((prev) => prev + 1); | ||
| }, [wrappingColumns]); | ||
|
|
||
| useEffect(() => { |
There was a problem hiding this comment.
I think this useeffect is adding complexity to this component. Lets create a custom hook for this as well.
Pros:
- Isolated change
- better unit testing.
When you do it, add a check at top, to not do anything if infinteScroll is disabled.
| </FixedSizeList> | ||
| ); | ||
| }); | ||
| import BaseVirtualList, { type BaseVirtualListProps } from "./BaseVirtualList"; |
There was a problem hiding this comment.
I would give it a more explicit name that explains the nature of the component it renders. (I mean it should convey something about dynamic sizing.)
| return variableHeightColumnsRef.current; | ||
| } | ||
|
|
||
| // Helper function to check if row data has changed |
| }) | ||
| .filter((index) => index !== -1); | ||
|
|
||
| // Check if variable height configuration has changed |
There was a problem hiding this comment.
remove all such ai generated comments that don't help us.
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
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/13811361268
Commit: 5b8a9cc
Cypress dashboard.
Tags:
@tag.Widget, @tag.Sanity, @tag.BindingSpec:
Wed, 12 Mar 2025 14:24:22 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Tests