Conversation
WalkthroughThe changes update both test and production code within the DataTreeEvaluator module. In the test file, the strict assertion on the call count for Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant DataTreeEvaluator
Caller->>DataTreeEvaluator: setupTree()
DataTreeEvaluator->>DataTreeEvaluator: getEvaluationOrderAndSetEvalTreeWithNewUnevalTreeValues(...)
DataTreeEvaluator->>DataTreeEvaluator: setOldUnevalTree(newUnEvalTree)
Note right of DataTreeEvaluator: Evaluation order set with updated unevaluated tree values
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (8)
🔇 Additional comments (7)
🪧 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
vsvamsi1
added
ok-to-test
vsvamsi1
force-pushed
the
test71
branch
2 times, most recently
from
8e9b8ef to
790c135
Compare
vsvamsi1
added
ok-to-test
vsvamsi1
added
ok-to-test
## Description We earlier had an issue where the oldUnEvalTree was getting mutated, it happened because of some properties of updatedUnevalTree were getting tied to evalTree in the getEvaluationOrder function. So any mutation on evalTree which definitely will happen in the subsequent steps in the evalAndValidateSubTree function would affect oldUnEvalTree. To prevent this mutation we previously performed a deepClone which had a cost in performance and this executed in evalTreeWithChanges and evalTree further exacerbating performance. To fix it we performed a deepClone on specific paths of updatedUnevalTree to evalTree thereby when make an assignment of updatedUnevalTree to oldUnvalTree these deepClones isolate mutations. This has led to a 0.3 seconds reduction in LCP and reduces the overall webworker scripting by 9-10%. 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/14223540588> > Commit: c1006f7 > <a href="https://internal.appsmith.com/app/cypress-dashboard/rundetails-65890b3c81d7400d08fa9ee5?branch=master&workflowId=14223540588&attempt=1" target="_blank">Cypress dashboard</a>. > Tags: `@tag.All` > Spec: > <hr>Wed, 02 Apr 2025 16:44:05 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 ## Summary by CodeRabbit - **Tests** - Updated evaluation test assertions to focus on performance expectations rather than strict operation counts. - **New Features** - Introduced a method to directly set historical evaluation data, improving clarity and functionality in the evaluation workflow. - Renamed and modified the evaluation order method to enhance clarity regarding its functionality. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
3 participants
Description
We earlier had an issue where the oldUnEvalTree was getting mutated, it happened because of some properties of updatedUnevalTree were getting tied to evalTree in the getEvaluationOrder function. So any mutation on evalTree which definitely will happen in the subsequent steps in the evalAndValidateSubTree function would affect oldUnEvalTree. To prevent this mutation we previously performed a deepClone which had a cost in performance and this executed in evalTreeWithChanges and evalTree further exacerbating performance.
To fix it we performed a deepClone on specific paths of updatedUnevalTree to evalTree thereby when make an assignment of updatedUnevalTree to oldUnvalTree these deepClones isolate mutations.
This has led to a 0.3 seconds in LCP and reduces the overall webworker scripting by 9-10%.
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/14223540588
Commit: c1006f7
Cypress dashboard.
Tags:
@tag.AllSpec:
Wed, 02 Apr 2025 16:44:05 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
Summary by CodeRabbit