Skip to content

Commit

Permalink
fix: infinite render loop in List Widget when hiding/unhiding with se…
Browse files Browse the repository at this point in the history
…lected item (#39262)

## Description
When a List widget has a selected item and is hidden then unhidden
(unmounted/mounted), an infinite render loop occurs in the following
flow:

```typescript
// Triggered in componentDidUpdate
if (this.shouldUpdateSelectedItemAndView() && isString(this.props.selectedItemKey)) {
    this.updateSelectedItemAndPageOnResetOrMount();
}
```

### Root Cause
This happens because:

1. `updateSelectedItemAndPageOnResetOrMount` calls `updatePageNumber`.
2. `updatePageNumber` uses `calculatePageNumberFromRowIndex`, which
depends on `this.pageSize`.
3. Each page number calculation triggers a meta property update through
`onPageChange`.
4. This meta update causes a rerender, repeating the cycle.

#### Problematic Calculation
`this.pageSize` is calculated dynamically based on widget dimensions:

```typescript
calculatePageNumberFromRowIndex = (index: number) => {
    return Math.ceil((index + 1) / this.pageSize); // Using unstable this.pageSize
};
```

This creates instability during mount/unmount cycles as:

- Widget dimensions may not be fully stabilized.
- Each meta property update triggers recalculation.
- No break in the update-rerender cycle.

### Solution
Use `this.props.pageSize` instead of `this.pageSize` in page
calculations:

```typescript
calculatePageNumberFromRowIndex = (index: number) => {
    return Math.ceil((index + 1) / this.props.pageSize); // Using stable props value
};
```

### Why This Works
- `this.props.pageSize` represents the last stable value set through the
meta property system.
- It's protected by the existing `pageSizeUpdated` flag mechanism.
- Breaking the dependency on dynamically calculated dimensions prevents
the infinite loop.




Fixes #37683 

## Automation

/ok-to-test tags="@tag.Widget, @tag.Binding, @tag.List, @tag.Sanity"

### 🔍 Cypress test results
<!-- This is an auto-generated comment: Cypress test results  -->
> [!TIP]
> 🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
> Workflow run:
<https://github.com/appsmithorg/appsmith/actions/runs/13366216706>
> Commit: 74028ec
> <a
href="https://internal.appsmith.com/app/cypress-dashboard/rundetails-65890b3c81d7400d08fa9ee5?branch=master&workflowId=13366216706&attempt=1"
target="_blank">Cypress dashboard</a>.
> Tags: `@tag.Widget, @tag.Binding, @tag.List, @tag.Sanity`
> Spec:
> <hr>Mon, 17 Feb 2025 10:23:19 UTC
<!-- end of auto-generated comment: Cypress test results  -->


## Communication
Should the DevRel and Marketing teams inform users about this change?
- [ ] Yes
- [ ] No


<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->
## Summary by CodeRabbit

- **Bug Fixes**
- Improved the list widget’s pagination by updating how page numbers are
calculated, ensuring accurate reflection of the current page size
settings.

- **Tests**
- Enhanced test coverage for the list widget's visibility, adding new
tests to validate user interactions and ensuring the widget behaves as
expected when items are selected.
<!-- end of auto-generated comment: release notes by coderabbit.ai -->
  • Loading branch information
jacquesikot authored Feb 17, 2025
1 parent 5b9153c commit c0e073e
Show file tree
Hide file tree
Showing 2 changed files with 55 additions and 4 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,58 @@ describe(
deployMode.NavigateBacktoEditor();
});

it("2. Toggle JS - Validate isVisible", function () {
it("2. Validate isVisible when list has selected item (#37683)", () => {
// Define selectors for widgets
const widgetSelector = (name: string) => `[data-widgetname-cy="${name}"]`;
const containerWidgetSelector = `[type="CONTAINER_WIDGET"]`;

// Drag and drop the Button widget onto the canvas
// entityExplorer.DragDropWidgetNVerify("listwidgetv2", 300, 300);
entityExplorer.DragDropWidgetNVerify("buttonwidget");

// Set up the button to make the list visible when clicked
EditorNavigation.SelectEntityByName("Button1", EntityType.Widget);
propPane.EnterJSContext("onClick", `{{List1.setVisibility(true)}}`, true);

// Set up the list widget to become invisible when an item is clicked
EditorNavigation.SelectEntityByName("List1", EntityType.Widget);
propPane.EnterJSContext(
"onItemClick",
`{{List1.setVisibility(false)}}`,
true,
);

// Deploy the application to test the visibility functionality
deployMode.DeployApp();

// Simulate a click on the first item in the list to hide the list
agHelper
.GetElement(widgetSelector("List1"))
.find(containerWidgetSelector)
.first()
.click({ force: true });
agHelper.WaitUntilEleDisappear(
locators._widgetInDeployed(draggableWidgets.LIST_V2),
);

// Assert that the list widget is not visible after clicking an item
agHelper.AssertElementAbsence(
locators._widgetInDeployed(draggableWidgets.LIST_V2),
);

// Click the button to make the list visible again
agHelper.GetNClick(widgetSelector("Button1"));

// Assert that the list widget is visible after clicking the button
agHelper.AssertElementVisibility(
locators._widgetInDeployed(draggableWidgets.LIST_V2),
);

// Navigate back to the editor after testing
deployMode.NavigateBacktoEditor();
});

it("3. Toggle JS - Validate isVisible", function () {
// Open Property pane
agHelper.AssertElementVisibility(
locators._widgetInDeployed(draggableWidgets.LIST_V2),
Expand All @@ -61,7 +112,7 @@ describe(
deployMode.NavigateBacktoEditor();
});

it("3. Renaming the widget from Property pane and Entity explorer ", function () {
it("4. Renaming the widget from Property pane and Entity explorer ", function () {
// Open Property pane
EditorNavigation.SelectEntityByName("List1", EntityType.Widget);
// Change the list widget name from property pane and Verify it
Expand All @@ -73,7 +124,7 @@ describe(
agHelper.AssertElementVisibility(locators._widgetName("List1"));
});

it("4. Item Spacing Validation ", function () {
it("5. Item Spacing Validation ", function () {
EditorNavigation.SelectEntityByName("List1", EntityType.Widget);
propPane.Search("item spacing");
propPane.UpdatePropertyFieldValue("Item Spacing (px)", "-1");
Expand Down
2 changes: 1 addition & 1 deletion app/client/src/widgets/ListWidgetV2/widget/index.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -878,7 +878,7 @@ class ListWidget extends BaseWidget<
};

calculatePageNumberFromRowIndex = (index: number) => {
return Math.ceil((index + 1) / this.pageSize);
return Math.ceil((index + 1) / this.props.pageSize);
};

shouldUpdatePageSize = () => {
Expand Down

0 comments on commit c0e073e

Please sign in to comment.