chore: makeParentDependedOnChildren optimisation only affected nodes in the graph is computed#36161
Conversation
WalkthroughThe recent changes introduce a new static method in the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant DependencyMapUtils
participant DependencyMap
User->>DependencyMap: Update Dependency Map
DependencyMap->>DependencyMapUtils: linkAffectedChildNodesToParent(affectedNodes)
DependencyMapUtils->>DependencyMap: Link affected child nodes to parents
DependencyMap-->>User: Dependency Map Updated
Tip Announcements
Recent review detailsConfiguration used: .coderabbit.yaml Files selected for processing (3)
Files skipped from review as they are similar to previous changes (3)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
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
the
skip-changelog
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range, codebase verification and nitpick comments (1)
app/client/src/workers/common/DependencyMap/index.ts (1)
76-81: Helper Function for Affected Nodes ApprovedThe function
addingAffectedNodesToListis correctly implemented to add nodes to the set of affected nodes. It uses a straightforward approach which is efficient for its purpose.Consider using a more performant iteration method if the number of nodes is large.
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (3)
- app/client/src/entities/DependencyMap/DependencyMapUtils.ts (2 hunks)
- app/client/src/entities/DependencyMap/tests/index.test.ts (1 hunks)
- app/client/src/workers/common/DependencyMap/index.ts (8 hunks)
Additional comments not posted (5)
app/client/src/entities/DependencyMap/DependencyMapUtils.ts (2)
40-40: Comment Clarification ApprovedThe added comment clarifies the purpose of the method, enhancing code readability and maintainability.
52-69: New Method for Affected Nodes ApprovedThe method
linkChildToItsParentNodeForAffectedChildNodesis well-implemented, focusing on affected nodes to optimize performance. It correctly checks theaffectedSetbefore linking, which should significantly reduce unnecessary computations.Consider verifying the performance impact of this change with larger datasets to ensure scalability.
app/client/src/workers/common/DependencyMap/index.ts (2)
83-91: Encapsulation of Dependency Map Updates ApprovedThe function
addNodesToDependencyMapcorrectly encapsulates the logic for adding nodes to the dependency map and updating the affected nodes set. This improves maintainability and reduces duplication.Verify the handling of the
strictparameter in different scenarios to ensure it behaves as expected.
93-99: Setting Dependencies and Updating Affected Nodes ApprovedThe function
setDependenciesToDependencyMapcorrectly sets dependencies for a node and updates the affected nodes set. This separation of concerns enhances code clarity and maintainability.Consider checking the performance impact when handling a large number of dependencies to ensure scalability.
app/client/src/entities/DependencyMap/__tests__/index.test.ts (1)
2711-2773: Review of New Test Suite:linkChildToItsParentNodeForAffectedChildNodesThe new test suite introduced in this PR is well-structured and covers several important scenarios:
- Linking a child node to its parent node when the child node is affected: This test ensures that the parent node is correctly linked to the affected child node, which is crucial for maintaining the integrity of the dependency graph.
- Handling cases where no child nodes are affected: It's important to verify that no unnecessary links are created when there are no affected nodes, which this test successfully checks.
- Extending the linking logic to grandparent nodes: This test is particularly valuable as it ensures that the dependency linking is not just superficial but extends throughout the hierarchy, which is essential for complex dependency graphs.
Overall, these tests are crucial for ensuring that the new functionality works as expected and maintains the integrity of the dependency graph in various scenarios. The use of
Setfor affected nodes and the direct manipulation of dependencies within theDependencyMapare appropriate and align with the objectives of the PR.
rishabhrathod01
left a comment
rishabhrathod01
left a comment
There was a problem hiding this comment.
Other than above nit pick, code looks good to me
…in the graph is computed (appsmithorg#36161) ## Description Optimising `makeParentDependedOnChildren` function by executing `makeParentsDependOnChild` for only affected nodes in the dependencyMap graph. This has brought down the cumulative latency of execution by about 90% when testing it against a customer's app during page load. Fixes #`Issue Number` _or_ Fixes `Issue URL` > [!WARNING] > _If no issue exists, please create an issue first, and check with the maintainers if the issue is valid._ ## Automation /ok-to-test tags="@tag.All" ### 🔍 Cypress test results <!-- This is an auto-generated comment: Cypress test results --> > [!TIP] > 🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉 > Workflow run: <https://github.com/appsmithorg/appsmith/actions/runs/10791040934> > Commit: 902f465 > <a href="https://internal.appsmith.com/app/cypress-dashboard/rundetails-65890b3c81d7400d08fa9ee5?branch=master&workflowId=10791040934&attempt=1" target="_blank">Cypress dashboard</a>. > Tags: `@tag.All` > Spec: > <hr>Tue, 10 Sep 2024 12:20:44 UTC <!-- end of auto-generated comment: Cypress test results --> ## Communication Should the DevRel and Marketing teams inform users about this change? - [ ] Yes - [ ] No <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **New Features** - Introduced a method for linking child nodes to parent nodes in the dependency graph, allowing for more precise management of affected nodes. - Added new helper functions to streamline the addition of affected nodes and dependencies within the dependency map. - **Bug Fixes** - Enhanced testing coverage for the dependency linking logic, ensuring accurate representation of hierarchical relationships between nodes. - **Documentation** - Improved comments in existing methods to clarify functionality and enhance code readability. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
2 participants
Description
Optimising
makeParentDependedOnChildrenfunction by executingmakeParentsDependOnChildfor only affected nodes in the dependencyMap graph. This has brought down the cumulative latency of execution by about 90% when testing it against a customer's app during page load.Fixes #
Issue Numberor
Fixes
Issue URLWarning
If no issue exists, please create an issue first, and check with the maintainers if the issue is valid.
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/10791040934
Commit: 902f465
Cypress dashboard.
Tags:
@tag.AllSpec:
Tue, 10 Sep 2024 12:20:44 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
New Features
Bug Fixes
Documentation