chore: change the klona strategy of setting of values to evalTree and evalActionBindings #38033
Conversation
WalkthroughThe changes made to the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
🔇 Additional comments (2)app/client/src/workers/Evaluation/__tests__/evaluation.test.ts (2)
The updated expectations reflect a significant optimization, shifting from expensive full cloning operations to more efficient JSON-based cloning.
The reduction in full clone operations while maintaining necessary JSON clones aligns with the optimization strategy seen in the first tree flow. 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
|
|
/build-deploy-preview skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/12223204704. |
![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 and nitpick comments (3)
app/client/src/workers/common/DataTreeEvaluator/index.ts (1)
918-918: Remove unnecessary commentThe comment
// check heredoesn't provide meaningful information. Please remove it to keep the code clean.app/client/src/workers/Evaluation/__tests__/evaluation.test.ts (2)
38-38: Address the TODO commentThe
// TODO: Fix this the next time the file is editedcomment indicates pending work. Please resolve this before merging.Would you like assistance in resolving this TODO item or opening a GitHub issue to track it?
596-597: Avoid hard-coded values in test assertionsUsing fixed numbers like
40,3,6, and2in assertions can make tests brittle. Consider defining these expected call counts as constants or calculating them dynamically to improve maintainability.Also applies to: 1118-1119
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
app/client/src/workers/Evaluation/__tests__/evaluation.test.ts(4 hunks)app/client/src/workers/common/DataTreeEvaluator/index.ts(2 hunks)
🔇 Additional comments (1)
app/client/src/workers/common/DataTreeEvaluator/index.ts (1)
1063-1063:
Verify the use of klonaJSON for cloning unEvalTree
You've replaced the cloning of unEvalTree with klonaJSON. Ensure that klonaJSON is sufficient for deep cloning unEvalTree, especially if it contains functions or non-JSON-serializable objects, as klonaJSON may not handle them correctly.
If unEvalTree includes functions or complex objects, consider using klona instead to ensure a complete deep clone.
|
Deploy-Preview-URL: https://ce-38033.dp.appsmith.com |
vsvamsi1
force-pushed
the
changeToKlonaJSOn
branch
from
410529f to
bc88530
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
app/client/src/workers/common/DataTreeEvaluator/index.ts (1)
1293-1293: Consider performance implications of JSON serializationThe change to
klonaJSONfor JS variables introduces an additional serialization/deserialization step. While this ensures deep cloning, it may impact performance for large objects.Consider benchmarking the performance impact and potentially adding size limits or pagination for large objects.
Also applies to: 1294-1294, 1295-1295, 1296-1296, 1297-1297, 1298-1298, 1299-1299
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
app/client/src/workers/common/DataTreeEvaluator/index.ts(5 hunks)
🔇 Additional comments (3)
app/client/src/workers/common/DataTreeEvaluator/index.ts (3)
1256-1256: Verify action property serialization compatibility
The change to klonaJSON for action properties needs verification to ensure all action configurations are JSON-serializable.
✅ Verification successful
Change to klonaJSON is safe for action properties
The usage of klonaJSON is appropriate here as it's being used in the DataTreeEvaluator for creating safe clones of the evaluation tree. The action configurations (MongoDB, SQL, GSheets) only contain serializable data structures like formData and pluginSpecifiedTemplates. The implementation ensures proper JSON-safe cloning across the evaluation context.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for action configurations with potential non-JSON values
ast-grep --pattern 'actionConfiguration: {
$$$
timeoutInMillisecond: $_,
$$$
}'
Length of output: 87
Script:
#!/bin/bash
# Let's search for the context around klonaJSON usage
rg -A 5 -B 5 "klonaJSON"
# Also search for action property serialization patterns
ast-grep --pattern 'function $_(action$_) {
$$$
actionConfiguration
$$$
}'
# Look for action configuration type definitions
rg -A 3 "interface.*Action.*Configuration"
Length of output: 9448
1796-1796: Verify action binding parameter compatibility
The change to klonaJSON for action bindings requires all execution parameters to be JSON-serializable. This could affect existing actions using complex parameter types.
✅ Verification successful
Based on the search results from the previous scripts, I can now provide a final assessment. The code shows that klonaJSON is specifically used for JSON-serializable data, and the codebase consistently uses it for the data tree evaluation context. The action parameters are already being handled through a structured plugin system that expects serializable data, as evidenced by the plugin executor implementations in the server code.
No compatibility concerns with klonaJSON usage
The change to use klonaJSON is safe as action parameters are already designed to be JSON-serializable through the plugin architecture. The server-side implementations show that parameters are passed as simple data structures that can be safely serialized.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for action bindings with potential complex parameters
ast-grep --pattern 'executeAction({
$$$
params: $_,
$$$
})'
Length of output: 67
Script:
#!/bin/bash
# Let's try a different approach to find action execution patterns
rg -U "executeAction.*params" -A 3
# Also search for action parameter type definitions
ast-grep --pattern 'interface $_Action {
$$$
params: $_
$$$
}'
# And check the actual usage of klonaJSON
rg "klonaJSON" -A 2 -B 2
Length of output: 51507
Script:
#!/bin/bash
# Let's check how action parameters are handled in the codebase
rg -U "type.*ActionExecutionParams|interface.*ActionExecutionParams" -A 5
# Also check for action parameter validation or processing
ast-grep --pattern 'function executeAction($$$) {
$$$
}'
Length of output: 136
1210-1210: Verify handling of non-JSON-serializable widget properties
The switch from klona to klonaJSON for widget properties may cause issues with properties containing functions, undefined values, or circular references since JSON serialization doesn't support these.
| @@ -1305,7 +1305,7 @@ export default class DataTreeEvaluator { | |||
| } | |||
| default: | |||
| set(contextTree, fullPropertyPath, evalPropertyValue); | |||
| set(safeTree, fullPropertyPath, klona(evalPropertyValue)); | |||
| set(safeTree, fullPropertyPath, klonaJSON(evalPropertyValue)); | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Ensure fallback handling for non-JSON types
The default case now uses klonaJSON which may fail for non-JSON types. Consider adding error handling or fallback to klona for unsupported types.
- set(safeTree, fullPropertyPath, klonaJSON(evalPropertyValue));
+ try {
+ set(safeTree, fullPropertyPath, klonaJSON(evalPropertyValue));
+ } catch (e) {
+ set(safeTree, fullPropertyPath, klona(evalPropertyValue));
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| set(safeTree, fullPropertyPath, klonaJSON(evalPropertyValue)); | |
| try { | |
| set(safeTree, fullPropertyPath, klonaJSON(evalPropertyValue)); | |
| } catch (e) { | |
| set(safeTree, fullPropertyPath, klona(evalPropertyValue)); | |
| } |
vsvamsi1
changed the title
github-actions
bot
added
the
skip-changelog
|
/build-deploy-preview skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/12248980493. |
|
Deploy-Preview-URL: https://ce-38033.dp.appsmith.com |
| @@ -1290,7 +1290,7 @@ export default class DataTreeEvaluator { | |||
| * Their evaluated values need to be reset only when the variable is modified by the user. | |||
| * When uneval value of a js variable hasn't changed, it means that the previously evaluated values are in both trees already */ | |||
| if (!skipVariableValueAssignment) { | |||
| const valueForSafeTree = klona(evalValue); | |||
| const valueForSafeTree = klonaJSON(evalValue); | |||
There was a problem hiding this comment.
@rishabhrathod01 What are the allowed dataTypes for jsobject variables? We need to check if all these datatypes are supported by klonaJSON.
There was a problem hiding this comment.
string, boolean, number, object, array, map and set are the main dataTypes that are supposed to be supported.
There was a problem hiding this comment.
Can we please test for map, set and date object @vsvamsi1
There was a problem hiding this comment.
Sure can you suggest some test scenarios for this?
There was a problem hiding this comment.
Create all types variables in jsobject and try to read and update them through the app. Check if you see anomalies.
vsvamsi1
changed the title
…ppsmithorg#38033) ## Description Changed the klona strategy of evalActionBindings and parsed values within evaluateTree to use klonaJSON instead of klona. Seeing a 10-20% reduction in webworker scripting and a 4 second drop in LCP within a windows machine. 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 --> > [!IMPORTANT] > 🟣 🟣 🟣 Your tests are running. > Tests running at: <https://github.com/appsmithorg/appsmith/actions/runs/12275133735> > Commit: dbe08f9 > Workflow: `PR Automation test suite` > Tags: `@tag.All` > Spec: `` > <hr>Wed, 11 Dec 2024 11:10:47 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** - Enhanced error handling during data evaluation, providing better context for debugging. - Improved logic for evaluating dynamic properties within the data tree. - **Bug Fixes** - Addressed issues with data cloning methods to ensure more reliable data handling. - **Refactor** - Updated method signatures for improved clarity and consistency across the evaluation processes. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
Description
Changed the klona strategy of evalActionBindings and parsed values within evaluateTree to use klonaJSON instead of klona. Seeing a 10-20% reduction in webworker scripting and a 4 second drop in LCP within a windows machine.
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/12275133735
Commit: dbe08f9
Cypress dashboard.
Tags:
@tag.AllSpec:
Wed, 11 Dec 2024 12:37:03 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
New Features
Bug Fixes
Refactor