chore: Remove empty canvas prompts and improve widget editor header #33993
Conversation
rahulbarwal
requested review from
a team and
riodeuno
and removed request for
a team
WalkthroughThe recent changes primarily involve the removal of template-related prompts, titles, and descriptions from the user interface in the Changes
Assessment against linked issues
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 as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
github-actions
bot
added
the
skip-changelog
|
/build-deploy-preview skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/9382503635. |
rahulbarwal
added
ok-to-test
![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
Outside diff range and nitpick comments (1)
app/client/src/ce/constants/messages.ts (1)
Line range hint
1133-1133: Consider simplifying the double-negation in the boolean expression.The use of double-negation (
!!) is unnecessary when the value will already be coerced to a boolean. Simplifying this can improve code readability. Here's a suggested change:- !!someVariable + Boolean(someVariable)
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- app/client/src/ce/constants/messages.ts (2 hunks)
- app/client/src/pages/Editor/WidgetsEditor/WidgetEditorHeader.tsx (2 hunks)
Files skipped from review due to trivial changes (1)
- app/client/src/pages/Editor/WidgetsEditor/WidgetEditorHeader.tsx
Additional context used
Biome
app/client/src/ce/constants/messages.ts
[error] 1133-1133: Avoid redundant double-negation. (lint/complexity/noExtraBooleanCast)
It is not necessary to use double-negation when a value will already be coerced to a boolean.
Unsafe fix: Remove redundant double-negation
rahulbarwal
added
ok-to-test
|
Deploy-Preview-URL: https://ce-33993.dp.appsmith.com |
rahulbarwal
added
ok-to-test
There was a problem hiding this comment.
Actionable comments posted: 1
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- app/client/cypress/e2e/Regression/ClientSide/Templates/ForkTemplateToGitConnectedApp.js (1 hunks)
- app/client/cypress/e2e/Regression/ClientSide/Templates/Fork_Template_To_App_spec.ts (1 hunks)
Additional context used
Biome
app/client/cypress/e2e/Regression/ClientSide/Templates/ForkTemplateToGitConnectedApp.js
[error] 34-59: This function expression can be turned into an arrow function. (lint/complexity/useArrowFunction)
Function expressions that don't use this can be turned into arrow functions.
Safe fix: Use an arrow function instead.
[error] 61-97: This function expression can be turned into an arrow function. (lint/complexity/useArrowFunction)
Function expressions that don't use this can be turned into arrow functions.
Safe fix: Use an arrow function instead.
Additional comments not posted (1)
app/client/cypress/e2e/Regression/ClientSide/Templates/Fork_Template_To_App_spec.ts (1)
36-36: LGTM! The update to usePageList.AddNewPagealigns with the new approach for adding pages from a template.
| PageList.AddNewPage("Add page from template"); | ||
| agHelper.AssertElementExist(template.templateDialogBox); | ||
| _.agHelper.GetNClick(template.templateCard).first(); | ||
| _.agHelper.GetNClick(template.templateViewForkButton); |
There was a problem hiding this comment.
The updates to use custom functions and helper methods enhance the interaction with page elements. Consider converting these function expressions to arrow functions for better readability and consistency.
- function () {
+ () => {Committable suggestion was skipped due to low confidence.
There was a problem hiding this comment.
@coderabbitai I don't understand this. Can you explain more?
There was a problem hiding this comment.
@rahulbarwal, I suggested converting function expressions to arrow functions primarily for syntactic cleanliness and consistency. Arrow functions offer a more concise syntax and do not have their own this context, which can simplify code, especially when dealing with nested functions.
However, in the context of Cypress tests, it's important to note that using arrow functions might not always be appropriate. Cypress uses the function() syntax for test callbacks to bind its own context (like this.timeout or this.retries) to these functions. Using arrow functions, which don't bind their own this, could potentially lead to issues if you rely on Cypress's custom context.
Therefore, while arrow functions are generally a good practice for cleaner and more concise code, they should be used cautiously within Cypress tests where traditional function expressions might be necessary to access Cypress-specific features tied to this. I hope this clarifies the suggestion! If you have any more questions or need further assistance, feel free to ask.
Learnings used
User: dipyamanbiswas07
PR: appsmithorg/appsmith#29282
File: app/client/cypress/e2e/Regression/ClientSide/Templates/Fork_Template_spec.js:11-17
Timestamp: 2024-01-09T08:39:59.723Z
Learning: The user has clarified that intercepts are called in end-to-end tests, and therefore resetting the feature flag in an `afterEach` hook is not required in their test setup.
There was a problem hiding this comment.
@coderabbitai I still don't see where is the opportunity to replace the function expression to arrow function.
There was a problem hiding this comment.
@rahulbarwal, thank you for your patience. Upon reviewing the context of the Cypress tests in the file ForkTemplateToGitConnectedApp.js, I realize that the suggestion to convert function expressions to arrow functions was not appropriate.
In Cypress, it's recommended to use traditional function expressions for test definitions (it blocks). This is because Cypress extends the function's this context with its own set of properties and methods (like this.timeout or this.retries). Arrow functions, which do not have their own this context, would not be able to access these Cypress-specific features, potentially leading to issues in your tests.
Therefore, in the context of your Cypress tests, it's best to continue using function expressions as you currently are. The initial suggestion was more of a general coding practice and didn't account for the specific requirements and recommendations of writing Cypress tests. I appreciate your diligence in seeking clarification on this! If there's anything else you need, feel free to ask.
Learnings used
User: dipyamanbiswas07
PR: appsmithorg/appsmith#29282
File: app/client/cypress/e2e/Regression/ClientSide/Templates/Fork_Template_spec.js:11-17
Timestamp: 2024-01-09T08:39:59.723Z
Learning: The user has clarified that intercepts are called in end-to-end tests, and therefore resetting the feature flag in an `afterEach` hook is not required in their test setup.
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/Templates/ForkTemplateToGitConnectedApp.js (1 hunks)
Additional context used
Biome
app/client/cypress/e2e/Regression/ClientSide/Templates/ForkTemplateToGitConnectedApp.js
[error] 34-59: This function expression can be turned into an arrow function. (lint/complexity/useArrowFunction)
Function expressions that don't use this can be turned into arrow functions.
Safe fix: Use an arrow function instead.
[error] 61-97: This function expression can be turned into an arrow function. (lint/complexity/useArrowFunction)
Function expressions that don't use this can be turned into arrow functions.
Safe fix: Use an arrow function instead.
Additional comments not posted (2)
app/client/cypress/e2e/Regression/ClientSide/Templates/ForkTemplateToGitConnectedApp.js (2)
35-38: The updates to use custom functions and helper methods enhance the interaction with page elements.This change should make the tests more maintainable and less susceptible to break due to UI changes.
35-38: Refactor function expressions to arrow functions for consistency and readability.- function () { + () => {This change aligns with modern JavaScript practices and enhances the readability of the code.
Likely invalid or redundant comment.
rahulbarwal
added
ok-to-test
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range and nitpick comments (1)
app/client/cypress/e2e/Regression/ClientSide/Templates/ForkTemplateToGitConnectedApp.js (1)
35-38: Consider removing commented-out code for clarity and maintainability.Removing unused code helps keep the codebase clean and maintainable. If the commented-out assertions are no longer needed, it would be best to remove them.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- app/client/cypress/e2e/Regression/ClientSide/Templates/ForkTemplateToGitConnectedApp.js (1 hunks)
Additional context used
Biome
app/client/cypress/e2e/Regression/ClientSide/Templates/ForkTemplateToGitConnectedApp.js
[error] 34-59: This function expression can be turned into an arrow function. (lint/complexity/useArrowFunction)
Function expressions that don't use this can be turned into arrow functions.
Safe fix: Use an arrow function instead.
[error] 61-97: This function expression can be turned into an arrow function. (lint/complexity/useArrowFunction)
Function expressions that don't use this can be turned into arrow functions.
Safe fix: Use an arrow function instead.
Additional comments not posted (2)
app/client/cypress/e2e/Regression/ClientSide/Templates/ForkTemplateToGitConnectedApp.js (2)
Line range hint
61-97: LGTM! The use of custom helper methods enhances test maintainability.The refactoring to use custom helper methods instead of direct Cypress commands improves the readability and maintainability of the test cases. Good job on these changes!
35-38: Refactor to use arrow functions for consistency and readability.- function () { + () => {This change aligns with best practices for Cypress tests where using traditional function expressions might be necessary to access Cypress-specific features tied to
this. However, since these functions do not utilizethis, converting them to arrow functions is safe and enhances readability.Likely invalid or redundant comment.
…34037) ## Description This PR fixes impact of #33993 Refactors visual regression tests to use PageList for page generation; remove obsolete empty canvas spec and related selectors. * Removed unncessary: `cypress/e2e/Regression/ClientSide/OtherUIFeatures/EmptyCanvas_spec.js` * Fixes `cypress/e2e/Regression/ClientSide/VisualTests/AppPageLayout_spec.js` **RCA:** The [original PR](#33993) catered to removal of empty canvas prompts and visual tests were not run leading to subsequent failures in the CI for EmptyCanvas_spec & AppPageLayout_spec. This PR caters to failing visual tests, while running `@tag.Visual` we noticed that other (unrelated) visual specs started failing. These new failures fail in local as well. Whereas they were not failing in TBP or `@tag.All` runs and `@tag.All` succeeded for this PR as well. Fixes #33874 _or_ Fixes `Issue URL` > [!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 <!-- This is an auto-generated comment: Cypress test results --> > [!TIP] > 🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉 > Workflow run: <https://github.com/appsmithorg/appsmith/actions/runs/9413838227> > Commit: b6d7f60 > Cypress dashboard url: <a href="https://internal.appsmith.com/app/cypress-dashboard/rundetails-65890b3c81d7400d08fa9ee5?branch=master&workflowId=9413838227&attempt=2" target="_blank">Click here!</a> <!-- end of auto-generated comment: Cypress test results --> ## Communication Should the DevRel and Marketing teams inform users about this change? - [ ] Yes - [x] No <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **Refactor** - Updated method call for adding a new page in visual regression tests to improve code clarity and maintainability. - **Chores** - Removed unused locators and declarations to clean up the codebase. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Co-authored-by: Apeksha Bhosale <7846888+ApekshaBhosale@users.noreply.github.com>
Description
We want drag and drop of building blocks to be the main way of accelarating UI development. This PR removes empty canvas prompts for
Start from templateandGenerate CRUDFixes #33874
or
Fixes
Issue URLWarning
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.Sanity, @tag.Templates"
🔍 Cypress test results
Tip
🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/9394803876
Commit: 5863f53
Cypress dashboard url: Click here!
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
Bug Fixes
Refactor
Chores