feat: ADS Entity List Tree #38493
feat: ADS Entity List Tree #38493
Conversation
…o feat/ads/entity-item
…/appsmith into feat/ads/entity-item
…o feat/ads/entity-item
…o feat/ads/entity-item
…/appsmith into feat/ads/entity-item
WalkthroughThe pull request introduces a comprehensive implementation of the Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (11)
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
Enhancement
|
/build-deploy-preview |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/12704712525. |
|
Deploy-Preview-URL: https://ce-38493.dp.appsmith.com |
![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 (2)
app/client/packages/design-system/ads/src/Templates/EntityExplorer/EntityListTree/EntityListTree.tsx (2)
43-58: Consider using optional chainingThe condition
item.children && item.children.lengthcould be simplified.-{item.children && item.children.length ? ( +{item.children?.length ? (🧰 Tools
🪛 Biome (1.9.4)
[error] 43-43: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
63-69: Consider memoizing recursive componentThe recursive rendering of
EntityListTreecould benefit from memoization to prevent unnecessary re-renders in large trees.Consider wrapping the component with React.memo:
export const EntityListTree = React.memo(function EntityListTree(props: EntityListTreeProps) { // ... existing implementation });
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
app/client/packages/design-system/ads/src/Templates/EntityExplorer/EmptyState/EmptyState.tsx(1 hunks)app/client/packages/design-system/ads/src/Templates/EntityExplorer/EmptyState/EmptyState.types.ts(1 hunks)app/client/packages/design-system/ads/src/Templates/EntityExplorer/EntityItem/EntityItem.styles.ts(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/EntityListTree/EntityListTree.styles.ts(1 hunks)app/client/packages/design-system/ads/src/Templates/EntityExplorer/EntityListTree/EntityListTree.tsx(1 hunks)app/client/packages/design-system/ads/src/Templates/EntityExplorer/ExplorerContainer/ExplorerContainer.tsx(1 hunks)app/client/packages/design-system/ads/src/Templates/EntityExplorer/NoSearchResults/NoSearchResults.tsx(1 hunks)app/client/packages/design-system/ads/src/Templates/EntityExplorer/SearchAndAdd/SearchAndAdd.styles.tsx(1 hunks)app/client/packages/design-system/ads/src/Templates/EntityExplorer/SearchAndAdd/SearchAndAdd.tsx(1 hunks)
✅ Files skipped from review due to trivial changes (4)
- app/client/packages/design-system/ads/src/Templates/EntityExplorer/SearchAndAdd/SearchAndAdd.styles.tsx
- app/client/packages/design-system/ads/src/Templates/EntityExplorer/EntityItem/EntityItem.styles.ts
- app/client/packages/design-system/ads/src/Templates/EntityExplorer/NoSearchResults/NoSearchResults.tsx
- app/client/packages/design-system/ads/src/Templates/EntityExplorer/EmptyState/EmptyState.tsx
🚧 Files skipped from review as they are similar to previous changes (2)
- app/client/packages/design-system/ads/src/Templates/EntityExplorer/EntityItem/EntityItem.tsx
- app/client/packages/design-system/ads/src/Templates/EntityExplorer/EntityListTree/EntityListTree.styles.ts
🧰 Additional context used
🪛 Biome (1.9.4)
app/client/packages/design-system/ads/src/Templates/EntityExplorer/EntityListTree/EntityListTree.tsx
[error] 43-43: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
⏰ 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: 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: chromatic-deployment
- GitHub Check: chromatic-deployment
- GitHub Check: storybook-tests
🔇 Additional comments (5)
app/client/packages/design-system/ads/src/Templates/EntityExplorer/ExplorerContainer/ExplorerContainer.tsx (2)
2-3: LGTM! Import paths are now more direct and maintainable.The refactored imports improve code organization by using direct paths.
Line range hint
6-19: Well-structured component with proper TypeScript typing.The component follows React best practices with:
- Proper prop typing
- Consistent use of the Flex component
- Clean prop spreading
app/client/packages/design-system/ads/src/Templates/EntityExplorer/EmptyState/EmptyState.types.ts (1)
1-2: LGTM! Improved import organizationThe split imports provide better clarity on the source of each type.
app/client/packages/design-system/ads/src/Templates/EntityExplorer/SearchAndAdd/SearchAndAdd.tsx (1)
3-3: LGTM! Aligned with design system structureImport path updated to use relative path within the design system.
app/client/packages/design-system/ads/src/Templates/EntityExplorer/EntityListTree/EntityListTree.tsx (1)
16-27: LGTM! Well-implemented event handlerThe
useCallbackoptimization and event propagation handling are correctly implemented.
...nt/packages/design-system/ads/src/Templates/EntityExplorer/EntityListTree/EntityListTree.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (5)
app/client/packages/design-system/ads/src/Templates/EntityExplorer/EntityListTree/EntityListTree.test.tsx (1)
81-112: Add test coverage for disabled state and selection behavior.The test suite should verify:
- Disabled items cannot be expanded/selected
- Selection state is correctly managed
- Name editor interactions work as expected
app/client/src/pages/Editor/IDE/EditorPane/UI/UIEntityListTree/hooks/useWidgetTreeExpandedState.ts (1)
22-25: Optimize array operations for better performance.Consider using Set for better performance with large arrays:
- setExpandedWidgets((prev) => [ - ...prev, - ...widgetsToExpand.filter((widgetId) => !prev.includes(widgetId)), - ]); + setExpandedWidgets((prev) => { + const prevSet = new Set(prev); + widgetsToExpand.forEach(id => prevSet.add(id)); + return Array.from(prevSet); + });app/client/packages/design-system/ads/src/Templates/EntityExplorer/EntityListTree/EntityListTree.tsx (1)
51-64: Optimize conditional rendering with optional chaining.Use optional chaining for cleaner code:
- {item.children && item.children.length ? ( + {item.children?.length ? (🧰 Tools
🪛 Biome (1.9.4)
[error] 51-51: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
app/client/src/pages/Editor/IDE/EditorPane/UI/UIEntityListTree/UIEntityListTree.tsx (2)
19-31: Consider memoizing selector resultsThe selected widgets and permissions checks could be expensive operations. Consider memoizing these values using
useMemoto prevent unnecessary recalculations.- const widgets = useSelector(selectWidgetsForCurrentPage); - const selectedWidgets = useSelector(getSelectedWidgets); + const widgets = useSelector(selectWidgetsForCurrentPage, React.memo); + const selectedWidgets = useSelector(getSelectedWidgets, React.memo);
47-73: Optimize item enhancement to prevent unnecessary re-rendersThe enhancement function and its callbacks are recreated on every render. Consider memoizing both the enhancement function and its callbacks.
+ const handleClick = useCallback((e, widget) => switchToWidget(e, widget), [switchToWidget]); + const handleDoubleClick = useCallback((widgetId) => enterEditMode(widgetId), [enterEditMode]); + const enhanceWidget = useCallback((widget) => ({ + id: widget.widgetId, + title: widget.widgetName, + startIcon: <WidgetTypeIcon type={widget.type} />, + isSelected: selectedWidgets.includes(widget.widgetId), + isExpanded: expandedWidgets.includes(widget.widgetId), + onClick: (e) => handleClick(e, widget), + onDoubleClick: () => handleDoubleClick(widget.widgetId), + // ... rest of the enhancement logic + }), [selectedWidgets, expandedWidgets, handleClick, handleDoubleClick]); - const items = enhanceItemsTree(widgets?.children || [], (widget) => ({ + const items = useMemo(() => + enhanceItemsTree(widgets?.children || [], enhanceWidget), + [widgets?.children, enhanceWidget]);
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
app/client/packages/design-system/ads/src/Templates/EntityExplorer/EntityListTree/EntityListTree.test.tsx(1 hunks)app/client/packages/design-system/ads/src/Templates/EntityExplorer/EntityListTree/EntityListTree.tsx(1 hunks)app/client/src/pages/Editor/IDE/EditorPane/UI/UIEntityListTree/UIEntityListTree.tsx(1 hunks)app/client/src/pages/Editor/IDE/EditorPane/UI/UIEntityListTree/WidgetTypeIcon.tsx(1 hunks)app/client/src/pages/Editor/IDE/EditorPane/UI/UIEntityListTree/hooks/useWidgetTreeExpandedState.ts(1 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
app/client/packages/design-system/ads/src/Templates/EntityExplorer/EntityListTree/EntityListTree.tsx
[error] 51-51: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
⏰ 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-lint / client-lint
- 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 (5)
app/client/src/pages/Editor/IDE/EditorPane/UI/UIEntityListTree/WidgetTypeIcon.tsx (1)
5-21: LGTM! Well-structured component with proper optimization.The implementation includes:
- Clear TypeScript interfaces
- Performance optimization with React.memo
- Proper fallback handling
app/client/src/pages/Editor/IDE/EditorPane/UI/UIEntityListTree/hooks/useWidgetTreeExpandedState.ts (1)
10-16:⚠️ Potential issueAdd setExpandedWidgets to useCallback dependencies.
The handleExpand callback should include setExpandedWidgets in its dependency array.
-}, []); +}, [setExpandedWidgets]);Likely invalid or redundant comment.
app/client/src/pages/Editor/IDE/EditorPane/UI/UIEntityListTree/UIEntityListTree.tsx (3)
1-18: LGTM! Well-organized importsThe imports are logically grouped and all dependencies are relevant to the component's functionality.
33-44: LGTM! Well-implemented name editing logicThe name editing handlers are properly memoized and follow React's best practices.
45-46: LGTM! Clean tree state managementThe tree state is managed efficiently using a custom hook.
| it("renders the EntityListTree component", () => { | ||
| render(<EntityListTree {...defaultProps} />); | ||
| expect(screen.getByRole("tree")).toBeInTheDocument(); | ||
| }); |
There was a problem hiding this comment.
💡 Codebase verification
Add keyboard navigation implementation and tests
The component implements ARIA attributes but lacks keyboard navigation handlers required for tree view accessibility. Add keyboard support for:
- Arrow keys for navigation
- Enter/Space for selection
- Home/End for first/last item
- Expand/Collapse with arrow keys
🔗 Analysis chain
Add test coverage for keyboard navigation and accessibility.
The component implements ARIA attributes but lacks tests for keyboard navigation and accessibility features.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for keyboard event handlers in the component
rg -A 2 'onKeyDown|onKeyPress|onKeyUp' app/client/packages/design-system/ads/src/Templates/EntityExplorer/EntityListTree/
Length of output: 123
Script:
#!/bin/bash
# Check component implementation and imports
ast-grep --pattern 'export const EntityListTree = $_' app/client/packages/design-system/ads/src/Templates/EntityExplorer/EntityListTree/EntityListTree.tsx
# Look for accessibility-related code
rg -i "aria-|role=" app/client/packages/design-system/ads/src/Templates/EntityExplorer/EntityListTree/EntityListTree.tsx
Length of output: 485
Script:
#!/bin/bash
# Check for imports and component inheritance
rg -A 3 "^import.*from" app/client/packages/design-system/ads/src/Templates/EntityExplorer/EntityListTree/EntityListTree.tsx
# Check if there's a base tree component being used
ast-grep --pattern '<$_Tree'
Length of output: 74527
| return ( | ||
| <Flex | ||
| flex="1" | ||
| flexDirection="column" | ||
| role={currentDepth == 0 ? "tree" : undefined} | ||
| > | ||
| {props.items.map((item) => ( | ||
| <Flex flex="1" flexDirection="column" key={item.id}> | ||
| <EntityItemWrapper | ||
| alignItems="center" | ||
| aria-expanded={item.isExpanded} | ||
| aria-level={currentDepth} | ||
| aria-selected={item.isSelected} | ||
| data-depth={currentDepth} | ||
| data-disabled={item.isDisabled || false} | ||
| data-selected={item.isSelected} | ||
| flexDirection="row" | ||
| role="treeitem" | ||
| > | ||
| {item.children && item.children.length ? ( | ||
| <CollapseWrapper | ||
| data-itemid={item.id} | ||
| data-testid="entity-item-expand-icon" | ||
| onClick={handleOnExpandClick} | ||
| > | ||
| <Icon | ||
| name={ | ||
| item.isExpanded ? "arrow-down-s-line" : "arrow-right-s-line" | ||
| } | ||
| size="md" | ||
| /> | ||
| </CollapseWrapper> | ||
| ) : ( | ||
| <CollapseSpacer /> | ||
| )} | ||
| <PaddingOverrider> | ||
| <EntityItem {...item} /> | ||
| </PaddingOverrider> | ||
| </EntityItemWrapper> | ||
| {item.children && item.isExpanded ? ( | ||
| <EntityListTree | ||
| depth={childrenDepth} | ||
| items={item.children} | ||
| onItemExpand={onItemExpand} | ||
| /> | ||
| ) : null} | ||
| </Flex> | ||
| ))} | ||
| </Flex> | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Implement keyboard navigation for accessibility.
Add keyboard navigation handlers for:
- Arrow keys for tree traversal
- Home/End for first/last item
- Enter/Space for selection
Would you like me to provide the implementation for keyboard navigation handlers?
🧰 Tools
🪛 Biome (1.9.4)
[error] 51-51: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
|
/build-deploy-preview |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/12707067301. |
|
Deploy-Preview-URL: https://ce-38493.dp.appsmith.com |
# Conflicts: # app/client/packages/design-system/ads/src/Templates/EntityExplorer/index.ts
...kages/design-system/ads/src/Templates/EntityExplorer/EntityListTree/EntityListTree.styles.ts
Show resolved
Hide resolved
|
/build-deploy-preview |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/12741829453. |
|
Deploy-Preview-URL: https://ce-38493.dp.appsmith.com |
...kages/design-system/ads/src/Templates/EntityExplorer/EntityListTree/EntityListTree.styles.ts
Outdated
Show resolved
Hide resolved
hetunandu
added
ok-to-test
Description
Introduces the List Tree component in ADS that is built on top of Entity Item component.
Uses feature flags to update the UI List Tree using this component
Fixes #37770
Automation
/ok-to-test tags="@tag.Widget"
🔍 Cypress test results
Tip
🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/12742248583
Commit: 31e9ae6
Cypress dashboard.
Tags:
@tag.WidgetSpec:
Mon, 13 Jan 2025 08:24:24 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
Based on the comprehensive changes, here are the updated release notes:
New Features
EntityListTreecomponent for hierarchical entity management.UIEntityListTreecomponent for rendering UI entities with enhanced navigation.WidgetContextMenufor contextual actions on widgets.OldWidgetEntityListcomponent for legacy widget rendering.Improvements
Bug Fixes
Design System Updates