From 3421f8bfd1e4dd1eb93c4b75af7ab29b9415231e Mon Sep 17 00:00:00 2001 From: Rahul Barwal Date: Thu, 3 Jul 2025 12:24:53 +0530 Subject: [PATCH] feat: implements onPageUnload functionality for the edit app mode page selector (#41074) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Description Problem onPageUnload functionality was not consistently triggered during all types of page navigation in edit mode, leading to potential missed cleanup or actions when navigating between pages via different UI elements or programmatic flows. Root cause Navigation logic was fragmented across multiple components and methods (button clicks, navigation tabs, page navigator), and direct history manipulation bypassed centralized handling, preventing reliable invocation of onPageUnload actions. Solution This PR handles the integration of onPageUnload functionality with all page navigation flows in edit mode by centralizing navigation logic through the navigateToAnotherPage action, enhancing type safety, and ensuring onPageUnload actions are filtered and executed based on the current page context. Fixes #40998 _or_ Fixes `Issue URL` > [!WARNING] > _If no issue exists, please create an issue first, and check with the maintainers if the issue is valid._ ## Automation /ok-to-test tags="@tag.JS, @tag.Sanity" ### :mag: Cypress test results > [!TIP] > 🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉 > Workflow run: > Commit: 8ea04e6bb1312d9f468ed3d74ccc080ed6e9bac9 > Cypress dashboard. > Tags: `@tag.JS, @tag.Sanity` > Spec: >
Thu, 03 Jul 2025 06:44:33 UTC ## Communication Should the DevRel and Marketing teams inform users about this change? - [ ] Yes - [ ] No ## Summary by CodeRabbit * **New Features** * Enhanced page unload actions to only trigger for the current page, improving accuracy and reliability. * **Bug Fixes** * Improved navigation consistency by updating the page switching mechanism to use a unified action. * **Tests** * Added tests to ensure correct filtering of JavaScript actions executed on page unload. * **Refactor** * Streamlined selector logic for better maintainability and performance. --- app/client/src/actions/pageActions.tsx | 3 +- .../AppIDE/components/PageList/PageEntity.tsx | 7 +- .../sagas/ActionExecution/PluginActionSaga.ts | 10 ++- .../pluginActionSelectors.test.ts | 71 +++++++++++++++++++ app/client/src/selectors/editorSelectors.tsx | 18 ++--- 5 files changed, 92 insertions(+), 17 deletions(-) create mode 100644 app/client/src/selectors/__tests__/editorSelectors/pluginActionSelectors.test.ts diff --git a/app/client/src/actions/pageActions.tsx b/app/client/src/actions/pageActions.tsx index 47aa7f95dd..f776940728 100644 --- a/app/client/src/actions/pageActions.tsx +++ b/app/client/src/actions/pageActions.tsx @@ -31,6 +31,7 @@ import type { ApiResponse } from "api/ApiResponses"; import type { EvaluationReduxAction } from "./EvaluationReduxActionTypes"; import { appsmithTelemetry } from "instrumentation"; import type { NavigateToAnotherPagePayload } from "sagas/ActionExecution/NavigateActionSaga/types"; +import type { Path } from "history"; export interface FetchPageListPayload { applicationId: string; @@ -699,7 +700,7 @@ export const setupPublishedPage = ( }); export const navigateToAnotherPage = ( - payload: NavigateToAnotherPagePayload, + payload: NavigateToAnotherPagePayload | Path, ) => ({ type: ReduxActionTypes.NAVIGATE_TO_ANOTHER_PAGE, payload, diff --git a/app/client/src/pages/AppIDE/components/PageList/PageEntity.tsx b/app/client/src/pages/AppIDE/components/PageList/PageEntity.tsx index fb71d77ffc..2d0ac0ab02 100644 --- a/app/client/src/pages/AppIDE/components/PageList/PageEntity.tsx +++ b/app/client/src/pages/AppIDE/components/PageList/PageEntity.tsx @@ -15,11 +15,10 @@ import { import { PERMISSION_TYPE, isPermitted } from "ee/utils/permissionHelpers"; import { getCurrentApplication } from "ee/selectors/applicationSelectors"; import type { DefaultRootState } from "react-redux"; -import { updatePageAction } from "actions/pageActions"; +import { navigateToAnotherPage, updatePageAction } from "actions/pageActions"; import { useGetPageFocusUrl } from "./hooks/useGetPageFocusUrl"; import AnalyticsUtil from "ee/utils/AnalyticsUtil"; import { toggleInOnboardingWidgetSelection } from "actions/onboardingActions"; -import history, { NavigationMethod } from "utils/history"; import { EntityItem } from "@appsmith/ads"; import { useNameEditorState } from "IDE/hooks/useNameEditorState"; import { useValidateEntityName } from "IDE"; @@ -90,9 +89,7 @@ export const PageEntity = ({ toUrl: navigateToUrl, }); dispatch(toggleInOnboardingWidgetSelection(true)); - history.push(navigateToUrl, { - invokedBy: NavigationMethod.EntityExplorer, - }); + dispatch(navigateToAnotherPage(navigateToUrl)); if (onClick) { onClick(); diff --git a/app/client/src/sagas/ActionExecution/PluginActionSaga.ts b/app/client/src/sagas/ActionExecution/PluginActionSaga.ts index b9856cf37b..9d748bb7bc 100644 --- a/app/client/src/sagas/ActionExecution/PluginActionSaga.ts +++ b/app/client/src/sagas/ActionExecution/PluginActionSaga.ts @@ -1702,14 +1702,18 @@ export function* executePageUnloadActionsSaga() { const span = startRootSpan("executePageUnloadActionsSaga"); try { - const pageActions: Action[] = yield select(getLayoutOnUnloadActions); - const actionCount = pageActions.length; + const pageOnUnloadActions: Action[] = yield select( + getLayoutOnUnloadActions, + ); + const actionCount = pageOnUnloadActions.length; setAttributesToSpan(span, { numActions: actionCount }); // Execute unload actions in parallel batches yield all( - pageActions.map((action) => call(executeOnPageUnloadJSAction, action)), + pageOnUnloadActions.map((action) => + call(executeOnPageUnloadJSAction, action), + ), ); // Publish success event after all actions are executed diff --git a/app/client/src/selectors/__tests__/editorSelectors/pluginActionSelectors.test.ts b/app/client/src/selectors/__tests__/editorSelectors/pluginActionSelectors.test.ts new file mode 100644 index 0000000000..31ddc869a9 --- /dev/null +++ b/app/client/src/selectors/__tests__/editorSelectors/pluginActionSelectors.test.ts @@ -0,0 +1,71 @@ +import type { DefaultRootState } from "react-redux"; +import { getLayoutOnUnloadActions } from "selectors/editorSelectors"; + +describe("getLayoutOnUnloadActions", () => { + it("should filter actions by current page ID", () => { + const state = { + entities: { + pageList: { currentPageId: "page1" }, + jsActions: [ + { + isLoading: false, + config: { + id: "collection1", + pageId: "page1", + actions: [ + { + id: "action1", + pageId: "page1", + runBehaviour: "ON_PAGE_UNLOAD", + name: "myFun1", + fullyQualifiedName: "JSObject1.myFun1", + collectionId: "collection1", + }, + { + id: "action3", + pageId: "page1", + runBehaviour: "ON_PAGE_LOAD", + name: "myFun2", + fullyQualifiedName: "JSObject1.myFun2", + collectionId: "collection1", + }, + ], + }, + }, + { + isLoading: false, + config: { + id: "collection2", + pageId: "page2", + actions: [ + { + id: "action2", + pageId: "page2", + runBehaviour: "ON_PAGE_UNLOAD", + name: "myFun1", + fullyQualifiedName: "JSObject2.myFun1", + collectionId: "collection2", + }, + ], + }, + }, + ], + }, + }; + + const result = getLayoutOnUnloadActions( + state as unknown as DefaultRootState, + ); + + expect(result).toEqual([ + { + id: "action1", + pageId: "page1", + runBehaviour: "ON_PAGE_UNLOAD", + name: "myFun1", + fullyQualifiedName: "JSObject1.myFun1", + collectionId: "collection1", + }, + ]); + }); +}); diff --git a/app/client/src/selectors/editorSelectors.tsx b/app/client/src/selectors/editorSelectors.tsx index b220b6ab77..2861488720 100644 --- a/app/client/src/selectors/editorSelectors.tsx +++ b/app/client/src/selectors/editorSelectors.tsx @@ -125,16 +125,21 @@ export const getPageSavingError = (state: DefaultRootState) => { return state.ui.editor.loadingStates.savingError; }; +export const getCurrentPageId = (state: DefaultRootState) => + state.entities.pageList.currentPageId; + export const getLayoutOnLoadActions = (state: DefaultRootState) => state.ui.editor.pageActions || []; export const getLayoutOnUnloadActions = createSelector( + getCurrentPageId, getAllJSCollectionActions, - (jsActions) => { - return jsActions.filter((action) => { - return action.runBehaviour === ActionRunBehaviour.ON_PAGE_UNLOAD; - }); - }, + (currentPageId, jsActions) => + jsActions.filter( + (action) => + action.runBehaviour === ActionRunBehaviour.ON_PAGE_UNLOAD && + action.pageId === currentPageId, + ), ); export const getLayoutOnLoadIssues = (state: DefaultRootState) => { @@ -167,9 +172,6 @@ export const getPageByBaseId = (basePageId: string) => pages.find((page) => page.basePageId === basePageId), ); -export const getCurrentPageId = (state: DefaultRootState) => - state.entities.pageList.currentPageId; - export const getCurrentBasePageId = (state: DefaultRootState) => state.entities.pageList.currentBasePageId;