Chore/first eval optimisation remove trigger based context #38077
Conversation
WalkthroughThe pull request introduces several modifications across multiple files, primarily focusing on enhancing the functionality and testing of the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
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
|
![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 (5)
app/client/src/ce/workers/Evaluation/Actions.ts (1)
107-109: Correct the comment to reflect the logicThe comment after the
if (!isTriggerBased)condition is misleading. Since we're returningEVAL_CONTEXTwhenisTriggerBasedisfalse, we are skipping the addition of entity functions in sync evaluations. Please update the comment for accuracy.Apply this diff to fix the comment:
- // if eval is not trigger based i.e., sync eval then we skip adding entity function to evalContext + // Since eval is not trigger-based (sync eval), we return EVAL_CONTEXT without adding entity functionsapp/client/src/sagas/EvaluationsSaga.ts (1)
28-28: ImportingevalWorkerenhances clarityReplacing
GracefulWorkerServicewith directevalWorkerimport simplifies worker usage.app/client/src/workers/common/DataTreeEvaluator/index.ts (2)
Line range hint
1237-1367: Consider Refactoring Repetitive Scope Cache CodeThe code block for setting values into
nonTriggerBasedDataTreeContextunder therelease_evaluation_scope_cacheflag is repeated multiple times. Refactoring this into a helper function could improve maintainability.
1479-1479: Update Function Documentation for 'getDynamicValue'The
getDynamicValuefunction signature now includes thescopeCacheparameter. Update the function's JSDoc comments to reflect this change and explain the parameter's purpose.app/client/src/sagas/EvalWorkerActionSagas.ts (1)
Line range hint
140-160: Consider optimizing the telemetry span usage.The span creation and ending could be moved into the
updateDataTreeHandlerto ensure accurate timing of the data tree update operation itself.- const rootSpan = startRootSpan("DataTreeFactory.create"); - const unEvalAndConfigTree: ReturnType<typeof getUnevaluatedDataTree> = yield select(getUnevaluatedDataTree); - - endSpan(rootSpan); yield call(updateDataTreeHandler, { + rootSpan: startRootSpan("DataTreeFactory.create"), evalTreeResponse: workerResponse as EvalTreeResponseData, unevalTree: unEvalAndConfigTree.unEvalTree || {}, requiresLogging: false, configTree: unEvalAndConfigTree.configTree, });
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (26)
app/client/src/ce/entities/DataTree/dataTreeJSAction.test.ts(2 hunks)app/client/src/ce/entities/DataTree/dataTreeJSAction.ts(1 hunks)app/client/src/ce/entities/DataTree/types.ts(1 hunks)app/client/src/ce/entities/FeatureFlag.ts(2 hunks)app/client/src/ce/workers/Evaluation/Actions.ts(3 hunks)app/client/src/ce/workers/Evaluation/evaluationUtils.ts(1 hunks)app/client/src/entities/Engine/AppViewerEngine.ts(0 hunks)app/client/src/index.tsx(1 hunks)app/client/src/sagas/ActionExecution/PluginActionSaga.ts(1 hunks)app/client/src/sagas/ActionExecution/geolocationSaga.ts(1 hunks)app/client/src/sagas/EvalWorkerActionSagas.ts(1 hunks)app/client/src/sagas/EvaluationsSaga.test.ts(1 hunks)app/client/src/sagas/EvaluationsSaga.ts(3 hunks)app/client/src/sagas/JSLibrarySaga.ts(1 hunks)app/client/src/utils/workerInstances.ts(1 hunks)app/client/src/workers/Evaluation/JSObject/index.ts(1 hunks)app/client/src/workers/Evaluation/JSObject/test.ts(1 hunks)app/client/src/workers/Evaluation/__tests__/Actions.test.ts(2 hunks)app/client/src/workers/Evaluation/__tests__/evaluate.test.ts(6 hunks)app/client/src/workers/Evaluation/__tests__/evaluation.test.ts(0 hunks)app/client/src/workers/Evaluation/evaluate.ts(5 hunks)app/client/src/workers/Evaluation/fns/resetWidget.ts(1 hunks)app/client/src/workers/Evaluation/handlers/jsLibrary.ts(1 hunks)app/client/src/workers/Evaluation/handlers/workerEnv.ts(1 hunks)app/client/src/workers/common/DataTreeEvaluator/index.ts(16 hunks)app/client/src/workers/common/DataTreeEvaluator/test.ts(2 hunks)
💤 Files with no reviewable changes (2)
- app/client/src/entities/Engine/AppViewerEngine.ts
- app/client/src/workers/Evaluation/tests/evaluation.test.ts
✅ Files skipped from review due to trivial changes (1)
- app/client/src/workers/Evaluation/JSObject/index.ts
🔇 Additional comments (28)
app/client/src/ce/workers/Evaluation/Actions.ts (1)
Line range hint 43-54: Function renaming and parameter adjustments look good
The renaming of addDataTreeToContext to getDataTreeContext and the updated parameters enhance clarity.
🧰 Tools
🪛 Biome (1.9.4)
[error] 58-58: Don't use 'Function' as a type.
Prefer explicitly define the function shape. This type accepts any function-like value, which can be a common source of bugs.
(lint/complexity/noBannedTypes)
app/client/src/workers/Evaluation/evaluate.ts (3)
Line range hint 106-119: Exporting resetWorkerGlobalScope improves modularity
Making resetWorkerGlobalScope an exported function enhances its reusability across modules.
276-284: Refactored context creation is appropriate
The introduction of getDataTreeContext for creating the evaluation context simplifies the code and promotes consistency.
Line range hint 369-426: Ensure EVAL_CONTEXT is correctly populated in evaluateSync
The restructuring of evaluateSync to initialize and populate EVAL_CONTEXT may affect existing functionality. Verify that all necessary data is included and that there are no side effects.
app/client/src/sagas/EvaluationsSaga.ts (1)
297-298: Assess the impact of waitForFetchEnvironments on evaluation timing
Introducing waitForFetchEnvironments may affect the evaluation flow. Ensure it does not introduce unwanted delays.
app/client/src/sagas/ActionExecution/PluginActionSaga.ts (2)
105-106: Updated imports align with worker instances
Importing evalWorker from utils/workerInstances standardizes worker references.
Line range hint 397-398: Review new file upload functions for correctness
The addition of triggerFileUploadInstrumentation, clearTriggerActionResponse, and softRefreshActionsSaga functions should be thoroughly tested to ensure correct integration.
app/client/src/workers/common/DataTreeEvaluator/index.ts (1)
1068-1069: Verify 'resetWorkerGlobalScope()' Usage
Ensure that invoking resetWorkerGlobalScope() here does not unintentionally affect the evaluation context, especially in concurrent scenarios.
app/client/src/utils/workerInstances.ts (1)
1-13: Initialization of 'evalWorker' Instance
The creation of the evalWorker instance using GracefulWorkerService is correct. Ensure that the worker script at the specified path is accessible and the worker functions as intended.
app/client/src/workers/Evaluation/handlers/workerEnv.ts (1)
4-4: Enhance Type Safety for 'flags' Property
Defining flags with the FeatureFlags type adds type safety to the WorkerEnv class. This change improves code reliability and maintainability.
app/client/src/ce/entities/DataTree/dataTreeJSAction.ts (1)
92-92: Include 'actionNames' in JSActionEntity Config
Adding actionNames to the JSActionEntityConfig provides direct access to the names of actions, which can enhance code clarity and usability.
app/client/src/ce/entities/FeatureFlag.ts (1)
46-46: LGTM! Verify feature flag usage.
The feature flag follows the naming convention and is properly configured.
Also applies to: 85-85
✅ Verification successful
Feature flag is properly implemented and used
The feature flag is correctly used in DataTreeEvaluator for controlling scope cache functionality:
- Defined in FeatureFlag.ts with proper typing and default value (false)
- Consistently used in DataTreeEvaluator for conditional cache operations
- Implementation follows the gradual rollout pattern with feature flag checks
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify the usage of the new feature flag across the codebase
rg "release_evaluation_scope_cache" --type ts --type tsx
Length of output: 84
Script:
#!/bin/bash
# Search for the feature flag usage with ripgrep, without file type filtering
rg "release_evaluation_scope_cache"
# Also search with ast-grep to find any TypeScript/JavaScript usage
ast-grep --pattern 'release_evaluation_scope_cache'
Length of output: 895
Script:
#!/bin/bash
# Let's check the context of usage in DataTreeEvaluator
rg "release_evaluation_scope_cache" app/client/src/workers/common/DataTreeEvaluator/index.ts -B 2 -A 2
Length of output: 1088
app/client/src/index.tsx (1)
3-4: LGTM! Worker initialization properly placed.
Early initialization of worker instances ensures they're available throughout the application lifecycle.
app/client/src/sagas/ActionExecution/geolocationSaga.ts (2)
8-8: LGTM! Import path updated for consistency.
The evalWorker import now comes from the centralized worker instances.
Line range hint 73-78: LGTM! Error handling improved for worker thread compatibility.
The sanitizeGeolocationError function properly handles GeolocationPositionError serialization for worker thread communication.
app/client/src/sagas/EvalWorkerActionSagas.ts (1)
18-18: LGTM!
The import path standardization improves code organization.
app/client/src/ce/entities/DataTree/types.ts (1)
99-99: LGTM!
The addition of actionNames array provides type safety for tracking JS action names.
app/client/src/workers/Evaluation/JSObject/test.ts (1)
216-216: LGTM!
The test data update properly validates the handling of action names, including the duplicate action scenario.
app/client/src/workers/Evaluation/fns/resetWidget.ts (1)
18-18: Verify import usage across the codebase
The change from default to named imports could affect other modules importing from "../evaluate".
✅ Verification successful
Let me search with a broader pattern to check both named imports and their usage. The first command will find any named imports of these functions, and the second will find their actual usage in the code.
Import change is consistent with codebase usage
All imports of evaluateSync and setEvalContext are using named imports from the evaluate module. The change aligns with the existing usage pattern across the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining default imports of evaluate
rg "import\s+evaluateSync\s+from\s+['\"].*evaluate['\"]"
Length of output: 62
Script:
#!/bin/bash
# Search for imports of evaluateSync and setEvalContext with broader pattern
rg "import.*\{.*(?:evaluateSync|setEvalContext).*\}.*from.*evaluate"
# Search for usage of these functions
rg "(?:evaluateSync|setEvalContext)\("
Length of output: 3433
app/client/src/sagas/EvaluationsSaga.test.ts (1)
6-6: LGTM: Centralized worker instance import
The change aligns with the centralization of worker instances, improving maintainability.
app/client/src/ce/entities/DataTree/dataTreeJSAction.test.ts (1)
194-194: LGTM: Test coverage for actionNames property
The test cases correctly validate the new actionNames configuration property.
Also applies to: 393-393
app/client/src/sagas/JSLibrarySaga.ts (1)
24-24: LGTM: Import path change looks good.
The centralization of worker instances improves maintainability.
✅ Verification successful
Import change is correctly implemented and safe
The import change is part of a proper refactoring where:
- The
evalWorkeris now centralized inutils/workerInstances - All relevant files (5 files found) are consistently using the new import path
- The
EvaluationsSagafile still exists and is used for other purposes, but worker initialization has been correctly moved
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify no broken imports exist for evalWorker
rg -g '!*.test.*' -g '!*.d.ts' "import.*evalWorker.*from.*EvaluationsSaga"
Length of output: 76
Script:
#!/bin/bash
# Let's verify the new import path exists and is being used correctly
rg -g '!*.test.*' -g '!*.d.ts' "import.*evalWorker.*from.*['\"]utils/workerInstances['\"]"
# Check if there are any remaining references to EvaluationsSaga
rg -g '!*.test.*' -g '!*.d.ts' "EvaluationsSaga"
# Verify the structure of the worker instances file
fd -g 'workerInstances.{ts,js}' -x cat {}
Length of output: 3017
app/client/src/workers/Evaluation/__tests__/Actions.test.ts (2)
9-9: LGTM: New import aligns with the optimization goals.
551-557: Great improvement: Moving to immutable context creation.
The change from mutation-based to functional approach reduces side effects and improves code predictability.
app/client/src/workers/Evaluation/handlers/jsLibrary.ts (2)
36-36: LGTM: Simplified error message.
Direct template literal improves maintainability by removing message constant dependency.
44-44: LGTM: Consistent error message simplification.
Follows the same pattern of using template literals for error messages.
app/client/src/workers/common/DataTreeEvaluator/test.ts (1)
771-771: LGTM: Test configuration updated correctly
The actionNames arrays are properly added to both JSObject configs to support testing the new isPropertyAnEntityAction function.
Also applies to: 825-825
app/client/src/workers/Evaluation/__tests__/evaluate.test.ts (1)
1-1: No changes to review in this file
| export const isPropertyAnEntityAction = ( | ||
| entity: DataTreeEntity, | ||
| propertyPath: string, | ||
| entityConfig: DataTreeEntityConfig, | ||
| ) => { | ||
| if (!isJSAction(entity)) return false; | ||
|
|
||
| const { actionNames } = entityConfig as JSActionEntityConfig; | ||
|
|
||
| return actionNames.includes(propertyPath); | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add type safety checks for actionNames access
The function should verify that entityConfig is a JSActionEntityConfig before accessing actionNames to prevent runtime errors.
export const isPropertyAnEntityAction = (
entity: DataTreeEntity,
propertyPath: string,
entityConfig: DataTreeEntityConfig,
) => {
if (!isJSAction(entity)) return false;
+ if (!isJSActionConfig(entityConfig)) return false;
+
const { actionNames } = entityConfig as JSActionEntityConfig;
return actionNames.includes(propertyPath);
};Committable suggestion skipped: line range outside the PR's diff.
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/12257427772
Commit: d08593b
Cypress dashboard.
Tags:
@tag.AllSpec:
Tue, 10 Dec 2024 15:04:31 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
Release Notes
New Features
actionNamesproperty in various configurations to enhance action handling.release_evaluation_scope_cachefor improved cache management.isPropertyAnEntityActionfunction to check property paths against action names.Bug Fixes
Documentation
Refactor
addDataTreeToContexttogetDataTreeContextfor clarity.Tests