feat: Introduce infinite scroll feature behind feature flag. #39207
Conversation
- Adding `infiniteScrollEnabled` prop to TableWidgetProps
WalkthroughThis pull request introduces a new feature flag ( Changes
Sequence Diagram(s)sequenceDiagram
participant Widget as TableWidgetV2
participant Table as Table Component
participant VT as VirtualTable
participant TB as TableBody
Widget->>Table: Pass isInfiniteScrollEnabled flag
alt Infinite Scroll Enabled
Table->>VT: Render VirtualTable with isInfiniteScrollEnabled=true
VT->>TB: Forward isInfiniteScrollEnabled prop
TB->>Widget: Render infinite scroll UI
else
Table->>Widget: Render static table view
end
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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
|
|
/build-deploy-preview skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/13278951380. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (2)
app/client/src/widgets/TableWidgetV2/widget/index.tsx (1)
190-235: 🛠️ Refactor suggestionAdd default value for infiniteScrollEnabled property.
The
infiniteScrollEnabledproperty should be initialized in thegetDefaults()method to ensure consistent behavior.Apply this diff to add the default value:
static getDefaults() { return { flexVerticalAlignment: FlexVerticalAlignment.Top, responsiveBehavior: ResponsiveBehavior.Fill, minWidth: FILL_WIDGET_MIN_WIDTH, rows: 28, canFreezeColumn: true, columnUpdatedAt: Date.now(), columns: 34, animateLoading: true, defaultSelectedRowIndex: 0, defaultSelectedRowIndices: [0], label: "Data", widgetName: "Table", searchKey: "", textSize: "0.875rem", horizontalAlignment: "LEFT", verticalAlignment: "CENTER", totalRecordsCount: 0, defaultPageSize: 0, dynamicPropertyPathList: [], borderColor: Colors.GREY_5, borderWidth: "1", dynamicBindingPathList: [], primaryColumns: {}, tableData: "", columnWidthMap: {}, columnOrder: [], enableClientSideSearch: true, isVisibleSearch: true, isVisibleFilters: false, isVisibleDownload: true, isVisiblePagination: true, isSortable: true, delimiter: ",", version: 2, inlineEditingSaveOption: InlineEditingSaveOptions.ROW_LEVEL, + infiniteScrollEnabled: false, enableServerSideFiltering: TableWidgetV2.getFeatureFlag( ALLOW_TABLE_WIDGET_SERVER_SIDE_FILTERING, ) ? false : undefined, customIsLoading: false, customIsLoadingValue: "", }; }app/client/src/widgets/TableWidgetV2/component/index.tsx (1)
289-346:⚠️ Potential issueAdd memo comparison for the new infinite scroll prop.
The
React.memocomparison function should include the newisInfiniteScrollEnabledprop to prevent unnecessary re-renders.export default React.memo(ReactTableComponent, (prev, next) => { return ( prev.applyFilter === next.applyFilter && prev.compactMode === next.compactMode && prev.delimiter === next.delimiter && + prev.isInfiniteScrollEnabled === next.isInfiniteScrollEnabled && // ... rest of the comparisons prev.showConnectDataOverlay === next.showConnectDataOverlay ); });
🧹 Nitpick comments (2)
app/client/src/widgets/TableWidgetV2/widget/propertyConfig/contentConfig.ts (1)
177-185: Improve the help text for infinite scroll property.The current help text appears to be copied from the pagination property. Consider updating it to better describe the infinite scroll behavior and its implications.
- helpText: - "Bind the Table.pageNo property in your API and call it onPageChange", + helpText: + "Enable infinite scrolling to automatically load more data as the user scrolls to the bottom of the table",app/client/src/widgets/TableWidgetV2/constants.ts (1)
251-252: Consider adding JSDoc comment for the feature flag constant.While the constant follows the established pattern, adding documentation would help other developers understand its purpose and impact.
+/** Feature flag to enable infinite scrolling functionality in TableWidgetV2 */ export const INFINITE_SCROLL_ENABLED = FEATURE_FLAG["release_tablev2_infinitescroll_enabled"];
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
app/client/src/ce/entities/FeatureFlag.ts(2 hunks)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/index.tsx(3 hunks)app/client/src/widgets/TableWidgetV2/component/VirtualTable.tsx(2 hunks)app/client/src/widgets/TableWidgetV2/component/index.tsx(3 hunks)app/client/src/widgets/TableWidgetV2/constants.ts(2 hunks)app/client/src/widgets/TableWidgetV2/widget/index.tsx(1 hunks)app/client/src/widgets/TableWidgetV2/widget/propertyConfig/contentConfig.ts(3 hunks)
🔇 Additional comments (9)
app/client/src/widgets/TableWidgetV2/component/VirtualTable.tsx (1)
40-40: LGTM! Clean prop addition and passing.The implementation correctly adds and propagates the infinite scroll property.
Also applies to: 63-63
app/client/src/widgets/TableWidgetV2/component/StaticTable.tsx (1)
84-84: LGTM! Appropriate hardcoding for static table.Correctly disables infinite scroll for static table implementation.
app/client/src/ce/entities/FeatureFlag.ts (1)
57-58: LGTM! Feature flag follows conventions.The feature flag is correctly defined and initialized with a safe default value of false.
Also applies to: 104-104
app/client/src/widgets/TableWidgetV2/component/TableBody/index.tsx (2)
82-82: LGTM! Clean prop type addition.The property is correctly typed in the BodyPropsType interface.
197-197: Verify prop passing in child components.The isInfiniteScrollEnabled prop is being passed as false to both TableVirtualBodyComponent and TableBodyComponent, but these components don't declare this prop in their interfaces.
Also applies to: 205-206
❌ Incorrect review comment
🏁 Script executed:
#!/bin/bash # Check if these components declare the prop in their interfaces ast-grep --pattern 'interface $_ { $$$ isInfiniteScrollEnabled: boolean; $$$ }'Length of output: 14506
Prop Declaration Confirmed in Component Interfaces
- In app/client/src/widgets/TableWidgetV2/component/TableBody/index.tsx, the
BodyPropsTypeinterface declaresisInfiniteScrollEnabled: boolean;(line 82).- In app/client/src/widgets/TableWidgetV2/component/Table.tsx, the interface
TablePropsdeclaresisInfiniteScrollEnabled: boolean;(line 137).Based on the grep results, both child components correctly declare the property, so no further changes are needed.
Likely an incorrect or invalid review comment.
app/client/src/widgets/TableWidgetV2/component/Table.tsx (1)
338-345: LGTM!The virtual scrolling logic is correctly implemented with the new infinite scroll feature. The props are properly passed to the appropriate components, and the conditions for enabling virtual scrolling are well-defined.
Also applies to: 492-492, 514-514
app/client/src/widgets/TableWidgetV2/constants.ts (1)
114-114: LGTM: Property addition follows TypeScript conventions.The
infiniteScrollEnabledboolean property is correctly added to theTableWidgetPropsinterface.app/client/src/widgets/TableWidgetV2/component/index.tsx (2)
109-109: LGTM: Property addition follows React prop typing conventions.The
isInfiniteScrollEnabledboolean property is correctly added to theReactTableComponentPropsinterface.
135-135: LGTM: Proper prop destructuring and passing.The infinite scroll property is correctly destructured from props and passed down to the Table component.
Also applies to: 251-251
| {isInfiniteScrollEnabled ? ( | ||
| <div>Infinite Scroll</div> | ||
| ) : useVirtual ? ( |
There was a problem hiding this comment.
Placeholder needs implementation.
The current implementation only shows a placeholder div. This needs to be replaced with actual infinite scroll functionality.
Would you like me to help create a proper infinite scroll implementation using react-window's FixedSizeList with onScroll handling?
| @@ -1310,6 +1310,7 @@ class TableWidgetV2 extends BaseWidget<TableWidgetProps, WidgetState> { | |||
| height={componentHeight} | |||
| isAddRowInProgress={this.props.isAddRowInProgress} | |||
| isEditableCellsValid={this.props.isEditableCellsValid} | |||
| isInfiniteScrollEnabled={this.props.infiniteScrollEnabled} | |||
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify interaction with pagination controls.
The infinite scroll feature might affect the visibility and behavior of pagination controls. Ensure that pagination controls are hidden when infinite scroll is enabled.
🏁 Script executed:
#!/bin/bash
# Description: Check if pagination controls are properly handled with infinite scroll.
# Search for pagination visibility logic
rg -A 5 "isVisiblePagination" Length of output: 105089
Action Required: Hide Pagination Controls When Infinite Scroll Is Enabled
The DSL fixtures show that the pagination flag ("isVisiblePagination") is always set to true. There is no apparent logic in the widget that conditionally hides pagination when the infinite scroll feature (mapped via the prop "infiniteScrollEnabled") is enabled. You should ensure that the widget's runtime behavior overrides the DSL setting (or updates it accordingly) so that pagination controls are hidden when infinite scroll is active.
- Check and update the conditional rendering logic in the widget (e.g., in TableWidgetV2) to disable pagination when infiniteScrollEnabled is true.
- Update or transform the DSL configuration at runtime to reflect that pagination should be hidden if infinite scrolling is enabled.
rahulbarwal
changed the title
…o rahulbarwal/issue39079/Feature-flag-to-control-visibility-of-this-feature
github-actions
bot
added
Query & Widgets Pod
|
/build-deploy-preview skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/13282434340. |
|
Deploy-Preview-URL: https://ce-39207.dp.appsmith.com |
| @@ -81,6 +81,7 @@ const StaticTable = (props: StaticTableProps, ref: React.Ref<SimpleBar>) => { | |||
| getTableBodyProps={props.getTableBodyProps} | |||
| height={props.height} | |||
| isAddRowInProgress={props.isAddRowInProgress} | |||
| isInfiniteScrollEnabled={false} | |||
There was a problem hiding this comment.
As we have added the feature flag, shouldn't we make use of that here instead of making it false always ?
There was a problem hiding this comment.
Static table is never going to use infinite scroll. So basically the idea behind infinite scroll is that it will always use virtual table.
Why do we have to pass this when static table is not using this variable? So this is just baggage that we have from earlier structuring of code. I am planning to handle this in my next PR where I will try to do a refactor if possible.
There was a problem hiding this comment.
If this prop is a new prop then we can mark this as the optional prop which would be undefined by default which will solve the purpose instead of passing false to this prop explicitly.
There was a problem hiding this comment.
This is what I am forced to do right now but I don't like it to be honest. Ideally I should not have to write any piece of code that I don't need.
If instead of hard coding it to false, if I give it a variable that means that the JS compiler has to evaluate that value before passing it down here. Which in this case might be a moot point but still in principle I should not have to write what I don't need.
What I am proposing is eliminating this unnecessary code in a refactor PR.
There was a problem hiding this comment.
We met over a call and agreed to make this an optional argument and deal with this whole component structure in a separate PR.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
app/client/src/widgets/TableWidgetV2/component/TableBody/index.tsx (1)
193-195:⚠️ Potential issuePlaceholder needs implementation.
The current implementation only shows a placeholder div. This needs to be replaced with actual infinite scroll functionality.
Would you like me to help create a proper infinite scroll implementation using react-window's FixedSizeList with onScroll handling?
🧹 Nitpick comments (1)
app/client/src/widgets/TableWidgetV2/component/TableBody/index.tsx (1)
196-202: Consider removing redundant prop passing.The
isInfiniteScrollEnabled={false}prop is being explicitly passed to child components that don't use it. This creates unnecessary prop drilling.<TableVirtualBodyComponent - isInfiniteScrollEnabled={false} ref={ref} rows={rows} width={width} {...restOfProps} /> <TableBodyComponent - isInfiniteScrollEnabled={false} rows={rows} {...restOfProps} />Also applies to: 204-208
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app/client/src/widgets/TableWidgetV2/component/TableBody/index.tsx(3 hunks)
⏰ 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 (1)
app/client/src/widgets/TableWidgetV2/component/TableBody/index.tsx (1)
82-82: LGTM!Clean addition of the optional boolean prop to the interface.
rahulbarwal
deleted the
rahulbarwal/issue39079/Feature-flag-to-control-visibility-of-this-feature
branch
Description
infiniteScrollEnabledprop to TableWidgetPropsrelease_tablev2_infinitescroll_enabledto control this.Fixes #39079
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"
🔍 Cypress test results
Tip
🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/13305863325
Commit: 182c03d
Cypress dashboard.
Tags:
@tag.Table, @tag.SanitySpec:
Thu, 13 Feb 2025 11:35:05 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit