Skip to content

fix: Updating the function definition for checking reactive cyclic dependencies #41049

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
ankitakinger merged 4 commits into from
Jun 27, 2025
Merged

fix: Updating the function definition for checking reactive cyclic dependencies #41049

Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
0ddddb7
updating the function definition for checking reactive cyclic depende…
ankitakinger Jun 26, 2025
672b406
adding unit test
ankitakinger Jun 26, 2025
612aaf7
adding additional checks to run the logic only when run behaviour is …
ankitakinger Jun 26, 2025
05c9f64
addressing review comment
ankitakinger Jun 27, 2025
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
120 changes: 54 additions & 66 deletions app/client/src/entities/DependencyMap/DependencyMapUtils.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -163,81 +163,69 @@ export class DependencyMapUtils {
) {
const dependencies = dependencyMap.rawDependencies;

// Helper function to get all transitive dependencies
const getAllTransitiveDependencies = (node: string): Set<string> => {
const allDeps = new Set<string>();
const queue = [node];

while (queue.length > 0) {
const current = queue.shift()!;
const deps = dependencyMap.getDirectDependencies(current) || [];

for (const dep of deps) {
if (!allDeps.has(dep)) {
allDeps.add(dep);
queue.push(dep);
}
}
}

return allDeps;
};
for (const node of dependencies.keys()) {
// For each entity, check if both .run and a .data path are present
let hasRun = false;
let hasData = false;
let dataPath = "";
let runPath = "";

const transitiveDeps = this.getAllTransitiveDependencies(
dependencyMap,
node,
);

for (const [node, deps] of dependencies.entries()) {
// Get all dependencies including transitive ones
const allDeps = new Set<string>();
const queue = Array.from(deps);
for (const dep of transitiveDeps) {
const { entityName } = getEntityNameAndPropertyPath(dep);
const entityConfig = configTree[entityName];

while (queue.length > 0) {
const dep = queue.shift()!;
if (entityConfig && entityConfig.ENTITY_TYPE === "ACTION") {
if (this.isTriggerPath(dep, configTree)) {
hasRun = true;
runPath = dep;
}

if (!allDeps.has(dep)) {
allDeps.add(dep);
const depDeps = dependencyMap.getDirectDependencies(dep) || [];
if (DependencyMapUtils.isDataPath(dep)) {
hasData = true;
dataPath = dep;
}

queue.push(...depDeps);
if (hasRun && hasData) {
throw Object.assign(
new Error(
`Reactive dependency misuse: '${node}' depends on both trigger path '${runPath}' and data path '${dataPath}' from the same entity. This can cause unexpected reactivity.`,
),
{ node, triggerPath: runPath, dataPath },
);
}
}
}
}
}

// Separate dependencies into trigger paths and data paths
const triggerPaths = Array.from(deps).filter((dep) =>
this.isTriggerPath(dep, configTree),
);
const dataPaths = Array.from(deps).filter((dep) => this.isDataPath(dep));

// For each trigger path, check if there's a data path from the same entity
for (const triggerPath of triggerPaths) {
const triggerEntity = triggerPath.split(".")[0];

// Find data paths from the same entity
const sameEntityDataPaths = dataPaths.filter((dataPath) => {
const dataEntity = dataPath.split(".")[0];

return dataEntity === triggerEntity;
});

if (sameEntityDataPaths.length > 0) {
// Check if any of these data paths depend on the trigger path (directly or indirectly)
for (const dataPath of sameEntityDataPaths) {
const dataPathTransitiveDeps =
getAllTransitiveDependencies(dataPath);

if (dataPathTransitiveDeps.has(triggerPath)) {
const error = new Error(
`Reactive dependency misuse: '${node}' depends on both trigger path '${triggerPath}' and data path '${dataPath}' from the same entity, and '${dataPath}' depends on '${triggerPath}' (directly or indirectly). This can cause unexpected reactivity.`,
);

// Add custom properties
// eslint-disable-next-line @typescript-eslint/ban-ts-comment
// @ts-ignore
// eslint-disable-next-line @typescript-eslint/no-explicit-any
(error as any).node = node;

throw error;
}
}
/**
* Returns all transitive dependencies (direct and indirect, no duplicates) for a given node.
*/
static getAllTransitiveDependencies(
dependencyMap: DependencyMap,
node: string,
): string[] {
const dependencies = dependencyMap.rawDependencies;
const visited = new Set<string>();

function traverse(current: string) {
const directDeps = dependencies.get(current) || new Set<string>();

for (const dep of directDeps) {
if (!visited.has(dep)) {
visited.add(dep);
traverse(dep);
}
}
}

traverse(node);

return Array.from(visited);
}
}
Loading