chore: refactor query duplication flow #36915
Conversation
github-actions
bot
added
the
skip-changelog
|
This PR has increased the number of cyclic dependencies by 1, when compared with the release branch. Refer this document to identify the cyclic dependencies introduced by this PR. |
github-actions
bot
added
Enhancement
| import { useCallback } from "react"; | ||
| import { useDispatch } from "react-redux"; | ||
|
|
||
| function useHandleDuplicateClick() { |
There was a problem hiding this comment.
@ayushpahwa any unit test case coverage needed here?
There was a problem hiding this comment.
This is just a hook which is passing on the action as per the input. I have added test for the rendering of the action menu which was the actual change and also covers the functionality of this hook. This test can be enhanced to introduce coverage of other options also but since they are already covered in cypress, I have just left a TODO comment for now for the same.
WalkthroughThe changes introduce a new custom React hook, Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
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: 8
🧹 Outside diff range and nitpick comments (11)
app/client/src/PluginActionEditor/hooks/index.ts (1)
Line range hint
1-7: Consider grouping exports by sourceThe exports could be organized by grouping EE imports together and local imports separately for better maintainability.
+// Enterprise Edition hooks export { useActionSettingsConfig } from "ee/PluginActionEditor/hooks/useActionSettingsConfig"; export { useHandleDeleteClick } from "ee/PluginActionEditor/hooks/useHandleDeleteClick"; -export { useHandleDuplicateClick } from "./useHandleDuplicateClick"; export { useHandleRunClick } from "ee/PluginActionEditor/hooks/useHandleRunClick"; export { useBlockExecution } from "ee/PluginActionEditor/hooks/useBlockExecution"; export { useAnalyticsOnRunClick } from "ee/PluginActionEditor/hooks/useAnalyticsOnRunClick"; + +// Community Edition hooks +export { useHandleDuplicateClick } from "./useHandleDuplicateClick";app/client/src/PluginActionEditor/hooks/useHandleDuplicateClick.ts (1)
6-24: Consider adding TypeScript types for better type safety.While the implementation is clean and follows React best practices, consider adding explicit types:
-function useHandleDuplicateClick() { +interface DuplicateClickHookResult { + handleDuplicateClick: (destinationEntityId: string) => void; +} + +function useHandleDuplicateClick(): DuplicateClickHookResult {app/client/src/ce/pages/Editor/AppPluginActionEditor/components/ToolbarMenu/Copy.tsx (1)
Line range hint
16-25: Consider adding type safety to the action payload.While the implementation is correct, consider creating a dedicated type for the action payload to improve maintainability.
interface CopyActionPayload { id: string; destinationEntityId: string; name: string; }app/client/src/ce/sagas/helpers.ts (1)
18-28: Consider adding parameter validation.The implementation is clean and follows the single responsibility principle. However, consider adding validation for the
destinationEntityIdparameter to prevent potential issues with empty or invalid IDs.export function generateDestinationIdInfoForQueryDuplication( destinationEntityId: string, parentEntityKey: CreateNewActionKeyInterface, ): GenerateDestinationIdInfoReturnType { + if (!destinationEntityId?.trim()) { + throw new Error("destinationEntityId cannot be empty"); + } if (parentEntityKey === CreateNewActionKey.PAGE) { return { pageId: destinationEntityId }; }app/client/src/pages/Editor/Explorer/Actions/ActionEntityContextMenu.test.tsx (3)
1-28: Consider organizing imports by CE/EE separation.The imports mix paths from both CE and EE. Consider grouping them separately for better maintainability.
// CE imports import React from "react"; import { fireEvent, render, waitFor } from "@testing-library/react"; import "@testing-library/jest-dom/extend-expect"; import { Provider } from "react-redux"; import { lightTheme } from "selectors/themeSelectors"; import { ThemeProvider } from "styled-components"; import configureStore from "redux-mock-store"; import { PluginType } from "entities/Action"; // EE imports import { ActionEntityContextMenu, type EntityContextMenuProps, } from "./ActionEntityContextMenu"; import { FilesContextProvider } from "../Files/FilesContextProvider"; import { ActionParentEntityType } from "ee/entities/Engine/actionHelpers"; // ... rest of EE imports
31-33: Extract page IDs as named constants.Consider moving these IDs to a constants object at the top of the file for better maintainability.
+const PAGE_IDS = { + PAGE_1: "605c435a91dea93f0eaf91ba", + PAGE_2: "605c435a91dea93f0eaf91bc", + BASE_PAGE_2: "605c435a91dea93f0eaf91bc", +} as const; -const page1Id = "605c435a91dea93f0eaf91ba"; -const page2Id = "605c435a91dea93f0eaf91bc"; -const basePage2Id = "605c435a91dea93f0eaf91bc";
147-156: Simplify type definition using type inference.The explicit type definition for the actions array can be simplified by letting TypeScript infer the type from store.getActions().
- let actions: Array< - ReduxAction<{ - payload: { - id: string; - destinationEntityId: string; - name: string; - }; - }> - > = []; + let actions = [] as ReturnType<typeof store.getActions>;app/client/src/pages/Editor/Explorer/Actions/MoreActionsMenu.tsx (1)
Line range hint
153-154: Consider implementing the suggested UX improvement.The existing comment suggests moving the menu visibility logic outside. This would indeed improve UX by preventing users from seeing empty or irrelevant menu options.
Would you like me to propose an implementation for this improvement?
app/client/src/pages/Editor/Explorer/Actions/ActionEntityContextMenu.tsx (1)
50-51: Consider adding type validation for parentEntityType.While the context destructuring looks good, consider adding runtime type validation for
parentEntityTypeto ensure it matchesActionParentEntityType.const { menuItems, parentEntityId, parentEntityType } = context; + if (!Object.values(ActionParentEntityType).includes(parentEntityType)) { + console.warn(`Invalid parentEntityType: ${parentEntityType}`); + }app/client/src/sagas/ActionSagas.ts (2)
774-780: Add logging for emptydestinationEntityIdInfoCurrently, if
destinationEntityIdInfois empty, the saga returns without any notification. Consider adding a log or error message to inform about this scenario, which could aid in debugging.
867-870: IncludedestinationEntityIdInfoin error payloadWhen dispatching
copyActionError, consider includingdestinationEntityIdInfoin the error payload for better context and easier debugging.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (10)
- app/client/src/PluginActionEditor/hooks/index.ts (1 hunks)
- app/client/src/PluginActionEditor/hooks/useHandleDuplicateClick.ts (1 hunks)
- app/client/src/actions/pluginActionActions.ts (3 hunks)
- app/client/src/ce/constants/messages.ts (2 hunks)
- app/client/src/ce/pages/Editor/AppPluginActionEditor/components/ToolbarMenu/Copy.tsx (1 hunks)
- app/client/src/ce/sagas/helpers.ts (1 hunks)
- app/client/src/pages/Editor/Explorer/Actions/ActionEntityContextMenu.test.tsx (1 hunks)
- app/client/src/pages/Editor/Explorer/Actions/ActionEntityContextMenu.tsx (4 hunks)
- app/client/src/pages/Editor/Explorer/Actions/MoreActionsMenu.tsx (1 hunks)
- app/client/src/sagas/ActionSagas.ts (10 hunks)
🧰 Additional context used
🔇 Additional comments (12)
app/client/src/PluginActionEditor/hooks/index.ts (1)
3-3: Verify architectural decision: Local vs EE import patternThe new hook is imported from a local path while other hooks are imported from the EE directory. This might indicate an architectural change moving functionality from EE to CE.
Let's verify the import patterns in the codebase:
✅ Verification successful
Let me verify the architectural pattern further by checking the CE implementation and imports:
This will help us understand where the new hook is implemented and how it's being used across the codebase.
Local hook implementation follows correct architectural pattern
The import pattern is consistent with the codebase architecture:
- Other hooks in EE directory (
useHandleDeleteClick,useHandleRunClick, etc.) are re-exports from CE implementations- The new
useHandleDuplicateClickhook follows the same pattern by being implemented directly in CE- The hook is properly scoped within the PluginActionEditor module
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check import patterns for hooks in PluginActionEditor rg -g '*.{ts,tsx}' -l "from.*PluginActionEditor/hooks" | xargs rg "from.*PluginActionEditor/hooks" # Check if there are other local hook imports rg -g '*.{ts,tsx}' --glob '!*.test.*' -l "from.*./.*hook" ./app/client/src/PluginActionEditor/Length of output: 3345
Script:
#!/bin/bash # Check CE implementation of hooks fd "useHandleDuplicateClick.ts" app/client/src/ rg -g '*.{ts,tsx}' "from.*useHandleDuplicateClick" app/client/src/Length of output: 301
app/client/src/PluginActionEditor/hooks/useHandleDuplicateClick.ts (2)
1-5: LGTM! Clean and focused imports.The imports are well-organized and include all necessary dependencies for the hook's functionality.
6-24: Add unit tests for the hook.While the functionality is covered by Cypress tests, adding unit tests would improve maintainability and catch potential issues earlier in the development cycle.
Let's check if there are any existing test files for hooks:
Would you like me to help create a unit test file for this hook?
✅ Verification successful
Let me gather more information about the testing setup in this codebase to provide a more accurate verification.
The first script will help us understand if there are test files at the component level. The second script will show us how hooks are typically tested in this codebase, and the third will reveal any test utilities that might be helpful for testing hooks. This will give us a better picture of the testing practices in the project.
Let me check the existing hook test to understand the testing pattern used in this codebase.
This will help us understand:
- How hooks are currently tested in this codebase
- What test setup is available
- How Redux-related functionality is typically tested
Add unit tests following the established pattern
Based on the existing hook test patterns in the codebase, create a test file at
app/client/src/PluginActionEditor/hooks/useHandleDuplicateClick.test.tsxwith the following test cases:
- Mock dependencies (
useDispatch,usePluginActionContext)- Test successful dispatch of
copyActionRequestwith correct parameters- Test when action context is not available
- Verify memoization by checking if callback changes when dependencies change
Example test structure:
import { renderHook } from "@testing-library/react-hooks/dom"; import { useDispatch } from "react-redux"; import { usePluginActionContext } from "../PluginActionContext"; import { copyActionRequest } from "../store"; import { useHandleDuplicateClick } from "./useHandleDuplicateClick"; jest.mock("react-redux"); jest.mock("../PluginActionContext"); jest.mock("../store");🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for existing hook test files to maintain consistency fd -e test.ts -e test.tsx . app/client/src/PluginActionEditor/hooks/Length of output: 70
Script:
#!/bin/bash # Check for test files in the parent directories to understand the testing patterns fd -e test.ts -e test.tsx . app/client/src/PluginActionEditor/ # Look for similar hook test patterns in the codebase fd -e test.ts -e test.tsx . app/client/src | rg -l "hook|Hook" # Check if there are any test utilities or setup files fd -e ts -e tsx . app/client/src | rg -l "test.*setup|test.*utils"Length of output: 744
Script:
#!/bin/bash # Check the content of the existing hook test to understand the testing pattern cat app/client/src/PluginActionEditor/components/PluginActionForm/hooks/useChangeActionCall.test.tsx # Look for test setup files in the project root fd -e ts -e tsx -e js . | rg -l "jest.setup|test.setup" # Check for any Redux-related test utilities since the hook uses dispatch rg -l "redux.*test|test.*redux" app/client/src/Length of output: 6667
app/client/src/pages/Editor/Explorer/Actions/MoreActionsMenu.tsx (1)
66-68: LGTM: Parameter rename aligns with refactoring goals.The change from
destinationPageIdtodestinationEntityIdin the copy action payload aligns with the PR's objective to refactor the query duplication flow.app/client/src/pages/Editor/Explorer/Actions/ActionEntityContextMenu.tsx (2)
23-23: LGTM! Clean interface and import changes.The exported interface and new imports align well with the refactoring objectives for query duplication.
Also applies to: 37-37, 39-47
Line range hint
58-67: LGTM! Clean action handler refactoring.The transition from page-specific to entity-based copying is well implemented. The useCallback dependency array is correctly maintained.
app/client/src/actions/pluginActionActions.ts (3)
24-24: LGTM: Import for new type from EE module.The import aligns with the PR objective of refactoring the duplication flow.
248-248: LGTM: Updated error payload structure.The change to use
destinationEntityIdInfowith the new type from EE maintains consistency with the refactoring approach.
229-231: LGTM: Updated payload type for action duplication.The change from
destinationPageIdtodestinationEntityIdaligns with the refactoring goal of eliminating page ID dependency.Let's verify the consistency of this change across the codebase:
app/client/src/ce/constants/messages.ts (2)
340-341: LGTM! Improved message flexibilityThe conditional message formatting allows for better reusability when pageName is empty.
1734-1734: LGTM! Follows constant naming conventionThe new CONTEXT_DUPLICATE constant follows the established pattern for context action messages.
app/client/src/sagas/ActionSagas.ts (1)
806-814: Verifyresponse.data.pageIdexistenceIn the
ifblock, you're accessingresponse.data.pageId, but there's a comment indicating it might not be present. Ensure thatpageIdexists onresponse.datato prevent potential runtime errors.
| @@ -17,7 +17,7 @@ export const Copy = () => { | |||
| dispatch( | |||
| copyActionRequest({ | |||
| id: action.id, | |||
| destinationPageId: pageId, | |||
| destinationEntityId: pageId, | |||
There was a problem hiding this comment.
💡 Codebase verification
Parameter renaming needs to be completed across the codebase
The verification reveals inconsistent parameter naming. While destinationEntityId is used in the new copy functionality, there are still multiple occurrences of destinationPageId across the codebase that need to be updated for consistency:
-
Frontend files still using
destinationPageId:app/client/src/sagas/ActionSagas.tsapp/client/src/pages/Editor/Explorer/Actions/ActionEntityContextMenu.tsxapp/client/src/pages/Editor/Explorer/Actions/MoreActionsMenu.tsxapp/client/src/ce/sagas/JSActionSagas.tsapp/client/src/api/ActionAPI.tsx
-
Backend files also using
destinationPageId:app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/LayoutCollectionServiceCEImpl.javaapp/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/LayoutActionServiceCEImpl.java
Please update all occurrences to maintain consistency with the new destinationEntityId parameter name.
🔗 Analysis chain
Verify consistent parameter naming across the codebase.
The change from destinationPageId to destinationEntityId looks good and aligns with the refactoring goals.
Let's verify the consistent usage across the codebase:
The previous commands failed due to unsupported file type flags. Let's try without them and get a broader context of the changes. The new commands will help us:
- Find any remaining instances of the old parameter name
- Verify the usage of the new parameter name
- Understand the implementation context of the copyActionRequest
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining instances of destinationPageId
rg "destinationPageId" --type ts --type tsx
# Search for new parameter usage
rg "destinationEntityId.*copyActionRequest" --type ts --type tsx
Length of output: 166
Script:
#!/bin/bash
# Search for any remaining instances of destinationPageId without file type flags
rg "destinationPageId"
# Search for new parameter usage
rg "destinationEntityId.*copyActionRequest"
# Let's also check for the action handler implementation
rg "copyActionRequest" -A 5
Length of output: 9829
| // This function is extended in EE. Please check the EE implementation before any modification. | ||
| export interface GenerateDestinationIdInfoReturnType { | ||
| pageId?: string; | ||
| } |
There was a problem hiding this comment.
💡 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:
- Move the interface definition from
app/client/src/ce/sagas/helpers.tsto the corresponding EE file - Update imports to reference the correct EE location
The 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:
#!/bin/bash
# Check for any EE-specific extensions of this interface
rg -A 5 "interface GenerateDestinationIdInfoReturnType" "ee/"
Length of output: 105
Script:
#!/bin/bash
# Let's try searching in the entire codebase for EE-specific extensions
rg -g '!node_modules' -g '!dist' -A 5 "interface GenerateDestinationIdInfoReturnType"
# Also search for any files that might contain EE implementations
fd -g '*.ts' -g '*.js' -E 'node_modules' -E 'dist' -x rg -l "GenerateDestinationIdInfoReturnType"
Length of output: 718
Script:
#!/bin/bash
# Let's search for any EE-specific implementations or extensions
rg -g '!node_modules' -g '!dist' -A 10 "generateDestinationIdInfoForQueryDuplication" --type ts
# Also check for any imports of this interface
rg -g '!node_modules' -g '!dist' "import.*GenerateDestinationIdInfoReturnType" --type ts
Length of output: 2599
| it("renders context menu with correct options for application editor", async () => { | ||
| store = mockStore(defaultState); | ||
| const { findByText, getByRole } = render( | ||
| <Provider store={store}> | ||
| <ThemeProvider theme={lightTheme}> | ||
| <FilesContextProvider {...defaultContext}> | ||
| <ActionEntityContextMenu {...defaultProps} /> | ||
| </FilesContextProvider> | ||
| </ThemeProvider> | ||
| </Provider>, | ||
| ); | ||
| const triggerToOpenMenu = getByRole("button"); | ||
|
|
||
| act(() => { | ||
| fireEvent.click(triggerToOpenMenu); | ||
| }); | ||
|
|
||
| await waitFor(() => { | ||
| expect(triggerToOpenMenu.parentNode).toHaveAttribute( | ||
| "aria-expanded", | ||
| "true", | ||
| ); | ||
| }); | ||
|
|
||
| // In context of pages, the copy to page option will be shown | ||
| const copyQueryToPageOptions = await findByText( | ||
| createMessage(CONTEXT_COPY), | ||
| ); | ||
|
|
||
| expect(await findByText(createMessage(CONTEXT_RENAME))).toBeInTheDocument(); | ||
| expect(await findByText(createMessage(CONTEXT_DELETE))).toBeInTheDocument(); | ||
| expect(await findByText(createMessage(CONTEXT_MOVE))).toBeInTheDocument(); | ||
| expect( | ||
| await findByText(createMessage(CONTEXT_SHOW_BINDING)), | ||
| ).toBeInTheDocument(); | ||
| expect(copyQueryToPageOptions).toBeInTheDocument(); | ||
|
|
||
| // Now we click on the copy to page option | ||
| act(() => { | ||
| fireEvent.click(copyQueryToPageOptions); | ||
| }); | ||
|
|
||
| // Now a menu with the list of pages will show up | ||
| const copyQueryToPageSubOptionPage1 = await findByText("Page1"); | ||
|
|
||
| expect(copyQueryToPageSubOptionPage1).toBeInTheDocument(); | ||
| expect(await findByText("Page2")).toBeInTheDocument(); | ||
|
|
||
| // Clicking on the page will trigger the correct action | ||
| act(() => { | ||
| fireEvent.click(copyQueryToPageSubOptionPage1); | ||
| }); | ||
|
|
||
| let actions: Array< | ||
| ReduxAction<{ | ||
| payload: { | ||
| id: string; | ||
| destinationEntityId: string; | ||
| name: string; | ||
| }; | ||
| }> | ||
| > = []; | ||
|
|
||
| await waitFor(() => { | ||
| actions = store.getActions(); | ||
| }); | ||
|
|
||
| expect(actions.length).toBe(1); | ||
| expect(actions[0].type).toBe(ReduxActionTypes.COPY_ACTION_INIT); | ||
| expect(actions[0].payload).toEqual({ | ||
| destinationEntityId: page1Id, | ||
| id: "test-action-id", | ||
| name: "test-action", | ||
| }); | ||
| }); | ||
| // TODO: add tests for all options rendered in the context menu |
There was a problem hiding this comment.
Expand test coverage for all context menu actions.
The current test only verifies the copy action. Consider adding test cases for:
- Delete action
- Rename action
- Move action
- Show binding functionality
- Permission-based rendering (when canManageAction/canDeleteAction are false)
Would you like me to help generate the additional test cases?
| destinationEntityId: pageId, | ||
| name: actionName, | ||
| }), |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider unifying the parameter naming between copy and move operations.
The copy operation uses destinationEntityId while the move operation still uses destinationPageId. This inconsistency might cause confusion.
Consider updating the move operation to match:
moveActionRequest({
id: actionId,
- destinationPageId,
+ destinationEntityId: destinationPageId,
originalPageId: propPageId ?? "",
name: actionName,
}),Also applies to: 73-80
| onSelect: | ||
| parentEntityType === ActionParentEntityType.PAGE | ||
| ? noop | ||
| : () => { | ||
| copyAction(props.id, props.name, parentEntityId); | ||
| }, | ||
| label: createMessage( | ||
| parentEntityType === ActionParentEntityType.PAGE | ||
| ? CONTEXT_COPY | ||
| : CONTEXT_DUPLICATE, | ||
| ), | ||
| children: | ||
| parentEntityType === ActionParentEntityType.PAGE && | ||
| menuPages.length > 0 && | ||
| menuPages.map((page) => { | ||
| return { | ||
| ...page, | ||
| onSelect: () => copyAction(props.id, props.name, page.id), | ||
| }; | ||
| }), |
There was a problem hiding this comment.
🛠️ 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),
}| const { destinationEntityId, id, name } = action.payload; | ||
| let actionObject: Action = yield select(getAction, id); | ||
|
|
||
| const { parentEntityId, parentEntityKey } = | ||
| resolveParentEntityMetadata(actionObject); | ||
|
|
||
| if (!parentEntityId || !parentEntityKey) return; | ||
|
|
There was a problem hiding this comment.
Handle potential undefined parentEntityId or parentEntityKey
In copyActionSaga, if parentEntityId or parentEntityKey are undefined, the function returns early. Ensure that this behavior is expected and that any necessary error handling or user feedback is in place.
| const { parentEntityId } = resolveParentEntityMetadata(actionPayload.payload); | ||
|
|
||
| if (!parentEntityId) return; | ||
|
|
There was a problem hiding this comment.
Ensure parentEntityId is defined before proceeding
In handleMoveOrCopySaga, if parentEntityId is undefined, the function returns early. Verify that this is the intended behavior and handle any necessary error messaging or fallback logic.
| const baseParentEntityId: string = yield select( | ||
| convertToBaseParentEntityIdSelector, | ||
| parentEntityId, | ||
| ); |
There was a problem hiding this comment.
Use baseParentEntityId safely
You're selecting baseParentEntityId using parentEntityId. Since parentEntityId might be undefined (as checked earlier), ensure that baseParentEntityId is valid before using it to avoid potential errors.
## Description This PR refactors the action duplication saga and action calls to remove dependency on pageID. As far as CE code is concerned, this PR doesn't change any functionality for the end user. Those changes are done for workflows editor in EE. Fixes appsmithorg#36886 ## Automation /test all ### 🔍 Cypress test results <!-- This is an auto-generated comment: Cypress test results --> > [!TIP] > 🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉 > Workflow run: <https://github.com/appsmithorg/appsmith/actions/runs/11462274307> > Commit: 04dfbfb > <a href="https://internal.appsmith.com/app/cypress-dashboard/rundetails-65890b3c81d7400d08fa9ee5?branch=master&workflowId=11462274307&attempt=2" target="_blank">Cypress dashboard</a>. > Tags: `@tag.All` > Spec: > <hr>Tue, 22 Oct 2024 16:21:32 UTC <!-- end of auto-generated comment: Cypress test results --> ## Communication Should the DevRel and Marketing teams inform users about this change? - [ ] Yes - [x] No <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **New Features** - Introduced a new hook, `useHandleDuplicateClick`, enhancing action duplication functionality. - Added a new interface and function for generating destination ID information. - **Bug Fixes** - Updated action request structure to use `destinationEntityId` for consistency across components. - **Documentation** - Enhanced success message flexibility for action copy notifications. - **Tests** - Added unit tests for the `ActionEntityContextMenu` component to ensure proper functionality and rendering. - **Refactor** - Improved context menu handling based on entity types for better user experience. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
Description
This PR refactors the action duplication saga and action calls to remove dependency on pageID. As far as CE code is concerned, this PR doesn't change any functionality for the end user. Those changes are done for workflows editor in EE.
Fixes #36886
Automation
/test all
🔍 Cypress test results
Tip
🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/11462274307
Commit: 04dfbfb
Cypress dashboard.
Tags:
@tag.AllSpec:
Tue, 22 Oct 2024 16:21:32 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
New Features
useHandleDuplicateClick, enhancing action duplication functionality.Bug Fixes
destinationEntityIdfor consistency across components.Documentation
Tests
ActionEntityContextMenucomponent to ensure proper functionality and rendering.Refactor