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
WalkthroughThis 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
…ilities (#38541) Co-authored-by: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com> Co-authored-by: [email protected] <[email protected]>
![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 (7)
app/client/src/pages/Editor/IDE/EditorPane/UI/UIEntityListTree/UIEntityListTree.types.ts (1)
14-17: Add JSDoc comments for configuration flagsThe boolean flags in WidgetTreeConfig would benefit from documentation explaining their purpose and impact.
Add JSDoc comments like this:
export interface WidgetTreeConfig { + /** Determines if the user has permissions to manage pages */ canManagePages: boolean; + /** Controls whether this feature is enabled for the user */ isFeatureEnabled: boolean; }app/client/src/pages/Editor/IDE/EditorPane/UI/UIEntityListTree/hooks/useWidgetTreeState.ts (1)
5-21: Consider adding error boundaries and validationWhile the implementation is solid, consider adding:
- Type validation for the Redux selector result
- Error handling for invalid widget IDs
- Maximum limit for expanded widgets to prevent memory issues
Example implementation:
export const useWidgetTreeState = () => { const widgetsToExpand = useSelector(getEntityExplorerWidgetsToExpand); - const [expandedWidgets, setExpandedWidgets] = useState<string[]>(widgetsToExpand); + const [expandedWidgets, setExpandedWidgets] = useState<string[]>( + Array.isArray(widgetsToExpand) ? widgetsToExpand.slice(0, 100) : [] + ); const handleExpand = useCallback((id: string) => { + if (typeof id !== 'string' || id.trim() === '') return; setExpandedWidgets((prev) => prev.includes(id) ? prev.filter((widgetId) => widgetId !== id) - : [...prev, id], + : prev.length >= 100 ? prev : [...prev, id], ); }, []);app/client/src/pages/Editor/IDE/EditorPane/UI/UIEntityListTree/hooks/useWidgetTreeHandlers.ts (2)
3-3: Remove unused importThe
WidgetTypeimport is not used in this file.-import type { WidgetType } from "constants/WidgetConstants";
13-25: Consider debouncing double-click handlerThe current implementation might cause both click and double-click handlers to fire when double-clicking. Consider adding debounce to prevent this.
+import debounce from "lodash/debounce"; export const useWidgetTreeHandlers = (config: HandlerConfig) => { const { enterEditMode, switchToWidget } = config; const handleClick = useCallback( (_e: MouseEvent, widgetId: string) => { + if (_e.detail === 1) { // Only handle single clicks switchToWidget(widgetId); + } }, [switchToWidget], ); const handleDoubleClick = useCallback( - (widgetId: string) => { + debounce((widgetId: string) => { enterEditMode(widgetId); - }, + }, 250), [enterEditMode], );app/client/src/pages/Editor/IDE/EditorPane/UI/UIEntityListTree/utils/treeEnhancer.ts (1)
10-15: Add input validation for enhanceWidgetTreeThe function assumes valid input array. Consider adding validation for null/undefined items.
export const enhanceWidgetTree = ( items: CanvasStructure[], config: TreeEnhancementConfig, ): EntityListTreeItem[] => { + if (!items?.length) return []; return items.map((item) => enhanceWidgetTreeItem(item, config)); };app/client/src/pages/Editor/IDE/EditorPane/UI/UIEntityListTree/UIEntityListTree.tsx (2)
73-88: Optimize enhancedItems memoizationThe
enhancedItemsmemo depends on too many variables. Consider breaking it down into smaller memoized parts.+ const baseItems = useMemo( + () => treeItems.map((item) => ({ + ...item, + rightControlVisibility: "hover" as const, + })), + [treeItems] + ); const enhancedItems = useMemo(() => { - return treeItems.map((item) => ({ + return baseItems.map((item) => ({ - ...item, - rightControlVisibility: "hover" as const, onClick: (e: React.MouseEvent) => handleClick(e, item.id), onDoubleClick: () => handleDoubleClick(item.id), rightControl: ( <WidgetContextMenu canManagePages={canManagePages} widgetId={item.id} /> ), nameEditorConfig: handleNameEditor(item.id), })); }, [baseItems, handleClick, handleDoubleClick, canManagePages, handleNameEditor]);
90-97: Add error boundary for tree renderingThe tree rendering could fail if the data structure is malformed. Consider wrapping it in an error boundary.
+ import { ErrorBoundary } from "components/editorComponents/ErrorBoundary"; return ( + <ErrorBoundary> <EntityListTree className={className} items={enhancedItems} onItemExpand={handleExpand} /> + </ErrorBoundary> );
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
app/client/src/pages/Editor/IDE/EditorPane/UI/UIEntityListTree/UIEntityListTree.tsx(1 hunks)app/client/src/pages/Editor/IDE/EditorPane/UI/UIEntityListTree/UIEntityListTree.types.ts(1 hunks)app/client/src/pages/Editor/IDE/EditorPane/UI/UIEntityListTree/hooks/index.ts(1 hunks)app/client/src/pages/Editor/IDE/EditorPane/UI/UIEntityListTree/hooks/useWidgetNameEditor.ts(1 hunks)app/client/src/pages/Editor/IDE/EditorPane/UI/UIEntityListTree/hooks/useWidgetTreeHandlers.ts(1 hunks)app/client/src/pages/Editor/IDE/EditorPane/UI/UIEntityListTree/hooks/useWidgetTreeState.ts(1 hunks)app/client/src/pages/Editor/IDE/EditorPane/UI/UIEntityListTree/index.ts(1 hunks)app/client/src/pages/Editor/IDE/EditorPane/UI/UIEntityListTree/utils/treeEnhancer.ts(1 hunks)
⏰ 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-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 (3)
app/client/src/pages/Editor/IDE/EditorPane/UI/UIEntityListTree/hooks/index.ts (1)
1-5: Clean and well-organized hook exportsThe barrel export pattern provides a clean interface for consuming components. The separation of hooks into individual files promotes maintainability.
app/client/src/pages/Editor/IDE/EditorPane/UI/UIEntityListTree/hooks/useWidgetNameEditor.ts (1)
21-25: Consider adding payload for exitEditMode actionThe
exitEditModeaction doesn't include any payload about which widget's edit mode is being exited. This could lead to state inconsistencies if multiple widgets are in edit mode.Let's verify if multiple widgets can be in edit mode simultaneously:
app/client/src/pages/Editor/IDE/EditorPane/UI/UIEntityListTree/index.ts (1)
1-2: LGTM!Clean barrel file implementation with named exports.
app/client/src/pages/Editor/IDE/EditorPane/UI/UIEntityListTree/UIEntityListTree.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
app/client/src/pages/Editor/IDE/EditorPane/UI/UIEntityListTree/utils/enhanceTree.ts (1)
4-16: Consider adding input validation.The implementation is clean and functional. However, consider adding validation for the input parameters to handle edge cases.
export const enhanceItemsTree = ( items: CanvasStructure[], enhancer: (item: CanvasStructure) => EntityListTreeItem, ) => { + if (!Array.isArray(items)) { + return []; + } + if (typeof enhancer !== 'function') { + throw new Error('Enhancer must be a function'); + } return items.map((child): EntityListTreeItem => {app/client/src/selectors/explorerSelector.ts (1)
37-38: Add JSDoc documentation for consistency.Other selectors in this file have JSDoc documentation. Consider adding documentation for this new selector to maintain consistency.
+/** + * returns the currently editing entity name + * + * @param state + * @returns + */ export const getEditingEntityName = (state: AppState) => state.ui.explorer.entity.editingEntityName;app/client/src/pages/Editor/IDE/EditorPane/UI/UIEntityListTree/UIEntityListTree.tsx (3)
43-43: Consider providing widget name for validation initialization.The
useValidateEntityNamehook is initialized with an empty object. Consider providing initial values for better type safety and validation context.- const validateName = useValidateEntityName({}); + const validateName = useValidateEntityName({ entityType: "widget" });
47-70: Consider memoizing the tree enhancement callback.The enhancement callback function is recreated on every render. Consider memoizing it with useCallback to optimize performance.
+ const enhanceWidget = useCallback( + (widget) => ({ + id: widget.widgetId, + title: widget.widgetName, + // ... rest of the enhancement logic + }), + [canManagePages, selectedWidgets, expandedWidgets, switchToWidget, enterEditMode, + updatingEntity, editingEntity, handleNameSave, exitEditMode, validateName] + ); - const items = enhanceItemsTree(widgets?.children || [], (widget) => ({ + const items = enhanceItemsTree(widgets?.children || [], enhanceWidget);
72-72: Consider memoizing items prop.The
itemsarray is recreated on every render. Consider memoizing it with useMemo to prevent unnecessary re-renders of EntityListTree.+ const memoizedItems = useMemo(() => items, [items]); - return <EntityListTree items={items} onItemExpand={handleExpand} />; + return <EntityListTree items={memoizedItems} onItemExpand={handleExpand} />;
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
app/client/src/IDE/Components/EditableName/index.ts(1 hunks)app/client/src/IDE/index.ts(1 hunks)app/client/src/ce/selectors/entitiesSelector.ts(0 hunks)app/client/src/pages/Editor/IDE/EditorPane/UI/UIEntityListTree/UIEntityListTree.tsx(1 hunks)app/client/src/pages/Editor/IDE/EditorPane/UI/UIEntityListTree/hooks/useWidgetTreeExpandedState.ts(1 hunks)app/client/src/pages/Editor/IDE/EditorPane/UI/UIEntityListTree/index.ts(1 hunks)app/client/src/pages/Editor/IDE/EditorPane/UI/UIEntityListTree/utils/enhanceTree.ts(1 hunks)app/client/src/pages/Editor/IDE/EditorPane/hooks/useNameEditorState.ts(1 hunks)app/client/src/selectors/explorerSelector.ts(1 hunks)
💤 Files with no reviewable changes (1)
- app/client/src/ce/selectors/entitiesSelector.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- app/client/src/pages/Editor/IDE/EditorPane/UI/UIEntityListTree/index.ts
⏰ 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-unit-tests / client-unit-tests
- GitHub Check: client-check-cyclic-deps / check-cyclic-dependencies
- 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/src/IDE/Components/EditableName/index.ts (1)
4-4: LGTM! Clean export addition.The new export follows the established pattern and groups related functionality together.
app/client/src/pages/Editor/IDE/EditorPane/hooks/useNameEditorState.ts (1)
10-36: LGTM! Well-structured custom hook implementation.The hook follows React best practices with proper dependency management and clear separation of concerns.
app/client/src/IDE/index.ts (1)
51-51: LGTM! Export addition follows existing pattern.The new export maintains the established organization of the file.
...ient/src/pages/Editor/IDE/EditorPane/UI/UIEntityListTree/hooks/useWidgetTreeExpandedState.ts
Show resolved
Hide resolved
|
🔴🔴🔴 Cyclic Dependency Check: This PR has increased the number of cyclic dependencies by 3, when compared with the release branch. Refer this document to identify the cyclic dependencies introduced by this PR. |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (7)
app/client/packages/design-system/ads/src/Templates/EntityExplorer/EntityItem/EntityItem.tsx (1)
Line range hint
49-53: Consider making dimensions configurableWhile using CSS variables for background color is good, consider making the height and padding values configurable through props or theme variables for better flexibility.
app/client/packages/design-system/ads/src/Templates/EntityExplorer/EntityListTree/EntityListTree.tsx (1)
42-57: Simplify children check with optional chainingConsider using optional chaining for a more concise check.
- {item.children && item.children.length ? ( + {item.children?.length ? (🧰 Tools
🪛 Biome (1.9.4)
[error] 42-42: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
app/client/src/sagas/selectors.tsx (1)
Line range hint
133-133: Consider addressing the TODO comments with eslint-disable directives.There are two instances of
eslint-disablecomments marked as TODO. These represent technical debt that should be addressed by properly typing the variables.The affected areas are:
dataTreeingetDataTreeForActionCreatoraccingetPluginIdToImageLocationAlso applies to: 185-185
app/client/src/pages/Editor/IDE/EditorPane/UI/UIEntityListTree/hooks/useDeleteWidget.ts (3)
20-22: Enhance the comment to better explain the tab deletion logic.The current comment could be more descriptive about why tabs require special handling.
- // If the widget is a tab we are updating the `tabs` of the property of the widget - // This is similar to deleting a tab from the property pane + // Special handling for tab widgets: + // When deleting a tab, we need to update the parent TABS_WIDGET's `tabs` property + // instead of directly deleting the widget. This ensures proper cleanup and state management + // similar to when deleting a tab through the property pane.
30-38: Improve error handling for tab deletion.The current implementation could be more robust with proper error handling and logging.
- if (widget?.parentId && !!filteredTabs.length) { + if (!widget?.tabId) { + console.error('Failed to delete tab: Missing tabId'); + return; + } + + if (widget?.parentId && filteredTabs.length) { dispatch({ type: ReduxActionTypes.WIDGET_DELETE_TAB_CHILD, - payload: { ...tabsObj[widget?.tabId] }, + payload: { ...tabsObj[widget.tabId] }, }); }
40-47: Consider adding error handling for widget deletion.The default widget deletion case could benefit from some basic validation.
+ if (!widget) { + console.error('Failed to delete widget: Widget not found'); + return; + } + dispatch({ type: WidgetReduxActionTypes.WIDGET_DELETE, payload: { widgetId, - parentId: widget?.parentId, + parentId: widget.parentId, }, });app/client/src/pages/Editor/IDE/EditorPane/UI/List.tsx (1)
90-93: Consider consolidating overflow propertiesThe overflow properties could be consolidated into a single
overflowproperty for better maintainability.- overflowX="hidden" - overflowY="auto" + overflow="hidden auto"
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
app/client/packages/design-system/ads/src/Templates/EntityExplorer/EntityItem/EntityItem.tsx(3 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/src/pages/Editor/IDE/EditorPane/UI/List.tsx(3 hunks)app/client/src/pages/Editor/IDE/EditorPane/UI/UIEntityListTree/WidgetContextMenu.tsx(1 hunks)app/client/src/pages/Editor/IDE/EditorPane/UI/UIEntityListTree/hooks/useDeleteWidget.ts(1 hunks)app/client/src/sagas/selectors.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- app/client/src/pages/Editor/IDE/EditorPane/UI/UIEntityListTree/WidgetContextMenu.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] 42-42: 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-unit-tests / client-unit-tests
- GitHub Check: client-check-cyclic-deps / check-cyclic-dependencies
- GitHub Check: client-lint / client-lint
- GitHub Check: client-build / client-build
- GitHub Check: client-prettier / prettier-check
- GitHub Check: storybook-tests
- GitHub Check: chromatic-deployment
- GitHub Check: chromatic-deployment
🔇 Additional comments (9)
app/client/packages/design-system/ads/src/Templates/EntityExplorer/EntityItem/EntityItem.tsx (2)
35-42: Clean implementation of startIcon with loading stateGood use of useMemo for performance optimization and clean handling of loading state.
81-88: Well-implemented rightControl visibility logicClean implementation with proper memoization and clear conditional rendering.
app/client/packages/design-system/ads/src/Templates/EntityExplorer/EntityListTree/EntityListTree.tsx (2)
15-26: Well-implemented expand click handlerGood use of useCallback and proper event handling with clear documentation.
62-68: Clean implementation of recursive tree renderingGood handling of depth and expansion state. Consider adding a maximum depth limit to prevent potential stack overflow with deeply nested structures.
app/client/src/sagas/selectors.tsx (1)
66-68: LGTM! Type safety improvement.The explicit return type annotation enhances type safety and makes the API contract clearer.
However, let's verify that this change doesn't break any existing usage:
✅ Verification successful
Return type change is safe and properly handled
The explicit
undefinedtype is already accounted for in existing usage patterns through optional chaining and null-safe operations.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for all usages of getWidgetByID to ensure they handle the undefined case rg "getWidgetByID\(" -A 2Length of output: 4392
app/client/src/pages/Editor/IDE/EditorPane/UI/UIEntityListTree/hooks/useDeleteWidget.ts (2)
1-11: LGTM! Well-organized imports and constant definition.The imports are logically grouped and demonstrate good separation of concerns.
13-17: LGTM! Clean hook setup with proper Redux integration.The hook signature and state setup demonstrate good practices with proper typing and Redux hook usage.
app/client/src/pages/Editor/IDE/EditorPane/UI/List.tsx (2)
6-6: Clean import organization!The new imports are well-organized and follow the project's import structure.
Also applies to: 14-15
94-98: Clean feature toggle implementation!The conditional rendering between old and new implementations is well-structured and maintainable.
app/client/src/pages/Editor/IDE/EditorPane/UI/UIEntityListTree/hooks/useDeleteWidget.ts
Show resolved
Hide resolved
|
🔴🔴🔴 Cyclic Dependency Check: This PR has increased the number of cyclic dependencies by 3, when compared with the release branch. Refer this document to identify the cyclic dependencies introduced by this PR. |
|
🔴🔴🔴 Cyclic Dependency Check: This PR has increased the number of cyclic dependencies by 3, when compared with the release branch. Refer this document to identify the cyclic dependencies introduced by this PR. |
|
/build-deploy-preview |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/12704712525. |
|
Deploy-Preview-URL: https://ce-38493.dp.appsmith.com |
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 |
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.IDE"
🔍 Cypress test results
Tip
🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/12707788731
Commit: 3bc67d9
Cypress dashboard.
Tags:
@tag.IDESpec:
Fri, 10 Jan 2025 11:05:24 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
Release Notes
New Features
EntityListTreecomponent for hierarchical entity management.UIEntityListTreecomponent for rendering UI entities.WidgetContextMenucomponent for widget actions.OldWidgetEntityListcomponent for legacy widget rendering.Improvements
Performance
Bug Fixes