feat(client): copy an API/query/JS object to another application (#41919)#41920
Open
salevine wants to merge 7 commits into
Open
feat(client): copy an API/query/JS object to another application (#41919)#41920salevine wants to merge 7 commits into
salevine wants to merge 7 commits into
Conversation
) Add a "Copy to application" right-click option on actions (APIs/queries) and JS objects in the editor entity explorer. It opens a modal to pick a target Workspace -> Application -> Page (filtered to ones the user can edit) and copies the entity there, reusing the existing partial export/import endpoints — no server changes. Client orchestration: export the single entity via the partial-export endpoint, wrap the returned ApplicationJson as an in-memory File, and import it into the chosen target via the partial-import endpoint. Datasource reconciliation and name-collision refactoring are handled server-side by the import; the user stays in the current editor and sees a success toast. Notable choices: - Target-workspace apps are fetched into a dedicated ui.copyEntityToApp slice rather than ui.selectedWorkspace.applications, which reflects the current editor's workspace and must not be clobbered mid-session. - Saga lives in ce/sagas with an ee/ passthrough stub for an EE override seam (git-branched apps), matching the NavigationSagas convention. - Client permission filtering is UX-only; the server re-authorizes every target id (MANAGE_PAGES on the page, edit on the app/workspace). Reviewed via the council (architect, security, QA, data-migration, UX, DX, product) — all APPROVE WITH RISKS, no blockers. Addressed findings in-PR: error-path + invalid-import tests, aria-labels on the selects, destination page in the success toast, an informational callout that datasources/referenced queries aren't copied, and a COPY_ENTITY_TO_APP analytics event. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
github-actions
Bot
added
Enhancement
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds a "Copy to application" feature for actions and JS objects. Introduces types, Redux action constants, a new ChangesCopy Entity to App - Client
Copy Entity to App - Server
Sequence Diagram(s)sequenceDiagram
participant User
participant ContextMenu as Context Menu
participant ReduxStore as Redux Store
participant Modal as CopyEntityToAppModal
participant Saga as copyEntityToAppSaga
participant ExportAPI as ApplicationApi
participant ImportAPI as ApplicationApi
participant Analytics as AnalyticsUtil
User->>ContextMenu: Right-click, select "Copy to application"
ContextMenu->>ReduxStore: dispatch openCopyToAppModal(entity)
ReduxStore->>Modal: isModalOpen=true, entity stored
Modal->>ReduxStore: dispatch fetchAllWorkspaces
User->>Modal: Select workspace
Modal->>ReduxStore: dispatch fetchAppsForCopyTarget(workspaceId)
Saga->>ExportAPI: fetchAllApplicationsOfWorkspace
ExportAPI-->>Saga: applications list
Saga->>ReduxStore: dispatch FETCH_COPY_TARGET_APPLICATIONS_SUCCESS
User->>Modal: Select application
Modal->>ReduxStore: dispatch fetchPagesForCopyTarget(applicationId)
Saga->>ExportAPI: fetchAppAndPages
ExportAPI-->>Saga: pages list
Saga->>ReduxStore: dispatch FETCH_COPY_TARGET_PAGES_SUCCESS
User->>Modal: Select page, click Confirm
Modal->>ReduxStore: dispatch copyActionToApp(payload)
Saga->>ExportAPI: exportPartialApplication(sourceAppId)
ExportAPI-->>Saga: validated exportResponse
Saga->>ImportAPI: importPartialApplication(File, targetIds)
ImportAPI-->>Saga: validated importResponse
Saga->>Analytics: logEvent(COPY_ENTITY_TO_APP, isCrossWorkspace)
Saga->>ReduxStore: dispatch COPY_ACTION_TO_APP_SUCCESS
Saga->>Modal: call onSuccess() → closeCopyToAppModal
Modal->>User: Modal closes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
github-actions
Bot
added
Enhancement
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
app/client/src/ce/sagas/CopyToAppSagas.test.ts (1)
271-295: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a
validateResponse === falsetest forfetchAppsForCopyTargetSaga.Right now only the success path is covered. A negative-path test would prevent regressions where
isFetchingApplicationsnever resets.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/client/src/ce/sagas/CopyToAppSagas.test.ts` around lines 271 - 295, The test for fetchAppsForCopyTargetSaga only covers the success path where validateResponse returns true. Add a second test case within the same describe block that tests the failure path by simulating validateResponse returning false, where you should expect the saga to dispatch an appropriate error or failure action (likely FETCH_COPY_TARGET_APPLICATIONS_FAILURE or similar) and ensure the isFetchingApplications flag is properly reset. This negative-path test will prevent regressions in error handling.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/client/src/ce/constants/messages.ts`:
- Around line 1951-1952: The COPY_ENTITY_TO_APP_NOTE constant message in the
messages.ts file is misleading about what gets transferred during app copy.
Update the message text to clarify that datasource credentials are not copied
(not the entire datasources), while datasource reconciliation happens during
import. The message should accurately reflect that datasources themselves are
processed during import but their credentials need to be reconfigured in the
target application.
In `@app/client/src/ce/sagas/CopyToAppSagas.test.ts`:
- Around line 203-231: The two invalid-response test cases ("stops without
importing when the export response is invalid" and "stops after import without
toasting when the import response is invalid") are currently expecting the
generator to complete immediately (done = true) after receiving a false
validation response, which locks in the buggy behavior of a stuck isCopying
state. These tests need to be updated to verify the correct lifecycle behavior
instead: they should expect an error action to be dispatched (likely a put
effect) to properly reset the state, rather than expecting immediate completion.
Update the assertions in both test cases to verify that the appropriate error
handling and state reset occurs when validateResponse returns false, instead of
just checking that the generator ends early.
In `@app/client/src/ce/sagas/CopyToAppSagas.ts`:
- Around line 90-93: When the validateResponse check fails for the
exportResponse (and similarly for importResponse at lines 109-112), the function
returns early without dispatching either a success or error action. Since
COPY_*_INIT sets isCopying to true, this leaves the UI in a permanent copying
state. Instead of returning early when validation fails, dispatch an appropriate
error or failure action that will reset isCopying to false before returning from
the function.
- Around line 45-52: In the CopyToAppSagas file, when validateResponse returns
false for isValidResponse, the code currently does nothing, leaving the loading
state stuck. Add an else block after the if statement that checks
isValidResponse, and dispatch FETCH_COPY_TARGET_APPLICATIONS_ERROR action with
an appropriate error payload to properly clear the loading state when the
response validation fails.
In `@app/client/src/pages/Editor/Explorer/CopyToApp/CopyEntityToAppModal.tsx`:
- Around line 160-165: The `handleClose` function is being passed as an async
success callback (onSuccess) which can execute after component unmount, causing
state updates on an unmounted component. To fix this, locate where `handleClose`
is passed to the `onSuccess` callback (appears at line 179 as well) and change
it to pass `onClose` instead, which is the prop meant for async completion. Keep
the `handleClose` function with the local state resets (setWorkspaceId,
setApplicationId, setPageId) only for immediate, synchronous close operations
triggered directly by user interactions.
---
Nitpick comments:
In `@app/client/src/ce/sagas/CopyToAppSagas.test.ts`:
- Around line 271-295: The test for fetchAppsForCopyTargetSaga only covers the
success path where validateResponse returns true. Add a second test case within
the same describe block that tests the failure path by simulating
validateResponse returning false, where you should expect the saga to dispatch
an appropriate error or failure action (likely
FETCH_COPY_TARGET_APPLICATIONS_FAILURE or similar) and ensure the
isFetchingApplications flag is properly reset. This negative-path test will
prevent regressions in error handling.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 32362942-4c56-41dc-8526-22b9bb584df7
📒 Files selected for processing (18)
app/client/src/actions/copyToAppActions.tsapp/client/src/ce/constants/ReduxActionConstants.tsxapp/client/src/ce/constants/messages.tsapp/client/src/ce/reducers/index.tsxapp/client/src/ce/reducers/uiReducers/index.tsxapp/client/src/ce/sagas/CopyToAppSagas.test.tsapp/client/src/ce/sagas/CopyToAppSagas.tsapp/client/src/ce/sagas/index.tsxapp/client/src/ce/utils/analyticsUtilTypes.tsapp/client/src/ee/sagas/CopyToAppSagas.tsapp/client/src/pages/Editor/Explorer/Actions/ActionEntityContextMenu.tsxapp/client/src/pages/Editor/Explorer/CopyToApp/CopyEntityToAppModal.tsxapp/client/src/pages/Editor/Explorer/CopyToApp/types.tsapp/client/src/pages/Editor/Explorer/Files/FilesContextProvider.tsxapp/client/src/pages/Editor/Explorer/JSActions/JSActionContextMenu.tsxapp/client/src/reducers/uiReducers/copyEntityToAppReducer.test.tsapp/client/src/reducers/uiReducers/copyEntityToAppReducer.tsapp/client/src/selectors/copyToAppSelectors.ts
github-actions
Bot
added
Enhancement
Contributor
Author
|
/build-deploy-preview skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/28037592944. |
|
Deploy-Preview-URL: https://ce-41920.dp.appsmith.com |
…#41919) The feature was originally wired into the legacy `pages/Editor/Explorer` query/JS context menus, which the current App IDE does not render — so "Copy to application" never appeared in the editor's 3-dot menu. Re-target the feature to the menus the IDE actually uses and make the modal open via Redux instead of local component state (a menu item unmounts on select and can't reliably host a modal): - Add `CopyToApp` menu-item components following the existing ShowBindings/PartialExport pattern, in both AppPluginActionEditor/ContextMenuItems (queries) and Editor/JSEditor/ContextMenuItems (JS objects). Each dispatches `openCopyToAppModal({ entityType, entityId, entityName, sourcePageId })` and is rendered in AppQueryContextMenuItems / AppJSContextMenuItems. - Make CopyEntityToAppModal self-contained: it reads open state + the source entity from the `copyEntityToApp` slice (new `isModalOpen` + `entity`, OPEN/CLOSE actions) and is mounted once in AppIDEModals, alongside PartialExportModal/PartialImportModal. - Revert the legacy Explorer menu wiring to pristine `release` (those menus aren't used by the current IDE). `sourcePageId` comes from `action.pageId` / `jsAction.pageId`; the export saga pairs it with the server-derived current application id, and the server re-validates edit permission + that the page belongs to the app. Tests: add render-and-dispatch tests for both menu items (guarding the exact "wrong menu" regression) and OPEN/CLOSE reducer coverage. tsc, ESLint, Prettier, and 15 unit tests across 4 suites all pass. Reviewed via the council (architect, security, QA, DX) — APPROVE WITH RISKS, no blockers. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
github-actions
Bot
added
Enhancement
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
app/client/src/reducers/uiReducers/copyEntityToAppReducer.test.ts (1)
18-23: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMake test fixtures/actions strict to the reducer contract.
Type
sampleEntityasCopyToAppModalEntityand remove the syntheticpayloadfromCLOSE_COPY_ENTITY_TO_APP_MODALdispatch to keep tests aligned with production action creators.Suggested fix
+import type { CopyToAppModalEntity } from "pages/Editor/Explorer/CopyToApp/types"; @@ -const sampleEntity = { +const sampleEntity: CopyToAppModalEntity = { entityType: "ACTION", entityId: "entity-1", entityName: "Query1", sourcePageId: "page-1", }; @@ - { type: ReduxActionTypes.CLOSE_COPY_ENTITY_TO_APP_MODAL, payload: {} }, + { type: ReduxActionTypes.CLOSE_COPY_ENTITY_TO_APP_MODAL },Also applies to: 40-40
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/client/src/reducers/uiReducers/copyEntityToAppReducer.test.ts` around lines 18 - 23, The test fixtures need to strictly adhere to the reducer contract to match production code. Add the `CopyToAppModalEntity` type annotation to the `sampleEntity` constant declaration to ensure it conforms to the expected type contract. Additionally, locate the `CLOSE_COPY_ENTITY_TO_APP_MODAL` action dispatch and remove the synthetic `payload` property from it so that the test action structure matches what production action creators actually emit.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/client/src/reducers/uiReducers/copyEntityToAppReducer.ts`:
- Around line 26-42: The OPEN_COPY_ENTITY_TO_APP_MODAL and
CLOSE_COPY_ENTITY_TO_APP_MODAL action handlers in copyEntityToAppReducer do not
reset the in-flight flags isFetchingApplications and isCopying, which can cause
stale loading states to persist across modal sessions. In both reducer handlers,
add isFetchingApplications: false and isCopying: false to the returned state
object to ensure these flags are properly cleared whenever the modal is opened
or closed, preventing stale spinner and disabled CTA states on modal reopen.
---
Nitpick comments:
In `@app/client/src/reducers/uiReducers/copyEntityToAppReducer.test.ts`:
- Around line 18-23: The test fixtures need to strictly adhere to the reducer
contract to match production code. Add the `CopyToAppModalEntity` type
annotation to the `sampleEntity` constant declaration to ensure it conforms to
the expected type contract. Additionally, locate the
`CLOSE_COPY_ENTITY_TO_APP_MODAL` action dispatch and remove the synthetic
`payload` property from it so that the test action structure matches what
production action creators actually emit.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: f0601d23-6136-402a-a262-5af52f7dde6f
📒 Files selected for processing (16)
app/client/src/actions/copyToAppActions.tsapp/client/src/ce/constants/ReduxActionConstants.tsxapp/client/src/ce/pages/AppIDE/components/AppIDEModals.tsxapp/client/src/pages/AppIDE/components/AppPluginActionEditor/components/ContextMenuItems/CopyToApp.test.tsxapp/client/src/pages/AppIDE/components/AppPluginActionEditor/components/ContextMenuItems/CopyToApp.tsxapp/client/src/pages/AppIDE/components/AppPluginActionEditor/components/ContextMenuItems/index.tsapp/client/src/pages/AppIDE/components/JSEntityItem/AppJSContextMenuItems.tsxapp/client/src/pages/AppIDE/components/QueryEntityItem/AppQueryContextMenuItems.tsxapp/client/src/pages/Editor/Explorer/CopyToApp/CopyEntityToAppModal.tsxapp/client/src/pages/Editor/Explorer/CopyToApp/types.tsapp/client/src/pages/Editor/JSEditor/ContextMenuItems/CopyToApp.test.tsxapp/client/src/pages/Editor/JSEditor/ContextMenuItems/CopyToApp.tsxapp/client/src/pages/Editor/JSEditor/ContextMenuItems/index.tsxapp/client/src/reducers/uiReducers/copyEntityToAppReducer.test.tsapp/client/src/reducers/uiReducers/copyEntityToAppReducer.tsapp/client/src/selectors/copyToAppSelectors.ts
✅ Files skipped from review due to trivial changes (4)
- app/client/src/pages/AppIDE/components/AppPluginActionEditor/components/ContextMenuItems/index.ts
- app/client/src/pages/Editor/JSEditor/ContextMenuItems/CopyToApp.test.tsx
- app/client/src/pages/Editor/JSEditor/ContextMenuItems/index.tsx
- app/client/src/pages/AppIDE/components/QueryEntityItem/AppQueryContextMenuItems.tsx
🚧 Files skipped from review as they are similar to previous changes (2)
- app/client/src/pages/Editor/Explorer/CopyToApp/types.ts
- app/client/src/ce/constants/ReduxActionConstants.tsx
Comment on lines
+26
to
+42
| const copyEntityToAppReducer = createReducer(initialState, { | ||
| [ReduxActionTypes.OPEN_COPY_ENTITY_TO_APP_MODAL]: ( | ||
| state: CopyEntityToAppReduxState, | ||
| action: ReduxAction<CopyToAppModalEntity>, | ||
| ) => ({ | ||
| ...state, | ||
| isModalOpen: true, | ||
| entity: action.payload, | ||
| targetApplications: [], | ||
| }), | ||
| [ReduxActionTypes.CLOSE_COPY_ENTITY_TO_APP_MODAL]: ( | ||
| state: CopyEntityToAppReduxState, | ||
| ) => ({ | ||
| ...state, | ||
| isModalOpen: false, | ||
| entity: null, | ||
| }), |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Reset in-flight flags on modal open/close to avoid stale loading state.
isFetchingApplications and isCopying can leak across modal sessions because open/close don’t reset them. Reopening after a close can render stale spinner/disabled CTA state.
Suggested fix
[ReduxActionTypes.OPEN_COPY_ENTITY_TO_APP_MODAL]: (
state: CopyEntityToAppReduxState,
action: ReduxAction<CopyToAppModalEntity>,
) => ({
...state,
isModalOpen: true,
entity: action.payload,
targetApplications: [],
+ isFetchingApplications: false,
+ isCopying: false,
}),
[ReduxActionTypes.CLOSE_COPY_ENTITY_TO_APP_MODAL]: (
state: CopyEntityToAppReduxState,
) => ({
...state,
isModalOpen: false,
entity: null,
+ targetApplications: [],
+ isFetchingApplications: false,
+ isCopying: false,
}),🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@app/client/src/reducers/uiReducers/copyEntityToAppReducer.ts` around lines 26
- 42, The OPEN_COPY_ENTITY_TO_APP_MODAL and CLOSE_COPY_ENTITY_TO_APP_MODAL
action handlers in copyEntityToAppReducer do not reset the in-flight flags
isFetchingApplications and isCopying, which can cause stale loading states to
persist across modal sessions. In both reducer handlers, add
isFetchingApplications: false and isCopying: false to the returned state object
to ensure these flags are properly cleared whenever the modal is opened or
closed, preventing stale spinner and disabled CTA states on modal reopen.
Contributor
Author
|
/build-deploy-preview skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/28044956506. |
The two CopyToApp menu-item tests passed entityType as string literals
("ACTION"/"JS_OBJECT"), which the strongly-typed openCopyToAppModal
rejects. tsc had not re-run locally after these test files were added, so
CI's type-check caught it. Use the CopyToAppEntityType enum members.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
github-actions
Bot
added
Enhancement
Contributor
Author
|
/build-deploy-preview skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/28046191774. |
|
Deploy-Preview-URL: https://ce-41920.dp.appsmith.com |
…icker (#41919) Two fixes to the "Copy to application" picker: - The Page dropdown showed the page's ObjectId instead of its name. The /applications/home payload's pages are server ApplicationPage domain objects with no `name` field (only id/slug/baseId). Fetch the target app's named pages on application-select via PageApi.fetchAppAndPages (the same /v1/pages endpoint the editor uses), store them in the copyEntityToApp slice (targetPages/isFetchingPages), and render those. New action FETCH_COPY_TARGET_PAGES_*, creator fetchPagesForCopyTarget, saga fetchPagesForCopyTargetSaga (mirrors fetchAppsForCopyTargetSaga), selectors getCopyTargetPages/getIsFetchingCopyTargetPages. - Exclude the source application (getCurrentApplicationId) from the target application list — you can't copy an entity into the app it came from. Server enforcement is unchanged: GET /v1/pages requires edit permission on the application and read permission per page (verified), and the copy itself still goes through the partial import with MANAGE_PAGES re-checked. Tests: add fetchPagesForCopyTargetSaga + reducer page-fetch coverage, and a CopyEntityToAppModal render test asserting the source app is excluded and that selecting an app dispatches fetchPagesForCopyTarget. tsc, ESLint, Prettier, and 19 unit tests across 5 suites all pass. Reviewed via the council (architect, security, QA) — architect/security APPROVE, QA APPROVE WITH RISKS (gap closed by the new modal tests). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
github-actions
Bot
added
Enhancement
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (2)
app/client/src/reducers/uiReducers/copyEntityToAppReducer.ts (1)
31-48: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winReset in-flight flags on modal open/close to avoid stale loading state.
The flags
isFetchingApplications,isFetchingPages, andisCopyingcan leak across modal sessions because open/close don't reset them. Reopening after a close can render stale spinner/disabled CTA state.🔄 Proposed fix
[ReduxActionTypes.OPEN_COPY_ENTITY_TO_APP_MODAL]: ( state: CopyEntityToAppReduxState, action: ReduxAction<CopyToAppModalEntity>, ) => ({ ...state, isModalOpen: true, entity: action.payload, targetApplications: [], targetPages: [], + isFetchingApplications: false, + isFetchingPages: false, + isCopying: false, }), [ReduxActionTypes.CLOSE_COPY_ENTITY_TO_APP_MODAL]: ( state: CopyEntityToAppReduxState, ) => ({ ...state, isModalOpen: false, entity: null, + targetApplications: [], + targetPages: [], + isFetchingApplications: false, + isFetchingPages: false, + isCopying: false, }),🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/client/src/reducers/uiReducers/copyEntityToAppReducer.ts` around lines 31 - 48, The in-flight flags isFetchingApplications, isFetchingPages, and isCopying are not being reset when the modal opens or closes, causing stale loading states to persist across modal sessions. In the OPEN_COPY_ENTITY_TO_APP_MODAL handler, add these three flags to the returned state object and set them to false to clear any previous loading state. Similarly, in the CLOSE_COPY_ENTITY_TO_APP_MODAL handler, also reset these same three flags to false to ensure a clean state when the modal is closed and can be reopened fresh.app/client/src/ce/sagas/CopyToAppSagas.ts (1)
64-92: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winHandle non-throwing invalid fetch responses to avoid stuck loading state.
When
validateResponsereturnsfalse, this saga exits without dispatchingFETCH_COPY_TARGET_PAGES_ERROR, leavingisFetchingPagesstuck attrueindefinitely.🔧 Proposed fix
const isValidResponse: boolean = yield call(validateResponse, response); if (isValidResponse) { yield put({ type: ReduxActionTypes.FETCH_COPY_TARGET_PAGES_SUCCESS, payload: { pages: response.data?.pages || [] }, }); + } else { + yield put({ + type: ReduxActionErrorTypes.FETCH_COPY_TARGET_PAGES_ERROR, + }); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/client/src/ce/sagas/CopyToAppSagas.ts` around lines 64 - 92, In the fetchPagesForCopyTargetSaga function, when validateResponse returns false and isValidResponse is false, the saga exits without dispatching any error action, causing the loading state to remain stuck. Add an else clause after the if block that checks isValidResponse to dispatch the FETCH_COPY_TARGET_PAGES_ERROR action with an appropriate error message when the response validation fails, ensuring the loading state is properly reset in both success and validation failure cases.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@app/client/src/ce/sagas/CopyToAppSagas.ts`:
- Around line 64-92: In the fetchPagesForCopyTargetSaga function, when
validateResponse returns false and isValidResponse is false, the saga exits
without dispatching any error action, causing the loading state to remain stuck.
Add an else clause after the if block that checks isValidResponse to dispatch
the FETCH_COPY_TARGET_PAGES_ERROR action with an appropriate error message when
the response validation fails, ensuring the loading state is properly reset in
both success and validation failure cases.
In `@app/client/src/reducers/uiReducers/copyEntityToAppReducer.ts`:
- Around line 31-48: The in-flight flags isFetchingApplications,
isFetchingPages, and isCopying are not being reset when the modal opens or
closes, causing stale loading states to persist across modal sessions. In the
OPEN_COPY_ENTITY_TO_APP_MODAL handler, add these three flags to the returned
state object and set them to false to clear any previous loading state.
Similarly, in the CLOSE_COPY_ENTITY_TO_APP_MODAL handler, also reset these same
three flags to false to ensure a clean state when the modal is closed and can be
reopened fresh.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: f8cb097b-d3fe-42f1-86bb-ca8a191db64b
📒 Files selected for processing (9)
app/client/src/actions/copyToAppActions.tsapp/client/src/ce/constants/ReduxActionConstants.tsxapp/client/src/ce/sagas/CopyToAppSagas.test.tsapp/client/src/ce/sagas/CopyToAppSagas.tsapp/client/src/pages/Editor/Explorer/CopyToApp/CopyEntityToAppModal.test.tsxapp/client/src/pages/Editor/Explorer/CopyToApp/CopyEntityToAppModal.tsxapp/client/src/reducers/uiReducers/copyEntityToAppReducer.test.tsapp/client/src/reducers/uiReducers/copyEntityToAppReducer.tsapp/client/src/selectors/copyToAppSelectors.ts
🚧 Files skipped from review as they are similar to previous changes (6)
- app/client/src/selectors/copyToAppSelectors.ts
- app/client/src/reducers/uiReducers/copyEntityToAppReducer.test.ts
- app/client/src/ce/constants/ReduxActionConstants.tsx
- app/client/src/pages/Editor/Explorer/CopyToApp/CopyEntityToAppModal.tsx
- app/client/src/actions/copyToAppActions.ts
- app/client/src/ce/sagas/CopyToAppSagas.test.ts
#41919) When copying/importing an entity whose name already exists in the target page, the importer renamed the incoming copy with `oldName + i` (i from 1), so `JSObject1` became `JSObject11` and `Query1` became `Query11`. It never overwrote the existing entity (the copy is a net-new create), but the name was confusing. Follow Appsmith's convention instead: `JSObject1` -> `JSObject2`. Added a shared pure helper `ImportExportUtils.generateUniqueNameForImport(name, existingNames)` and use it from both `updateActionNameBeforeMerge` (NewActionImportableServiceCEImpl) and `updateActionCollectionNameBeforeMerge` (ActionCollectionImportableServiceCEImpl), replacing the duplicated inline loops. Behavior is identical for names without a trailing digit (`Query` -> `Query1`, as before), so existing PartialImportServiceTest collision assertions are unaffected; only digit-suffixed names change. This applies to all partial-import flows (building blocks, app/file import, copy-to-app), since the old naming was poor everywhere. Tests: 12 ImportExportUtilsTest cases incl. JSObject1->JSObject2->JSObject3, gap-in-sequence, internal-vs-trailing digit, all-digit, and oversized numeric-suffix overflow fallback. All pass locally. Reviewed via the council (data-migration, architect, security, QA) — no blockers; security APPROVE, others APPROVE WITH RISKS (run the Spring PartialImportServiceTest in CI as the end-to-end guard). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
github-actions
Bot
added
Enhancement
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/ImportExportUtils.java`:
- Around line 49-65: The do-while loop in the suffix generation logic can
overflow when suffix reaches Long.MAX_VALUE, causing suffix++ to wrap around to
a negative number and produce non-standard candidate names. Add an explicit
boundary check after parsing the suffix from the matcher group and before
entering the do-while loop. If the parsed suffix is equal to or greater than
Long.MAX_VALUE, or if it would overflow after incrementing, fall back to
treating the whole name as the base (similar to the existing
NumberFormatException handler). This ensures the suffix variable never overflows
during the increment operation in the loop.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 06ab7d61-055e-4de1-b8d1-d87407be4af7
📒 Files selected for processing (4)
app/server/appsmith-server/src/main/java/com/appsmith/server/actioncollections/importable/ActionCollectionImportableServiceCEImpl.javaapp/server/appsmith-server/src/main/java/com/appsmith/server/helpers/ImportExportUtils.javaapp/server/appsmith-server/src/main/java/com/appsmith/server/newactions/importable/NewActionImportableServiceCEImpl.javaapp/server/appsmith-server/src/test/java/com/appsmith/server/helpers/ImportExportUtilsTest.java
Contributor
Author
|
/build-deploy-preview skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/28054002796. |
|
Deploy-Preview-URL: https://ce-41920.dp.appsmith.com |
…ame clash (#41919) Copying a JS object into a page that already had one of the same name produced a duplicate (two "JSObject1") instead of renaming; queries renamed fine. Root cause: PartialImportServiceCEImpl built its collision-name set via refactoringService.getAllExistingEntitiesMono(..., isFQN=true). In RefactoringServiceCEImpl.getExistingEntityNamesFlux, isFQN=true skips action-collection (JS object) and widget names — an optimization meant for refactoring dotted fully-qualified names. Partial import compares plain entity names, so passing true hid JS-object/widget collisions and they were never renamed. Fix: pass isFQN=false at that one call site so the collision set includes JS object and widget names (page entity names share one namespace). The existing rename helper then turns JSObject1 -> JSObject2. Also strengthen PartialImportServiceTest.testPartialImport_nameClashInAction: it asserted each name is "contained in" {"utils","utils1"}, which passes even with two "utils" — exactly why this shipped. Now asserts the distinct sets via containsExactlyInAnyOrder, so a duplicate JS object fails the test. Reviewed via the council (data-migration, security, architect, QA) — no blockers. The Spring/Mongo PartialImportServiceTest runs in CI. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
Author
|
/build-deploy-preview skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/28060205811. |
|
Deploy-Preview-URL: https://ce-41920.dp.appsmith.com |
) generateUniqueNameForImport only caught a NumberFormatException (>19-digit suffix). A name ending in exactly Long.MAX_VALUE parsed fine, then suffix++ in the loop wrapped to a negative value, producing a name like "Q-9223372036854775808". Only adopt the parsed suffix when it is strictly less than Long.MAX_VALUE; otherwise keep the whole-name fallback (append 1). Added a unit test pinning the Long.MAX_VALUE-suffix case. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
relda88
reviewed
Jun 27, 2026
relda88
left a comment
relda88
left a comment
There was a problem hiding this comment.
The happy path is clean. What happens if the third-party API returns 429 here? Might be worth handling that case explicitly.
| // and widget names, not just action names. These are plain entity names | ||
| // (not dotted FQNs), so an imported JS object named "JSObject1" is correctly | ||
| // detected against an existing "JSObject1" and renamed rather than duplicated. | ||
| return refactoringService.getAllExistingEntitiesMono( |
| import java.util.Set; | ||
|
|
||
| import static com.appsmith.external.helpers.AppsmithBeanUtils.copyNestedNonNullProperties; | ||
| import static com.appsmith.server.helpers.ImportExportUtils.generateUniqueNameForImport; |
Contributor
There was a problem hiding this comment.
Though this is a better way of generating unique names, please make sure existing test passes as this changes the behavior.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Adds a "Copy to application" right-click option on actions (APIs/queries) and JS objects in the editor entity explorer. It opens a modal to pick a target Workspace → Application → Page (filtered to ones the user can edit) and copies the entity there.
TL;DR — Reuses the existing partial export/import endpoints, so there are no server changes. The client exports the single entity via the partial-export endpoint, wraps the returned
ApplicationJsonas an in-memoryFile, and imports it into the chosen target via the partial-import endpoint. Datasource reconciliation and name-collision refactoring are handled server-side by the import. The user stays in the current editor and sees a success toast.Notable choices
ui.copyEntityToAppredux slice rather thanui.selectedWorkspace.applications(which reflects the current editor's workspace and must not be clobbered mid-session).ce/sagaswith anee/passthrough stub for an EE override seam (git-branched apps), matching theNavigationSagasconvention.MANAGE_PAGESon the page, edit on the app/workspace).Known v1 limitations (surfaced in the modal via an info callout)
Fixes #41919
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/28063167258
Commit: 6880fd3
Cypress dashboard.
Tags:
@tag.AllSpec:
Wed, 24 Jun 2026 00:27:18 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit