Skip to content

chore: converted isNewEntity lookup to use a set #38264

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

Merged
vsvamsi1 merged 1 commit into from
Dec 20, 2024
+9 −7
Merged

chore: converted isNewEntity lookup to use a set #38264

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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/ce/workers/Evaluation/evaluationUtils.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ import type {
ConfigTree,
} from "entities/DataTree/dataTreeTypes";
import { ENTITY_TYPE } from "ee/entities/DataTree/types";
import _, { difference, find, get, has, isEmpty, isNil, set } from "lodash";
import _, { difference, get, has, isEmpty, isNil, set } from "lodash";
import type { WidgetTypeConfigMap } from "WidgetProvider/factory";
import { PluginType } from "entities/Action";
import { klona } from "klona/full";
Expand Down Expand Up @@ -987,11 +987,8 @@ export const isATriggerPath = (
};

// Checks if entity newly got added to the unevalTree
export const isNewEntity = (updates: DataTreeDiff[], entityName: string) => {
return !!find(updates, {
event: DataTreeDiffEvent.NEW,
payload: { propertyPath: entityName },
});
export const isNewEntity = (updates: Set<string>, entityName: string) => {
return updates.has(entityName);
Comment on lines +990 to +991

@coderabbitai coderabbitai Bot Dec 19, 2024

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.

⚠️ Potential issue

Entity path vs. entity name mismatch.
This function only checks updates.has(entityName), but the set is populated with full property paths in index.ts unless corrected. Ensure that the stored values and this membership check are aligned.

If the goal is to confirm a new entity by name, consider storing only the entity name in the set or changing the membership check to parse out the entity name from the stored path.

};

const widgetPathsNotToOverride = (
Expand Down
7 changes: 6 additions & 1 deletion app/client/src/workers/common/DataTreeEvaluator/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1092,6 +1092,11 @@ export default class DataTreeEvaluator {
const { isFirstTree, metaWidgets, unevalUpdates } = options;
let staleMetaIds: string[] = [];

const allNewEntityDiffSet = new Set(
unevalUpdates
.filter((v) => v.event === DataTreeDiffEvent.NEW)
.map((v) => v.payload.propertyPath),
);
Comment on lines +1095 to +1099

@coderabbitai coderabbitai Bot Dec 19, 2024

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.

⚠️ Potential issue

Potential mismatch when storing entity property paths in a set.
Currently, this code stores the entire property path (e.g., "Table1.selectedRow") in allNewEntityDiffSet, then checks for membership with just the entity name (e.g., "Table1"). This will fail if the property path includes anything more than the entity name.

A possible fix is to store just the entity names in the set, for example:

-const allNewEntityDiffSet = new Set(
-  unevalUpdates
-    .filter((v) => v.event === DataTreeDiffEvent.NEW)
-    .map((v) => v.payload.propertyPath),
-);
+const allNewEntityDiffSet = new Set(
+  unevalUpdates
+    .filter((v) => v.event === DataTreeDiffEvent.NEW)
+    .map((v) => getEntityNameAndPropertyPath(v.payload.propertyPath).entityName),
+);

Committable suggestion skipped: line range outside the PR's diff.

let evalContextCache;

if (WorkerEnv.flags.release_evaluation_scope_cache) {
Expand Down Expand Up @@ -1198,7 +1203,7 @@ export default class DataTreeEvaluator {
if (isATriggerPath) continue;

const isNewWidget =
isFirstTree || isNewEntity(unevalUpdates, entityName);
isFirstTree || isNewEntity(allNewEntityDiffSet, entityName);

const widgetEntity = entity as WidgetEntity;

Expand Down