fix: Updating the function definition for checking reactive cyclic dependencies#41049
Merged
ankitakinger merged 4 commits intoreleasefrom Jun 27, 2025
Merged
fix: Updating the function definition for checking reactive cyclic dependencies#41049ankitakinger merged 4 commits intoreleasefrom
ankitakinger merged 4 commits intoreleasefrom
Conversation
Contributor
WalkthroughThe Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant DependencyMapUtils
participant DependencyMap
Caller->>DependencyMapUtils: detectReactiveDependencyMisuse(dependencyMap)
loop For each node in dependencyMap
DependencyMapUtils->>DependencyMapUtils: getAllTransitiveDependencies(dependencyMap, node)
DependencyMapUtils->>DependencyMap: Access dependency data
DependencyMapUtils->>DependencyMapUtils: Check for trigger/data path misuse
alt Misuse detected
DependencyMapUtils-->>Caller: Throw error
end
end
DependencyMapUtils-->>Caller: Complete without error
Assessment against linked issues
Assessment against linked issues: Out-of-scope changesNo out-of-scope changes found. Suggested labels
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
🧰 Additional context used🧠 Learnings (2)📓 Common learningsapp/client/src/entities/DependencyMap/DependencyMapUtils.ts (20)🪛 Biome (1.9.4)app/client/src/entities/DependencyMap/DependencyMapUtils.ts[error] 202-202: Using this in a static context can be confusing. this refers to the class. (lint/complexity/noThisInStatic) [error] 212-212: Using this in a static context can be confusing. this refers to the class. (lint/complexity/noThisInStatic) ⏰ Context from checks skipped due to timeout of 90000ms (8)
🔇 Additional comments (4)
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
github-actions
bot
added
Bug
Contributor
Author
|
/build-deploy-preview skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/15899081506. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app/client/src/entities/DependencyMap/DependencyMapUtils.ts(1 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
app/client/src/entities/DependencyMap/DependencyMapUtils.ts
[error] 173-173: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 183-183: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: perform-test / rts-build / build
- GitHub Check: perform-test / client-build / client-build
- GitHub Check: perform-test / server-build / server-unit-tests
- GitHub Check: client-lint / client-lint
- GitHub Check: client-prettier / prettier-check
- GitHub Check: client-unit-tests / client-unit-tests
- GitHub Check: client-check-cyclic-deps / check-cyclic-dependencies
- GitHub Check: client-build / client-build
🔇 Additional comments (1)
app/client/src/entities/DependencyMap/DependencyMapUtils.ts (1)
209-230: Well-implemented DFS traversal with proper cycle handling.The implementation correctly handles transitive dependency collection with cycle prevention. The use of a visited set and nested traversal function is appropriate.
Minor suggestion for consistency:
/** * Returns all transitive dependencies (direct and indirect, no duplicates) for a given node. + * @param dependencyMap The dependency map to traverse + * @param node The starting node + * @returns Array of all transitive dependencies */
|
Deploy-Preview-URL: https://ce-41049.dp.appsmith.com |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
app/client/src/entities/DependencyMap/__tests__/DependencyMapUtils.test.ts (1)
86-86: Fix typo in comment.There's a duplicated "and" in the comment.
- // JSObject1.myFun1 depends on both JSObject1.myFun2 and and Query2.data (transitive) + // JSObject1.myFun1 depends on both JSObject1.myFun2 and Query2.data (transitive)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app/client/src/entities/DependencyMap/__tests__/DependencyMapUtils.test.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: perform-test / server-build / server-unit-tests
- GitHub Check: perform-test / client-build / client-build
- GitHub Check: perform-test / rts-build / build
- GitHub Check: client-prettier / prettier-check
- GitHub Check: client-unit-tests / client-unit-tests
- GitHub Check: client-build / client-build
- GitHub Check: client-check-cyclic-deps / check-cyclic-dependencies
- GitHub Check: client-lint / client-lint
🔇 Additional comments (5)
app/client/src/entities/DependencyMap/__tests__/DependencyMapUtils.test.ts (5)
1-5: Imports look clean and appropriate.All necessary dependencies are imported correctly for the test scenarios.
7-24: Well-structured helper function for test setup.The
makeConfigTreeWithActionfunction creates a realistic mock ConfigTree with all required properties for an ACTION entity. Good separation of test setup logic.
26-50: Comprehensive test for direct dependency conflict.This test correctly validates that the function throws when a node directly depends on both
.runand.dataof the same ACTION entity. The setup and assertions are clear.
52-72: Good coverage of non-conflicting scenarios.Testing both
.run-only and.data-only dependencies ensures no false positives. Using separate DependencyMap instances for each scenario is the right approach.
74-101: Excellent test for transitive dependency detection.This test validates the core enhancement mentioned in the PR - detecting reactive dependency misuse through transitive relationships. The setup correctly creates a scenario where a node transitively depends on both
.runand.dataof the same ACTION entity.
Contributor
Author
|
/build-deploy-preview skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/15909895928. |
|
Deploy-Preview-URL: https://ce-41049.dp.appsmith.com |
ApekshaBhosale
suggested changes
Jun 27, 2025
Contributor
Author
|
/build-deploy-preview skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/15919298508. |
|
Deploy-Preview-URL: https://ce-41049.dp.appsmith.com |
ApekshaBhosale
approved these changes
Jun 27, 2025
2 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Updating the function definition for checking reactive cyclic dependencies to throw an error in all such scenarios and block multiple execute API calls.
Fixes #41048
Automation
/ok-to-test tags="@tag.All"
🔍 Cypress test results
Tip
🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/15919295485
Commit: 05c9f64
Cypress dashboard.
Tags:
@tag.AllSpec:
Fri, 27 Jun 2025 07:06:10 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
Refactor
New Features
Tests