Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chore: Add evalTreeWithDiff to evalWorkerAction #34403

Merged
merged 16 commits into from
Jun 27, 2024

Conversation

rishabhrathod01
Copy link
Contributor

@rishabhrathod01 rishabhrathod01 commented Jun 22, 2024

Description

These changes add evalTreeWithDiff to evalWorkerAction. Using this method it will be possible to send the unevalTree and configTree updates from mainThread to trigger evaluation.

  • This will skip diffing complete unEvaltree

Next steps after this PR

  • Rename EvalTreeWithChanges to evalTreeWithDiff
  • Generate Diff instead of updatedValuePaths for the current evalTreeWithChanges references.
  • Handle evalTreeWithDiff for
    • JSObject updates
    • updateDependencyMap

Fixes #

Automation

/test js

🔍 Cypress test results

Tip

🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/9684706533
Commit: 3de988a
Cypress dashboard.
Tags: @tag.JS

Communication

Should the DevRel and Marketing teams inform users about this change?

  • Yes
  • No

Summary by CodeRabbit

  • New Features

    • Enhanced widget evaluation logic and improved handling of dynamic bindings and dependencies.
  • Bug Fixes

    • Corrected type handling in various evaluation functions to improve stability and accuracy.
  • Tests

    • Updated test cases for widget property setters to include additional tags for better categorization.
  • Refactor

    • Streamlined function signatures and improved type safety across evaluation utilities.

Copy link
Contributor

coderabbitai bot commented Jun 22, 2024

Walkthrough

The updates apply general enhancements and refactorings across multiple files, focusing on type improvements, method signature adjustments, and better handling of JavaScript evaluations and dependencies. Key changes include the introduction of more specific type handling, renaming types for clarity, added utility functions, and reorganized logic for handling evaluations in response to tree updates.

Changes

File Path Change Summary
app/client/src/UITelemetry/generateWebWorkerTraces.ts Updated profileFn to use generic types.
app/client/src/ce/workers/Evaluation/evaluationUtils.ts Introduced import of Difference from "microdiff" and updated function to use new type.
app/client/src/workers/common/DataTreeEvaluator/index.ts Major renaming of types (e.g., EvalError to TEvalError), updated function signatures, and improved JS objects handling.
app/client/src/workers/common/DataTreeEvaluator/utils.ts Added getIsNewWidgetAdded function and new imports.
app/client/src/workers/common/DependencyMap/test.ts Removed imports and declarations for ButtonWidget and SelectWidget.
app/client/src/workers/Evaluation/handlers/evalTree.ts Modified evalTree to use specific request and response types.
app/client/src/workers/Evaluation/evalTreeWithChanges.ts Refactored evalTreeWithChanges to accept structured request object, moving pushResponseToMainThread to the bottom.
app/client/src/workers/Evaluation/JSObject/JSVariableUpdates.ts Added import and updated logic for applyJSVariableUpdatesToEvalTree.
app/client/src/workers/Evaluation/handlers/updateActionData.ts Updated handling of action data updates, importing new constants and updating function calls.
app/client/src/workers/Evaluation/setters.ts Modified function call to evalTreeWithChanges in the Setters class.
app/client/src/workers/common/DataTreeEvaluator/utils.test.ts Updated imports and adjusted references to use string literals.
app/client/cypress/e2e/Regression/ClientSide/SetProperty/SetOptions_Spec.ts Updated test descriptions to include @tag.JS.
app/client/cypress/e2e/Regression/ClientSide/SetProperty/WidgetPropertySetters1_spec.ts Added tag @tag.JS to test descriptions.
app/client/cypress/e2e/Regression/ClientSide/SetProperty/WidgetPropertySetters2_spec.ts Included tag @tag.JS in test descriptions.

Poem

In lines of code, the changes do glide,
Types and functions now clarified.
With imports new and logic keen,
Refactored scripts in a code serene.
On digital plains where bytes do dream,
🌟 A smoother run, in software's theme. 🌟


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?

Share
Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai generate interesting stats about this repository and render them as a table.
    • @coderabbitai show all the console.log statements in this repository.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

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)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Additionally, you can add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.

CodeRabbit Configration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

@rishabhrathod01 rishabhrathod01 changed the title chore: Add evalTreeWithChanges to evalWorkerAction chore: Add evalTreeWithDiff to evalWorkerAction Jun 22, 2024
@github-actions github-actions bot added the skip-changelog Adding this label to a PR prevents it from being listed in the changelog label Jun 24, 2024
@rishabhrathod01 rishabhrathod01 requested a review from vsvamsi1 June 24, 2024 18:49
@rishabhrathod01 rishabhrathod01 self-assigned this Jun 24, 2024
Copy link

⚠️ Cyclic Dependency Check:

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.

Copy link

⚠️ Cyclic Dependency Check:

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.

@rishabhrathod01 rishabhrathod01 added the ok-to-test Required label for CI label Jun 24, 2024
@rishabhrathod01 rishabhrathod01 marked this pull request as ready for review June 25, 2024 07:18
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 4

Outside diff range and nitpick comments (6)
app/client/src/workers/Evaluation/setters.ts (1)

Line range hint 85-85: Consider using optional chaining for better safety and readability.

As suggested by static analysis, converting to optional chaining here can prevent potential runtime errors when accessing deeply nested properties.

-    if (validationPaths) {
+    if (validationPaths?) {
app/client/src/workers/Evaluation/__tests__/evaluation.test.ts (1)

Line range hint 29-286: Remove exports from test file.

Test files should not contain exports as they are meant to be isolated units of code. The presence of exports may indicate that the test file is being used improperly, such as shared logic or configurations which should be refactored into separate modules.

- export const WIDGET_CONFIG_MAP: WidgetTypeConfigMap = { ... };
- export const BASE_WIDGET = { ... };
- export const BASE_WIDGET_CONFIG = { ... };
- export const BASE_ACTION: ActionEntity = { ... };
- export const BASE_ACTION_CONFIG: ActionEntityConfig = { ... };
app/client/src/workers/common/DataTreeEvaluator/index.ts (4)

Line range hint 991-999: Consider reordering default parameters to follow best practices.

Default parameters should be listed after all required parameters. This ensures that they can be omitted when the function is called, which is not the case if a default parameter precedes a required parameter.

- function example(a = 10, b) { ... }
+ function example(b, a = 10) { ... }

Line range hint 1260-1265: Avoid using 'return' inside 'finally' blocks.

Using 'return' inside a 'finally' block can lead to unexpected behavior by overriding the return values of 'try' or 'catch' blocks. Consider restructuring the logic to avoid this pattern.

try {
  // logic here
} catch (error) {
  // handle error
} finally {
  // cleanup code, no return statement
}

Line range hint 1297-1333: Remove unnecessary 'else' clause after early exit.

The 'else' clause is redundant here because the preceding branches of the 'if' or 'switch' statement terminate early (e.g., with a 'return', 'break', or 'continue'). Removing the 'else' clause can simplify the code and reduce nesting.

if (condition) {
  return value;
}
- else {
  // other logic
- }

Line range hint 1549-1549: Use optional chaining for cleaner and safer code.

Optional chaining (?.) can be used here to safely access deeply nested properties without having to check each level for null or undefined.

- if (object && object.property && object.property.subProperty) {
+ if (object?.property?.subProperty) {
Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

Commits

Files that changed from the base of the PR and between dc79ebb and 5e72062.

Files selected for processing (11)
  • app/client/src/ce/workers/Evaluation/evalWorkerActions.ts (1 hunks)
  • app/client/src/utils/DynamicBindingUtils.ts (1 hunks)
  • app/client/src/workers/Evaluation/JSObject/JSVariableUpdates.ts (2 hunks)
  • app/client/src/workers/Evaluation/tests/evaluation.test.ts (1 hunks)
  • app/client/src/workers/Evaluation/evalTreeWithChanges.ts (4 hunks)
  • app/client/src/workers/Evaluation/handlers/evalTree.ts (3 hunks)
  • app/client/src/workers/Evaluation/handlers/index.ts (1 hunks)
  • app/client/src/workers/Evaluation/handlers/updateActionData.ts (2 hunks)
  • app/client/src/workers/Evaluation/setters.ts (2 hunks)
  • app/client/src/workers/common/DataTreeEvaluator/index.ts (19 hunks)
  • app/client/src/workers/common/DataTreeEvaluator/utils.ts (2 hunks)
Files skipped from review due to trivial changes (1)
  • app/client/src/ce/workers/Evaluation/evalWorkerActions.ts
Additional context used
Biome
app/client/src/workers/common/DataTreeEvaluator/utils.ts

[error] 55-55: Unnecessary continue statement (lint/correctness/noUnnecessaryContinue)

Unsafe fix: Delete the unnecessary continue statement


[error] 56-63: This else clause can be omitted because previous branches break early. (lint/style/noUselessElse)

Unsafe fix: Omit the else clause.


[error] 87-87: 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

app/client/src/workers/Evaluation/setters.ts

[error] 85-85: Change to an optional chain. (lint/complexity/useOptionalChain)

Unsafe fix: Change to an optional chain.

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/utils/DynamicBindingUtils.ts

[error] 114-116: This else clause can be omitted because previous branches break early. (lint/style/noUselessElse)

Unsafe fix: Omit the else clause.


[error] 174-175: Change to an optional chain. (lint/complexity/useOptionalChain)

Unsafe fix: Change to an optional chain.


[error] 188-189: Change to an optional chain. (lint/complexity/useOptionalChain)

Unsafe fix: Change to an optional chain.


[error] 210-211: Change to an optional chain. (lint/complexity/useOptionalChain)

Unsafe fix: Change to an optional chain.


[error] 221-222: Change to an optional chain. (lint/complexity/useOptionalChain)

Unsafe fix: Change to an optional chain.


[error] 234-235: Change to an optional chain. (lint/complexity/useOptionalChain)

Unsafe fix: Change to an optional chain.


[error] 248-249: Change to an optional chain. (lint/complexity/useOptionalChain)

Unsafe fix: Change to an optional chain.


[error] 263-263: Change to an optional chain. (lint/complexity/useOptionalChain)

Unsafe fix: Change to an optional chain.


[error] 505-505: 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] 543-543: 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] 562-562: 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

app/client/src/workers/Evaluation/__tests__/evaluation.test.ts

[error] 29-232: Do not export from a test file. (lint/suspicious/noExportsInTest)


[error] 233-250: Do not export from a test file. (lint/suspicious/noExportsInTest)


[error] 251-257: Do not export from a test file. (lint/suspicious/noExportsInTest)


[error] 258-271: Do not export from a test file. (lint/suspicious/noExportsInTest)


[error] 271-286: Do not export from a test file. (lint/suspicious/noExportsInTest)

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] 484-484: 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] 991-999: 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] 1260-1265: Unsafe usage of 'return'. (lint/correctness/noUnsafeFinally)

'return' in 'finally' overwrites the control flow statements inside 'try' and 'catch'.


[error] 1297-1333: This else clause can be omitted because previous branches break early. (lint/style/noUselessElse)

Unsafe fix: Omit the else clause.


[error] 1549-1549: Change to an optional chain. (lint/complexity/useOptionalChain)

Unsafe fix: Change to an optional chain.

Additional comments not posted (13)
app/client/src/workers/Evaluation/handlers/updateActionData.ts (2)

6-6: Ensure proper usage of EVAL_WORKER_SYNC_ACTION import.

The import of EVAL_WORKER_SYNC_ACTION from @appsmith/workers/Evaluation/evalWorkerActions is used in the handleActionsDataUpdate function. Make sure this import is necessary and used effectively throughout the file.


44-48: Review the updated evalTreeWithChanges function call.

The function now includes additional parameters for updatedValuePaths and metaUpdates. Ensure that these changes align with the new logic for handling action data updates and that they are being used correctly in the context of evaluation logic.

app/client/src/workers/Evaluation/handlers/index.ts (2)

25-25: Check the import of evalTreeWithChanges.

Ensure that the import of evalTreeWithChanges is consistent with its usage in the syncHandlerMap and that there are no typos or incorrect paths.


33-33: Validate the addition of evalTreeWithChanges to syncHandlerMap.

The addition of evalTreeWithChanges to the syncHandlerMap is crucial for the new evaluation logic. Confirm that it is correctly mapped and that the corresponding action type is correct.

app/client/src/workers/Evaluation/JSObject/JSVariableUpdates.ts (2)

10-10: Check the necessity of the EVAL_WORKER_SYNC_ACTION import.

The import of EVAL_WORKER_SYNC_ACTION is used in the applyJSVariableUpdatesToEvalTree function. Verify that this import is essential for the updated logic in handling JS variable updates.


47-68: Review updated logic in applyJSVariableUpdatesToEvalTree.

The function now skips the evaluation of updated values themselves and only evaluates their dependents. This optimization should improve performance but requires thorough testing to ensure it doesn't introduce any regressions.

Consider adding more descriptive comments.

While the existing comments explain the logic well, consider adding more details about why only dependents are evaluated and the potential impacts this change might have.

app/client/src/workers/common/DataTreeEvaluator/utils.ts (2)

1-6: Verify the necessity and correct usage of new imports.

The imports of DataTreeDiff, DataTreeDiffEvent, isWidget and others are crucial for the new utility functions. Ensure these imports are correctly used and necessary for the functions defined in this file.


117-134: Review the implementation of getIsNewWidgetAdded.

This function checks if a new widget has been added by examining the diffs. Ensure that the logic correctly identifies new widgets and that it integrates well with the rest of the evaluation process.

app/client/src/workers/Evaluation/evalTreeWithChanges.ts (2)

18-18: Ensure the correct usage of DataTreeEvaluator type import.

Verify that the DataTreeEvaluator type is correctly used in the context of the evalTreeWithChanges function and that there are no issues with the import path or usage.


38-62: Thoroughly review the evalTreeWithChanges function implementation.

This function now handles the evaluation of changes more efficiently by skipping certain paths and using meta updates. Ensure that these changes are correctly implemented and that they do not introduce any unintended side effects.

app/client/src/workers/Evaluation/setters.ts (1)

22-22: Ensure correct import for EVAL_WORKER_SYNC_ACTION.

The import statement for EVAL_WORKER_SYNC_ACTION is crucial for the subsequent changes in this file. Ensure that the path and the imported entity are correctly referenced.

app/client/src/workers/Evaluation/handlers/evalTree.ts (1)

Line range hint 48-297: Refactor evalTree function to enhance clarity and maintainability.

The restructuring of the evalTree function helps clarify the flow of data and the conditions under which different types of tree evaluations occur. This should improve the maintainability of the code. However, ensure that all new variables and logic changes are thoroughly tested to prevent regressions.

Additionally, consider renaming the variable EvalError as it shadows a global property, which could lead to confusion or errors in other parts of the application.

- import type { EvalError } from "utils/DynamicBindingUtils";
+ import type { EvaluationError as EvalError } from "utils/DynamicBindingUtils";
app/client/src/workers/Evaluation/__tests__/evaluation.test.ts (1)

682-682: Potential Typographical Error

It seems there is an extraneous line number 682~ without any accompanying code. This might be a typographical error or misplaced line marker.

app/client/src/workers/common/DataTreeEvaluator/index.ts Outdated Show resolved Hide resolved
app/client/src/workers/Evaluation/setters.ts Show resolved Hide resolved
app/client/src/utils/DynamicBindingUtils.ts Show resolved Hide resolved
Copy link

⚠️ Cyclic Dependency Check:

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.

@rishabhrathod01 rishabhrathod01 added ok-to-test Required label for CI and removed ok-to-test Required label for CI labels Jun 26, 2024
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

Commits

Files that changed from the base of the PR and between 5cde3d6 and 94e07ed.

Files selected for processing (6)
  • app/client/cypress/e2e/Regression/ClientSide/SetProperty/SetOptions_Spec.ts (1 hunks)
  • app/client/cypress/e2e/Regression/ClientSide/SetProperty/WidgetPropertySetters1_spec.ts (1 hunks)
  • app/client/cypress/e2e/Regression/ClientSide/SetProperty/WidgetPropertySetters2_spec.ts (1 hunks)
  • app/client/src/workers/Evaluation/evalTreeWithChanges.ts (4 hunks)
  • app/client/src/workers/Evaluation/handlers/updateActionData.ts (2 hunks)
  • app/client/src/workers/Evaluation/setters.ts (2 hunks)
Files skipped from review due to trivial changes (3)
  • app/client/cypress/e2e/Regression/ClientSide/SetProperty/SetOptions_Spec.ts
  • app/client/cypress/e2e/Regression/ClientSide/SetProperty/WidgetPropertySetters1_spec.ts
  • app/client/cypress/e2e/Regression/ClientSide/SetProperty/WidgetPropertySetters2_spec.ts
Additional comments not posted (6)
app/client/src/workers/Evaluation/handlers/updateActionData.ts (2)

39-40: Properly handle data updates and path management.

The changes in the handleActionsDataUpdate function now include the setting of action data and the management of paths to skip from evaluation. This is crucial for performance optimization by avoiding unnecessary evaluations.

Also applies to: 42-42


43-56: Review the structure of evalTreeWithChanges call.

The call to evalTreeWithChanges now includes a structured request object with updatedValuePaths, metaUpdates, and pathsToSkipFromEval. This structure enhances clarity and modularity. However, ensure that the metaUpdates and pathsToSkipFromEval are properly utilized within the function to prevent any unintended side effects.

#!/bin/bash
# Description: Verify proper usage of `metaUpdates` and `pathsToSkipFromEval`.

# Test: Search for their usage within the project to ensure they are handled appropriately.
rg --type python "metaUpdates" "pathsToSkipFromEval"
app/client/src/workers/Evaluation/evalTreeWithChanges.ts (3)

4-8: Update type imports for clarity and maintainability.

The updated type imports enhance the clarity and maintainability of the code by specifying exact types used in function signatures and responses.


38-55: Refactor evalTreeWithChanges to handle new structured request.

The refactoring of evalTreeWithChanges to accept a structured request object is a significant improvement. This enhances the function's flexibility and clarity by explicitly stating what each part of the request contains.


63-63: Ensure additionalPathsAddedAsUpdates is utilized effectively.

The variable additionalPathsAddedAsUpdates is created but its effective use within the function needs to be verified to ensure it contributes positively to the evaluation logic.

#!/bin/bash
# Description: Verify the effective use of `additionalPathsAddedAsUpdates`.

# Test: Search for its usage within the project to ensure it is handled appropriately.
rg --type python "additionalPathsAddedAsUpdates"
app/client/src/workers/Evaluation/setters.ts (1)

124-133: Review updated evalTreeWithChanges call parameters.

The updated call to evalTreeWithChanges within applySetterMethod now passes structured data, including updatedValuePaths, metaUpdates, and pathsToSkipFromEval. This structured approach enhances the clarity and modularity of the code.

@rishabhrathod01 rishabhrathod01 added ok-to-test Required label for CI and removed ok-to-test Required label for CI labels Jun 26, 2024
@rishabhrathod01 rishabhrathod01 added ok-to-test Required label for CI and removed ok-to-test Required label for CI labels Jun 26, 2024
@appsmithorg appsmithorg deleted a comment from github-actions bot Jun 26, 2024
@appsmithorg appsmithorg deleted a comment from github-actions bot Jun 26, 2024
Copy link

⚠️ Cyclic Dependency Check:

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.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

Outside diff range and nitpick comments (1)
app/client/src/workers/Evaluation/setters.ts (1)

Line range hint 85-132: Optimize error handling and evaluation logic in applySetterMethod.

Consider using optional chaining to simplify null checks. Also, ensure that the logic correctly handles setter methods and that the evaluation call is optimized for performance.

-    if (!dataTreeEvaluator) return;
+    if (!dataTreeEvaluator?.getEvalTree()) return;
Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

Commits

Files that changed from the base of the PR and between 94e07ed and 3de988a.

Files selected for processing (4)
  • app/client/src/workers/Evaluation/JSObject/JSVariableUpdates.ts (2 hunks)
  • app/client/src/workers/Evaluation/evalTreeWithChanges.ts (4 hunks)
  • app/client/src/workers/Evaluation/handlers/updateActionData.ts (2 hunks)
  • app/client/src/workers/Evaluation/setters.ts (2 hunks)
Files skipped from review as they are similar to previous changes (2)
  • app/client/src/workers/Evaluation/evalTreeWithChanges.ts
  • app/client/src/workers/Evaluation/handlers/updateActionData.ts
Additional context used
Biome
app/client/src/workers/Evaluation/setters.ts

[error] 85-85: 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/JSObject/JSVariableUpdates.ts (2)

10-10: Verify the use of the new import.

Ensure that the newly imported EVAL_WORKER_SYNC_ACTION is used appropriately in the context of the changes made in this file.

Verification successful

The new import is used appropriately.

The imported EVAL_WORKER_SYNC_ACTION is utilized in the file as expected.

  • Location of usage:
    • app/client/src/workers/Evaluation/JSObject/JSVariableUpdates.ts:
      import { EVAL_WORKER_SYNC_ACTION } from "@appsmith/workers/Evaluation/evalWorkerActions";
      method: EVAL_WORKER_SYNC_ACTION.EVAL_TREE_WITH_CHANGES,
Scripts executed

The following scripts were executed for the analysis:


Script:

#!/bin/bash
# Description: Verify the use of the new import for EVAL_WORKER_SYNC_ACTION.

# Test: Search for the usage of EVAL_WORKER_SYNC_ACTION in this file. Expect: At least one usage.
rg --type python 'EVAL_WORKER_SYNC_ACTION' app/client/src/workers/Evaluation/JSObject/JSVariableUpdates.ts

Length of output: 137



Script:

#!/bin/bash
# Description: Verify the use of the new import for EVAL_WORKER_SYNC_ACTION.

# Test: Search for the usage of EVAL_WORKER_SYNC_ACTION in this file. Expect: At least one usage.
rg --type typescript 'EVAL_WORKER_SYNC_ACTION' app/client/src/workers/Evaluation/JSObject/JSVariableUpdates.ts

Length of output: 145



Script:

#!/bin/bash
# Description: Verify the use of the new import for EVAL_WORKER_SYNC_ACTION.

# Test: Search for the usage of EVAL_WORKER_SYNC_ACTION in this file. Expect: At least one usage.
rg 'EVAL_WORKER_SYNC_ACTION' app/client/src/workers/Evaluation/JSObject/JSVariableUpdates.ts

Length of output: 242


47-65: Check the logic and performance of variable updates handling.

The function applyJSVariableUpdatesToEvalTree has been modified to use the new EVAL_WORKER_SYNC_ACTION enum. Ensure that the logic correctly identifies modified variables and that the evaluation call is optimized for performance.

Verification successful

Check the logic and performance of variable updates handling.

The function applyJSVariableUpdatesToEvalTree in app/client/src/workers/Evaluation/JSObject/JSVariableUpdates.ts correctly uses the EVAL_WORKER_SYNC_ACTION enum and implements logic to handle modified variables efficiently.

  • The function checks if the modifiedVariablesList is empty and returns early if it is.
  • The EVAL_WORKER_SYNC_ACTION.EVAL_TREE_WITH_CHANGES enum value is used correctly to invoke the evaluation function with the appropriate parameters.

No significant performance issues were detected.

Scripts executed

The following scripts were executed for the analysis:


Script:

#!/bin/bash
# Description: Check the logic and performance of the function applyJSVariableUpdatesToEvalTree.

# Test: Run a performance benchmark on the function. Expect: No significant performance degradation.
echo "Performance benchmark for applyJSVariableUpdatesToEvalTree"

Length of output: 126



Script:

#!/bin/bash
# Find the implementation of the applyJSVariableUpdatesToEvalTree function
ast-grep --lang typescript --pattern 'function applyJSVariableUpdatesToEvalTree($_, $_) { $$$ }'

# Verify the usage of the EVAL_WORKER_SYNC_ACTION enum within the codebase
rg 'EVAL_WORKER_SYNC_ACTION'

# Check for the logic handling modified variables and calling the evaluation function
ast-grep --lang typescript --pattern 'if (!$_.length) return;'

Length of output: 2253

app/client/src/workers/Evaluation/setters.ts (1)

22-22: Group related imports for better readability.

The import of EVAL_WORKER_SYNC_ACTION should be grouped with other related imports from @appsmith/workers/Evaluation/evalWorkerActions to improve code organization and readability.

- import { EVAL_WORKER_SYNC_ACTION } from "@appsmith/workers/Evaluation/evalWorkerActions";
+ // Grouped with other related imports

({ dataPath, entityName }) => [entityName, dataPath],
);
evalTreeWithChanges(updatedProperties, []);
const updatedProperties: string[][] = [];
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

undo this change in the next PR to make it more readable.

);
},
() =>
parseJSActions(this, unEvalTree, this.oldUnEvalTree, jsTranslatedDiffs),
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

instead of returning object with jsUpdates prefer returning jsUpdates itself.


describe("getOnlyAffectedJSObjects", () => {
const dataTree = {
JSObject1: {
actionId: "1234",
variables: ["var", "var2"],
ENTITY_TYPE: ENTITY_TYPE.JSACTION,
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we could retain the enum here if the change doesn't have a significant performance change in a unit test.

@vsvamsi1 vsvamsi1 self-requested a review June 27, 2024 06:37
@rishabhrathod01 rishabhrathod01 merged commit cb3ddf1 into release Jun 27, 2024
46 checks passed
@rishabhrathod01 rishabhrathod01 deleted the chore/eval-with-changes branch June 27, 2024 06:38
Shivam-z pushed a commit to Shivam-z/appsmith that referenced this pull request Jul 10, 2024
## Description

These changes add evalTreeWithDiff to evalWorkerAction. Using this
method it will be possible to send the unevalTree and configTree updates
from mainThread to trigger evaluation.
- This will skip diffing complete unEvaltree

### Next steps after this PR
- [ ] Rename EvalTreeWithChanges to evalTreeWithDiff
- [ ] Generate Diff instead of updatedValuePaths for the current
evalTreeWithChanges references.
- [ ] Handle evalTreeWithDiff for
	- [ ] JSObject updates 
	- [ ] updateDependencyMap

Fixes #

## Automation

/test js

### 🔍 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/9684706533>
> Commit: 3de988a
> <a
href="https://internal.appsmith.com/app/cypress-dashboard/rundetails-65890b3c81d7400d08fa9ee5?branch=master&workflowId=9684706533&attempt=1"
target="_blank">Cypress dashboard</a>.
> Tags: `@tag.JS`

<!-- end of auto-generated comment: Cypress test results  -->


















## Communication
Should the DevRel and Marketing teams inform users about this change?
- [ ] Yes
- [x] No


<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->
## Summary by CodeRabbit

- **New Features**
- Enhanced widget evaluation logic and improved handling of dynamic
bindings and dependencies.

- **Bug Fixes**
- Corrected type handling in various evaluation functions to improve
stability and accuracy.

- **Tests**
- Updated test cases for widget property setters to include additional
tags for better categorization.

- **Refactor**
- Streamlined function signatures and improved type safety across
evaluation utilities.
<!-- end of auto-generated comment: release notes by coderabbit.ai -->
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Integrations Pod General Issues related to the Integrations Pod that don't fit into other tags. Integrations Product Issues related to a specific integration Javascript Product Issues related to users writing javascript in appsmith JS Evaluation Issues related to JS evaluation on the platform ok-to-test Required label for CI skip-changelog Adding this label to a PR prevents it from being listed in the changelog
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants