chore: added caching layer for table widget data#39703
chore: added caching layer for table widget data#39703AmanAgarwal041 merged 24 commits intoreleasefrom
Conversation
AmanAgarwal041
requested review from
jacquesikot
and removed request for
a team
WalkthroughThis pull request introduces a new property, Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
github-actions
bot
added
the
skip-changelog
github-actions
bot
added
Query Widgets & IDE Pod
|
/build-deploy-preview skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/13821990053. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
app/client/src/widgets/TableWidgetV2/component/TableBody/useCachingVirtualization.tsx (1)
33-46: Stateful caching approach.
Storing cached rows in local state is straightforward. Consider watchouts around memory usage if datasets grow large.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
app/client/src/widgets/TableWidgetV2/component/Table.tsx(4 hunks)app/client/src/widgets/TableWidgetV2/component/TableBody/InifiniteScrollBody/index.tsx(1 hunks)app/client/src/widgets/TableWidgetV2/component/TableBody/index.tsx(1 hunks)app/client/src/widgets/TableWidgetV2/component/TableBody/useCachingVirtualization.tsx(1 hunks)app/client/src/widgets/TableWidgetV2/component/VirtualTable.tsx(2 hunks)app/client/src/widgets/TableWidgetV2/component/header/actions/index.tsx(1 hunks)app/client/src/widgets/TableWidgetV2/component/index.tsx(4 hunks)app/client/src/widgets/TableWidgetV2/widget/index.tsx(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: perform-test / rts-build / build
- 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 (27)
app/client/src/widgets/TableWidgetV2/component/TableBody/index.tsx (1)
66-66: Good addition for virtualization cachingThis property is essential for the caching layer implementation, allowing the component to determine if an item at a specific index is already loaded.
app/client/src/widgets/TableWidgetV2/component/VirtualTable.tsx (2)
44-44: LGTM - Consistent interface additionThe property addition is correctly defined in the VirtualTableProps interface.
68-68: Property correctly passed to child componentThe isItemLoaded prop is properly forwarded to the TableBody component, maintaining the data flow needed for the caching system.
app/client/src/widgets/TableWidgetV2/component/header/actions/index.tsx (1)
98-98: Improved pagination signature for better controlThe updated method signature provides explicit control over which data range to load on pagination events, which is important for the caching layer implementation.
app/client/src/widgets/TableWidgetV2/widget/index.tsx (2)
1923-1927: Well-implemented row index calculationThis helper method adds clarity to the implementation by encapsulating the page-relative row index calculation logic. It's a clean approach that supports the caching virtualization system.
1936-1936: Good use of abstractionReplacing the direct index access with the getRowIndex method improves code maintainability and ensures consistent row index calculations throughout the component.
app/client/src/widgets/TableWidgetV2/component/index.tsx (5)
17-17: Clean import.
No concerns with importing the new hook.
174-183: Verify side effects in caching hook usage.
Ensure that any stale data or leftover states are properly reset when switching between infinite scrolling and normal pagination mode. Keeping state in sync is crucial to prevent edge-case rendering bugs.
250-250: Confirm that cached data meets all requirements.
ReplacingtableDatawithcachedRowsis logical for caching, but confirm that any dynamic row-level logic (e.g., row expansions or cell-level editing) still works as expected.
264-265: Great integration of loading checks.
UsingisItemLoadedandisLoadingDatahelps ensure consistent infinite scroll behavior.
272-272: Adapt function references for clarity.
RenamingnextPageClicktoloadMoreNextPageclarifies usage, making the code more readable.app/client/src/widgets/TableWidgetV2/component/Table.tsx (4)
94-94: Align pagination method signature.
Accepting(startIndex, endIndex)innextPageClickis consistent with virtualization patterns.
138-138: Handy method for loaded state.
isItemLoadedhelps manage partial data loading, especially beneficial in infinite scrolling scenarios.
534-534: Fluid data loading checks.
PassingisItemLoadedintoVirtualTableensures the component knows if more data needs to be fetched.
538-538: Naming change in load callback.
loadMoreFromEvaluations={props.nextPageClick}is conceptually correct because it delegates to the updated method signature. Just confirm that callers handle both parameters.app/client/src/widgets/TableWidgetV2/component/TableBody/InifiniteScrollBody/index.tsx (3)
18-18: Enhanced item loading signature.
isItemLoadedclarifies how items are loaded in infinite scroll, which is helpful for large datasets.
26-29: Good integration with InfiniteLoader.
ProvidingisItemLoaded,itemCount, andloadMoreItemsensures a seamless infinite scroll experience.
45-45: Accurate loading indicator.
Displaying<LoadingIndicator />based onprops.isLoadingis consistent with typical infinite scroll patterns.app/client/src/widgets/TableWidgetV2/component/TableBody/useCachingVirtualization.tsx (9)
1-2: Use of lodash concat is acceptable.
No concerns with the chosen approach; it’s concise for extending arrays.
3-11: Comprehensive prop typing.
Defining clear interfaces for the hook props helps ensure consistent usage.
13-22: Return type fosters clarity.
Exporting an interface for the hook’s return object is a good practice, aiding maintainability.
24-32: Constructive default logic for pagination.
LeveragingInfinitywhentotalRecordsCountis absent is intuitive; be sure to handle any overhead from unbounded pages.
47-59: Valid check for loaded item.
(index) => pageIndex < lastLoadedPageRef.currentis a pragmatic approach to detect previously fetched pages.
60-94: Syncing cached data intricacies.
The code appropriately handles newly added rows. Double-check concurrency with other table actions (e.g., row updates or deletions) to avoid mismatch.
96-99: Clever decoupled loading state.
UpdatingisLoadingDatafromisLoadingfosters a simpler mental model for hooking logic.
100-127: loadMoreNextPage logic is thorough.
VerifyinghasMoreDataRefand!isLoadingDatabefore callingnextPageClickprevents extraneous calls.
129-135: Overall well-structured hook.
The final return object is cleanly exposed and easy to integrate into table components.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
app/client/src/widgets/TableWidgetV2/component/index.tsx (1)
174-183: Well-structured usage ofuseCachingVirtualization.
These props align well with the hook’s interface. Consider verifying thattableDatais always defined to avoid null references.app/client/src/widgets/TableWidgetV2/component/TableBody/InifiniteScrollBody/index.tsx (1)
45-45: Loader visibility
Conditionally rendering the loader viaprops.isLoadingis straightforward. Just confirm downstream code covers error states if needed.app/client/src/widgets/TableWidgetV2/component/TableBody/useCachingVirtualization.tsx (3)
3-11:UseCachingVirtualizationPropsdefinition
The prop definitions look comprehensive. Consider how to handle undefinednextPageClickordata.
33-94: Caching logic and side effects
The approach robustly accumulates loaded pages. For edge cases with partial pages or no results, ensure adjacent code properly updateshasMoreDataRef.
100-127: AsyncloadMoreNextPage
Nicely checkshasMoreDataRefandisLoadingDatato prevent redundant calls. Consider logs or callbacks for debugging if data fails to load.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
app/client/src/widgets/TableWidgetV2/component/Table.tsx(4 hunks)app/client/src/widgets/TableWidgetV2/component/TableBody/InifiniteScrollBody/index.tsx(1 hunks)app/client/src/widgets/TableWidgetV2/component/TableBody/index.tsx(1 hunks)app/client/src/widgets/TableWidgetV2/component/TableBody/useCachingVirtualization.tsx(1 hunks)app/client/src/widgets/TableWidgetV2/component/VirtualTable.tsx(2 hunks)app/client/src/widgets/TableWidgetV2/component/header/actions/index.tsx(1 hunks)app/client/src/widgets/TableWidgetV2/component/index.tsx(4 hunks)app/client/src/widgets/TableWidgetV2/widget/index.tsx(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: perform-test / client-build / client-build
- GitHub Check: client-unit-tests / client-unit-tests
- GitHub Check: client-lint / client-lint
🔇 Additional comments (20)
app/client/src/widgets/TableWidgetV2/component/TableBody/index.tsx (1)
66-66: Adding isItemLoaded property for loading state managementThis new property enhances the interface to determine if specific items are loaded, which is necessary for the caching layer implementation.
app/client/src/widgets/TableWidgetV2/component/VirtualTable.tsx (2)
44-44: Adding isItemLoaded to VirtualTableProps typeThis addition complements the same property in the TableBody component, maintaining the propagation of loading state through the component hierarchy.
68-68: Passing isItemLoaded prop to TableBodyCorrectly propagates the isItemLoaded function to the child component.
app/client/src/widgets/TableWidgetV2/component/header/actions/index.tsx (1)
98-98: Updated nextPageClick signature to accept start and end indicesThis enhancement provides more granular control over pagination by specifying exactly which range of data to load for the next page.
app/client/src/widgets/TableWidgetV2/widget/index.tsx (2)
1923-1927: Added getRowIndex method for calculating effective row indexThis method calculates the row index within the current page, which is important for correctly rendering data in a paginated context. The modulo operation ensures row indices are calculated relative to page size.
Consider adding a brief doc comment explaining the purpose of this method for better maintainability:
+/** + * Calculates the effective row index within the current page + * @param rowIndex The absolute row index + * @returns The relative row index in the current page + */ getRowIndex = (rowIndex: number) => { const { pageSize } = this.props; return rowIndex % pageSize; };
1936-1936: Using getRowIndex in renderCell instead of direct index accessCorrectly implements the row index calculation for rendering cells within the current page context.
app/client/src/widgets/TableWidgetV2/component/index.tsx (4)
17-17: Good addition of the new hook import.
This import neatly organizes the caching logic in a separate module.
250-250: Switch tocachedRows
Replacing directtableDatausage withcachedRowsis consistent with the caching approach. Looks good.
264-265: Updated loading management
BindingisItemLoadedandisLoadingDataensures the component is more granularly aware of data readiness.
272-272:Details
❓ Verification inconclusive
Replacing
nextPageClickwithloadMoreNextPage
This complements the caching approach. Verify that no other calls still expect the old signature.
🏁 Script executed:
#!/bin/bash # Check for references of the old signature rg "nextPageClick\s*\(\)" -A 3 -B 3Length of output: 4074
Action: Verify that all components using
nextPageClicknow correctly delegate to the new caching function implementation.The update in
app/client/src/widgets/TableWidgetV2/component/index.tsxassigns the new function,loadMoreNextPage, to thenextPageClickprop. However, our grep results show that several parts of the codebase still invokenextPageClick(). In particular:
TableWidgetV2 subcomponents:
- In
app/client/src/widgets/TableWidgetV2/component/header/actions/index.tsx– there’s a call toprops.nextPageClick().- In
app/client/src/widgets/TableWidgetV2/component/TableBody/useCachingVirtualization.tsx– multiple direct calls tonextPageClick()are present.Other widgets:
- In
app/client/src/widgets/TableWidget/component/TableHeader.tsxand both ListWidget components (ListWidgetV2andListWidget) –nextPageClick()is still used.Please confirm that these invocations now receive the updated
loadMoreNextPagebehavior and that their expected signatures remain compatible. If updating the signature in TableWidgetV2 should affect related components outside of it, ensure these are updated accordingly.app/client/src/widgets/TableWidgetV2/component/Table.tsx (2)
93-93: ExtendednextPageClicksignature
Accepting(startIndex, endIndex)aligns with the new pagination and caching model. Ensure all call sites are updated.
138-138: NewisItemLoadedproperty
This property is key for infinite scrolling logic. Implementation appears consistent with the rest of the codebase.app/client/src/widgets/TableWidgetV2/component/TableBody/InifiniteScrollBody/index.tsx (3)
18-18: AddedisItemLoadedprop
This prop will help determine load completion for each row. A solid approach for infinite scrolling.
26-29: Direct references for item loading and page size
Usingpropsdirectly simplifies the data flow after removing the old hook. This is easier to follow.
36-41: Consistent item count & row usage
AligningitemCountwith eithertotalRecordsCountorrows.lengthis practical.app/client/src/widgets/TableWidgetV2/component/TableBody/useCachingVirtualization.tsx (5)
1-2: Imports and initial setup
Imports fromlodashand React are standard. No issues spotted.
13-22:UseCachingTableDataReturninterface
Clear structure for returned fields. Good job providing a typed method signature forisItemLoaded.
24-32: Hook declaration
Well-labeled function and consistent naming.
96-99: Separate effect for loading state
Separating loading state updates keeps the logic simple. This is a good pattern.
129-135: Hook's return object
Exposes all the relevant fields needed for the table’s caching. The design fosters clarity in usage.
There was a problem hiding this comment.
Actionable comments posted: 3
🔭 Outside diff range comments (1)
app/client/src/widgets/TableWidgetV2/component/StaticTable.tsx (1)
80-99:⚠️ Potential issueMissing implementation of the new isItemLoaded prop
The
isItemLoadedproperty has been added to the interface, but it's not being passed to theTableBodycomponent. This could lead to undefined behavior when the TableBody tries to access this property.<TableBody accentColor={props.accentColor} borderRadius={props.borderRadius} columns={props.columns} getTableBodyProps={props.getTableBodyProps} height={props.height} isAddRowInProgress={props.isAddRowInProgress} isLoading={props.isLoading} loadMoreFromEvaluations={props.loadMoreFromEvaluations} + isItemLoaded={props.isItemLoaded} multiRowSelection={!!props.multiRowSelection} pageSize={props.pageSize} prepareRow={props.prepareRow} primaryColumnId={props.primaryColumnId} rows={props.subPage} selectTableRow={props.selectTableRow} selectedRowIndex={props.selectedRowIndex} selectedRowIndices={props.selectedRowIndices} tableSizes={props.tableSizes} useVirtual={props.useVirtual} width={props.width - TABLE_SCROLLBAR_WIDTH / 2} />
🧹 Nitpick comments (3)
app/client/src/widgets/TableWidgetV2/component/TableBody/InifiniteScrollBody/useInfiniteVirtualization.tsx (1)
96-109: Consider proper error handling for edge casesThe current implementation doesn't handle edge cases where
startIndexis greater thanstopIndexor where either index is negative. As these parameters will now be optional, adding validation would make the code more robust.const loadMoreItems = useCallback( - async (startIndex: number, stopIndex: number) => { + async (startIndex?: number, stopIndex?: number) => { + // Handle edge cases and validate inputs + const effectiveStartIndex = startIndex ?? 0; + const effectiveStopIndex = stopIndex ?? (effectiveStartIndex + pageSize); + + if (effectiveStartIndex < 0 || effectiveStopIndex < 0 || effectiveStartIndex > effectiveStopIndex) { + console.warn("Invalid startIndex or stopIndex provided to loadMoreItems"); + return Promise.resolve(); + } if (!isLoading && hasMoreDataRef.current) { - const targetPage = Math.floor(stopIndex / pageSize); + const targetPage = Math.floor(effectiveStopIndex / pageSize); if (targetPage >= lastLoadedPageRef.current && targetPage < maxPages) { loadMore(); } } return Promise.resolve(); }, [isLoading, loadMore, pageSize, maxPages], );app/client/src/widgets/TableWidgetV2/component/TableBody/useCachingVirtualization.tsx (2)
15-15: Remove commented code.This commented
itemCountproperty should be either implemented or removed to maintain clean code.- // itemCount: number;
60-94: Consider simplifying the complex caching logic.The effect handling has several nested conditions making it harder to follow. This effect contains complex logic for managing cached rows in different scenarios.
Extract the complex nested logic into separate helper functions:
useEffect(() => { + const handleAddRowInProgress = () => { + if (isAddRowInProgressRef.current) { + isAddRowInProgressRef.current = false; + setCachedRows((e) => e.slice(1)); + return true; + } + return false; + }; + + const handleNewData = () => { + if (data.length > 0) { + const currentPageIndex = lastLoadedPageRef.current; + + // Only increment if we got a full page or some data + if (data.length === pageSize) { + lastLoadedPageRef.current = currentPageIndex + 1; + } else if (data.length < pageSize && data.length > 0) { + // If we got less than a full page, assume this is the last page + hasMoreDataRef.current = false; + } + + setCachedRows((e) => concat(e, data)); + } else if (data.length === 0 && lastLoadedPageRef.current > 0) { + // If no rows are returned and we've loaded at least one page, assume end of data + hasMoreDataRef.current = false; + } + }; if (isInfiniteScrollEnabled) { - if (isAddRowInProgressRef.current) { - isAddRowInProgressRef.current = false; - setCachedRows((e) => e.slice(1)); - - // TODO: If new rows are added, we would have to update the cached rows - return; - } else { + if (!handleAddRowInProgress()) { if (isAddRowInProgress) { isAddRowInProgressRef.current = true; setCachedRows((e) => concat([data[0]], e)); - } else if (data.length > 0) { - const currentPageIndex = lastLoadedPageRef.current; - - // Only increment if we got a full page or some data - if (data.length === pageSize) { - lastLoadedPageRef.current = currentPageIndex + 1; - } else if (data.length < pageSize && data.length > 0) { - // If we got less than a full page, assume this is the last page - hasMoreDataRef.current = false; - } - - setCachedRows((e) => concat(e, data)); - } else if (data.length === 0 && lastLoadedPageRef.current > 0) { - // If no rows are returned and we've loaded at least one page, assume end of data - hasMoreDataRef.current = false; + } else { + handleNewData(); } } } else { if (data.length > 0) { setCachedRows(data); } } }, [data, isInfiniteScrollEnabled, pageSize]);
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (7)
app/client/src/widgets/TableWidgetV2/component/StaticTable.tsx(1 hunks)app/client/src/widgets/TableWidgetV2/component/Table.tsx(4 hunks)app/client/src/widgets/TableWidgetV2/component/TableBody/InifiniteScrollBody/useInfiniteVirtualization.tsx(1 hunks)app/client/src/widgets/TableWidgetV2/component/TableBody/index.tsx(2 hunks)app/client/src/widgets/TableWidgetV2/component/TableBody/useCachingVirtualization.tsx(1 hunks)app/client/src/widgets/TableWidgetV2/component/VirtualTable.tsx(2 hunks)app/client/src/widgets/TableWidgetV2/component/header/actions/index.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- app/client/src/widgets/TableWidgetV2/component/header/actions/index.tsx
- app/client/src/widgets/TableWidgetV2/component/Table.tsx
- app/client/src/widgets/TableWidgetV2/component/VirtualTable.tsx
- app/client/src/widgets/TableWidgetV2/component/TableBody/index.tsx
🔇 Additional comments (4)
app/client/src/widgets/TableWidgetV2/component/StaticTable.tsx (1)
46-47: Interface updated properly for caching layerThe signature changes to the
StaticTablePropstype definition align with implementing a caching layer, making the pagination more efficient. The optional parameters forloadMoreFromEvaluationsallow for more flexible data loading strategies.app/client/src/widgets/TableWidgetV2/component/TableBody/useCachingVirtualization.tsx (3)
47-58: Good implementation of isItemLoaded function.The function correctly determines if an item is loaded based on its index and current pagination state. The use of
useCallbackwith proper dependencies is appropriate for performance optimization.
100-127: Return type inconsistency in loadMoreNextPage.The function returns a Promise in one case but is void in another while the signature declares it can return either.
Check if any consuming components expect a consistent return type. If they do, consider:
const loadMoreNextPage = useCallback( async (startIndex?: number, stopIndex?: number) => { if (isInfiniteScrollEnabled) { if (!isLoadingData && hasMoreDataRef.current && !isAddRowInProgress) { - const targetPage = Math.floor(stopIndex! / pageSize); + const targetPage = stopIndex !== undefined ? Math.floor(stopIndex / pageSize) : 0; if ( targetPage >= lastLoadedPageRef.current && targetPage < maxPages ) { nextPageClick(); } } return Promise.resolve(); } else { nextPageClick(); + return Promise.resolve(); } }, [ isLoadingData, nextPageClick, isInfiniteScrollEnabled, pageSize, maxPages, isAddRowInProgress, ], );
129-135: Clean and concise return structure.The hook properly returns all necessary data and functions with a clean interface.
| isItemLoaded: (index: number) => boolean; | ||
| itemCount: number; | ||
| loadMoreItems: (startIndex: number, stopIndex: number) => Promise<void>; | ||
| loadMoreItems: (startIndex?: number, stopIndex?: number) => Promise<void>; |
There was a problem hiding this comment.
Interface definition mismatch with implementation
The interface makes startIndex and stopIndex optional, but the implementation on line 97 still treats them as required parameters. This will cause TypeScript errors and potential runtime issues.
Fix the implementation to handle optional parameters:
const loadMoreItems = useCallback(
- async (startIndex: number, stopIndex: number) => {
+ async (startIndex?: number, stopIndex?: number) => {
+ // Handle the case when parameters are not provided
+ const effectiveStopIndex = stopIndex ?? (startIndex ? startIndex + pageSize : pageSize);
+ const effectiveStartIndex = startIndex ?? 0;
if (!isLoading && hasMoreDataRef.current) {
- const targetPage = Math.floor(stopIndex / pageSize);
+ const targetPage = Math.floor(effectiveStopIndex / pageSize);
if (targetPage >= lastLoadedPageRef.current && targetPage < maxPages) {
loadMore();
}
}
return Promise.resolve();
},
[isLoading, loadMore, pageSize, maxPages],
);📝 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.
| loadMoreItems: (startIndex?: number, stopIndex?: number) => Promise<void>; | |
| const loadMoreItems = useCallback( | |
| - async (startIndex: number, stopIndex: number) => { | |
| + async (startIndex?: number, stopIndex?: number) => { | |
| + // Handle the case when parameters are not provided | |
| + const effectiveStopIndex = stopIndex ?? (startIndex ? startIndex + pageSize : pageSize); | |
| + const effectiveStartIndex = startIndex ?? 0; | |
| if (!isLoading && hasMoreDataRef.current) { | |
| - const targetPage = Math.floor(stopIndex / pageSize); | |
| + const targetPage = Math.floor(effectiveStopIndex / pageSize); | |
| if (targetPage >= lastLoadedPageRef.current && targetPage < maxPages) { | |
| loadMore(); | |
| } | |
| } | |
| return Promise.resolve(); | |
| }, | |
| [isLoading, loadMore, pageSize, maxPages], | |
| ); |
| async (startIndex?: number, stopIndex?: number) => { | ||
| if (isInfiniteScrollEnabled) { | ||
| if (!isLoadingData && hasMoreDataRef.current && !isAddRowInProgress) { | ||
| const targetPage = Math.floor(stopIndex! / pageSize); |
There was a problem hiding this comment.
Non-null assertion operator on stopIndex may cause runtime errors.
Using stopIndex! assumes stopIndex is never undefined, but the function signature declares it as optional.
- const targetPage = Math.floor(stopIndex! / pageSize);
+ const targetPage = stopIndex !== undefined ? Math.floor(stopIndex / pageSize) : 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.
| const targetPage = Math.floor(stopIndex! / pageSize); | |
| const targetPage = stopIndex !== undefined ? Math.floor(stopIndex / pageSize) : 0; |
| isAddRowInProgressRef.current = false; | ||
| setCachedRows((e) => e.slice(1)); | ||
|
|
||
| // TODO: If new rows are added, we would have to update the cached rows |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Address the TODO comment.
The TODO comment indicates a pending implementation for handling added rows.
🌐 Web query:
What's the best practice for handling TODO comments in React applications?
💡 Result:
When handling TODO comments in React applications, follow these best practices to maintain code quality and accountability:
Make TODOs Actionable with Tracking
- Link to external issue trackers for accountability:
// TODO: Refactor legacy context usage. JIRA-456[1][4] - Avoid vague TODOs like
// TODO: Fix later. Instead, specify the required action:
// TODO: Replace class component with hooks implementation[1]
Implement Expiration Conditions
Use ESLint rules to enforce temporal constraints:
// Expires March 2026
// TODO(2026-03-31): Remove React.lazy loading after analytics migration
const Dashboard = React.lazy(() => import('./Dashboard'));The eslint-plugin-unicorn rule supports multiple expiration triggers [2]:
- Date-based expiration (
TODO(2025-12-31)) - Version-based conditions (
TODO(package@2.0.0)) - Package dependency checks (
TODO(react@19))
Deprecation Guidance
When marking deprecated React components:
/**
* @deprecated Use <NewDataGrid> with virtualization instead
*/
class LegacyGrid extends React.Component {
// ...
}Include migration instructions in the comment [1][4]
IDE Integration Strategies
- Use WebStorm's TODO tool window to track items across components [4]
- Create custom filters for team-specific patterns:
\bOPTIMIZE\b.* // Track performance improvements \bRFC\b.* // Flag design discussions
Maintenance Practices
- Review TODOs during sprint planning using IDE-generated reports [4]
- Pair with code ownership - Add team/person tags:
// TODO(@backend-team): Implement API error handling - Delete resolved TODOs immediately after implementation
For React-specific patterns, combine TODO comments with component documentation:
// TODO: Convert to TypeScript (see ADR-45)
/**
* Handles complex form validation
* @param {Object} config - Validation rules
* @example
* <FormValidator config={emailRules}>
*/
class FormValidator extends React.Component {
// ...
}Key principle: Treat TODO comments as temporary debt with clear ownership. Use automated tooling and team processes to prevent accumulation of stale tasks [2][4]. In React components, prioritize code clarity through proper component decomposition over excessive commenting [3][6].
Citations:
- 1: https://dev.to/adammc331/todo-write-a-better-comment-4c8c
- 2: https://github.com/sindresorhus/eslint-plugin-unicorn/blob/main/docs/rules/expiring-todo-comments.md
- 3: https://dev.to/sathishskdev/part-4-writing-clean-and-efficient-react-code-best-practices-and-optimization-techniques-423d
- 4: https://www.jetbrains.com/help/webstorm/using-todo.html
- 5: https://www.youtube.com/watch?v=LoYbN6qoQHA
- 6: https://developer.mozilla.org/en-US/docs/Learn_web_development/Core/Frameworks_libraries/React_todo_list_beginning
Action Required: Update the TODO comment for caching new rows
In file app/client/src/widgets/TableWidgetV2/component/TableBody/useCachingVirtualization.tsx (line 66), please replace the vague TODO with an actionable note. For example, specify how to handle the update of cached rows by either:
- Linking to an external issue/ticket (e.g., “// TODO: Refresh cached rows when new rows are added. See JIRA-123 for details.”), or
- Adding an expiration condition to enforce timely resolution (e.g., “// TODO(2025-12-31): Update cached rows when new rows are added.”)
These enhancements will help maintain code quality and ensure accountability.
|
/build-deploy-preview skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/13828840757. |
AmanAgarwal041
force-pushed
the
chore/caching-table-data
branch
from
98ca64e to
d739bd1
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
app/client/src/widgets/TableWidgetV2/component/types.ts (1)
83-83: Add usage detail or docstring for clarity.
A brief comment explaining the purpose ofisItemLoadedwould aid maintainability.app/client/src/widgets/TableWidgetV2/component/useCachingVirtualization.tsx (3)
5-14: Props interface definition.
Well-structured, but consider optional JSDoc for usage across the codebase.
71-75: Resetting cachedRows on infinite scroll toggle.
Consider preserving partial data if toggling infinite scroll is common, but this is acceptable.
108-138: Primary effect merging add-row logic.
Multiple nested conditions for row addition, partial pages, and empties. You may want to refactor for clearer flow.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (5)
app/client/src/widgets/TableWidgetV2/component/VirtualTable/InifiniteScrollBody/index.tsx(3 hunks)app/client/src/widgets/TableWidgetV2/component/index.tsx(4 hunks)app/client/src/widgets/TableWidgetV2/component/types.ts(1 hunks)app/client/src/widgets/TableWidgetV2/component/useCachingVirtualization.tsx(1 hunks)app/client/src/widgets/TableWidgetV2/widget/index.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- app/client/src/widgets/TableWidgetV2/component/index.tsx
⏰ 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-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
🔇 Additional comments (21)
app/client/src/widgets/TableWidgetV2/widget/index.tsx (1)
1960-1961: Validate edge cases for rowIndex=0.
Ifrowis not defined whenrowIndex=0, verify that the fallback to-1won't break downstream references.app/client/src/widgets/TableWidgetV2/component/VirtualTable/InifiniteScrollBody/index.tsx (6)
1-1: New imports for improved typing.
These additions are consistent; no concerns.
7-7: Use of useAppsmithTable hook.
Switching to a shared context hook is a clean approach to consolidate table state.
17-17: Synchronize isItemLoaded usage.
Confirm that the returned value fromisItemLoadedmatches InfiniteLoader’s pagination triggers.Also applies to: 25-25
26-28: Memoized itemCount calculation.
This is a suitable place to use memoization to avoid excessive recalculations.
35-35: Delegating loadMoreItems to nextPageClick.
Double-check that InfiniteLoader’s load callback properly handles partial loads and page boundaries.
47-47: Passing rows directly.
Ensure droppingcachedRowswon’t affect any interim caching and confirm the updated behavior remains correct for infinite scroll.app/client/src/widgets/TableWidgetV2/component/useCachingVirtualization.tsx (14)
1-4: Imports and setup.
Importingconcatis fine for array concatenation. No immediate issues.
16-24: Return interface definition.
The defined shape is straightforward and suitable for API clarity.
26-35: Hook declaration and args.
Clean function signature. Parameters align well with the table’s caching needs.
36-46: Local states and refs.
Check if simultaneous updates (e.g., row addition + page fetch) could lead to inconsistent data. Concurrency checks may be needed.
47-51: Storing editable cell reference.
Capturing editing state in a ref is consistent with typical React patterns.
52-56: maxPages computation.
Good approach for deriving the maximum number of pages from totalRecordsCount.
58-69: isItemLoaded callback logic.
Looks solid for skipping redundant loads. Ensure any partial empty pages are handled correctly.
77-79: isAddRowInProgress effect.
Straightforward sync of the ref with the component prop. No issues seen.
81-96: Editable cell effect.
Updates merged into the cached array. Check for index out-of-bounds if the array is smaller than the cell index.
98-106: Syncing editableCell with current row.
Approach is efficient. Confirm newly inserted data is validated before re-render.
139-143: Non-infinite assignment.
Assigning data directly is correct in non-infinite scroll mode.
145-148: Loading state effect.
Simple reflection ofisLoading. No issues apparent.
149-176: loadMoreNextPage function.
Check concurrency if multiple load requests come in quick succession. Otherwise logic is sound.
178-184: Returning final hook properties.
Clear return structure, well-named, and meets the table’s data management needs.
|
/build-deploy-preview skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/13837112386. |
|
Deploy-Preview-URL: https://ce-39703.dp.appsmith.com |
…ng data add to cached key
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
app/client/src/widgets/TableWidgetV2/widget/index.tsx (1)
932-935: Consider adding a reset mechanism for endOfData.Currently, I only see code that sets
endOfDatato true, but no logic to reset it back to false when new data might be available (e.g., after filter changes or query updates).Consider adding logic to reset
endOfDatato false when:
- Filters change
- Search text changes
- A new query is executed
// Example location to add reset logic (in updateFilters method) pushBatchMetaUpdates("filters", filters); + if(this.props.infiniteScrollEnabled) { + pushBatchMetaUpdates("endOfData", false); + }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (1)
app/client/src/widgets/TableWidgetV2/widget/index.tsx(6 hunks)
🧰 Additional context used
🧬 Code Definitions (1)
app/client/src/widgets/TableWidgetV2/widget/index.tsx (2)
app/client/src/widgets/TableWidgetV2/widget/derived.js (6)
tableData(201-201)tableData(1072-1072)pageSize(188-192)pageSize(1115-1115)row(98-98)row(992-992)app/client/src/widgets/TableWidgetV2/constants.ts (1)
ORIGINAL_INDEX_KEY(128-128)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: perform-test / client-build / client-build
- GitHub Check: client-unit-tests / client-unit-tests
- GitHub Check: client-check-cyclic-deps / check-cyclic-dependencies
- GitHub Check: client-lint / client-lint
- GitHub Check: client-build / client-build
🔇 Additional comments (7)
app/client/src/widgets/TableWidgetV2/widget/index.tsx (7)
232-233: Added new state properties for infinite scroll caching.These new state properties will enable caching table data for improved infinite scrolling performance.
902-936: Implemented caching mechanism in componentDidMount.The implementation populates the cache when the component mounts, storing table data by page number. It also correctly detects when we've reached the end of data by comparing the data length against page size.
1008-1027: Added caching mechanism during table data updates.This properly updates the cache when table data changes, ensuring consistency between the cache and current table state.
1031-1044: Added handling for infinite scroll toggle.The code handles the case when infinite scrolling is enabled after initialization, ensuring the cache is correctly populated.
1323-1323: Propagated endOfData status to ReactTableComponent.This ensures the table component knows when all data has been loaded.
2016-2018: Improved safety with optional chaining.The use of optional chaining (
?.) when accessingrow[ORIGINAL_INDEX_KEY]prevents potential null/undefined errors.
908-911:Details
✅ Verification successful
Verify that infinite scroll doesn't cause unnecessary re-renders.
While the caching implementation looks good, it's important to ensure it doesn't trigger unnecessary re-renders in the table component.
🏁 Script executed:
#!/bin/bash # Check if the component handles re-renders efficiently by looking for relevant memoization patterns echo "Checking for memoization patterns in the table component:" rg -A 5 "useMemo|memo\(|React\.memo|shouldComponentUpdate" app/client/src/widgets/TableWidgetV2/component/Length of output: 29051
Memoization Confirmed: Infinite Scroll Renders are Optimized
The inspection indicates that critical parts of the table component—including its VirtualTable and related cell components—are properly wrapped in
React.memoand leverageuseMemowhere applicable. These measures effectively ensure that infinite scroll updates do not trigger unnecessary re-renders. No further changes are needed at this time.
| row = filteredTableData[rowIndex - 1]; | ||
| originalIndex = rowIndex === 0 ? -1 : row[ORIGINAL_INDEX_KEY] ?? rowIndex; | ||
| originalIndex = | ||
| rowIndex === 0 ? -1 : row?.[ORIGINAL_INDEX_KEY] ?? rowIndex; |
There was a problem hiding this comment.
Unnecessary space here
There was a problem hiding this comment.
row[ORIGINAL_INDEX_KEY] -> row?.[ORIGINAL_INDEX_KEY] due to this change, new space / line is added. cc @jacquesikot
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
app/client/src/widgets/TableWidgetV2/widget/index.tsx (2)
3004-3034: Consider implementing cache size management.The
updateInfiniteScrollPropertiesmethod effectively caches table data, but lacks a mechanism to limit cache size. For large tables with many pages, this could lead to memory issues.Consider adding a cache eviction strategy to prevent unbounded growth of the
cachedTableDataobject:updateInfiniteScrollProperties() { const { cachedTableData, infiniteScrollEnabled, pageNo, pageSize, processedTableData, pushBatchMetaUpdates, tableData, totalRecordsCount, } = this.props; if (infiniteScrollEnabled) { // Update the cache key for a particular page whenever this function is called. The pageNo data is updated with the tableData. const updatedCachedTableData = { ...(cachedTableData || {}), [pageNo]: tableData, }; + // Limit cache size to avoid memory issues (e.g., keep only last 20 pages) + const MAX_CACHED_PAGES = 20; + const cachedPages = Object.keys(updatedCachedTableData).map(Number).sort((a, b) => b - a); + + if (cachedPages.length > MAX_CACHED_PAGES) { + // Remove oldest pages beyond the limit + cachedPages.slice(MAX_CACHED_PAGES).forEach(oldPage => { + delete updatedCachedTableData[oldPage]; + }); + } + pushBatchMetaUpdates("cachedTableData", updatedCachedTableData); // The check (!!totalRecordsCount && processedTableData.length === totalRecordsCount) is added if the totalRecordsCount property is set then match the length with the processedTableData which has all flatted data from each page in a single array except the current tableData page i.e. [ ...array of page 1 data, ...array of page 2 data ]. Another 'or' check is if (tableData.length < pageSize) when totalRecordsCount is undefined. Table data has a single page data and if the data comes out to be lesser than the pageSize, it is assumed that the data is finished. if ( (!!totalRecordsCount && processedTableData.length + tableData.length === totalRecordsCount) || tableData.length < pageSize ) { pushBatchMetaUpdates("endOfData", true); } } }
3025-3032: Consider adding a reset mechanism for endOfData.The code sets
endOfDatato true when certain conditions are met, but there's no mechanism to reset it to false when those conditions no longer apply (e.g., after data refresh or filter changes).Add a condition to reset
endOfDatawhen appropriate:if ( (!!totalRecordsCount && processedTableData.length + tableData.length === totalRecordsCount) || tableData.length < pageSize ) { pushBatchMetaUpdates("endOfData", true); +} else { + // Reset endOfData when conditions no longer apply + if (this.props.endOfData) { + pushBatchMetaUpdates("endOfData", false); + } }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (2)
app/client/src/widgets/TableWidgetV2/widget/derived.js(1 hunks)app/client/src/widgets/TableWidgetV2/widget/index.tsx(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- app/client/src/widgets/TableWidgetV2/widget/derived.js
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: perform-test / rts-build / build
- GitHub Check: perform-test / client-build / client-build
- GitHub Check: perform-test / server-build / server-unit-tests
- GitHub Check: client-unit-tests / client-unit-tests
- GitHub Check: client-lint / client-lint
- GitHub Check: client-check-cyclic-deps / check-cyclic-dependencies
- GitHub Check: client-build / client-build
- GitHub Check: client-prettier / prettier-check
🔇 Additional comments (5)
app/client/src/widgets/TableWidgetV2/widget/index.tsx (5)
232-233: Properties added for infinite scroll caching.These new properties support the caching layer for table widget data, which will improve performance when scrolling through pages.
918-918: Initializing cache on component mount.Calling
updateInfiniteScrollPropertiesincomponentDidMountensures the caching layer is properly initialized when the component is first mounted.
999-1011: Updating cache when table data changes or infinite scroll is enabled.This implementation correctly updates the cache when either the table data is modified or when the infinite scroll feature is toggled from disabled to enabled.
1290-1290: Passing endOfData to ReactTableComponent.This propagates the end-of-data state to the child component, which will use it to control infinite scrolling behavior.
1984-1985: Improved null safety with optional chaining.The change adds optional chaining when accessing
row[ORIGINAL_INDEX_KEY], which prevents potential runtime errors ifrowis undefined.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
app/client/src/widgets/TableWidgetV2/widget/index.tsx (1)
3004-3033: Implementation of updateInfiniteScrollProperties method.The implementation correctly:
- Updates the cache with current page data
- Determines end-of-data condition using either totalRecordsCount or pageSize comparison
Consider adding cache invalidation logic for when underlying data changes (sorting, filtering) to prevent stale data issues.
updateInfiniteScrollProperties() { const { cachedTableData, infiniteScrollEnabled, pageNo, processedTableData, pushBatchMetaUpdates, tableData, totalRecordsCount, + filters, + searchText, + sortOrder, } = this.props; if (infiniteScrollEnabled) { + // Create a cache key that includes filter/sort state to prevent stale data + const cacheKeyBase = `${pageNo}`; + const cacheKey = filters?.length || searchText || sortOrder?.column + ? `${cacheKeyBase}_${JSON.stringify(filters)}_${searchText}_${JSON.stringify(sortOrder)}` + : cacheKeyBase; + // Update the cache key for a particular page whenever this function is called. The pageNo data is updated with the tableData. const updatedCachedTableData = { ...(cachedTableData || {}), - [pageNo]: tableData, + [cacheKey]: tableData, }; pushBatchMetaUpdates("cachedTableData", updatedCachedTableData); // The check (!!totalRecordsCount && processedTableData.length === totalRecordsCount) is added if the totalRecordsCount property is set then match the length with the processedTableData which has all flatted data from each page in a single array except the current tableData page i.e. [ ...array of page 1 data, ...array of page 2 data ]. Another 'or' check is if (tableData.length < pageSize) when totalRecordsCount is undefined. Table data has a single page data and if the data comes out to be lesser than the pageSize, it is assumed that the data is finished. if ( (!!totalRecordsCount && processedTableData.length + tableData.length === totalRecordsCount) || tableData.length < pageSize ) { pushBatchMetaUpdates("endOfData", true); } } }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (1)
app/client/src/widgets/TableWidgetV2/widget/index.tsx(6 hunks)
🧰 Additional context used
🧬 Code Definitions (1)
app/client/src/widgets/TableWidgetV2/widget/index.tsx (2)
app/client/src/widgets/TableWidgetV2/widget/derived.js (7)
row(98-98)row(993-993)tableData(201-201)tableData(1073-1073)processedTableData(279-279)pageSize(188-192)pageSize(1116-1116)app/client/src/widgets/TableWidgetV2/constants.ts (1)
ORIGINAL_INDEX_KEY(128-128)
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: perform-test / client-build / client-build
- GitHub Check: perform-test / server-build / server-unit-tests
- GitHub Check: perform-test / rts-build / build
- GitHub Check: client-lint / client-lint
- GitHub Check: client-check-cyclic-deps / check-cyclic-dependencies
- GitHub Check: client-build / client-build
- GitHub Check: client-unit-tests / client-unit-tests
- GitHub Check: client-prettier / prettier-check
🔇 Additional comments (7)
app/client/src/widgets/TableWidgetV2/widget/index.tsx (7)
232-233: Added caching properties for infinite scroll functionality.The addition of
cachedTableDataandendOfDataproperties establish the foundation for the table widget's infinite scrolling capability, allowing the widget to cache loaded data and track when all data has been fetched.
918-918: Initialization of infinite scroll properties on component mount.This call ensures that caching is properly set up when the component is first mounted, which is essential for the infinite scroll functionality to work correctly from the start.
999-1001: Update cache when table data changes.This ensures the cache stays in sync with data changes, maintaining consistency for the infinite scroll experience.
1004-1011: Handle toggling of infinite scroll feature.Good defensive programming to update the cache when infinite scroll is enabled after previously being disabled. This prevents stale or missing data in the cache.
1290-1290: Pass endOfData flag to ReactTableComponent.Adding the
endOfDataprop to the component allows it to properly manage loading states and prevent unnecessary data fetch attempts when all data has been loaded.
1984-1985: Fixed potential null reference with optional chaining.The addition of optional chaining (
?.) inrow?.[ORIGINAL_INDEX_KEY]prevents potential runtime errors when accessing properties of a possibly undefinedrowobject.
3024-3030:Details
❓ Verification inconclusive
Verify that endOfData logic is robust for all edge cases.
The current implementation has good logic for detecting end of data, but verify it works correctly in these scenarios:
- When users filter/sort data after scrolling through multiple pages
- When
totalRecordsCountis updated asynchronously after data loads- When partial page data is returned but more data exists
Run this script to check for any places where the
endOfDataflag might be reset when filters change:
🏁 Script executed:
#!/bin/bash # Check if endOfData is reset when filters change rg -A 10 -B 10 "filters.*endOfData" --glob "*.{js,jsx,ts,tsx}"Length of output: 64
Verify End-of-Data Flag Handling in Edge Scenarios
The current logic appears sound at a glance for typical pagination, but please double-check the following edge cases manually:
- Verify that filtering/sorting after scrolling through multiple pages does not inadvertently reset or preserve the previous
endOfDatastate.- Confirm that asynchronous updates to
totalRecordsCountdo not lead to premature or delayed triggering of the end-of-data flag.- Ensure that partial page data (when more data exists) isn’t mistakenly marked as "end of data."
Note: The recent search for
filters.*endOfDataproduced no output. Given this and the asynchronous nature of data updates, please perform a manual walkthrough or additional targeted tests to confirm that no code path unexpectedly resetsendOfData.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
app/client/src/widgets/TableWidgetV2/widget/index.tsx (1)
3004-3034: Well implemented caching mechanism for infinite scroll.The
updateInfiniteScrollPropertiesmethod effectively:
- Updates the cache for the current page
- Determines when the end of data has been reached
Consider adding validation for edge cases like empty tableData or invalid pageNo values.
updateInfiniteScrollProperties() { const { cachedTableData, infiniteScrollEnabled, pageNo, pageSize, processedTableData, pushBatchMetaUpdates, tableData, totalRecordsCount, } = this.props; + // Skip cache update if not enabled or invalid data/page + if (!infiniteScrollEnabled || !tableData || !Array.isArray(tableData) || pageNo < 1) { + return; + } - if (infiniteScrollEnabled) { // Update the cache key for a particular page whenever this function is called. The pageNo data is updated with the tableData. const updatedCachedTableData = { ...(cachedTableData || {}), [pageNo]: tableData, }; pushBatchMetaUpdates("cachedTableData", updatedCachedTableData); // The check (!!totalRecordsCount && processedTableData.length === totalRecordsCount) is added if the totalRecordsCount property is set then match the length with the processedTableData which has all flatted data from each page in a single array except the current tableData page i.e. [ ...array of page 1 data, ...array of page 2 data ]. Another 'or' check is if (tableData.length < pageSize) when totalRecordsCount is undefined. Table data has a single page data and if the data comes out to be lesser than the pageSize, it is assumed that the data is finished. if ( (!!totalRecordsCount && processedTableData.length + tableData.length === totalRecordsCount) || tableData.length < pageSize ) { pushBatchMetaUpdates("endOfData", true); + } else { + // Ensure endOfData is reset when we're not at the end + pushBatchMetaUpdates("endOfData", false); } - } }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (1)
app/client/src/widgets/TableWidgetV2/widget/index.tsx(6 hunks)
🧰 Additional context used
🧬 Code Definitions (1)
app/client/src/widgets/TableWidgetV2/widget/index.tsx (2)
app/client/src/widgets/TableWidgetV2/widget/derived.js (7)
row(98-98)row(993-993)tableData(201-201)tableData(1073-1073)processedTableData(279-279)pageSize(188-192)pageSize(1116-1116)app/client/src/widgets/TableWidgetV2/constants.ts (1)
ORIGINAL_INDEX_KEY(128-128)
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: perform-test / client-build / client-build
- GitHub Check: perform-test / server-build / server-unit-tests
- GitHub Check: perform-test / rts-build / build
- GitHub Check: client-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
🔇 Additional comments (5)
app/client/src/widgets/TableWidgetV2/widget/index.tsx (5)
232-233: Good addition of cache-related properties to default state.The new properties
cachedTableDataandendOfDataare correctly initialized with appropriate default values.
918-918: LGTM, initializing cache properties in componentDidMount.The call to
updateInfiniteScrollProperties()incomponentDidMountensures the cache is properly initialized when the component is first mounted.
999-1011: Well implemented cache updates for table data.The code correctly updates the caching layer when tableData changes and handles the case when infiniteScrollEnabled is toggled from false to true.
1290-1290: Properly passing endOfData to ReactTableComponent.The
endOfDataproperty is correctly passed down to the ReactTableComponent, which is necessary for the infinite scroll functionality.
1984-1985: Good defensive programming with optional chaining.The use of optional chaining when accessing
row?.[ORIGINAL_INDEX_KEY]prevents potential runtime errors ifrowis undefined.
AmanAgarwal041
added
ok-to-test
rahulbarwal
left a comment
rahulbarwal
left a comment
There was a problem hiding this comment.
Minor changes requested.
| } | ||
| }; | ||
|
|
||
| updateInfiniteScrollProperties() { |
There was a problem hiding this comment.
As the method finishes executing whenever we are using pushBatchMetaUpdates we have to call commitBatchMetaUpdates as well.
There was a problem hiding this comment.
Updated the changes for adding commitBatchMetaUpdates only on componentDidMount. @rahulbarwal
| frozenColumnIndices: { a: 1 }, | ||
| infiniteScrollEnabled: false, | ||
| endOfData: false, | ||
| cachedTableData: {}, |
There was a problem hiding this comment.
We should add some unit tests for this new cachedTableData property in this unit test file.
There was a problem hiding this comment.
Added test file for the getProcessedTableData which uses cachedTableData to return response when the infinite scroll is enabled. @rahulbarwal
|
❤️ this @AmanAgarwal041 . |
… scroll, added getProcessedTableData test case
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
app/client/src/widgets/TableWidgetV2/widget/index.tsx (1)
3011-3048: Implementation of the caching layer for infinite scroll.This method effectively:
- Updates the cache for the current page
- Determines when the end of data has been reached using two strategies:
- Comparing processed data length with totalRecordsCount
- Checking if current page data is less than pageSize
The implementation is robust, handling both when totalRecordsCount is known and when it's not available.
However, the condition to determine if we've reached the end of data is complex. Consider extracting this logic into a separate helper method for improved readability and maintainability.
- if ( - (!!totalRecordsCount && - processedTableData.length + tableData.length === totalRecordsCount) || - (!totalRecordsCount && tableData.length < pageSize) - ) { - pushBatchMetaUpdates("endOfData", true); - } else { - pushBatchMetaUpdates("endOfData", false); - } + const hasReachedEndOfData = + (!!totalRecordsCount && + processedTableData.length + tableData.length === totalRecordsCount) || + (!totalRecordsCount && tableData.length < pageSize); + + pushBatchMetaUpdates("endOfData", hasReachedEndOfData);
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (3)
app/client/src/widgets/TableWidgetV2/component/VirtualTable/BaseVirtualList.tsx(1 hunks)app/client/src/widgets/TableWidgetV2/widget/__tests__/derived.test/getProcessedTableData.test.js(1 hunks)app/client/src/widgets/TableWidgetV2/widget/index.tsx(6 hunks)
🧰 Additional context used
🧬 Code Definitions (1)
app/client/src/widgets/TableWidgetV2/widget/index.tsx (2)
app/client/src/widgets/TableWidgetV2/widget/derived.js (7)
row(98-98)row(993-993)tableData(201-201)tableData(1073-1073)processedTableData(279-279)pageSize(188-192)pageSize(1116-1116)app/client/src/widgets/TableWidgetV2/constants.ts (1)
ORIGINAL_INDEX_KEY(128-128)
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: perform-test / rts-build / build
- GitHub Check: perform-test / client-build / client-build
- GitHub Check: perform-test / server-build / server-unit-tests
- GitHub Check: client-unit-tests / client-unit-tests
- GitHub Check: client-lint / client-lint
- GitHub Check: client-check-cyclic-deps / check-cyclic-dependencies
- GitHub Check: client-prettier / prettier-check
- GitHub Check: client-build / client-build
🔇 Additional comments (7)
app/client/src/widgets/TableWidgetV2/component/VirtualTable/BaseVirtualList.tsx (1)
150-150: AddedhasMoreDatato useMemo dependencies to ensure proper re-rendering.The addition of
hasMoreDatato the dependency array ensures that the memoizedrowRendererfunction re-computes when thehasMoreDataprop changes. This is necessary becausehasMoreDatais used within the rendered component, and without this dependency, the UI might not reflect the correct state whenhasMoreDatachanges.app/client/src/widgets/TableWidgetV2/widget/__tests__/derived.test/getProcessedTableData.test.js (1)
1-64: Comprehensive test cases for the new caching mechanism.The test file effectively validates both scenarios for the
getProcessedTableDatafunction:
- When
infiniteScrollEnabledis false, verifying only the currenttableDatais returned- When
infiniteScrollEnabledis true, confirming that data from multiple cached pages is combined correctlyThe test structure is clean and focused on the specific functionality being implemented.
app/client/src/widgets/TableWidgetV2/widget/index.tsx (5)
232-233: Added new state properties for caching implementation.The addition of
cachedTableDataandendOfDataproperties to the default state properly sets up the foundation for the caching layer and infinite scroll functionality.
918-919: Initializing caching layer on component mount.Correctly initializes the caching mechanism when the component mounts, with the
trueparameter ensuring batch updates are committed sincecommitBatchMetaUpdatesis not automatically called duringcomponentDidMount.
991-1019: Added cache update logic in componentDidUpdate.The implementation properly updates the cache:
- When table data changes
- When infinite scroll is toggled from disabled to enabled
This ensures the cached data stays in sync with the current view and reacts appropriately to configuration changes.
1991-1992: Added null check with optional chaining.Good defensive programming practice to prevent potential null reference errors if
rowis undefined in certain edge cases.
1297-1297: Added endOfData prop to ReactTableComponent.Properly passes the
endOfDatastate to the component, enabling downstream components to adjust their rendering based on whether more data is available.
AmanAgarwal041
added
ok-to-test
3 participants
Description
Added a new property to table properties i.e.
cachedTableDatawhich will cache the loaded pages data and in case of infinite scroll table, we would be able to view all the loaded pages by scroll back and forth.This caching is based on the pageNumber, i.e.
cachedTableDataproperty will be an object with key as apageNoand value as the array of rows.By just checking the flag infiniteScrollEnabled we would be processing data on
cachedTableDataproperty ortableDataproperty as per the valuetrueorfalserespectively.Fixes #39085
Fixes #39814
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.All"
🔍 Cypress test results
Tip
🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/14213335215
Commit: 51faddb
Cypress dashboard.
Tags:
@tag.AllSpec:
Wed, 02 Apr 2025 08:07:14 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
Summary by CodeRabbit
New Features
endOfData).cachedTableData) for better performance.Bug Fixes
Refactor