fix: Skip JSObject eval where the path is the function #41157

ashit-rath · 2025-08-01T03:41:48Z

Description

JSObjects and JSModuleInstance function paths were earlier skipped eval when present in the eval order. In the reactive actions PR this check was removed and due to that JSModuleInstances function were overriden in evalContextCache with it's uneval value. Due to which during any eval where the JSModuleInstance function is present as a binding, fails to evaluate

This PR reverts the check

Fixes #41146

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/16665740260
Commit: 9a10adb
Cypress dashboard.
Tags: @tag.All
Spec:

Fri, 01 Aug 2025 05:00:23 UTC

Communication

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

Yes
No

Summary by CodeRabbit

Bug Fixes
- Improved evaluation process to prevent unintended evaluation of action properties within JSObjects, resulting in more stable and predictable behavior for users.

coderabbitai · 2025-08-01T03:41:55Z

Walkthrough

A conditional check was added to the evaluateTree method in the DataTreeEvaluator. This check skips the evaluation of properties recognized as entity actions within JSObjects, ensuring such properties are not processed during the evaluation loop. No exported or public entity signatures were changed.

Changes

Cohort / File(s)	Change Summary
DataTreeEvaluator Logic Update `app/client/src/workers/common/DataTreeEvaluator/index.ts`	Added a conditional to skip evaluating JSObject properties that are identified as entity actions.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~7 minutes

Poem

In the evaluator’s thoughtful loop,
Some actions now slip through the group.
With a check, they’re left aside,
No longer in the mainline ride.
JSObjects breathe a lighter air,
As entity actions get special care.
🎩✨

Note

⚡️ Unit Test Generation is now available in beta!

Learn more here, or try it out under "Finishing Touches" below.

📜 Recent review details

Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between eaea235 and 9a10adb.

📒 Files selected for processing (1)

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

🧰 Additional context used

🧠 Learnings (9)

📓 Common learnings

Learnt from: brayn003
PR: appsmithorg/appsmith#40462
File: app/client/src/instrumentation/index.ts:0-0
Timestamp: 2025-04-29T09:12:14.552Z
Learning: Only comment on files that are directly related to the PR's objectives, even if other files appear in the diff. For PR #40462, the focus is on the import override feature for artifacts, not on instrumentation or telemetry files.

Learnt from: brayn003
PR: appsmithorg/appsmith#37984
File: app/client/src/git/requests/pullRequest.ts:6-10
Timestamp: 2024-12-05T10:58:36.272Z
Learning: Error handling is not required for the `pullRequest` function in `app/client/src/git/requests/pullRequest.ts`.

Learnt from: rahulbarwal
PR: appsmithorg/appsmith#34563
File: app/client/src/sagas/BuildingBlockSagas/index.ts:77-87
Timestamp: 2024-10-08T15:32:34.115Z
Learning: The `accessNestedObjectValue` function in the `sagas/PasteWidgetUtils` module has been tested by the user (rahulbarwal) and confirmed to handle null or undefined values correctly.

Learnt from: rahulbarwal
PR: appsmithorg/appsmith#34563
File: app/client/src/sagas/BuildingBlockSagas/index.ts:77-87
Timestamp: 2024-06-27T14:27:49.746Z
Learning: The `accessNestedObjectValue` function in the `sagas/PasteWidgetUtils` module has been tested by the user (rahulbarwal) and confirmed to handle null or undefined values correctly.

Learnt from: Aishwarya-U-R
PR: appsmithorg/appsmith#29405
File: app/client/cypress/e2e/Regression/ClientSide/Binding/TableV2_Widget_API_Pagination_spec.js:37-41
Timestamp: 2024-07-26T21:12:57.228Z
Learning: The pull request titled "test: Cypress | Replace static with Dynamic waits - Part 1" is part of a phased approach where only certain test specifications are targeted for static wait removal in the initial phase. Future phases will address additional specs.

Learnt from: Aishwarya-U-R
PR: appsmithorg/appsmith#29405
File: app/client/cypress/e2e/Regression/ClientSide/Binding/TableV2_Widget_API_Pagination_spec.js:37-41
Timestamp: 2024-10-08T15:32:39.374Z
Learning: The pull request titled "test: Cypress | Replace static with Dynamic waits - Part 1" is part of a phased approach where only certain test specifications are targeted for static wait removal in the initial phase. Future phases will address additional specs.

Learnt from: nidhi-nair
PR: appsmithorg/appsmith#29372
File: app/server/appsmith-server/src/main/java/com/appsmith/server/applications/jslibs/ApplicationJsLibServiceCEImpl.java:51-56
Timestamp: 2024-10-08T15:32:34.115Z
Learning: The `updateJsLibsInContext` method in the `ApplicationJsLibServiceCEImpl` class was mistakenly referred to as a new method in a previous comment. It is important to accurately assess the context of changes in a pull request to avoid misrepresenting the nature of the code modifications.

Learnt from: nidhi-nair
PR: appsmithorg/appsmith#29372
File: app/server/appsmith-server/src/main/java/com/appsmith/server/applications/jslibs/ApplicationJsLibServiceCEImpl.java:51-56
Timestamp: 2024-07-26T21:12:57.228Z
Learning: The `updateJsLibsInContext` method in the `ApplicationJsLibServiceCEImpl` class was mistakenly referred to as a new method in a previous comment. It is important to accurately assess the context of changes in a pull request to avoid misrepresenting the nature of the code modifications.

Learnt from: sagar-qa007
PR: appsmithorg/appsmith#34955
File: app/client/cypress/e2e/Regression/ClientSide/ActionExecution/General_settings_Spec.ts:14-56
Timestamp: 2024-10-08T15:32:34.115Z
Learning: Learnt from: sagar-qa007
PR: appsmithorg/appsmith#34955
File: app/client/cypress/e2e/Regression/ClientSide/ActionExecution/General_settings_Spec.ts:14-56
Timestamp: 2024-07-16T06:44:55.118Z
Learning: Avoid using sleep functions like `agHelper.Sleep`, `this.Sleep`, and other related sleep functions in Cypress tests within the `app/client/cypress` directory to prevent non-deterministic behaviors and ensure tests are more reliable and maintainable.

Learnt from: AmanAgarwal041
PR: appsmithorg/appsmith#36539
File: app/client/src/workers/Evaluation/handlers/jsLibrary.ts:329-329
Timestamp: 2024-10-08T15:32:39.374Z
Learning: In `app/client/src/workers/Evaluation/handlers/jsLibrary.ts`, when removing keys from `self`, use `delete` to completely remove the property, as assigning `undefined` leaves the key in the object and can affect subsequent difference checks.

Learnt from: ankitakinger
PR: appsmithorg/appsmith#29573
File: app/client/src/pages/Editor/Explorer/Entity/EntityProperties.tsx:46-65
Timestamp: 2024-07-26T21:12:57.228Z
Learning: The use of `any` for the `entityProperties` parameter in the `getJSActionBindings` function within `EntityProperties.tsx` is intentional and should not be suggested for refactoring to improve type safety.

Learnt from: sharat87
PR: appsmithorg/appsmith#34471
File: app/client/src/ce/api/JSActionAPI.tsx:143-161
Timestamp: 2024-10-08T15:32:39.374Z
Learning: The `updateJSCollectionActionRefactor` method in `JSActionAPI.tsx` includes a check to ensure `action.datasource` is not null before manipulating its properties.

Learnt from: sharat87
PR: appsmithorg/appsmith#34471
File: app/client/src/ce/api/JSActionAPI.tsx:143-161
Timestamp: 2024-06-26T06:22:15.976Z
Learning: The `updateJSCollectionActionRefactor` method in `JSActionAPI.tsx` includes a check to ensure `action.datasource` is not null before manipulating its properties.

Learnt from: ashit-rath
PR: appsmithorg/appsmith#33809
File: app/client/src/workers/common/DataTreeEvaluator/test.ts:196-196
Timestamp: 2024-10-08T15:32:34.115Z
Learning: In test cases within `DataTreeEvaluator/test.ts`, type safety is not a priority, and using `any` is acceptable for flexibility as per user ashit-rath.

Learnt from: ashit-rath
PR: appsmithorg/appsmith#33809
File: app/client/src/workers/common/DataTreeEvaluator/test.ts:196-196
Timestamp: 2024-07-26T21:12:57.228Z
Learning: In test cases within `DataTreeEvaluator/test.ts`, type safety is not a priority, and using `any` is acceptable for flexibility as per user ashit-rath.

Learnt from: AmanAgarwal041
PR: appsmithorg/appsmith#36539
File: app/client/src/workers/Evaluation/handlers/jsLibrary.ts:329-329
Timestamp: 2024-09-25T18:58:34.275Z
Learning: In `app/client/src/workers/Evaluation/handlers/jsLibrary.ts`, when removing keys from `self`, use `delete` to completely remove the property, as assigning `undefined` leaves the key in the object and can affect subsequent difference checks.

Learnt from: sharat87
PR: appsmithorg/appsmith#34471
File: app/client/src/ce/api/JSActionAPI.tsx:98-111
Timestamp: 2024-10-08T15:32:34.115Z
Learning: The `updateJSCollection` method in `JSActionAPI.tsx` uses optional chaining and logical AND to safely handle potential null or undefined values for `action.datasource`.

Learnt from: ankitakinger
PR: appsmithorg/appsmith#29573
File: app/client/src/pages/Editor/Explorer/Entity/EntityProperties.tsx:240-0
Timestamp: 2024-10-08T15:32:34.114Z
Learning: The `ENTITY_TYPE.MODULE_INSTANCE` case in `EntityProperties.tsx` is intentionally a combination of the logic from both `ENTITY_TYPE.ACTION` and `ENTITY_TYPE.JSACTION`, which explains the presence of what might seem like duplicated code.

Learnt from: ankitakinger
PR: appsmithorg/appsmith#29573
File: app/client/src/pages/Editor/Explorer/Entity/EntityProperties.tsx:240-0
Timestamp: 2024-07-26T21:12:57.228Z
Learning: The `ENTITY_TYPE.MODULE_INSTANCE` case in `EntityProperties.tsx` is intentionally a combination of the logic from both `ENTITY_TYPE.ACTION` and `ENTITY_TYPE.JSACTION`, which explains the presence of what might seem like duplicated code.

Learnt from: ashit-rath
PR: appsmithorg/appsmith#33809
File: app/client/src/workers/Evaluation/evaluate.ts:209-229
Timestamp: 2024-10-08T15:32:34.115Z
Learning: User ashit-rath prefers using `forEach` over `for...of` for iterating over objects and arrays in JavaScript.

Learnt from: ashit-rath
PR: appsmithorg/appsmith#33809
File: app/client/src/workers/Evaluation/evaluate.ts:209-229
Timestamp: 2024-07-26T21:12:57.228Z
Learning: User ashit-rath prefers using `forEach` over `for...of` for iterating over objects and arrays in JavaScript.

📚 Learning: in `app/client/src/workers/evaluation/handlers/jslibrary.ts`, when removing keys from `self`, use `d...

Learnt from: AmanAgarwal041
PR: appsmithorg/appsmith#36539
File: app/client/src/workers/Evaluation/handlers/jsLibrary.ts:329-329
Timestamp: 2024-09-25T18:58:34.275Z
Learning: In `app/client/src/workers/Evaluation/handlers/jsLibrary.ts`, when removing keys from `self`, use `delete` to completely remove the property, as assigning `undefined` leaves the key in the object and can affect subsequent difference checks.

Applied to files:

app/client/src/workers/common/DataTreeEvaluator/index.ts

📚 Learning: the use of `any` for the `entityproperties` parameter in the `getjsactionbindings` function within `...

Learnt from: ankitakinger
PR: appsmithorg/appsmith#29573
File: app/client/src/pages/Editor/Explorer/Entity/EntityProperties.tsx:46-65
Timestamp: 2024-07-26T21:12:57.228Z
Learning: The use of `any` for the `entityProperties` parameter in the `getJSActionBindings` function within `EntityProperties.tsx` is intentional and should not be suggested for refactoring to improve type safety.

Applied to files:

app/client/src/workers/common/DataTreeEvaluator/index.ts

📚 Learning: the `updatejscollectionactionrefactor` method in `jsactionapi.tsx` includes a check to ensure `actio...

Learnt from: sharat87
PR: appsmithorg/appsmith#34471
File: app/client/src/ce/api/JSActionAPI.tsx:143-161
Timestamp: 2024-10-08T15:32:39.374Z
Learning: The `updateJSCollectionActionRefactor` method in `JSActionAPI.tsx` includes a check to ensure `action.datasource` is not null before manipulating its properties.

Applied to files:

app/client/src/workers/common/DataTreeEvaluator/index.ts

📚 Learning: in test cases within `datatreeevaluator/test.ts`, type safety is not a priority, and using `any` is ...

Learnt from: ashit-rath
PR: appsmithorg/appsmith#33809
File: app/client/src/workers/common/DataTreeEvaluator/test.ts:196-196
Timestamp: 2024-10-08T15:32:34.115Z
Learning: In test cases within `DataTreeEvaluator/test.ts`, type safety is not a priority, and using `any` is acceptable for flexibility as per user ashit-rath.

Applied to files:

app/client/src/workers/common/DataTreeEvaluator/index.ts

📚 Learning: the `updatejscollection` method in `jsactionapi.tsx` uses optional chaining and logical and to safel...

Learnt from: sharat87
PR: appsmithorg/appsmith#34471
File: app/client/src/ce/api/JSActionAPI.tsx:98-111
Timestamp: 2024-10-08T15:32:34.115Z
Learning: The `updateJSCollection` method in `JSActionAPI.tsx` uses optional chaining and logical AND to safely handle potential null or undefined values for `action.datasource`.

Applied to files:

app/client/src/workers/common/DataTreeEvaluator/index.ts

📚 Learning: the `entity_type.module_instance` case in `entityproperties.tsx` is intentionally a combination of t...

Learnt from: ankitakinger
PR: appsmithorg/appsmith#29573
File: app/client/src/pages/Editor/Explorer/Entity/EntityProperties.tsx:240-0
Timestamp: 2024-07-26T21:12:57.228Z
Learning: The `ENTITY_TYPE.MODULE_INSTANCE` case in `EntityProperties.tsx` is intentionally a combination of the logic from both `ENTITY_TYPE.ACTION` and `ENTITY_TYPE.JSACTION`, which explains the presence of what might seem like duplicated code.

Applied to files:

app/client/src/workers/common/DataTreeEvaluator/index.ts

📚 Learning: the use of `any` type for `moduleinstanceentities` in `app/client/src/ce/entities/datatree/types.ts`...

Learnt from: ankitakinger
PR: appsmithorg/appsmith#29573
File: app/client/src/ce/entities/DataTree/types.ts:182-186
Timestamp: 2024-07-26T21:12:57.228Z
Learning: The use of `any` type for `moduleInstanceEntities` in `app/client/src/ce/entities/DataTree/types.ts` is intentional and was done by another developer. This decision is likely to serve a specific purpose within the codebase, which may be related to maintaining compatibility between CE and EE or other architectural reasons.

Applied to files:

app/client/src/workers/common/DataTreeEvaluator/index.ts

📚 Learning: the `geterrorpropertypath` function in `anvilwidgetselectionsaga.ts` includes a temporary fix to han...

Learnt from: riodeuno
PR: appsmithorg/appsmith#33070
File: app/client/src/layoutSystems/anvil/integrations/sagas/anvilWidgetSelectionSaga.ts:94-103
Timestamp: 2024-10-08T15:32:34.115Z
Learning: The `getErrorPropertyPath` function in `anvilWidgetSelectionSaga.ts` includes a temporary fix to handle cases where no errors are found by returning an empty string. This is planned to be removed in the future.

Applied to files:

app/client/src/workers/common/DataTreeEvaluator/index.ts

🔇 Additional comments (2)

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

42-42: LGTM: Clean import addition.

The isPropertyAnEntityAction import follows the existing pattern and is appropriately placed with related evaluation utilities.

196-200: LGTM: Well-implemented performance optimization.

The conditional check is properly placed in the evaluation loop and effectively skips unnecessary evaluation of JSObject function properties. The clear comment and appropriate use of continue make this a clean implementation that aligns with the PR objective.

✨ Finishing Touches

📝 Generate Docstrings

🧪 Generate unit tests

Create PR with unit tests
Post copyable unit tests in a comment
Commit unit tests in branch fix/js-module-fn-call

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ 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>, please review it.
- Explain this complex logic.
- 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 explain this code block.
- @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 gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
- @coderabbitai read src/utils.ts and explain its main purpose.
- @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.

Support

Need help? Create a ticket on our support page for assistance with any issues or questions.

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)

@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 generate docstrings to generate docstrings for this PR.
@coderabbitai generate sequence diagram to generate a sequence diagram of the changes in this PR.
@coderabbitai generate unit tests to generate unit tests for this 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.

Other keywords and placeholders

Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
Add @coderabbitai anywhere in the PR title to generate the title automatically.

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.

ashit-rath · 2025-08-01T08:41:22Z

Shadow EE PR to verify tests https://github.com/appsmithorg/appsmith-ee/pull/8057

fix: Skip JSObject eval where the path is the function

9a10adb

ashit-rath requested a review from ApekshaBhosale as a code owner August 1, 2025 03:41

ashit-rath added the ok-to-test Required label for CI label Aug 1, 2025

github-actions bot added the Bug Something isn't working label Aug 1, 2025

ashit-rath self-assigned this Aug 1, 2025

ashit-rath added ok-to-test Required label for CI and removed ok-to-test Required label for CI labels Aug 1, 2025

ApekshaBhosale approved these changes Aug 1, 2025

View reviewed changes

ashit-rath merged commit d0b8899 into release Aug 1, 2025
102 of 104 checks passed

ashit-rath deleted the fix/js-module-fn-call branch August 1, 2025 11:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

fix: Skip JSObject eval where the path is the function #41157

fix: Skip JSObject eval where the path is the function #41157

Uh oh!

ashit-rath commented Aug 1, 2025 •

edited

Loading

Uh oh!

coderabbitai bot commented Aug 1, 2025 •

edited

Loading

Chat

Support

CodeRabbit Commands (Invoked using PR comments)

Other keywords and placeholders

Documentation and Community

Uh oh!

ashit-rath commented Aug 1, 2025

Uh oh!

Uh oh!

Uh oh!

fix: Skip JSObject eval where the path is the function #41157

fix: Skip JSObject eval where the path is the function #41157

Uh oh!

Conversation

ashit-rath commented Aug 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Automation

🔍 Cypress test results

Communication

Summary by CodeRabbit

Uh oh!

coderabbitai bot commented Aug 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Estimated code review effort

Poem

Chat

Support

CodeRabbit Commands (Invoked using PR comments)

Other keywords and placeholders

Documentation and Community

Uh oh!

ashit-rath commented Aug 1, 2025

Uh oh!

Uh oh!

Uh oh!

ashit-rath commented Aug 1, 2025 •

edited

Loading

coderabbitai bot commented Aug 1, 2025 •

edited

Loading