chore: Adding Group component in ADS Templates #38512
Conversation
WalkthroughThe pull request introduces a comprehensive redesign of the list and group rendering components across the Appsmith design system. The changes primarily focus on creating a more flexible and reusable Changes
Assessment against linked issues
Possibly related issues
Possibly related PRs
Suggested Labels
Suggested Reviewers
Poem
Finishing Touches
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
the
skip-changelog
ankitakinger
added
ok-to-test
github-actions
bot
added
IDE Pod
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (8)
app/client/packages/design-system/ads/src/Templates/EntityExplorer/Group/Group.types.ts (1)
3-7: LGTM! Consider adding JSDoc comments.The interface is well-structured. Consider adding JSDoc comments to document the purpose of each property, especially
groupTitlewhich is optional.+/** + * Props for the Group component + */ export interface GroupProps { + /** Optional title to display above the group */ groupTitle?: string; + /** CSS class name for styling the group container */ className: string; + /** Array of list items to display in the group */ items: ListItemProps[]; }app/client/packages/design-system/ads/src/Templates/EntityExplorer/Group/Group.styles.ts (1)
20-30: Add hover transition for smoother UX.Consider adding a transition for the hover effect to make it smoother.
export const LoadMore = styled(Text)` color: var(--ads-v2-color-fg-subtle); padding: var(--ads-v2-spaces-2); padding-left: var(--ads-v2-spaces-3); cursor: pointer; border-radius: var(--ads-v2-border-radius); + transition: background-color 0.2s ease; &:hover { background-color: var(--ads-v2-colors-content-surface-hover-bg); } `;app/client/packages/design-system/ads/src/Templates/EntityExplorer/index.ts (1)
10-10: Consider explicit exports instead of wildcard.Using
export *might expose unintended internal implementations. Consider explicitly listing the exports.-export * from "./Group"; +export { Group } from "./Group"; +export type { GroupProps } from "./Group";app/client/src/pages/Editor/IDE/EditorPane/components/GroupedList.tsx (1)
5-8: Add prop validation for required props.Consider adding prop-types or runtime validation for the required
groupsprop to provide better error messages.+import { isNonEmptyArray } from "utils/helpers"; interface Props { groups: GroupProps[]; flexProps?: FlexProps; } +const validateProps = (props: Props) => { + if (!isNonEmptyArray(props.groups)) { + console.warn("GroupedList: groups prop must be a non-empty array"); + } +};app/client/packages/design-system/ads/src/Templates/EntityExplorer/Group/Group.tsx (2)
23-39: Consider using constants for string literals and simplifying logic.The hardcoded strings "Datasources" and "New datasource" should be moved to constants to improve maintainability.
+ const DATASOURCES_GROUP = "Datasources"; + const NEW_DATASOURCE_ITEM = "New datasource"; const { addConfig, items } = useMemo(() => { const items = groupItems.slice(0, visibleItemsCount); let addConfig = undefined; if ( - groupTitle === "Datasources" && - groupItems[groupItems.length - 1].title === "New datasource" + groupTitle === DATASOURCES_GROUP && + groupItems[groupItems.length - 1].title === NEW_DATASOURCE_ITEM ) {
45-76: Pass className to StyledGroup for consistent styling.The className prop from the group is not being passed to StyledGroup, which might affect custom styling.
<StyledGroup className="groups-list-group" + className={`groups-list-group ${className || ''}`} flexDirection="column" key={groupTitle} pb="spaces-3" >app/client/packages/design-system/ads/src/Templates/EntityExplorer/Group/Group.stories.tsx (2)
22-77: Consider adding ARIA labels for better accessibility.The list items could benefit from ARIA labels to improve screen reader support.
{ startIcon: <Icon name="database-2-line" />, className: "t--datasoucre-create-option-users", title: "Users", + "aria-label": "Open Users datasource", onClick: () => console.log("Users clicked"), },
79-110: Consider extracting common test data.The item structures are duplicated across stories. Consider creating shared test data objects.
const createDatasourceItem = (title: string) => ({ startIcon: <Icon name="database-2-line" />, className: `t--datasoucre-create-option-${title.toLowerCase()}`, title, onClick: () => console.log(`${title} clicked`), });
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
app/client/packages/design-system/ads/src/List/List.styles.tsx(0 hunks)app/client/packages/design-system/ads/src/Templates/EntityExplorer/Group/Group.constants.ts(1 hunks)app/client/packages/design-system/ads/src/Templates/EntityExplorer/Group/Group.stories.tsx(1 hunks)app/client/packages/design-system/ads/src/Templates/EntityExplorer/Group/Group.styles.ts(1 hunks)app/client/packages/design-system/ads/src/Templates/EntityExplorer/Group/Group.tsx(1 hunks)app/client/packages/design-system/ads/src/Templates/EntityExplorer/Group/Group.types.ts(1 hunks)app/client/packages/design-system/ads/src/Templates/EntityExplorer/Group/index.ts(1 hunks)app/client/packages/design-system/ads/src/Templates/EntityExplorer/index.ts(1 hunks)app/client/src/pages/Editor/IDE/EditorPane/components/GroupedList.tsx(1 hunks)
💤 Files with no reviewable changes (1)
- app/client/packages/design-system/ads/src/List/List.styles.tsx
✅ Files skipped from review due to trivial changes (2)
- app/client/packages/design-system/ads/src/Templates/EntityExplorer/Group/Group.constants.ts
- app/client/packages/design-system/ads/src/Templates/EntityExplorer/Group/index.ts
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: client-unit-tests / client-unit-tests
- GitHub Check: client-lint / client-lint
- GitHub Check: client-build / client-build
- GitHub Check: client-check-cyclic-deps / check-cyclic-dependencies
- GitHub Check: client-prettier / prettier-check
- GitHub Check: storybook-tests
- GitHub Check: chromatic-deployment
- GitHub Check: chromatic-deployment
🔇 Additional comments (6)
app/client/packages/design-system/ads/src/Templates/EntityExplorer/Group/Group.styles.ts (1)
16-18: Verify spacing impact of padding removal.Removing padding might affect the visual hierarchy. Ensure this doesn't break the layout in different viewport sizes.
✅ Verification successful
Padding removal is correctly implemented
The padding removal is intentional and safe as the Group component handles its own spacing hierarchy, while ListItems maintain their internal padding structure.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for other components that might rely on List padding rg -l "StyledList" app/client/packages/design-system/Length of output: 344
Script:
#!/bin/bash # Check List implementation and styles echo "=== List.styles.tsx ===" cat app/client/packages/design-system/ads/src/List/List.styles.tsx echo -e "\n=== Group.tsx usage ===" cat app/client/packages/design-system/ads/src/Templates/EntityExplorer/Group/Group.tsx echo -e "\n=== List.tsx ===" cat app/client/packages/design-system/ads/src/List/List.tsxLength of output: 10560
app/client/packages/design-system/ads/src/Templates/EntityExplorer/Group/Group.tsx (3)
1-10: LGTM! Clean imports and type definitions.
12-21: LGTM! Proper state management and memoization.
41-43: LGTM! Clear and focused event handler.app/client/packages/design-system/ads/src/Templates/EntityExplorer/Group/Group.stories.tsx (2)
1-21: LGTM! Well-structured story setup.
112-131: LGTM! Good coverage of component variants.
hetunandu
added
ok-to-test
| const Group: React.FC<Props> = ({ group }) => { | ||
| const { className, groupTitle, items: groupItems } = group; | ||
| const [visibleItemsCount, setVisibleItemsCount] = useState<number>( | ||
| DEFAULT_GROUP_LIST_SIZE, |
app/client/packages/design-system/ads/src/Templates/EntityExplorer/Group/Group.tsx
Outdated
Show resolved
Hide resolved
app/client/packages/design-system/ads/src/Templates/EntityExplorer/Group/Group.tsx
Outdated
Show resolved
Hide resolved
app/client/packages/design-system/ads/src/Templates/EntityExplorer/Group/Group.tsx
Outdated
Show resolved
Hide resolved
app/client/packages/design-system/ads/src/Templates/EntityExplorer/Group/Group.stories.tsx
Outdated
Show resolved
Hide resolved
app/client/packages/design-system/ads/src/Templates/EntityExplorer/Group/Group.stories.tsx
Outdated
Show resolved
Hide resolved
app/client/packages/design-system/ads/src/Templates/EntityExplorer/Group/Group.tsx
Outdated
Show resolved
Hide resolved
app/client/packages/design-system/ads/src/Templates/EntityExplorer/Group/Group.types.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (12)
app/client/src/pages/Editor/IDE/LeftPane/DataSidePane.tsx (1)
155-155: Consider memoizing the click handlerThe click handler is being recreated for each item in the list. Consider memoizing it with useCallback to optimize performance.
-onClick={() => goToDatasource(data.id)} +onClick={useCallback(() => goToDatasource(data.id), [data.id, goToDatasource])}app/client/src/ce/pages/Editor/IDE/EditorPane/Query/hooks.tsx (2)
75-75: Add explicit return type for better type safety.The removal of the explicit return type reduces type safety and documentation value.
-export const useGroupedAddQueryOperations = () => { +export const useGroupedAddQueryOperations = (): Array<{ + groupTitle: string; + className: string; + items: ListItemProps[] | EntityItemProps[]; + addConfig?: { + icon: ReactNode; + title: string; + onClick: () => void; + }; +}> => {
123-149: Simplify group transformation logic.The current implementation has nested conditionals and mutates arrays in-place. Consider using a more functional approach.
- groups.map((group) => { - const items = getListItems(group.operations); - const lastItem = items[items.length - 1]; - - if (group.title === "Datasources" && lastItem.title === "New datasource") { - items.splice(items.length - 1); - - const addConfig = { - icon: lastItem.startIcon, - onClick: lastItem.onClick, - title: lastItem.title, - }; - - groupedItems.push({ - groupTitle: group.title, - className: group.className, - items, - addConfig, - }); - } else { - groupedItems.push({ - groupTitle: group.title || "", - className: group.className, - items, - }); - } - }); + return groups.map((group) => { + const items = getListItems(group.operations); + const lastItem = items[items.length - 1]; + const isDatasourcesGroup = group.title === "Datasources" && + lastItem?.title === "New datasource"; + + return { + groupTitle: group.title || "", + className: group.className, + items: isDatasourcesGroup ? items.slice(0, -1) : items, + ...(isDatasourcesGroup && { + addConfig: { + icon: lastItem.startIcon, + onClick: lastItem.onClick, + title: lastItem.title, + }, + }), + }; + });app/client/packages/design-system/ads/src/Templates/EntityExplorer/EntityGroupsList/EntityGroupsList.styles.ts (1)
6-9: Consider using a semantic class name.The class name 'groups-list-group' could be more semantic, like 'entity-groups-list-group' to match the component name.
app/client/packages/design-system/ads/src/Templates/EntityExplorer/EntityGroupsList/EntityGroupsList.types.ts (1)
4-14: Consider adding constraints and documentation.The generic type parameter
Tcould benefit from constraints if there are any common properties required across all item types. Also, consider adding JSDoc comments to document the purpose of each interface and its properties.+/** + * Props for an individual entity group + * @template T The type of items in the group + */ export interface EntityGroupProps<T> { + /** Title displayed for the group */ groupTitle: string; + /** CSS class name for styling */ className: string; + /** Array of items in the group */ items: T[]; + /** Optional configuration for adding new items */ addConfig?: { icon: ReactNode; title: string; onClick: () => void; }; + /** Optional custom renderer for list items */ renderList?: (item: T) => React.ReactNode; }app/client/src/pages/Editor/IDE/EditorPane/Query/Add.tsx (1)
Line range hint
27-30: Consider adding loading state handling.The component might benefit from handling loading states during data fetching.
+ const [isLoading, setIsLoading] = useState(false); const itemGroups = useGroupedAddQueryOperations(); return ( ... - {filteredItemGroups.length > 0 ? ( + {isLoading ? ( + <LoadingState /> + ) : filteredItemGroups.length > 0 ? ( <EntityGroupsList groups={filteredItemGroups} /> ) : null}Also applies to: 61-70
app/client/packages/design-system/ads/src/Templates/EntityExplorer/EntityGroupsList/EntityGroupsList.tsx (2)
28-38: Consider memoizing the EntityGroup componentThe EntityGroup component is recreated on every render of EntityGroupsList. Since it's a complex component with state management, consider wrapping it with React.memo to prevent unnecessary re-renders.
-const EntityGroup = <T,>({ group }: { group: EntityGroupProps<T> }) => { +const EntityGroup = React.memo(<T,>({ group }: { group: EntityGroupProps<T> }) => { // ... component implementation -}; +});
65-67: Consider adding aria-label to LoadMore buttonThe LoadMore button should have an aria-label for better accessibility.
-<LoadMore onClick={lazyLoading?.handleLoadMore} title="Load more..." /> +<LoadMore + onClick={lazyLoading?.handleLoadMore} + title="Load more..." + aria-label={`Load more items in ${group.groupTitle}`} +/>app/client/packages/design-system/ads/src/Templates/IDEHeader/IDEHeader.stories.tsx (1)
61-70: Consider adding more diverse test casesThe items array only includes basic cases. Consider adding test cases for:
- Items with icons
- Items with descriptions
- Items with different states (disabled, selected)
app/client/src/pages/Editor/IDE/EditorPane/JS/Add.tsx (1)
61-64: Consider memoizing itemGroups transformationThe itemGroups transformation runs on every render. Consider using useMemo to optimize performance.
-const itemGroups = groupedJsOperations.map( +const itemGroups = React.useMemo(() => groupedJsOperations.map( ({ className, operations, title }) => ({ groupTitle: title || "", className: className, items: operations.map(getListItems), }), - ); + ), [groupedJsOperations]);app/client/packages/design-system/ads/src/List/List.tsx (1)
30-40: LGTM! Consider adding prop type validation.The refactoring to support both grouped and ungrouped lists looks good. The conditional rendering based on
groupTitleis clean and maintainable.Consider adding PropTypes validation or TypeScript type guards for the
groupTitleprop to ensure type safety.app/client/packages/design-system/ads/src/List/List.stories.tsx (1)
29-58: Consider using a helper function for mock data generation.The mock data structure is good, but for better maintainability in tests and stories, consider extracting this to a helper function.
const createMockItems = (count: number) => Array.from({ length: count }, (_, i) => ({ startIcon: <Icon name="file-list-2-line" size="md" />, title: `Action item ${i + 1}`, })); const items = createMockItems(7);
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (21)
app/client/packages/design-system/ads/src/List/List.stories.tsx(1 hunks)app/client/packages/design-system/ads/src/List/List.styles.tsx(2 hunks)app/client/packages/design-system/ads/src/List/List.tsx(2 hunks)app/client/packages/design-system/ads/src/List/List.types.tsx(1 hunks)app/client/packages/design-system/ads/src/Templates/EntityExplorer/EntityGroupsList/EntityGroupsList.styles.ts(1 hunks)app/client/packages/design-system/ads/src/Templates/EntityExplorer/EntityGroupsList/EntityGroupsList.tsx(1 hunks)app/client/packages/design-system/ads/src/Templates/EntityExplorer/EntityGroupsList/EntityGroupsList.types.ts(1 hunks)app/client/packages/design-system/ads/src/Templates/EntityExplorer/EntityGroupsList/index.ts(1 hunks)app/client/packages/design-system/ads/src/Templates/EntityExplorer/EntityItem/index.ts(1 hunks)app/client/packages/design-system/ads/src/Templates/EntityExplorer/index.ts(1 hunks)app/client/packages/design-system/ads/src/Templates/IDEHeader/IDEHeader.stories.tsx(3 hunks)app/client/src/ce/pages/Editor/IDE/EditorPane/Query/hooks.tsx(5 hunks)app/client/src/components/editorComponents/Debugger/StateInspector/StateInspector.tsx(2 hunks)app/client/src/pages/Editor/CustomWidgetBuilder/Preview/Debugger/helpDropdown.tsx(3 hunks)app/client/src/pages/Editor/IDE/EditorPane/JS/Add.tsx(3 hunks)app/client/src/pages/Editor/IDE/EditorPane/Query/Add.tsx(3 hunks)app/client/src/pages/Editor/IDE/EditorPane/components/Group.tsx(0 hunks)app/client/src/pages/Editor/IDE/EditorPane/components/GroupedList.tsx(0 hunks)app/client/src/pages/Editor/IDE/EditorPane/components/constants.ts(0 hunks)app/client/src/pages/Editor/IDE/LeftPane/DataSidePane.tsx(2 hunks)app/client/src/pages/Editor/IDE/RightPane/components/CreateNewQueryModal.tsx(2 hunks)
💤 Files with no reviewable changes (3)
- app/client/src/pages/Editor/IDE/EditorPane/components/constants.ts
- app/client/src/pages/Editor/IDE/EditorPane/components/Group.tsx
- app/client/src/pages/Editor/IDE/EditorPane/components/GroupedList.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- app/client/packages/design-system/ads/src/Templates/EntityExplorer/index.ts
🔇 Additional comments (16)
app/client/src/pages/Editor/IDE/LeftPane/DataSidePane.tsx (2)
3-3: LGTM!The import statement is correctly updated to include the ListItem component.
147-166: LGTM!The list rendering implementation is clean and follows React best practices. The ListItem components are properly configured with all necessary props.
app/client/src/ce/pages/Editor/IDE/EditorPane/Query/hooks.tsx (1)
1-1: LGTM! Clean import additions.The new type imports are well-organized and properly scoped.
Also applies to: 31-31, 38-38
app/client/packages/design-system/ads/src/Templates/EntityExplorer/EntityItem/index.ts (1)
Line range hint
1-3: LGTM! Clean barrel exports.The addition of EntityItemProps type export enhances the module's public API.
app/client/packages/design-system/ads/src/Templates/EntityExplorer/EntityGroupsList/index.ts (1)
Line range hint
1-6: LGTM! Consistent export pattern.Clean barrel exports maintaining consistency with other modules in the design system.
app/client/packages/design-system/ads/src/Templates/EntityExplorer/EntityGroupsList/EntityGroupsList.styles.ts (1)
11-13: LGTM! Proper use of design system tokens.Good use of design system color tokens and component extension.
app/client/packages/design-system/ads/src/Templates/EntityExplorer/EntityGroupsList/EntityGroupsList.types.ts (1)
16-19: LGTM! Clean props structure.Well-structured props with good separation of concerns between group and list level properties.
app/client/src/pages/Editor/IDE/RightPane/components/CreateNewQueryModal.tsx (2)
2-8: LGTM! Clean import restructuring.The imports have been properly organized to include the new EntityGroupsList component.
18-18: Verify the data structure compatibility.The transition from
groupedActionOperationstoitemGroupslooks good, but let's ensure the data structure matches the EntityGroupsList requirements.Also applies to: 39-39
✅ Verification successful
Data structure compatibility verified ✓
The
useGroupedAddQueryOperationshook's return type perfectly matches theEntityGroupsListcomponent's props structure.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check the structure expected by EntityGroupsList ast-grep --pattern 'interface EntityGroupsListProps { $$$ }'Length of output: 64
Script:
#!/bin/bash # Find EntityGroupsList component file fd EntityGroupsList # Find the hook implementation ast-grep --pattern 'const useGroupedAddQueryOperations = () => { $$$ }' # Find any type definitions related to the hook ast-grep --pattern 'type $_ = { groups: $_[] }' # Find any interface definitions that might be related to EntityGroupsList ast-grep --pattern 'interface $_ { groups: $_[] }'Length of output: 7911
Script:
#!/bin/bash # View EntityGroupsList types cat "app/client/packages/design-system/ads/src/Templates/EntityExplorer/EntityGroupsList/EntityGroupsList.types.ts"Length of output: 538
app/client/packages/design-system/ads/src/List/List.types.tsx (1)
44-45: LGTM! Enhanced type flexibility.The updated ListProps interface with ReactNode children and optional groupTitle provides better composition support and type safety.
app/client/src/pages/Editor/CustomWidgetBuilder/Preview/Debugger/helpDropdown.tsx (1)
67-71: LGTM! Clean list rendering implementation.The map implementation with proper key usage is correct and maintainable.
app/client/src/pages/Editor/IDE/EditorPane/Query/Add.tsx (1)
23-23: LGTM! Clean implementation of EntityGroupsList.The integration with search functionality and proper null checks makes this implementation robust.
Also applies to: 61-61
app/client/src/components/editorComponents/Debugger/StateInspector/StateInspector.tsx (1)
73-77:⚠️ Potential issueAdd error boundary for ReactJson rendering
The component should handle potential JSON parsing errors gracefully.
+class JsonErrorBoundary extends React.Component { + componentDidCatch(error) { + // Handle error logging + } + render() { + try { + return this.props.children; + } catch (e) { + return <Text>Error rendering JSON data</Text>; + } + } +} // Usage in render: -<ReactJson src={selectedItemCode} {...reactJsonProps} /> +<JsonErrorBoundary> + <ReactJson src={selectedItemCode} {...reactJsonProps} /> +</JsonErrorBoundary>Likely invalid or redundant comment.
app/client/src/pages/Editor/IDE/EditorPane/JS/Add.tsx (1)
101-102: Add loading state handlingThe component should handle loading states when fetching operations.
app/client/packages/design-system/ads/src/List/List.styles.tsx (2)
165-173: LGTM! Clean implementation of hover effects.The StyledGroup component implements a clean hover effect for descriptions, improving the UI's visual hierarchy.
175-182: LGTM! Good use of design system variables.The styling is consistent with the design system, and the components are well-focused on their specific responsibilities.
app/client/src/pages/Editor/CustomWidgetBuilder/Preview/Debugger/helpDropdown.tsx
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/List/List.stories.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
app/client/packages/design-system/ads/src/Templates/EntityExplorer/EntityGroupsList/EntityGroupsList.stories.tsx (2)
1-15: Consider using a more specific ESLint disable ruleInstead of disabling all console-related rules, target the specific rule:
-/* eslint-disable no-console */ +/* eslint-disable no-console-log */
34-74: Consider extracting icon configurationThe database icon is repeated across all items. Consider extracting it to a constant:
const DATABASE_ICON = <Icon name="database-2-line" />; // Then use it in items: { startIcon: DATABASE_ICON, // ... rest of the item config }app/client/src/ce/pages/Editor/IDE/EditorPane/Query/hooks.tsx (2)
134-134: Consider using a constant for "Datasources" string.Using string literals for comparisons can be error-prone and makes maintenance harder.
+ const DATASOURCES_GROUP_TITLE = "Datasources"; + const NEW_DATASOURCE_TITLE = "New datasource"; - if (group.title === "Datasources" && lastItem.title === "New datasource") { + if (group.title === DATASOURCES_GROUP_TITLE && lastItem.title === NEW_DATASOURCE_TITLE) {
135-135: Use array filter instead of splice.Using splice mutates the original array. Consider using filter for a more functional approach.
- items.splice(items.length - 1); + const filteredItems = items.slice(0, -1);
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
app/client/packages/design-system/ads/src/Templates/EntityExplorer/EntityGroupsList/EntityGroupsList.stories.tsx(1 hunks)app/client/packages/design-system/ads/src/Templates/EntityExplorer/EntityGroupsList/EntityGroupsList.tsx(1 hunks)app/client/packages/design-system/ads/src/Templates/EntityExplorer/EntityGroupsList/EntityGroupsList.types.ts(1 hunks)app/client/src/ce/pages/Editor/IDE/EditorPane/Query/hooks.tsx(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- app/client/packages/design-system/ads/src/Templates/EntityExplorer/EntityGroupsList/EntityGroupsList.types.ts
- app/client/packages/design-system/ads/src/Templates/EntityExplorer/EntityGroupsList/EntityGroupsList.tsx
⏰ Context from checks skipped due to timeout of 90000ms (10)
- GitHub Check: perform-test / client-build / client-build
- GitHub Check: perform-test / server-build / server-unit-tests
- GitHub Check: perform-test / rts-build / build
- GitHub Check: client-unit-tests / client-unit-tests
- GitHub Check: client-lint / client-lint
- GitHub Check: client-build / client-build
- GitHub Check: client-prettier / prettier-check
- GitHub Check: chromatic-deployment
- GitHub Check: chromatic-deployment
- GitHub Check: storybook-tests
🔇 Additional comments (4)
app/client/packages/design-system/ads/src/Templates/EntityExplorer/EntityGroupsList/EntityGroupsList.stories.tsx (2)
167-169: Duplicate group title detectedThere are two groups with the title "Apis". Consider using unique titles or merging these groups.
1-185: Overall structure looks good!The stories provide comprehensive coverage of the EntityGroupsList component features with proper TypeScript integration and mock data.
app/client/src/ce/pages/Editor/IDE/EditorPane/Query/hooks.tsx (2)
1-8: LGTM! Type safety improvements.The addition of explicit React types and EntityItemProps enhances type safety.
Also applies to: 45-45
103-112: LGTM! Well-structured type definition.The new type definition clearly describes the shape of grouped items with proper typing for all properties.
...design-system/ads/src/Templates/EntityExplorer/EntityGroupsList/EntityGroupsList.stories.tsx
Outdated
Show resolved
Hide resolved
...design-system/ads/src/Templates/EntityExplorer/EntityGroupsList/EntityGroupsList.stories.tsx
Outdated
Show resolved
Hide resolved
ankitakinger
added
ok-to-test
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
app/client/packages/design-system/ads/src/List/List.tsx (1)
30-30: Consider adding runtime type validation for groupTitle.Since groupTitle controls the component's rendering behavior, consider adding PropTypes or runtime type checking to ensure it's a non-empty string when provided.
+import PropTypes from 'prop-types'; function List({ children, className, groupTitle, ...rest }: ListProps) { + if (groupTitle !== undefined && typeof groupTitle !== 'string') { + console.warn('List: groupTitle prop must be a string'); + }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
app/client/packages/design-system/ads/src/List/List.tsx(2 hunks)app/client/packages/design-system/ads/src/Templates/EntityExplorer/EntityGroupsList/EntityGroupsList.stories.tsx(1 hunks)app/client/packages/design-system/ads/src/Templates/EntityExplorer/EntityGroupsList/EntityGroupsList.styles.ts(1 hunks)app/client/packages/design-system/ads/src/Templates/EntityExplorer/EntityGroupsList/EntityGroupsList.tsx(1 hunks)app/client/packages/design-system/ads/src/Templates/EntityExplorer/EntityGroupsList/EntityGroupsList.types.ts(1 hunks)app/client/src/pages/Editor/IDE/EditorPane/JS/Add.tsx(3 hunks)app/client/src/pages/Editor/IDE/EditorPane/Query/Add.tsx(3 hunks)app/client/src/pages/Editor/IDE/RightPane/components/CreateNewQueryModal.tsx(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (7)
- app/client/packages/design-system/ads/src/Templates/EntityExplorer/EntityGroupsList/EntityGroupsList.styles.ts
- app/client/src/pages/Editor/IDE/EditorPane/JS/Add.tsx
- app/client/packages/design-system/ads/src/Templates/EntityExplorer/EntityGroupsList/EntityGroupsList.tsx
- app/client/packages/design-system/ads/src/Templates/EntityExplorer/EntityGroupsList/EntityGroupsList.types.ts
- app/client/src/pages/Editor/IDE/RightPane/components/CreateNewQueryModal.tsx
- app/client/src/pages/Editor/IDE/EditorPane/Query/Add.tsx
- app/client/packages/design-system/ads/src/Templates/EntityExplorer/EntityGroupsList/EntityGroupsList.stories.tsx
⏰ 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: client-unit-tests / client-unit-tests
- GitHub Check: client-lint / client-lint
- GitHub Check: client-build / client-build
- GitHub Check: client-check-cyclic-deps / check-cyclic-dependencies
- GitHub Check: client-prettier / prettier-check
- GitHub Check: chromatic-deployment
- GitHub Check: chromatic-deployment
- GitHub Check: storybook-tests
🔇 Additional comments (2)
app/client/packages/design-system/ads/src/List/List.tsx (2)
7-11: LGTM! Clean import organization.The new group-related component imports are well-organized and follow the existing pattern.
30-40: LGTM! Clean implementation of group functionality.The conditional rendering and component structure are well-implemented, following React best practices by using children prop pattern.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
app/client/src/ce/pages/Editor/IDE/EditorPane/Query/hooks.tsx (1)
117-123:⚠️ Potential issueAdd null check for empty items array.
The array access at
items[items.length - 1]could throw if the items array is empty.- const lastItem = items[items.length - 1]; + const lastItem = items.length > 0 ? items[items.length - 1] : null; - if (group.title === "Datasources" && lastItem.title === "New datasource") { + if (group.title === "Datasources" && lastItem?.title === "New datasource") {
🧹 Nitpick comments (3)
app/client/src/pages/Editor/IDE/EditorPane/Query/Add.tsx (1)
28-31: Consider extracting type parameters to a type alias.The generic type parameters for
filterEntityGroupsBySearchTermcould be moved to a type alias to improve readability and reusability.+type GroupFilterTypes = { + Group: { groupTitle: string; className: string }; + Item: ListItemProps; +}; + -const filteredItemGroups = filterEntityGroupsBySearchTerm< - { groupTitle: string; className: string }, - ListItemProps ->(searchTerm, itemGroups); +const filteredItemGroups = filterEntityGroupsBySearchTerm< + GroupFilterTypes['Group'], + GroupFilterTypes['Item'] +>(searchTerm, itemGroups);app/client/src/ce/pages/Editor/IDE/EditorPane/Query/hooks.tsx (2)
124-135: Consider extracting addConfig creation to a separate function.The addConfig object creation logic could be moved to a separate function to improve readability and maintainability.
+const createAddConfig = (lastItem: ListItemProps) => ({ + icon: lastItem.startIcon, + onClick: lastItem.onClick, + title: lastItem.title, +}); - const addConfig = { - icon: lastItem.startIcon, - onClick: lastItem.onClick, - title: lastItem.title, - }; + const addConfig = createAddConfig(lastItem);
99-99: Consider using a more specific type for groupedItems.The type annotation could be more specific to better document the expected structure.
- const groupedItems: EntityGroupProps<ListItemProps | EntityItemProps>[] = []; + type QueryGroupItem = EntityGroupProps<ListItemProps | EntityItemProps>; + const groupedItems: QueryGroupItem[] = [];
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
app/client/src/ce/pages/Editor/IDE/EditorPane/Query/hooks.tsx(4 hunks)app/client/src/pages/Editor/IDE/EditorPane/Query/Add.tsx(3 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-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: chromatic-deployment
- GitHub Check: chromatic-deployment
- GitHub Check: storybook-tests
🔇 Additional comments (1)
app/client/src/pages/Editor/IDE/EditorPane/Query/Add.tsx (1)
62-62: Verify the showDivider prop behavior.The addition of
showDividerprop might affect the visual appearance of the list. Please ensure this matches the design requirements.✅ Verification successful
The showDivider prop implementation is verified and correct.
The prop is properly implemented to show dividers between groups using the design system's standard styling. Its usage in Add.tsx follows the established pattern seen across the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for other usages of EntityGroupsList to verify consistent prop usage ast-grep --pattern 'EntityGroupsList $props={$$$}'Length of output: 52
Script:
#!/bin/bash # Search for EntityGroupsList component definition and usage rg "EntityGroupsList" -A 5 -B 5 # Also try with a different ast-grep pattern for JSX ast-grep --pattern '<EntityGroupsList $$$' # Search specifically for showDivider usage rg "showDivider"Length of output: 90676
Script:
#!/bin/bash # Search for the EntityGroupsList implementation file rg -l "EntityGroupsList" app/client/packages/design-system/ads/src/Templates/EntityExplorer/EntityGroupsList/ # Look for showDivider usage in implementation rg -A 5 "showDivider" app/client/packages/design-system/ads/src/Templates/EntityExplorer/EntityGroupsList/Length of output: 3448
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
app/client/src/ce/pages/Editor/IDE/EditorPane/Query/hooks.tsx (1)
117-143:⚠️ Potential issueAdd null check for empty items array.
The array access at
items[items.length - 1]could throw if the items array is empty.- const lastItem = items[items.length - 1]; + const lastItem = items.length > 0 ? items[items.length - 1] : null; - if (group.title === "Datasources" && lastItem?.title === "New datasource") { + if (group.title === "Datasources" && lastItem?.title === "New datasource") {
🧹 Nitpick comments (6)
app/client/src/ce/pages/Editor/IDE/EditorPane/Query/hooks.tsx (2)
78-78: Add explicit return type for better type safety.The hook's return type should be explicitly defined for better maintainability and type safety.
-export const useGroupedAddQueryOperations = () => { +export const useGroupedAddQueryOperations = (): EntityGroupProps<ListItemProps | EntityItemProps>[] => {
124-128: Consider extracting addConfig creation to a separate function.The addConfig object creation logic could be moved to a separate function for better readability and reusability.
+const createAddConfig = (lastItem: ListItemProps) => ({ + icon: lastItem.startIcon, + onClick: lastItem.onClick, + title: lastItem.title, +}); - const addConfig = { - icon: lastItem.startIcon, - onClick: lastItem.onClick, - title: lastItem.title, - }; + const addConfig = createAddConfig(lastItem);app/client/packages/design-system/ads/src/Templates/EntityExplorer/EntityGroupsList/EntityGroupsList.stories.tsx (4)
19-23: Add JSDoc documentation for the template function.Consider adding documentation to improve component discoverability and usage understanding.
+/** + * Template for rendering a single EntityGroup + * @template T - Type parameter for the group items + * @param props - Props for the EntityGroup component + */ const EntityGroupTemplate = <T,>(props: EntityGroupProps<T>) => {
37-73: Replace console.log with Storybook actions.Use Storybook's action handlers instead of console.log for better debugging and documentation.
+import { action } from '@storybook/addon-actions'; - onClick: () => console.log("Users clicked"), + onClick: action("Users clicked"),
83-87: Add JSDoc documentation for the list template.Consider adding documentation to improve component discoverability.
+/** + * Template for rendering multiple EntityGroups + * @template T - Type parameter for the group items + * @param props - Props for the EntityGroupsList component + */ const EntityGroupsListTemplate = <T,>(props: EntityGroupsListProps<T>) => {
93-185: Consider extracting mock data.The mock data for groups is quite large. Consider extracting it to a separate mock data file for better maintainability.
// mockData.ts export const mockGroups = { datasources: { groupTitle: "Datasources", // ... rest of the data }, apis: { groupTitle: "Apis", // ... rest of the data } }; // EntityGroupsList.stories.tsx import { mockGroups } from './mockData'; MultipleGroupsWithAddNLazyLoad.args = { groups: [mockGroups.datasources, mockGroups.apis] };
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
app/client/packages/design-system/ads/src/List/List.styles.tsx(2 hunks)app/client/packages/design-system/ads/src/Templates/EntityExplorer/EntityGroupsList/EntityGroupsList.stories.tsx(1 hunks)app/client/src/ce/pages/Editor/IDE/EditorPane/Query/hooks.tsx(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- app/client/packages/design-system/ads/src/List/List.styles.tsx
⏰ Context from checks skipped due to timeout of 90000ms (8)
- 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: chromatic-deployment
- GitHub Check: storybook-tests
🔇 Additional comments (4)
app/client/src/ce/pages/Editor/IDE/EditorPane/Query/hooks.tsx (1)
31-35: LGTM! Clean type imports.The new type imports from @appsmith/ads are well-structured and follow TypeScript best practices.
app/client/packages/design-system/ads/src/Templates/EntityExplorer/EntityGroupsList/EntityGroupsList.stories.tsx (3)
1-18: LGTM! Clean imports and meta configuration.
35-71: Fix className prefix typo.The className values contain a typo: 't--datasource-create-option-*' is misspelled.
167-168: Differentiate duplicate group titles.There are two groups titled "Apis". Consider using unique titles for clarity.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
app/client/packages/design-system/ads/src/Templates/EntityExplorer/EntityGroupsList/EntityGroupsList.tsx (4)
11-28: Add prop validation for required props.While the component is well-structured, consider adding runtime validation for required props using PropTypes or a similar validation mechanism to catch potential issues early in development.
33-39: Optimize useMemo dependencies.The useMemo hook's dependency array could be simplified since
group.items.lengthis derived fromgroup.items. Consider memoizinggroup.itemsinstead.- }, [visibleItemsCount, group.items.length]); + }, [visibleItemsCount, group.items]);
66-68: Remove redundant optional chaining.The optional chaining on lazyLoading is unnecessary since it's a local object that's always defined.
- {lazyLoading?.hasMore && ( - <LoadMore onClick={lazyLoading?.handleLoadMore} title="Load more..." /> + {lazyLoading.hasMore && ( + <LoadMore onClick={lazyLoading.handleLoadMore} title="Load more..." />
69-75: Add error boundary for click handler.The onClick handler for addConfig should be wrapped in a try-catch block to handle potential errors gracefully.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
app/client/packages/design-system/ads/src/List/List.stories.tsx(1 hunks)app/client/packages/design-system/ads/src/Templates/EntityExplorer/EntityGroupsList/EntityGroupsList.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- app/client/packages/design-system/ads/src/List/List.stories.tsx
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: client-lint / client-lint
- GitHub Check: client-check-cyclic-deps / check-cyclic-dependencies
- GitHub Check: client-build / client-build
- GitHub Check: client-prettier / prettier-check
- GitHub Check: chromatic-deployment
- GitHub Check: chromatic-deployment
- GitHub Check: storybook-tests
🔇 Additional comments (2)
app/client/packages/design-system/ads/src/Templates/EntityExplorer/EntityGroupsList/EntityGroupsList.tsx (2)
1-10: LGTM! Clean imports structure.The imports are well-organized and all dependencies are properly declared.
80-80: LGTM! Clean exports.The export statement is clear and follows best practices.
|
/build-deploy-preview skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/12706506131. |
|
Deploy-Preview-URL: https://ce-38512.dp.appsmith.com |
## Description Adding Group component in ADS Templates to use the same for a single list of grouped entities in the product. Fixes [appsmithorg#37615](appsmithorg#37615) [appsmithorg#37616](appsmithorg#37616) [appsmithorg#38288](appsmithorg#38288) [appsmithorg#38287](appsmithorg#38287) ## Automation /ok-to-test tags="@tag.All" ### 🔍 Cypress test results <!-- This is an auto-generated comment: Cypress test results --> > [!TIP] > 🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉 > Workflow run: <https://github.com/appsmithorg/appsmith/actions/runs/12704682977> > Commit: b62fecb > <a href="https://internal.appsmith.com/app/cypress-dashboard/rundetails-65890b3c81d7400d08fa9ee5?branch=master&workflowId=12704682977&attempt=2" target="_blank">Cypress dashboard</a>. > Tags: `@tag.All` > Spec: > <hr>Fri, 10 Jan 2025 09:11:50 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 ## Release Notes - **New Features** - Enhanced List component with improved grouping capabilities. - Added support for dynamic list rendering with custom children. - Introduced `EntityGroupsList` for more flexible group management. - Added new styled components for better visual representation of lists. - Added `LoadMore` styled component for improved UX in entity groups. - New Storybook examples for `EntityGroupsList` and `EntityGroup` components. - **Improvements** - Refined List component to support more flexible item rendering. - Updated styling for list components. - Improved type safety for list-related components. - Streamlined code structure for better maintainability. - **Changes** - Removed `GroupedList` component. - Updated list rendering across multiple components. - Simplified list item management. - Expanded public API for `EntityExplorer` module with new exports. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
Description
Adding Group component in ADS Templates to use the same for a single list of grouped entities in the product.
Fixes #37615 #37616 #38288 #38287
Automation
/ok-to-test tags="@tag.All"
🔍 Cypress test results
Tip
🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/12704682977
Commit: b62fecb
Cypress dashboard.
Tags:
@tag.AllSpec:
Fri, 10 Jan 2025 09:11:50 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
Release Notes
New Features
EntityGroupsListfor more flexible group management.LoadMorestyled component for improved UX in entity groups.EntityGroupsListandEntityGroupcomponents.Improvements
Changes
GroupedListcomponent.EntityExplorermodule with new exports.