Skip to content
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

fix: remove redundant eval trigger #36433

Open
wants to merge 6 commits into
base: release
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 3 additions & 6 deletions app/client/src/actions/pageActions.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -68,25 +68,23 @@ export const fetchPageAction = (
export interface FetchPublishedPageActionPayload {
pageId: string;
bustCache?: boolean;
firstLoad?: boolean;
pageWithMigratedDsl?: FetchPageResponse;
}

export interface FetchPublishedPageResourcesPayload {
pageId: string;
applicationId: string;
}

export const fetchPublishedPageAction = (
pageId: string,
bustCache = false,
firstLoad = false,
pageWithMigratedDsl?: FetchPageResponse,
): ReduxAction<FetchPublishedPageActionPayload> => ({
type: ReduxActionTypes.FETCH_PUBLISHED_PAGE_INIT,
payload: {
pageId,
bustCache,
firstLoad,
pageWithMigratedDsl,
},
});
Expand Down Expand Up @@ -301,10 +299,12 @@ export const clonePageSuccess = ({
// In future we can reuse this for fetching other page level resources in published mode
export const fetchPublishedPageResourcesAction = (
pageId: string,
applicationId: string,
): ReduxAction<FetchPublishedPageResourcesPayload> => ({
type: ReduxActionTypes.FETCH_PUBLISHED_PAGE_RESOURCES_INIT,
payload: {
pageId,
applicationId,
},
});

Expand Down Expand Up @@ -675,21 +675,18 @@ export const setupPageAction = (
export interface SetupPublishedPageActionPayload {
pageId: string;
bustCache: boolean;
firstLoad: boolean;
pageWithMigratedDsl?: FetchPageResponse;
}

export const setupPublishedPage = (
pageId: string,
bustCache = false,
firstLoad = false,
pageWithMigratedDsl?: FetchPageResponse,
): ReduxAction<SetupPublishedPageActionPayload> => ({
type: ReduxActionTypes.SETUP_PUBLISHED_PAGE_INIT,
payload: {
pageId,
bustCache,
firstLoad,
pageWithMigratedDsl,
},
});
98 changes: 49 additions & 49 deletions app/client/src/ce/sagas/PageSagas.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -325,12 +325,47 @@ export function* fetchPageSaga(action: ReduxAction<FetchPageActionPayload>) {
}
}

export function* updateCanvasLayout(response: FetchPageResponse) {
// Wait for widget config to load before we can get the canvas payload
yield call(waitForWidgetConfigBuild);
// Get Canvas payload
const canvasWidgetsPayload = getCanvasWidgetsPayload(response);

// resize main canvas
resizePublishedMainCanvasToLowestWidget(canvasWidgetsPayload.widgets);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remember to use 'yield' when calling functions inside generator functions

In the updateCanvasLayout generator function, the call to resizePublishedMainCanvasToLowestWidget should be yielded to properly handle side effects within the generator.

Apply this diff to fix the issue:

-  resizePublishedMainCanvasToLowestWidget(canvasWidgetsPayload.widgets);
+  yield call(resizePublishedMainCanvasToLowestWidget, canvasWidgetsPayload.widgets);
Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
resizePublishedMainCanvasToLowestWidget(canvasWidgetsPayload.widgets);
yield call(resizePublishedMainCanvasToLowestWidget, canvasWidgetsPayload.widgets);

// Update the canvas
yield put(initCanvasLayout(canvasWidgetsPayload));

// Since new page has new layout, we need to generate a data structure
// to compute dynamic height based on the new layout.
yield put(generateAutoHeightLayoutTreeAction(true, true));
}

export function* postFetchedPublishedPage(
response: FetchPageResponse,
pageId: string,
) {
// Clear any existing caches
yield call(clearEvalCache);
// Set url params
yield call(setDataUrl);

yield call(updateCanvasLayout, response);
// set current page
yield put(
updateCurrentPage(
pageId,
response.data.slug,
response.data.userPermissions,
),
);
}

export function* fetchPublishedPageSaga(
action: ReduxAction<FetchPublishedPageActionPayload>,
) {
try {
const { bustCache, firstLoad, pageId, pageWithMigratedDsl } =
action.payload;
const { bustCache, pageId, pageWithMigratedDsl } = action.payload;

const params = { pageId, bustCache };
const response: FetchPageResponse = yield call(
Expand All @@ -342,41 +377,9 @@ export function* fetchPublishedPageSaga(
const isValidResponse: boolean = yield validateResponse(response);

if (isValidResponse) {
// Clear any existing caches
yield call(clearEvalCache);
// Set url params
yield call(setDataUrl);
// Wait for widget config to load before we can get the canvas payload
yield call(waitForWidgetConfigBuild);
// Get Canvas payload
const canvasWidgetsPayload = getCanvasWidgetsPayload(response);

// resize main canvas
resizePublishedMainCanvasToLowestWidget(canvasWidgetsPayload.widgets);
// Update the canvas
yield put(initCanvasLayout(canvasWidgetsPayload));
// set current page
yield put(
updateCurrentPage(
pageId,
response.data.slug,
response.data.userPermissions,
),
);
yield call(postFetchedPublishedPage, response, pageId);

// dispatch fetch page success
yield put(fetchPublishedPageSuccess());

// Since new page has new layout, we need to generate a data structure
// to compute dynamic height based on the new layout.
yield put(generateAutoHeightLayoutTreeAction(true, true));

/* Currently, All Actions are fetched in initSagas and on pageSwitch we only fetch page
*/
// Hence, if is not isFirstLoad then trigger evaluation with execute pageLoad action
if (!firstLoad) {
yield put(fetchAllPageEntityCompletion([executePageLoadActions()]));
}
}
} catch (error) {
yield put({
Expand All @@ -392,7 +395,7 @@ export function* fetchPublishedPageResourcesSaga(
action: ReduxAction<FetchPublishedPageResourcesPayload>,
) {
try {
const { pageId } = action.payload;
const { applicationId, pageId } = action.payload;

const params = { defaultPageId: pageId };
const initConsolidatedApiResponse: ApiResponse<InitConsolidatedApi> =
Expand All @@ -410,9 +413,14 @@ export function* fetchPublishedPageResourcesSaga(
// In future, we can reuse this saga to fetch other resources of the page like actionCollections etc
const { publishedActions } = response;

// Sending applicationId as empty as we have publishedActions present,
// it won't call the actions view api with applicationId
yield put(fetchActionsForView({ applicationId: "", publishedActions }));
yield call(
postFetchedPublishedPage,
response.pageWithMigratedDsl,
pageId,
);

// NOTE: fetchActionsForView is used here to update publishedActions in redux store and not to fetch actions again
yield put(fetchActionsForView({ applicationId, publishedActions }));
yield put(fetchAllPageEntityCompletion([executePageLoadActions()]));
}
} catch (error) {
Expand Down Expand Up @@ -1425,21 +1433,13 @@ export function* setupPublishedPageSaga(
action: ReduxAction<SetupPublishedPageActionPayload>,
) {
try {
const { bustCache, firstLoad, pageId, pageWithMigratedDsl } =
action.payload;
const { bustCache, pageId, pageWithMigratedDsl } = action.payload;

/*
Added the first line for isPageSwitching redux state to be true when page is being fetched to fix scroll position issue.
Added the second line for sync call instead of async (due to first line) as it was leading to issue with on page load actions trigger.
*/
yield put(
fetchPublishedPageAction(
pageId,
bustCache,
firstLoad,
pageWithMigratedDsl,
),
);
yield put(fetchPublishedPageAction(pageId, bustCache, pageWithMigratedDsl));
rishabhrathod01 marked this conversation as resolved.
Show resolved Hide resolved
yield take(ReduxActionTypes.FETCH_PUBLISHED_PAGE_SUCCESS);

yield put({
Expand Down
2 changes: 0 additions & 2 deletions app/client/src/ce/sagas/__tests__/PageSaga.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,6 @@ describe("ce/PageSaga", () => {
pageWithMigratedDsl: mockResponse.data
.pageWithMigratedDsl as FetchPageResponse,
bustCache: false,
firstLoad: true,
},
};

Expand All @@ -57,7 +56,6 @@ describe("ce/PageSaga", () => {
fetchPublishedPageAction(
action.payload.pageId,
action.payload.bustCache,
action.payload.firstLoad,
action.payload.pageWithMigratedDsl,
),
)
Expand Down
2 changes: 1 addition & 1 deletion app/client/src/entities/Engine/AppViewerEngine.ts
Original file line number Diff line number Diff line change
Expand Up @@ -105,7 +105,7 @@ export default class AppViewerEngine extends AppEngine {
}),
fetchSelectedAppThemeAction(applicationId, currentTheme),
fetchAppThemesAction(applicationId, themes),
setupPublishedPage(toLoadPageId, true, true, pageWithMigratedDsl),
setupPublishedPage(toLoadPageId, true, pageWithMigratedDsl),
];

const successActionEffects = [
Expand Down
11 changes: 4 additions & 7 deletions app/client/src/pages/AppViewer/index.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -28,10 +28,7 @@ import { useSelector } from "react-redux";
import BrandingBadge from "./BrandingBadge";
import { setAppViewHeaderHeight } from "actions/appViewActions";
import { CANVAS_SELECTOR } from "constants/WidgetConstants";
import {
setupPublishedPage,
fetchPublishedPageResourcesAction,
} from "actions/pageActions";
import { fetchPublishedPageResourcesAction } from "actions/pageActions";
import usePrevious from "utils/hooks/usePrevious";
import { getIsBranchUpdated } from "../utils";
import { APP_MODE } from "entities/App";
Expand Down Expand Up @@ -165,10 +162,10 @@ function AppViewer(props: Props) {
)?.pageId;

if (pageId) {
dispatch(setupPublishedPage(pageId, true));

// Used for fetching page resources
dispatch(fetchPublishedPageResourcesAction(basePageId));
dispatch(
fetchPublishedPageResourcesAction(basePageId, baseApplicationId),
);
}
}
}
Expand Down
Loading