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
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
app/client/src/widgets/TableWidgetV2/component/cellComponents/EmptyCell.tsx (1)
167-169: Use optional chaining for safer property access.The current implementation may cause runtime errors if
columnis undefined.- column && column.isHidden + column?.isHidden🧰 Tools
🪛 Biome (1.9.4)
[error] 167-167: 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/TableBodyCoreComponents/Row.tsx(3 hunks)app/client/src/widgets/TableWidgetV2/component/cellComponents/EmptyCell.tsx(3 hunks)app/client/src/widgets/TableWidgetV2/component/header/actions/Actions.test.tsx(8 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- app/client/src/widgets/TableWidgetV2/component/TableBodyCoreComponents/Row.tsx
🧰 Additional context used
🪛 Biome (1.9.4)
app/client/src/widgets/TableWidgetV2/component/cellComponents/EmptyCell.tsx
[error] 167-167: 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 (16)
app/client/src/widgets/TableWidgetV2/component/header/actions/Actions.test.tsx (12)
5-6: Good use of the new TableProvider context.The imports for TableProvider and its type definition look correct. This aligns with the PR's objective to use a central context instead of props drilling.
39-142: Comprehensive table provider props setup.The defaultProps have been properly updated to match the TableProviderProps interface. This ensures the tests have all the necessary context values available.
A couple of observations:
- All required props for the TableProvider are included
- Mock functions are provided for all callbacks
- Default state values align with typical table initial state
144-152: Clean testing helper function implementation.The
renderWithTableProviderfunction effectively encapsulates the boilerplate code for rendering the Actions component within the context providers. This improves test readability and reduces duplication.
155-155: Test case simplification through context usage.The test case now uses the helper function which simplifies the test setup. This is more maintainable than the previous approach.
160-161: Proper test for conditional rendering.The test correctly verifies that the search component is not rendered when isVisibleSearch is false.
167-167: Effective mock function passing.The test correctly overrides the searchTableData function to verify it's called with the right parameters.
179-184: Proper pagination test setup.The test correctly overrides multiple properties to test pagination with the right data and configuration.
194-199: Well structured navigation testing.The test properly configures the context for testing the next page navigation functionality.
214-221: Comprehensive previous page navigation test.The test effectively sets up the initial state needed to test the previous page functionality, including currentPageIndex and pageNo.
234-236: Simplified feature flag testing.The test for the "add new row" feature correctly uses the context to enable this functionality.
244-247: Appropriate infinite scroll configuration.The test correctly sets up the infinite scroll configuration with test data to verify record counting behavior.
256-260: Complete record count test with totalRecordsCount.The test properly configures both the data array and totalRecordsCount to verify the correct display of record counts with infinite scrolling.
app/client/src/widgets/TableWidgetV2/component/cellComponents/EmptyCell.tsx (4)
4-7: Updated imports to align with hook-based pattern.The imports now reflect the shift to using context through the
useAppsmithTablehook, which is consistent with reducing props drilling as mentioned in the PR objectives.
23-32: Function signature simplified with context pattern.The function has been refactored to use the context-based pattern, reducing the number of parameters from 8 to just 2. This matches the PR goal of improving context management and reducing prop drilling.
39-93: Improved return structure using React fragments.Returning a React fragment instead of an array provides a more idiomatic React approach while maintaining the same rendering functionality. This change also improves readability.
107-183: Same pattern applied to the empty state rendering.The empty state rendering now consistently uses React fragments and leverages the table context, maintaining parallelism with the previous code block.
🧰 Tools
🪛 Biome (1.9.4)
[error] 167-167: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (7)
app/client/src/widgets/TableWidgetV2/component/StaticTable/__tests__/StaticTableBody.test.tsx (2)
127-141: Performance test could be more meaningful.While you're testing with 1000 rows, this test only verifies that rendering completes without error, not actual performance metrics. Consider adding more assertions around rendering time or memory usage.
it("[Performance] should maintain rendering performance with large data sets", () => { 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); + // Set a reasonable threshold based on your performance requirements + expect(end - start).toBeLessThan(500); // Example: render should take less than 500ms });
1-262: Consider adding accessibility tests.The test suite is comprehensive for functional aspects but doesn't cover accessibility concerns. Consider adding tests that verify accessibility attributes.
You could add a test like this:
it("should have appropriate accessibility attributes", () => { const mockRows = [createMockRow(1, 0), createMockRow(2, 1)]; (useAppsmithTable as jest.Mock).mockReturnValue({ ...mockTableContextState, pageSize: 2, subPage: mockRows, } as unknown as TableContextState); const { container } = render(<StaticTableBodyComponent />); // Update these expectations based on your actual accessibility requirements const tbody = container.querySelector('.tbody'); expect(tbody).toHaveAttribute('role', 'rowgroup'); });app/client/src/widgets/TableWidgetV2/component/cellComponents/EmptyCell.tsx (2)
115-191: Empty table rows logic.
The approach for rendering placeholders whenpage.lengthis zero is thorough. Consider extracting repeated sticky-column calculations into a helper to reduce complexity.🧰 Tools
🪛 Biome (1.9.4)
[error] 175-175: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
175-175: Use optional chaining to avoid null checks.
Replacingcolumn && column.isHiddenwithcolumn?.isHiddencan simplify the logic.- className={ - column && column.isHidden - ? "td hidden-cell" - : "td" + addStickyModifierClass(columns, cellIndex) - } + className={ + column?.isHidden + ? "td hidden-cell" + : "td" + addStickyModifierClass(columns, cellIndex) + }🧰 Tools
🪛 Biome (1.9.4)
[error] 175-175: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
app/client/src/widgets/TableWidgetV2/component/header/actions/index.tsx (3)
245-249: Simplify pagination conditional logicThe conditional logic for disabling the next button and the conditional inside the onClick handler are redundant and can be simplified.
- disabled={!!totalRecordsCount && pageNo === pageCount - 1} - onClick={() => { - if (!(!!totalRecordsCount && pageNo === pageCount - 1)) - nextPageClick(); - }} + disabled={!!totalRecordsCount && pageNo === pageCount - 1} + onClick={nextPageClick}
269-274: Simplify page navigation logicThe current implementation calculates pageNo and then has a separate condition to check if navigation should occur. This can be simplified.
- onClick={() => { - const pageNo = currentPageIndex > 0 ? currentPageIndex - 1 : 0; - - !(currentPageIndex === 0) && - updatePageNo(pageNo + 1, EventType.ON_PREV_PAGE); - }} + onClick={() => { + if (currentPageIndex > 0) { + updatePageNo(currentPageIndex, EventType.ON_PREV_PAGE); + } + }}
296-301: Simplify next page navigation logicSimilar to the previous comment, this logic can be simplified for better readability.
- onClick={() => { - const pageNo = - currentPageIndex < pageCount - 1 ? currentPageIndex + 1 : 0; - - !(currentPageIndex === pageCount - 1) && - updatePageNo(pageNo + 1, EventType.ON_NEXT_PAGE); - }} + onClick={() => { + if (currentPageIndex < pageCount - 1) { + updatePageNo(currentPageIndex + 2, EventType.ON_NEXT_PAGE); + } + }}
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
app/client/src/widgets/TableWidgetV2/component/StaticTable/StaticTableBody/index.tsx(1 hunks)app/client/src/widgets/TableWidgetV2/component/StaticTable/__tests__/StaticTableBody.test.tsx(1 hunks)app/client/src/widgets/TableWidgetV2/component/TableBodyCoreComponents/Row.tsx(2 hunks)app/client/src/widgets/TableWidgetV2/component/TableBodyCoreComponents/VirtualList.tsx(3 hunks)app/client/src/widgets/TableWidgetV2/component/cellComponents/EmptyCell.tsx(3 hunks)app/client/src/widgets/TableWidgetV2/component/header/TableColumnHeader.tsx(3 hunks)app/client/src/widgets/TableWidgetV2/component/header/actions/index.tsx(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- app/client/src/widgets/TableWidgetV2/component/StaticTable/StaticTableBody/index.tsx
🧰 Additional context used
🪛 Biome (1.9.4)
app/client/src/widgets/TableWidgetV2/component/cellComponents/EmptyCell.tsx
[error] 175-175: 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-build / client-build
- GitHub Check: client-lint / client-lint
- GitHub Check: client-check-cyclic-deps / check-cyclic-dependencies
- GitHub Check: client-unit-tests / client-unit-tests
- GitHub Check: client-prettier / prettier-check
🔇 Additional comments (26)
app/client/src/widgets/TableWidgetV2/component/StaticTable/__tests__/StaticTableBody.test.tsx (7)
1-11: Good import organization and mocking setup.The imports are well-organized and all the necessary dependencies have been included.
12-28: Well-structured mocks for isolated component testing.The mocks for TableContext, Row component, and EmptyRows provide good isolation for the component under test. Clean implementation of mock functions with appropriate return values.
30-53: Comprehensive test context setup.Good approach for creating mock data and setup. The createMockRow helper function is an efficient way to generate test data.
55-69: Good error boundary testing.Proper implementation of error case testing, including console error suppression to keep test output clean.
113-126: Good edge case handling for pageSize=0.Testing the edge case where pageSize is 0 ensures the component behaves correctly in all scenarios.
173-200: Well-implemented test for dynamic updates.The test correctly validates that the component updates properly when the data changes, an important aspect for reactive components.
203-260: Comprehensive EmptyRows testing.All the necessary edge cases for empty row rendering are covered with clear assertions.
app/client/src/widgets/TableWidgetV2/component/TableBodyCoreComponents/Row.tsx (3)
1-2: Imports look good.
No issues found with importingKeyfrom "react" andReactfor component usage.
7-7: Context usage updated.
ImportinguseAppsmithTablealigns with the new approach of using hooks for state management.
28-28: Destructuring fromuseAppsmithTable.
The destructured properties match usage in the component. Good job keeping the interface clean.app/client/src/widgets/TableWidgetV2/component/TableBodyCoreComponents/VirtualList.tsx (5)
3-3: Typed ref import.
Great job leveragingtype Reffrom React for stronger typing.
7-7: Switching to the new Row component.
Referencing theRowcomponent here is consistent with the refactor to a centralized hook-based context.
10-10: IntroducingEmptyRows.
Replacing older empty row implementations withEmptyRowsunifies empty state handling.
28-28: Rendering a single empty row.
Returning exactly one empty row withrows={1}is a neat fallback, but ensure it's the intended design for leftover space in the virtual list.
38-38: RequiredouterRef.
MakingouterRefmandatory enforces consistent usage. Verify all call sites now supply it to avoid runtime errors.app/client/src/widgets/TableWidgetV2/component/cellComponents/EmptyCell.tsx (5)
4-4: Refined “Cell” import.
UsingCellfrom "react-table" aligns with typed usage in the new structure.
7-7: Hook-based table access.
ImportinguseAppsmithTableenables consistent and centralized state retrieval.
23-26: NewEmptyRowsPropsinterface.
Defining a dedicated interface improves clarity and type safety for theEmptyRowscomponent.
28-40: Functional component for empty rows.
Adopting a functional component withuseAppsmithTableis clean and consistent.
47-101: Reusing first row for styling.
Rendering additional empty rows by referencing the first row’s structure is clever. Just confirm thatpage[0]is always valid in these scenarios.app/client/src/widgets/TableWidgetV2/component/header/TableColumnHeader.tsx (4)
2-8: Import organization looks cleanGood job consolidating the imports and adding the useAppsmithTable hook from TableContext.
18-31: Good transition to context-based state managementThe component now properly uses the useAppsmithTable hook to access shared state, eliminating the need for props drilling. This aligns well with the PR objectives.
33-39: Efficient memoization implementationGood use of useMemo for headerWidth calculation with the correct dependency array, preventing unnecessary recalculations.
114-114: Simplified empty rows renderingThe rendering of empty rows has been simplified, which improves readability.
app/client/src/widgets/TableWidgetV2/component/header/actions/index.tsx (2)
93-124: Effective transition to context-based approachThe component successfully migrates from a props-based approach to using context via useAppsmithTable, which reduces prop drilling as intended in the PR.
126-132: Proper memoization of derived dataGood use of useMemo for headerColumns with an appropriate dependency array, which prevents unnecessary recalculations.
| {/* TODO: Fix this the next time the file is edited */} | ||
| {/* eslint-disable-next-line @typescript-eslint/no-explicit-any */} | ||
| {headerGroup.headers.map((column: any, columnIndex: number) => { |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Consider addressing the existing TODO comment
There's a TODO comment about fixing something "the next time the file is edited." Since you're already refactoring this file, now would be a good time to address this issue.
🌐 Web query:
How to fix TypeScript eslint disable comment in React components?
💡 Result:
To properly handle TypeScript ESLint disable comments in React components while maintaining code quality, follow these guidelines:
1. Configure ban-ts-comment Rule
Adjust the rule in your ESLint config to allow specific directives when necessary:
// eslint.config.js
export default tseslint.config({
rules: {
"@typescript-eslint/ban-ts-comment": [
"error",
{
"ts-expect-error": "allow-with-description",
"ts-ignore": true // defaults to false (disallow)
}
]
}
});This configuration allows @ts-expect-error with descriptions but blocks @ts-ignore[1][4].
2. Valid Comment Format
Always include a description when suppressing errors:
// @ts-expect-error - Temporary workaround for API response type mismatch
const data = await fetchResponse<ExpectedType>();3. React Component Usage
For JSX blocks, use block-style ESLint disable comments:
{/* eslint-disable-next-line @typescript-eslint/ban-ts-comment */}
// @ts-expect-error - Legacy component compatibility
<DeprecatedComponent />4. Preferred Alternatives
Instead of directive comments:
- Use type assertions sparingly:
const element = document.getElementById('root') as HTMLElement;
- Fix type declarations properly
- Leverage TypeScript utility types (
Partial,Omit, etc.)
5. Temporary Suppression
When absolutely necessary:
// eslint-disable-next-line @typescript-eslint/ban-ts-comment
// @ts-ignore - Remove after backend API V2 migration
fetchLegacyEndpoint();Key Recommendations
- Audit Suppressions regularly (weekly/bi-weekly)
- Track Technical Debt using TODO comments with Jira/GitHub issue links
- Enable Type Checking in ESLint:
// eslint.config.js export default tseslint.config({ extends: [tseslint.configs.recommendedTypeChecked], parserOptions: { project: true, tsconfigRootDir: import.meta.dirname, }, });
For React-specific patterns, combine with React ESLint rules:
// eslint.config.js
export default tseslint.config({
extends: [
tseslint.configs.recommended,
'plugin:react/recommended'
],
rules: {
'react/jsx-key': 'error',
'@typescript-eslint/naming-convention': [
'error',
{ selector: 'variable', format: ['PascalCase', 'camelCase'] }
]
}
});Citations:
- 1: https://typescript-eslint.io/rules/ban-ts-comment/
- 2: https://mysticatea.github.io/eslint-plugin-eslint-comments/rules/no-restricted-disable.html
- 3: https://typescript-eslint.io/rules/naming-convention/
- 4: https://eslint.org/docs/latest/use/configure/rules
- 5: https://codeomelet.com/posts/react-eslint-configuration
- 6: https://timjames.dev/blog/the-best-eslint-rules-for-react-projects-30i8
- 7: https://codedamn.com/news/javascript/how-to-ignore-a-line-of-code-in-eslint
- 8: How to disable eslint complexity rule for functional component name eslint/eslint#17025
- 9: disable-next-line doesn't work with react rules in JSX jsx-eslint/eslint-plugin-react#2369
- 10: [eslint-config-react-app] Can't disable rule "@typescript-eslint/no-unused-vars" in config, outside of CRA facebook/create-react-app#6958
Action Required: Address the TODO and Improve Type Suppression Handling
Please remove or update the TODO comment by replacing the temporary ESLint disable with a more explicit, descriptive fix. For example, instead of simply disabling the rule for "any", either refactor by providing a proper type definition for the header or add a descriptive suppression comment (using, for instance, @ts-expect-error - description) that clearly documents the temporary workaround. This will improve maintainability and allow for easier auditing of technical debt.
- File:
app/client/src/widgets/TableWidgetV2/component/header/TableColumnHeader.tsx(Lines 80-82) - Update Suggestion:
Replace:With a more descriptive, temporary fix such as:{/* TODO: Fix this the next time the file is edited */} {/* eslint-disable-next-line @typescript-eslint/no-explicit-any */} {headerGroup.headers.map((column: any, columnIndex: number) => {
{/* TODO: Replace with proper type annotations for header columns */} {/* eslint-disable-next-line @typescript-eslint/no-explicit-any */} {/* @ts-expect-error - Using 'any' until header type is defined */} {headerGroup.headers.map((column: any, columnIndex: number) => {
- Additional Consideration: If possible, update the ESLint configuration to enforce descriptive error suppressions (see typescript-eslint/ban-ts-comment guidelines) and gradually eliminate the need for such suppressions.
…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
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.