chore: update local uneval tree only for jsObjects and exit early for other entities #34524
Conversation
ashit-rath
added
Frontend
WalkthroughA conditional check for Changes
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 (
|
github-actions
bot
added
the
skip-changelog
![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
Outside diff range and nitpick comments (4)
app/client/src/workers/common/DataTreeEvaluator/index.ts (4)
Line range hint
1014-1022: Reorder parameters to place default parameters last.This change ensures that default parameters are correctly utilized and follow conventional patterns.
- function example(param1 = "default", param2) { + function example(param2, param1 = "default") {Tools
Biome
[error] 475-475: Avoid redundant double-negation.
It is not necessary to use double-negation when a value will already be coerced to a boolean.
Unsafe fix: Remove redundant double-negation(lint/complexity/noExtraBooleanCast)
Line range hint
1283-1288: Avoid 'return' in 'finally' block to prevent overriding 'try' and 'catch'.Using 'return' in a 'finally' block can lead to unexpected behavior by overriding error handling or other logic in 'try' and 'catch' blocks.
try { // some logic } catch (error) { // handle error } finally { // cleanup // remove the return statement }Tools
Biome
[error] 475-475: Avoid redundant double-negation.
It is not necessary to use double-negation when a value will already be coerced to a boolean.
Unsafe fix: Remove redundant double-negation(lint/complexity/noExtraBooleanCast)
Line range hint
1320-1356: Remove unnecessary 'else' clauses to simplify control flow.After 'break', 'continue', 'throw', or 'return' statements, 'else' clauses are redundant and can be removed to simplify the control flow.
if (condition) { return value; } // remove else, just continue with the next block of codeAlso applies to: 1431-1433
Tools
Biome
[error] 475-475: Avoid redundant double-negation.
It is not necessary to use double-negation when a value will already be coerced to a boolean.
Unsafe fix: Remove redundant double-negation(lint/complexity/noExtraBooleanCast)
Line range hint
1579-1579: Utilize optional chaining for safer and cleaner code.Optional chaining (
?.) allows for safer access to deeply nested properties and can prevent runtime errors due toundefinedornullvalues.- const value = object && object.property; + const value = object?.property;Also applies to: 1684-1685
Tools
Biome
[error] 475-475: Avoid redundant double-negation.
It is not necessary to use double-negation when a value will already be coerced to a boolean.
Unsafe fix: Remove redundant double-negation(lint/complexity/noExtraBooleanCast)
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- app/client/src/workers/common/DataTreeEvaluator/index.ts (1 hunks)
Additional context used
Biome
app/client/src/workers/common/DataTreeEvaluator/index.ts
[error] 3-3: Do not shadow the global "EvalError" property.
Consider renaming this variable. It's easy to confuse the origin of variables when they're named after a known global.
(lint/suspicious/noShadowRestrictedNames)
[error] 475-475: Avoid redundant double-negation.
It is not necessary to use double-negation when a value will already be coerced to a boolean.
Unsafe fix: Remove redundant double-negation(lint/complexity/noExtraBooleanCast)
[error] 1014-1022: This default parameter should follow the last required parameter or should be a required parameter.
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.(lint/style/useDefaultParameterLast)
[error] 1283-1288: Unsafe usage of 'return'.
'return' in 'finally' overwrites the control flow statements inside 'try' and 'catch'.
(lint/correctness/noUnsafeFinally)
[error] 1320-1356: This else clause can be omitted because previous branches break early.
Unsafe fix: Omit the else clause.
(lint/style/noUselessElse)
[error] 1431-1433: This else clause can be omitted because previous branches break early.
Unsafe fix: Omit the else clause.
(lint/style/noUselessElse)
[error] 1579-1579: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 1684-1685: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
Additional comments not posted (2)
app/client/src/workers/common/DataTreeEvaluator/index.ts (2)
470-472: Consider early return to simplify logic.The current implementation uses
if (!isJSAction(entity)) return;which effectively filters out non-JSAction entities. This aligns with the PR's objective to only process JSObjects.
Line range hint
3-3: Rename theEvalErrortype to avoid shadowing global property.Using a name that shadows a global JavaScript object can lead to confusion and bugs. Consider renaming
EvalErrortoEvaluationErroror another distinct name.
[ISSMUE]- import type { EvalError, EvaluationError } from "utils/DynamicBindingUtils"; + import type { EvaluationError as AppsmithEvaluationError, EvaluationError } from "utils/DynamicBindingUtils";Tools
Biome
[error] 475-475: Avoid redundant double-negation.
It is not necessary to use double-negation when a value will already be coerced to a boolean.
Unsafe fix: Remove redundant double-negation(lint/complexity/noExtraBooleanCast)
ashit-rath
added
ok-to-test
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range and nitpick comments (1)
app/client/src/workers/common/DataTreeEvaluator/index.ts (1)
Line range hint
481-488: Consider optimizing the method for clarity and performance.The method iterates over keys and performs checks and updates. Consider refactoring to improve clarity and reduce potential performance overhead, especially in scenarios with a large number of entities.
- Object.keys(unEvalJSCollection).forEach((entityName) => { + for (const entityName of Object.keys(unEvalJSCollection)) {This change uses a
for...ofloop, which can be more performant and clearer in this context.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- app/client/src/workers/common/DataTreeEvaluator/index.ts (1 hunks)
Additional context used
Biome
app/client/src/workers/common/DataTreeEvaluator/index.ts
[error] 990-998: This default parameter should follow the last required parameter or should be a required parameter.
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.(lint/style/useDefaultParameterLast)
[error] 1259-1264: Unsafe usage of 'return'.
'return' in 'finally' overwrites the control flow statements inside 'try' and 'catch'.
(lint/correctness/noUnsafeFinally)
[error] 1296-1332: This else clause can be omitted because previous branches break early.
Unsafe fix: Omit the else clause.
(lint/style/noUselessElse)
[error] 1548-1548: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
Additional comments not posted (1)
app/client/src/workers/common/DataTreeEvaluator/index.ts (1)
483-483: Ensure early exit condition is correctly implemented.The addition of the conditional check
if (!isJSAction(entity)) return;is intended to prevent unwanted modifications by exiting early if the entity is not a JSAction. This change aligns with the PR's objectives and should improve performance by reducing unnecessary processing.
… other entities (appsmithorg#34524) ## Description When a new evaluation cycle starts the uneval tree goes through a processing for updating the js objects in the right format. But the update condition depends on values present in `JSObjectCollection`. Due to the nature of evaluation for modules; when a JSModule is opened in an editor then the JSModule in evaluation is treated as a jsobject but when a different JSModule is opened then the previously opened JSModule is treated as a JSModule instance. Since in one of the eval cycle the first JSModule was treated as an JSObject so it's functions made it's way into the `JSObjectCollection` and now when it was treated as `JSModuleInstance`, due to it's functions present in `JSObjectCollection` during previous eval cycles; it's values gets altered which is undesirable. The change here dictates that if an entity is a JSObject, then only proceed for any alteration else the dataTree for the entity stays intact. PR for https://github.com/appsmithorg/appsmith-ee/pull/4547 ## Automation /ok-to-test tags="@tag.All" ### 🔍 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/9707356261> > Commit: f242dfa > <a href="https://internal.appsmith.com/app/cypress-dashboard/rundetails-65890b3c81d7400d08fa9ee5?branch=master&workflowId=9707356261&attempt=2" target="_blank">Cypress dashboard</a>. > Tags: `` <!-- 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 - **Bug Fixes** - Enhanced stability and control flow in the `DataTreeEvaluator` to prevent issues when processing non-JSAction entities. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
Description
When a new evaluation cycle starts the uneval tree goes through a processing for updating the js objects in the right format. But the update condition depends on values present in
JSObjectCollection.Due to the nature of evaluation for modules; when a JSModule is opened in an editor then the JSModule in evaluation is treated as a jsobject but when a different JSModule is opened then the previously opened JSModule is treated as a JSModule instance. Since in one of the eval cycle the first JSModule was treated as an JSObject so it's functions made it's way into the
JSObjectCollectionand now when it was treated asJSModuleInstance, due to it's functions present inJSObjectCollectionduring previous eval cycles; it's values gets altered which is undesirable.The change here dictates that if an entity is a JSObject, then only proceed for any alteration else the dataTree for the entity stays intact.
PR for https://github.com/appsmithorg/appsmith-ee/pull/4547
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/9707356261
Commit: f242dfa
Cypress dashboard.
Tags: ``
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
DataTreeEvaluatorto prevent issues when processing non-JSAction entities.