chore: AppIDE new Entity Explorer changes#39557
Conversation
WalkthroughThis pull request implements several targeted updates across various components in the design system and IDE. Changes include enhancing class names for list items, introducing new optional props (such as Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant EditableEntityName
participant DOM
participant Tooltip
User->>EditableEntityName: Enters text
EditableEntityName->>DOM: Check overflow (isEllipsisActive)
DOM-->>EditableEntityName: Returns overflow status
EditableEntityName->>EditableEntityName: useEffect updates showTooltip
EditableEntityName->>Tooltip: Conditionally render tooltip
sequenceDiagram
participant Editor
participant AppJSEditorContextMenu
participant EntityContextMenu
Editor->>AppJSEditorContextMenu: Request context menu
AppJSEditorContextMenu->>AppJSEditorContextMenu: Check jsCollection.isMainJSCollection
alt Is Main Collection
AppJSEditorContextMenu-->>Editor: Return null (skip rendering)
else
AppJSEditorContextMenu->>EntityContextMenu: Render context menu with dataTestId
EntityContextMenu-->>Editor: Display context menu
end
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 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 Navigation
hetunandu
added
ok-to-test
github-actions
bot
added
skip-changelog
app/client/packages/design-system/ads/src/Templates/EditableEntityName/EditableEntityName.tsx
Outdated
Show resolved
Hide resolved
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (8)
app/client/src/IDE/Components/EntityContextMenu.tsx (1)
30-30: Properly implemented the dataTestId propThe implementation correctly uses the provided dataTestId on the button element.
However, consider providing a default value to ensure this element always has a test identifier even when the prop is not provided.
-data-testid={props.dataTestId} +data-testid={props.dataTestId || "t--entity-context-menu-trigger"}app/client/src/pages/AppIDE/components/UIEntityListTree/WidgetContextMenu.tsx (1)
104-113: Improved MenuContent implementation with structured classNameThe use of clsx for combining classnames and the inclusion of EntityClassNames constants enhances maintainability and consistency across the codebase.
Consider extracting the width value to a constant if this width is used elsewhere in the application for similar context menus.
app/client/packages/design-system/ads/src/Templates/EntityExplorer/EntityGroupsList/EntityGroupsList.tsx (1)
72-72: Enhanced test identification with dynamic testidAdding a dynamic data-testid based on the item title improves test stability and readability.
Consider handling the case where the title might contain special characters that could cause test selector issues:
-data-testid={`entity-group-item-${(item as ListItemProps)?.title}`} +data-testid={`entity-group-item-${(item as ListItemProps)?.title?.replace(/[^a-zA-Z0-9-_]/g, '-')}`}app/client/src/pages/Editor/JSEditor/AppJSEditorContextMenu.tsx (1)
31-33: Remove redundant Boolean callThe Boolean call is unnecessary as the value will already be coerced to a boolean in the if statement.
- if (Boolean(jsCollection?.isMainJSCollection)) { + if (jsCollection?.isMainJSCollection) { return null; }🧰 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/AppIDE/components/JSEntityItem/JSEntityItem.tsx (2)
41-51: Improved context menu rendering logicThe component now conditionally prevents rendering the context menu for main JS collections, which is appropriate for these special entities.
However, the Boolean call is redundant since the condition will already be coerced to a boolean.
- if (Boolean(jsAction.isMainJSCollection)) { + if (jsAction.isMainJSCollection) {🧰 Tools
🪛 Biome (1.9.4)
[error] 42-42: 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)
62-65: Extracted permission logic into a dedicated variableEncapsulating the edit permission logic into a dedicated variable improves code readability and maintainability.
The Boolean call is redundant here as well.
- () => canManageJSAction && !Boolean(jsAction?.isMainJSCollection), + () => canManageJSAction && !jsAction?.isMainJSCollection,🧰 Tools
🪛 Biome (1.9.4)
[error] 63-63: 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/packages/design-system/ads/src/Templates/EditableEntityName/EditableEntityName.tsx (2)
72-73: Consider input placeholder implementation.The placeholder is defined in the style object rather than as a proper input property. This may not work as expected since placeholder is typically an attribute, not a style property.
- placeholder: "Name", + // Remove placeholder from style objectThen, add it to the inputProps object:
inputProps = useMemo( () => ({ ["data-testid"]: inputTestId, onKeyUp: handleKeyUp, onChange: handleTitleChange, autoFocus: true, + placeholder: "Name", style: { backgroundColor: "var(--ads-v2-color-bg)", paddingTop: "4px", paddingBottom: "4px", top: "-5px", }, }), [handleKeyUp, handleTitleChange, inputTestId], );
87-88: Consider implementing the suggested tooltip separation.The comment suggests using two separate tooltips for validation errors and long names. This might be a cleaner implementation as it would separate concerns.
Instead of conditionally configuring a single tooltip, consider:
- // Tooltip can either show the validation error or the name incase of long names - // Maybe we should use two different tooltips for this @Ankita - const tooltipProps: TooltipProps = useMemo( - () => - validationError - ? { - content: validationError, - placement: "bottom", - visible: true, - isDisabled: false, - mouseEnterDelay: 0, - showArrow: true, - } - : { - content: name, - placement: "topLeft", - visible: !showTooltip, - isDisabled: !showTooltip, - mouseEnterDelay: 1, - showArrow: false, - }, - [name, showTooltip, validationError], - ); + // Separate tooltip configurations for validation error and long names + const validationTooltipProps = useMemo( + () => ({ + content: validationError, + placement: "bottom", + visible: !!validationError, + isDisabled: !validationError, + mouseEnterDelay: 0, + showArrow: true, + }), + [validationError] + ); + + const nameTooltipProps = useMemo( + () => ({ + content: name, + placement: "topLeft", + visible: showTooltip, + isDisabled: !showTooltip, + mouseEnterDelay: 1, + showArrow: false, + }), + [name, showTooltip] + ); + + // Then in the JSX, conditionally render the appropriate tooltip + {validationError ? ( + <Tooltip {...validationTooltipProps}> + <StyledComponent /> + </Tooltip> + ) : ( + <Tooltip {...nameTooltipProps}> + <StyledComponent /> + </Tooltip> + )}
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (22)
app/client/packages/design-system/ads/src/List/List.tsx(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/EntityContextMenu/EntityContextMenu.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(4 hunks)app/client/packages/design-system/ads/src/Templates/EntityExplorer/EntityItem/EntityItem.types.ts(1 hunks)app/client/packages/design-system/ads/src/Templates/EntityExplorer/EntityListTree/EntityListTree.stories.tsx(2 hunks)app/client/packages/design-system/ads/src/Templates/EntityExplorer/EntityListTree/EntityListTree.test.tsx(6 hunks)app/client/packages/design-system/ads/src/Templates/EntityExplorer/EntityListTree/EntityListTree.tsx(1 hunks)app/client/packages/design-system/ads/src/Templates/EntityExplorer/EntityListTree/EntityListTree.types.ts(1 hunks)app/client/packages/design-system/ads/src/__hooks__/useEditableText/useEditableText.ts(2 hunks)app/client/src/IDE/Components/EntityContextMenu.tsx(2 hunks)app/client/src/pages/AppIDE/components/AppPluginActionEditor/components/ContextMenuItems/Copy.tsx(1 hunks)app/client/src/pages/AppIDE/components/AppPluginActionEditor/components/ContextMenuItems/Move.tsx(1 hunks)app/client/src/pages/AppIDE/components/JSEntityItem/JSEntityItem.tsx(5 hunks)app/client/src/pages/AppIDE/components/UIEntityListTree/UIEntityListTree.tsx(1 hunks)app/client/src/pages/AppIDE/components/UIEntityListTree/WidgetContextMenu.tsx(3 hunks)app/client/src/pages/AppIDE/components/UIEntityListTree/WidgetTreeItem.tsx(5 hunks)app/client/src/pages/Editor/JSEditor/AppJSEditorContextMenu.tsx(1 hunks)app/client/src/pages/Editor/JSEditor/ContextMenuItems/Copy.tsx(1 hunks)app/client/src/pages/Editor/JSEditor/ContextMenuItems/Move.tsx(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- app/client/packages/design-system/ads/src/List/List.tsx
🧰 Additional context used
🪛 Biome (1.9.4)
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/pages/AppIDE/components/JSEntityItem/JSEntityItem.tsx
[error] 42-42: 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)
[error] 63-63: 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 / server-build / server-unit-tests
- GitHub Check: perform-test / rts-build / build
- GitHub Check: client-unit-tests / client-unit-tests
- GitHub Check: client-check-cyclic-deps / check-cyclic-dependencies
- GitHub Check: client-build / client-build
- GitHub Check: client-prettier / prettier-check
- GitHub Check: client-lint / client-lint
- GitHub Check: chromatic-deployment
- GitHub Check: storybook-tests
- GitHub Check: chromatic-deployment
🔇 Additional comments (39)
app/client/src/pages/Editor/JSEditor/ContextMenuItems/Copy.tsx (1)
34-34: Proper disabled state propagation implemented.The addition of the
disabledprop toMenuSubTriggerensures consistent state propagation from parent to child components.app/client/packages/design-system/ads/src/Templates/EntityExplorer/EntityItem/EntityItem.types.ts (1)
24-25: Well-documented enhancement for name normalization.The addition of the optional
normalizeNameproperty with appropriate documentation provides flexible control over entity name handling during the rename process.app/client/packages/design-system/ads/src/Templates/EntityExplorer/EntityListTree/EntityListTree.types.ts (1)
7-7:Details
❓ Verification inconclusive
Verify all implementations include the new required property.
Adding the required
nameproperty toEntityListTreeIteminterface enhances identification but might cause breaking changes if not all implementations are updated.
🏁 Script executed:
#!/bin/bash # Search for implementations of EntityListTreeItem to verify they include the name property # Find where EntityListTreeItem is constructed or defined rg "EntityListTreeItem\s*(\{|\[)" --type tsLength of output: 467
Action Required: Verify that all instantiations of EntityListTreeItem include the new
namepropertyThe interface update in
app/client/packages/design-system/ads/src/Templates/EntityExplorer/EntityListTree/EntityListTree.types.ts
now mandates aname: string;property. Our repository search only shows the updated interface definition and no obvious literal implementations missing this property.
- Confirm that any object literal or constructor using
EntityListTreeItemsupplies a validnamevalue.- Double-check areas invoking this interface (even outside the immediate file) to prevent potential type errors.
app/client/src/pages/Editor/JSEditor/ContextMenuItems/Move.tsx (1)
44-44: Proper disabled state propagation implemented.The addition of the
disabledprop toMenuSubTriggerensures proper state reflection and maintains UI consistency with the Copy component implementation.app/client/src/IDE/Components/EntityContextMenu.tsx (1)
12-12: Good addition for improving component testabilityAdding an optional dataTestId prop allows for more flexible testing configurations.
app/client/src/pages/AppIDE/components/UIEntityListTree/UIEntityListTree.tsx (1)
18-18: Appropriate addition of name propertyIncluding the widget's name in the enhanced items tree is a good approach for maintaining proper identification in the UI.
app/client/src/pages/AppIDE/components/UIEntityListTree/WidgetContextMenu.tsx (1)
25-26: Good addition of necessary importsThe imports for EntityClassNames and clsx appropriately support the updated component implementation.
app/client/packages/design-system/ads/src/Templates/EntityExplorer/EntityListTree/EntityListTree.tsx (2)
52-54: Enhanced icon state representation in DOMThe addition of the
data-iconattribute improves component testability by making the current expansion state directly accessible in the DOM structure.
56-56: Updated test identifier for better semantic clarityRenaming the test ID from what appears to be
t--entity-item-expand-icontot--entity-collapse-togglebetter reflects the component's purpose, which is to toggle the collapse state.app/client/src/pages/AppIDE/components/AppPluginActionEditor/components/ContextMenuItems/Copy.tsx (1)
34-34: Added disabled state propagation to MenuSubTriggerGood implementation of the disabled state. The menu trigger now respects the parent component's disabled state, ensuring consistent UI behavior and preventing confusing user interactions.
app/client/packages/design-system/ads/src/Templates/EditableEntityName/EditableEntityName.types.ts (1)
27-29:Details
✅ Verification successful
Added configuration options for name handling and display
The addition of
normalizeNameandshowEllipsisoptional properties enhances the flexibility of the component. These well-documented props allow for better control over naming conventions and text overflow handling.Make sure to implement these properties correctly in the component implementation file to ensure proper name normalization and ellipsis behavior.
🏁 Script executed:
#!/bin/bash # Check if the new props are properly implemented in the component rg -A 5 -B 5 "normalizeName|showEllipsis" --type typescript app/client/packages/design-system/ads/src/Templates/EditableEntityName/EditableEntityName.tsxLength of output: 190
🏁 Script executed:
#!/bin/bash # Re-run searchable pattern to verify the new props implementation in the component without using an unrecognized file type flag. rg -A 5 -B 5 "normalizeName|showEllipsis" app/client/packages/design-system/ads/src/Templates/EditableEntityName/EditableEntityName.tsxLength of output: 1432
Confirmed: Configuration Options Implemented Correctly
The new props (
normalizeNameandshowEllipsis) have been successfully integrated. Verification confirms that:
- They are declared as optional properties in the types file with clear documentation.
- They are appropriately destructured with default values in the component file.
- The ellipsis behavior is properly handled via the conditional logic in the
useEffecthook.No further changes are needed.
app/client/src/pages/AppIDE/components/AppPluginActionEditor/components/ContextMenuItems/Move.tsx (1)
45-45: Added disabled state propagation to MenuSubTriggerEffective implementation of the disabled state for the Move operation. This change ensures UI consistency with the Copy operation and provides appropriate visual feedback to users when actions are unavailable.
app/client/packages/design-system/ads/src/Templates/EntityContextMenu/EntityContextMenu.tsx (3)
17-17: Standardizing prop naming conventionUpdated prop name from
dataTestidtodataTestIdfor consistency with other component props in the codebase, improving maintainability.
25-25: Updated destructuring to match interface changeCorrectly updated the destructured prop name to match the interface change.
43-43: Updated attribute to use new prop nameThe
data-testidattribute now correctly uses the renameddataTestIdprop.app/client/src/pages/Editor/JSEditor/AppJSEditorContextMenu.tsx (1)
36-36: Updated to use consistent dataTestId propAdded the test ID for the EntityContextMenu component in line with the prop renaming in the component definition.
app/client/packages/design-system/ads/src/Templates/EntityExplorer/EntityItem/EntityItem.tsx (4)
20-20: Added normalizeName prop with default valueNew optional property added to control name normalization behavior.
38-38: Enhanced EditableEntityName with additional propsAdded normalizeName and showEllipsis props to improve the name display functionality:
- normalizeName: Controls name normalization behavior
- showEllipsis: Shows ellipsis for text overflow
Also applies to: 41-41
50-50: Updated useMemo dependency arrayCorrectly added normalizeName to the dependency array of useMemo to ensure the cached value is recalculated when this prop changes.
72-72: Updated data-testId attribute caseChanged attribute from
data-testidtodata-testIdto maintain consistent naming convention across components.app/client/packages/design-system/ads/src/Templates/EntityExplorer/EntityListTree/EntityListTree.stories.tsx (2)
36-36: Direct name property assignmentSimplified data structure by adding name property directly to tree items instead of using a separate names mapping object. This improves code maintainability and readability.
Also applies to: 42-42, 48-48, 55-55, 63-63, 71-71
111-111: Updated to use direct name referenceModified to directly access item.name instead of looking up in a separate mapping object, aligning with the tree structure changes.
app/client/packages/design-system/ads/src/__hooks__/useEditableText/useEditableText.ts (2)
22-22: Enhanced function signature with normalization controlAdding the optional
normalizeNameparameter provides flexibility for consumers of this hook to control the name normalization behavior.
91-93: Well-implemented conditional name normalizationThe implementation correctly uses the
normalizeNameflag to conditionally apply normalization, maintaining backward compatibility while adding new functionality.app/client/src/pages/AppIDE/components/UIEntityListTree/WidgetTreeItem.tsx (2)
15-15: Improved type safety with explicit typingAdding explicit type imports and annotations for the widget variable enhances type safety and code clarity.
Also applies to: 18-18
40-42: Refactored to use direct item properties instead of widget propertiesThe component now consistently uses properties from the
itemparameter rather than retrieving them from the widget object. This improves robustness by removing potential null reference issues and simplifies the code.Also applies to: 44-49, 71-73, 81-83, 87-90, 102-102
app/client/src/pages/AppIDE/components/JSEntityItem/JSEntityItem.tsx (2)
23-23: Added conditional class names with clsxUsing clsx for managing conditional class names is a good practice that improves readability and maintainability.
Also applies to: 110-112
114-114: Fixed selection logic to use item keyUsing
item.keyfor the selection comparison instead ofjsAction.idaligns with the Entity Explorer's selection model.app/client/packages/design-system/ads/src/Templates/EntityExplorer/EntityListTree/EntityListTree.test.tsx (2)
12-12: Simplified ItemComponent to use direct name propertyThe component now directly accesses the name from the item object, which simplifies the code and aligns with the updated data structure.
23-24: Updated test data and queries to match new component structureTests now include name properties in the test data and use role-based queries with name attributes, which is more robust and matches the new component implementation.
Also applies to: 30-31, 47-47, 62-63, 69-69, 86-87, 93-94, 103-105, 111-111
app/client/packages/design-system/ads/src/Templates/EditableEntityName/EditableEntityName.tsx (9)
12-14: Clean utility function for checking text overflow.Good abstraction that encapsulates overflow detection logic into a reusable function.
25-26: Props correctly introduced with default values.The new props
normalizeNameandshowEllipsisare properly defined with sensible defaults, promoting flexibility while maintaining backward compatibility.Also applies to: 28-29
34-35: State management for tooltip visibility.Good approach using dedicated state variable and ref for tracking overflow condition.
78-85: Effect for detecting text overflow.The implementation correctly handles the ellipsis detection and updates tooltip visibility accordingly.
117-118: Improved class naming with clsx.Good use of the clsx utility to conditionally apply the editing class.
124-125: Conditional ref application.The ref is correctly applied only when the showEllipsis prop is true, avoiding unnecessary DOM references.
1-1: Properly organized imports.Good organization of React hooks in the import statement.
49-50:Details
❓ Verification inconclusive
Verify normalizeName implementation in useEditableText.
The
normalizeNameprop is now passed to theuseEditableTexthook. Ensure the hook implementation properly handles this parameter.
🏁 Script executed:
#!/bin/bash # Check the implementation of useEditableText echo "Checking useEditableText hook implementation..." rg -A 20 "export const useEditableText" --type tsx --glob "**/packages/design-system/ads/src/**"Length of output: 231
Action Required: Verify
normalizeNameHandling inuseEditableTextHookIt appears our initial script using
--type tsxdid not yield results due to an unrecognized file type. Please either re-run the search with the appropriate file type flag (e.g.,--type typescriptreact) or manually inspect theuseEditableTexthook implementation to ensure that thenormalizeNameprop is properly processed.
- Verify that the normalization logic in
useEditableTextcorrectly applies thenormalizeNamefunction.- Confirm that any related tests cover scenarios involving this parameter.
87-109:Details
❓ Verification inconclusive
Tooltip logic needs a review.
There's a potential logic issue with the tooltip visibility configuration.
For validation errors:
visible: true(always visible)isDisabled: false(tooltip enabled)For name tooltips:
visible: !showTooltip(hidden when showTooltip is true)isDisabled: !showTooltip(disabled when showTooltip is true)This appears contradictory since
showTooltipstate is meant to control when tooltips should appear.
🏁 Script executed:
#!/bin/bash # Check if there are any similar tooltip implementations in the codebase echo "Searching for similar tooltip implementations..." rg -A 3 -B 3 "content: .+, placement: .+, visible: .+" --type tsx --glob "!**/node_modules/**"Length of output: 231
I've noticed that the original search command didn't work as expected due to an unrecognized file type ("tsx"). To gather a more reliable result, please run a revised shell script that explicitly filters for TSX files:
This should help us verify whether similar tooltip configurations exist elsewhere in the codebase. Based on the review of the snippet, there is a potential inversion in the logic controlling the tooltip for non-error cases (i.e., using
!showTooltipfor bothvisibleandisDisabled). Once you verify the output from the above script, please re-check if the tooltip state (showTooltip) is handled consistently throughout the project.Action Required:
- Run the revised shell script to confirm how tooltip properties are configured in other TSX files.
- Double-check if the tooltip's logic (for instance, when
showTooltipis true/false) is consistent with the intended behavior.
🏁 Script executed:
#!/bin/bash echo "Searching for similar tooltip implementations in TSX files..." rg -A 3 -B 3 "content: .+, placement: .+, visible:" -g "*.tsx"Length of output: 319
Tooltip Logic Requires Careful Verification
The current implementation uses
!showTooltipfor bothvisibleandisDisabledin the non-error branch, which is potentially counterintuitive ifshowTooltipis meant to directly control the tooltip’s display state. No similar implementations were identified elsewhere in the codebase, so it’s unclear whether this was intended or an oversight.
- Verify that
showTooltipis defined and used consistently in this context.- Consider whether the tooltip for validation errors and the regular name tooltip should have distinct state controls.
- If the intent is to display the tooltip when
showTooltipis true, then using!showTooltipmay need refactoring or a clearer separation of concerns.
app/client/src/pages/AppIDE/components/UIEntityListTree/WidgetContextMenu.tsx
Show resolved
Hide resolved
app/client/packages/design-system/ads/src/Templates/EditableEntityName/EditableEntityName.tsx
Outdated
Show resolved
Hide resolved
...ackages/design-system/ads/src/Templates/EntityExplorer/EntityGroupsList/EntityGroupsList.tsx
Outdated
Show resolved
Hide resolved
app/client/packages/design-system/ads/src/Templates/EntityExplorer/EntityItem/EntityItem.tsx
Outdated
Show resolved
Hide resolved
...nt/packages/design-system/ads/src/Templates/EntityExplorer/EntityListTree/EntityListTree.tsx
Show resolved
Hide resolved
app/client/src/pages/AppIDE/components/UIEntityListTree/WidgetContextMenu.tsx
Outdated
Show resolved
Hide resolved
app/client/src/pages/AppIDE/components/UIEntityListTree/WidgetTreeItem.tsx
Show resolved
Hide resolved
ankitakinger
left a comment
ankitakinger
left a comment
There was a problem hiding this comment.
You have missed the following file changes:
app/client/src/pages/Editor/Explorer/Entity/Name.tsx
app/client/src/pages/AppIDE/components/PageList/PageElement.tsx
app/client/src/pages/AppIDE/components/PageList/PagesSection.tsx
app/client/src/pages/AppIDE/components/QueryEntityItem/QueryEntityItem.tsx
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
app/client/packages/design-system/ads/src/Templates/EditableEntityName/EditableEntityName.tsx (1)
78-85: Optimize effect dependencies.The effect only tracks changes to
editableNameandshowEllipsis, but the DOM element's dimensions could change for other reasons (like window resize).useEffect( function handleShowTooltipOnEllipsis() { if (showEllipsis) { setShowTooltip(!!isEllipsisActive(longNameRef.current)); } }, - [editableName, showEllipsis], + [editableName, showEllipsis, name], );Consider also adding a window resize listener if the component needs to respond to layout changes.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
app/client/packages/design-system/ads/src/Templates/EditableEntityName/EditableEntityName.tsx(4 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(4 hunks)app/client/src/IDE/Components/EntityContextMenu.tsx(2 hunks)app/client/src/pages/AppIDE/components/UIEntityListTree/WidgetContextMenu.tsx(3 hunks)app/client/src/pages/AppIDE/components/UIEntityListTree/WidgetTreeItem.tsx(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- app/client/src/pages/AppIDE/components/UIEntityListTree/WidgetContextMenu.tsx
- app/client/packages/design-system/ads/src/Templates/EntityExplorer/EntityItem/EntityItem.tsx
- app/client/src/IDE/Components/EntityContextMenu.tsx
- app/client/packages/design-system/ads/src/Templates/EntityExplorer/EntityGroupsList/EntityGroupsList.tsx
- app/client/src/pages/AppIDE/components/UIEntityListTree/WidgetTreeItem.tsx
⏰ Context from checks skipped due to timeout of 90000ms (11)
- GitHub Check: perform-test / server-build / server-unit-tests
- GitHub Check: perform-test / rts-build / build
- GitHub Check: perform-test / client-build / client-build
- 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: client-lint / client-lint
- GitHub Check: chromatic-deployment
- GitHub Check: storybook-tests
- GitHub Check: chromatic-deployment
🔇 Additional comments (8)
app/client/packages/design-system/ads/src/Templates/EditableEntityName/EditableEntityName.tsx (8)
12-14: Good utility function for checking text overflow.This is a clean implementation to detect if text is overflowing its container.
25-28: New props improve component flexibility.The props
normalizeNameandshowEllipsiswith sensible default values allow for better customization of the component behavior.
34-35: Proper state tracking for tooltip visibility.Good implementation using local state and ref to control when to show tooltips.
49-49: Correct propagation of normalizeName param.Passing the normalizeName prop to the useEditableText hook ensures consistent behavior.
72-72: Added placeholder improves UX.Adding a placeholder text for the input field enhances user experience.
117-117: Using clsx for conditional classnames is good practice.The use of
clsxto conditionally apply the editing class is clean and follows best practices.
124-124: Conditional ref assignment is appropriate.Only setting the ref when
showEllipsisis true prevents unnecessary DOM operations.
87-109:Details
❓ Verification inconclusive
Verify tooltip behavior for dual purposes.
The tooltip now serves two different purposes (showing validation errors and full names) which might confuse users.
In response to previous comment from @ankitakinger: "how would you add 2 different tooltips when it has to be on the same element?"
This implementation technically handles both cases, but consider if two separate tooltip components would be clearer.
🏁 Script executed:
#!/bin/bash # Search for other components that may use this pattern to ensure consistency rg -t tsx "isDisabled: !showTooltip" --glob "!**/EditableEntityName.tsx"Length of output: 102
Action Required: Verify Dual-Purpose Tooltip Consistency
The current implementation uses a single tooltip to display either validation errors or the full entity name, which might lead to user confusion. Before deciding if a refactor (such as splitting into two distinct tooltip components) is needed, we should verify that this dual-purpose pattern isn’t used elsewhere unexpectedly in the codebase.
A previous search for the pattern produced an error due to an unrecognized file type ("tsx"). Please re-run the modified search to check for consistent usage:
Once you review the output:
- If similar patterns exist, consider evaluating whether a dedicated tooltip for each case could improve clarity.
- If not, the dual-purpose tooltip may be acceptable, but bear in mind the potential for user confusion.
🏁 Script executed:
#!/bin/bash # Re-run the search for 'isDisabled: !showTooltip' in TSX files (excluding EditableEntityName.tsx), # ensuring TSX files are recognized. rg --type-add='tsx:*.tsx' -t tsx "isDisabled: !showTooltip" --glob "!**/EditableEntityName.tsx"Length of output: 219
Action: Reassess Dual-Purpose Tooltip Usage
Although a search for the pattern outside of EditableEntityName did not yield additional matches, the dual-use of the tooltip—handling both validation errors and full entity names—could still confuse users. There’s no evidence of this pattern being consistently implemented across the codebase, so please verify that this design is intentional. Consider if using two distinct tooltip components might provide clearer UX, especially when a validation error occurs.
- Confirm that the combined tooltip behavior meets UX expectations.
- Evaluate whether splitting the tooltip for error messages and for displaying lengthy names improves clarity.
- Perform manual testing to ensure that users understand the tooltip’s dual role.
|
/build-deploy-preview skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/13670430773. |
|
Deploy-Preview-URL: https://ce-39557.dp.appsmith.com |
|
@ankitakinger Ill look into and the page list conversion in a different PR |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
app/client/src/pages/Editor/Explorer/Entity/Name.tsx (1)
48-48: Element type change improves component rendering consistencyChanging from
divtospanaligns with expected inline rendering for this wrapper. Since this component wraps editable text within a form context, using an inline element is semantically more appropriate.This change addresses potential layout inconsistencies, but consider adding a display style property to ensure consistent rendering across browsers:
const EditableWrapper = styled.span` + display: inline-block; overflow: hidden; margin: 0 ${(props) => props.theme.spaces[1]}px; padding: ${(props) => props.theme.spaces[3] + 1}px 0; line-height: 13px; `;
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
app/client/src/pages/AppIDE/components/QueryEntityItem/QueryEntityItem.tsx(3 hunks)app/client/src/pages/Editor/Explorer/Entity/Name.tsx(1 hunks)
⏰ 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-prettier / prettier-check
- GitHub Check: client-build / client-build
- GitHub Check: storybook-tests
- GitHub Check: chromatic-deployment
- GitHub Check: chromatic-deployment
🔇 Additional comments (6)
app/client/src/pages/AppIDE/components/QueryEntityItem/QueryEntityItem.tsx (6)
53-55: Added dataTestId for improved test selection.Adding a test identifier enhances component testability. Good practice for automated testing.
60-60: Improved null safety with optional chaining.Using
action?.userPermissions || []prevents runtime errors when action is undefined or null.
77-78: Properly retrieving icon from action configuration.Using the config's getIcon method ensures correct icon rendering based on the action and plugin type, which is more reliable than using the direct item.icon reference.
111-115: Fixed useMemo dependencies for correct recalculation.Added missing dependencies to the useMemo array ensures the memoized nameEditorConfig is properly updated when these dependencies change.
121-121: Updated selection logic to use consistent identifiers.Changed comparison from action.id to item.key for selection state, which aligns with how the component receives and manages its data structure.
128-128: Using derived icon variable for consistent rendering.Correctly leveraging the derived icon variable from the action config instead of directly using item.icon.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app/client/packages/design-system/ads/src/Templates/EditableEntityName/EditableEntityName.tsx(4 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (11)
- GitHub Check: perform-test / server-build / server-unit-tests
- GitHub Check: perform-test / rts-build / build
- GitHub Check: perform-test / 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: client-build / client-build
- GitHub Check: client-prettier / prettier-check
- GitHub Check: chromatic-deployment
- GitHub Check: storybook-tests
- GitHub Check: chromatic-deployment
🔇 Additional comments (8)
app/client/packages/design-system/ads/src/Templates/EditableEntityName/EditableEntityName.tsx (8)
1-4: Import additions are appropriate for new functionality.The added imports for React hooks and TooltipProps properly support the new tooltip and element reference functionality.
12-14: Utility function implementation is sound.This utility function provides a clean way to detect text overflow, which is essential for the tooltip visibility logic.
25-28: Props with default values maintain backward compatibility.Good practice adding default values for the new props, ensuring existing implementations won't break.
34-35: State and ref for tooltip functionality is well structured.The state and ref declarations are appropriately scoped for managing tooltip visibility based on text overflow.
49-49: Hook updated to support name normalization.The normalizeName prop is correctly passed to the useEditableText hook.
78-85: Effective tooltip visibility management with proper dependencies.The useEffect correctly manages tooltip visibility based on text overflow and includes appropriate dependencies.
88-107: Well-structured tooltip props with proper memoization.The tooltip props are conditionally structured based on validation state and visually managed through the showTooltip state. The useMemo optimization is appropriate for performance.
112-123: Enhanced rendering with conditional ref and improved class management.The conditional ref assignment and clsx usage for class names improve the component's flexibility and readability.
I see this addresses the previous comment about handling multiple tooltips on the same element.
| paddingTop: "4px", | ||
| paddingBottom: "4px", | ||
| top: "-5px", | ||
| placeholder: "Name", |
There was a problem hiding this comment.
Incorrect use of 'placeholder' in style object.
The 'placeholder' property isn't a valid CSS style property and should be moved to the input attributes.
- placeholder: "Name",
This should be added to the inputProps object instead:
inputProps = useMemo(
() => ({
["data-testid"]: inputTestId,
onKeyUp: handleKeyUp,
onChange: handleTitleChange,
autoFocus: true,
+ placeholder: "Name",
style: {
backgroundColor: "var(--ads-v2-color-bg)",
paddingTop: "4px",
paddingBottom: "4px",
top: "-5px",
},
}),
[handleKeyUp, handleTitleChange, inputTestId],
);
📝 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.
| placeholder: "Name", | |
| - placeholder: "Name", | |
| + // Removed placeholder from the style object. | |
| - // ... rest of the code in the style object ... | |
| + inputProps = useMemo( | |
| + () => ({ | |
| + ["data-testid"]: inputTestId, | |
| + onKeyUp: handleKeyUp, | |
| + onChange: handleTitleChange, | |
| + autoFocus: true, | |
| + placeholder: "Name", | |
| + style: { | |
| + backgroundColor: "var(--ads-v2-color-bg)", | |
| + paddingTop: "4px", | |
| + paddingBottom: "4px", | |
| + top: "-5px", | |
| + }, | |
| + }), | |
| + [handleKeyUp, handleTitleChange, inputTestId], | |
| + ); |
app/client/packages/design-system/ads/src/Templates/EditableEntityName/EditableEntityName.tsx
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
app/client/packages/design-system/ads/src/Templates/EditableEntityName/EditableEntityName.tsx (1)
72-72:⚠️ Potential issueMove 'placeholder' from style object to input attributes.
The 'placeholder' property isn't a valid CSS style property and should be moved to the inputProps object.
- placeholder: "Name",This should be added to the inputProps object instead:
inputProps = useMemo( () => ({ ["data-testid"]: inputTestId, onKeyUp: handleKeyUp, onChange: handleTitleChange, autoFocus: true, + placeholder: "Name", style: { backgroundColor: "var(--ads-v2-color-bg)", paddingTop: "4px", paddingBottom: "4px", top: "-5px", }, }), [handleKeyUp, handleTitleChange, inputTestId], );
🧹 Nitpick comments (1)
app/client/packages/design-system/ads/src/Templates/EditableEntityName/EditableEntityName.tsx (1)
78-85: Add window resize listener for tooltip visibility.The current implementation only checks for text overflow when
editableNameorshowEllipsischanges, but not when window or container size changes.useEffect( function handleShowTooltipOnEllipsis() { if (showEllipsis) { setShowTooltip(!!isEllipsisActive(longNameRef.current)); } }, [editableName, showEllipsis], ); +useEffect(() => { + if (!showEllipsis) return; + + const handleResize = () => { + setShowTooltip(!!isEllipsisActive(longNameRef.current)); + }; + + window.addEventListener("resize", handleResize); + return () => window.removeEventListener("resize", handleResize); +}, [showEllipsis]);
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app/client/packages/design-system/ads/src/Templates/EditableEntityName/EditableEntityName.tsx(4 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (10)
- GitHub Check: perform-test / rts-build / 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-unit-tests / client-unit-tests
- GitHub Check: client-prettier / prettier-check
- GitHub Check: chromatic-deployment
- GitHub Check: chromatic-deployment
- GitHub Check: storybook-tests
🔇 Additional comments (7)
app/client/packages/design-system/ads/src/Templates/EditableEntityName/EditableEntityName.tsx (7)
12-14: Helper function to detect text overflow looks good.The
isEllipsisActiveutility function provides a clean way to detect text overflow and will be useful for dynamic tooltip display.
25-28: Props added with sensible defaults.The new props
normalizeNameandshowEllipsishave appropriate default values set to false, maintaining backward compatibility with existing usage.
34-35: State and ref for tooltip management.Good addition of state and ref to handle tooltip visibility based on text overflow.
49-49: Normalization flag passed to hook.The
normalizeNameflag is correctly passed to theuseEditableTexthook to ensure consistent behavior.
90-97: Tooltip configuration looks good.The primary tooltip shows the full name when text is truncated, with appropriate delay and positioning.
98-105: Nested tooltip pattern for validation errors.The nested tooltip pattern allows for showing both the full name and validation errors without conflict. This is a valid approach based on the previous review comments about needing different tooltips on the same element.
106-118: Enhanced text component with accessibility improvements.The use of
aria-invalidand class handling withclsximproves both accessibility and styling flexibility. The conditional ref assignment is also a good optimization.
|
/build-deploy-preview skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/13673826456. |
|
Deploy-Preview-URL: https://ce-39557.dp.appsmith.com |
## Description Just UI changes from appsmithorg#39093 Fixes appsmithorg#39556 ## Automation /ok-to-test tags="@tag.IDE" ### 🔍 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/13673781581> > Commit: b78e21d > <a href="https://internal.appsmith.com/app/cypress-dashboard/rundetails-65890b3c81d7400d08fa9ee5?branch=master&workflowId=13673781581&attempt=1" target="_blank">Cypress dashboard</a>. > Tags: `@tag.IDE` > Spec: > <hr>Wed, 05 Mar 2025 10:57:58 UTC <!-- end of auto-generated comment: Cypress test results --> ## Communication Should the DevRel and Marketing teams inform users about this change? - [ ] Yes - [ ] No <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **New Features** - Enhanced editable text components now support name normalization with dynamic ellipsis and improved tooltip handling. - Entity and widget list views now display names directly with updated expand/collapse icons and refined selection behavior. - Context menus reflect conditional interactivity by disabling options based on permissions and item types. - New identifiers for list items and context menus improve testing capabilities. - **Refactor** - Consolidated property handling and class name assignments across components for consistent behavior. - Streamlined data structures and conditional rendering in entity explorers and context menus to enhance clarity and maintainability. - Updated components to use direct property access for improved performance and readability. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
2 participants
Description
Just UI changes from #39093
Fixes #39556
Automation
/ok-to-test tags="@tag.IDE"
🔍 Cypress test results
Tip
🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/13673781581
Commit: b78e21d
Cypress dashboard.
Tags:
@tag.IDESpec:
Wed, 05 Mar 2025 10:57:58 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
New Features
Refactor