chore: Removing the feature flag for using Entity Item component from ADS templates #39093
Conversation
…o chore/remove-ff-entity-item-usage
WalkthroughThis pull request removes the Changes
Sequence Diagram(s)sequenceDiagram
participant UI as Editor UI
participant CMT as getJSContextMenuByIdeType
participant Logic as Switch-Case Logic
UI->>CMT: Invoke context menu with jsAction
alt jsAction.isMainJSCollection is true
CMT-->>UI: Return null (no context menu)
else
CMT->>Logic: Process jsAction based on type
Logic-->>UI: Return context menu items
end
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
IDE Pod
github-actions
bot
added
the
skip-changelog
![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
🧹 Nitpick comments (1)
app/client/src/pages/Editor/IDE/EditorPane/UI/List.tsx (1)
54-93: Consider simplifying the nested conditional rendering.The component has multiple nested conditional renders (
!widgetsExist,canManagePages,widgetsExist). Consider extracting these into separate components or using early returns to improve readability.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
app/client/src/ce/entities/FeatureFlag.ts(0 hunks)app/client/src/ce/pages/Editor/IDE/EditorPane/JS/old/ListItem.tsx(0 hunks)app/client/src/ce/pages/Editor/IDE/EditorPane/Query/old/ListItem.tsx(0 hunks)app/client/src/ee/pages/Editor/IDE/EditorPane/JS/old/ListItem.tsx(0 hunks)app/client/src/ee/pages/Editor/IDE/EditorPane/Query/old/ListItem.tsx(0 hunks)app/client/src/pages/Editor/Explorer/Widgets/OldWidgetEntityList.tsx(0 hunks)app/client/src/pages/Editor/Explorer/Widgets/WidgetContextMenu.tsx(0 hunks)app/client/src/pages/Editor/Explorer/Widgets/WidgetEntity.tsx(0 hunks)app/client/src/pages/Editor/IDE/EditorPane/JS/List.tsx(1 hunks)app/client/src/pages/Editor/IDE/EditorPane/Query/List.tsx(1 hunks)app/client/src/pages/Editor/IDE/EditorPane/UI/List.tsx(1 hunks)
💤 Files with no reviewable changes (8)
- app/client/src/ee/pages/Editor/IDE/EditorPane/JS/old/ListItem.tsx
- app/client/src/ee/pages/Editor/IDE/EditorPane/Query/old/ListItem.tsx
- app/client/src/pages/Editor/Explorer/Widgets/WidgetContextMenu.tsx
- app/client/src/ce/entities/FeatureFlag.ts
- app/client/src/ce/pages/Editor/IDE/EditorPane/Query/old/ListItem.tsx
- app/client/src/ce/pages/Editor/IDE/EditorPane/JS/old/ListItem.tsx
- app/client/src/pages/Editor/Explorer/Widgets/WidgetEntity.tsx
- app/client/src/pages/Editor/Explorer/Widgets/OldWidgetEntityList.tsx
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: perform-test / client-build / client-build
- GitHub Check: perform-test / rts-build / build
- GitHub Check: perform-test / server-build / server-unit-tests
- GitHub Check: client-check-cyclic-deps / check-cyclic-dependencies
- GitHub Check: client-unit-tests / client-unit-tests
- GitHub Check: client-prettier / prettier-check
- GitHub Check: client-lint / client-lint
- GitHub Check: client-build / client-build
🔇 Additional comments (3)
app/client/src/pages/Editor/IDE/EditorPane/Query/List.tsx (1)
61-72: LGTM! Clean and simplified rendering logic.The direct rendering of
EntityGroupsListwith mapped groups and items improves code readability and maintainability.app/client/src/pages/Editor/IDE/EditorPane/JS/List.tsx (1)
69-80: LGTM! Clean and simplified rendering logic.The direct rendering of
EntityGroupsListwith mapped groups and items improves code readability. Good handling of the "NA" group case.app/client/src/pages/Editor/IDE/EditorPane/UI/List.tsx (1)
22-27: Verify if the license feature flag is still required.The component still uses
FEATURE_FLAG.license_gac_enabled. Let's verify if this feature flag is still needed.✅ Verification successful
🏁 Script executed:
#!/bin/bash # Search for other usages of the license feature flag rg "license_gac_enabled" -A 5Length of output: 45237
License Feature Flag Usage Confirmed as Required
Our search confirms that
FEATURE_FLAG.license_gac_enabledis widely used across the codebase, including in theList.tsxfile. Its continued presence in multiple modules and permission checks indicates that it remains essential for current logic.
- The flag is actively used in various permission and feature check contexts.
- No redundant or obsolete usage was identified.
…o chore/remove-ff-entity-item-usage
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
app/client/src/ce/pages/Editor/IDE/EditorPane/JS/utils/getJSContextMenuByIdeType.tsx (1)
11-13: Simplify the boolean condition.The
Booleancast is redundant as the expression will be automatically coerced to a boolean.- if (Boolean(jsAction?.isMainJSCollection)) { + if (jsAction?.isMainJSCollection) {🧰 Tools
🪛 Biome (1.9.4)
[error] 11-11: Avoid redundant
BooleancallIt is not necessary to use
Booleancall when a value will already be coerced to a boolean.
Unsafe fix: Remove redundantBooleancall(lint/complexity/noExtraBooleanCast)
app/client/src/pages/Editor/IDE/EditorPane/JS/EntityItem/AppJSContextMenuItems.tsx (1)
38-40: Simplify the boolean condition.The
Booleancast is redundant as the expression will be automatically coerced to a boolean.- if (Boolean(jsAction?.isMainJSCollection)) { + if (jsAction?.isMainJSCollection) {🧰 Tools
🪛 Biome (1.9.4)
[error] 38-38: Avoid redundant
BooleancallIt is not necessary to use
Booleancall when a value will already be coerced to a boolean.
Unsafe fix: Remove redundantBooleancall(lint/complexity/noExtraBooleanCast)
app/client/src/pages/Editor/JSEditor/AppJSEditorContextMenu.tsx (1)
31-33: Simplify the boolean condition.The
Booleancast is redundant as the expression will be automatically coerced to a boolean.- if (Boolean(jsCollection?.isMainJSCollection)) { + if (jsCollection?.isMainJSCollection) {🧰 Tools
🪛 Biome (1.9.4)
[error] 31-31: Avoid redundant
BooleancallIt is not necessary to use
Booleancall when a value will already be coerced to a boolean.
Unsafe fix: Remove redundantBooleancall(lint/complexity/noExtraBooleanCast)
app/client/src/pages/Editor/IDE/EditorPane/JS/EntityItem/JSEntityItem.tsx (1)
51-54: Simplify the boolean condition and approve the computed property.The computed property improves code organization, but the
Booleancast is redundant.const canEdit = useMemo( - () => canManageJSAction && !Boolean(jsAction?.isMainJSCollection), + () => canManageJSAction && !jsAction?.isMainJSCollection, [canManageJSAction, jsAction?.isMainJSCollection], );🧰 Tools
🪛 Biome (1.9.4)
[error] 52-52: Avoid redundant
BooleancallIt is not necessary to use
Booleancall when a value will already be coerced to a boolean.
Unsafe fix: Remove redundantBooleancall(lint/complexity/noExtraBooleanCast)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
app/client/packages/design-system/ads/src/Templates/EditableEntityName/EditableEntityName.tsx(1 hunks)app/client/packages/design-system/ads/src/Templates/EntityExplorer/EntityListTree/EntityListTree.tsx(1 hunks)app/client/src/ce/entities/FeatureFlag.ts(0 hunks)app/client/src/ce/pages/Editor/IDE/EditorPane/JS/utils/getJSContextMenuByIdeType.tsx(1 hunks)app/client/src/pages/Editor/IDE/EditorPane/JS/EntityItem/AppJSContextMenuItems.tsx(1 hunks)app/client/src/pages/Editor/IDE/EditorPane/JS/EntityItem/JSEntityItem.tsx(3 hunks)app/client/src/pages/Editor/IDE/EditorPane/JS/JSRender.test.tsx(1 hunks)app/client/src/pages/Editor/IDE/EditorPane/JS/List.tsx(1 hunks)app/client/src/pages/Editor/IDE/EditorPane/Query/EntityItem/QueryEntityItem.tsx(1 hunks)app/client/src/pages/Editor/IDE/EditorPane/Query/List.tsx(1 hunks)app/client/src/pages/Editor/IDE/EditorPane/Query/QueryRender.test.tsx(3 hunks)app/client/src/pages/Editor/IDE/EditorPane/components/EntityContextMenu.tsx(2 hunks)app/client/src/pages/Editor/JSEditor/AppJSEditorContextMenu.tsx(1 hunks)
💤 Files with no reviewable changes (1)
- app/client/src/ce/entities/FeatureFlag.ts
✅ Files skipped from review due to trivial changes (1)
- app/client/packages/design-system/ads/src/Templates/EditableEntityName/EditableEntityName.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- app/client/src/pages/Editor/IDE/EditorPane/Query/List.tsx
🧰 Additional context used
🪛 Biome (1.9.4)
app/client/src/pages/Editor/IDE/EditorPane/JS/EntityItem/AppJSContextMenuItems.tsx
[error] 38-38: Avoid redundant Boolean call
It is not necessary to use Boolean call when a value will already be coerced to a boolean.
Unsafe fix: Remove redundant Boolean call
(lint/complexity/noExtraBooleanCast)
app/client/src/pages/Editor/JSEditor/AppJSEditorContextMenu.tsx
[error] 31-31: Avoid redundant Boolean call
It is not necessary to use Boolean call when a value will already be coerced to a boolean.
Unsafe fix: Remove redundant Boolean call
(lint/complexity/noExtraBooleanCast)
app/client/src/ce/pages/Editor/IDE/EditorPane/JS/utils/getJSContextMenuByIdeType.tsx
[error] 11-11: Avoid redundant Boolean call
It is not necessary to use Boolean call when a value will already be coerced to a boolean.
Unsafe fix: Remove redundant Boolean call
(lint/complexity/noExtraBooleanCast)
app/client/src/pages/Editor/IDE/EditorPane/JS/EntityItem/JSEntityItem.tsx
[error] 52-52: Avoid redundant Boolean call
It is not necessary to use Boolean call when a value will already be coerced to a boolean.
Unsafe fix: Remove redundant Boolean call
(lint/complexity/noExtraBooleanCast)
🔇 Additional comments (9)
app/client/src/pages/Editor/IDE/EditorPane/components/EntityContextMenu.tsx (2)
10-13: LGTM! Props interface improvements.The changes improve type safety by making
childrenrequired and enhance testability with the optionaldataTestId.
28-35: LGTM! Improved Button component testability.Good practice to make the test ID configurable through props instead of hardcoding it.
app/client/packages/design-system/ads/src/Templates/EntityExplorer/EntityListTree/EntityListTree.tsx (1)
68-71: LGTM! Added data-testid for better test stability.The addition of the dataTestId prop follows a consistent pattern and improves test maintainability.
app/client/src/pages/Editor/IDE/EditorPane/JS/List.tsx (1)
69-80: LGTM! Simplified rendering logic by removing feature flag.The code is now cleaner and more maintainable with the removal of conditional rendering based on feature flags.
app/client/src/pages/Editor/IDE/EditorPane/Query/EntityItem/QueryEntityItem.tsx (1)
110-112: LGTM! Improved consistency in component identification and selection.The changes improve consistency by:
- Adding dataTestId following the established pattern
- Using item.key for selection state instead of action.id
app/client/src/pages/Editor/IDE/EditorPane/JS/JSRender.test.tsx (1)
148-149: LGTM! Updated test assertions for better reliability.The test now uses data-selected attribute instead of class-based checks, making it more reliable and consistent with the component changes.
app/client/src/pages/Editor/IDE/EditorPane/Query/QueryRender.test.tsx (1)
159-160: LGTM! The changes align with the feature flag removal.The test assertions have been consistently updated to check the
data-selectedattribute instead of usingclassList.contains("active"). This change reflects the new implementation after removing the feature flag for the Entity Item component.Also applies to: 370-371, 574-575
app/client/src/pages/Editor/JSEditor/AppJSEditorContextMenu.tsx (1)
36-36: LGTM! Good addition of test ID.Adding
dataTestIdimproves testability.app/client/src/pages/Editor/IDE/EditorPane/JS/EntityItem/JSEntityItem.tsx (1)
100-103: LGTM! Good improvements to component props.The changes to className and isSelected props improve clarity and consistency.
ankitakinger
added
ok-to-test
…o chore/remove-ff-entity-item-usage
ankitakinger
requested review from
ApekshaBhosale and
sagar-qa007
as code owners
There was a problem hiding this comment.
Actionable comments posted: 3
🔭 Outside diff range comments (2)
app/client/cypress/support/Pages/EntityExplorer.ts (2)
83-83: Remove usage ofagHelper.Sleep().The code contains multiple instances of
agHelper.Sleep()which is against the coding guidelines. Instead, use Cypress's built-in retry-ability and wait for specific elements or conditions.Here's how to fix this:
- this.agHelper.Sleep(); + this.agHelper.WaitUntil(() => /* specific condition */); - this.agHelper.Sleep(1000); + cy.wait("@relevantAPICall"); // or + cy.get("selector").should("exist"); - this.agHelper.Sleep(500); + this.agHelper.WaitUntil(() => /* specific condition */);Also applies to: 92-92, 115-115, 146-146, 152-152, 190-190, 241-241, 281-281
87-91: Replace force clicks with proper element assertions.Using
force: truewith clicks should be avoided. Instead, ensure elements are properly visible and clickable.Here's how to improve this:
- cy.xpath(this._contextMenu(entityNameinLeftSidebar)) - .scrollIntoView() - .last() - .click({ force: true }); + cy.xpath(this._contextMenu(entityNameinLeftSidebar)) + .scrollIntoView() + .last() + .should("be.visible") + .should("be.enabled") + .click();Also applies to: 111-114
🧹 Nitpick comments (4)
app/client/src/pages/Editor/IDE/EditorPane/JS/List.tsx (1)
69-80: LGTM! Consider extracting group title transformation.The simplified rendering logic looks good and aligns with removing the feature flag.
Consider extracting the group title transformation to improve readability:
<EntityGroupsList groups={filteredItemGroups.map(({ group, items }) => { + const groupTitle = group === "NA" ? "" : group; return { - groupTitle: group === "NA" ? "" : group, + groupTitle, items: items, className: "", renderList: (item: EntityItem) => { return <JSEntity item={item} key={item.key} />; }, }; })} />app/client/src/ce/pages/Editor/IDE/EditorPane/JS/utils/getJSContextMenuByIdeType.tsx (1)
11-13: Simplify boolean expression by removing redundant Boolean cast.- if (Boolean(jsAction?.isMainJSCollection)) { + if (jsAction?.isMainJSCollection) {🧰 Tools
🪛 Biome (1.9.4)
[error] 11-11: Avoid redundant
BooleancallIt is not necessary to use
Booleancall when a value will already be coerced to a boolean.
Unsafe fix: Remove redundantBooleancall(lint/complexity/noExtraBooleanCast)
app/client/src/pages/Editor/IDE/EditorPane/JS/EntityItem/JSEntityItem.tsx (1)
51-54: Simplify boolean expression in canEdit computation.const canEdit = useMemo( - () => canManageJSAction && !Boolean(jsAction?.isMainJSCollection), + () => canManageJSAction && !jsAction?.isMainJSCollection, [canManageJSAction, jsAction?.isMainJSCollection], );🧰 Tools
🪛 Biome (1.9.4)
[error] 52-52: Avoid redundant
BooleancallIt is not necessary to use
Booleancall when a value will already be coerced to a boolean.
Unsafe fix: Remove redundantBooleancall(lint/complexity/noExtraBooleanCast)
app/client/cypress/support/Pages/EntityExplorer.ts (1)
168-202: Improve drag and drop implementation.The current implementation uses low-level mouse events which can be flaky. Consider using Cypress's drag-and-drop commands or a more reliable approach.
Consider using
@4tw/cypress-drag-dropplugin:- .trigger("dragstart", { force: true }) - .trigger("mousemove", x, y, { force: true }); + .drag(dropSelector, { + force: true, + target: { x, y } + });
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
app/client/cypress/support/Objects/CommonLocators.ts(3 hunks)app/client/cypress/support/Pages/EditorNavigation.ts(1 hunks)app/client/cypress/support/Pages/EntityExplorer.ts(1 hunks)app/client/cypress/support/Pages/IDE/LeftPane.ts(2 hunks)app/client/packages/design-system/ads/src/Templates/EditableEntityName/EditableEntityName.tsx(1 hunks)app/client/packages/design-system/ads/src/Templates/EntityExplorer/EntityListTree/EntityListTree.tsx(2 hunks)app/client/src/ce/pages/Editor/IDE/EditorPane/JS/utils/getJSContextMenuByIdeType.tsx(1 hunks)app/client/src/pages/Editor/IDE/EditorPane/JS/EntityItem/JSEntityItem.tsx(3 hunks)app/client/src/pages/Editor/IDE/EditorPane/JS/JSRender.test.tsx(1 hunks)app/client/src/pages/Editor/IDE/EditorPane/JS/List.tsx(1 hunks)app/client/src/pages/Editor/IDE/EditorPane/Query/EntityItem/QueryEntityItem.tsx(1 hunks)app/client/src/pages/Editor/IDE/EditorPane/Query/List.tsx(1 hunks)app/client/src/pages/Editor/IDE/EditorPane/Query/QueryRender.test.tsx(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- app/client/packages/design-system/ads/src/Templates/EditableEntityName/EditableEntityName.tsx
- app/client/packages/design-system/ads/src/Templates/EntityExplorer/EntityListTree/EntityListTree.tsx
- app/client/src/pages/Editor/IDE/EditorPane/JS/JSRender.test.tsx
- app/client/src/pages/Editor/IDE/EditorPane/Query/QueryRender.test.tsx
- app/client/src/pages/Editor/IDE/EditorPane/Query/EntityItem/QueryEntityItem.tsx
- app/client/src/pages/Editor/IDE/EditorPane/Query/List.tsx
🧰 Additional context used
📓 Path-based instructions (1)
`app/client/cypress/**/**.*`: Review the following e2e test ...
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.
app/client/cypress/support/Pages/EntityExplorer.tsapp/client/cypress/support/Pages/EditorNavigation.tsapp/client/cypress/support/Pages/IDE/LeftPane.tsapp/client/cypress/support/Objects/CommonLocators.ts
🪛 Biome (1.9.4)
app/client/src/ce/pages/Editor/IDE/EditorPane/JS/utils/getJSContextMenuByIdeType.tsx
[error] 11-11: Avoid redundant Boolean call
It is not necessary to use Boolean call when a value will already be coerced to a boolean.
Unsafe fix: Remove redundant Boolean call
(lint/complexity/noExtraBooleanCast)
app/client/src/pages/Editor/IDE/EditorPane/JS/EntityItem/JSEntityItem.tsx
[error] 52-52: Avoid redundant Boolean call
It is not necessary to use Boolean call when a value will already be coerced to a boolean.
Unsafe fix: Remove redundant Boolean call
(lint/complexity/noExtraBooleanCast)
⏰ Context from checks skipped due to timeout of 90000ms (10)
- GitHub Check: perform-test / rts-build / build
- GitHub Check: perform-test / client-build / client-build
- GitHub Check: perform-test / server-build / server-unit-tests
- GitHub Check: client-lint / client-lint
- GitHub Check: client-check-cyclic-deps / check-cyclic-dependencies
- GitHub Check: client-build / client-build
- GitHub Check: client-prettier / prettier-check
- GitHub Check: chromatic-deployment
- GitHub Check: chromatic-deployment
- GitHub Check: storybook-tests
🔇 Additional comments (4)
app/client/src/pages/Editor/IDE/EditorPane/JS/EntityItem/JSEntityItem.tsx (1)
98-112: LGTM! Clean implementation of EntityItem component.The component properly implements conditional editing, selection state, and navigation functionality.
app/client/cypress/support/Pages/EntityExplorer.ts (1)
71-71: LGTM: New entity add button selector added.The selector follows the data-testid pattern with a meaningful name.
app/client/cypress/support/Pages/IDE/LeftPane.ts (1)
91-98: LGTM! Well-structured method addition.The new selectedItem method is well-typed and follows the codebase patterns.
app/client/cypress/support/Objects/CommonLocators.ts (1)
339-345: LGTM! Well-structured locators using data-testid.The new module-related locators follow best practices by using data-testid attributes.
sagar-qa007
added
ok-to-test
…o chore/remove-ff-entity-item-usage
|
/build-deploy-preview skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/13257191139. |
|
Deploy-Preview-URL: https://ce-39093.dp.appsmith.com |
…o chore/remove-ff-entity-item-usage
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (2)
app/client/cypress/e2e/Regression/ClientSide/ExplorerTests/Tab_rename_Delete_spec.ts (1)
27-42: 🛠️ Refactor suggestionImprove test reliability with better assertions.
The test could be more robust by:
- Adding assertions after each action
- Using proper wait strategies
- Verifying element state before actions
it("2. Tab Widget Functionality To delete Tabs from entity explorer", function () { EditorNavigation.SelectEntityByName("Tabs1", EntityType.Widget); + agHelper.AssertElementVisibility(locators._entityExplorer("Tabs1")); PageLeftPane.expandCollapseItem("Tabs1"); + agHelper.AssertElementVisibility(locators._entityExplorer("Tab2")); entityExplorer.ActionContextMenuByEntityName({ entityNameinLeftSidebar: "Tab2", action: "Rename", }); agHelper.TypeText(locators._entityNameEditing(), tabname); + agHelper.WaitUntilToastDisappear(); entityExplorer.ValidateDuplicateMessageToolTip(tabname); cy.get(explorer.editEntity) .last() .click() .type("Tab2" + "{enter}", { force: true }); + agHelper.AssertElementVisibility(locators._entityExplorer("Tab2")); entityExplorer.DeleteWidgetFromEntityExplorer(tabname + "Tab2"); + agHelper.AssertElementAbsence(locators._entityExplorer(tabname + "Tab2")); });app/client/cypress/support/Pages/JSEditor.ts (1)
135-135:⚠️ Potential issueReplace Sleep() calls with proper Cypress wait commands.
Multiple instances of Sleep() usage found. Replace them with appropriate wait commands based on the specific conditions you're waiting for.
For example:
- this.agHelper.Sleep(); + this.agHelper.WaitUntilAllToastsDisappear(); - this.agHelper.Sleep(2000); + this.agHelper.WaitUntilElementIsPresent(this.locator._jsResponsePane); - this.agHelper.Sleep(2000); //Settling time for edited js code + this.agHelper.WaitUntilToastDisappear(); - this.agHelper.Sleep(2000); //Settling time for edited js code + this.agHelper.WaitUntilAutoSave();Also applies to: 180-180, 206-206, 218-218
🧹 Nitpick comments (3)
app/client/cypress/support/Pages/IDE/LeftPane.ts (1)
92-99: Add JSDoc for better documentation.The new
selectedItemmethod would benefit from JSDoc documentation explaining the purpose and parameters.+/** + * Gets the currently selected item in the left pane + * @param exists - Optional parameter to verify existence + * @returns Cypress.Chainable representing the selected item + */ public selectedItem( exists?: "exist" | "not.exist" | "noVerify", ): Cypress.Chainable {app/client/src/pages/Editor/IDE/EditorPane/Pages/PageElement.tsx (2)
66-76: Consider using a more descriptive name for the effect.While extracting the effect into a named function improves readability, consider a name that better describes its purpose, such as
scrollCurrentPageIntoView.- useEffect( - function scrollPageIntoView() { + useEffect( + function scrollCurrentPageIntoView() {
137-150: Consider adding data-testid for automated testing.The EntityItem component could benefit from a data-testid attribute for easier test selection.
<EntityItem className={`page fullWidth ${isCurrentPage && "activePage"}`} + data-testid={`page-item-${page.pageId}`} id={page.pageId}
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
app/client/cypress/e2e/Regression/ClientSide/ExplorerTests/Tab_rename_Delete_spec.ts(1 hunks)app/client/cypress/support/Objects/CommonLocators.ts(3 hunks)app/client/cypress/support/Pages/EditorNavigation.ts(1 hunks)app/client/cypress/support/Pages/EntityExplorer.ts(3 hunks)app/client/cypress/support/Pages/IDE/LeftPane.ts(3 hunks)app/client/cypress/support/Pages/JSEditor.ts(1 hunks)app/client/packages/design-system/ads/src/Templates/EditableEntityName/EditableEntityName.tsx(1 hunks)app/client/src/pages/Editor/IDE/EditorPane/Pages/PageElement.tsx(5 hunks)app/client/src/pages/Editor/IDE/EditorPane/Pages/PagesSection.tsx(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- app/client/packages/design-system/ads/src/Templates/EditableEntityName/EditableEntityName.tsx
- app/client/cypress/support/Pages/EntityExplorer.ts
- app/client/cypress/support/Objects/CommonLocators.ts
- app/client/cypress/support/Pages/EditorNavigation.ts
🧰 Additional context used
📓 Path-based instructions (1)
`app/client/cypress/**/**.*`: Review the following e2e test ...
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.
app/client/cypress/support/Pages/JSEditor.tsapp/client/cypress/support/Pages/IDE/LeftPane.tsapp/client/cypress/e2e/Regression/ClientSide/ExplorerTests/Tab_rename_Delete_spec.ts
⏰ Context from checks skipped due to timeout of 90000ms (11)
- GitHub Check: perform-test / client-build / client-build
- GitHub Check: perform-test / rts-build / build
- GitHub Check: perform-test / server-build / server-unit-tests
- GitHub Check: client-unit-tests / client-unit-tests
- GitHub Check: client-lint / client-lint
- GitHub Check: client-check-cyclic-deps / check-cyclic-dependencies
- GitHub Check: client-build / client-build
- GitHub Check: client-prettier / prettier-check
- GitHub Check: chromatic-deployment
- GitHub Check: chromatic-deployment
- GitHub Check: storybook-tests
🔇 Additional comments (10)
app/client/cypress/support/Pages/IDE/LeftPane.ts (3)
11-12: Replace XPath selector with data-testid attribute.The locator uses XPath which is against Cypress best practices. Consider using data-testid attributes for the entire selector chain.
15-15: LGTM! Good use of data-testid.The new
entityItemlocator follows Cypress best practices by using data-testid attributes.
39-44: LGTM! Good refactor.The
assertPresencemethod now uses the new data-testid based locator, which aligns with Cypress best practices.app/client/src/pages/Editor/IDE/EditorPane/Pages/PagesSection.tsx (3)
2-2: LGTM! Clean import structure.The import of ListWithHeader from @appsmith/ads is correctly scoped and aligns with the component simplification.
57-57: LGTM! Simplified component structure.Direct rendering of PageElement without wrapper improves code clarity while maintaining functionality.
59-59: LGTM! Optimized memoization dependencies.The dependency array correctly matches the values used in the memoized function, potentially improving performance by reducing unnecessary re-renders.
app/client/src/pages/Editor/IDE/EditorPane/Pages/PageElement.tsx (4)
1-1: LGTM! Import statements are well-organized.The new imports enhance the component's functionality with name editing capabilities and validation.
Also applies to: 24-26
47-51: LGTM! Hook usage follows React best practices.The custom hooks
useNameEditorStateanduseValidateEntityNameare properly implemented with appropriate parameters.
110-134: LGTM! Name editor configuration is well-structured.The
nameEditorConfigobject is properly memoized with all necessary dependencies. The validation and update logic is cleanly implemented.
78-93: Verify analytics event parameters.The analytics event includes navigation details. Ensure that all required tracking parameters are included for proper analytics tracking.
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Search for other PAGE_NAME_CLICK events to verify parameter consistency rg "PAGE_NAME_CLICK" -A 5Length of output: 1025
Analytics event parameters verified
The parameters used in thePAGE_NAME_CLICKevent here—name,fromUrl,type, andtoUrl—are consistent with the definitions seen elsewhere in the codebase. No additional parameters seem to be required for proper analytics tracking.
app/client/cypress/e2e/Regression/ClientSide/ExplorerTests/Tab_rename_Delete_spec.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (11)
app/client/packages/design-system/ads/src/Templates/EntityExplorer/EntityItem/EntityItem.tsx (1)
31-57:⚠️ Potential issueAdd handleDoubleClick to useMemo dependencies.
The handleDoubleClick function is used in the customTitle but not included in the useMemo dependencies array.
Apply this diff to fix the dependency array:
}, [ canEdit, isEditing, isLoading, onEditComplete, onNameSave, props.title, startIcon, validateName, + handleDoubleClick, ]);app/client/cypress/support/Pages/PageList.ts (2)
107-116: 🛠️ Refactor suggestionImprove test reliability by following Cypress best practices.
The
DeletePagemethod has several anti-patterns:
- Using hardcoded
cy.wait(2000)which makes tests flaky- Using
cy.wait('@deletePage')without proper route interception- Using string selectors directly instead of locator variables
Consider this refactoring:
DeletePage(name: string) { this.ShowList(); - cy.get(this.locators.pageListItem(name)).within(() => { - cy.get(".t--context-menu").click({ force: true }); - }); - cy.wait(2000); - cy.selectAction("Delete"); - cy.selectAction("Are you sure?"); - cy.wait("@deletePage") - .its("response.body.responseMeta.status") - .should("eq", 200); + const contextMenuLocator = ".t--context-menu"; + cy.intercept('DELETE', '**/api/v1/pages/*').as('deletePage'); + cy.get(this.locators.pageListItem(name)) + .find(contextMenuLocator) + .click({ force: true }); + cy.selectAction("Delete") + .should('be.visible'); + cy.selectAction("Are you sure?") + .should('be.visible'); + cy.wait('@deletePage') + .its('response.body.responseMeta.status') + .should('eq', 200); this.HideList(); }
21-40: 🛠️ Refactor suggestionImprove test reliability in AddNewPage method.
The method has potential reliability issues:
- Missing proper route interception before clicking
- Using force click without ensuring element visibility
Consider this refactoring:
public AddNewPage( option: | "New blank page" | "Generate page with data" | "Add page from template" = "New blank page", ) { AppSidebar.navigate(AppSidebarButton.Editor); this.ShowList(); + cy.intercept('POST', '**/api/v1/pages').as('createPage'); ObjectsRegistry.AggregateHelper.GetNClick(this.locators.newButton); cy.get(this.locators.newPageOption) + .should('be.visible') .contains(option, { matchCase: false }) - .click({ force: true }); + .click(); if (option === "New blank page") { ObjectsRegistry.AssertHelper.AssertNetworkStatus("@createPage", 201); return cy .get("@createPage") .then(($pageName: any) => $pageName.response?.body.data.name); } }app/client/cypress/e2e/Regression/ClientSide/OtherUIFeatures/ApplicationURL_spec.js (2)
95-95:⚠️ Potential issueReplace hardcoded wait with proper assertion or intercept.
Avoid using
cy.wait()with a hardcoded timeout. Instead, wait for a specific element to be visible or for a network request to complete.- cy.wait(2000); + agHelper.WaitUntilEleAppear(locators._sidebar);
173-173:⚠️ Potential issueReplace hardcoded wait with proper assertion or intercept.
Avoid using
cy.wait()with a hardcoded timeout. Instead, wait for a specific element to be visible or for a network request to complete.- cy.wait(2000); + agHelper.WaitUntilEleAppear(locators._sidebar);app/client/cypress/e2e/Regression/ClientSide/Git/GitSync/SwitchBranches_spec.js (3)
241-241:⚠️ Potential issueReplace hardcoded wait with proper assertion or intercept.
Avoid using
cy.wait()with a hardcoded timeout. Instead, wait for a specific element to be visible or for a network request to complete.- cy.wait(400); + agHelper.WaitUntilEleAppear(gitSync.locators.branchItem);
226-226:⚠️ Potential issueReplace hardcoded wait with proper assertion or intercept.
Avoid using
cy.wait()with a hardcoded timeout. Instead, wait for a specific element to be visible or for a network request to complete.- cy.wait(4000); // wait for switch branch + agHelper.WaitUntilEleAppear(locators._sidebar);
243-243:⚠️ Potential issueReplace hardcoded wait with proper assertion or intercept.
Avoid using
cy.wait()with a hardcoded timeout. Instead, wait for a specific element to be visible or for a network request to complete.- cy.wait(4000); + agHelper.WaitUntilEleAppear(locators._sidebar);app/client/cypress/support/widgetCommands.js (3)
172-174: 🛠️ Refactor suggestionRemove unnecessary cy.wait() calls and use better waiting strategies.
The code contains multiple instances of hardcoded waits using
cy.wait(). This is against Cypress best practices as it makes tests flaky and slow.Replace these waits with more reliable waiting strategies:
- Wait for specific elements to be visible/interactive
- Wait for network requests to complete
- Wait for animations to finish
Example refactor:
- cy.wait(2000); + cy.get(modalWidgetPage.createModalButton).should('be.visible').and('be.enabled');Also applies to: 415-416, 481-482, 635-636, 657-658, 738-740, 756-758, 771-773, 792-793, 798-800, 809-811
148-148: 🛠️ Refactor suggestionReplace XPath selectors with data- attributes.*
Using XPath selectors is against the coding guidelines. They are slower and more brittle than data-* attributes.
Replace XPath selectors with data-* attributes:
- cy.xpath(option).click({ force: true }); + cy.get('[data-cy="option"]').click({ force: true }); - cy.xpath("//div[@class='tableWrap']//div[@class='table']//div[contains(@class, 'tbody')]/div[@class='tr']/div[@class ='td']") + cy.get('[data-cy="table-cell"]')Also applies to: 1291-1296
34-35: 🛠️ Refactor suggestionUse data- attributes instead of CSS selectors.*
The code extensively uses CSS selectors which are brittle and can break with UI changes. Use data-* attributes for more reliable test selectors.
Replace CSS selectors with data-* attributes:
- cy.get(commonlocators.changeZoomlevel).last() + cy.get('[data-cy="zoom-level"]').last() - cy.get(".t--dropdown-option") + cy.get('[data-cy="dropdown-option"]')Also applies to: 54-55, 79-82, 101-102, 129-130, 187-190, 242-243, 269-270, 274-277, 300-303, 310-314, 331-338, 355-359, 370-374, 395-398, 422-425, 449-453, 486-490, 508-512, 517-525, 562-566, 582-586, 602-606, 617-621, 639-643
🧹 Nitpick comments (2)
app/client/cypress/locators/CMSApplocators.js (1)
1-18: Consider refactoring all XPath selectors.Multiple locators in this file use XPath selectors (lines 4, 5, 8, 9, 10, 14, 16, 17). As per our Cypress best practices, we should use data-* attributes instead.
Would you like me to provide a comprehensive refactor suggestion for all locators?
app/client/cypress/support/Pages/EntityExplorer.ts (1)
71-71: LGTM! Consider using data- attribute.*The selector is correctly defined but could be more maintainable.
Consider using a data-* attribute:
- _entityAddButton = ".t--entity-add-btn"; + _entityAddButton = "[data-cy='entity-add-btn']";
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (19)
app/client/cypress/e2e/Regression/ClientSide/ExplorerTests/Tab_rename_Delete_spec.ts(1 hunks)app/client/cypress/e2e/Regression/ClientSide/Git/GitSync/GitSyncedApps_spec.js(1 hunks)app/client/cypress/e2e/Regression/ClientSide/Git/GitSync/SwitchBranches_spec.js(3 hunks)app/client/cypress/e2e/Regression/ClientSide/OtherUIFeatures/ApplicationURL_spec.js(1 hunks)app/client/cypress/e2e/Regression/ClientSide/PartialImportExport/PartialImportRegularApp.ts(1 hunks)app/client/cypress/locators/CMSApplocators.js(1 hunks)app/client/cypress/support/Objects/CommonLocators.ts(3 hunks)app/client/cypress/support/Pages/AggregateHelper.ts(1 hunks)app/client/cypress/support/Pages/EntityExplorer.ts(5 hunks)app/client/cypress/support/Pages/JSEditor.ts(1 hunks)app/client/cypress/support/Pages/PageList.ts(2 hunks)app/client/cypress/support/queryCommands.js(0 hunks)app/client/cypress/support/widgetCommands.js(1 hunks)app/client/packages/design-system/ads/src/List/List.tsx(0 hunks)app/client/packages/design-system/ads/src/List/List.types.tsx(0 hunks)app/client/packages/design-system/ads/src/Templates/EditableEntityName/EditableEntityName.tsx(2 hunks)app/client/packages/design-system/ads/src/Templates/EditableEntityName/EditableEntityName.types.ts(1 hunks)app/client/packages/design-system/ads/src/Templates/EntityExplorer/EntityItem/EntityItem.tsx(3 hunks)app/client/packages/design-system/ads/src/Templates/EntityExplorer/EntityItem/EntityItem.types.ts(1 hunks)
💤 Files with no reviewable changes (3)
- app/client/packages/design-system/ads/src/List/List.tsx
- app/client/cypress/support/queryCommands.js
- app/client/packages/design-system/ads/src/List/List.types.tsx
🚧 Files skipped from review as they are similar to previous changes (4)
- app/client/cypress/e2e/Regression/ClientSide/ExplorerTests/Tab_rename_Delete_spec.ts
- app/client/cypress/support/Pages/JSEditor.ts
- app/client/packages/design-system/ads/src/Templates/EditableEntityName/EditableEntityName.tsx
- app/client/cypress/support/Objects/CommonLocators.ts
🧰 Additional context used
📓 Path-based instructions (1)
`app/client/cypress/**/**.*`: Review the following e2e test ...
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.
app/client/cypress/e2e/Regression/ClientSide/OtherUIFeatures/ApplicationURL_spec.jsapp/client/cypress/support/widgetCommands.jsapp/client/cypress/e2e/Regression/ClientSide/Git/GitSync/GitSyncedApps_spec.jsapp/client/cypress/support/Pages/PageList.tsapp/client/cypress/e2e/Regression/ClientSide/PartialImportExport/PartialImportRegularApp.tsapp/client/cypress/support/Pages/EntityExplorer.tsapp/client/cypress/locators/CMSApplocators.jsapp/client/cypress/e2e/Regression/ClientSide/Git/GitSync/SwitchBranches_spec.jsapp/client/cypress/support/Pages/AggregateHelper.ts
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: perform-test / client-build / client-build
- GitHub Check: client-build / client-build
- GitHub Check: client-unit-tests / client-unit-tests
- GitHub Check: client-lint / client-lint
- GitHub Check: client-check-cyclic-deps / check-cyclic-dependencies
- GitHub Check: chromatic-deployment
- GitHub Check: chromatic-deployment
- GitHub Check: storybook-tests
🔇 Additional comments (12)
app/client/packages/design-system/ads/src/Templates/EditableEntityName/EditableEntityName.types.ts (1)
26-27: LGTM! Well-documented optional prop addition.The type definition and documentation for the onDoubleClick prop follow the established patterns.
app/client/packages/design-system/ads/src/Templates/EntityExplorer/EntityItem/EntityItem.types.ts (1)
10-11: LGTM! Consistent type definition.The onDoubleClick prop type matches EditableEntityName.types.ts, maintaining consistency across components.
app/client/packages/design-system/ads/src/Templates/EntityExplorer/EntityItem/EntityItem.tsx (1)
22-29: LGTM! Well-implemented event handler.The handleDoubleClick implementation correctly handles event propagation and checks component state.
app/client/cypress/support/Pages/PageList.ts (2)
11-12: LGTM: Selector update aligns with Entity Item component changes.The selector change from
.t--entity.pageto.t--entity-item.pagecorrectly reflects the new Entity Item component implementation.
51-53: LGTM: Consistent selector update.The selector change maintains consistency with the Entity Item component changes.
🧰 Tools
🪛 Biome (1.9.4)
[error] 53-53: This code is unreachable
... because this statement will return from the function beforehand
(lint/correctness/noUnreachable)
app/client/cypress/support/Pages/EntityExplorer.ts (1)
103-103: LGTM! Condition updated correctly.The condition has been updated to exclude the "Rename" action from hiding the page list.
app/client/cypress/support/Pages/AggregateHelper.ts (1)
1016-1016: LGTM! Improved input field handling.The addition of clear() before typing ensures a clean state for input fields, preventing any unintended text concatenation.
app/client/cypress/e2e/Regression/ClientSide/OtherUIFeatures/ApplicationURL_spec.js (1)
45-50: Verify the impact of enabling strict mode in RenameEntityFromExplorer.The third parameter in
RenameEntityFromExplorerhas been changed totrue, which might enable strict validation during renaming. Ensure this change doesn't affect the test's behavior.✅ Verification successful
🏁 Script executed:
#!/bin/bash # Search for other usages of RenameEntityFromExplorer to verify consistency ast-grep --pattern 'RenameEntityFromExplorer($$$, $$$, false, $$$)'Length of output: 69
I'll run an additional script to search for all invocations of
RenameEntityFromExplorerto help us get a complete picture of how the function is used, rather than just looking for those with thefalseparameter. This will let us see if the test call usingtrueis consistent with other usages.
🏁 Script executed:
#!/bin/bash # Search for all invocations of RenameEntityFromExplorer in the codebase rg "RenameEntityFromExplorer(" -n .Length of output: 138
I'll run a new script to directly search within the affected file for the usage of
RenameEntityFromExplorer. This will help us verify how the parameter is being set there.
🏁 Script executed:
#!/bin/bash rg "RenameEntityFromExplorer" app/client/cypress/e2e/Regression/ClientSide/OtherUIFeatures/ApplicationURL_spec.js -n .Length of output: 5003
I'll run a script to inspect the implementation of
RenameEntityFromExplorerwithin the EntityExplorer file. This should help us understand the implications of enabling strict mode.
🏁 Script executed:
#!/bin/bash rg "RenameEntityFromExplorer\(" -n app/client/cypress/support/Pages/EntityExplorer.tsLength of output: 126
Impact of Strict Mode Parameter Verified
- The change to pass
true(enabling strict mode) in the ApplicationURL_spec.js test is consistent with other tests in our codebase that use strict mode.- We found several invocations of
RenameEntityFromExplorerwhere the strict flag is explicitly enabled, indicating this option is an intentional part of our testing strategy.- There's no indication from the file usages that enabling strict mode adversely affects the renaming behavior.
app/client/cypress/e2e/Regression/ClientSide/PartialImportExport/PartialImportRegularApp.ts (1)
25-30: Verify the impact of enabling strict mode in RenameEntityFromExplorer.The third parameter in
RenameEntityFromExplorerhas been changed totrue, which might enable strict validation during renaming. Ensure this change doesn't affect the test's behavior.app/client/cypress/e2e/Regression/ClientSide/Git/GitSync/SwitchBranches_spec.js (1)
83-88: Verify the impact of enabling strict mode in RenameEntityFromExplorer.The third parameter in
RenameEntityFromExplorerhas been changed totrue, which might enable strict validation during renaming. Ensure this change doesn't affect the test's behavior.Also applies to: 101-106, 132-137
app/client/cypress/e2e/Regression/ClientSide/Git/GitSync/GitSyncedApps_spec.js (1)
119-124: Verify the impact of enabling strict mode in RenameEntityFromExplorer.The third parameter in
RenameEntityFromExplorerhas been changed totrue, which might enable strict validation during renaming. Ensure this change doesn't affect the test's behavior.app/client/cypress/support/widgetCommands.js (1)
965-982: LGTM! The change to usetrueforRenameEntityFromExplorerlooks correct.The change ensures consistent behavior with other entity renaming operations.
| @@ -12,7 +12,7 @@ export default { | |||
| confirmButton: "span:contains('Confirm')", | |||
| closeButton: "span:contains('Close')", | |||
| datasourcesbutton: "//div[text()='Datasources']", | |||
| postApi: "//div[text()='send_mail']", | |||
| postApi: "//span[text()='send_mail']", | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Replace XPath selector with data- attribute.*
The locator uses XPath which violates our Cypress best practices. Consider using a data-* attribute instead.
- postApi: "//span[text()='send_mail']",
+ postApi: "[data-cy='send-mail-button']",Please add the corresponding data-cy attribute to the component:
<span data-cy="send-mail-button">send_mail</span>| public _contextMenu = (entityNameinLeftSidebar: string) => | ||
| "//div[text()='" + | ||
| "//span[text()='" + | ||
| entityNameinLeftSidebar + | ||
| "']/ancestor::div[1]/following-sibling::div//button[contains(@class, 'entity-context-menu')]"; | ||
| _entityNameInExplorer = (entityNameinLeftSidebar: string) => | ||
| "//div[contains(@class, 't--entity-explorer')]//div[contains(@class, 't--entity-name')][text()='" + | ||
| entityNameinLeftSidebar + | ||
| "']"; | ||
| "']/parent::div/following-sibling::div//button"; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Update selector to use data- attribute.*
The selector uses XPath with hardcoded HTML tags. This is fragile and may break if the UI structure changes.
Replace the XPath selector with a data-* attribute:
- "//span[text()='" +
- entityNameinLeftSidebar +
- "']/parent::div/following-sibling::div//button";
+ `[data-cy="entity-context-menu-${entityNameinLeftSidebar}"]`;📝 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.
| public _contextMenu = (entityNameinLeftSidebar: string) => | |
| "//div[text()='" + | |
| "//span[text()='" + | |
| entityNameinLeftSidebar + | |
| "']/ancestor::div[1]/following-sibling::div//button[contains(@class, 'entity-context-menu')]"; | |
| _entityNameInExplorer = (entityNameinLeftSidebar: string) => | |
| "//div[contains(@class, 't--entity-explorer')]//div[contains(@class, 't--entity-name')][text()='" + | |
| entityNameinLeftSidebar + | |
| "']"; | |
| "']/parent::div/following-sibling::div//button"; | |
| public _contextMenu = (entityNameinLeftSidebar: string) => | |
| `[data-cy="entity-context-menu-${entityNameinLeftSidebar}"]`; |
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (4)
app/client/cypress/support/ApiCommands.js (4)
152-160: 🛠️ Refactor suggestionImprove selector strategy and wait mechanism in DeleteWidgetFromSideBar command.
While the change from xpath to cy.get is a step in the right direction, there are several improvements needed:
- Replace string-based selector with data-* attribute
- Remove force: true by ensuring element is actionable
- Replace static wait with cy.intercept() or cy.wait('@alias')
-Cypress.Commands.add("DeleteWidgetFromSideBar", () => { - cy.get(apiwidget.popover).last().click({ force: true }); - cy.get(apiwidget.delete).click({ force: true }); - cy.wait("@updateLayout").should( - "have.nested.property", - "response.body.responseMeta.status", - 200, - ); -}); +Cypress.Commands.add("DeleteWidgetFromSideBar", () => { + cy.intercept('PUT', '/api/v1/layouts/*').as('deleteWidget'); + cy.get('[data-testid="widget-popover-menu"]') + .should('be.visible') + .last() + .click(); + cy.get('[data-testid="widget-delete-button"]') + .should('be.visible') + .click(); + cy.wait('@deleteWidget') + .its('response.body.responseMeta.status') + .should('eq', 200); +});
31-39: 🛠️ Refactor suggestionImprove wait strategy in enterDatasource command.
The command uses a static wait which should be replaced with a proper assertion or intercept.
Cypress.Commands.add("enterDatasource", (datasource) => { cy.get(apiwidget.resourceUrl) .first() .click({ force: true }) .type(datasource, { parseSpecialCharSequences: false }); - cy.wait(2000); + cy.get(apiwidget.resourceUrl) + .should('have.value', datasource); agHelper.AssertAutoSave(); });
64-78: 🛠️ Refactor suggestionRemove commented code and improve wait strategy in EditApiNameFromExplorer.
The command contains commented code and uses a static wait.
Cypress.Commands.add("EditApiNameFromExplorer", (apiname) => { - /* - cy.xpath(apiwidget.popover) - .last() - .click({ force: true }); - cy.get(apiwidget.editName).click({ force: true }); - */ cy.get(explorer.editNameField) .clear() .type(apiname, { force: true }) .should("have.value", apiname) .blur({ force: true }); - // eslint-disable-next-line cypress/no-unnecessary-waiting - cy.wait(3000); + cy.get(explorer.editNameField) + .should('not.have.focus'); });
124-133: 🛠️ Refactor suggestionReplace xpath usage in RenameEntity command.
The command uses xpath selectors which should be avoided as per guidelines.
Cypress.Commands.add("RenameEntity", (value, selectFirst) => { if (selectFirst) { - cy.xpath(apiwidget.popover).first().click({ force: true }); + cy.get('[data-testid="entity-popover"]') + .should('be.visible') + .first() + .click(); } else { - cy.xpath(apiwidget.popover).last().click({ force: true }); + cy.get('[data-testid="entity-popover"]') + .should('be.visible') + .last() + .click(); } - cy.get(apiwidget.renameEntity).click({ force: true }); - cy.get(explorer.editEntity).last().type(value, { force: true }); + cy.get('[data-testid="rename-entity"]') + .should('be.visible') + .click(); + cy.get('[data-testid="edit-entity-field"]') + .should('be.visible') + .last() + .clear() + .type(value); });
♻️ Duplicate comments (1)
app/client/cypress/support/Pages/EntityExplorer.ts (1)
54-57:⚠️ Potential issueUpdate selector to use data- attribute.*
The selector uses XPath with hardcoded HTML tags. This is fragile and may break if the UI structure changes.
Apply this diff:
- "//span[text()='" + - entityNameinLeftSidebar + - "']/parent::div/following-sibling::div//button"; + `[data-cy="entity-context-menu-${entityNameinLeftSidebar}"]`;
🧹 Nitpick comments (6)
app/client/packages/design-system/ads/src/Templates/EntityExplorer/EntityGroupsList/EntityGroupsList.tsx (1)
72-72: LGTM! Consider adding type guard for safer title access.The addition of dataTestId improves component testability. However, the type cast could be made safer.
-dataTestId={`t--entity-item-${(item as ListItemProps)?.title}`} +dataTestId={`t--entity-item-${ + 'title' in item ? (item as ListItemProps).title : `item-${index}` +}`}app/client/cypress/e2e/Regression/ClientSide/PartialImportExport/PageActions_spec.ts (2)
18-39: Consider enhancing test coverage for page actions.While the test verifies basic page actions, consider adding assertions for:
- The success/failure state of the clone operation
- Verification of widget content after cloning
- Validation of page navigation after actions
80-100: Add validation for page state after navigation.Consider adding assertions to verify that the page state remains consistent after navigation, particularly for:
- Widget visibility
- Page order in the explorer
- Active page indicators
app/client/cypress/locators/apiWidgetslocator.json (1)
1-60: Consider updating remaining XPath and class-based selectors.Several locators still use XPath and class-based selectors, which should be replaced with data-testid attributes for better maintainability and reliability:
- XPath selectors to update:
- Line 3:
searchInputPlaceholder- Line 14:
autoSuggest- Line 27:
postbody- Line 28:
paginationTab- Line 38:
Request- Lines 39-41:
RequestURL,RequestMethod,content-Type- Line 44:
Responsetab- Line 54:
paramsTab
- Class-based selectors to update:
- Multiple instances using
.t--prefix should use data-testidapp/client/src/pages/Editor/IDE/EditorPane/JS/EntityItem/JSEntityItem.tsx (2)
51-54: Optimize Boolean conversion.The
Booleancall is redundant since the value will already be coerced to a boolean.Apply this diff:
- const canEdit = useMemo( - () => canManageJSAction && !Boolean(jsAction?.isMainJSCollection), - [canManageJSAction, jsAction?.isMainJSCollection], - ); + const canEdit = useMemo( + () => canManageJSAction && !jsAction?.isMainJSCollection, + [canManageJSAction, jsAction?.isMainJSCollection], + );🧰 Tools
🪛 Biome (1.9.4)
[error] 52-52: Avoid redundant
BooleancallIt is not necessary to use
Booleancall when a value will already be coerced to a boolean.
Unsafe fix: Remove redundantBooleancall(lint/complexity/noExtraBooleanCast)
100-100: Use template literals for className concatenation.The className concatenation can be more readable using template literals.
Apply this diff:
- className={`t--jsaction ${canEdit ? "editable" : ""}`} + className={`t--jsaction${canEdit ? " editable" : ""}`}
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
app/client/cypress/e2e/Regression/ClientSide/PartialImportExport/PageActions_spec.ts(3 hunks)app/client/cypress/locators/CMSApplocators.js(1 hunks)app/client/cypress/locators/apiWidgetslocator.json(1 hunks)app/client/cypress/support/ApiCommands.js(1 hunks)app/client/cypress/support/Pages/EntityExplorer.ts(5 hunks)app/client/cypress/support/Pages/IDE/ListView.ts(1 hunks)app/client/packages/design-system/ads/src/List/List.tsx(1 hunks)app/client/packages/design-system/ads/src/Templates/EditableEntityName/EditableEntityName.tsx(2 hunks)app/client/packages/design-system/ads/src/Templates/EntityExplorer/EntityGroupsList/EntityGroupsList.tsx(1 hunks)app/client/packages/design-system/ads/src/Templates/EntityExplorer/EntityItem/EntityItem.tsx(1 hunks)app/client/packages/design-system/ads/src/Templates/EntityExplorer/EntityListTree/EntityListTree.tsx(1 hunks)app/client/src/pages/Editor/IDE/EditorPane/JS/EntityItem/JSEntityItem.tsx(3 hunks)app/client/src/pages/Editor/IDE/EditorPane/Query/EntityItem/QueryEntityItem.tsx(1 hunks)app/client/src/pages/Editor/IDE/EditorPane/UI/UIEntityListTree/WidgetContextMenu.tsx(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- app/client/packages/design-system/ads/src/Templates/EntityExplorer/EntityItem/EntityItem.tsx
🚧 Files skipped from review as they are similar to previous changes (4)
- app/client/packages/design-system/ads/src/Templates/EntityExplorer/EntityListTree/EntityListTree.tsx
- app/client/cypress/locators/CMSApplocators.js
- app/client/packages/design-system/ads/src/Templates/EditableEntityName/EditableEntityName.tsx
- app/client/src/pages/Editor/IDE/EditorPane/Query/EntityItem/QueryEntityItem.tsx
🧰 Additional context used
📓 Path-based instructions (1)
`app/client/cypress/**/**.*`: Review the following e2e test ...
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.
app/client/cypress/support/ApiCommands.jsapp/client/cypress/locators/apiWidgetslocator.jsonapp/client/cypress/e2e/Regression/ClientSide/PartialImportExport/PageActions_spec.tsapp/client/cypress/support/Pages/IDE/ListView.tsapp/client/cypress/support/Pages/EntityExplorer.ts
🪛 Biome (1.9.4)
app/client/src/pages/Editor/IDE/EditorPane/JS/EntityItem/JSEntityItem.tsx
[error] 52-52: Avoid redundant Boolean call
It is not necessary to use Boolean call when a value will already be coerced to a boolean.
Unsafe fix: Remove redundant Boolean call
(lint/complexity/noExtraBooleanCast)
⏰ Context from checks skipped due to timeout of 90000ms (11)
- GitHub Check: perform-test / client-build / client-build
- GitHub Check: perform-test / rts-build / build
- GitHub Check: perform-test / server-build / server-unit-tests
- GitHub Check: client-unit-tests / client-unit-tests
- GitHub Check: client-lint / client-lint
- GitHub Check: client-prettier / prettier-check
- GitHub Check: client-check-cyclic-deps / check-cyclic-dependencies
- GitHub Check: client-build / client-build
- GitHub Check: chromatic-deployment
- GitHub Check: chromatic-deployment
- GitHub Check: storybook-tests
🔇 Additional comments (5)
app/client/packages/design-system/ads/src/List/List.tsx (1)
122-122: LGTM! Class addition follows conventions.The addition of "t--ide-list-item" class follows the team's naming conventions and is properly integrated using clsx.
app/client/cypress/e2e/Regression/ClientSide/PartialImportExport/PageActions_spec.ts (1)
32-36: LGTM! Attribute assertions updated to use data- attributes.*The change from "disabled" to "data-disabled" aligns with Cypress best practices for using data attributes as selectors.
Also applies to: 93-97, 114-118
app/client/cypress/locators/apiWidgetslocator.json (1)
7-7: LGTM! Good improvement in locator strategy.The change from XPath to data-testid improves test reliability and follows best practices.
app/client/src/pages/Editor/IDE/EditorPane/UI/UIEntityListTree/WidgetContextMenu.tsx (2)
23-23: LGTM!Clean import addition that aligns with the PR objective of standardizing Entity Item component usage.
97-103: Verify test selector usage in Cypress tests.The MenuContent changes look good, but since Cypress tests are failing and we've modified a test selector class (
t--entity-context-menu), we should verify the selector usage.❌ Verification inconclusive
🏁 Script executed:
#!/bin/bash # Search for usage of the context menu test selector in Cypress tests rg "t--entity-context-menu" -t js -t ts -t tsx --glob "cypress/**/*"Length of output: 96
I'll run a revised shell script to search for the test selector using a glob that supports all three extensions. Please verify the output after execution.
🏁 Script executed:
#!/bin/bash # Search for usage of the context menu test selector in Cypress tests across js, ts, and tsx files rg "t--entity-context-menu" --glob 'cypress/**/*.{js,ts,tsx}'Length of output: 210
I'll run a fallback command that searches all files under the cypress folder for the test selector to confirm whether it's being referenced.
🏁 Script executed:
#!/bin/bash find cypress -type f \( -name "*.js" -o -name "*.ts" -o -name "*.tsx" \) -exec grep -H "t--entity-context-menu" {} \;Length of output: 165
Let's try a more specific search for the selector in any Cypress test expressions by looking for usage patterns like
cy.get()in spec files.
🏁 Script executed:
#!/bin/bash # Search for Cypress test usage of the test selector in spec files fd --type f -e spec.js -e spec.ts -e spec.tsx | xargs grep -HE "cy\.get\([^)]*t--entity-context-menu"Length of output: 103
Test selector usage verification:
Our searches did not reveal any references to the"t--entity-context-menu"selector in any potential Cypress test files. Neither direct grep in common test file extensions nor specific searches forcy.get(...)usage produced any output. It appears that, in our checked paths, no tests are currently using this selector.Please verify manually if your Cypress tests might reside in a nonstandard directory or use a different file naming convention.
| @@ -3,7 +3,7 @@ import { ObjectsRegistry } from "../../Objects/Registry"; | |||
| class ListView { | |||
| public locators = { | |||
| list: "[data-testid='t--ide-list']", | |||
| listItem: "[data-testid='t--ide-list-item']", | |||
| listItem: ".t--ide-list-item", | |||
There was a problem hiding this comment.
Use data-testid instead of class selector.
The change from [data-testid='t--ide-list-item'] to .t--ide-list-item violates Cypress best practices. Class selectors are more brittle and can break if styling changes.
Apply this diff to use data-testid:
- listItem: ".t--ide-list-item",
+ listItem: "[data-testid='t--ide-list-item']",📝 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.
| listItem: ".t--ide-list-item", | |
| listItem: "[data-testid='t--ide-list-item']", |
| @@ -72,6 +68,7 @@ export class EntityExplorer { | |||
| _widgetTagSuggestedWidgets = ".widget-tag-collapsible-suggested"; | |||
| _widgetTagBuildingBlocks = ".widget-tag-collapsible-building-blocks"; | |||
| _widgetSeeMoreButton = "[data-testid='t--explorer-ui-entity-tag-see-more']"; | |||
| _entityAddButton = ".t--entity-add-btn"; | |||
There was a problem hiding this comment.
Use data-testid for entity add button selector.
The selector uses a class which is more brittle than data attributes.
Apply this diff:
- _entityAddButton = ".t--entity-add-btn";
+ _entityAddButton = "[data-testid='t--entity-add-btn']";📝 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.
| _entityAddButton = ".t--entity-add-btn"; | |
| _entityAddButton = "[data-testid='t--entity-add-btn']"; |
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (6)
app/client/cypress/e2e/Regression/ClientSide/IDE/Canvas_Context_Bug_Fixes.js (2)
8-19: 🛠️ Refactor suggestionRefactor selectors to use data-testid attributes.
The current selectors use CSS paths which is against our guidelines. Consider updating them to use data-testid attributes.
-cy.get( - `.t--property-control-options > div:nth-child(2) > div[orientation="HORIZONTAL"]`, -).should("have.length", 3); +cy.get('[data-testid="t--property-control-options-item"]') + .should("have.length", 3); -cy.get( - ".t--property-control-options > div:nth-child(2) > div:nth-child(2) > button", -).click(); +cy.get('[data-testid="t--property-control-options-delete-btn"]') + .click();
24-24: 🛠️ Refactor suggestionAvoid using cy.reload().
Using cy.reload() implicitly introduces a wait which is against our guidelines. Consider setting up the test state directly.
app/client/cypress/e2e/Regression/ClientSide/JSObject/JSObject_ForkApp_spec.ts (1)
28-28: 🛠️ Refactor suggestionAvoid using agHelper.WaitUntilEleDisappear.
Using WaitUntilEleDisappear is similar to using cy.wait which is against our guidelines. Consider using Cypress's built-in retry-ability with assertions instead.
-agHelper.WaitUntilEleDisappear(homePage._forkModal); +cy.get(homePage._forkModal).should('not.exist');app/client/packages/design-system/ads/src/Templates/EntityExplorer/EntityItem/EntityItem.tsx (1)
39-48:⚠️ Potential issueAdd missing dependency to useMemo.
The
normalizeNameprop is used in the customTitle memo but not listed in the dependencies array.Add
normalizeNameto the dependencies array:}, [ canEdit, isEditing, isLoading, onEditComplete, onNameSave, props.title, startIcon, validateName, + normalizeName, ]);app/client/cypress/e2e/Regression/Apps/EchoApiCMS_spec.js (2)
81-92: 🛠️ Refactor suggestionRefactor element selectors to use data-testid attributes.
Multiple instances of xpath and hardcoded selectors found. This violates our coding guidelines and makes tests brittle.
Examples of recommended changes:
-cy.xpath("//span[text()='3']").click({ force: true }); +cy.get('[data-testid="item-count"]').click(); -cy.xpath(appPage.sendMailText).should("be.visible"); +cy.get('[data-testid="send-mail-text"]').should("be.visible"); -cy.xpath("//input[@value='[email protected]']").should("be.visible"); +cy.get('[data-testid="email-input"]').should("be.visible");
22-25: 🛠️ Refactor suggestionUse API calls for setup instead of UI interactions.
The setup should use API calls instead of UI interactions for better test stability and performance.
-before(() => { - homePage.RenameApplication("EchoApiCMSApp"); - agHelper.AddDsl("CMSdsl"); -}); +before(() => { + cy.LoginFromAPI(); + cy.CreateAppFromAPI("EchoApiCMSApp"); + cy.AddDslFromAPI("CMSdsl"); +});
🧹 Nitpick comments (4)
app/client/packages/design-system/ads/src/Templates/EditableEntityName/EditableEntityName.tsx (1)
79-96: Add error handling for tooltip content.The tooltip content directly uses the name without any sanitization or length checks.
Consider adding length truncation and sanitization:
- content: name, + content: name.length > 100 ? `${name.slice(0, 97)}...` : name,app/client/packages/design-system/ads/src/__hooks__/useEditableText/useEditableText.ts (1)
122-134: Consider adding error handling for the select() call.While the text selection improves UX, the select() call could throw if the input is not selectable.
useEffect( function recaptureFocusInEventOfFocusRetention() { const input = inputRef.current; if (isEditing && input) { setTimeout(() => { input.focus(); - input.select(); + try { + input.select(); + } catch (e) { + // Ignore selection errors + } }, 200); } }, [isEditing, inputRef], );app/client/src/pages/Editor/IDE/EditorPane/Pages/PageElement.tsx (1)
138-151: Consider adding a data-testid attribute for testing.Adding a test identifier would make the component more testable.
<EntityItem className={`page fullWidth ${isCurrentPage && "activePage"}`} + data-testid={`page-item-${page.pageId}`} id={page.pageId} isDisabled={page.isHidden} isSelected={isCurrentPage} key={page.pageId} nameEditorConfig={nameEditorConfig} onClick={switchPage} onDoubleClick={() => enterEditMode(page.pageId)} rightControl={contextMenu} rightControlVisibility="hover" startIcon={icon} title={page.pageName} />app/client/cypress/e2e/Regression/Apps/EchoApiCMS_spec.js (1)
95-95: Good move switching from xpath to cy.get(), but avoid force: true.While it's great that you've moved away from xpath selectors, using
force: trueshould be avoided as it bypasses Cypress's built-in retry-ability and can make tests flaky.-cy.get(appPage.postApi).click({ force: true }); +cy.get(appPage.postApi).should('be.visible').click();
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
app/client/cypress/e2e/Regression/Apps/EchoApiCMS_spec.js(1 hunks)app/client/cypress/e2e/Regression/ClientSide/ExplorerTests/Tab_rename_Delete_spec.ts(1 hunks)app/client/cypress/e2e/Regression/ClientSide/IDE/Canvas_Context_Bug_Fixes.js(1 hunks)app/client/cypress/e2e/Regression/ClientSide/JSObject/JSObject_ForkApp_spec.ts(2 hunks)app/client/cypress/locators/CMSApplocators.js(1 hunks)app/client/cypress/support/Pages/AggregateHelper.ts(3 hunks)app/client/cypress/support/Pages/JSEditor.ts(1 hunks)app/client/packages/design-system/ads/src/Templates/EditableEntityName/EditableEntityName.tsx(4 hunks)app/client/packages/design-system/ads/src/Templates/EditableEntityName/EditableEntityName.types.ts(1 hunks)app/client/packages/design-system/ads/src/Templates/EntityExplorer/EntityItem/EntityItem.tsx(3 hunks)app/client/packages/design-system/ads/src/Templates/EntityExplorer/EntityItem/EntityItem.types.ts(1 hunks)app/client/packages/design-system/ads/src/__hooks__/useEditableText/useEditableText.ts(3 hunks)app/client/src/pages/Editor/IDE/EditorPane/Pages/PageElement.tsx(5 hunks)app/client/src/pages/Editor/IDE/EditorPane/Query/EntityItem/QueryEntityItem.tsx(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- app/client/cypress/locators/CMSApplocators.js
- app/client/cypress/e2e/Regression/ClientSide/ExplorerTests/Tab_rename_Delete_spec.ts
- app/client/cypress/support/Pages/JSEditor.ts
- app/client/src/pages/Editor/IDE/EditorPane/Query/EntityItem/QueryEntityItem.tsx
🧰 Additional context used
📓 Path-based instructions (1)
`app/client/cypress/**/**.*`: Review the following e2e test ...
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.
app/client/cypress/e2e/Regression/ClientSide/IDE/Canvas_Context_Bug_Fixes.jsapp/client/cypress/e2e/Regression/Apps/EchoApiCMS_spec.jsapp/client/cypress/e2e/Regression/ClientSide/JSObject/JSObject_ForkApp_spec.tsapp/client/cypress/support/Pages/AggregateHelper.ts
⏰ Context from checks skipped due to timeout of 90000ms (10)
- GitHub Check: perform-test / client-build / client-build
- GitHub Check: perform-test / server-build / server-unit-tests
- GitHub Check: client-check-cyclic-deps / check-cyclic-dependencies
- GitHub Check: client-unit-tests / client-unit-tests
- GitHub Check: client-lint / client-lint
- GitHub Check: client-prettier / prettier-check
- GitHub Check: client-build / client-build
- GitHub Check: chromatic-deployment
- GitHub Check: chromatic-deployment
- GitHub Check: storybook-tests
🔇 Additional comments (12)
app/client/cypress/support/Pages/AggregateHelper.ts (1)
989-990: LGTM! The newclearparameter enhances the flexibility of theTypeTextmethod.The implementation correctly handles clearing the input field before typing when the
clearparameter is set totrue. The code maintains consistency with the existing codebase and follows TypeScript best practices.Also applies to: 996-996, 1008-1008, 1019-1026
app/client/cypress/e2e/Regression/ClientSide/IDE/Canvas_Context_Bug_Fixes.js (1)
28-28: LGTM! Good use of data-testid attribute.The selector update follows best practices by using data-testid.
app/client/cypress/e2e/Regression/ClientSide/JSObject/JSObject_ForkApp_spec.ts (1)
38-42: LGTM! Good use of attribute-based assertions.The updated assertions follow best practices by:
- Using data attributes for state verification
- Using helper functions for consistent assertions
Also applies to: 47-51, 62-66, 68-72
app/client/packages/design-system/ads/src/Templates/EditableEntityName/EditableEntityName.types.ts (1)
26-27: LGTM! Well-documented property addition.The new
normalizeNameproperty is properly typed and documented.app/client/packages/design-system/ads/src/Templates/EntityExplorer/EntityItem/EntityItem.types.ts (1)
24-25: LGTM! Good choice making the property optional.The optional
normalizeNameproperty maintains backward compatibility while allowing new functionality.app/client/packages/design-system/ads/src/Templates/EditableEntityName/EditableEntityName.tsx (2)
10-12: LGTM! Clean utility function implementation.The
isEllipsisActivefunction is well-focused and handles null checks properly.
75-77:⚠️ Potential issueAdd missing dependency to useEffect.
The effect uses
longNameRefbut it's not included in the dependency array.- }, [name]); + }, [name, longNameRef]);Likely an incorrect or invalid review comment.
app/client/packages/design-system/ads/src/__hooks__/useEditableText/useEditableText.ts (2)
22-22: LGTM! Good addition of the optional normalize parameter.The optional boolean parameter provides flexibility in text normalization without breaking existing usage.
89-96: LGTM! Clean implementation of conditional normalization.The ternary operator provides a clear and concise way to handle conditional normalization while maintaining the existing validation flow.
app/client/src/pages/Editor/IDE/EditorPane/Pages/PageElement.tsx (3)
1-1: LGTM! Clean import organization.The imports are well-organized, separating React core, hooks, and components.
Also applies to: 24-26
66-76: LGTM! Good function naming in useEffect.The named function
scrollPageIntoViewimproves code readability and debugging.
110-135: LGTM! Well-structured nameEditorConfig with proper memoization.The config object is properly memoized with all necessary dependencies, and the normalization is explicitly disabled.
| cy.ResponseCheck("Test"); | ||
| // cy.ResponseCheck("Task completed"); | ||
| cy.ResponseCheck("[email protected]"); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use structured assertions instead of plain strings.
Direct string assertions should be replaced with more structured expectations.
-cy.ResponseCheck("Test");
-cy.ResponseCheck("[email protected]");
+cy.ResponseCheck({
+ subject: "Test",
+ recipient: "[email protected]"
+});📝 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.ResponseCheck("Test"); | |
| // cy.ResponseCheck("Task completed"); | |
| cy.ResponseCheck("[email protected]"); | |
| cy.ResponseCheck({ | |
| subject: "Test", | |
| recipient: "[email protected]" | |
| }); | |
| // cy.ResponseCheck("Task completed"); |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
app/client/packages/design-system/ads/src/Templates/EditableEntityName/EditableEntityName.tsx (2)
31-32: Consider memoizing tooltip state.The tooltip visibility state could benefit from memoization to prevent unnecessary re-renders.
- const [showTooltip, setShowTooltip] = useState(false); + const [showTooltip, setShowTooltip] = useState(false); + const memoizedShowTooltip = useMemo(() => showTooltip, [showTooltip]);
79-95: Consider extracting tooltip configuration.The tooltip props configuration logic could be moved to a separate memoized function for better maintainability.
+ const getTooltipProps = useMemo( + () => + validationError + ? { + content: validationError, + placement: "bottom", + visible: Boolean(validationError), + isDisabled: false, + mouseEnterDelay: 0, + showArrow: true, + } + : { + content: name, + placement: "topLeft", + isDisabled: !showTooltip, + mouseEnterDelay: 1, + showArrow: false, + ...(!showTooltip ? { visible: false } : {}), + }, + [validationError, name, showTooltip] + ); - const tooltipProps: TooltipProps = validationError - ? { - content: validationError, - placement: "bottom", - visible: Boolean(validationError), - isDisabled: false, - mouseEnterDelay: 0, - showArrow: true, - } - : { - content: name, - placement: "topLeft", - isDisabled: !showTooltip, - mouseEnterDelay: 1, - showArrow: false, - ...(!showTooltip ? { visible: false } : {}), - };
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
app/client/packages/design-system/ads/src/Templates/EditableEntityName/EditableEntityName.tsx(4 hunks)app/client/packages/design-system/ads/src/Templates/EditableEntityName/EditableEntityName.types.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- app/client/packages/design-system/ads/src/Templates/EditableEntityName/EditableEntityName.types.ts
⏰ Context from checks skipped due to timeout of 90000ms (9)
- GitHub Check: perform-test / rts-build / build
- GitHub Check: perform-test / server-build / server-unit-tests
- GitHub Check: client-check-cyclic-deps / check-cyclic-dependencies
- GitHub Check: client-unit-tests / client-unit-tests
- GitHub Check: client-prettier / prettier-check
- GitHub Check: client-build / client-build
- GitHub Check: chromatic-deployment
- GitHub Check: storybook-tests
- GitHub Check: chromatic-deployment
🔇 Additional comments (3)
app/client/packages/design-system/ads/src/Templates/EditableEntityName/EditableEntityName.tsx (3)
10-12: LGTM! Clean utility function implementation.The
isEllipsisActivefunction provides a clear and efficient way to detect text overflow.
101-107: LGTM! Clean tooltip props spreading.The tooltip props are cleanly spread and conditionally applied.
110-110: LGTM! Proper ref and className handling.The ref assignment and className concatenation are implemented correctly.
Also applies to: 117-117
| useEffect(() => { | ||
| setShowTooltip(!!isEllipsisActive(longNameRef.current)); | ||
| }, [name]); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add cleanup for effect hook.
The effect hook should clean up when the component unmounts.
useEffect(() => {
setShowTooltip(!!isEllipsisActive(longNameRef.current));
+
+ return () => {
+ setShowTooltip(false);
+ };
}, [name]);📝 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.
| useEffect(() => { | |
| setShowTooltip(!!isEllipsisActive(longNameRef.current)); | |
| }, [name]); | |
| useEffect(() => { | |
| setShowTooltip(!!isEllipsisActive(longNameRef.current)); | |
| return () => { | |
| setShowTooltip(false); | |
| }; | |
| }, [name]); |
Description
Removing the feature flag for using Entity Item component from ADS templates in the Entity Explorer in App Editor.
Fixes #39067
Automation
/ok-to-test tags="@tag.All"
🔍 Cypress test results
Caution
🔴 🔴 🔴 Some tests have failed.
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/13293248305
Commit: 8acb96f
Cypress dashboard.
Tags: @tag.All
Spec:
The following are new failures, please fix them before merging the PR:
- cypress/e2e/Regression/ClientSide/Autocomplete/Bugs_AC_Spec.ts
- cypress/e2e/Regression/ClientSide/ExplorerTests/API_Pane_spec.js
- cypress/e2e/Regression/ClientSide/ExplorerTests/Long_Name_Tooltip_spec.js
- cypress/e2e/Regression/ClientSide/ExplorerTests/Renaming_spec.js
- cypress/e2e/Regression/ClientSide/ExplorerTests/Tab_rename_Delete_spec.ts
- cypress/e2e/Regression/ClientSide/JSObject/RenameJSObjectBug38207_spec.ts
- cypress/e2e/Regression/ClientSide/Linting/BasicLint_spec.ts
- cypress/e2e/Regression/ClientSide/OtherUIFeatures/Omnibar_spec.js
- cypress/e2e/Regression/ClientSide/Refactoring/Refactoring_spec.ts
- cypress/e2e/Regression/ClientSide/Widgets/Button/Button_spec.js
- cypress/e2e/Regression/ClientSide/Widgets/Chart/Custom_Chart_spec.js
- cypress/e2e/Regression/ClientSide/Widgets/List/List4_2_spec.js
- cypress/e2e/Regression/ServerSide/ApiTests/API_Additional_Tests_spec.ts
List of identified flaky tests.Wed, 12 Feb 2025 21:22:54 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
UI Improvements
Refactor
Testing & Reliability