app/client/src/workers/Evaluation/types.ts (1)

70-75: Provide a default value for executeReactiveActions

Many call-sites build EvalTreeResponseData by object spread & selective overrides.
If they omit the new field, TypeScript will allow undefined, forcing extra nullish checks.

Consider initialising with an empty array when constructing the response:

- executeReactiveActions: string[];
+ executeReactiveActions: string[]; // always defined

and where the object is created:

- executeReactiveActions,
+ executeReactiveActions: executeReactiveActions ?? [],

app/client/src/workers/Evaluation/handlers/evalTrigger.ts (1)

24-41: Deep-cloning the full eval tree on every trigger may be heavy

klona(dataTreeEvaluator.getEvalTree()) copies the entire tree for every non-onPageLoad trigger.
On large apps this could be several MB per call and run frequently during typing.

Options:

- const oldEvalTree = klona(dataTreeEvaluator.getEvalTree());
+ // PERF: consider a shallow clone or diff-targeted snapshot
+ const oldEvalTree = { ...dataTreeEvaluator.getEvalTree() };

Or gate it:

if (process.env.NODE_ENV !== "production") {
  oldEvalTree = klona(...);
}

Measure perf before shipping.

app/client/src/ce/reducers/uiReducers/editorReducer.tsx (1)

15-41: Missing initial value for onLoadActionExecution

initialState doesn’t declare onLoadActionExecution.
Any first call to SET_ONLOAD_ACTION_EXECUTED before INIT_CANVAS_LAYOUT will hit the spread on an undefined value and throw.

 export const initialState: EditorReduxState = {
   ...
   zoomLevel: 1,
+  onLoadActionExecution: {},
 };

app/client/src/widgets/MetaHOC.tsx (1)

90-97: Early-return guard is good, but switch map → forEach

The new guard neatly avoids useless work – nice.
However, batchActionsToRun.map(...) is still being used purely for side-effects; the returned array is discarded. Using forEach conveys intent better and avoids the linter nit.

- batchActionsToRun.map(([propertyName, actionExecution]) => {
+ batchActionsToRun.forEach(([propertyName, actionExecution]) => {

app/client/src/ce/sagas/index.tsx (1)

55-118: Saga list update OK, keep alphabetical grouping

PostEvaluationSagas is correctly imported and appended.
Minor: the array was roughly alphabetical; inserting after LintingSaga would preserve that ordering and reduce merge noise, but not functional.
No action needed if team doesn’t enforce order.

app/client/src/workers/Evaluation/evalTreeWithChanges.test.ts (1)

124-134: Tests updated for new arg – consider a positive case

All calls now pass the new oldEvalTree {} placeholder – good catch. A quick follow-up: add at least one test where oldEvalTree is non-empty to exercise the new code path and guard against regressions.

Also applies to: 228-229, 262-263, 302-303, 343-344, 377-378, 421-422, 498-499, 569-570

app/client/src/selectors/editorSelectors.tsx (1)

133-144: Selector looks solid – minor perf nit

The new getOnLoadActionsWithExecutionStatus neatly flattens the nested PageAction[][] and pairs each action with its execution flag. The optional-chaining guard handles absent state keys gracefully.

Tiny optional tweak: cache the onLoadActions.flat() result to avoid a second flatten if this selector grows, but not a blocker.

app/client/src/workers/Evaluation/__tests__/evaluation.test.ts (1)

638-640: Repeated literal arguments – consider a tiny helper.
evalAndValidateSubTree is now invoked ten times with identical arguments ([], evaluator.evalTree). Extracting a small constant or helper (const NO_REACTIVE: [] = [];) would shave noise and guard against typos.

Also applies to: 678-680, 727-728, 791-793, 836-838, 903-905, 977-979, 1013-1015, 1099-1101, 1139-1141

app/client/src/sagas/EvaluationsSaga.ts (1)

234-243: Guard update & tiny style nit

1. Spreading the full dataTree into a Redux payload will considerably increase the action size and serialisation cost.
   If the consumer saga can grab it from the store, prefer dispatching only the list of IDs and let the saga select(getDataTree).

2. Minor readability: optional chaining is enough and avoids the explicit length check:

-if (executeReactiveActions && executeReactiveActions.length) {
+if (executeReactiveActions?.length) {

app/client/src/workers/Evaluation/handlers/evalTree.ts (1)

197-198: Full deep-clone of the data tree is expensive

klona/json copies the whole evaluation tree on every update cycle.
For large apps this can be a significant CPU & memory hit.

If evalAndValidateSubTree only needs immutable access to previous values, consider:

• Keeping a frozen reference to dataTreeEvaluator.evalTree before mutations.
• Or cloning only the sub-paths present in evalOrder.

app/client/src/ce/entities/DataTree/dataTreeAction.ts (1)

77-81: Check for existing "run" dependency before overwrite

dependencyMap["run"] is unconditionally reassigned. If dependencyConfig already contained an entry for run, the previous set of dependencies is silently discarded.

-  dependencyMap["run"] = dynamicBindingPathList.map(
+  dependencyMap["run"] = [
+    ...(dependencyMap["run"] ?? []),
+    ...dynamicBindingPathList.map(
       (path: { key: string }) => path.key,
-  );
+    ),
+  ];

Merging prevents accidental loss of declarative dependencies.

app/client/src/ce/entities/DataTree/dataTreeJSAction.ts (1)

54-76: Reactive .data paths added but not in bindingPaths

You correctly add every <action>.data to reactivePaths, but you do not add them to bindingPaths. Since bindingPaths is later spread into reactivePaths ({ ...reactivePaths, ...bindingPaths }), this omission is harmless – yet it is asymmetric and can be confusing to future readers. Prefer adding the key to both maps for consistency.

+      bindingPaths[`${action.name}.data`] =
+        EvaluationSubstitutionType.SMART_SUBSTITUTE;

[nitpick]

app/client/src/ce/workers/Evaluation/evaluationUtils.ts (1)

1186-1208: isDataPath should include module-instance entities

The new module-instance entity type (MODULE_INSTANCE) also exposes a .data-like surface. Omitting it here will exclude it from several dependency and diff shortcuts added elsewhere.

-  if (isAction(entity) && propertyPath === "data") {
+  if ((isAction(entity) || isJSModuleInstance(entity)) && propertyPath === "data") {

You’ll of course need a proper isJSModuleInstance implementation first.

app/client/src/ee/entities/DataTree/types.ts (1)

35-48: Duplicate dynamicTriggerPathList redeclaration

JSModuleInstanceEntityConfig re-declares dynamicTriggerPathList, even though it’s already present in the parent ModuleInstanceEntityConfig. While TypeScript allows this if the type is identical, it’s redundant noise and easy to get out-of-sync.

   meta: Record<string, MetaArgs>;
   actionNames: Set<string>;
-  dynamicTriggerPathList: DynamicPath[];

Also check whether runBehaviour should be surfaced here for parity with ActionEntityConfig.

app/client/src/sagas/PostEvaluationSagas.ts (2)

387-403: Clean up boolean casts & leverage optional-chaining

if (!!entityConfig.moduleInstanceId) { ... }          // double negation
const runBehaviour = entityConfig?.meta && entityConfig.meta[action.name]?.runBehaviour;

1. !!value is unnecessary – use a direct truthy check.
2. entityConfig?.meta?.[action.name]?.runBehaviour is clearer and safer.

These minor tweaks improve readability without functional change.

---

412-421: Consider bounding the race wait

race([take(...)]) may wait indefinitely if no completion / error action is ever dispatched (e.g., network failure).
Add a timeout or cancellation path to prevent the saga from hanging forever.

app/client/src/entities/DependencyMap/DependencyMapUtils.ts (2)

15-33: Reuse existing getEntityNameAndPropertyPath instead of redefining

A utility with the same semantics already exists in ee/workers/Evaluation/evaluationUtils.ts.
Importing it avoids duplication and keeps behaviour consistent across the codebase.

-import type { ConfigTree } from "entities/DataTree/dataTreeTypes";
-import { isPathDynamicTrigger } from "utils/DynamicBindingUtils";
+import { getEntityNameAndPropertyPath } from "ee/workers/Evaluation/evaluationUtils";

---

188-266: detectReactiveDependencyMisuse is O(N²) – cache transitive deps

The current BFS per node recomputes transitive dependencies from scratch, which can explode on large graphs.
Caching the results of getAllTransitiveDependencies (or memoising during traversal) will drop complexity to near-linear.

app/client/src/workers/Evaluation/evalTreeWithChanges.ts (1)

74-78: Deep-cloning the entire eval tree every sync call is expensive

oldEvalTree = klona(dataTreeEvaluator.getEvalTree());

For large apps this can dominate worker time.
Consider capturing only the paths present in updatedValuePaths (or using a structural-sharing lib) instead of cloning the whole tree.

app/client/src/workers/common/DataTreeEvaluator/dataTreeEvaluator.test.ts (3)

306-312: DRY up the repeated evalAndValidateSubTree invocations

Every call now passes the same two trailing arguments:

[],                     // executeReactiveActions
dataTreeEvaluator.evalTree // oldEvalTree

1. Add a small helper (e.g. callEvalAndValidate) or default these params inside evalAndValidateSubTree so the tests stay readable.
2. Consider asserting on the returned executeReactiveActions array instead of always passing []. Doing so will actually test the new reactive-action flow rather than bypassing it.

- dataTreeEvaluator.evalAndValidateSubTree(order, cfg, updates, [], dataTreeEvaluator.evalTree);
+ callEvalAndValidate(order, cfg, updates);

This will trim ~30 lines and future-proof the tests if the signature changes again.

Also applies to: 483-485, 510-512, 540-542, 556-557, 586-588, 648-650, 663-665, 708-710

---

814-820: Enum value duplication inside meta objects

runBehaviour: ActionRunBehaviour.MANUAL is declared for each myFun* twice (JSObject1 & JSObject2).
If the enum is required for every action, consider populating it programmatically to avoid drift between test fixtures and production defaults. Otherwise keep the fixture minimal and only add the property where absolutely necessary.

Also applies to: 876-882

---

861-868: dynamicTriggerPathList test data bloating

The new dynamicTriggerPathList blocks are sizeable and duplicated. Since isDataField doesn’t reference this field, trimming them (or building them with a helper) will keep the fixture lean and reduce cognitive load for future maintainers.

Also applies to: 924-930

Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro

Learnt from: ankitakinger
PR: appsmithorg/appsmith#29573
File: app/client/src/pages/Editor/Explorer/Entity/EntityProperties.tsx:240-0
Timestamp: 2024-10-08T15:32:34.114Z
Learning: The `ENTITY_TYPE.MODULE_INSTANCE` case in `EntityProperties.tsx` is intentionally a combination of the logic from both `ENTITY_TYPE.ACTION` and `ENTITY_TYPE.JSACTION`, which explains the presence of what might seem like duplicated code.

Learnt from: ankitakinger
PR: appsmithorg/appsmith#29573
File: app/client/src/pages/Editor/Explorer/Entity/EntityProperties.tsx:240-0
Timestamp: 2024-07-26T21:12:57.228Z
Learning: The `ENTITY_TYPE.MODULE_INSTANCE` case in `EntityProperties.tsx` is intentionally a combination of the logic from both `ENTITY_TYPE.ACTION` and `ENTITY_TYPE.JSACTION`, which explains the presence of what might seem like duplicated code.

Learnt from: ankitakinger
PR: appsmithorg/appsmith#29573
File: app/client/src/ce/entities/DataTree/types.ts:182-186
Timestamp: 2024-07-26T21:12:57.228Z
Learning: The use of `any` type for `moduleInstanceEntities` in `app/client/src/ce/entities/DataTree/types.ts` is intentional and was done by another developer. This decision is likely to serve a specific purpose within the codebase, which may be related to maintaining compatibility between CE and EE or other architectural reasons.

Learnt from: ankitakinger
PR: appsmithorg/appsmith#29573
File: app/client/src/ce/entities/DataTree/types.ts:182-186
Timestamp: 2024-10-08T15:32:34.114Z
Learning: The use of `any` type for `moduleInstanceEntities` in `app/client/src/ce/entities/DataTree/types.ts` is intentional and was done by another developer. This decision is likely to serve a specific purpose within the codebase, which may be related to maintaining compatibility between CE and EE or other architectural reasons.

Learnt from: ankitakinger
PR: appsmithorg/appsmith#29573
File: app/client/src/pages/Editor/Explorer/Entity/EntityProperties.tsx:240-0
Timestamp: 2024-07-26T21:12:57.228Z
Learning: The `ENTITY_TYPE.MODULE_INSTANCE` case in `EntityProperties.tsx` is intentionally a combination of the logic from both `ENTITY_TYPE.ACTION` and `ENTITY_TYPE.JSACTION`, which explains the presence of what might seem like duplicated code.

Learnt from: ankitakinger
PR: appsmithorg/appsmith#29573
File: app/client/src/pages/Editor/Explorer/Entity/EntityProperties.tsx:240-0
Timestamp: 2024-10-08T15:32:34.114Z
Learning: The `ENTITY_TYPE.MODULE_INSTANCE` case in `EntityProperties.tsx` is intentionally a combination of the logic from both `ENTITY_TYPE.ACTION` and `ENTITY_TYPE.JSACTION`, which explains the presence of what might seem like duplicated code.

app/client/src/sagas/PostEvaluationSagas.ts (1)

355-357: Variable shadowing ‑ duplicates earlier feedback

The inner const action = … shadows the outer action parameter, repeating the issue flagged in an earlier review. Rename the loop variable to avoid confusion.

-    const action = Array.from(pendingActions)[0];
+    const nextPath = Array.from(pendingActions)[0];
+    // …
-    pendingActions.delete(action);
+    pendingActions.delete(nextPath);

app/client/src/workers/common/DataTreeEvaluator/index.ts (1)

347-352: Duplicate call to sortDependencies still present

The first invocation’s result is thrown away immediately by the second one, wasting CPU on every first-tree setup.

-    this.sortedDependencies = this.sortDependencies(this.dependencyMap);
-    this.sortedDependencies = this.sortDependencies(
-      this.dependencyMap,
-      [],
-      configTree,
-    );
+    this.sortedDependencies = this.sortDependencies(
+      this.dependencyMap,
+      [],
+      configTree,
+    );

app/client/src/sagas/PostEvaluationSagas.ts (3)

381-386: Drop redundant double negation

!!entityConfig.moduleInstanceId is equivalent to Boolean(entityConfig.moduleInstanceId) but neither is needed here – a simple truthy check reads cleaner.

-      if (!!entityConfig.moduleInstanceId) {
+      if (entityConfig.moduleInstanceId) {

---

394-396: Use optional chaining instead of explicit &&

The pattern entityConfig?.meta && entityConfig.meta[action.name] can be simplified and made safer with optional chaining.

-        const runBehaviour =
-          entityConfig?.meta && entityConfig.meta[action.name]?.runBehaviour;
+        const runBehaviour =
+          entityConfig?.meta?.[action.name]?.runBehaviour;

---

368-372: Prefer log.warn to console.warn inside sagas

console.warn bypasses the app’s logging level control and is harder to stub in tests. Replace with the centralized logger for consistency.

-      console.warn(`Entity ${entityName} not found in dataTree`);
+      log.warn(`Entity ${entityName} not found in dataTree`);

app/client/src/entities/DependencyMap/DependencyMapUtils.ts (2)

60-63: Static function should reference class name, not this

Inside a static context, using this.detectReactiveDependencyMisuse can be misleading and is flagged by Biome. Call the method via the class name for clarity.

-      if (configTree) {
-        this.detectReactiveDependencyMisuse(dependencyMap, configTree);
+      if (configTree) {
+        DependencyMapUtils.detectReactiveDependencyMisuse(
+          dependencyMap,
+          configTree,
+        );

---

223-226: Replace this. with class reference inside static method

Same rationale as above; avoid this in static context.

-      const triggerPaths = Array.from(deps).filter((dep) =>
-        this.isTriggerPath(dep, configTree),
-      );
-      const dataPaths = Array.from(deps).filter((dep) => this.isDataPath(dep));
+      const triggerPaths = Array.from(deps).filter((dep) =>
+        DependencyMapUtils.isTriggerPath(dep, configTree),
+      );
+      const dataPaths = Array.from(deps).filter((dep) =>
+        DependencyMapUtils.isDataPath(dep),
+      );

app/client/src/workers/common/DataTreeEvaluator/index.ts (3)

781-799: Potential stale reference while skipping .data paths

entity is pulled from unEvalTree, but the updated tree at this point is updatedUnEvalTreeJSObjects.
If JS-object processing mutated the structure, the check could be inaccurate.

-          const entity = unEvalTree[entityName];
+          const entity = updatedUnEvalTreeJSObjects[entityName];

Verify that this does not miss valid .data paths after JS updates.

---

1208-1244: Reactive-action dependency scan performs O(N×M) membership tests

Inside the evaluation loop every trigger path iterates over its direct dependencies and then scans Object.keys(valuechanged) for each.
Flatten this to constant-time look-ups by keeping valuechanged as a Set:

-    const valuechanged: Record<string, boolean> = {};
+    const valuechanged = new Set<string>();
...
-    valuechanged[fullPropertyPath] = true;
+    valuechanged.add(fullPropertyPath);
...
-    Object.keys(valuechanged).some((path) => dependenciesForPath.includes(path))
+    [...valuechanged].some((path) => dependenciesForPath.includes(path))

Gains both clarity and speed.

---

1577-1585: Parameter order drift in sortDependencies

diffs is now unused but still sits between dependencyMap and configTree, leading to confusing calls such as sortDependencies(map, [], config).
Recommend changing to an object parameter or re-ordering so that optional args are trailing.

Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro

app/client/src/workers/common/DataTreeEvaluator/index.ts (1)

1140-1144: Still prone to duplicate reactive executions – switch to a Set.

An earlier review flagged this; the array is unchanged. Duplicate pushes will dispatch identical EXECUTE_REACTIVE_QUERIES, wasting saga cycles.

-    const executeReactiveActions: string[] = [];
+    const executeReactiveActionsSet = new Set<string>();
...
-                executeReactiveActions.push(fullPropertyPath);
+                executeReactiveActionsSet.add(fullPropertyPath);
...
-      return {
-        evaluatedTree: safeTree,
-        ...
-        executeReactiveActions,
+      const executeReactiveActions = Array.from(executeReactiveActionsSet);
+      return {
+        evaluatedTree: safeTree,
+        ...
+        executeReactiveActions,
       };

app/client/src/entities/Action/actionProperties.test.ts (2)

31-38: Avoid copy-pasted test names

The test description "returns default list of no config is sent" appears again later for a different assertion set (binding paths). Re-using the same wording makes it harder to spot which test failed in CI.

-  it("returns default list of no config is sent", () => {
+  it("returns default reactive paths when no config is passed", () => {

---

311-318: Rename duplicated test title for clarity

This second test validates bindingPaths, not reactivePaths, yet reuses the earlier title. Renaming will prevent confusion.

-  it("returns default list of no config is sent", () => {
+  it("returns empty binding paths when no config is passed", () => {

app/client/src/ce/entities/DataTree/dataTreeJSAction.test.ts (1)

189-196: DRY up duplicated fixture data (runBehaviour: "MANUAL").

The same runBehaviour meta snippet appears in multiple test setups. Extract a helper (e.g., getDefaultMeta()) or use a shared constant to reduce maintenance overhead and chances of inconsistency.

app/client/src/workers/common/DataTreeEvaluator/index.ts (1)

780-799: Clarify & cache .data-path filtering loop.

The repeated isDataPath lookup per path runs on every edit cycle. Caching entity once per loop avoids the second object lookup and simplifies the guard:

-        pathsToSkipFromEval = pathsToSkipFromEval.filter((path) => {
-          if (!path.endsWith(".data")) return true;
-
-          const { entityName } = getEntityNameAndPropertyPath(path);
-          const entity = unEvalTree[entityName];
-          if (entity && isDataPath(entity, path)) {
-            return false; // filter out
-          }
-          return true; // keep
-        });
+        pathsToSkipFromEval = pathsToSkipFromEval.filter((path) => {
+          if (!path.endsWith(".data")) return true;
+          const { entityName } = getEntityNameAndPropertyPath(path);
+          return !isDataPath(unEvalTree[entityName], path);
+        });

Minor, but keeps the hot path lean.

Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro

app/client/src/sagas/PostEvaluationSagas.ts (1)

364-372: Fix variable shadowing issue (duplicate from previous review)

The loop variable action shadows the function parameter, reducing readability and potentially causing bugs.

-    const action = Array.from(pendingActions)[0];
+    const currentAction = Array.from(pendingActions)[0];

Ensure all subsequent references use currentAction instead of the shadowed action.

app/client/src/sagas/PostEvaluationSagas.ts (3)

395-395: Remove redundant double-negation

The double-negation is unnecessary as the value will be coerced to boolean in the conditional context.

-      if (!!entityConfig.moduleInstanceId) {
+      if (entityConfig.moduleInstanceId) {

---

407-408: Use optional chaining for safer property access

Replace nested property access with optional chaining for better readability and safety.

-        const runBehaviour =
-          entityConfig?.meta && entityConfig.meta[action.name]?.runBehaviour;
+        const runBehaviour = entityConfig?.meta?.[action.name]?.runBehaviour;

---

380-382: Use proper logging instead of console.warn

Replace console.warn with the existing logging infrastructure for consistency.

-      //eslint-disable-next-line no-console
-      console.warn(`Entity ${entityName} not found in dataTree`);
+      log.warn(`Entity ${entityName} not found in dataTree`);

Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro

app/client/src/sagas/PostEvaluationSagas.ts (1)

366-366: Fix variable shadowing

The variable action shadows the function parameter action, which reduces readability and could cause bugs.

-    const action = Array.from(pendingActions)[0];
+    const currentPath = Array.from(pendingActions)[0];

Make sure to update all references to use currentPath instead of the shadowed action variable throughout the loop.

app/client/src/entities/DependencyMap/DependencyMapUtils.ts (3)

42-42: Replace this with class name in static methods

Using this in static methods is confusing and should be replaced with the class name for clarity.

-        this.detectReactiveDependencyMisuse(dependencyMap, configTree);
+        DependencyMapUtils.detectReactiveDependencyMisuse(dependencyMap, configTree);

-        this.isTriggerPath(dep, configTree),
+        DependencyMapUtils.isTriggerPath(dep, configTree),

-      const dataPaths = Array.from(deps).filter((dep) => this.isDataPath(dep));
+      const dataPaths = Array.from(deps).filter((dep) => DependencyMapUtils.isDataPath(dep));

Also applies to: 204-204, 206-206

---

47-60: Improve error handling type safety

The error handling contains TypeScript ignore comments and unsafe type casting that could be improved.

-      if (
-        error instanceof Error &&
-        error.message.includes("Reactive dependency misuse")
-      ) {
-        // eslint-disable-next-line @typescript-eslint/ban-ts-comment
-        // @ts-ignore
-        return {
-          success: false,
-          // eslint-disable-next-line @typescript-eslint/no-explicit-any
-          cyclicNode: (error as any).node,
-          error,
-        };
-      }
+      if (
+        error instanceof Error &&
+        error.message.includes("Reactive dependency misuse")
+      ) {
+        const reactiveMisuseError = error as Error & { node?: string };
+        return {
+          success: false,
+          cyclicNode: reactiveMisuseError.node || "",
+          error,
+        };
+      }

---

160-242: Consider breaking down the complex reactive dependency detection

The detectReactiveDependencyMisuse method is quite complex and handles multiple responsibilities. Consider extracting helper methods for better readability and maintainability.

The method could be broken down into:

• getAllTransitiveDependencies (already extracted inline)
• categorizePathsByType - to separate trigger and data paths
• checkForReactiveMisuse - to perform the actual misuse detection

This would improve testability and readability of the complex logic.

app/client/src/sagas/PostEvaluationSagas.ts (2)

395-395: Remove redundant double-negation

The double-negation !! is unnecessary when the value will be coerced to boolean.

-      if (!!entityConfig.moduleInstanceId) {
+      if (entityConfig.moduleInstanceId) {

---

407-409: Use optional chaining for safer property access

Replace nested property access with optional chaining for better safety.

-        const runBehaviour =
-          entityConfig?.meta && entityConfig.meta[action.name]?.runBehaviour;
+        const runBehaviour = entityConfig?.meta?.[action.name]?.runBehaviour;

Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro