chore: Replace entities in entity explorer with new ADS templates #38750
Conversation
…oups list templates
…o chore/replace-entities-in-entity-explorer
WalkthroughThis pull request introduces significant refactoring across multiple components in the Appsmith client application, focusing on enhancing the entity management and context menu functionality. The changes primarily involve removing Changes
Assessment against linked issues
Possibly related issues
Possibly related PRs
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (14)
app/client/src/ce/pages/Editor/IDE/EditorPane/Query/ActionEntity/index.tsx (1)
5-10: LGTM! Consider adding JSDoc documentation.Clean pass-through component with proper typing. Adding JSDoc would help document the component's purpose and props.
+/** + * ActionEntity serves as a wrapper for QueryEntity, providing type-safe prop forwarding + * @param props.parentEntityId - ID of the parent entity + * @param props.item - Entity item details + */ export const ActionEntity = (props: {app/client/src/ce/pages/Editor/IDE/EditorPane/Query/ListItem.tsx (1)
Line range hint
12-23: Consider making searchKeyword prop configurable.The searchKeyword prop is hardcoded to an empty string. If search functionality is needed, this should be made configurable through props.
export interface QueryListItemProps { item: EntityItem; isActive: boolean; parentEntityId: string; + searchKeyword?: string; } export const QueryListItem = (props: QueryListItemProps) => { - const { isActive, item, parentEntityId } = props; + const { isActive, item, parentEntityId, searchKeyword = "" } = props;app/client/src/ce/IDE/hooks/useParentEntityInfo.ts (1)
14-19: Consider making parentEntityType dynamic.The parentEntityType is hardcoded to PAGE. Consider deriving it from ideType parameter for more flexibility.
return { editorId: appId || "", parentEntityId: basePageId || "", - parentEntityType: ActionParentEntityType.PAGE, + parentEntityType: getParentEntityTypeFromIDE(ideType), };app/client/src/ce/entities/IDE/utils.ts (1)
81-88: Simplify redundant switch statement.The switch statement can be simplified since both cases return the same items.
-export const getMenuItemsForActionEntityByIdeType = (ideType: IDEType) => { - switch (ideType) { - case IDE_TYPE.App: - return defaultActionMenuItems; - default: - return defaultActionMenuItems; - } -}; +export const getMenuItemsForActionEntityByIdeType = (ideType: IDEType) => { + return defaultActionMenuItems; +};app/client/src/ce/pages/Editor/IDE/EditorPane/Query/utils.ts (1)
60-78: Simplify redundant switch statement and consider future extensibility.The switch statement can be simplified as it returns the same items for all cases. However, if this is a placeholder for future differentiation between parent types, consider adding a TODO comment.
export const getMenuItemsForActionEntityByParentType = ( parentEntityType: ActionParentEntityTypeInterface, ) => { const defaultMenuItems = [ ActionEntityContextMenuItemsEnum.RENAME, ActionEntityContextMenuItemsEnum.DELETE, ActionEntityContextMenuItemsEnum.SHOW_BINDING, ActionEntityContextMenuItemsEnum.COPY, ActionEntityContextMenuItemsEnum.MOVE, ActionEntityContextMenuItemsEnum.CONVERT_QUERY_MODULE_INSTANCE, ]; - - switch (parentEntityType) { - case "PAGE": - return defaultMenuItems; - default: - return defaultMenuItems; - } + // TODO: Add specific menu items for different parent types if needed + return defaultMenuItems; };app/client/src/ce/entities/IDE/constants.ts (2)
143-150: LGTM! Consider using const enum for better performance.The enum values are well-defined and descriptive.
-export enum ActionEntityContextMenuItemsEnum { +export const enum ActionEntityContextMenuItemsEnum {
152-159: LGTM! Consider adding type annotation for better maintainability.The array correctly uses all enum values.
-export const defaultActionMenuItems = [ +export const defaultActionMenuItems: ActionEntityContextMenuItemsEnum[] = [app/client/src/pages/Editor/Explorer/Actions/ActionEntity.tsx (1)
50-50: Consider memoizing ideType to prevent unnecessary recalculations.The IDE type is derived from the location pathname on every render.
- const ideType = getIDETypeByUrl(location.pathname); + const ideType = useMemo(() => getIDETypeByUrl(location.pathname), [location.pathname]);app/client/src/ce/hooks/datasourceEditorHooks.tsx (1)
Line range hint
75-134: Consider extracting the nested logic into separate functions.The useHeaderActions hook contains deeply nested logic that could be simplified for better maintainability.
Consider splitting into:
- useGeneratePageButton
- useNewActionButton
- useHeaderPermissions
app/client/src/pages/Editor/IDE/EditorPane/Query/ActionEntity/QueryEntity.tsx (2)
54-54: Consider memoizing validateName hook dependencies.The validateName hook is created with empty dependencies on every render.
- const validateName = useValidateEntityName({}); + const validateNameOptions = useMemo(() => ({}), []); + const validateName = useValidateEntityName(validateNameOptions);
79-94: Consider extracting analytics logic into a custom hook.The switchToAction callback handles both navigation and analytics tracking. These concerns could be separated for better maintainability.
Consider creating a custom hook like
useActionAnalyticsto handle all analytics events.app/client/src/pages/Editor/CustomWidgetBuilder/useCustomBuilder.tsx (1)
Line range hint
166-167: Consider addressing the TODO comment.The event type could be properly typed instead of using
any. This would improve type safety.- window.addEventListener("message", (event: any) => { + window.addEventListener("message", (event: MessageEvent<{ + type: string; + name?: string; + widgetId?: string; + srcDoc?: string; + uncompiledSrcDoc?: string; + model?: unknown; + events?: unknown; + theme?: unknown; + }>) => {app/client/src/pages/Editor/IDE/EditorPane/Query/ActionEntity/QueryEntityContextMenu.tsx (1)
115-198: Consider extracting menu options configuration.The optionsTree array is quite large and contains complex logic. Consider extracting it into a separate configuration function for better maintainability.
app/client/src/pages/Editor/IntegrationEditor/APIOrSaasPlugins.tsx (1)
Line range hint
117-125: Consider extracting plugin type determination logic.The inline ternary for determining plugin type could be extracted into a helper function for better readability.
+ const getPluginType = (source: string) => + source === API_ACTION.CREATE_NEW_GRAPHQL_API + ? PluginPackageName.GRAPHQL + : PluginPackageName.REST_API; props.createNewApiActionBasedOnIdeType( ideType, editorId, parentEntityId || (isOnboardingScreen && pageId) || "", parentEntityType, - source === API_ACTION.CREATE_NEW_GRAPHQL_API - ? PluginPackageName.GRAPHQL - : PluginPackageName.REST_API, + getPluginType(source), );
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (37)
app/client/src/PluginActionEditor/components/PluginActionResponse/components/DatasourceTab/DatasourceInfo.tsx(2 hunks)app/client/src/PluginActionEditor/components/PluginActionResponse/components/DatasourceTab/DatasourceTab.tsx(3 hunks)app/client/src/ce/IDE/hooks/useParentEntityInfo.ts(1 hunks)app/client/src/ce/actions/helpers.ts(3 hunks)app/client/src/ce/entities/IDE/constants.ts(1 hunks)app/client/src/ce/entities/IDE/utils.ts(2 hunks)app/client/src/ce/hooks/datasourceEditorHooks.tsx(3 hunks)app/client/src/ce/hooks/hooks.test.ts(0 hunks)app/client/src/ce/hooks/index.ts(0 hunks)app/client/src/ce/pages/Editor/IDE/EditorPane/Query/ActionEntity/index.tsx(1 hunks)app/client/src/ce/pages/Editor/IDE/EditorPane/Query/ListItem.tsx(1 hunks)app/client/src/ce/pages/Editor/IDE/EditorPane/Query/utils.ts(2 hunks)app/client/src/ce/pages/Editor/gitSync/useReconnectModalData.ts(2 hunks)app/client/src/ce/utils/BusinessFeatures/permissionPageHelpers.tsx(2 hunks)app/client/src/ce/utils/lintRulesHelpers.ts(1 hunks)app/client/src/ee/IDE/hooks/useParentEntityInfo.ts(1 hunks)app/client/src/ee/pages/Editor/IDE/EditorPane/Query/ActionEntity/index.tsx(1 hunks)app/client/src/pages/Editor/CustomWidgetBuilder/useCustomBuilder.tsx(2 hunks)app/client/src/pages/Editor/DataSourceEditor/DSFormHeader.tsx(3 hunks)app/client/src/pages/Editor/DataSourceEditor/hooks.ts(3 hunks)app/client/src/pages/Editor/DatasourceInfo/DatasourceStructure.tsx(2 hunks)app/client/src/pages/Editor/DatasourceInfo/DatasourceViewModeSchema.tsx(3 hunks)app/client/src/pages/Editor/DatasourceInfo/GoogleSheetSchema.tsx(3 hunks)app/client/src/pages/Editor/Explorer/Actions/ActionEntity.tsx(3 hunks)app/client/src/pages/Editor/Explorer/Files/index.tsx(0 hunks)app/client/src/pages/Editor/IDE/EditorPane/Query/ActionEntity/QueryEntity.tsx(1 hunks)app/client/src/pages/Editor/IDE/EditorPane/Query/ActionEntity/QueryEntityContextMenu.tsx(1 hunks)app/client/src/pages/Editor/IDE/EditorPane/Query/List.tsx(4 hunks)app/client/src/pages/Editor/IDE/LeftPane/DataSidePane.tsx(2 hunks)app/client/src/pages/Editor/IDE/hooks.ts(2 hunks)app/client/src/pages/Editor/IntegrationEditor/APIOrSaasPlugins.tsx(6 hunks)app/client/src/pages/Editor/IntegrationEditor/DBOrMostPopularPlugins.tsx(9 hunks)app/client/src/pages/Editor/IntegrationEditor/DatasourceCard.tsx(3 hunks)app/client/src/pages/Editor/gitSync/ReconnectDatasourceModal.tsx(2 hunks)app/client/src/plugins/Linting/constants.ts(3 hunks)app/client/src/plugins/Linting/tests/generateLintingGlobalData.test.ts(0 hunks)app/client/src/plugins/Linting/utils/getLintingErrors.ts(5 hunks)
💤 Files with no reviewable changes (4)
- app/client/src/ce/hooks/hooks.test.ts
- app/client/src/pages/Editor/Explorer/Files/index.tsx
- app/client/src/plugins/Linting/tests/generateLintingGlobalData.test.ts
- app/client/src/ce/hooks/index.ts
✅ Files skipped from review due to trivial changes (3)
- app/client/src/ee/pages/Editor/IDE/EditorPane/Query/ActionEntity/index.tsx
- app/client/src/ee/IDE/hooks/useParentEntityInfo.ts
- app/client/src/pages/Editor/gitSync/ReconnectDatasourceModal.tsx
⏰ Context from checks skipped due to timeout of 90000ms (5)
- 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
🔇 Additional comments (27)
app/client/src/ce/pages/Editor/gitSync/useReconnectModalData.ts (2)
1-1: LGTM! Clean import of IDE_TYPE constant.The import from the enterprise edition constants maintains good separation of concerns.
39-39: Verify the IDE_TYPE.App usage across the codebase.The change from EditorNames to IDE_TYPE is consistent with the PR's objective. Let's verify other usages to ensure consistency.
✅ Verification successful
Migration to IDE_TYPE.App is complete and consistent
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any remaining EditorNames.APPLICATION usage that should be migrated rg "EditorNames.APPLICATION" # Verify consistent usage of IDE_TYPE.App rg "IDE_TYPE.App"Length of output: 1755
app/client/src/pages/Editor/IntegrationEditor/DBOrMostPopularPlugins.tsx (4)
30-30: LGTM! Clean import restructuring.The imports reflect a well-structured transition from editor-type to IDE-type based architecture.
Also applies to: 41-41, 53-54
189-198: LGTM! Consistent prop propagation.The IDE type changes are consistently propagated through the component hierarchy.
Also applies to: 287-287
Line range hint
348-363: LGTM! Consistent Redux integration.The dispatch mapping correctly implements the new action creator signature.
257-259: Verify hook usage and dependencies.The transition to
getIDETypeByUrlanduseParentEntityInfolooks good. Let's verify the hook's usage pattern.✅ Verification successful
✓ Hook usage verification completed successfully
The transition from
useEditorTypetouseParentEntityInfois complete and consistent across all components.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for any remaining useEditorType usage rg -g '*.{ts,tsx}' 'useEditorType' # Verify consistent useParentEntityInfo usage rg -g '*.{ts,tsx}' 'useParentEntityInfo.*ideType'Length of output: 1116
app/client/src/ce/utils/lintRulesHelpers.ts (1)
Line range hint
3-17: Verify the empty implementation of getLintRulesBasedOnContext.The function returns an empty object despite accepting a ContextType parameter. This might indicate incomplete implementation or missing logic after the type change.
✅ Verification successful
Empty implementation is correct as this is the CE (Community Edition) version
The empty implementation in the CE version is intentional, as the actual rules are defined in the Enterprise Edition (ee/utils/lintRulesHelpers). This follows the standard CE/EE split pattern.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for usages of getLintRulesBasedOnContext to understand expected behavior rg -A 3 "getLintRulesBasedOnContext"Length of output: 1293
app/client/src/PluginActionEditor/components/PluginActionResponse/components/DatasourceTab/DatasourceInfo.tsx (1)
28-29: LGTM! The transition to IDE-type looks good.The change from
useEditorTypetogetIDETypeByUrlaligns with the PR's objective of standardizing IDE-type usage.app/client/src/ce/actions/helpers.ts (1)
Line range hint
67-85: Address TODO comment and improve type safety.The function has a TODO comment and returns
anytype. Consider addressing both issues.export const createNewApiActionBasedOnIdeType = ( ideType: IDEType, editorId: string, parentEntityId: string, parentEntityType: ActionParentEntityTypeInterface, apiType: string, - // TODO: Fix this the next time the file is edited - // eslint-disable-next-line @typescript-eslint/no-explicit-any -): any => { +): ReturnType<typeof createNewAPIBasedOnParentEntity> => { if (parentEntityId) { return createNewAPIBasedOnParentEntity( parentEntityId, "API_PANE", apiType, parentEntityType, ); } + return undefined; };app/client/src/pages/Editor/IDE/EditorPane/Query/List.tsx (2)
55-57: LGTM! Feature flag implementation follows best practices.The feature flag is appropriately used to control the rollout of the new EntityGroupsList component.
78-93: Well-structured component composition with proper type safety.The EntityGroupsList implementation provides better type safety and cleaner separation of concerns.
app/client/src/pages/Editor/DataSourceEditor/hooks.ts (1)
Line range hint
144-154: Verify permission checks with new IDE type system.The switch from editorType to ideType affects permission checks. Please ensure all permission scenarios are tested.
✅ Verification successful
Permission checks with IDE type system verified successfully
The permission system correctly handles different IDE types with appropriate permission checks for each context.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for all permission check usages with the new IDE type rg -A 3 "hasCreateDSActionPermissionInApp.*ideType"Length of output: 53
Script:
#!/bin/bash # Find the function definition ast-grep --pattern 'export function hasCreateDSActionPermissionInApp($_) { $$$ }' # Search for IDE type related code rg -A 2 "getIDETypeByUrl|ideType.*=.*getIDEType" # Broader search for the permission function rg "hasCreateDSActionPermissionInApp"Length of output: 18916
Script:
#!/bin/bash # Get the implementation of hasCreateDSActionPermissionInApp ast-grep --pattern 'export const hasCreateDSActionPermissionInApp = $_' # Also check for any IDE type constants that might be used in the implementation rg -A 5 "IDE_TYPE.*=.*{"Length of output: 78325
app/client/src/pages/Editor/IDE/LeftPane/DataSidePane.tsx (1)
62-63: LGTM! Consistent usage of IDE type system.The dsUsageSelector update maintains consistency with the new IDE type pattern.
app/client/src/pages/Editor/CustomWidgetBuilder/useCustomBuilder.tsx (1)
32-33: LGTM! Clean hook implementation with IDE type.The useParentEntityInfo hook is correctly updated to use the new IDE type system.
app/client/src/pages/Editor/DataSourceEditor/DSFormHeader.tsx (2)
30-30: LGTM! Clean transition from editor type to IDE type.The refactoring from
useEditorTypetogetIDETypeByUrlmaintains the component's functionality while improving type safety.Also applies to: 120-120
168-168: LGTM! Updated hook usage.The
useHeaderActionshook is correctly updated to use the newideTypeparameter.app/client/src/PluginActionEditor/components/PluginActionResponse/components/DatasourceTab/DatasourceTab.tsx (1)
26-26: LGTM! Consistent refactoring to IDE types.The changes align with the codebase-wide transition from editor types to IDE types, improving type safety without affecting functionality.
Also applies to: 36-36, 61-62
app/client/src/plugins/Linting/constants.ts (2)
46-46: LGTM! Updated context parameter.The
getLintRulesBasedOnContextcall correctly uses the newideTypeparameter.
6-6: Verify the impact of the type change.The parameter type change from
stringtoIDETypeimproves type safety but may affect existing callers oflintOptions.Also applies to: 16-16
✅ Verification successful
Type change verification completed - No issues found.
The change from
stringtoIDETypeis safe as the only caller ingetLintingErrors.tsalready uses the correct type throughgetIDETypeByUrlandIDE_TYPE.App.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for all calls to lintOptions to verify they pass the correct type rg -A 2 "lintOptions\("Length of output: 277
Script:
#!/bin/bash # Find lintOptions implementation ast-grep --pattern 'const lintOptions = $_' # Find IDEType definition ast-grep --pattern 'type IDEType = $_' # Check the file where lintOptions is used cat app/client/src/plugins/Linting/utils/getLintingErrors.tsLength of output: 82735
Script:
#!/bin/bash # Find IDEType definition and usages rg "IDEType" -A 3 # Find getLintRulesBasedOnContext implementation rg "getLintRulesBasedOnContext" -A 3Length of output: 33527
app/client/src/pages/Editor/IDE/hooks.ts (1)
30-30: LGTM! Consistent IDE type usage.The changes in
useSegmentNavigationmaintain the same functionality while adopting the new IDE type system.Also applies to: 34-34, 66-67
app/client/src/ce/utils/BusinessFeatures/permissionPageHelpers.tsx (1)
46-46: LGTM! Type safety improvement.The transition from string-based editorType to strongly-typed IDEType enhances type safety and maintainability.
Also applies to: 96-96
app/client/src/pages/Editor/DatasourceInfo/DatasourceViewModeSchema.tsx (1)
Line range hint
85-96: LGTM! Clean transition to IDE-type.The change from editorType to ideType is clean and maintains the existing functionality.
app/client/src/pages/Editor/IntegrationEditor/APIOrSaasPlugins.tsx (1)
Line range hint
65-71: LGTM! Well-typed function signature.The createNewApiActionBasedOnIdeType function signature is well-defined with proper typing.
app/client/src/pages/Editor/IntegrationEditor/DatasourceCard.tsx (1)
63-63: LGTM! Clean transition from editor-type to IDE-type.The changes correctly implement the new IDE-type based approach while maintaining the component's functionality.
Also applies to: 182-182, 193-193
app/client/src/pages/Editor/DatasourceInfo/GoogleSheetSchema.tsx (1)
Line range hint
24-24: LGTM! Consistent implementation of IDE-type transition.The changes align with the codebase-wide refactoring from editor-type to IDE-type.
Also applies to: 69-69, 75-75
app/client/src/plugins/Linting/utils/getLintingErrors.ts (1)
48-49: LGTM! Proper integration of IDE-type in linting context.The changes correctly integrate the IDE-type concept into the linting process while maintaining the existing error handling functionality.
Also applies to: 84-84, 143-143, 160-160, 376-376, 381-381
app/client/src/pages/Editor/DatasourceInfo/DatasourceStructure.tsx (1)
24-24: LGTM! Consistent implementation of IDE-type transition.The changes follow the established pattern of transitioning from editor-type to IDE-type.
Also applies to: 69-69, 75-75
app/client/src/pages/Editor/IntegrationEditor/DBOrMostPopularPlugins.tsx
Outdated
Show resolved
Hide resolved
app/client/src/pages/Editor/IDE/EditorPane/Query/ActionEntity/QueryEntityContextMenu.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
app/client/src/pages/Editor/IDE/EditorPane/Query/ListItem/QueryEntityItem.tsx (2)
39-46: Consider memoizing selector results.Multiple useSelector calls could benefit from memoization to prevent unnecessary re-renders.
+const selectAction = useCallback( + (state: AppState) => getActionByBaseId(state, item.key), + [item.key] +); +const action = useSelector(selectAction) as Action; +const selectDatasource = useCallback( + (state) => getDatasource(state, action?.datasource?.id), + [action?.datasource?.id] +); +const datasource = useSelector(selectDatasource) as Datasource;
79-94: Consider extracting analytics logic.The switchToAction callback handles both navigation and analytics. Consider separating these concerns for better maintainability.
+const logAnalytics = useCallback((url: string) => { + AnalyticsUtil.logEvent("ENTITY_EXPLORER_CLICK", { + type: "QUERIES/APIs", + fromUrl: location.pathname, + toUrl: url, + name: action.name, + }); + AnalyticsUtil.logEvent("EDIT_ACTION_CLICK", { + actionId: action?.id, + datasourceId: datasource?.id, + pluginName: pluginGroups[action?.pluginId]?.name, + actionType: action?.pluginType === PluginType.DB ? "Query" : "API", + isMock: !!datasource?.isMock, + }); +}, [location.pathname, action, datasource, pluginGroups]); const switchToAction = useCallback(() => { url && history.push(url, { invokedBy: NavigationMethod.EntityExplorer }); - AnalyticsUtil.logEvent... + logAnalytics(url); }, [url, logAnalytics]);app/client/src/pages/Editor/IDE/EditorPane/Query/List.tsx (2)
78-129: Consider extracting the group rendering logic.The conditional rendering creates duplicate group structure logic. Consider extracting this into a separate component.
+const GroupRenderer = ({ group, items, isNewTemplate, parentEntityId }) => { + if (isNewTemplate) { + return ( + <ActionEntityItem + item={items} + parentEntityId={parentEntityId} + /> + ); + } + return ( + <QueryListItem + isActive={items.key === activeActionBaseId} + item={items} + parentEntityId={parentEntityId} + /> + ); +};
55-57: Document the feature flag's purpose.Add a comment explaining what this feature flag controls and its implications.
+// Controls the rollout of new ADS templates for entity rendering +// When enabled, uses EntityGroupsList for improved list organization const isNewADSTemplatesEnabled = useFeatureFlag( FEATURE_FLAG.release_ads_entity_item_enabled, );app/client/src/pages/Editor/IDE/EditorPane/Query/ListItem/QueryEntityContextMenu.tsx (1)
132-153: Document IDE-specific copy behavior.The copy functionality differs between App and other IDE types. Consider adding a comment explaining this distinction for better maintainability.
+ // In App IDE, copy shows a submenu of pages + // In other IDE types, it performs an immediate duplicate action menuItems.includes(ActionEntityContextMenuItemsEnum.COPY) &&
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
app/client/src/ce/pages/Editor/IDE/EditorPane/Query/ListItem/index.tsx(1 hunks)app/client/src/ce/pages/Editor/IDE/EditorPane/Query/ListItem/old/ListItem.tsx(1 hunks)app/client/src/ce/selectors/entitiesSelector.ts(2 hunks)app/client/src/ee/pages/Editor/IDE/EditorPane/Query/ListItem/old/ListItem.tsx(1 hunks)app/client/src/pages/Editor/IDE/EditorPane/Query/List.tsx(3 hunks)app/client/src/pages/Editor/IDE/EditorPane/Query/ListItem/QueryEntityContextMenu.tsx(1 hunks)app/client/src/pages/Editor/IDE/EditorPane/Query/ListItem/QueryEntityItem.tsx(1 hunks)app/client/src/pages/Editor/IDE/LeftPane/DataSidePane.tsx(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- app/client/src/ee/pages/Editor/IDE/EditorPane/Query/ListItem/old/ListItem.tsx
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: client-unit-tests / client-unit-tests
- GitHub Check: client-lint / client-lint
- GitHub Check: client-check-cyclic-deps / check-cyclic-dependencies
- GitHub Check: client-build / client-build
- GitHub Check: client-prettier / prettier-check
🔇 Additional comments (10)
app/client/src/pages/Editor/IDE/LeftPane/DataSidePane.tsx (2)
33-33: LGTM! Clean import addition.The import of
getIDETypeByUrlaligns with the PR's objective of transitioning from editor type to IDE type.
62-64: LGTM! Clean transition to IDE type.The changes maintain proper React hooks usage order and successfully transition from editor type to IDE type.
Let's verify that all usages of
useEditorTypehave been replaced:✅ Verification successful
Verified: Clean transition to IDE type completed
The codebase shows no remaining instances of
useEditorTypeand demonstrates consistent usage ofgetIDETypeByUrlacross all relevant components. The implementation in DataSidePane.tsx follows the established pattern.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any remaining instances of useEditorType rg "useEditorType" --type ts --type tsx # Verify consistent usage of getIDETypeByUrl rg "getIDETypeByUrl" --type ts --type tsxLength of output: 137
Script:
#!/bin/bash # Search for any remaining instances of useEditorType echo "=== Searching for useEditorType ===" rg "useEditorType" -g "*.{ts,tsx}" # Search for imports of useEditorType echo -e "\n=== Searching for useEditorType imports ===" rg "import.*useEditorType" -g "*.{ts,tsx}" # Verify consistent usage of getIDETypeByUrl echo -e "\n=== Searching for getIDETypeByUrl ===" rg "getIDETypeByUrl" -g "*.{ts,tsx}" # Search for imports of getIDETypeByUrl echo -e "\n=== Searching for getIDETypeByUrl imports ===" rg "import.*getIDETypeByUrl" -g "*.{ts,tsx}"Length of output: 10326
app/client/src/ce/pages/Editor/IDE/EditorPane/Query/ListItem/index.tsx (1)
5-10: Clean implementation of the adapter pattern!The
ActionEntityItemcomponent provides a good abstraction layer overQueryEntityItem, making future modifications easier to manage.app/client/src/ce/pages/Editor/IDE/EditorPane/Query/ListItem/old/ListItem.tsx (1)
12-12: Verify the impact of removing parentEntityType.The removal of parentEntityType from props destructuring aligns with the IDE type refactoring. However, let's verify all usages are updated.
✅ Verification successful
Removal of parentEntityType is verified as safe.
The prop removal is part of an intentional refactoring where the old implementation has been replaced with new versions in both CE and EE that explicitly exclude this prop.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any remaining references to parentEntityType in action-related components rg -l 'parentEntityType.*ExplorerActionEntity' app/client/src/Length of output: 64
Script:
#!/bin/bash # Search for ListItem component usage rg -l "ListItem" app/client/src/ # Search for parentEntityType in broader context rg "parentEntityType" app/client/src/ # Find ExplorerActionEntity component definition ast-grep --pattern 'interface ExplorerActionEntityProps { $$$ }'Length of output: 9301
Script:
#!/bin/bash # Check new ListItem implementations cat app/client/src/ce/pages/Editor/IDE/EditorPane/Query/ListItem/index.tsx cat app/client/src/ee/pages/Editor/IDE/EditorPane/Query/ListItem/index.tsxLength of output: 539
app/client/src/ce/selectors/entitiesSelector.ts (2)
67-67: LGTM!The
IDETypetype import is correctly placed with other IDE-related type imports.
1705-1714: LGTM! Verify all call sites are updated.The changes improve type safety by replacing the generic string type with the specific
IDETypetype.Let's verify that all call sites have been updated to use the new type:
✅ Verification successful
Changes verified - parameter type change is properly implemented
The change from
editorType: stringtoideType: IDETypeis correctly implemented and all usages have been updated. The type is properly defined in the constants file and imported where needed.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any remaining usage of the old parameter name rg -l "getDatasourceUsageCountForApp.*editorType" # Search for all current usages to verify they use the new parameter rg -l "getDatasourceUsageCountForApp.*ideType"Length of output: 100
Script:
#!/bin/bash # Search for any usage of the selector rg "getDatasourceUsageCountForApp" -A 2 # Search for any remaining editorType references in the same directory fd ".*\.ts$" app/client/src/ce/selectors -x rg "editorType" # Use AST to find selector usage ast-grep --pattern 'getDatasourceUsageCountForApp($$$)'Length of output: 67108
Script:
#!/bin/bash # Search for IDEType definition rg "type IDEType" -A 2 # Search for any remaining editorType in DataSidePane rg "editorType" app/client/src/pages/Editor/IDE/LeftPane/DataSidePane.tsx -A 2 # Get context of the selector usage in DataSidePane rg "dsUsageSelector" app/client/src/pages/Editor/IDE/LeftPane/DataSidePane.tsx -A 5Length of output: 2374
app/client/src/pages/Editor/IDE/EditorPane/Query/ListItem/QueryEntityContextMenu.tsx (4)
1-50: Clean and well-structured imports and interface definition!The imports are logically organized and the EntityContextMenuProps interface is well-defined with clear typing.
51-60: Well-implemented component setup with proper state management!The component correctly uses the new IDE type utilities and maintains clean state management.
200-207: Clean and efficient rendering logic!The conditional rendering and type handling are well-implemented.
84-89: Consider handling deletion failures.The delete action callback doesn't handle potential failures. Consider adding error handling to prevent navigation on failed deletions.
…o chore/replace-entities-in-entity-explorer
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
app/client/src/pages/Editor/IntegrationEditor/APIOrSaasPlugins.tsx (1)
Line range hint
118-126: Add error handling for plugin creation.The plugin creation process should handle potential failures and provide user feedback.
props.createNewApiActionBasedOnIdeType( ideType, editorId, parentEntityId || (isOnboardingScreen && pageId) || "", parentEntityType, source === API_ACTION.CREATE_NEW_GRAPHQL_API ? PluginPackageName.GRAPHQL : PluginPackageName.REST_API, - ); + ).catch((error) => { + // Handle error and show appropriate feedback + console.error("Failed to create API action:", error); + // Show error notification to user + });
🧹 Nitpick comments (9)
app/client/src/pages/Editor/IDE/EditorPane/Query/List.tsx (2)
49-51: Consider moving blank state check after search functionality.The current implementation shows the blank state before the search functionality, which might not be the optimal user experience. Consider moving the blank state check after the search component to allow users to search even when starting with no items.
- {objectKeys(itemGroups).length === 0 && <BlankState />} {itemGroups.length > 0 ? ( <SearchAndAdd onAdd={openAddQuery} onSearch={setSearchTerm} showAddButton={canCreateActions} /> ) : null} + {objectKeys(itemGroups).length === 0 && searchTerm === "" && <BlankState />}Also applies to: 62-71
72-123: Extract common group title rendering logic.Both implementations render group titles similarly. Consider extracting this into a shared component to maintain consistency and reduce duplication.
const GroupTitle = ({ title }: { title: string }) => ( <Text className="overflow-hidden overflow-ellipsis whitespace-nowrap" kind="body-s" > {title} </Text> );Then use it in both implementations:
{isNewADSTemplatesEnabled ? ( <EntityGroupsList groups={filteredItemGroups.map(({ group, items }) => ({ groupTitle: group, items: items, + titleComponent: <GroupTitle title={group} />, className: "", renderList: (item: EntityItem) => ( <ActionEntityItem item={item} parentEntityId={parentEntityId} /> ), }))} /> ) : ( filteredItemGroups.map(({ group, items }) => ( <Flex flexDirection={"column"} key={group}> <Flex py="spaces-1"> - <Text - className="overflow-hidden overflow-ellipsis whitespace-nowrap" - kind="body-s" - > - {group} - </Text> + <GroupTitle title={group} /> </Flex> // ... rest of the codeapp/client/src/pages/Editor/IDE/EditorPane/Query/ListItem/QueryEntityItem.tsx (4)
36-42: Consider memoizing selector results to prevent unnecessary re-renders.The Redux selectors might trigger re-renders on unrelated state updates. Consider using
createSelectorfromreselectfor these selectors.- const action = useSelector((state: AppState) => - getActionByBaseId(state, item.key), - ) as Action; - const datasource = useSelector((state) => - getDatasource(state, (action?.datasource as StoredDatasource)?.id), - ) as Datasource; + const action = useSelector( + useMemo( + () => (state: AppState) => getActionByBaseId(state, item.key), + [item.key] + ) + ) as Action; + const datasource = useSelector( + useMemo( + () => (state: AppState) => + getDatasource(state, (action?.datasource as StoredDatasource)?.id), + [action?.datasource?.id] + ) + ) as Datasource;
59-62: Add error handling for permission check.The permission check should handle potential errors and provide appropriate feedback to users when permissions are denied.
const canManageAction = getHasManageActionPermission( isFeatureEnabled, actionPermissions, ); + const handlePermissionError = useCallback(() => { + // Show appropriate error message or feedback + AnalyticsUtil.logEvent("PERMISSION_DENIED", { + actionId: action.id, + permissionType: "manage_action", + }); + }, [action.id]);
74-86: Consider consolidating analytics events.Multiple analytics events for the same user action could be consolidated into a single event with comprehensive data.
- AnalyticsUtil.logEvent("ENTITY_EXPLORER_CLICK", { - type: "QUERIES/APIs", - fromUrl: location.pathname, - toUrl: url, - name: action.name, - }); - AnalyticsUtil.logEvent("EDIT_ACTION_CLICK", { - actionId: action?.id, - datasourceId: datasource?.id, - pluginName: pluginGroups[action?.pluginId]?.name, - actionType: action?.pluginType === PluginType.DB ? "Query" : "API", - isMock: !!datasource?.isMock, - }); + AnalyticsUtil.logEvent("ACTION_INTERACTION", { + type: "QUERIES/APIs", + interaction: "EDIT", + fromUrl: location.pathname, + toUrl: url, + name: action.name, + actionId: action?.id, + datasourceId: datasource?.id, + pluginName: pluginGroups[action?.pluginId]?.name, + actionType: action?.pluginType === PluginType.DB ? "Query" : "API", + isMock: !!datasource?.isMock, + });
89-111: Add accessibility attributes to improve user experience.The EntityItem component should include ARIA attributes for better accessibility support.
<EntityItem className="action t--action-entity" + role="listitem" + aria-label={`${item.title} action`} id={action.id} isSelected={activeActionBaseId === action.id} key={action.id}app/client/src/pages/Editor/IntegrationEditor/APIOrSaasPlugins.tsx (3)
Line range hint
66-71: Add prop type validation for required parameters.The
createNewApiActionBasedOnIdeTypecallback props should validate its parameters.+ const validateIdeParams = ( + ideType: IDEType, + editorId: string, + parentEntityId: string, + parentEntityType: ActionParentEntityTypeInterface, + ) => { + if (!ideType || !editorId) { + throw new Error("Invalid IDE parameters"); + } + };
Line range hint
134-182: Simplify event handling logic.The switch statement and nested conditions could be simplified using a map of handlers.
+ const actionHandlers = { + [API_ACTION.CREATE_NEW_API]: () => handleCreateNew(API_ACTION.CREATE_NEW_API), + [API_ACTION.CREATE_NEW_GRAPHQL_API]: () => handleCreateNew(API_ACTION.CREATE_NEW_GRAPHQL_API), + [API_ACTION.CREATE_DATASOURCE_FORM]: (params) => { + if (params?.pluginId) { + props.createTempDatasourceFromForm({ + pluginId: params.pluginId, + type: params.type!, + }); + } + }, + [API_ACTION.AUTH_API]: () => handleCreateAuthApiDatasource(), + }; + - switch (actionType) { - case API_ACTION.CREATE_NEW_API: - case API_ACTION.CREATE_NEW_GRAPHQL_API: - handleCreateNew(actionType); - break; - // ... other cases - } + const handler = actionHandlers[actionType]; + if (handler) handler(params);
351-351: Consider using React.memo for performance optimization.The component could benefit from memoization to prevent unnecessary re-renders.
- createNewApiActionBasedOnIdeType, + createNewApiActionBasedOnIdeType: React.useCallback( + (...args) => createNewApiActionBasedOnIdeType(...args), + [] + ),
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
app/client/src/ce/entities/IDE/hooks/useActionContextMenuByIdeType.tsx(1 hunks)app/client/src/ce/entities/IDE/hooks/useCreateActionsPermissions.ts(1 hunks)app/client/src/ee/entities/IDE/hooks/useActionContextMenuByIdeType.tsx(1 hunks)app/client/src/ee/entities/IDE/hooks/useCreateActionsPermissions.ts(1 hunks)app/client/src/pages/Editor/IDE/EditorPane/Query/List.tsx(3 hunks)app/client/src/pages/Editor/IDE/EditorPane/Query/ListItem/QueryEntityItem.tsx(1 hunks)app/client/src/pages/Editor/IntegrationEditor/APIOrSaasPlugins.tsx(6 hunks)
✅ Files skipped from review due to trivial changes (2)
- app/client/src/ee/entities/IDE/hooks/useCreateActionsPermissions.ts
- app/client/src/ee/entities/IDE/hooks/useActionContextMenuByIdeType.tsx
⏰ Context from checks skipped due to timeout of 90000ms (8)
- 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-check-cyclic-deps / check-cyclic-dependencies
- GitHub Check: client-prettier / prettier-check
🔇 Additional comments (5)
app/client/src/pages/Editor/IDE/EditorPane/Query/List.tsx (2)
2-29: LGTM! Clean organization of imports and hooks.The new imports and hooks are well-structured and follow React best practices.
36-38: Verify IDE type determination across routes.The IDE type determination is crucial for the component's functionality. Let's verify its usage across different routes.
✅ Verification successful
IDE type determination implementation is correct and consistent
The usage of
getIDETypeByUrlin List.tsx follows the established pattern used consistently across the codebase, maintaining proper type safety and route handling.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for other usages of getIDETypeByUrl to ensure consistent implementation rg "getIDETypeByUrl" --type ts --type tsx -B 2 -A 2 # Check for any hardcoded IDE types that might conflict ast-grep --pattern 'ideType = $_' -l typescriptLength of output: 256
Script:
#!/bin/bash # Search with correct file type specifications rg "getIDETypeByUrl" --type typescript --type tsx -B 2 -A 2 # Find the definition of getIDETypeByUrl ast-grep --pattern 'function getIDETypeByUrl($_) { $$$ }' # Look for related IDE type handling in routes rg "IDE_TYPE" --type typescript --type tsx -B 2 -A 2Length of output: 244
Script:
#!/bin/bash # Search for the file and function usage with correct type arguments rg "getIDETypeByUrl" -t js -t ts # Find relevant files fd -e ts -e tsx List.tsx # Try alternative pattern for function definition ast-grep --pattern 'export function getIDETypeByUrl($_) { $$$ }' # Look for IDE type related constants rg "IDE_TYPE" -t js -t tsLength of output: 12422
app/client/src/pages/Editor/IDE/EditorPane/Query/ListItem/QueryEntityItem.tsx (1)
64-70: 🛠️ Refactor suggestionAdd URL validation before navigation.
The URL generation should be validated to ensure it's properly formatted and safe for navigation.
const config = getActionConfig(action.pluginType); const url = config?.getURL( parentEntityId ?? "", action.baseId, action.pluginType, pluginGroups[action.pluginId], ); + const isValidUrl = url && /^[/][a-zA-Z0-9-/_]+$/.test(url); + if (!isValidUrl) { + console.error("Invalid URL generated:", url); + return; + }app/client/src/ce/entities/IDE/hooks/useCreateActionsPermissions.ts (1)
12-19: Consider explicit handling of all IDE types.The switch statement only handles
IDE_TYPE.Appexplicitly, defaulting totruefor all other cases. This might be too permissive and could lead to unintended access.Let's verify all possible IDE types:
Consider handling each IDE type explicitly to make the permission logic more intentional:
switch (ideType) { case IDE_TYPE.App: { return getHasCreateActionPermission(isFeatureEnabled, pagePermissions); } + case IDE_TYPE.Query: { + return true; // Add explicit logic if needed + } + case IDE_TYPE.API: { + return true; // Add explicit logic if needed + } default: { - return true; + console.warn(`Unhandled IDE type: ${ideType}`); + return false; // Fail safe by default } }app/client/src/ce/entities/IDE/hooks/useActionContextMenuByIdeType.tsx (1)
34-42: Add prop types validation for QueryEntityContextMenu.The component is being used without proper prop types validation.
Let's verify if prop types exist:
Consider creating an interface for the props and validating them:
interface QueryEntityContextMenuProps { canDeleteAction: boolean; canManageAction: boolean; id: string; name: string; parentEntityId: string; pluginType: string; }
app/client/src/ce/entities/IDE/hooks/useActionContextMenuByIdeType.tsx
Outdated
Show resolved
Hide resolved
…o chore/replace-entities-in-entity-explorer
|
🔴🔴🔴 Cyclic Dependency Check: This PR has increased the number of cyclic dependencies by 115, 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: 3
🔭 Outside diff range comments (1)
app/client/src/components/editorComponents/form/fields/DropdownWrapper.tsx (1)
Line range hint
31-33: Address technical debt: Remove TODOs and type annotations.Multiple TODOs and
eslint-disabledirectives indicate areas needing improvement:
- Replace
anytypes with proper type definitions- Implement the pending fixes mentioned in TODOs
Consider this type-safe implementation:
- const [selectedOption, setSelectedOption] = useState<any>([ + const [selectedOption, setSelectedOption] = useState<Partial<SelectOptionProps>[]>([ { value: props.placeholder, }, ]); - const onSelectHandler = (value?: any, option?: any) => { + const onSelectHandler = ( + value?: string, + option?: Partial<SelectOptionProps> | Partial<SelectOptionProps>[] + ) => { - const onRemoveOptions = (value: any) => { + const onRemoveOptions = (value: string) => { const updatedItems = selectedOption.filter( - (option: any) => option.value !== value, + (option: Partial<SelectOptionProps>) => option.value !== value, );Also applies to: 37-39, 52-54, 55-57
🧹 Nitpick comments (17)
app/client/src/pages/Editor/AppPluginActionEditor/components/ContextMenuItems/Move.tsx (2)
46-49: Consider more explicit conditional prop assignment.While the spread operator works, a more explicit condition would improve readability.
- <MenuSubTrigger - disabled={disabled} - {...(!hideIcon && { startIcon: "swap-horizontal" })} - > + <MenuSubTrigger + disabled={disabled} + startIcon={hideIcon ? undefined : "swap-horizontal"} + >
52-52: Consider moving styles to styled-components or CSS modules.Inline styles work but could be better maintained in a separate styling solution.
const StyledMenuSubContent = styled(MenuSubContent)` max-height: 350px; width: 220px; `;app/client/src/ce/pages/Editor/EntityEditor/ConvertToModuleInstanceCTA/index.tsx (1)
8-8: LGTM! Consider documenting the hideIcon prop.The addition of the optional hideIcon prop aligns with the component's purpose. Consider adding JSDoc to document its usage.
+ /** Whether to hide the icon in the UI */ hideIcon?: boolean;app/client/src/pages/Editor/AppPluginActionEditor/components/ContextMenuItems/Rename.tsx (1)
15-20: Consider extracting the timeout delay to a constant.The setTimeout delay is hardcoded. Consider extracting it to a named constant for better maintainability.
+const RENAME_FOCUS_DELAY = 100; const setRename = useCallback(() => { // We add a delay to avoid having the focus stuck in the menu trigger setTimeout(() => { dispatch(initExplorerEntityNameEdit(action.id)); - }, 100); + }, RENAME_FOCUS_DELAY); }, [dispatch, action.id]);app/client/src/pages/Editor/AppPluginActionEditor/components/ContextMenuItems/Copy.tsx (1)
41-41: Extract style values to constants.The hardcoded style values should be extracted to named constants for better maintainability.
+const MENU_SUB_CONTENT_STYLES = { + maxHeight: "350px", + width: "220px", +} as const; -<MenuSubContent style={{ maxHeight: "350px" }} width="220px"> +<MenuSubContent style={MENU_SUB_CONTENT_STYLES}>app/client/src/pages/Editor/AppPluginActionEditor/components/ContextMenuItems/ConvertToModule.tsx (1)
34-37: Add a detailed comment explaining the workflow query limitation.The early return for internal plugin types would benefit from a more detailed comment explaining the technical limitation.
- if (action.pluginType === PluginType.INTERNAL) { - // Workflow queries cannot be converted to modules - return null; - } + // Internal plugin types represent workflow queries which have dependencies + // on workflow-specific features that are not supported in modules + if (action.pluginType === PluginType.INTERNAL) { + return null; + }app/client/src/components/editorComponents/form/fields/DropdownWrapper.tsx (2)
Line range hint
5-22: Enhance type safety for selected values.The
DropdownWrapperPropsinterface is well-defined, but the component's implementation could be more type-safe.Consider adding a generic type parameter:
interface DropdownWrapperProps<T extends string | number = string> { value?: T; options: Partial<SelectOptionProps & { value: T }>[]; // ... rest of the props }
Line range hint
59-71: Consider splitting component and simplifying effect logic.The component handles both single and multi-select modes, which increases complexity. The
useEffectalso has multiple conditions and dependencies.Consider:
- Splitting into
SingleSelectDropdownandMultiSelectDropdowncomponents- Moving the selection state management to a custom hook
- Simplifying the effect logic by using a controlled component pattern
app/client/src/pages/Editor/AppPluginActionEditor/components/ContextMenuItems/ShowBindings.tsx (1)
14-34: Consider adding error handling and performance optimizations.The component implementation is clean, but could benefit from some improvements:
+import React, { useCallback, memo } from "react"; interface Props { action: Action; + /** Whether the menu item should be disabled */ disabled?: boolean; } -export const ShowBindings = ({ action, disabled }: Props) => { +export const ShowBindings = memo(({ action, disabled }: Props) => { const dispatch = useDispatch(); const handleSelect = useCallback(() => { + try { dispatch({ type: ReduxActionTypes.SET_ENTITY_INFO, payload: { entityId: action.id, entityName: action.name, entityType: ENTITY_TYPE.ACTION, show: true, }, }); + } catch (error) { + console.error("Failed to show bindings:", error); + } }, [action.id, action.name]); return ( <MenuItem disabled={disabled} onSelect={handleSelect}> {createMessage(CONTEXT_SHOW_BINDING)} </MenuItem> ); -}; +});app/client/src/pages/Editor/AppPluginActionEditor/components/ContextMenuItems/Delete.tsx (1)
49-54: Consider more explicit prop spreading.While the spread operator works, consider making the startIcon prop more explicit for better maintainability.
- {...(!hideIcon && { startIcon: "trash" })} + startIcon={hideIcon ? undefined : "trash"}app/client/src/pages/Editor/IDE/EditorPane/Query/ListItem/AppQueryContextMenuItems.tsx (3)
1-17: Consider grouping related imports.The imports could be better organized by grouping related imports together (React, types, hooks, components).
import React from "react"; + import type { Action } from "entities/Action"; + import { useFeatureFlag } from "utils/hooks/useFeatureFlag"; import { FEATURE_FLAG } from "ee/entities/FeatureFlag"; import { getHasDeleteActionPermission, getHasManageActionPermission, } from "ee/utils/BusinessFeatures/permissionPageHelpers"; + import { ConvertToModule, Copy, Delete, Move, Rename, ShowBindings, } from "pages/Editor/AppPluginActionEditor/components/ContextMenuItems";
22-48: Inconsistent use of hideIcon prop.The
hideIconprop is applied to some menu items but not to others (Rename and ShowBindings). This inconsistency might affect the visual uniformity of the menu.- <Rename action={action} disabled={!canManageAction} /> - <ShowBindings action={action} disabled={false} /> + <Rename action={action} disabled={!canManageAction} hideIcon /> + <ShowBindings action={action} disabled={false} hideIcon />
41-42: Consider adding a tooltip for disabled menu items.When menu items are disabled due to insufficient permissions, it would be helpful to show a tooltip explaining why.
app/client/src/pages/Editor/IDE/EditorPane/Query/ListItem/EntityContextMenu.tsx (2)
10-12: Consider adding documentation for the Props interface.Add JSDoc comments to explain the purpose of the
menuContentprop and its optional nature.+/** + * Props for EntityContextMenu component + * @property {React.ReactNode[] | React.ReactNode} menuContent - Optional content to be rendered inside the menu + */ interface Props { menuContent?: React.ReactNode[] | React.ReactNode; }
36-42: Avoid hardcoding dimensions in the component.The width "220px" is hardcoded in the component. Consider moving this to a constants file or making it configurable through props.
app/client/src/pages/Editor/IDE/EditorPane/Query/ListItem/QueryEntityItem.tsx (1)
70-85: Memoize analytics event data.The analytics event data object is recreated on every render. Consider memoizing it with useMemo to optimize performance.
+const analyticsData = useMemo(() => ({ + actionId: action?.id, + datasourceId: datasource?.id, + pluginName: pluginGroups[action?.pluginId]?.name, + actionType: action?.pluginType === PluginType.DB ? "Query" : "API", + isMock: !!datasource?.isMock, +}), [action, datasource, pluginGroups]); const switchToAction = useCallback(() => { url && history.push(url, { invokedBy: NavigationMethod.EntityExplorer }); AnalyticsUtil.logEvent("ENTITY_EXPLORER_CLICK", { type: "QUERIES/APIs", fromUrl: location.pathname, toUrl: url, name: action.name, }); - AnalyticsUtil.logEvent("EDIT_ACTION_CLICK", { - actionId: action?.id, - datasourceId: datasource?.id, - pluginName: pluginGroups[action?.pluginId]?.name, - actionType: action?.pluginType === PluginType.DB ? "Query" : "API", - isMock: !!datasource?.isMock, - }); + AnalyticsUtil.logEvent("EDIT_ACTION_CLICK", analyticsData); }, [url, location.pathname, action, datasource, pluginGroups]);app/client/src/pages/Editor/IDE/EditorPane/Query/List.tsx (1)
72-118: Consider extracting common group rendering logic.The group title rendering logic is duplicated between the new EntityGroupsList and the traditional mapping approach. Consider extracting this into a shared component.
+ const GroupTitle = ({ title }: { title: string }) => ( + <Flex py="spaces-1"> + <Text + className="overflow-hidden overflow-ellipsis whitespace-nowrap" + kind="body-s" + > + {title} + </Text> + </Flex> + ); {isNewADSTemplatesEnabled ? ( <EntityGroupsList groups={filteredItemGroups.map(({ group, items }) => ({ groupTitle: group, items, className: "", renderList: (item: EntityItem) => ( <ActionEntityItem item={item} /> ), }))} /> ) : ( filteredItemGroups.map(({ group, items }) => ( <Flex flexDirection="column" key={group}> - <Flex py="spaces-1"> - <Text - className="overflow-hidden overflow-ellipsis whitespace-nowrap" - kind="body-s" - > - {group} - </Text> - </Flex> + <GroupTitle title={group} /> <FilesContextProvider
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (18)
app/client/src/ce/entities/IDE/utils.tsx(3 hunks)app/client/src/ce/pages/Editor/EntityEditor/ConvertToModuleInstanceCTA/index.tsx(1 hunks)app/client/src/ce/pages/Editor/IDE/EditorPane/Query/ListItem/index.tsx(1 hunks)app/client/src/components/editorComponents/form/fields/DropdownWrapper.tsx(1 hunks)app/client/src/pages/Editor/AppPluginActionEditor/components/ContextMenuItems/ConvertToModule.tsx(1 hunks)app/client/src/pages/Editor/AppPluginActionEditor/components/ContextMenuItems/Copy.tsx(2 hunks)app/client/src/pages/Editor/AppPluginActionEditor/components/ContextMenuItems/Delete.tsx(2 hunks)app/client/src/pages/Editor/AppPluginActionEditor/components/ContextMenuItems/Move.tsx(2 hunks)app/client/src/pages/Editor/AppPluginActionEditor/components/ContextMenuItems/Rename.tsx(1 hunks)app/client/src/pages/Editor/AppPluginActionEditor/components/ContextMenuItems/ShowBindings.tsx(1 hunks)app/client/src/pages/Editor/AppPluginActionEditor/components/ContextMenuItems/index.ts(1 hunks)app/client/src/pages/Editor/AppPluginActionEditor/components/ConvertToModule/index.tsx(0 hunks)app/client/src/pages/Editor/AppPluginActionEditor/components/ToolbarMenu/Rename.tsx(0 hunks)app/client/src/pages/Editor/AppPluginActionEditor/components/ToolbarMenu/ToolbarMenu.tsx(2 hunks)app/client/src/pages/Editor/IDE/EditorPane/Query/List.tsx(3 hunks)app/client/src/pages/Editor/IDE/EditorPane/Query/ListItem/AppQueryContextMenuItems.tsx(1 hunks)app/client/src/pages/Editor/IDE/EditorPane/Query/ListItem/EntityContextMenu.tsx(1 hunks)app/client/src/pages/Editor/IDE/EditorPane/Query/ListItem/QueryEntityItem.tsx(1 hunks)
💤 Files with no reviewable changes (2)
- app/client/src/pages/Editor/AppPluginActionEditor/components/ToolbarMenu/Rename.tsx
- app/client/src/pages/Editor/AppPluginActionEditor/components/ConvertToModule/index.tsx
✅ Files skipped from review due to trivial changes (1)
- app/client/src/pages/Editor/AppPluginActionEditor/components/ContextMenuItems/index.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- app/client/src/ce/pages/Editor/IDE/EditorPane/Query/ListItem/index.tsx
🧰 Additional context used
📓 Learnings (1)
app/client/src/ce/entities/IDE/utils.tsx (1)
Learnt from: ankitakinger
PR: appsmithorg/appsmith#37056
File: app/client/src/pages/Editor/JSEditor/JSObjectNameEditor/JSObjectNameEditor.tsx:22-25
Timestamp: 2024-11-12T08:11:43.278Z
Learning: "constants/AppConstants" does not export "SaveActionNameParams".
⏰ Context from checks skipped due to timeout of 90000ms (8)
- 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-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
🔇 Additional comments (14)
app/client/src/pages/Editor/AppPluginActionEditor/components/ContextMenuItems/Move.tsx (2)
14-20: LGTM! Clean type definitions.The Props interface is well-structured with proper typing for required and optional properties.
22-22: LGTM! Clean component signature.Props destructuring follows React best practices.
app/client/src/components/editorComponents/form/fields/DropdownWrapper.tsx (1)
83-83: LGTM! Follows HTML5 attribute naming convention.The change from
data-testIdtodata-testidaligns with HTML5 standards.app/client/src/pages/Editor/AppPluginActionEditor/components/ContextMenuItems/ShowBindings.tsx (2)
1-12: LGTM! Clean imports and well-defined Props interface.The type definitions and imports are well-organized and properly typed.
19-20: Verify Redux action type definition.Ensure that
ReduxActionTypes.SET_ENTITY_INFOis properly defined in the Redux action types.✅ Verification successful
Redux action type is properly defined and used correctly.
The
SET_ENTITY_INFOaction type is defined ince/constants/ReduxActionConstants.tsxand is used consistently throughout the codebase for entity information management. The implementation in ShowBindings.tsx follows the established pattern.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for SET_ENTITY_INFO action type definition rg "SET_ENTITY_INFO.*=" -A 2Length of output: 30
Script:
#!/bin/bash # Search for files that might contain Redux action types fd "actions|types|constants" --type f # Search for SET_ENTITY_INFO with more flexible pattern rg -i "SET_ENTITY_INFO" -A 2 # Search specifically in TypeScript/JavaScript files rg "SET_ENTITY_INFO" --type ts --type js -A 2Length of output: 47059
app/client/src/pages/Editor/AppPluginActionEditor/components/ContextMenuItems/Delete.tsx (4)
8-15: LGTM! Clean type definitions and imports.The Props interface is well-structured with proper typing for the action prop and optional flags.
18-18: LGTM! Clean component signature.Props destructuring matches the interface definition perfectly.
35-42: LGTM! Proper event handling.Good addition of stopPropagation to prevent event bubbling.
22-33: Add null checks for action object.The current implementation might throw if action is undefined. Consider adding proper null checks.
- const handleDeleteClick = useCallback( - ({ onSuccess }: { onSuccess?: () => void }) => { - dispatch( - deleteAction({ - id: action?.id ?? "", - name: action?.name ?? "", - onSuccess, - }), - ); - }, - [action.id, action.name, dispatch], + const handleDeleteClick = useCallback( + ({ onSuccess }: { onSuccess?: () => void }) => { + if (!action) return; + dispatch( + deleteAction({ + id: action.id, + name: action.name, + onSuccess, + }), + ); + }, + [action?.id, action?.name, dispatch],app/client/src/pages/Editor/AppPluginActionEditor/components/ToolbarMenu/ToolbarMenu.tsx (2)
13-13: Clean import consolidation!Good job consolidating the imports from ContextMenuItems module.
32-37: Verify component prop changes across the codebase.The component updates look good with consistent prop passing. Let's verify the changes are applied uniformly.
✅ Verification successful
Component prop changes are consistently implemented
The search results confirm that all instances of these components in the codebase properly include the
actionprop and maintain consistent permission-based disabled states. No issues found.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any remaining instances of these components without the action prop ast-grep --pattern 'ConvertToModule $$$' ast-grep --pattern 'Copy disabled={$_}' ast-grep --pattern 'Move disabled={$_}' ast-grep --pattern 'Delete disabled={$_}'Length of output: 42
Script:
#!/bin/bash # Search for component usage with ripgrep echo "=== ConvertToModule Usage ===" rg "<ConvertToModule" --type ts --type tsx -l echo "=== Copy Component Usage ===" rg "<Copy\s" --type ts --type tsx -l echo "=== Move Component Usage ===" rg "<Move\s" --type ts --type tsx -l echo "=== Delete Component Usage ===" rg "<Delete\s" --type ts --type tsx -l # If we find the files, we can then check specific instances for component in "ConvertToModule" "Copy" "Move" "Delete"; do echo "=== $component Component Instances ===" rg "<$component.*?>" --type ts --type tsx -A 1 doneLength of output: 1415
Script:
#!/bin/bash # Search for component usage without type restrictions echo "=== Component Usage with Context ===" rg -U "<(ConvertToModule|Copy|Move|Delete).*?>" -A 1 # Search specifically for usage without action prop echo "=== Usage without action prop ===" rg -U "<(ConvertToModule|Copy|Move|Delete)[^>]*?(?<!action=)[^>]*?>" -A 1Length of output: 9204
app/client/src/pages/Editor/IDE/EditorPane/Query/List.tsx (3)
31-51: Clean hooks implementation and feature flag usage.The transition to IDE-based architecture is well implemented with proper hooks usage and clean feature flag integration.
73-84: Well-structured EntityGroupsList implementation.The new EntityGroupsList implementation provides better component composition and cleaner rendering logic.
2-29: Verify imports after the refactor.Let's verify that all imports are being used after the transition to IDE-based architecture.
✅ Verification successful
All imports are properly used in the component.
The verification confirms that all imported components, hooks, types and utilities are actively used within the ListQuery component.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for unused imports ast-grep --pattern 'import { $_ } from "$_"' | while read -r line; do import_name=$(echo "$line" | grep -o '{.*}' | tr -d '{} ') for name in $import_name; do if ! rg "\b$name\b" --type ts --type tsx > /dev/null; then echo "Potential unused import: $name" fi done doneLength of output: 131070
Script:
#!/bin/bash # Check usage of imported components and functions in the file echo "Checking imports usage in List.tsx..." # Check ADS components usage ast-grep --pattern '<$_/>' app/client/src/pages/Editor/IDE/EditorPane/Query/List.tsx | grep -E 'Flex|Text|SearchAndAdd|NoSearchResults|EntityGroupsList' # Check hooks and utility imports usage ast-grep --pattern '$_\(' app/client/src/pages/Editor/IDE/EditorPane/Query/List.tsx | grep -E 'useSelector|useActiveActionBaseId|useFeatureFlag|useQueryAdd|useLocation|useParentEntityInfo|useCreateActionsPermissions' # Check type usage ast-grep --pattern ': $_' app/client/src/pages/Editor/IDE/EditorPane/Query/List.tsx | grep -E 'EntityItem|ActionParentEntityType'Length of output: 590
Script:
#!/bin/bash # First verify file exists and show content echo "=== File content ===" cat app/client/src/pages/Editor/IDE/EditorPane/Query/List.tsx echo -e "\n=== Checking component usage ===" # Check both self-closing and regular JSX tags ast-grep --pattern ' <$_> $$$ </$_>' app/client/src/pages/Editor/IDE/EditorPane/Query/List.tsx ast-grep --pattern '<$_ $$$/> ' app/client/src/pages/Editor/IDE/EditorPane/Query/List.tsx echo -e "\n=== Checking hook usage ===" # Check hook calls with different patterns ast-grep --pattern ' const $_ = $_($$$) ' app/client/src/pages/Editor/IDE/EditorPane/Query/List.tsxLength of output: 9854
app/client/src/pages/Editor/IDE/EditorPane/Query/ListItem/QueryEntityItem.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
app/client/src/pages/Editor/IDE/EditorPane/Query/ListItem/QueryEntityItem.tsx (3)
70-82: Consider extracting analytics logic to a separate function.Multiple analytics events are logged together, making the code harder to maintain. Consider extracting these into a separate analytics utility function.
+const logActionAnalytics = (action: Action, datasource: Datasource, fromUrl: string, toUrl: string, pluginGroups: Record<string, any>) => { + AnalyticsUtil.logEvent("ENTITY_EXPLORER_CLICK", { + type: "QUERIES/APIs", + fromUrl, + toUrl, + name: action.name, + }); + AnalyticsUtil.logEvent("EDIT_ACTION_CLICK", { + actionId: action?.id, + datasourceId: datasource?.id, + pluginName: pluginGroups[action?.pluginId]?.name, + actionType: action?.pluginType === PluginType.DB ? "Query" : "API", + isMock: !!datasource?.isMock, + }); +};
30-30: Add JSDoc documentation for the component props.Consider adding JSDoc documentation to describe the expected shape and purpose of the EntityItemProps interface.
+/** + * QueryEntityItem component renders a query or API entity in the IDE. + * @param {Object} props - Component props + * @param {EntityItemProps} props.item - The entity item data + */ export const QueryEntityItem = ({ item }: { item: EntityItemProps }) => {
38-38: Optimize memoization dependencies.The
switchToActioncallback has many dependencies that could trigger unnecessary re-renders. Consider breaking down the callback into smaller functions or moving some computations outside the component.Also applies to: 68-83
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
app/client/src/ce/entities/IDE/utils.ts(2 hunks)app/client/src/ce/pages/Editor/IDE/EditorPane/Query/utils.tsx(3 hunks)app/client/src/pages/Editor/IDE/EditorPane/Query/ListItem/QueryEntityItem.tsx(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- 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
🔇 Additional comments (3)
app/client/src/ce/pages/Editor/IDE/EditorPane/Query/utils.tsx (1)
Line range hint
1-18: LGTM!The imports are well-organized and properly typed.
app/client/src/pages/Editor/IDE/EditorPane/Query/ListItem/QueryEntityItem.tsx (2)
30-37: Add error handling for missing action or datasource.The component assumes that action and datasource will be available. Add proper error handling or fallback UI for cases where they might be undefined.
68-69: Add error handling for navigation failures.The URL navigation lacks error handling. Consider adding user feedback for cases where the URL is invalid or navigation fails.
|
🔴🔴🔴 Cyclic Dependency Check: This PR has increased the number of cyclic dependencies by 168, 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: 0
🧹 Nitpick comments (2)
app/client/src/pages/Editor/AppPluginActionEditor/components/ContextMenuItems/PageMenuItem.tsx (1)
14-18: Consider adding aria-label for accessibility.While the implementation is clean, adding an aria-label would improve accessibility for screen readers.
- <MenuItem disabled={props.disabled} onSelect={handleOnSelect}> + <MenuItem + disabled={props.disabled} + onSelect={handleOnSelect} + aria-label={`Page ${props.page.pageName}`} + >app/client/src/pages/Editor/AppPluginActionEditor/components/ContextMenuItems/Copy.tsx (1)
38-38: Consider making dimensions configurable via props.The hardcoded dimensions (
maxHeight: "350px",width: "220px") in MenuSubContent might limit reusability. Consider making these configurable through props with these values as defaults.- <MenuSubContent style={{ maxHeight: "350px" }} width="220px"> + <MenuSubContent + style={{ maxHeight: props.maxHeight ?? "350px" }} + width={props.width ?? "220px"} + >
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
app/client/src/pages/Editor/AppPluginActionEditor/components/ContextMenuItems/Copy.tsx(2 hunks)app/client/src/pages/Editor/AppPluginActionEditor/components/ContextMenuItems/Move.tsx(2 hunks)app/client/src/pages/Editor/AppPluginActionEditor/components/ContextMenuItems/PageMenuItem.tsx(1 hunks)app/client/src/pages/Editor/AppPluginActionEditor/components/ContextMenuItems/Rename.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- app/client/src/pages/Editor/AppPluginActionEditor/components/ContextMenuItems/Rename.tsx
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: perform-test / rts-build / build
- GitHub Check: perform-test / server-build / server-unit-tests
- GitHub Check: client-check-cyclic-deps / check-cyclic-dependencies
- GitHub Check: client-build / client-build
- GitHub Check: client-prettier / prettier-check
🔇 Additional comments (7)
app/client/src/pages/Editor/AppPluginActionEditor/components/ContextMenuItems/PageMenuItem.tsx (1)
8-8: LGTM! Clean prop interface extension.The optional
disabledprop is well-typed and follows React conventions.app/client/src/pages/Editor/AppPluginActionEditor/components/ContextMenuItems/Copy.tsx (2)
1-14: LGTM! Props interface is well-defined.The type definitions are clear and properly typed with the Action interface. Optional props are correctly marked.
Line range hint
17-34: LGTM! Component implementation is clean and reusable.The component now receives the action directly as a prop instead of relying on context, making it more modular and easier to test.
app/client/src/pages/Editor/AppPluginActionEditor/components/ContextMenuItems/Move.tsx (4)
14-14: LGTM! Type-safe props definition.The addition of the Action type and the Props interface update improves type safety.
Also applies to: 17-19
22-22: LGTM! Component signature matches Props interface.The destructuring pattern correctly matches the updated Props interface.
54-54: LGTM! Consistent prop forwarding.The disabled state is correctly propagated to the child component.
46-49: Verify the impact of fixed dimensions on MenuSubContent.The fixed width and maxHeight might affect the component's appearance in different contexts. Please ensure these values work well across all usage scenarios.
Run this script to find other instances of MenuSubContent to compare dimensions:
✅ Verification successful
The fixed dimensions align with existing menu patterns.
The specified width (220px) and maxHeight (350px) match the standard dimensions used across other MenuSubContent components in the codebase, ensuring consistent menu appearance.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for other MenuSubContent usage patterns rg -A 2 "MenuSubContent" --type tsxLength of output: 63
Script:
#!/bin/bash # Search for MenuSubContent without file type restriction rg "MenuSubContent" # Search for style properties in menu-related components rg -B 2 -A 2 'MenuSub.*Content.*style' # Search for width/height properties in menu components rg -B 2 -A 2 'Menu.*Content.*width|Menu.*Content.*height'Length of output: 82237
|
🔴🔴🔴 Cyclic Dependency Check: This PR has increased the number of cyclic dependencies by 168, 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: 0
🧹 Nitpick comments (2)
app/client/src/pages/Editor/AppPluginActionEditor/components/ContextMenuItems/ConvertToModule.tsx (2)
21-31: Consider aligning variable names with their usage.The permission check variables (
isCreatePermitted,isDeletePermitted) could be renamed to match their usage inconvertToModuleProps(canCreateModuleInstance,canDeleteEntity).- const isCreatePermitted = getHasCreateActionPermission( + const canCreateModuleInstance = getHasCreateActionPermission( isFeatureEnabled, pagePermissions, ); - const isDeletePermitted = getHasDeleteActionPermission( + const canDeleteEntity = getHasDeleteActionPermission( isFeatureEnabled, action.userPermissions, );
33-36: Consider enhancing the comment for better clarity.While the early return pattern is good, the comment could be more specific about why workflow queries cannot be converted.
- // Workflow queries cannot be converted to modules + // Skip conversion for internal workflow queries as they require special handling and cannot be modularized return null;
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
app/client/src/pages/Editor/AppPluginActionEditor/components/ContextMenuItems/ConvertToModule.tsx(1 hunks)app/client/src/pages/Editor/JSEditor/ContextMenuItems/ConvertToModule.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- app/client/src/pages/Editor/JSEditor/ContextMenuItems/ConvertToModule.tsx
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: client-unit-tests / client-unit-tests
- GitHub Check: client-lint / client-lint
- GitHub Check: client-check-cyclic-deps / check-cyclic-dependencies
- GitHub Check: client-build / client-build
- GitHub Check: client-prettier / prettier-check
🔇 Additional comments (3)
app/client/src/pages/Editor/AppPluginActionEditor/components/ContextMenuItems/ConvertToModule.tsx (3)
13-13: LGTM! Clean interface definition.The Props interface is well-defined with proper type imports.
Also applies to: 15-17
19-19: LGTM! Clean component declaration.Good use of named exports and props destructuring.
Line range hint
38-47: LGTM! Clean props handling and rendering.Well-structured props object with clear property names.
ankitakinger
added
ok-to-test
|
/build-deploy-preview skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/12926817081. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (6)
app/client/src/pages/Editor/IDE/EditorPane/Query/EntityItem/QueryEntityItem.tsx (4)
1-29: Consider reducing enterprise edition coupling.The component has tight coupling with enterprise edition (ee) modules. Consider extracting shared logic to common utilities to improve maintainability.
30-50: Consider memoizing selector results.Multiple selector calls (
getActionByBaseId,getDatasource) could impact performance. Consider combining these into a single memoized selector.// Example combined selector const selectActionAndDatasource = createSelector( [ (state: AppState, itemKey: string) => getActionByBaseId(state, itemKey), (state: AppState) => state, ], (action, state) => ({ action, datasource: action ? getDatasource(state, action.datasource?.id) : null, }) );
70-85: Extract analytics logic to a separate function.The
switchToActionfunction handles both navigation and analytics. Consider separating these concerns for better maintainability.const logActionAnalytics = useCallback((action: Action, datasource: Datasource) => { AnalyticsUtil.logEvent("ENTITY_EXPLORER_CLICK", { type: "QUERIES/APIs", fromUrl: location.pathname, toUrl: url, name: action.name, }); AnalyticsUtil.logEvent("EDIT_ACTION_CLICK", { actionId: action?.id, datasourceId: datasource?.id, pluginName: pluginGroups[action?.pluginId]?.name, actionType: action?.pluginType === PluginType.DB ? "Query" : "API", isMock: !!datasource?.isMock, }); }, [location.pathname, url]);
93-101: Extract name editor configuration.Consider extracting the
nameEditorConfigobject to improve readability and reusability.const getNameEditorConfig = ( action: Action, canManageAction: boolean, editingEntity: string, updatingEntity: string, ) => ({ canEdit: canManageAction, isEditing: editingEntity === action.id, isLoading: updatingEntity === action.id, onEditComplete: exitEditMode, onNameSave: (newName: string) => dispatch(saveActionNameBasedOnIdeType(action.id, newName, ideType)), validateName: (newName) => validateName(newName, item.title), });app/client/src/pages/Editor/IDE/EditorPane/JS/EntityItem/JSEntityItem.tsx (2)
78-78: Remove redundant Boolean call.The Boolean call is unnecessary as the expression will already be coerced to a boolean.
-canEdit: canManageJSAction && !Boolean(jsAction?.isMainJSCollection), +canEdit: canManageJSAction && !jsAction?.isMainJSCollection,🧰 Tools
🪛 Biome (1.9.4)
[error] 78-78: Avoid redundant
BooleancallIt is not necessary to use
Booleancall when a value will already be coerced to a boolean.
Unsafe fix: Remove redundantBooleancall(lint/complexity/noExtraBooleanCast)
57-69: Optimize useCallback dependencies.The dependency array includes
parentEntityIdwhich is used innavigateToUrl, but sincenavigateToUrlis already computed outside the callback, it should be included in the dependencies instead.- }, [parentEntityId, jsAction.baseId, jsAction.name, location.pathname]); + }, [navigateToUrl, jsAction.baseId, jsAction.name, location.pathname]);
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
app/client/src/pages/Editor/IDE/EditorPane/JS/EntityItem/JSEntityItem.tsx(1 hunks)app/client/src/pages/Editor/IDE/EditorPane/Query/EntityItem/QueryEntityItem.tsx(1 hunks)app/client/src/pages/Editor/JSEditor/ContextMenuItems/Move.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- app/client/src/pages/Editor/JSEditor/ContextMenuItems/Move.tsx
🧰 Additional context used
🪛 Biome (1.9.4)
app/client/src/pages/Editor/IDE/EditorPane/JS/EntityItem/JSEntityItem.tsx
[error] 78-78: Avoid redundant Boolean call
It is not necessary to use Boolean call when a value will already be coerced to a boolean.
Unsafe fix: Remove redundant Boolean call
(lint/complexity/noExtraBooleanCast)
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: perform-test / rts-build / build
- GitHub Check: perform-test / client-build / client-build
- GitHub Check: client-unit-tests / client-unit-tests
- GitHub Check: client-lint / client-lint
- GitHub Check: client-check-cyclic-deps / check-cyclic-dependencies
- GitHub Check: client-prettier / prettier-check
- GitHub Check: client-build / client-build
🔇 Additional comments (2)
app/client/src/pages/Editor/IDE/EditorPane/JS/EntityItem/JSEntityItem.tsx (2)
1-23: LGTM! Well-organized imports.The imports are properly structured with a clear separation between types and regular imports.
71-95: LGTM! Clean and well-structured component rendering.The EntityItem implementation is comprehensive with proper event handling and prop management.
🧰 Tools
🪛 Biome (1.9.4)
[error] 78-78: Avoid redundant
BooleancallIt is not necessary to use
Booleancall when a value will already be coerced to a boolean.
Unsafe fix: Remove redundantBooleancall(lint/complexity/noExtraBooleanCast)
app/client/src/pages/Editor/IDE/EditorPane/JS/EntityItem/JSEntityItem.tsx
Show resolved
Hide resolved
|
Deploy-Preview-URL: https://ce-38750.dp.appsmith.com |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
app/client/src/pages/Editor/JSEditor/ContextMenuItems/Delete.tsx (4)
12-16: Consider strengthening the Props interface type safety.The
deleteJSActioncallback is optional whilejsActionis required, but the code uses them together at line 23. This could lead to runtime errors.interface Props { jsAction: JSCollection; disabled?: boolean; - deleteJSAction?: () => void; + deleteJSAction: () => void; }
18-21: Consider memoizing the component for better performance.Since this component might be rendered in a list of actions, wrapping it with React.memo could prevent unnecessary re-renders.
-export const Delete = ({ deleteJSAction, disabled, jsAction }: Props) => { +export const Delete = React.memo(({ deleteJSAction, disabled, jsAction }: Props) => { // ... component implementation -}; +});
33-40: Improve event type safety.The event parameter type should be more specific than the generic
Event.- (e?: Event) => { + (e?: React.MouseEvent | React.KeyboardEvent) => {
46-55: Consider adding error handling feedback.The delete operation should handle and communicate potential failures to users.
Consider wrapping the delete operation in a try-catch and showing a toast notification on error:
const handleDeleteClick = useCallback(() => { + try { jsAction.isPublic && deleteJSAction ? deleteJSAction() : dispatch( deleteJSCollection({ id: jsAction?.id ?? "", name: jsAction?.name ?? "", }), ); + } catch (error) { + Toaster.show({ + text: "Failed to delete JS collection", + type: "error", + }); + } }, [jsAction.id, jsAction.name, dispatch]);
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app/client/src/pages/Editor/JSEditor/ContextMenuItems/Delete.tsx(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: perform-test / client-build / client-build
- GitHub Check: perform-test / rts-build / build
- GitHub Check: client-unit-tests / client-unit-tests
- GitHub Check: client-lint / client-lint
- GitHub Check: client-check-cyclic-deps / check-cyclic-dependencies
- GitHub Check: client-build / client-build
- GitHub Check: client-prettier / prettier-check
app/client/src/pages/Editor/JSEditor/ContextMenuItems/Delete.tsx
Outdated
Show resolved
Hide resolved
|
/build-deploy-preview skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/12928349798. |
|
Deploy-Preview-URL: https://ce-38750.dp.appsmith.com |
|
/build-deploy-preview skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/12931036842. |
|
Deploy-Preview-URL: https://ce-38750.dp.appsmith.com |
ankitakinger
changed the title
…o chore/replace-entities-in-entity-explorer
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (15)
app/client/src/pages/Editor/JSEditor/ContextMenuItems/PageMenuItem.tsx (3)
5-9: Consider destructuring props for clarity.
Instead of referencingprops, you could destructure fields in the parameter list to make the component more readable. For instance:-export const PageMenuItem = (props: { - page: Page; - onSelect: (id: string) => void; - disabled?: boolean; -}) => { +export const PageMenuItem = ({ + page, + onSelect, + disabled, +}: { + page: Page; + onSelect: (id: string) => void; + disabled?: boolean; +}) => {
10-12: OptimizeuseCallbackdependency.
Using[props]as the sole dependency means the callback will re-initialize whenever any prop changes. You could destructurepageIdandonSelectin the dependencies if only those fields are used.
15-17: Enhance accessibility for screen readers.
Consider adding anaria-labelfor the<MenuItem>to assist visually-impaired users:-<MenuItem disabled={props.disabled} onSelect={handleOnSelect}> +<MenuItem + aria-label={`Page menu item for ${props.page.pageName}`} + disabled={props.disabled} + onSelect={handleOnSelect} +>app/client/src/pages/Editor/IDE/EditorPane/components/EntityContextMenu.tsx (3)
10-12: Consider adding role and aria-label props for better accessibility.The Props interface could benefit from additional accessibility-related props to make the context menu more screen-reader friendly.
interface Props { children?: React.ReactNode[] | React.ReactNode; + role?: string; + 'aria-label'?: string; }
14-48: Consider memoizing the component for better performance.Since this is a reusable component that might be rendered multiple times in a list, consider wrapping it with React.memo to prevent unnecessary re-renders.
-const EntityContextMenu = (props: Props) => { +const EntityContextMenu = React.memo((props: Props) => { // ... component implementation -}; +});
41-41: Avoid using inline styles.The
maxHeight: "unset"style should be moved to a CSS class for better maintainability.app/client/src/pages/Editor/JSEditor/AppJSEditorContextMenu.tsx (1)
Line range hint
13-31: Consider extracting permission logic to a custom hook.The permission checking logic could be extracted into a custom hook for better reusability and cleaner component code.
+const useJSCollectionPermissions = ( + jsCollection: JSCollection +) => { + const isFeatureEnabled = useFeatureFlag(FEATURE_FLAG.license_gac_enabled); + return { + isDeletePermitted: getHasDeleteActionPermission( + isFeatureEnabled, + jsCollection?.userPermissions || [], + ), + isChangePermitted: getHasManageActionPermission( + isFeatureEnabled, + jsCollection?.userPermissions || [], + ), + }; +};app/client/src/pages/Editor/IDE/EditorPane/JS/EntityItem/JSEntityItem.tsx (2)
57-70: IncludenavigateToUrlin the dependency array
TheuseCallbackreferencesnavigateToUrlbut omits it from dependencies, which can lead to stale references ifnavigateToUrlchanges.- }, [parentEntityId, jsAction.baseId, jsAction.name, location.pathname]); + }, [parentEntityId, jsAction.baseId, jsAction.name, location.pathname, navigateToUrl]);
73-73: Remove redundantBooleancast
The linter warns about unnecessary boolean coercion. Simply negate the expression directly.- canEdit: canManageJSAction && !Boolean(jsAction.isMainJSCollection), + canEdit: canManageJSAction && !jsAction.isMainJSCollection,🧰 Tools
🪛 Biome (1.9.4)
[error] 73-73: Avoid redundant
BooleancallIt is not necessary to use
Booleancall when a value will already be coerced to a boolean.
Unsafe fix: Remove redundantBooleancall(lint/complexity/noExtraBooleanCast)
app/client/src/pages/Editor/IDE/EditorPane/JS/List.tsx (3)
47-49: Consider adding a fallback for feature flag.While the feature flag implementation is correct, consider adding error handling for cases where the flag value might be undefined.
const isNewADSTemplatesEnabled = useFeatureFlag( FEATURE_FLAG.release_ads_entity_item_enabled, + false );
87-87: Extract group title logic to avoid duplication.The group title logic is duplicated between the new and old implementations. Consider extracting it to a shared utility function.
+ const getGroupTitle = (group: string) => group === "NA" ? "" : group; // In new implementation - groupTitle: group === "NA" ? "" : group, + groupTitle: getGroupTitle(group), // In old implementation - {group !== "NA" ? ( - <Flex py="spaces-1"> - <Text - className="overflow-hidden overflow-ellipsis whitespace-nowrap" - kind="body-s" - > - {group} - </Text> - </Flex> - ) : null} + {getGroupTitle(group) && ( + <Flex py="spaces-1"> + <Text + className="overflow-hidden overflow-ellipsis whitespace-nowrap" + kind="body-s" + > + {getGroupTitle(group)} + </Text> + </Flex> + )}Also applies to: 100-109
90-92: Memoize the renderList callback to prevent unnecessary re-renders.The renderList callback is recreated on every render. Consider memoizing it with useCallback.
+ const renderJSEntity = useCallback((item: EntityItem) => { + return <JSEntity item={item} />; + }, []); // In EntityGroupsList props - renderList: (item: EntityItem) => { - return <JSEntity item={item} />; - }, + renderList: renderJSEntity,app/client/src/pages/Editor/IDE/EditorPane/Query/EntityItem/QueryEntityItem.tsx (3)
1-29: Consider organizing imports for better maintainability.Group imports into sections: external dependencies, internal utilities, types, and enterprise-specific imports.
+ // External dependencies import React, { useCallback, useMemo } from "react"; import { useDispatch, useSelector } from "react-redux"; import { useLocation } from "react-router"; import { keyBy } from "lodash"; + // Internal components and types import { EntityItem } from "@appsmith/ads"; import type { EntityItem as EntityItemProps } from "ee/entities/IDE/constants"; import type { Action, type StoredDatasource } from "entities/Action"; import type { Datasource } from "entities/Datasource"; import { PluginType } from "entities/Plugin"; + // State management and selectors import type { AppState } from "ee/reducers"; import { getActionByBaseId, getDatasource, getPlugins, } from "ee/selectors/entitiesSelector"; + // Utilities and hooks import { useFeatureFlag } from "utils/hooks/useFeatureFlag"; import { FEATURE_FLAG } from "ee/entities/FeatureFlag"; import history, { NavigationMethod } from "utils/history";
30-43: Optimize selector usage and add error handling.Consider these improvements:
- Combine selectors to reduce re-renders
- Add error handling for cases where action or datasource is undefined
- const action = useSelector((state: AppState) => - getActionByBaseId(state, item.key), - ) as Action; - const datasource = useSelector((state) => - getDatasource(state, (action?.datasource as StoredDatasource)?.id), - ) as Datasource; + const { action, datasource } = useSelector((state: AppState) => ({ + action: getActionByBaseId(state, item.key), + datasource: action?.datasource?.id + ? getDatasource(state, (action.datasource as StoredDatasource).id) + : null, + })); + + if (!action) { + return null; // or error state + }
70-85: Consider combining analytics events for better performance.Multiple separate analytics events could be combined into a single call.
- AnalyticsUtil.logEvent("ENTITY_EXPLORER_CLICK", { - type: "QUERIES/APIs", - fromUrl: location.pathname, - toUrl: url, - name: action.name, - }); - AnalyticsUtil.logEvent("EDIT_ACTION_CLICK", { - actionId: action?.id, - datasourceId: datasource?.id, - pluginName: pluginGroups[action?.pluginId]?.name, - actionType: action?.pluginType === PluginType.DB ? "Query" : "API", - isMock: !!datasource?.isMock, - }); + AnalyticsUtil.logEvent("ACTION_NAVIGATION", { + type: "QUERIES/APIs", + navigation: { + fromUrl: location.pathname, + toUrl: url, + name: action.name, + }, + action: { + id: action?.id, + datasourceId: datasource?.id, + pluginName: pluginGroups[action?.pluginId]?.name, + type: action?.pluginType === PluginType.DB ? "Query" : "API", + isMock: !!datasource?.isMock, + } + });
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
app/client/src/ce/pages/Editor/IDE/EditorPane/JS/utils/getJSContextMenuByIdeType.tsx(1 hunks)app/client/src/ce/pages/Editor/IDE/EditorPane/Query/utils/getQueryContextMenuByIdeType.tsx(1 hunks)app/client/src/pages/Editor/IDE/EditorPane/JS/EntityItem/JSEntityItem.tsx(1 hunks)app/client/src/pages/Editor/IDE/EditorPane/JS/List.tsx(3 hunks)app/client/src/pages/Editor/IDE/EditorPane/Query/EntityItem/QueryEntityItem.tsx(1 hunks)app/client/src/pages/Editor/IDE/EditorPane/components/EntityContextMenu.tsx(1 hunks)app/client/src/pages/Editor/JSEditor/AppJSEditorContextMenu.tsx(2 hunks)app/client/src/pages/Editor/JSEditor/ContextMenuItems/Copy.tsx(1 hunks)app/client/src/pages/Editor/JSEditor/ContextMenuItems/Move.tsx(1 hunks)app/client/src/pages/Editor/JSEditor/ContextMenuItems/PageMenuItem.tsx(1 hunks)app/client/src/pages/Editor/JSEditor/JSEditorContextMenu.tsx(0 hunks)
💤 Files with no reviewable changes (1)
- app/client/src/pages/Editor/JSEditor/JSEditorContextMenu.tsx
🚧 Files skipped from review as they are similar to previous changes (4)
- app/client/src/ce/pages/Editor/IDE/EditorPane/Query/utils/getQueryContextMenuByIdeType.tsx
- app/client/src/ce/pages/Editor/IDE/EditorPane/JS/utils/getJSContextMenuByIdeType.tsx
- app/client/src/pages/Editor/JSEditor/ContextMenuItems/Copy.tsx
- app/client/src/pages/Editor/JSEditor/ContextMenuItems/Move.tsx
🧰 Additional context used
🪛 Biome (1.9.4)
app/client/src/pages/Editor/IDE/EditorPane/JS/EntityItem/JSEntityItem.tsx
[error] 73-73: Avoid redundant Boolean call
It is not necessary to use Boolean call when a value will already be coerced to a boolean.
Unsafe fix: Remove redundant Boolean call
(lint/complexity/noExtraBooleanCast)
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: perform-test / rts-build / build
- GitHub Check: perform-test / client-build / client-build
- GitHub Check: perform-test / server-build / server-unit-tests
- GitHub Check: client-lint / client-lint
- GitHub Check: client-check-cyclic-deps / check-cyclic-dependencies
- GitHub Check: client-build / client-build
- GitHub Check: client-unit-tests / client-unit-tests
- GitHub Check: client-prettier / prettier-check
🔇 Additional comments (9)
app/client/src/pages/Editor/JSEditor/ContextMenuItems/PageMenuItem.tsx (1)
1-3: Imports look consistent with the codebase.
No issues found in the import statements. You are following best practices by using type imports in TypeScript to reduce compile-time overhead.app/client/src/pages/Editor/JSEditor/AppJSEditorContextMenu.tsx (1)
32-42: LGTM! Clean implementation of the context menu items.The component structure is clean and follows a good separation of concerns. Each menu item is properly configured with its permissions.
app/client/src/pages/Editor/IDE/EditorPane/JS/EntityItem/JSEntityItem.tsx (2)
25-27: Add runtime check forjsActionto avoid potential null/undefined issues
This reiterates a point made previously in the code review history regarding the risky type assertion.) as JSCollection; + if (!jsAction) { + return null; // or render a fallback component + }
94-109: File looks good overall
Apart from the above remarks, the rest of the component design is consistent and maintainable. Great job!app/client/src/pages/Editor/IDE/EditorPane/JS/List.tsx (1)
19-19: Verify the JSListItem import path change.The import path includes 'old' in its structure (
ee/pages/Editor/IDE/EditorPane/JS/old/ListItem). This might indicate a temporary migration state.app/client/src/pages/Editor/IDE/EditorPane/Query/EntityItem/QueryEntityItem.tsx (4)
44-50: LGTM - Name validation implementation is correct.The useValidateEntityName hook is properly configured with the required entityName prop.
87-105: LGTM - Name editor configuration is well implemented.The useMemo implementation with proper dependencies and validation function composition follows React best practices.
107-122: LGTM - Component rendering implementation is clean and follows best practices.The EntityItem implementation with proper event handling and conditional rendering is well structured.
62-69: Add validation for URL construction parameters.The URL construction could fail silently if pluginGroups[action.pluginId] is undefined.
const config = getActionConfig(action.pluginType); + const plugin = pluginGroups[action.pluginId]; + if (!plugin) { + console.warn(`Plugin not found for action: ${action.name}`); + } const url = config?.getURL( parentEntityId ?? "", action.baseId, action.pluginType, - pluginGroups[action.pluginId], + plugin, );
Description
Replace entities in entity explorer with new ADS templates
Fixes #38286 #39289
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/12945261277
Commit: bf508e2
Cypress dashboard.
Tags:
@tag.AllSpec:
Fri, 24 Jan 2025 08:55:33 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
Summary by CodeRabbit
Release Notes
New Features
Rename,Copy,Move,Delete,ShowBindings,Prettify, andEntityContextMenu.Improvements
Bug Fixes
Refactoring
These changes primarily focus on improving the user experience and developer flexibility within the application's editor interfaces.