chore: allKeys computation is constrained by diff #35303
Conversation
|
Important Review skippedAuto reviews are limited to specific labels. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThe recent updates introduce the Changes
Sequence Diagram(s)sequenceDiagram
participant A as Records
participant B as Diff (DataTreeDiff)
participant C as Previous Result
participant D as New Result
A->>B: Provides data updates
B->>C: Checks event type (DELETE or NEW)
C-->>D: Updates paths accordingly
D-->>C: Returns updated paths
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 as PR comments)
Additionally, you can add CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (2)
- app/client/src/ce/workers/Evaluation/evaluationUtils.ts (1 hunks)
- app/client/src/workers/common/DataTreeEvaluator/index.ts (2 hunks)
Additional comments not posted (1)
app/client/src/workers/common/DataTreeEvaluator/index.ts (1)
664-668: Excellent integration of dynamic path management!The replacement of
getAllPathswithgetAllPathsBasedOnDiffPathsenhances the function's ability to handle differential updates. However, consider the following improvements:
- Error Handling: Ensure that any potential errors from
getAllPathsBasedOnDiffPathsare properly handled to prevent unexpected failures.- Performance Monitoring: Monitor the performance impact of this change, especially if the
diffarray is large. Consider adding logging to track execution time.
| export const getAllPathsBasedOnDiffPaths = ( | ||
| records: any, | ||
| diff: DataTreeDiff[], | ||
| previousResult: Record<string, true> = {}, | ||
| ): Record<string, true> => { | ||
| const newResult = previousResult; | ||
| diff.forEach((curr) => { | ||
| const { event, payload } = curr; | ||
| if (event === DataTreeDiffEvent.DELETE) { | ||
| delete newResult[payload.propertyPath]; | ||
| } | ||
| if (event === DataTreeDiffEvent.NEW) { | ||
| const newDataSegments = get(records, payload.propertyPath); | ||
| // directly mutates on the result so we don't have to merge it back to the result | ||
| getAllPaths(newDataSegments, payload.propertyPath, newResult); | ||
| } | ||
| }); | ||
|
|
||
| return newResult; | ||
| }; |
There was a problem hiding this comment.
Great job on implementing dynamic path management!
The function getAllPathsBasedOnDiffPaths is well-structured and handles differential updates effectively. However, consider the following improvements:
- Optimize Mutation: Directly mutating
newResultcan be risky. Consider creating a copy ofpreviousResultto avoid unintended side effects. - Add Comments: Adding comments to explain the logic, especially for the recursive call to
getAllPaths, would improve readability. - Error Handling: Include error handling to manage unexpected cases, such as invalid
propertyPathorrecords.
export const getAllPathsBasedOnDiffPaths = (
records: any,
diff: DataTreeDiff[],
previousResult: Record<string, true> = {},
): Record<string, true> => {
const newResult = { ...previousResult };
diff.forEach((curr) => {
const { event, payload } = curr;
if (event === DataTreeDiffEvent.DELETE) {
delete newResult[payload.propertyPath];
}
if (event === DataTreeDiffEvent.NEW) {
const newDataSegments = get(records, payload.propertyPath);
// directly mutates on the result so we don't have to merge it back to the result
getAllPaths(newDataSegments, payload.propertyPath, newResult);
}
});
return newResult;
};Committable suggestion was skipped due to low confidence.
github-actions
bot
added
the
skip-changelog
vsvamsi1
added
ok-to-test
vsvamsi1
added
ok-to-test
vsvamsi1
changed the title
|
@vsvamsi1 Do we have plan for adding unit test cases as well? |
vsvamsi1
added
ok-to-test
vsvamsi1
added
ok-to-test
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (2)
- app/client/src/ce/workers/Evaluation/evaluationUtils.ts (1 hunks)
- app/client/src/workers/common/DataTreeEvaluator/index.ts (2 hunks)
Files skipped from review as they are similar to previous changes (2)
- app/client/src/ce/workers/Evaluation/evaluationUtils.ts
- app/client/src/workers/common/DataTreeEvaluator/index.ts
There was a problem hiding this comment.
Actionable comments posted: 1
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (2)
- app/client/src/ce/workers/Evaluation/evaluationUtils.ts (1 hunks)
- app/client/src/workers/common/DataTreeEvaluator/utils.test.ts (2 hunks)
Files skipped from review as they are similar to previous changes (1)
- app/client/src/ce/workers/Evaluation/evaluationUtils.ts
Additional context used
Biome
app/client/src/workers/common/DataTreeEvaluator/utils.test.ts
[error] 168-168: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 184-184: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
Additional comments not posted (6)
app/client/src/workers/common/DataTreeEvaluator/utils.test.ts (6)
7-11: Good job updating the imports.The import statements correctly include
DataTreeDiffEventandgetAllPathsBasedOnDiffPaths, which are necessary for the new test suite.
109-109: Excellent addition of the test suite forgetAllPathsBasedOnDiffPaths.The new test suite is well-structured and covers various scenarios, ensuring comprehensive testing of the function.
145-158: Great job on the test case for adding a new widget.This test case effectively verifies that all paths are generated correctly when a new widget is added.
159-165: Well done on the test case for no diffs.This test case correctly verifies that
allKeysremains unchanged when there are no diffs.
190-216: Great test case for adding a node.This test case effectively verifies that the correct paths are added when a node is added.
218-241: Well done on the test case for editing a node.This test case correctly verifies that the correct paths are generated when a node's value changes from a simple primitive to a collection.
| test("should delete the correct paths within allKeys when a node within a widget is deleted", () => { | ||
| const deletedWidgetName = produce(initialTree, (draft: any) => { | ||
| delete draft.WidgetName.name; | ||
| }); | ||
| const updatedAllKeys = getAllPathsBasedOnDiffPaths( | ||
| deletedWidgetName, | ||
| [ | ||
| { | ||
| event: DataTreeDiffEvent.DELETE, | ||
| payload: { | ||
| propertyPath: "WidgetName.name", | ||
| }, | ||
| }, | ||
| ], | ||
| // we have to make a copy since allKeys is mutable | ||
| { ...initialAllKeys }, | ||
| ); | ||
| const deletedWidgetNameAllKeys = produce(initialAllKeys, (draft: any) => { | ||
| delete draft["WidgetName.name"]; | ||
| }); | ||
|
|
||
| expect(deletedWidgetNameAllKeys).toEqual(updatedAllKeys); | ||
| }); |
There was a problem hiding this comment.
Good test case for deleting a node, but consider optimizing performance.
This test case correctly verifies that the correct paths are deleted when a node is deleted. However, using the delete operator can impact performance. Consider using an undefined assignment instead.
- delete draft.WidgetName.name;
+ draft.WidgetName.name = undefined;Similarly, update the deletion in deletedWidgetNameAllKeys:
- delete draft["WidgetName.name"];
+ draft["WidgetName.name"] = undefined;Tools
Biome
[error] 168-168: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 184-184: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
vsvamsi1
added
ok-to-test
vsvamsi1
added
ok-to-test
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (1)
- app/client/src/ce/workers/Evaluation/evaluationUtils.ts (2 hunks)
Files skipped from review as they are similar to previous changes (1)
- app/client/src/ce/workers/Evaluation/evaluationUtils.ts
github-actions
bot
added
Performance
Description
allKeys is previously computed from the complete unevalTree, we used to recursively traverse through the entire unevalTree during each evaluation update cycle. We are optimising this by leveraging the diff which we have previously computed and using the diff to directly update the allKeys result.
When delete based diffs detected are detected we are directly deleting the nodes in the previous allKeys result, for only new nodes we are recursively computing the allKeys for those nodes and merging it back to the previous allKeys result. The time complexity of the solution has improved by limiting iterations to the diff alone instead of computing the entire unevalTree.
Fixes #35386
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
Tip
🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/10220298181
Commit: 12b94c3
Cypress dashboard.
Tags:
@tag.AllSpec:
Fri, 02 Aug 2024 19:22:32 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Bug Fixes
Tests