-
Notifications
You must be signed in to change notification settings - Fork 4.7k
chore: refactor query duplication flow #36915
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 10 commits
8e24f09
2558f69
7a25592
0aa8f37
1278d2b
8a4eef5
65c94e3
0e6a414
5d94282
991b26a
9662d00
56ad42b
84b5cda
098b318
67eb34d
fe4c2ee
5fe13ea
6a18c58
7f4a43e
fe98984
04dfbfb
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,5 +1,6 @@ | ||
| export { useActionSettingsConfig } from "ee/PluginActionEditor/hooks/useActionSettingsConfig"; | ||
| export { useHandleDeleteClick } from "ee/PluginActionEditor/hooks/useHandleDeleteClick"; | ||
| export { useHandleDuplicateClick } from "ee/PluginActionEditor/hooks/useHandleDuplicateClick"; | ||
| export { useHandleRunClick } from "ee/PluginActionEditor/hooks/useHandleRunClick"; | ||
| export { useBlockExecution } from "ee/PluginActionEditor/hooks/useBlockExecution"; | ||
| export { useAnalyticsOnRunClick } from "ee/PluginActionEditor/hooks/useAnalyticsOnRunClick"; |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,26 @@ | ||
| import { copyActionRequest } from "actions/pluginActionActions"; | ||
| import { usePluginActionContext } from "PluginActionEditor/PluginActionContext"; | ||
| import { useCallback } from "react"; | ||
| import { useDispatch } from "react-redux"; | ||
|
|
||
| function useHandleDuplicateClick() { | ||
| const { action } = usePluginActionContext(); | ||
| const dispatch = useDispatch(); | ||
|
|
||
| const handleDuplicateClick = useCallback( | ||
| (destinationEditorId: string) => { | ||
| dispatch( | ||
| copyActionRequest({ | ||
| id: action.id, | ||
| destinationEditorId, | ||
| name: action.name, | ||
| }), | ||
| ); | ||
| }, | ||
| [action.id, action.name, dispatch], | ||
| ); | ||
|
|
||
| return { handleDuplicateClick }; | ||
| } | ||
|
|
||
| export { useHandleDuplicateClick }; |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1 @@ | ||
| export * from "ce/PluginActionEditor/hooks/useHandleDuplicateClick"; |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -20,6 +20,7 @@ import { | |
| CONTEXT_NO_PAGE, | ||
| CONTEXT_SHOW_BINDING, | ||
| createMessage, | ||
| CONTEXT_DUPLICATE, | ||
| } from "ee/constants/messages"; | ||
| import { builderURL } from "ee/RouteBuilder"; | ||
|
|
||
|
|
@@ -33,6 +34,7 @@ import { useConvertToModuleOptions } from "ee/pages/Editor/Explorer/hooks"; | |
| import { MODULE_TYPE } from "ee/constants/ModuleConstants"; | ||
| import { PluginType } from "entities/Action"; | ||
| import { convertToBaseParentEntityIdSelector } from "selectors/pageListSelectors"; | ||
| import { ActionParentEntityType } from "ee/entities/Engine/actionHelpers"; | ||
|
|
||
| interface EntityContextMenuProps { | ||
| id: string; | ||
|
|
@@ -45,20 +47,20 @@ interface EntityContextMenuProps { | |
| export function ActionEntityContextMenu(props: EntityContextMenuProps) { | ||
| // Import the context | ||
| const context = useContext(FilesContext); | ||
| const { menuItems, parentEntityId } = context; | ||
| const { menuItems, parentEntityId, parentEntityType } = context; | ||
| const baseParentEntityId = useSelector((state) => | ||
| convertToBaseParentEntityIdSelector(state, parentEntityId), | ||
| ); | ||
|
|
||
| const { canDeleteAction, canManageAction } = props; | ||
| const dispatch = useDispatch(); | ||
| const [confirmDelete, setConfirmDelete] = useState(false); | ||
| const copyActionToPage = useCallback( | ||
| (actionId: string, actionName: string, pageId: string) => | ||
| const copyAction = useCallback( | ||
| (actionId: string, actionName: string, destinationEditorId: string) => | ||
| dispatch( | ||
| copyActionRequest({ | ||
| id: actionId, | ||
| destinationPageId: pageId, | ||
| destinationEditorId, | ||
| name: actionName, | ||
| }), | ||
| ), | ||
|
|
@@ -129,14 +131,24 @@ export function ActionEntityContextMenu(props: EntityContextMenuProps) { | |
| menuItems.includes(ActionEntityContextMenuItemsEnum.COPY) && | ||
| canManageAction && { | ||
| value: "copy", | ||
| onSelect: noop, | ||
| label: createMessage(CONTEXT_COPY), | ||
| children: menuPages.map((page) => { | ||
| return { | ||
| ...page, | ||
| onSelect: () => copyActionToPage(props.id, props.name, page.id), | ||
| }; | ||
| }), | ||
| onSelect: | ||
| parentEntityType === ActionParentEntityType.PAGE | ||
| ? noop | ||
| : () => { | ||
| copyAction(props.id, props.name, parentEntityId); | ||
| }, | ||
| label: createMessage( | ||
| menuPages.length > 0 ? CONTEXT_COPY : CONTEXT_DUPLICATE, | ||
| ), | ||
| children: | ||
| parentEntityType === ActionParentEntityType.PAGE && | ||
| menuPages.length > 0 && | ||
| menuPages.map((page) => { | ||
| return { | ||
| ...page, | ||
| onSelect: () => copyAction(props.id, props.name, page.id), | ||
| }; | ||
| }), | ||
|
Comment on lines
+134
to
+153
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion Consider extracting copy action logic into separate functions. The copy action configuration is complex and would benefit from being broken down into smaller, more testable functions. Consider this refactoring: const getCopyActionLabel = (parentEntityType: ActionParentEntityType) =>
parentEntityType === ActionParentEntityType.PAGE
? createMessage(CONTEXT_COPY)
: createMessage(CONTEXT_DUPLICATE);
const getCopyActionHandler = (
parentEntityType: ActionParentEntityType,
id: string,
name: string,
parentEntityId: string,
) =>
parentEntityType === ActionParentEntityType.PAGE
? noop
: () => copyAction(id, name, parentEntityId);
const getCopyActionChildren = (
parentEntityType: ActionParentEntityType,
menuPages: Array<any>,
id: string,
name: string,
) =>
parentEntityType === ActionParentEntityType.PAGE && menuPages.length > 0
? menuPages.map((page) => ({
...page,
onSelect: () => copyAction(id, name, page.id),
}))
: undefined;
// In optionsTree:
{
value: "copy",
onSelect: getCopyActionHandler(parentEntityType, props.id, props.name, parentEntityId),
label: getCopyActionLabel(parentEntityType),
children: getCopyActionChildren(parentEntityType, menuPages, props.id, props.name),
} |
||
| }, | ||
| menuItems.includes(ActionEntityContextMenuItemsEnum.MOVE) && | ||
| canManageAction && { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -63,7 +63,7 @@ export function MoreActionsMenu(props: EntityContextMenuProps) { | |
| dispatch( | ||
| copyActionRequest({ | ||
| id: actionId, | ||
| destinationPageId: pageId, | ||
| destinationEditorId: pageId, | ||
| name: actionName, | ||
| }), | ||
|
Comment on lines
+66
to
68
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion Consider unifying the parameter naming between copy and move operations. The copy operation uses Consider updating the move operation to match: moveActionRequest({
id: actionId,
- destinationPageId,
+ destinationEntityId: destinationPageId,
originalPageId: propPageId ?? "",
name: actionName,
}),Also applies to: 73-80 |
||
| ), | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -134,7 +134,10 @@ import { sendAnalyticsEventSaga } from "./AnalyticsSaga"; | |
| import { EditorModes } from "components/editorComponents/CodeEditor/EditorConfig"; | ||
| import { updateActionAPICall } from "ee/sagas/ApiCallerSagas"; | ||
| import FocusRetention from "./FocusRetentionSaga"; | ||
| import { resolveParentEntityMetadata } from "ee/sagas/helpers"; | ||
| import { | ||
| generateDestinationIdInfoForQueryDuplication, | ||
| resolveParentEntityMetadata, | ||
| } from "ee/sagas/helpers"; | ||
| import { handleQueryEntityRedirect } from "./IDESaga"; | ||
| import { EditorViewMode, IDE_TYPE } from "ee/entities/IDE/constants"; | ||
| import { getIDETypeByUrl } from "ee/entities/IDE/utils"; | ||
|
|
@@ -144,7 +147,8 @@ import { | |
| } from "actions/ideActions"; | ||
| import { getIsSideBySideEnabled } from "selectors/ideSelectors"; | ||
| import { CreateNewActionKey } from "ee/entities/Engine/actionHelpers"; | ||
| import { convertToBasePageIdSelector } from "selectors/pageListSelectors"; | ||
| import { objectKeys } from "@appsmith/utils"; | ||
| import { convertToBaseParentEntityIdSelector } from "selectors/pageListSelectors"; | ||
|
|
||
| export const DEFAULT_PREFIX = { | ||
| QUERY: "Query", | ||
|
|
@@ -745,17 +749,34 @@ function* moveActionSaga( | |
| } | ||
|
|
||
| function* copyActionSaga( | ||
| action: ReduxAction<{ id: string; destinationPageId: string; name: string }>, | ||
| action: ReduxAction<{ | ||
| id: string; | ||
| destinationEditorId: string; | ||
| name: string; | ||
| }>, | ||
| ) { | ||
| let actionObject: Action = yield select(getAction, action.payload.id); | ||
|
|
||
| const { parentEntityId, parentEntityKey } = | ||
| resolveParentEntityMetadata(actionObject); | ||
|
|
||
| if (!parentEntityId || !parentEntityKey) return; | ||
|
|
||
|
Comment on lines
+758
to
+765
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Handle potential undefined In |
||
| const newName: string = yield select(getNewEntityName, { | ||
| prefix: action.payload.name, | ||
| parentEntityId: action.payload.destinationPageId, | ||
| parentEntityKey: CreateNewActionKey.PAGE, | ||
| parentEntityId, | ||
| parentEntityKey, | ||
| suffix: "Copy", | ||
| startWithoutIndex: true, | ||
| }); | ||
|
|
||
| const destinationEditorIdInfo = generateDestinationIdInfoForQueryDuplication( | ||
| action.payload.destinationEditorId, | ||
| parentEntityKey, | ||
| ); | ||
|
|
||
| if (objectKeys(destinationEditorIdInfo).length === 0) return; | ||
|
|
||
| try { | ||
| if (!actionObject) throw new Error("Could not find action to copy"); | ||
|
|
||
|
|
@@ -768,7 +789,7 @@ function* copyActionSaga( | |
|
|
||
| const copyAction = Object.assign({}, actionObject, { | ||
| name: newName, | ||
| pageId: action.payload.destinationPageId, | ||
| ...destinationEditorIdInfo, | ||
| }) as Partial<Action>; | ||
|
|
||
| // Indicates that source of action creation is copy action | ||
|
|
@@ -781,11 +802,15 @@ function* copyActionSaga( | |
| const datasources: Datasource[] = yield select(getDatasources); | ||
|
|
||
| const isValidResponse: boolean = yield validateResponse(response); | ||
| const pageName: string = yield select( | ||
| getPageNameByPageId, | ||
| // @ts-expect-error: pageId not present on ActionCreateUpdateResponse | ||
| response.data.pageId, | ||
| ); | ||
| let pageName: string = ""; | ||
|
|
||
| if (parentEntityKey === CreateNewActionKey.PAGE) { | ||
| pageName = yield select( | ||
| getPageNameByPageId, | ||
| // @ts-expect-error: pageId not present on ActionCreateUpdateResponse | ||
| response.data.pageId, | ||
| ); | ||
| } | ||
|
|
||
| if (isValidResponse) { | ||
| toast.show( | ||
|
|
@@ -807,6 +832,8 @@ function* copyActionSaga( | |
| AnalyticsUtil.logEvent("DUPLICATE_ACTION", { | ||
| // @ts-expect-error: name not present on ActionCreateUpdateResponse | ||
| actionName: response.data.name, | ||
| parentEntityId, | ||
| parentEntityKey, | ||
| pageName: pageName, | ||
| actionId: response.data.id, | ||
| originalActionId, | ||
|
|
@@ -836,7 +863,8 @@ function* copyActionSaga( | |
|
|
||
| yield put( | ||
| copyActionError({ | ||
| ...action.payload, | ||
| id: action.payload.id, | ||
| destinationEditorIdInfo, | ||
| show: true, | ||
| error: { | ||
| message: errorMessage, | ||
|
|
@@ -1039,21 +1067,23 @@ function* toggleActionExecuteOnLoadSaga( | |
| } | ||
|
|
||
| function* handleMoveOrCopySaga(actionPayload: ReduxAction<Action>) { | ||
| const { | ||
| baseId: baseActionId, | ||
| pageId, | ||
| pluginId, | ||
| pluginType, | ||
| } = actionPayload.payload; | ||
| const { baseId: baseActionId, pluginId, pluginType } = actionPayload.payload; | ||
| const isApi = pluginType === PluginType.API; | ||
| const isQuery = pluginType === PluginType.DB; | ||
| const isSaas = pluginType === PluginType.SAAS; | ||
| const basePageId: string = yield select(convertToBasePageIdSelector, pageId); | ||
| const { parentEntityId } = resolveParentEntityMetadata(actionPayload.payload); | ||
|
|
||
| if (!parentEntityId) return; | ||
|
|
||
|
Comment on lines
+1075
to
+1078
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ensure In |
||
| const baseParentEntityId: string = yield select( | ||
| convertToBaseParentEntityIdSelector, | ||
| parentEntityId, | ||
| ); | ||
|
Comment on lines
+1079
to
+1082
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Use You're selecting |
||
|
|
||
| if (isApi) { | ||
| history.push( | ||
| apiEditorIdURL({ | ||
| basePageId, | ||
| baseParentEntityId, | ||
| baseApiId: baseActionId, | ||
| }), | ||
| ); | ||
|
|
@@ -1062,7 +1092,7 @@ function* handleMoveOrCopySaga(actionPayload: ReduxAction<Action>) { | |
| if (isQuery) { | ||
| history.push( | ||
| queryEditorIdURL({ | ||
| basePageId, | ||
| baseParentEntityId, | ||
| baseQueryId: baseActionId, | ||
| }), | ||
| ); | ||
|
|
@@ -1076,7 +1106,7 @@ function* handleMoveOrCopySaga(actionPayload: ReduxAction<Action>) { | |
|
|
||
| history.push( | ||
| saasEditorApiIdURL({ | ||
| basePageId, | ||
| baseParentEntityId, | ||
| pluginPackageName: plugin.packageName, | ||
| baseApiId: baseActionId, | ||
| }), | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 Codebase verification
Interface implementation needs to be moved to EE directory
The interface is currently defined in CE but imported from EE (
import type { GenerateDestinationIdInfoReturnType } from "ee/sagas/helpers"). This mismatch needs to be addressed:app/client/src/ce/sagas/helpers.tsto the corresponding EE fileThe current setup could lead to import resolution issues and doesn't align with the codebase's CE/EE separation pattern.
🔗 Analysis chain
LGTM! Verify EE implementation compatibility.
The interface is well-defined and follows TypeScript best practices.
Let's verify the EE implementation:
The previous search in the
ee/directory failed because the directory might be named differently or located elsewhere. Let's search the entire codebase for any references or extensions of this interface.Let's check for EE implementations since the comment indicates this function is extended in EE. The previous search only found the CE implementation, so we need to look for actual extensions or usages.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
Length of output: 105
Script:
Length of output: 718
Script:
Length of output: 2599