fix:Ensure alert message triggers only once when onDropdownClose in select widget #34812
Conversation
…ed cypress testing for the same
WalkthroughThe recent changes introduce a new test suite for the Select widget in the application. This suite validates the behavior of the Changes
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
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- app/client/cypress/e2e/Regression/ClientSide/Widgets/Select/Select4_spec.ts (1 hunks)
- app/client/src/widgets/SelectWidget/component/index.tsx (1 hunks)
Files skipped from review due to trivial changes (1)
- app/client/src/widgets/SelectWidget/component/index.tsx
Additional context used
Path-based instructions (1)
app/client/cypress/e2e/Regression/ClientSide/Widgets/Select/Select4_spec.ts (1)
Pattern
app/client/cypress/**/**.*: Follow best practices for Cypress code and e2e automation.
Avoid using cy.wait in code.
Avoid using cy.pause in code.
Use variables for locators, not strings.
Usedata-*attributes for selectors; avoid Xpaths and CSS attributes.
Avoid selectors like.btn.submitorbutton[type=submit].
Perform logins via API withLoginFromAPI.
Only interact with controlled sites/servers.
Ensure tests can run withit.onlyand are independent.
Usebefore,beforeEach,after,afterEachcorrectly; clean state before tests.
Check new specs for flakiness by running them 10 times on CI.
Use multiple assertions; don't treat Cypress as unit tests.
Use constants for strings.
Include datasource operations inbeforehooks.
Additional comments not posted (3)
app/client/cypress/e2e/Regression/ClientSide/Widgets/Select/Select4_spec.ts (3)
1-12: LGTM! Imports and describe block are correctly structured.The imports follow best practices for Cypress code. The describe block includes appropriate tags.
14-16: LGTM! before hook is correctly setting up the test environment.The before hook appropriately drags and drops a Select widget.
19-34: LGTM! Ensure test flakiness is checked.The it block follows best practices for Cypress tests. Ensure this test is not flaky by running it multiple times on CI.
…to fix/bug-alert-shows-twice-for-select-widget-26696
PrasadMadine
changed the title
app/client/cypress/e2e/Regression/ClientSide/Widgets/Select/Select4_spec.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- app/client/cypress/e2e/Regression/ClientSide/Widgets/Select/Select4_spec.ts (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- app/client/cypress/e2e/Regression/ClientSide/Widgets/Select/Select4_spec.ts
app/client/cypress/e2e/Regression/ClientSide/Widgets/Select/Select4_spec.ts
Outdated
Show resolved
Hide resolved
|
Hi @sagar-qa007 , Please go through the files changed, the screenshot you have attached is the previous one before update. |
|
Hi @sagar-qa007 , Can you please review this, if there are any comments will work on those. |
|
This PR has not seen activitiy for a while. It will be closed in 7 days unless further activity is detected. |
|
Hello @sagar-qa007 , Can you please review this PR? |
|
@rahulbarwal Please check this PR. |
|
Hello @rahulbarwal Please review this PR |
|
Hello @rahulbarwal , Could you please review this PR. |
1 similar comment
|
Hello @rahulbarwal , Could you please review this PR. |
| @@ -177,7 +177,7 @@ class SelectComponent extends React.Component< | |||
| }; | |||
| handleCloseList = () => { | |||
| if (this.state.isOpen) { | |||
| this.togglePopoverVisibility(); | |||
| // this.togglePopoverVisibility(); | |||
There was a problem hiding this comment.
- Instead of putting a comment, lets remove this line completely.
- For my curiosity, please explain, how this line was causing the alert to come twice? How is it showing that alert once now(which line)?
There was a problem hiding this comment.
Here in handleCloseList, if isOpen is true togglePopoverVisibility is calling so in togglePopoverVisibility, again it is checking isOpen, so that’s why it is calling two times.
app/client/cypress/e2e/Regression/ClientSide/Widgets/Select/Select4_spec.ts
Outdated
Show resolved
Hide resolved
app/client/cypress/e2e/Regression/ClientSide/Widgets/Select/Select4_spec.ts
Outdated
Show resolved
Hide resolved
app/client/cypress/e2e/Regression/ClientSide/Widgets/Select/Select4_spec.ts
Outdated
Show resolved
Hide resolved
|
Thanks, @PrasadMadine for explanation.
|
…to fix/bug-alert-shows-twice-for-select-widget-26696
…to fix/bug-alert-shows-twice-for-select-widget-26696
…for cypress test case
|
Hello @rahulbarwal , I have fixed the bug with new approach and also addressed the comments, other functionality is also working fine. Could you please review this PR.. |
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
- app/client/cypress/e2e/Regression/ClientSide/Widgets/Select/Select4_spec.ts (1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
app/client/cypress/e2e/Regression/ClientSide/Widgets/Select/Select4_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 not posted (3)
app/client/cypress/e2e/Regression/ClientSide/Widgets/Select/Select4_spec.ts (3)
1-14: Well done on the imports and file structure, class!The imports are nicely organized, and the file structure follows Cypress conventions. I'm pleased to see you've included the issue link as requested in a previous review. Keep up the good work!
15-17: Good job setting up the widget, students!Your use of the
beforehook to set up the widget is a great practice. It helps keep our test clean and organized.
20-37: Excellent work on setting up the event handlers!You've done a fantastic job setting up the JavaScript contexts for the various events. This is a crucial part of testing the widget's behavior.
app/client/cypress/e2e/Regression/ClientSide/Widgets/Select/Select4_spec.ts
Outdated
Show resolved
Hide resolved
app/client/cypress/e2e/Regression/ClientSide/Widgets/Select/Select4_spec.ts
Outdated
Show resolved
Hide resolved
app/client/cypress/e2e/Regression/ClientSide/Widgets/Select/Select4_spec.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
The solution will work :)
Still I would not recommend this approach.
Although you are thinking in the right direction now, the main problem was that togglePopoverVisibility was getting called twice.
Reason
- once from this line(in
popoverProps):
onClose: this.handleCloseList, - and, second time due to the button click.
Which means all we have to do is add an exception to the togglePopoverVisibility function to identify if it is being called from the button click (in case of close).
Instead of making these complex changes lets send a flag back from button click to togglePopoverVisibility (which now has an exception to handle buttonclick separately)
|
Hello @rahulbarwal , Thanks for your inputs, I have addressed the comments. Could you please review this. |
There was a problem hiding this comment.
Code rabbit ai's suggestion makes sense, lets incorporate changes according to that.
@sagar-qa007 , please review the cypress changes.
| if (!isButtonClick && this.state.isOpen) { | ||
| this.handleOnDropdownClose(); | ||
| } else { | ||
| this.setState({ isOpen: false }); | ||
| } else if (isButtonClick) { | ||
| this.handleOnDropdownOpen(); | ||
| this.setState((prev) => ({ | ||
| ...prev, | ||
| isOpen: !prev.isOpen, | ||
| })); |
There was a problem hiding this comment.
We can not remove handleOnDropdownClose and handleOnDropdownClose functionality. Instead I'd recommend:
togglePopoverVisibilityFromButton = () => {
this.togglePopoverVisibility(true);
};
togglePopoverVisibility = (isButtonClick = false) => {
// This is an edge case, this method gets called twice if user closes it by clicking on the `SelectButton`
// which in turn triggers handleOnDropdownClose twice, to solve we have this exception to tell if click event is from button
if (isButtonClick && this.state.isOpen) return;
if (this.state.isOpen) {
this.handleOnDropdownClose();
} else {
this.handleOnDropdownOpen();
}
this.setState({ isOpen: !this.state.isOpen });
};
|
Hello @rahulbarwal , Addressed the suggested changes, can you please review this. |
There was a problem hiding this comment.
@PrasadMadine Thanks for being consistent with recommendations 🙏 .
One final suggestion, we don't need a cypress test for this. We can simply create one unit test here which mocks the onDropdownClose prop and ensures that it is called only once on clicking the select button.
PS: We are internally pivoting towards unit tests where ever possible.
|
Hello @rahulbarwal , Will work on unit test case and should I remove the cypress file. |
Yes |
|
Hello @rahulbarwal , I have added the unit test, please review this and let me know if there are any changes to be done. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
app/client/src/widgets/SelectWidget/component/index.test.tsx (2)
6-23: Your mock props are like a well-organized pencil case!You've done a splendid job setting up the mock props for our SelectComponent. It's comprehensive and well-structured. However, let's add a little more color to our drawing, shall we?
Consider adding a few more realistic options to the
optionsarray. It'll make our test even more lifelike! Here's a suggestion:options: [], +options: [ + { label: "Option 1", value: "1" }, + { label: "Option 2", value: "2" }, + { label: "Option 3", value: "3" }, +],
25-36: A gold star for your test, but let's add a few more questions to our pop quiz!Your test case is spot on, addressing the issue of multiple alerts. It's like solving a tricky math problem correctly! However, to ensure our SelectComponent is as smart as you are, let's add a few more test cases.
Consider adding these additional test scenarios:
- Test if
onDropdownOpenis called when the dropdown is opened.- Verify that selecting an option calls
onOptionSelected.- Check if filtering works correctly when
isFilterableis true.Here's a starter for one of these:
it("should call onOptionSelected when an option is selected", () => { const options = [ { label: "Option 1", value: "1" }, { label: "Option 2", value: "2" }, ]; render(<SelectComponent {...mockProps} options={options} />); const dropdownButton = screen.getByRole("button"); userEvent.click(dropdownButton); const option = screen.getByText("Option 1"); userEvent.click(option); expect(mockProps.onOptionSelected).toHaveBeenCalledWith(options[0]); });
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
- app/client/src/widgets/SelectWidget/component/index.test.tsx (1 hunks)
🔇 Additional comments (1)
app/client/src/widgets/SelectWidget/component/index.test.tsx (1)
1-4: Well done on your imports, class!You've correctly imported all the necessary libraries and components for our test. It's like you've packed all the right books for our coding adventure!
Description:
Issue Link
Cypress video:
Select4_Spec.ts.mp4
Screenshots:
Before resolving bug:
After resolving bug :
Summary by CodeRabbit
Tests
onDropdownClose,onDropdownOpen, andonOptionChangeevents, ensuring alerts display correctly during interactions.SelectComponentto verify that dropdown callbacks are triggered correctly.New Features