Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Introduce infinite scroll feature behind feature flag. #39207

Merged
merged 5 commits into from
Feb 14, 2025
Merged

feat: Introduce infinite scroll feature behind feature flag. #39207

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
4b8cfce
feat: Add infinite scroll feature flag for TableWidgetV2
rahulbarwal Feb 10, 2025
6a15d81
feat: Add infinite scroll support for TableWidgetV2
rahulbarwal Feb 10, 2025
982dfce
Merge branch 'release' of https://github.com/appsmithorg/appsmith int…
rahulbarwal Feb 12, 2025
037e613
Add missing argument to unit test.
rahulbarwal Feb 12, 2025
182c03d
refactor: Make infinite scroll optional in table widget
rahulbarwal Feb 13, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions app/client/src/ce/entities/FeatureFlag.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,8 @@ export const FEATURE_FLAG = {
release_ads_entity_item_enabled: "release_ads_entity_item_enabled",
release_external_saas_plugins_enabled:
"release_external_saas_plugins_enabled",
release_tablev2_infinitescroll_enabled:
"release_tablev2_infinitescroll_enabled",
} as const;

export type FeatureFlag = keyof typeof FEATURE_FLAG;
Expand Down Expand Up @@ -96,6 +98,7 @@ export const DEFAULT_FEATURE_FLAG_VALUE: FeatureFlags = {
config_mask_session_recordings_enabled: true,
release_ads_entity_item_enabled: false,
release_external_saas_plugins_enabled: false,
release_tablev2_infinitescroll_enabled: false,
};

export const AB_TESTING_EVENT_KEYS = {
Expand Down
1 change: 1 addition & 0 deletions app/client/src/widgets/TableWidgetV2/component/StaticTable.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,7 @@ const StaticTable = (props: StaticTableProps, ref: React.Ref<SimpleBar>) => {
getTableBodyProps={props.getTableBodyProps}
height={props.height}
isAddRowInProgress={props.isAddRowInProgress}
isInfiniteScrollEnabled={false}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As we have added the feature flag, shouldn't we make use of that here instead of making it false always ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We met over a call and agreed to make this an optional argument and deal with this whole component structure in a separate PR.

multiRowSelection={!!props.multiRowSelection}
pageSize={props.pageSize}
prepareRow={props.prepareRow}
Expand Down
17 changes: 10 additions & 7 deletions app/client/src/widgets/TableWidgetV2/component/Table.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -134,6 +134,7 @@ export interface TableProps {
canFreezeColumn?: boolean;
showConnectDataOverlay: boolean;
onConnectData: () => void;
isInfiniteScrollEnabled: boolean;
}

const defaultColumn = {
Expand Down Expand Up @@ -335,12 +336,13 @@ export function Table(props: TableProps) {
* for server paginated tables it needs this extra handling.
*/
const shouldUseVirtual =
props.serverSidePaginationEnabled &&
!props.columns.some(
(column) =>
!!column.columnProperties.allowCellWrapping ||
column.metaProperties?.type === ColumnTypes.HTML,
);
props.isInfiniteScrollEnabled ||
(props.serverSidePaginationEnabled &&
!props.columns.some(
(column) =>
!!column.columnProperties.allowCellWrapping ||
column.metaProperties?.type === ColumnTypes.HTML,
));

useEffect(() => {
if (props.isAddRowInProgress) {
Expand Down Expand Up @@ -487,7 +489,7 @@ export function Table(props: TableProps) {
subPage={subPage}
tableSizes={tableSizes}
totalColumnsWidth={totalColumnsWidth}
useVirtual={shouldUseVirtual}
useVirtual={false}
widgetId={props.widgetId}
width={props.width}
/>
Expand All @@ -509,6 +511,7 @@ export function Table(props: TableProps) {
headerGroups={headerGroups}
height={props.height}
isAddRowInProgress={props.isAddRowInProgress}
isInfiniteScrollEnabled={props.isInfiniteScrollEnabled}
isResizingColumn={isResizingColumn}
isSortable={props.isSortable}
multiRowSelection={props?.multiRowSelection}
Expand Down
13 changes: 11 additions & 2 deletions app/client/src/widgets/TableWidgetV2/component/TableBody/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,7 @@ interface BodyPropsType {
width?: number;
tableSizes: TableSizes;
innerElementType?: ReactElementType;
isInfiniteScrollEnabled: boolean;
}

const TableVirtualBodyComponent = React.forwardRef(
Expand Down Expand Up @@ -137,6 +138,7 @@ export const TableBody = React.forwardRef(
handleReorderColumn,
headerGroups,
isAddRowInProgress,
isInfiniteScrollEnabled,
isResizingColumn,
isSortable,
multiRowSelection,
Expand Down Expand Up @@ -188,15 +190,22 @@ export const TableBody = React.forwardRef(
totalColumnsWidth: props.totalColumnsWidth,
}}
>
{useVirtual ? (
{isInfiniteScrollEnabled ? (
<div>Infinite Scroll</div>
) : useVirtual ? (
Comment on lines +193 to +195
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

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?

<TableVirtualBodyComponent
isInfiniteScrollEnabled={false}
ref={ref}
rows={rows}
width={width}
{...restOfProps}
/>
) : (
<TableBodyComponent rows={rows} {...restOfProps} />
<TableBodyComponent
isInfiniteScrollEnabled={false}
rows={rows}
{...restOfProps}
/>
)}
</BodyContext.Provider>
);
Expand Down
2 changes: 2 additions & 0 deletions app/client/src/widgets/TableWidgetV2/component/VirtualTable.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@ type VirtualTableProps = TableColumnHeaderProps & {
// eslint-disable-next-line @typescript-eslint/no-explicit-any
scrollContainerStyles: any;
useVirtual: boolean;
isInfiniteScrollEnabled: boolean;
};

const VirtualTable = (props: VirtualTableProps, ref: React.Ref<SimpleBar>) => {
Expand All @@ -59,6 +60,7 @@ const VirtualTable = (props: VirtualTableProps, ref: React.Ref<SimpleBar>) => {
height={props.height}
innerElementType={VirtualTableInnerElement}
isAddRowInProgress={props.isAddRowInProgress}
isInfiniteScrollEnabled={props.isInfiniteScrollEnabled}
isResizingColumn={props.isResizingColumn}
isSortable={props.isSortable}
multiRowSelection={!!props.multiRowSelection}
Expand Down
3 changes: 3 additions & 0 deletions app/client/src/widgets/TableWidgetV2/component/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -106,6 +106,7 @@ interface ReactTableComponentProps {
canFreezeColumn?: boolean;
showConnectDataOverlay: boolean;
onConnectData: () => void;
isInfiniteScrollEnabled: boolean;
}

function ReactTableComponent(props: ReactTableComponentProps) {
Expand All @@ -131,6 +132,7 @@ function ReactTableComponent(props: ReactTableComponentProps) {
handleResizeColumn,
height,
isAddRowInProgress,
isInfiniteScrollEnabled,
isLoading,
isSortable,
isVisibleDownload,
Expand Down Expand Up @@ -246,6 +248,7 @@ function ReactTableComponent(props: ReactTableComponentProps) {
handleResizeColumn={handleResizeColumn}
height={height}
isAddRowInProgress={isAddRowInProgress}
isInfiniteScrollEnabled={isInfiniteScrollEnabled}
isLoading={isLoading}
isSortable={isSortable}
isVisibleDownload={isVisibleDownload}
Expand Down
4 changes: 4 additions & 0 deletions app/client/src/widgets/TableWidgetV2/constants.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -111,6 +111,7 @@ export interface TableWidgetProps
onTableFilterUpdate: string;
customIsLoading: boolean;
customIsLoadingValue: boolean;
infiniteScrollEnabled: boolean;
}

export enum TableVariantTypes {
Expand Down Expand Up @@ -243,3 +244,6 @@ export const DEFAULT_COLUMN_NAME = "Table Column";

export const ALLOW_TABLE_WIDGET_SERVER_SIDE_FILTERING =
FEATURE_FLAG["release_table_serverside_filtering_enabled"];

export const INFINITE_SCROLL_ENABLED =
FEATURE_FLAG["release_tablev2_infinitescroll_enabled"];
1 change: 1 addition & 0 deletions app/client/src/widgets/TableWidgetV2/widget/__tests__/TableRendered.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -107,6 +107,7 @@ describe("TableWidgetV2 getWidgetView", () => {
allowAddNewRow: false,
defaultNewRow: {},
frozenColumnIndices: { a: 1 },
infiniteScrollEnabled: false,
};

describe("TableWidgetV2 loading checks", () => {
Expand Down
1 change: 1 addition & 0 deletions app/client/src/widgets/TableWidgetV2/widget/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1271,6 +1271,7 @@ class TableWidgetV2 extends BaseWidget<TableWidgetProps, WidgetState> {
height={componentHeight}
isAddRowInProgress={this.props.isAddRowInProgress}
isEditableCellsValid={this.props.isEditableCellsValid}
isInfiniteScrollEnabled={this.props.infiniteScrollEnabled}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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.

isLoading={
customIsLoading
? customIsLoadingValue || this.props.isLoading
Expand Down
13 changes: 13 additions & 0 deletions app/client/src/widgets/TableWidgetV2/widget/propertyConfig/contentConfig.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ import {
} from "../propertyUtils";
import panelConfig from "./PanelConfig";
import Widget from "../index";
import { INFINITE_SCROLL_ENABLED } from "../../constants";

export default [
{
Expand Down Expand Up @@ -160,6 +161,8 @@ export default [
isBindProperty: true,
isTriggerProperty: false,
validation: { type: ValidationTypes.BOOLEAN },
hidden: (props: TableWidgetProps) => props.infiniteScrollEnabled,
dependencies: ["infiniteScrollEnabled"],
},
{
helpText:
Expand All @@ -170,6 +173,16 @@ export default [
isBindProperty: false,
isTriggerProperty: false,
},
{
helpText:
"Bind the Table.pageNo property in your API and call it onPageChange",
propertyName: "infiniteScrollEnabled",
label: "Infinite scroll",
controlType: "SWITCH",
isBindProperty: false,
isTriggerProperty: false,
hidden: () => !Widget.getFeatureFlag(INFINITE_SCROLL_ENABLED),
},
{
helpText: createMessage(TABLE_WIDGET_TOTAL_RECORD_TOOLTIP),
propertyName: "totalRecordsCount",
Expand Down
Loading