chore: refactor table widget UI code to use central context to reduce props drilling #39367
Conversation
…management - Extracts table header logic into separate components - Introduces TableProvider context to manage table state - Moves types to separate file for better organization - Adds default compact mode fallback - Simplifies imports and reorganizes code structure
- Moves table state management to TableContext - Adds memoization for computed values - Extracts scroll container styles to context - Simplifies StaticTable component props - Adds SCROLL_BAR_OFFSET constant
- Removes redundant SimpleBar wrapper in Table component - Adds SimpleBar directly in TableHeader for consistent scrolling - Imports SimpleBar CSS for proper styling
…ering - Refactors VirtualTable to use TableContext for props - Simplifies Table component's conditional rendering - Removes redundant prop passing in VirtualTable - Adds type definition for StaticTable props
- Refactors HeaderCell to use TableContext for accessing shared props - Removes redundant prop passing in HeaderCell - Simplifies header cell rendering and interaction logic - Improves component readability and reduces prop drilling
- Removes explicit prop passing in TableColumnHeader - Uses TableContext to access shared table state - Calculates header width using useMemo - Simplifies header rendering and interaction logic
- Refactors renderEmptyRows to use TableContext for accessing table state - Removes explicit prop passing in EmptyRows and EmptyRow components - Simplifies empty row rendering logic - Improves component readability and reduces prop drilling
WalkthroughThis pull request refactors the table widget code by centralizing the state management into a new TableContext and associated hook ( Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant VT as VirtualTable
participant UA as useAppsmithTable
participant ISC as InfiniteScrollBodyComponent
participant VTB as VirtualTableBodyComponent
participant SB as SimpleBar
U->>VT: Render Table
VT->>UA: Fetch table config and state
alt Infinite scroll enabled
VT->>ISC: Render Infinite Scroll Body
else
VT->>VTB: Render Virtual Table Body
end
VT->>SB: Wrap content in SimpleBar
SB-->>U: Display scrollable table view
Suggested labels
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 (6)
🪧 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
|
github-actions
bot
added
Query & Widgets Pod
…o rahulbarwal/tablev2-table-ui-components-refactor
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (16)
app/client/src/widgets/TableWidgetV2/component/header/index.tsx (3)
13-49: Refactor for Enhanced ReadabilityThe direct extraction of many properties from
useAppsmithTable()can become unwieldy. Consider grouping related properties into sub-objects to improve clarity and reduce repetitive prop spreads throughout the code.
59-77: Check Scrolling BehaviorWrapping with
SimpleBaris a neat approach to manage scrolling. Validate that scroll events and styles are correctly handled in all browsers. A fallback for environments without custom scrollbar support could further improve UX.
78-112: Consider Bundling Props
BannerNActionsreceives numerous props, which might be streamlined using an object or a context. This can help reduce repetitive passing of props and improve maintainability.app/client/src/widgets/TableWidgetV2/component/header/BannerNActions.tsx (2)
7-16: Leverage TypeScript Union TypesExplicitly defining a union type for scenarios like
isAddRowInProgresscan create clearer, narrower prop contracts. Consider applying such patterns to better communicate the intended usage.
18-34: Centralize Conditional LogicConditional rendering is handled cleanly. If more states and banner types emerge in the future, moving conditional logic into a dedicated helper function or a switch-based approach may improve scalability.
app/client/src/widgets/TableWidgetV2/component/cellComponents/HeaderCell.tsx (2)
177-178: Offset logic for sticky column with multi-row selection.
Ensureprops.column.totalLeftis always a valid numeric value to avoid misaligned headers.
204-205: Optional chaining to handle freeze.
IfhandleColumnFreezewill always be defined from the hook, consider removing the guard.🧰 Tools
🪛 Biome (1.9.4)
[error] 204-208: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
app/client/src/widgets/TableWidgetV2/component/Table.tsx (1)
133-136: Confirm behavior for out-of-range pageNo.
Ifprops.pageNoexceedspageCount, resetting to 0 may be unexpected. Consider clamping to the last page instead if that's the desired user experience.app/client/src/widgets/TableWidgetV2/component/StaticTable/index.tsx (1)
8-8: Use a type alias instead of an empty interface.
Empty interfaces are equivalent to{}. Updating with a type alias improves clarity:-interface StaticTableProps {} +type StaticTableProps = Record<string, never>;🧰 Tools
🪛 Biome (1.9.4)
[error] 7-8: An empty interface is equivalent to {}.
Safe fix: Use a type alias instead.
(lint/suspicious/noEmptyInterface)
app/client/src/widgets/TableWidgetV2/component/VirtualTable/InifiniteScrollBody/index.tsx (1)
36-36: Extract magic number into a named constant.The buffer size of 5 should be defined as a named constant to improve code maintainability and clarity.
+const ITEM_COUNT_BUFFER = 5; - itemCount={itemCount + 5} + itemCount={itemCount + ITEM_COUNT_BUFFER}app/client/src/widgets/TableWidgetV2/component/TableContext.tsx (2)
76-84: Update error message to match hook name.The error message refers to "useTable" but the hook is named "useAppsmithTable".
- throw new Error("useTable must be used within a TableProvider"); + throw new Error("useAppsmithTable must be used within a TableProvider");
54-65: Optimize memoization dependencies.The
scrollContainerStylesmemo dependency onstate.compactModeis unnecessary sincetableSizesis already derived from it.}, [ state.isHeaderVisible, state.height, - state.compactMode, state.width ]);app/client/src/widgets/TableWidgetV2/component/TableBodyCoreComponents/Row.tsx (2)
67-99: Extract cell rendering logic to a separate component.The cell rendering logic is complex and could benefit from being extracted into a separate component for better maintainability and potential reuse.
+ const TableCell = React.memo(({ cell, cellIndex, columns, multiRowSelection }) => { + const cellProperties = cell.getCellProps(); + cellProperties["style"] = { + ...cellProperties.style, + left: + columns[cellIndex].sticky === StickyType.LEFT && multiRowSelection + ? cell.column.totalLeft + MULTISELECT_CHECKBOX_WIDTH + : cellProperties?.style?.left, + }; + return ( + <div + {...cellProperties} + className={/* ... existing className logic ... */} + data-colindex={cellIndex} + data-rowindex={props.index} + key={cellIndex} + > + {cell.render("Cell")} + </div> + ); + });
32-38: Memoize row style computation.The row style computation could be memoized to prevent unnecessary recalculations.
+ const rowStyle = React.useMemo( + () => ({ + display: "flex", + ...(props.style || {}), + }), + [props.style] + ); const rowProps = { ...props.row.getRowProps(), - style: { - display: "flex", - ...(props.style || {}), - }, + style: rowStyle, };app/client/src/widgets/TableWidgetV2/component/header/TableColumnHeader.tsx (1)
113-113: Add error boundary for empty rows renderingConsider wrapping the empty rows rendering with an error boundary to gracefully handle potential rendering failures.
app/client/src/widgets/TableWidgetV2/component/cellComponents/EmptyCell.tsx (1)
127-154: Optimize sticky column calculationsConsider memoizing the sticky column calculations to prevent unnecessary recalculations during re-renders.
+const calculateLeftDistance = React.useMemo(() => { const leftColWidths = tableColumns .slice(0, colIndex) .map((col) => col.width); if (multiRowSelection) { return colIndex === 0 ? MULTISELECT_CHECKBOX_WIDTH : sum(leftColWidths) + MULTISELECT_CHECKBOX_WIDTH; } return colIndex === 0 ? 0 : sum(leftColWidths); +}, [colIndex, multiRowSelection, tableColumns]);
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (22)
app/client/src/widgets/TableWidgetV2/component/Constants.ts(1 hunks)app/client/src/widgets/TableWidgetV2/component/StaticTable.tsx(0 hunks)app/client/src/widgets/TableWidgetV2/component/StaticTable/StaticTableBody/index.tsx(1 hunks)app/client/src/widgets/TableWidgetV2/component/StaticTable/index.tsx(1 hunks)app/client/src/widgets/TableWidgetV2/component/Table.tsx(7 hunks)app/client/src/widgets/TableWidgetV2/component/TableBody/index.tsx(0 hunks)app/client/src/widgets/TableWidgetV2/component/TableBodyCoreComponents/Row.tsx(3 hunks)app/client/src/widgets/TableWidgetV2/component/TableBodyCoreComponents/VirtualList.tsx(2 hunks)app/client/src/widgets/TableWidgetV2/component/TableContext.tsx(1 hunks)app/client/src/widgets/TableWidgetV2/component/VirtualTable.tsx(0 hunks)app/client/src/widgets/TableWidgetV2/component/VirtualTable/InifiniteScrollBody/index.tsx(2 hunks)app/client/src/widgets/TableWidgetV2/component/VirtualTable/VirtualTableBodyComponent.tsx(1 hunks)app/client/src/widgets/TableWidgetV2/component/VirtualTable/VirtualTableInnerElement.tsx(1 hunks)app/client/src/widgets/TableWidgetV2/component/VirtualTable/index.tsx(1 hunks)app/client/src/widgets/TableWidgetV2/component/cellComponents/EmptyCell.tsx(2 hunks)app/client/src/widgets/TableWidgetV2/component/cellComponents/HeaderCell.tsx(9 hunks)app/client/src/widgets/TableWidgetV2/component/header/BannerNActions.tsx(1 hunks)app/client/src/widgets/TableWidgetV2/component/header/TableColumnHeader.tsx(3 hunks)app/client/src/widgets/TableWidgetV2/component/header/VirtualTableInnerElement.tsx(0 hunks)app/client/src/widgets/TableWidgetV2/component/header/index.tsx(1 hunks)app/client/src/widgets/TableWidgetV2/component/index.tsx(2 hunks)app/client/src/widgets/TableWidgetV2/component/types.ts(1 hunks)
💤 Files with no reviewable changes (4)
- app/client/src/widgets/TableWidgetV2/component/VirtualTable.tsx
- app/client/src/widgets/TableWidgetV2/component/StaticTable.tsx
- app/client/src/widgets/TableWidgetV2/component/TableBody/index.tsx
- app/client/src/widgets/TableWidgetV2/component/header/VirtualTableInnerElement.tsx
✅ Files skipped from review due to trivial changes (1)
- app/client/src/widgets/TableWidgetV2/component/Constants.ts
🧰 Additional context used
🪛 Biome (1.9.4)
app/client/src/widgets/TableWidgetV2/component/VirtualTable/index.tsx
[error] 8-9: An empty interface is equivalent to {}.
Safe fix: Use a type alias instead.
(lint/suspicious/noEmptyInterface)
app/client/src/widgets/TableWidgetV2/component/StaticTable/index.tsx
[error] 7-8: An empty interface is equivalent to {}.
Safe fix: Use a type alias instead.
(lint/suspicious/noEmptyInterface)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: perform-test / rts-build / build
- GitHub Check: perform-test / client-build / client-build
- GitHub Check: perform-test / server-build / server-unit-tests
🔇 Additional comments (30)
app/client/src/widgets/TableWidgetV2/component/header/index.tsx (1)
51-56: Validate Memo DependenciesUsing
React.useMemohere is optimal. However, ensure that all sources influencingcolumnsare properly included in its dependency array. Ifcolumnsis derived from dynamic data, confirm that the memo remains valid on each relevant update.app/client/src/widgets/TableWidgetV2/component/header/BannerNActions.tsx (1)
1-5: Verify Import PathsThe new
BannerNActionscomponent correctly imports fromactionsandbanner. Ensure that any rename or path relocations get updated consistently across the codebase.app/client/src/widgets/TableWidgetV2/component/cellComponents/HeaderCell.tsx (11)
159-169: Great use of the central hook for table props.
This centralizes state effectively and simplifies prop management.
172-172: No issue with spreading header props.
This is a standard approach and looks good.
182-182: ConfirmisResizingColumntype.
EnsureisResizingColumnis a ref to avoid runtime errors calling.current.
193-193: Validate negative index usage for sorting.
Check if thesortTableColumn(-1, ...)call gracefully handles "no sort" scenarios.
199-201: Readability of editable checks.
This chain of conditions is clear and concise.
274-276: Horizontal alignment logic is clear.
No issues in referencingcolumnProperties.horizontalAlignment.
286-286: Menu visibility condition.
The condition for hiding the menu is straightforward and looks correct.
296-306: Sorting actions on menu items.
Both ascending and descending sort calls appear consistent.
317-321: Freeze column menu items.
Conditional disabling and check icons are used effectively.Also applies to: 329-333
347-347: Portal class referencing the widget ID.
This namespacing strategy is good for preventing clashes.
365-366: Resizer props integration.
The resizing logic is properly delegated togetResizerProps().app/client/src/widgets/TableWidgetV2/component/Table.tsx (9)
1-3: Imports look good.
No issues noticed. The new references to Blueprint libraries, React Table, and related modules are appropriately declared.Also applies to: 7-7, 9-15, 17-17, 21-21, 23-24, 27-27, 29-30
120-132: Check for zero or negative page sizes.
Consider verifying ifprops.pageSizecan be zero or negative, which could lead to unexpected logic or infinitepageCountscenarios.
214-224: Confirm partial row selection toggle logic.
toggleAllRowSelect(!Boolean(rowSelectionState), page)also clears partial selections (state = 2). Double-check that this aligns with the intended tri-state selection behavior.🧰 Tools
🪛 Biome (1.9.4)
[error] 219-219: Avoid redundant
BooleancallIt is not necessary to use
Booleancall when a value will already be coerced to a boolean.
Unsafe fix: Remove redundantBooleancall(lint/complexity/noExtraBooleanCast)
224-239: Header visibility logic is clear.
Memoizing the condition for showing the header based on search, filters, and other flags is straightforward and readable.
255-269: Good approach to determining virtualization.
LeveragingisInfiniteScrollEnabledor checking for HTML columns is a clear strategy for deciding between static rendering and virtual scrolling.
282-296: Proper centralization with TableProvider.
Handing off table-specific props and state to a provider nicely simplifies the rest of the component.
326-326: Conditional table header is well-structured.
Showing the header only when necessary avoids clutter.
338-338: Swapping between VirtualTable and StaticTable is neat.
This ternary-based approach cleanly handles different rendering modes.
342-342: No additional remarks.app/client/src/widgets/TableWidgetV2/component/StaticTable/StaticTableBody/index.tsx (1)
1-21: StaticTableBodyComponent is straightforward.
Mapping overrowsto render aRowfor each entry, and renderingEmptyRowsifpageSizeexceeds the number of rows, is efficient and clear.app/client/src/widgets/TableWidgetV2/component/StaticTable/index.tsx (1)
1-7: StaticTable implementation looks good.
Forwarding the ref toSimpleBarand composing column headers with the table body is well-structured and easy to follow.Also applies to: 9-21
app/client/src/widgets/TableWidgetV2/component/VirtualTable/VirtualTableBodyComponent.tsx (1)
1-30: LGTM! Clean implementation with proper separation of concerns.The component follows React best practices by using forwardRef and centralizing state management through the useAppsmithTable hook.
app/client/src/widgets/TableWidgetV2/component/TableBodyCoreComponents/VirtualList.tsx (1)
37-37:❓ Verification inconclusive
Breaking Change:
outerRefis now required.Making
outerRefrequired could break existing implementations that don't provide this prop. Ensure all consumers of this component are updated.
🏁 Script executed:
#!/bin/bash # Find all usages of BaseVirtualListProps or VirtualList components ast-grep --pattern 'type $_ = BaseVirtualListProps' ast-grep --pattern 'const $_ = <(FixedVirtualList|FixedInfiniteVirtualList)'Length of output: 76644
Action Required: Verify Consumers for
outerRefUpdateThe change making
outerRef: Ref<SimpleBar>required in VirtualList (line 37) is a breaking change that could impact any consumer not supplying this prop. Our shell searches did not reveal any clear usages of BaseVirtualListProps or the FixedVirtualList/FixedInfiniteVirtualList components, so we couldn’t confirm whether current consumers cover this new requirement. Please ensure that:
- Every instance of VirtualList / its wrapped components is updated to pass a valid
outerRef.- Manual verification is done for all consumers (particularly within TableWidgetV2) to prevent runtime errors.
app/client/src/widgets/TableWidgetV2/component/header/TableColumnHeader.tsx (2)
16-29: Well-structured state management using context!The refactoring successfully centralizes state management by using the
useAppsmithTablehook, which reduces prop drilling and improves code organization.
30-36:❓ Verification inconclusive
Verify handling of zero totalColumnsWidth
While the memoization is correct, verify that the component handles cases where
totalColumnsWidthis 0 appropriately.
🏁 Script executed:
#!/bin/bash # Search for tests or handling of zero totalColumnsWidth rg -A 5 "totalColumnsWidth.*[=><].*0"Length of output: 39
Verify zero
totalColumnsWidthedge case handlingThe memoization implementation is correct; however, no explicit tests or handling for when
totalColumnsWidthis 0 were found in the repository. Please verify manually that whentotalColumnsWidthis 0, the component computesheaderWidthas expected (i.e., it should remain 0 rather than unexpectedly applying the multi-selection width). If this is an important edge case, consider adding a dedicated unit test to cover this behavior.app/client/src/widgets/TableWidgetV2/component/cellComponents/EmptyCell.tsx (1)
23-32: Clean function signature refactoring!The function has been successfully simplified by leveraging the context hook while maintaining its core functionality.
app/client/src/widgets/TableWidgetV2/component/index.tsx (1)
238-238: Good default value handling!Adding a default value for
compactModeensures consistent behavior when the prop is not provided.
app/client/src/widgets/TableWidgetV2/component/VirtualTable/VirtualTableInnerElement.tsx
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
app/client/src/widgets/TableWidgetV2/component/StaticTable/__tests__/StaticTable.test.tsx (2)
8-21: Consider adding error cases to SimpleBar mock.The SimpleBar mock is well-structured but only handles the happy path. Consider adding test cases for error scenarios.
jest.mock("simplebar-react", () => { return { __esModule: true, default: React.forwardRef< HTMLDivElement, { children: React.ReactNode; style?: React.CSSProperties } >((props, ref) => ( + // Add error handling for invalid props + !props ? null : <div className="simplebar-content-wrapper" ref={ref} style={props.style}> {props.children} </div> )), }; });
55-62: Add test for table rendering with empty data.The test suite covers basic rendering but misses edge cases. Consider adding a test for rendering with empty data.
it("renders correctly with empty data", () => { (useAppsmithTable as jest.Mock).mockReturnValue({ scrollContainerStyles: { height: 0, width: 0 }, }); const ref = React.createRef<SimpleBar>(); render(<StaticTable ref={ref} />); expect(screen.getByTestId("mock-table-header")).toBeInTheDocument(); expect(screen.getByTestId("mock-table-body")).toBeInTheDocument(); });app/client/src/widgets/TableWidgetV2/component/TableContext.test.tsx (1)
287-308: Enhance memoization test with multiple re-renders.The current memoization test only checks a single re-render. Consider testing multiple re-renders and prop changes.
it("maintains memoization across multiple re-renders", () => { const { rerender } = render( <TableProvider {...mockTableProviderProps}> <TestParent /> </TableProvider> ); const firstText = screen.getByText(/.+/).textContent; // Multiple re-renders with same props for (let i = 0; i < 5; i++) { rerender( <TableProvider {...mockTableProviderProps}> <TestParent /> </TableProvider> ); expect(screen.getByText(/.+/).textContent).toBe(firstText); } // Re-render with different props rerender( <TableProvider {...{ ...mockTableProviderProps, width: 900 }}> <TestParent /> </TableProvider> ); expect(screen.getByText(/.+/).textContent).not.toBe(firstText); });app/client/src/widgets/TableWidgetV2/component/StaticTable/__tests__/StaticTableBody.test.tsx (1)
122-136: Enhance performance test with timing measurements.The performance test verifies rendering of large datasets but doesn't measure actual performance metrics.
it("[Performance] should render large datasets within performance budget", () => { const largeDataSet = Array.from({ length: 1000 }, (_, index) => createMockRow(index, index) ); (useAppsmithTable as jest.Mock).mockReturnValue({ ...mockTableContextState, pageSize: 1000, subPage: largeDataSet, } as unknown as TableContextState); const start = performance.now(); const { getAllByTestId } = render(<StaticTableBodyComponent />); const end = performance.now(); expect(getAllByTestId("mocked-row")).toHaveLength(1000); expect(end - start).toBeLessThan(100); // 100ms budget });
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
app/client/src/widgets/TableWidgetV2/component/StaticTable/__tests__/StaticTable.test.tsx(1 hunks)app/client/src/widgets/TableWidgetV2/component/StaticTable/__tests__/StaticTableBody.test.tsx(1 hunks)app/client/src/widgets/TableWidgetV2/component/TableContext.test.tsx(1 hunks)
🔇 Additional comments (1)
app/client/src/widgets/TableWidgetV2/component/StaticTable/__tests__/StaticTableBody.test.tsx (1)
30-37: LGTM! Well-structured test helper function.The
createMockRowhelper function is well-implemented and reusable across tests.
app/client/src/widgets/TableWidgetV2/component/TableBodyCoreComponents/Row.tsx
Outdated
Show resolved
Hide resolved
app/client/src/widgets/TableWidgetV2/component/VirtualTable/VirtualTableInnerElement.tsx
Show resolved
Hide resolved
app/client/src/widgets/TableWidgetV2/component/header/TableColumnHeader.tsx
Outdated
Show resolved
Hide resolved
…o rahulbarwal/tablev2-table-ui-components-refactor
rahulbarwal
dismissed stale reviews from jacquesikot and AmanAgarwal041
via
ed5a94f
|
/build-deploy-preview skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/13714575880. |
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (3)
app/client/src/widgets/TableWidgetV2/component/Table.tsx(6 hunks)app/client/src/widgets/TableWidgetV2/component/TableBodyCoreComponents/VirtualList.tsx(3 hunks)app/client/src/widgets/TableWidgetV2/component/VirtualTable/InifiniteScrollBody/index.tsx(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- app/client/src/widgets/TableWidgetV2/component/TableBodyCoreComponents/VirtualList.tsx
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: client-lint / client-lint
- GitHub Check: client-unit-tests / client-unit-tests
- GitHub Check: client-check-cyclic-deps / check-cyclic-dependencies
🔇 Additional comments (9)
app/client/src/widgets/TableWidgetV2/component/Table.tsx (4)
120-136: Good use of useMemo for performance optimizationThe refactoring of
pageCountandcurrentPageIndexinto useMemo hooks with proper dependencies is a good performance optimization. This ensures calculations are only performed when relevant props change.
225-239: Well-structured memoization of UI visibility logicThe isHeaderVisible calculation has been properly extracted into a useMemo hook with appropriate dependencies, which improves performance and readability.
254-269: Consider addressing the TODO commentThe TODO comment suggests this code is intended for removal. Consider either removing it now or documenting why it's still needed and when it can be removed.
305-356: Good use of context provider for prop drilling reductionThe implementation of TableProvider to centralize state management is a good approach. This effectively reduces prop drilling and makes the component hierarchy easier to maintain.
The simplified rendering of TableHeader and conditional table type rendering also improves readability.
app/client/src/widgets/TableWidgetV2/component/VirtualTable/InifiniteScrollBody/index.tsx (5)
5-6: Good refactoring of importsThe import paths have been updated appropriately to match the new project structure.
10-12: Effective simplification of component propsRemoving unnecessary props from the interface in favor of using the context is a good refactoring approach that reduces coupling between components.
16-24: Well-implemented use of central contextUsing useAppsmithTable to access shared state is a cleaner approach than prop drilling. The destructuring with renamed variables (like subPage: rows) maintains compatibility with existing code while transitioning to the new naming convention.
30-30: Consistent naming updateThe reference to nextPageClick instead of loadMoreFromEvaluations maintains functional consistency while improving naming clarity.
44-52: Consistent property updatesProperties have been correctly updated to use context values instead of direct props, completing the refactoring to use centralized state.
| props.isVisibleSearch || | ||
| props.isVisibleFilters || | ||
| props.isVisibleDownload || | ||
| props.isVisiblePagination || | ||
| props.allowAddNewRow; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove dangling boolean expression
This boolean expression has no effect - it's not assigned to any variable or used in any conditional logic.
-props.isVisibleSearch ||
- props.isVisibleFilters ||
- props.isVisibleDownload ||
- props.isVisiblePagination ||
- props.allowAddNewRow;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| props.isVisibleSearch || | |
| props.isVisibleFilters || | |
| props.isVisibleDownload || | |
| props.isVisiblePagination || | |
| props.allowAddNewRow; |
|
/build-deploy-preview skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/13714702722. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
app/client/src/widgets/TableWidgetV2/component/VirtualTable/VirtualTableBodyComponent.tsx (3)
11-29: Consider using React.memo for performance optimization.The component is well-structured with ForwardRef, but adding memoization could help prevent unnecessary re-renders in a virtualized context where performance is critical.
-const VirtualTableBodyComponent = React.forwardRef( +const VirtualTableBodyComponent = React.memo(React.forwardRef( (props: VirtualTableBodyComponentProps, ref: Ref<SimpleBar>) => { const { height, pageSize, subPage: rows, tableSizes } = useAppsmithTable(); return ( <div className="simplebar-content-wrapper"> <FixedVirtualList height={height} innerElementType={props.innerElementType} itemCount={rows.length} outerRef={ref} pageSize={pageSize} rows={rows} tableSizes={tableSizes} /> </div> ); }, -); +));
13-13: Add fallback handling for context values.The destructured values from useAppsmithTable might be undefined if the component is rendered outside the expected context.
- const { height, pageSize, subPage: rows, tableSizes } = useAppsmithTable(); + const { height = 0, pageSize = 0, subPage: rows = [], tableSizes = {} } = useAppsmithTable();
31-32: Add component display name for better debugging.Setting a display name helps with identifying components in React DevTools and error stacks.
+VirtualTableBodyComponent.displayName = "VirtualTableBodyComponent"; export default VirtualTableBodyComponent;
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (1)
app/client/src/widgets/TableWidgetV2/component/VirtualTable/VirtualTableBodyComponent.tsx(1 hunks)
⏰ 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-prettier / prettier-check
- GitHub Check: client-check-cyclic-deps / check-cyclic-dependencies
- GitHub Check: client-lint / client-lint
- GitHub Check: client-build / client-build
🔇 Additional comments (2)
app/client/src/widgets/TableWidgetV2/component/VirtualTable/VirtualTableBodyComponent.tsx (2)
1-5: Appropriate imports for a virtual table component.The imports cover React essentials, SimpleBar for scrolling, and the required custom components. Good use of explicit type imports.
7-9: Clean interface definition.Interface is well-typed and focused on just the necessary props required by the component.
| <FixedVirtualList | ||
| height={height} | ||
| innerElementType={props.innerElementType} | ||
| itemCount={rows.length} |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Ensure safe access to rows.length.
If rows is null or undefined, accessing rows.length will cause a runtime error.
- itemCount={rows.length}
+ itemCount={rows?.length || 0}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| itemCount={rows.length} | |
| itemCount={rows?.length || 0} |
|
/build-deploy-preview skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/13714840830. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
app/client/src/widgets/TableWidgetV2/component/VirtualTable/InifiniteScrollBody/index.tsx (1)
17-24: Effective context-based refactor.Destructuring the table parameters from
useAppsmithTable()is a clean approach to reducing dependencies on parent props and enhancing maintainability. Ensure all usage sites correctly reference these destructured items and handle edge cases (e.g.,totalRecordsCountremains defined).
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (1)
app/client/src/widgets/TableWidgetV2/component/VirtualTable/InifiniteScrollBody/index.tsx(2 hunks)
⏰ 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 (5)
app/client/src/widgets/TableWidgetV2/component/VirtualTable/InifiniteScrollBody/index.tsx (5)
5-6: Good use of new imports to centralize logic.Importing
FixedInfiniteVirtualListanduseAppsmithTablehelps consolidate table functionality and reduces prop drilling by relying on context-based data.
11-11: Inner element customization.Exposing
innerElementTypeinInfiniteScrollBodyPropsis a neat addition that offers flexibility in rendering the table structure.
30-30: Naming consistency check.Renaming
loadMoretonextPageClickclarifies intent. Please verify that this updated function name is consistently referenced in related components and hooks throughout the codebase.
44-44: Height prop now sourced from context.Using
height={height}straight from the context is consistent and straightforward. No further issues here.
52-52: Contextual table sizing.Passing
tableSizes={tableSizes}from context aligns with the new approach for unified state. This is a good improvement over redundant props.
|
Deploy-Preview-URL: https://ce-39367.dp.appsmith.com |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
app/client/src/widgets/TableWidgetV2/component/StaticTable/StaticTableBodyComponent.tsx (1)
13-13: Consider using a more stable key for row mapping.Using array index as a key might cause rendering issues if rows are reordered or filtered dynamically.
- return <Row index={index} key={index} row={row} />; + return <Row index={index} key={row.id || index} row={row} />;app/client/src/widgets/TableWidgetV2/component/StaticTable/index.tsx (2)
8-8: Empty interface can be replaced with a type alias.An empty interface is equivalent to
{}. Consider using a type alias instead.- interface StaticTableProps {} + type StaticTableProps = Record<string, never>;🧰 Tools
🪛 Biome (1.9.4)
[error] 7-8: An empty interface is equivalent to {}.
Safe fix: Use a type alias instead.
(lint/suspicious/noEmptyInterface)
9-9: Improve parameter naming in the StaticTable component.Using underscore
_for props parameter can reduce code readability. Consider using a more descriptive name even if props aren't currently used.- const StaticTable = (_: StaticTableProps, ref: React.Ref<SimpleBar>) => { + const StaticTable = (props: StaticTableProps, ref: React.Ref<SimpleBar>) => {
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (5)
app/client/src/widgets/TableWidgetV2/component/StaticTable/StaticTableBodyComponent.tsx(1 hunks)app/client/src/widgets/TableWidgetV2/component/StaticTable/__tests__/StaticTableBody.test.tsx(1 hunks)app/client/src/widgets/TableWidgetV2/component/StaticTable/index.tsx(1 hunks)app/client/src/widgets/TableWidgetV2/component/VirtualTable/InifiniteScrollBody/index.tsx(3 hunks)app/client/src/widgets/TableWidgetV2/component/VirtualTable/index.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- app/client/src/widgets/TableWidgetV2/component/StaticTable/tests/StaticTableBody.test.tsx
🧰 Additional context used
🪛 Biome (1.9.4)
app/client/src/widgets/TableWidgetV2/component/StaticTable/index.tsx
[error] 7-8: An empty interface is equivalent to {}.
Safe fix: Use a type alias instead.
(lint/suspicious/noEmptyInterface)
app/client/src/widgets/TableWidgetV2/component/VirtualTable/index.tsx
[error] 8-9: An empty interface is equivalent to {}.
Safe fix: Use a type alias instead.
(lint/suspicious/noEmptyInterface)
⏰ 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-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 (12)
app/client/src/widgets/TableWidgetV2/component/StaticTable/StaticTableBodyComponent.tsx (1)
1-18: Clean implementation of the static table body component.The component structure looks good. It correctly leverages the TableContext through useAppsmithTable hook to access necessary props and data, resulting in cleaner code with less prop drilling.
app/client/src/widgets/TableWidgetV2/component/StaticTable/index.tsx (1)
1-20: Effective implementation of the StaticTable component using context.The component successfully utilizes React's context API through the useAppsmithTable hook to get the necessary styles and props. The use of SimpleBar for scrolling and proper component composition with TableColumnHeader and StaticTableBodyComponent is well-structured.
🧰 Tools
🪛 Biome (1.9.4)
[error] 7-8: An empty interface is equivalent to {}.
Safe fix: Use a type alias instead.
(lint/suspicious/noEmptyInterface)
app/client/src/widgets/TableWidgetV2/component/VirtualTable/index.tsx (4)
9-10: Remove unnecessary empty interface.The empty interface
VirtualTablePropsserves no purpose and can be replaced with an empty object type.-interface VirtualTableProps {} -const VirtualTable = (_: VirtualTableProps, ref: React.Ref<SimpleBar>) => { +const VirtualTable = (_: Record<string, never>, ref: React.Ref<SimpleBar>) => {
1-7: The imports look clean and appropriate.The component correctly imports necessary dependencies and custom components for its functionality.
11-30: Good implementation of the virtual table component.The component properly uses the context hook to determine rendering behavior and correctly implements conditional rendering based on the scroll mode. The use of SimpleBar and ref forwarding is appropriate for this use case.
32-32: Appropriate use of forwardRef.Correctly exports the component with React.forwardRef to ensure ref is passed to the SimpleBar component.
app/client/src/widgets/TableWidgetV2/component/VirtualTable/InifiniteScrollBody/index.tsx (6)
5-6: Good refactoring to use context.The import changes reflect the move to a context-based approach, which helps reduce prop drilling.
10-12: Props interface simplified appropriately.The interface has been correctly simplified to only include necessary props, as the other properties are now accessed through the context.
14-24: Good implementation of context consumer.The component has been renamed with the proper "Component" suffix for consistency, and now correctly uses the context hook to retrieve necessary values that were previously passed as props.
30-30: Property name update is consistent with context naming.The
nextPageClickproperty is now used instead ofloadMoreFromEvaluations, maintaining consistency with the context API.
44-52: Props are correctly sourced from context.The component now properly uses values from the context instead of props, which reduces prop drilling and makes the component more maintainable.
62-62: Consistent component export.The export correctly uses the updated component name.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
app/client/src/widgets/TableWidgetV2/component/StaticTable/__tests__/StaticTable.test.tsx (1)
88-104: Consider using try/finally for spy restorationWhile the current approach works, using try/finally would ensure the console spy is always restored, even if the test fails.
it("throws error when used outside TableContext", () => { const consoleErrorSpy = jest .spyOn(console, "error") .mockImplementation(() => {}); (useAppsmithTable as jest.Mock).mockImplementation(() => { throw new Error("useTable must be used within a TableProvider"); }); const ref = React.createRef<SimpleBar>(); - expect(() => render(<StaticTable ref={ref} />)).toThrow( - "useTable must be used within a TableProvider", - ); - - consoleErrorSpy.mockRestore(); + try { + expect(() => render(<StaticTable ref={ref} />)).toThrow( + "useTable must be used within a TableProvider", + ); + } finally { + consoleErrorSpy.mockRestore(); + } });
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (1)
app/client/src/widgets/TableWidgetV2/component/StaticTable/__tests__/StaticTable.test.tsx(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: client-lint / client-lint
- GitHub Check: client-unit-tests / client-unit-tests
- GitHub Check: client-check-cyclic-deps / check-cyclic-dependencies
- GitHub Check: client-build / client-build
- GitHub Check: client-prettier / prettier-check
- GitHub Check: perform-test / rts-build / build
- GitHub Check: perform-test / server-build / server-unit-tests
- GitHub Check: perform-test / client-build / client-build
🔇 Additional comments (7)
app/client/src/widgets/TableWidgetV2/component/StaticTable/__tests__/StaticTable.test.tsx (7)
1-7: Good import structureAll necessary testing utilities and components are properly imported for the test file.
8-21: Well-implemented SimpleBar mockThe mock implementation correctly handles ref forwarding and preserves the required props interface, which is essential for testing the component behavior.
23-37: Appropriate dependency mockingThe test correctly mocks all required dependencies with test IDs, allowing for proper component isolation during testing.
39-53: Good test setup and teardownThe test suite properly initializes mock data and clears mocks between tests to prevent test contamination.
55-62: Correct rendering verificationThis test effectively verifies that both header and body components are rendered as expected.
64-76: Thorough style application testGood test that verifies the styles from context are correctly applied to the scroll container.
78-86: Robust ref forwarding testThe test properly verifies that the ref is forwarded to the correct element. The class check is an appropriate alternative to instanceof for DOM elements.
…o rahulbarwal/tablev2-table-ui-components-refactor
rahulbarwal
deleted the
rahulbarwal/tablev2-table-ui-components-refactor
branch
Description
Refactor TableWidget for Improved Organization, Context Management & Rendering Efficiency
Overview
This PR restructures the TableWidget to enhance code organization, improve context management, and optimize rendering performance. Key improvements include extracting reusable components, introducing a shared TableContext, and simplifying table rendering logic.
Key Changes
✅ Improved Table Header Organization & Context Management
✅ Refactored Table & Virtualization Logic
✅ Simplified Header Components
✅ Enhanced Empty Row Handling
✅ Reorganized Core Components(Static table, virtual table)
Why These Changes?
Fixes #39308
or
Fixes
Issue URLWarning
If no issue exists, please create an issue first, and check with the maintainers if the issue is valid.
Automation
/ok-to-test tags="@tag.Table, @tag.Sanity, @tag.Datasource"
🔍 Cypress test results
Tip
🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/13761069297
Commit: 524a846
Cypress dashboard.
Tags:
@tag.Table, @tag.Sanity, @tag.DatasourceSpec:
Mon, 10 Mar 2025 12:31:27 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
Summary by CodeRabbit
New Features
BannerNActionscomponent for improved header action management.StaticTableBodyComponentfor rendering static table bodies.Refactor
Actionscomponent to utilize context for state management.Tablecomponent to improve clarity and efficiency.Tests
StaticTableBodyComponentandBannerNActions.