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. |
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 , 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.
|
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!
|
Hello @rahulbarwal , Can you please review this PR and let me know if there are any changes to be done. |
|
Hello @rahulbarwal , I have made the suggested changes, Could you please review this. |
|
Client lint is failing: https://github.com/appsmithorg/appsmith/actions/runs/11119240699/job/30894114764?pr=36575#step:9:5209 Please review and fix. Run |
|
Hello @rahulbarwal , I have fixed the linting errors, Could you please review this again. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
app/client/src/widgets/SelectWidget/component/index.tsx (1)
Line range hint
1-524: Excellent work on fixing the double-alert issue!Class, let's recap what we've learned from this code review:
- We added a new method
togglePopoverVisibilityFromButtonto handle button-specific logic.- We updated
togglePopoverVisibilityto prevent double-triggering of alerts.- We connected our new method to the
SelectButtoncomponent.These changes effectively solve the issue of the alert being triggered twice when closing the dropdown. The code is well-structured and the solution is elegantly implemented.
For extra credit, consider the following:
- Add comments explaining the edge case handling in
togglePopoverVisibility. This will help future developers understand the reasoning behind the condition.- Consider writing unit tests for these new scenarios to ensure the fix remains robust as the codebase evolves.
Overall, excellent problem-solving skills demonstrated here!
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
- app/client/src/widgets/SelectWidget/component/index.test.tsx (1 hunks)
- app/client/src/widgets/SelectWidget/component/index.tsx (3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- app/client/src/widgets/SelectWidget/component/index.test.tsx
🔇 Additional comments (3)
app/client/src/widgets/SelectWidget/component/index.tsx (3)
78-80: Well done on adding a specific method for button interactions!Class, let's take a moment to appreciate the new
togglePopoverVisibilityFromButtonmethod. This is an excellent example of separating concerns in our code. By creating a dedicated method for button click events, we're making our code more readable and maintainable.Can anyone tell me why this might be beneficial? That's right! It allows us to handle button clicks differently if needed in the future, without affecting other ways of toggling the popover.
82-86: Excellent solution to prevent double-triggering!Now, class, let's examine the improvements made to our
togglePopoverVisibilitymethod. Can anyone spot the clever addition? That's right, we've added anisButtonClickparameter!This change directly addresses our problem of the alert appearing twice. By adding a condition to check if it's a button click and if the popover is already open, we prevent unnecessary toggling. This is a perfect example of how small, thoughtful changes can solve tricky issues.
Remember, students: always consider edge cases in your code!
467-467: Great job connecting the dots!Class, let's turn our attention to line 467. Can anyone tell me what changed here? Correct! We've updated the
togglePopoverVisibilityprop to use our newtogglePopoverVisibilityFromButtonmethod.This is a crucial step in implementing our solution. By making this change, we ensure that when the select button is clicked, it uses our new logic that prevents double-triggering of the alert.
Remember, students: when you make changes to your code, always make sure to update all the relevant parts to maintain consistency!
|
@PrasadMadine it is still failing with your latest change.
|
Hello @rahulbarwal , Can I rename it as SelectWidgetComponent.test.tsx ? |
|
Hello @rahulbarwal , I have made the changes, run this |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
app/client/src/widgets/SelectWidget/component/SelectWidgetComponent.test.tsx (2)
5-22: Good job setting up your mock props, students!Your mock props cover most of the component's properties, which is excellent. However, let's make it even better:
Consider using more realistic values for properties like
heightandwidth. For example:height: 40, width: 200,It might be helpful to add a few options to test selection functionality:
options: [ { label: "Option 1", value: "1" }, { label: "Option 2", value: "2" }, ],These changes will make your tests more robust and realistic. Keep up the good work!
24-36: Excellent start on your test suite, class!Your test case effectively addresses the issue of
onDropdownClosebeing called multiple times. However, to ensure a comprehensive test suite, consider adding the following test cases:
Test the
onDropdownOpenevent:it("should call onDropdownOpen when select button is clicked", () => { render(<SelectComponent {...mockProps} />); const dropdownButton = screen.getByTestId("selectbutton.btn.main"); fireEvent.click(dropdownButton); expect(mockProps.onDropdownOpen).toHaveBeenCalledTimes(1); });Test option selection:
it("should call onOptionSelected when an option is selected", () => { render(<SelectComponent {...mockProps} options={[{ label: "Option 1", value: "1" }]} />); const dropdownButton = screen.getByTestId("selectbutton.btn.main"); fireEvent.click(dropdownButton); const option = screen.getByText("Option 1"); fireEvent.click(option); expect(mockProps.onOptionSelected).toHaveBeenCalledTimes(1); });Test filtering if
isFilterableis true:it("should call onFilterChange when filter input changes", () => { render(<SelectComponent {...mockProps} isFilterable={true} />); const dropdownButton = screen.getByTestId("selectbutton.btn.main"); fireEvent.click(dropdownButton); const filterInput = screen.getByPlaceholderText("Search..."); fireEvent.change(filterInput, { target: { value: 'test' } }); expect(mockProps.onFilterChange).toHaveBeenCalledTimes(1); });Adding these test cases will provide better coverage and ensure the component behaves correctly in various scenarios. Keep up the great work!
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
- app/client/src/widgets/SelectWidget/component/SelectWidgetComponent.test.tsx (1 hunks)
- app/client/src/widgets/SelectWidget/component/index.tsx (2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- app/client/src/widgets/SelectWidget/component/index.tsx
🔇 Additional comments (1)
app/client/src/widgets/SelectWidget/component/SelectWidgetComponent.test.tsx (1)
1-3: Well done, class! Your imports are spot on!You've correctly imported React, testing utilities, and the component under test. This sets a good foundation for your test suite.
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