From c0e073efa35f6354405c908e90a0b100b350a45c Mon Sep 17 00:00:00 2001 From: Jacques Ikot Date: Mon, 17 Feb 2025 11:43:24 +0100 Subject: [PATCH] fix: infinite render loop in List Widget when hiding/unhiding with selected item (#39262) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## 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" ### :mag: Cypress test results > [!TIP] > 🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉 > Workflow run: > Commit: 74028ec9f52aa25daf0d72c7cdf3c4fa9701e86e > Cypress dashboard. > Tags: `@tag.Widget, @tag.Binding, @tag.List, @tag.Sanity` > Spec: >
Mon, 17 Feb 2025 10:23:19 UTC ## Communication Should the DevRel and Marketing teams inform users about this change? - [ ] Yes - [ ] No ## 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. --- .../Widgets/ListV2/PropertyPane_spec.ts | 57 ++++++++++++++++++- .../src/widgets/ListWidgetV2/widget/index.tsx | 2 +- 2 files changed, 55 insertions(+), 4 deletions(-) diff --git a/app/client/cypress/e2e/Regression/ClientSide/Widgets/ListV2/PropertyPane_spec.ts b/app/client/cypress/e2e/Regression/ClientSide/Widgets/ListV2/PropertyPane_spec.ts index 6433daf835..5da2544f0d 100644 --- a/app/client/cypress/e2e/Regression/ClientSide/Widgets/ListV2/PropertyPane_spec.ts +++ b/app/client/cypress/e2e/Regression/ClientSide/Widgets/ListV2/PropertyPane_spec.ts @@ -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), @@ -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 @@ -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"); diff --git a/app/client/src/widgets/ListWidgetV2/widget/index.tsx b/app/client/src/widgets/ListWidgetV2/widget/index.tsx index 6f16437d34..6705ba29bb 100644 --- a/app/client/src/widgets/ListWidgetV2/widget/index.tsx +++ b/app/client/src/widgets/ListWidgetV2/widget/index.tsx @@ -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 = () => {