fix: Resolve list widget Error #37747
Conversation
WalkthroughThis pull request addresses a bug in the List widget's auto-height update mechanism. The changes involve modifying the widget props handling to prevent an infinite loop error when interacting with the List widget. A new Cypress test specification is introduced to validate the widget's behavior, specifically testing the visibility and interaction of the List widget under specific conditions. Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
🔇 Additional comments (1)
Finishing Touches
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
|
saharAdem
changed the title
|
@albinAppsmith @ankitakinger Could you please review the PR? |
There was a problem hiding this comment.
Thanks @saharAdem.
While we are reviewing this for any side effects. Can you please add some automation tests to ensure that we don't miss this again?
saharAdem
requested review from
ApekshaBhosale and
sagar-qa007
as code owners
![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 and nitpick comments (1)
app/client/cypress/e2e/Regression/ClientSide/BugTests/ListWidget_Bug37683_spec.ts (1)
12-15: Consider using relative positioning for widgetsThe hardcoded x,y coordinates (300,600 and 600,600) might make the test brittle. Consider using relative positioning or grid-based placement if available.
- cy.dragAndDropToCanvas("listwidgetv2", { - x: 300, - y: 600, - }); + cy.dragAndDropToCanvas("listwidgetv2", { relative: true });Also applies to: 22-25
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
app/client/cypress/e2e/Regression/ClientSide/BugTests/ListWidget_Bug37683_spec.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
app/client/cypress/e2e/Regression/ClientSide/BugTests/ListWidget_Bug37683_spec.ts (1)
Pattern app/client/cypress/**/**.*: Review the following e2e test code written using the Cypress test library. Ensure that:
- Follow best practices for Cypress code and e2e automation.
- Avoid using cy.wait in code.
- Avoid using cy.pause in code.
- Avoid using agHelper.sleep().
- Use locator variables for locators and do not use plain strings.
- Use data-* attributes for selectors.
- Avoid Xpaths, Attributes and CSS path.
- Avoid selectors like .btn.submit or button[type=submit].
- Perform logins via API with LoginFromAPI.
- Perform logout via API with LogOutviaAPI.
- Perform signup via API with SignupFromAPI.
- Avoid using it.only.
- Avoid using after and aftereach in test cases.
- Use multiple assertions for expect statements.
- Avoid using strings for assertions.
- Do not use duplicate filenames even with different paths.
- Avoid using agHelper.Sleep, this.Sleep in any file in code.
🔇 Additional comments (2)
app/client/cypress/e2e/Regression/ClientSide/BugTests/ListWidget_Bug37683_spec.ts (2)
38-39: LGTM: Assertions look good
The visibility assertions for both widgets are well-structured and follow best practices.
10-42: 🛠️ Refactor suggestion
Add cleanup after test
Consider adding cleanup to reset the application state after the test.
describe("Bug 37683: List widget auto-height update infinite loop error", () => {
+ afterEach(() => {
+ // Clean up the canvas
+ agHelper.DeleteAllWidgets();
+ });
it("The list widget height should not trigger an auto-height update", () => {Likely invalid or redundant comment.
| cy.get(`${widgetSelector("List1")} ${containerWidgetSelector}`) | ||
| .eq(1) | ||
| .click({ force: true }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid force clicking without proper wait conditions
Using force: true might mask underlying issues. Consider waiting for the element to be properly interactable.
- cy.get(`${widgetSelector("List1")} ${containerWidgetSelector}`)
- .eq(1)
- .click({ force: true });
+ cy.get(`${widgetSelector("List1")} ${containerWidgetSelector}`)
+ .eq(1)
+ .should('be.visible')
+ .and('be.enabled')
+ .click();📝 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.
| cy.get(`${widgetSelector("List1")} ${containerWidgetSelector}`) | |
| .eq(1) | |
| .click({ force: true }); | |
| cy.get(`${widgetSelector("List1")} ${containerWidgetSelector}`) | |
| .eq(1) | |
| .should('be.visible') | |
| .and('be.enabled') | |
| .click(); |
| cy.get(`${widgetSelector("List1")} ${containerWidgetSelector}`) | ||
| .eq(1) | ||
| .click({ force: true }); | ||
| cy.wait(4000); |
There was a problem hiding this comment.
Replace cy.wait with proper assertions
Using arbitrary wait times can make tests flaky. Instead, wait for specific conditions.
- cy.wait(4000);
+ cy.get(widgetSelector("List1")).should("not.be.visible");📝 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.
| cy.wait(4000); | |
| cy.get(widgetSelector("List1")).should("not.be.visible"); |
|
This PR has not seen activitiy for a while. It will be closed in 7 days unless further activity is detected. |
|
Hi @rahulbarwal, |
|
@saharAdem I think the solution is incorrect here, we shouldn't add |
|
This PR has not seen activitiy for a while. It will be closed in 7 days unless further activity is detected. |
|
I believe the issue is caused by an infinite loop of auto-height updates. I will investigate further. However, this has occurred multiple times when using the list widget. |
|
This PR has not seen activitiy for a while. It will be closed in 7 days unless further activity is detected. |
@jacquesikot Thanks for your explanation. The issue occurs due to auto-height updates in the list widget, which causes an infinite loop. Any list widget should be prevented from auto height updates. The inner widgets of the list widget are already excluded from auto-height updates using the Since the existing code and logic in the MetaWidgetGenerator.ts are based on applying |
Description
This PR resolves an issue with the List widget caused by infinite auto height update loops. The child widgets of the List widget already have the isMetaWidget flag, which prevents auto height updates. However, the List widget itself did not have this flag, leading to the issue. This fix ensures that the isMetaWidget flag is also applied to the List widget, preventing the infinite update loop.
Fixes #37683
Automation
/ok-to-test tags=""
🔍 Cypress test results
Caution
If you modify the content in this section, you are likely to disrupt the CI result for your PR.
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
Tests
Bug Fixes