sending js patches #34290
sending js patches #34290
Conversation
|
This PR has increased the number of cyclic dependencies by 1, when compared with the release branch. Refer this document to identify the cyclic dependencies introduced by this PR. |
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThis update enhances the functionality related to handling JS actions within the application. It introduces new action types for fetching JS actions, skips certain tests, refactors and optimizes existing functions, and modifies data structures for evaluating JS objects. These changes aim to improve the accuracy and efficiency of JS object handling in the evaluation process. Changes
Sequence Diagram(s)sequenceDiagram
participant UI
participant Saga as Evaluation Saga
participant EvalTree as evalTree.ts
participant Handler as evalTrigger.ts
UI ->> Saga: FETCH_JS_ACTIONS_FOR_PAGE_SUCCESS
Saga ->> EvalTree: Call evalTree function
EvalTree ->> EvalTree: Use mergeJSObjectsToUnevalTree and getAffectedJSObjectIds
EvalTree ->> Saga: Return unevalTree and jsPatches
Saga ->> Handler: Call evalAndValidateSubTree with jsPatches
Handler ->> EvalTree: Get and validate results
EvalTree ->> UI: Update UI with new evaluations
Poem
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 Configration 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: 2
Outside diff range and nitpick comments (11)
app/client/src/workers/Evaluation/handlers/evalTree.ts (1)
Line range hint
5-5: Shadowing the globalEvalErrorproperty can lead to confusion. Consider renaming this variable to avoid potential issues in debugging and maintenance.- import type { EvalError } from "utils/DynamicBindingUtils"; + import type { EvalError as DynamicBindingEvalError } from "utils/DynamicBindingUtils";app/client/src/sagas/EvaluationsSaga.ts (2)
Line range hint
29-29: Shadowing the globalEvalErrorproperty can lead to confusion. Consider renaming this variable to improve clarity and maintainability.- import type { EvalError, EvaluationError } from "utils/DynamicBindingUtils"; + import type { EvalError as DynamicBindingEvalError, EvaluationError } from "utils/DynamicBindingUtils";
Line range hint
233-233: Consider using an optional chain to ensure safety when accessing properties on potentially undefined objects.- dataTreeEvaluator?.oldUnEvalTree, + dataTreeEvaluator?.oldUnEvalTree?,app/client/src/workers/common/DataTreeEvaluator/index.ts (8)
Line range hint
3-3: Consider renaming theEvalErrortype to avoid shadowing the globalEvalErrorproperty. Shadowing global variables can lead to confusion and errors in code maintenance.- import type { EvalError } from "utils/DynamicBindingUtils"; + import type { EvaluationError as EvalError } from "utils/DynamicBindingUtils";
Line range hint
466-466: Remove the redundant double-negation to simplify the expression.- if (!!unevalTree[update]) { + if (unevalTree[update]) {
Line range hint
1005-1013: The default parameter should follow the last required parameter or should be a required parameter. This ensures that all required parameters are provided values when the function is called.- evaluateActionBindings(bindings: string[], executionParams?: Record<string, unknown> | string = {}) { + evaluateActionBindings(bindings: string[], executionParams: Record<string, unknown> | string = {}) {
Line range hint
1274-1279: Avoid using 'return' statements in 'finally' blocks. This can lead to unexpected behavior as it overwrites the control flow statements inside 'try' and 'catch'.- return { + // Removed return statement from finally block
Line range hint
1311-1347: The 'else' clause can be omitted because previous branches break early, simplifying the control flow.- } else { + // Removed unnecessary else clause
Line range hint
1422-1424: Omit the unnecessary 'else' clause to clean up and simplify the control flow.- } else { + // Removed unnecessary else clause
Line range hint
1570-1570: Change to an optional chain to simplify the expression and improve readability.- if (entity && isWidget(entity)) { + if (entity?.isWidget) {
Line range hint
1675-1676: Use optional chaining to simplify the code and enhance its readability.- if (entity && isAction(entity)) { + if (entity?.isAction) {
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (8)
- app/client/src/ce/sagas/InferAffectedJSObjects.ts (1 hunks)
- app/client/src/sagas/EvaluationsSaga.test.ts (1 hunks)
- app/client/src/sagas/EvaluationsSaga.ts (4 hunks)
- app/client/src/sagas/EvaluationsSagaUtils.test.ts (4 hunks)
- app/client/src/workers/Evaluation/handlers/evalTree.ts (5 hunks)
- app/client/src/workers/Evaluation/handlers/evalTrigger.ts (2 hunks)
- app/client/src/workers/Evaluation/types.ts (2 hunks)
- app/client/src/workers/common/DataTreeEvaluator/index.ts (5 hunks)
Files not reviewed due to errors (2)
- app/client/src/workers/Evaluation/types.ts (no review received)
- app/client/src/sagas/EvaluationsSagaUtils.test.ts (no review received)
Files skipped from review due to trivial changes (1)
- app/client/src/sagas/EvaluationsSaga.test.ts
Additional context used
Biome
app/client/src/workers/Evaluation/types.ts
[error] 7-7: Do not shadow the global "EvalError" property. (lint/suspicious/noShadowRestrictedNames)
Consider renaming this variable. It's easy to confuse the origin of variables when they're named after a known global.
app/client/src/workers/Evaluation/handlers/evalTree.ts
[error] 5-5: Do not shadow the global "EvalError" property. (lint/suspicious/noShadowRestrictedNames)
Consider renaming this variable. It's easy to confuse the origin of variables when they're named after a known global.
app/client/src/sagas/EvaluationsSaga.ts
[error] 29-29: Do not shadow the global "EvalError" property. (lint/suspicious/noShadowRestrictedNames)
Consider renaming this variable. It's easy to confuse the origin of variables when they're named after a known global.
[error] 233-233: Change to an optional chain. (lint/complexity/useOptionalChain)
Unsafe fix: Change to an optional chain.
[error] 643-643: This default parameter should follow the last required parameter or should be a required parameter. (lint/style/useDefaultParameterLast)
The last required parameter is here:
A default parameter that precedes a required parameter cannot be omitted at call site.
Unsafe fix: Turn the parameter into a required parameter.app/client/src/workers/common/DataTreeEvaluator/index.ts
[error] 3-3: Do not shadow the global "EvalError" property. (lint/suspicious/noShadowRestrictedNames)
Consider renaming this variable. It's easy to confuse the origin of variables when they're named after a known global.
[error] 466-466: Avoid redundant double-negation. (lint/complexity/noExtraBooleanCast)
It is not necessary to use double-negation when a value will already be coerced to a boolean.
Unsafe fix: Remove redundant double-negation
[error] 1005-1013: This default parameter should follow the last required parameter or should be a required parameter. (lint/style/useDefaultParameterLast)
The last required parameter is here:
A default parameter that precedes a required parameter cannot be omitted at call site.
Unsafe fix: Turn the parameter into a required parameter.
[error] 1274-1279: Unsafe usage of 'return'. (lint/correctness/noUnsafeFinally)
'return' in 'finally' overwrites the control flow statements inside 'try' and 'catch'.
[error] 1311-1347: This else clause can be omitted because previous branches break early. (lint/style/noUselessElse)
Unsafe fix: Omit the else clause.
[error] 1422-1424: This else clause can be omitted because previous branches break early. (lint/style/noUselessElse)
Unsafe fix: Omit the else clause.
[error] 1570-1570: Change to an optional chain. (lint/complexity/useOptionalChain)
Unsafe fix: Change to an optional chain.
[error] 1675-1676: Change to an optional chain. (lint/complexity/useOptionalChain)
Unsafe fix: Change to an optional chain.
Additional comments not posted (3)
app/client/src/workers/Evaluation/handlers/evalTrigger.ts (1)
4-4: EnsuregetJSEntitiesis properly tested to handle edge cases.app/client/src/workers/Evaluation/handlers/evalTree.ts (1)
69-82: The functiongetAffectedJSObjectIdscorrectly computes the affected JS object IDs based on the patches. However, ensure that eachpatch.valueactually contains anactionIdto avoid potential runtime errors.app/client/src/sagas/EvaluationsSaga.ts (1)
250-290: The functionseperateOutAffectedJSactionseffectively separates JS actions from the unevaluated tree. However, consider using TypeScript's utility types for better type safety and readability.
[REFACTOR_Suggestion]- affectedJSObjects: AffectedJSObjects, + affectedJSObjects: { ids: string[], isAllAffected: boolean },
| [ | ||
| ...Object.values( | ||
| getJSEntities(dataTreeEvaluator.getOldUnevalTree()), | ||
| ).map((v) => v.actionId), | ||
| ...Object.values(getJSEntities(unEvalTree.unEvalTree)).map( | ||
| (v) => v.actionId, | ||
| ), | ||
| ], |
There was a problem hiding this comment.
Optimize the array construction to avoid potential performance issues with large datasets.
- [...Object.values(getJSEntities(dataTreeEvaluator.getOldUnevalTree())).map((v) => v.actionId),
- ...Object.values(getJSEntities(unEvalTree.unEvalTree)).map((v) => v.actionId)]
+ const oldActions = Object.values(getJSEntities(dataTreeEvaluator.getOldUnevalTree())).map(v => v.actionId);
+ const newActions = Object.values(getJSEntities(unEvalTree.unEvalTree)).map(v => v.actionId);
+ const combinedActions = [...new Set([...oldActions, ...newActions])];This change avoids creating intermediate arrays and ensures uniqueness, potentially improving memory usage and performance.
Committable suggestion was skipped due to low confidence.
| const mergeJSObjectsToUnevalTree = ( | ||
| prevUevalTree: DataTree | undefined, | ||
| currUnevalTree: DataTree, | ||
| jsPatches: any, | ||
| ) => { | ||
| const { patches, shouldReplaceAllNodes } = jsPatches; | ||
| const unevalTreeWithPatches = patches.reduce( | ||
| (acc: DataTree, patch: any) => { | ||
| acc[patch.path] = patch.value; | ||
| return acc; | ||
| }, | ||
| { ...currUnevalTree }, | ||
| ); | ||
| if (shouldReplaceAllNodes) { | ||
| return unevalTreeWithPatches; | ||
| } | ||
| const prevJSObjects = getJSEntities(prevUevalTree || {}); | ||
| return { ...prevJSObjects, ...unevalTreeWithPatches }; | ||
| }; |
There was a problem hiding this comment.
The function mergeJSObjectsToUnevalTree integrates JS patches into the unevaluated tree. The logic appears correct, but consider using more descriptive type annotations for jsPatches to enhance code clarity and safety.
- jsPatches: any,
+ jsPatches: { patches: Array<{ path: string; value: any }>; shouldReplaceAllNodes: boolean },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.
| const mergeJSObjectsToUnevalTree = ( | |
| prevUevalTree: DataTree | undefined, | |
| currUnevalTree: DataTree, | |
| jsPatches: any, | |
| ) => { | |
| const { patches, shouldReplaceAllNodes } = jsPatches; | |
| const unevalTreeWithPatches = patches.reduce( | |
| (acc: DataTree, patch: any) => { | |
| acc[patch.path] = patch.value; | |
| return acc; | |
| }, | |
| { ...currUnevalTree }, | |
| ); | |
| if (shouldReplaceAllNodes) { | |
| return unevalTreeWithPatches; | |
| } | |
| const prevJSObjects = getJSEntities(prevUevalTree || {}); | |
| return { ...prevJSObjects, ...unevalTreeWithPatches }; | |
| }; | |
| const mergeJSObjectsToUnevalTree = ( | |
| prevUevalTree: DataTree | undefined, | |
| currUnevalTree: DataTree, | |
| jsPatches: { patches: Array<{ path: string; value: any }>; shouldReplaceAllNodes: boolean }, | |
| ) => { | |
| const { patches, shouldReplaceAllNodes } = jsPatches; | |
| const unevalTreeWithPatches = patches.reduce( | |
| (acc: DataTree, patch: any) => { | |
| acc[patch.path] = patch.value; | |
| return acc; | |
| }, | |
| { ...currUnevalTree }, | |
| ); | |
| if (shouldReplaceAllNodes) { | |
| return unevalTreeWithPatches; | |
| } | |
| const prevJSObjects = getJSEntities(prevUevalTree || {}); | |
| return { ...prevJSObjects, ...unevalTreeWithPatches }; | |
| }; |
vsvamsi1
added
ok-to-test
|
This PR has increased the number of cyclic dependencies by 1, when compared with the release branch. Refer this document to identify the cyclic dependencies introduced by this PR. |
vsvamsi1
added
ok-to-test
|
This PR has increased the number of cyclic dependencies by 1, when compared with the release branch. Refer this document to identify the cyclic dependencies introduced by this PR. |
vsvamsi1
added
ok-to-test
|
This PR has increased the number of cyclic dependencies by 2, when compared with the release branch. Refer this document to identify the cyclic dependencies introduced by this PR. |
Description
Tip
Add a TL;DR when the description is longer than 500 words or extremely technical (helps the content, marketing, and DevRel team).
Please also include relevant motivation and context. List any dependencies that are required for this change. Add links to Notion, Figma or any other documents that might be relevant to the PR.
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/9582983286
Commit: 43eb900
Cypress dashboard.
Tags: ``
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Refactor
Chores