fix: widget icon in Connect To options in oneclick binding menu #36703
Conversation
…o fix/list-ocb-widget-icon
…o fix/list-ocb-widget-icon
WalkthroughThe changes in this pull request involve enhancements to 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
|
github-actions
bot
added
Bug
rahulbarwal
removed
Bug
github-actions
bot
added
Bug
rahulbarwal
changed the title
rahulbarwal
added
ok-to-test
rahulbarwal
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: 1
🧹 Outside diff range and nitpick comments (1)
app/client/cypress/e2e/Regression/ClientSide/OneClickBinding/spec_utility.ts (1)
Line range hint
1-105: Let's review our overall approach and make some improvements!Class, you've done a good job with the structure of this file, but there are a few areas where we can make it even better:
In the
expandLoadMoreOptionsfunction, we're usingcy.get('body'). This is a bit too broad. Can anyone suggest a more specific selector we could use?Throughout the file, let's make sure we're using data-* attributes for our selectors wherever possible. This will make our tests more resilient to changes in the UI.
We should consider creating more custom commands or helper functions for common operations. This will make our tests more readable and maintainable.
Here's an example of how we could improve the
expandLoadMoreOptionsfunction:export function expandLoadMoreOptions() { agHelper.GetElement(oneClickBindingLocator.loadMoreContainer).then(($container) => { const loadMoreButtons = $container.find(oneClickBindingLocator.loadMore); if (loadMoreButtons.length > 0) { loadMoreButtons.each((index, button) => { agHelper.GetNClick(oneClickBindingLocator.loadMore, index); }); } }); }Remember, writing good tests is like writing a good essay. We want to be clear, specific, and organized!
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (3)
- app/client/cypress/e2e/Regression/ClientSide/OneClickBinding/spec_utility.ts (1 hunks)
- app/client/src/components/editorComponents/WidgetQueryGeneratorForm/CommonControls/DatasourceDropdown/useSource/useConnectToOptions.tsx (2 hunks)
- app/client/src/components/editorComponents/WidgetQueryGeneratorForm/WidgetSpecificControls/OtherFields/Field/Dropdown/useDropdown.tsx (1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
app/client/cypress/e2e/Regression/ClientSide/OneClickBinding/spec_utility.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 (3)
app/client/cypress/e2e/Regression/ClientSide/OneClickBinding/spec_utility.ts (1)
33-33:⚠️ Potential issueLet's replace this hardcoded sleep with a more reliable waiting mechanism!
Class, can anyone tell me why using
agHelper.Sleep(3000)might not be the best approach here? That's right! It's because it can make our tests flaky and slow.Instead, let's use Cypress's built-in waiting mechanisms. Here's how we can improve this:
// Replace this line agHelper.Sleep(3000); //for tables to populate for CI runs // With something like this agHelper.WaitUntil(() => { return cy.get(oneClickBindingLocator.tableOrSpreadsheetDropdown).should('be.visible'); }, 'Wait for tables to populate');This way, we're waiting for a specific condition to be met rather than an arbitrary amount of time. It's like waiting for the bell to ring instead of looking at the clock every few seconds!
⛔ Skipped due to learnings
Learnt from: sagar-qa007 PR: appsmithorg/appsmith#35412 File: app/client/cypress/e2e/Regression/ClientSide/OneClickBinding/TableWidget/mysql_spec.ts:99-99 Timestamp: 2024-08-06T05:59:19.000Z Learning: In Cypress tests within the `app/client/cypress` directory, avoid using `agHelper.Sleep`, `this.Sleep`, and other related sleep functions to prevent non-deterministic behaviors and ensure tests are more reliable and maintainable. Instead, use Cypress methods like `cy.get()`, `cy.contains()`, and `cy.intercept()` to wait for specific conditions.app/client/src/components/editorComponents/WidgetQueryGeneratorForm/WidgetSpecificControls/OtherFields/Field/Dropdown/useDropdown.tsx (1)
84-86: Well done, class! Let's review the improvements in our icon handling.Students, pay attention to the new
iconSVGvariable. It's like having a backup plan for our widget icons. First, we try to get the icon from our widget factory - think of it as our icon library. If that doesn't work, we use the widget's own icon. This way, we always have an icon to show!On line 92, we're using this new
iconSVGin our dropdown options. It's like updating our class roster to always have a picture for each student, even if they forgot their school photo!This change makes our code more robust and ensures we always have an icon to display. Great job!
Also applies to: 92-92
app/client/src/components/editorComponents/WidgetQueryGeneratorForm/CommonControls/DatasourceDropdown/useSource/useConnectToOptions.tsx (1)
243-245: Class, let's examine this excellent improvement in our code!Dear students, I'm pleased to see this enhancement in our
widgetOptionsuseMemo hook. The introduction of theiconSVGvariable is a smart move that ensures we always have the correct icon for our widgets. Let's break it down:
- We first try to fetch the icon from the widget configuration.
- If that's not available, we fall back to the current widget's
iconSVG.- This icon is then used in the
DatasourceImagecomponent.This change demonstrates good coding practices by providing a fallback mechanism and improving the reliability of our UI. Well done!
Also applies to: 256-256
| agHelper.GetElement("[role='menu']").then(($menu) => { | ||
| if ( | ||
| $menu.find(oneClickBindingLocator.datasourceQuerySelector()).length > 0 | ||
| ) { | ||
| cy.wrap($menu) | ||
| .find(oneClickBindingLocator.datasourceQuerySelector()) | ||
| .should("have.length.greaterThan", 0) | ||
| .each(($item) => { | ||
| cy.wrap($item) | ||
| .find("img") | ||
| .should(($img) => { | ||
| expect($img).to.have.attr("src").and.not.be.empty; | ||
| }); | ||
| }); | ||
| } | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Class, let's review this new code block and make some improvements!
While the addition of image validation for datasource items is a good step, there are a few areas where we can enhance our code:
-
Instead of using
cy.wrapdirectly, let's create a custom command or helper method for better maintainability and reusability. -
We should avoid using hardcoded waits like
agHelper.Sleep(3000). Instead, let's use Cypress's built-in waiting mechanisms or create custom waiting functions. -
This method is becoming quite long. Consider breaking it down into smaller, more focused methods for better readability and maintainability.
Here's an example of how we could refactor this part:
private assertDatasourceImages() {
return agHelper.GetElement("[role='menu']").then(($menu) => {
if ($menu.find(oneClickBindingLocator.datasourceQuerySelector()).length > 0) {
cy.wrap($menu)
.find(oneClickBindingLocator.datasourceQuerySelector())
.should("have.length.greaterThan", 0)
.each(($item) => {
cy.wrap($item)
.find("img")
.should("have.attr", "src")
.and("not.be.empty");
});
}
});
}Then, in the ChooseAndAssertForm method:
this.assertDatasourceImages();Remember, class, breaking down complex methods into smaller, more focused ones is like solving a big problem by tackling smaller parts one at a time. It makes our code easier to understand and maintain!
|
/build-deploy-preview skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/11208943387. |
|
Deploy-Preview-URL: https://ce-36703.dp.appsmith.com |
Description
This pull request fixes the issue with the widget icon not displaying correctly in the Connect To options.
The
useConnectToOptionsfunction now correctly retrieves the widget config for the current widget and uses theiconSVGproperty from the config to display the widget icon.Fixes #
Issue Numberor
Fixes #36353
Warning
If no issue exists, please create an issue first, and check with the maintainers if the issue is valid.
Automation
/ok-to-test tags="@tag.All"
🔍 Cypress test results
Tip
🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/11208692370
Commit: 80a6355
Cypress dashboard.
Tags:
@tag.AllSpec:
Mon, 07 Oct 2024 09:25:24 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
New Features
Bug Fixes