Skip to content
Merged
Show file tree
Hide file tree
Changes from 2 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
142 changes: 141 additions & 1 deletion app/client/src/sagas/EvaluationsSaga.test.ts
Original file line number Diff line number Diff line change
@@ -1,10 +1,14 @@
import {
defaultAffectedJSObjects,
evalAndLintingHandler,
evalQueueBuffer,
evaluateTreeSaga,
evaluationLoopWithDebounce,
type BUFFERED_ACTION,
} from "./EvaluationsSaga";
import { evalWorker } from "utils/workerInstances";
import { expectSaga } from "redux-saga-test-plan";
import { expectSaga, testSaga } from "redux-saga-test-plan";

import { EVAL_WORKER_ACTIONS } from "ee/workers/Evaluation/evalWorkerActions";
import { select } from "redux-saga/effects";
import { getMetaWidgets, getWidgets, getWidgetsMeta } from "./selectors";
Expand Down Expand Up @@ -76,6 +80,7 @@ describe("evaluateTreeSaga", () => {
widgetsMeta: {},
shouldRespondWithLogs: true,
affectedJSObjects: { ids: [], isAllAffected: false },
actionDataPayloadConsolidated: undefined,
})
.run();
});
Expand Down Expand Up @@ -121,6 +126,7 @@ describe("evaluateTreeSaga", () => {
widgetsMeta: {},
shouldRespondWithLogs: false,
affectedJSObjects: { ids: [], isAllAffected: false },
actionDataPayloadConsolidated: undefined,
})
.run();
});
Expand Down Expand Up @@ -175,6 +181,7 @@ describe("evaluateTreeSaga", () => {
widgetsMeta: {},
shouldRespondWithLogs: false,
affectedJSObjects,
actionDataPayloadConsolidated: undefined,
})
.run();
});
Expand Down Expand Up @@ -385,3 +392,136 @@ describe("evalQueueBuffer", () => {
});
});
});

describe("evaluationLoopWithDebounce", () => {
describe("debounce", () => {
test("should call a regular evaluation with the consolidated action data payload when both updateActionData and evaluation action is triggered", async () => {
const buffer = evalQueueBuffer();

buffer.put(
updateActionData([
{
entityName: "widget1",
dataPath: "data",
data: { a: 1 },
dataPathRef: "",
},
]),
);
buffer.put(
updateActionData([
{
entityName: "widget2",
dataPath: "data",
data: { a: 2 },
dataPathRef: "",
},
]),
);
// eslint-disable-next-line @typescript-eslint/no-explicit-any
buffer.put(createJSCollectionSuccess({ id: "1" } as any));
const action = buffer.take();

const mockChannel = "mock-channel";

// assert that a regular evaluation is only triggered and no evalTreeWithChanges evaluation is triggered
return (
testSaga(evaluationLoopWithDebounce, mockChannel)
.next()
.take(mockChannel)
.next(action)
.call(evalAndLintingHandler, true, action, {
actionDataPayloadConsolidated: [
{
entityName: "widget1",
dataPath: "data",
data: { a: 1 },
dataPathRef: "",
},
{
entityName: "widget2",
dataPath: "data",
data: { a: 2 },
dataPathRef: "",
},
],
shouldReplay: undefined,
forceEvaluation: false,
requiresLogging: undefined,
affectedJSObjects: { isAllAffected: false, ids: ["1"] },
})
.next()
// wait for the next action in the event loop
.take(mockChannel)
);
});
test("should call an evalTreeWithChanges when only updateActionData actions are triggered", async () => {
const buffer = evalQueueBuffer();

buffer.put(
updateActionData([
{
entityName: "widget1",
dataPath: "data",
data: { a: 1 },
dataPathRef: "",
},
]),
);
buffer.put(
updateActionData([
{
entityName: "widget2",
dataPath: "data",
data: { a: 2 },
dataPathRef: "",
},
]),
);
const action = buffer.take() as unknown as BUFFERED_ACTION;

const mockChannel = "mock-channel";

return (
testSaga(evaluationLoopWithDebounce, mockChannel)
.next()
.take(mockChannel)
.next(action)
.call(
evalWorker.request,
EVAL_WORKER_ACTIONS.UPDATE_ACTION_DATA,
action.actionDataPayloadConsolidated,
)
.next()
// wait for the next action in the event loop
.take(mockChannel)
);
});
test("should call a regular evaluation when evaluation actions are triggered", async () => {
const buffer = evalQueueBuffer();

// eslint-disable-next-line @typescript-eslint/no-explicit-any
buffer.put(createJSCollectionSuccess({ id: "1" } as any));

const action = buffer.take();

const mockChannel = "mock-channel";

return (
testSaga(evaluationLoopWithDebounce, mockChannel)
.next()
.take(mockChannel)
.next(action)
.call(evalAndLintingHandler, true, action, {
shouldReplay: undefined,
forceEvaluation: false,
requiresLogging: undefined,
affectedJSObjects: { isAllAffected: false, ids: ["1"] },
})
.next()
// wait for the next action in the event loop
.take(mockChannel)
);
});
});
});
42 changes: 37 additions & 5 deletions app/client/src/sagas/EvaluationsSaga.ts
Original file line number Diff line number Diff line change
Expand Up @@ -249,6 +249,7 @@ export function* evaluateTreeSaga(
forceEvaluation = false,
requiresLogging = false,
affectedJSObjects: AffectedJSObjects = defaultAffectedJSObjects,
actionDataPayloadConsolidated?: actionDataPayload,
) {
const allActionValidationConfig: ReturnType<
typeof getAllActionValidationConfig
Expand Down Expand Up @@ -291,6 +292,7 @@ export function* evaluateTreeSaga(
widgetsMeta,
shouldRespondWithLogs,
affectedJSObjects,
actionDataPayloadConsolidated,
};

const workerResponse: EvalTreeResponseData = yield call(
Expand Down Expand Up @@ -542,7 +544,7 @@ export const defaultAffectedJSObjects: AffectedJSObjects = {
ids: [],
};

interface BUFFERED_ACTION {
export interface BUFFERED_ACTION {
hasDebouncedHandleUpdate: boolean;
hasBufferedAction: boolean;
actionDataPayloadConsolidated: actionDataPayload[];
Expand Down Expand Up @@ -674,19 +676,25 @@ function getPostEvalActions(
return postEvalActions;
}

function* evalAndLintingHandler(
export function* evalAndLintingHandler(
isBlockingCall = true,
action: ReduxAction<unknown>,
options: Partial<{
shouldReplay: boolean;
forceEvaluation: boolean;
requiresLogging: boolean;
affectedJSObjects: AffectedJSObjects;
actionDataPayloadConsolidated: actionDataPayload[];
}>,
) {
const span = startRootSpan("evalAndLintingHandler");
const { affectedJSObjects, forceEvaluation, requiresLogging, shouldReplay } =
options;
const {
actionDataPayloadConsolidated,
affectedJSObjects,
forceEvaluation,
requiresLogging,
shouldReplay,
} = options;

const requiresLinting = getRequiresLinting(action);

Expand Down Expand Up @@ -728,6 +736,7 @@ function* evalAndLintingHandler(
forceEvaluation,
requiresLogging,
affectedJSObjects,
actionDataPayloadConsolidated,
),
);
}
Expand Down Expand Up @@ -800,6 +809,13 @@ function* evaluationChangeListenerSaga(): any {
evalQueueBuffer(),
);

yield call(evaluationLoopWithDebounce, evtActionChannel);
}

export function* evaluationLoopWithDebounce(
// eslint-disable-next-line @typescript-eslint/no-explicit-any
evtActionChannel: ActionPattern<Action<any>>,
) {
while (true) {
const action: EvaluationReduxAction<unknown | unknown[]> =
yield take(evtActionChannel);
Expand Down Expand Up @@ -834,6 +850,23 @@ function* evaluationChangeListenerSaga(): any {
hasDebouncedHandleUpdate,
} = action as unknown as BUFFERED_ACTION;

// when there are both debounced action updates evaluation and a regular evaluation
// we will convert that to a regular evaluation this should help in performance by
// not performing a debounced action updates evaluation
if (hasDebouncedHandleUpdate && hasBufferedAction) {
const affectedJSObjects = getAffectedJSObjectIdsFromAction(action);

yield call(evalAndLintingHandler, true, action, {
actionDataPayloadConsolidated,
shouldReplay: get(action, "payload.shouldReplay"),
forceEvaluation: shouldForceEval(action),
requiresLogging: shouldLog(action),
affectedJSObjects,
});

continue;
}
Comment on lines +853 to +868

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Add error handling for edge cases.

The debounced action handling logic should include error handling for cases where the action payload might be malformed.

    if (hasDebouncedHandleUpdate && hasBufferedAction) {
+     try {
        const affectedJSObjects = getAffectedJSObjectIdsFromAction(action);

        yield call(evalAndLintingHandler, true, action, {
          actionDataPayloadConsolidated,
          shouldReplay: get(action, "payload.shouldReplay"),
          forceEvaluation: shouldForceEval(action),
          requiresLogging: shouldLog(action),
          affectedJSObjects,
        });
+     } catch (error) {
+       console.error("Failed to handle debounced action:", error);
+       // Consider adding error reporting here
+     }

      continue;
    }
📝 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
// when there are both debounced action updates evaluation and a regular evaluation
// we will convert that to a regular evaluation this should help in performance by
// not performing a debounced action updates evaluation
if (hasDebouncedHandleUpdate && hasBufferedAction) {
const affectedJSObjects = getAffectedJSObjectIdsFromAction(action);
yield call(evalAndLintingHandler, true, action, {
actionDataPayloadConsolidated,
shouldReplay: get(action, "payload.shouldReplay"),
forceEvaluation: shouldForceEval(action),
requiresLogging: shouldLog(action),
affectedJSObjects,
});
continue;
}
// when there are both debounced action updates evaluation and a regular evaluation
// we will convert that to a regular evaluation this should help in performance by
// not performing a debounced action updates evaluation
if (hasDebouncedHandleUpdate && hasBufferedAction) {
try {
const affectedJSObjects = getAffectedJSObjectIdsFromAction(action);
yield call(evalAndLintingHandler, true, action, {
actionDataPayloadConsolidated,
shouldReplay: get(action, "payload.shouldReplay"),
forceEvaluation: shouldForceEval(action),
requiresLogging: shouldLog(action),
affectedJSObjects,
});
} catch (error) {
console.error("Failed to handle debounced action:", error);
// Consider adding error reporting here
}
continue;
}


if (hasDebouncedHandleUpdate) {
yield call(
evalWorker.request,
Expand All @@ -856,7 +889,6 @@ function* evaluationChangeListenerSaga(): any {
}
}
}

// TODO: Fix this the next time the file is edited
// eslint-disable-next-line @typescript-eslint/no-explicit-any
export function* evaluateActionSelectorFieldSaga(action: any) {
Expand Down
10 changes: 10 additions & 0 deletions app/client/src/workers/Evaluation/handlers/evalTree.ts
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ import {
import type { CanvasWidgetsReduxState } from "reducers/entityReducers/canvasWidgetsReducer";
import type { MetaWidgetsReduxState } from "reducers/entityReducers/metaWidgetsReducer";
import type { Attributes } from "instrumentation/types";
import { updateActionsToEvalTree } from "./updateActionData";

// TODO: Fix this the next time the file is edited
// eslint-disable-next-line @typescript-eslint/no-explicit-any
Expand Down Expand Up @@ -70,6 +71,7 @@ export async function evalTree(
let isNewWidgetAdded = false;

const {
actionDataPayloadConsolidated,
affectedJSObjects,
allActionValidationConfig,
appMode,
Expand Down Expand Up @@ -190,6 +192,13 @@ export async function evalTree(
});
staleMetaIds = dataTreeResponse.staleMetaIds;
} else {
const tree = dataTreeEvaluator.getEvalTree();

// during update cycles update actions to the dataTree directly
// this is useful in cases where we have debounced updateActionData and a regular evaluation
// triggered together, in those cases we merge them both into a regular evaluation
updateActionsToEvalTree(tree, actionDataPayloadConsolidated);

if (dataTreeEvaluator && !isEmpty(allActionValidationConfig)) {
dataTreeEvaluator.setAllActionValidationConfig(
allActionValidationConfig,
Expand All @@ -212,6 +221,7 @@ export async function evalTree(
configTree,
webworkerTelemetry,
affectedJSObjects,
actionDataPayloadConsolidated,
),
);

Expand Down
38 changes: 24 additions & 14 deletions app/client/src/workers/Evaluation/handlers/updateActionData.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import set from "lodash/set";
import { evalTreeWithChanges } from "../evalTreeWithChanges";
import DataStore from "../dataStore";
import { EVAL_WORKER_SYNC_ACTION } from "ee/workers/Evaluation/evalWorkerActions";
import type { DataTree } from "entities/DataTree/dataTreeTypes";

export interface UpdateActionProps {
entityName: string;
Expand All @@ -26,6 +27,29 @@ export function handleActionsDataUpdate(actionsToUpdate: UpdateActionProps[]) {

const evalTree = dataTreeEvaluator.getEvalTree();

updateActionsToEvalTree(evalTree, actionsToUpdate);

const updatedProperties: string[][] = [];

actionsToUpdate.forEach(({ dataPath, entityName }) => {
updatedProperties.push([entityName, dataPath]);
});
evalTreeWithChanges({
data: {
updatedValuePaths: updatedProperties,
metaUpdates: [],
},
method: EVAL_WORKER_SYNC_ACTION.EVAL_TREE_WITH_CHANGES,
webworkerTelemetry: {},
});
}

export function updateActionsToEvalTree(
evalTree: DataTree,
actionsToUpdate?: UpdateActionProps[],
) {
if (!actionsToUpdate) return;

for (const actionToUpdate of actionsToUpdate) {
const { dataPath, dataPathRef, entityName } = actionToUpdate;
let { data } = actionToUpdate;
Expand All @@ -44,18 +68,4 @@ export function handleActionsDataUpdate(actionsToUpdate: UpdateActionProps[]) {

DataStore.setActionData(path, data);
}

const updatedProperties: string[][] = [];

actionsToUpdate.forEach(({ dataPath, entityName }) => {
updatedProperties.push([entityName, dataPath]);
});
evalTreeWithChanges({
data: {
updatedValuePaths: updatedProperties,
metaUpdates: [],
},
method: EVAL_WORKER_SYNC_ACTION.EVAL_TREE_WITH_CHANGES,
webworkerTelemetry: {},
});
}
2 changes: 2 additions & 0 deletions app/client/src/workers/Evaluation/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ import type { APP_MODE } from "entities/App";
import type { WebworkerSpanData, Attributes } from "instrumentation/types";
import type { ICacheProps } from "../common/AppComputationCache/types";
import type { AffectedJSObjects } from "actions/EvaluationReduxActionTypes";
import type { UpdateActionProps } from "./handlers/updateActionData";

// TODO: Fix this the next time the file is edited
// eslint-disable-next-line @typescript-eslint/no-explicit-any
Expand Down Expand Up @@ -51,6 +52,7 @@ export interface EvalTreeRequestData {
widgetsMeta: Record<string, any>;
shouldRespondWithLogs?: boolean;
affectedJSObjects: AffectedJSObjects;
actionDataPayloadConsolidated?: UpdateActionProps[];
}

export interface EvalTreeResponseData {
Expand Down
Loading