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.

Sign up for GitHub

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

Jump to bottom

fix: Revert PageSaga changes #36666

Merged
merged 2 commits into from
Oct 3, 2024
Merged

fix: Revert PageSaga changes #36666

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
09ed2a2
Revert "fix: Revert "fix the gitSync related issue" (#36658)"
rishabhrathod01 Oct 3, 2024
f550232
Revert "fix: remove redundant eval trigger (#36433)"
rishabhrathod01 Oct 3, 2024
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: 6 additions & 3 deletions app/client/src/actions/pageActions.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,23 +68,25 @@ 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 @@ -299,12 +301,10 @@ 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,18 +675,21 @@ 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
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -325,47 +325,12 @@ 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);
// 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,
) {
// set current page
yield put(
updateCurrentPage(
pageId,
response.data.slug,
response.data.userPermissions,
),
);
// Clear any existing caches
yield call(clearEvalCache);
// Set url params
yield call(setDataUrl);

yield call(updateCanvasLayout, response);
}

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

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

if (isValidResponse) {
yield call(postFetchedPublishedPage, response, pageId);
// 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,
),
);

// 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 @@ -395,7 +392,7 @@ export function* fetchPublishedPageResourcesSaga(
action: ReduxAction<FetchPublishedPageResourcesPayload>,
) {
try {
const { applicationId, pageId } = action.payload;
const { pageId } = action.payload;

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

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 }));
// 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 put(fetchAllPageEntityCompletion([executePageLoadActions()]));
}
} catch (error) {
Expand Down Expand Up @@ -1433,13 +1425,21 @@ export function* setupPublishedPageSaga(
action: ReduxAction<SetupPublishedPageActionPayload>,
) {
try {
const { bustCache, pageId, pageWithMigratedDsl } = action.payload;
const { bustCache, firstLoad, 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, pageWithMigratedDsl));
yield put(
fetchPublishedPageAction(
pageId,
bustCache,
firstLoad,
pageWithMigratedDsl,
),
);
yield take(ReduxActionTypes.FETCH_PUBLISHED_PAGE_SUCCESS);

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

Expand All @@ -56,6 +57,7 @@ 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
View file
Open in desktop
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, pageWithMigratedDsl),
setupPublishedPage(toLoadPageId, true, true, pageWithMigratedDsl),
Copy link
Contributor

Choose a reason for hiding this comment

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

💡 Codebase verification

Attention to Detail is Key!

You've added an extra true parameter to the setupPublishedPage function call in AppViewerEngine.ts. Great job on making progress! However, I noticed that there's another usage of setupPublishedPage in index.tsx with only two arguments:

  • app/client/src/pages/AppViewer/index.tsx: dispatch(setupPublishedPage(pageId, true));

To ensure consistency and prevent potential issues:

  • Please update all instances of setupPublishedPage to include the new boolean parameter.
  • Verify that the function implementation can handle the additional parameter without causing unintended side effects.
  • Consider adding tests to cover the updated function signature and its usages.
🔗 Analysis chain

Class, let's examine this change carefully!

I see you've added an extra true parameter to the setupPublishedPage function call. This is an important modification, so let's make sure we understand its implications:

  1. This change aligns with the PR objective of reverting previous modifications. Well done on following through with the reversion!

  2. However, we need to ensure this doesn't cause any unintended side effects. Can you please verify that this change doesn't negatively impact the page setup process in the published view?

  3. Remember, good documentation is crucial for maintaining our codebase. If you haven't already, please update any relevant documentation or comments to reflect this change in the function signature.

To help us understand the impact, let's run a quick check on the setupPublishedPage function:

Class, after running this check, we'll have a better understanding of how this change fits into the bigger picture. Remember, in coding as in life, it's important to consider the ripple effects of our actions!

Great effort on setting up the initial verification! Let's refine our approach to ensure we gather the needed information effectively.

It looks like the rg command didn't recognize the typescript file type. Let's adjust it to use the correct identifier and try again:

Let's run this updated script to accurately locate the function definition and its usages. Once we have the results, we'll be better equipped to finalize the verification!

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check the implementation and usage of setupPublishedPage

# Search for the setupPublishedPage function definition
echo "setupPublishedPage function definition:"
ast-grep --lang typescript --pattern $'function setupPublishedPage($_) {
  $$$
}'

# Search for other usages of setupPublishedPage
echo "\nOther usages of setupPublishedPage:"
rg --type typescript 'setupPublishedPage\(' -A 2

Length of output: 342

Script:

#!/bin/bash
# Description: Verify the implementation and usage of setupPublishedPage with corrected file type

# Search for the setupPublishedPage function definition
echo "setupPublishedPage function definition:"
ast-grep --lang typescript --pattern $'function setupPublishedPage($_) {
  $$$
}'

# Search for other usages of setupPublishedPage with correct file type
echo "\nOther usages of setupPublishedPage:"
rg --type ts 'setupPublishedPage\(' -A 2

Length of output: 757

];

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

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

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