chore: merge updateActionData with an evaluation #38548
Conversation
WalkthroughThe pull request introduces enhancements to the evaluation and linting processes in the application's client-side code. The primary modification involves adding an optional Changes
Possibly related PRs
Suggested Labels
Suggested Reviewers
Poem
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (5)
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
🧹 Nitpick comments (1)
app/client/src/sagas/EvaluationsSaga.ts (1)
251-251: Consider adding JSDoc for the new parameter.Document the purpose and expected structure of
actionDataPayloadConsolidated.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
app/client/src/sagas/EvaluationsSaga.ts(6 hunks)app/client/src/workers/Evaluation/dataStore/utils.ts(1 hunks)app/client/src/workers/Evaluation/handlers/evalTree.ts(3 hunks)app/client/src/workers/Evaluation/types.ts(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- app/client/src/workers/Evaluation/dataStore/utils.ts
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: client-lint / client-lint
- GitHub Check: client-unit-tests / client-unit-tests
- GitHub Check: client-check-cyclic-deps / check-cyclic-dependencies
- GitHub Check: client-prettier / prettier-check
- GitHub Check: client-build / client-build
🔇 Additional comments (2)
app/client/src/workers/Evaluation/handlers/evalTree.ts (1)
196-215: LGTM! Well-structured action data handling.The implementation correctly:
- Handles data path references
- Updates evaluation tree and context
- Manages data store updates
app/client/src/sagas/EvaluationsSaga.ts (1)
845-857: Verify the order of operations in buffered action handling.The new condition handles both debounced updates and buffered actions together. Ensure this doesn't affect the evaluation order or cause race conditions.
✅ Verification successful
The order of operations in buffered action handling is properly implemented and safe.
The implementation uses a buffer queue system with proper guards and debouncing mechanisms to prevent race conditions. The test suite confirms correct handling of various scenarios including action clubbing and concurrent updates.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for other places where buffered actions are handled rg -A 5 "BUFFERED_ACTION" --type tsLength of output: 9180
| @@ -51,6 +52,7 @@ export interface EvalTreeRequestData { | |||
| widgetsMeta: Record<string, any>; | |||
| shouldRespondWithLogs?: boolean; | |||
| affectedJSObjects: AffectedJSObjects; | |||
| actionDataPayloadConsolidated: any; | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Define a proper type for actionDataPayloadConsolidated.
Using any type reduces type safety. Consider defining a proper interface or type for the consolidated action data payload.
- actionDataPayloadConsolidated: any;
+ actionDataPayloadConsolidated?: {
+ dataPath: string;
+ dataPathRef?: string;
+ entityName: string;
+ data: unknown;
+ }[];📝 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.
| actionDataPayloadConsolidated: any; | |
| actionDataPayloadConsolidated?: { | |
| dataPath: string; | |
| dataPathRef?: string; | |
| entityName: string; | |
| data: unknown; | |
| }[]; |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
app/client/src/workers/Evaluation/types.ts (1)
55-55: Consider defining a more specific type for actionDataPayloadConsolidated.Using
anyreduces type safety. Consider creating a dedicated type or interface that accurately represents the structure of the consolidated action data payload.app/client/src/sagas/EvaluationsSaga.ts (1)
251-251: Consider adding JSDoc for the new parameter.Add documentation for the
actionDataPayloadConsolidatedparameter to improve code maintainability.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
app/client/src/sagas/EvaluationsSaga.ts(6 hunks)app/client/src/workers/Evaluation/dataStore/utils.ts(1 hunks)app/client/src/workers/Evaluation/handlers/evalTree.ts(3 hunks)app/client/src/workers/Evaluation/types.ts(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- app/client/src/workers/Evaluation/dataStore/utils.ts
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: client-lint / client-lint
- GitHub Check: client-unit-tests / client-unit-tests
- GitHub Check: client-build / client-build
🔇 Additional comments (2)
app/client/src/workers/Evaluation/handlers/evalTree.ts (1)
196-215: Verify data consistency across updates.The code updates multiple data stores (tree, context, and DataStore) with the same data. Ensure that:
- The order of updates is correct to maintain consistency
- Error handling is in place for failed updates
- The datastore path construction is safe from injection
Consider extracting this logic into a separate function for better maintainability and reusability.
app/client/src/sagas/EvaluationsSaga.ts (1)
845-857: Review the order of operations in buffered action handling.The new condition block handles both debounced updates and buffered actions together. Ensure that:
- The order of operations is correct
- No race conditions exist between updates
- Error handling is consistent with other code paths
vsvamsi1
added
ok-to-test
vsvamsi1
changed the title
github-actions
bot
added
the
skip-changelog
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
app/client/src/workers/Evaluation/handlers/updateActionData.ts (1)
32-45: Consider using a Set for better performance.The code creates an array of updated properties and then iterates over it. Using a Set would be more efficient for this use case.
- const updatedProperties: string[][] = []; + const updatedProperties = new Set<string[]>(); actionsToUpdate.forEach(({ dataPath, entityName }) => { - updatedProperties.push([entityName, dataPath]); + updatedProperties.add([entityName, dataPath]); });app/client/src/workers/common/DataTreeEvaluator/index.ts (1)
729-734: Consider using a more descriptive variable name.
pathsChangedSetcould be renamed to better reflect its purpose, e.g.,actionDataPaths.app/client/src/workers/Evaluation/handlers/evalTree.ts (1)
195-198: Consider adding error handling.The
updateActionsToEvalTreecall should be wrapped in a try-catch block to handle potential errors gracefully.- updateActionsToEvalTree(tree, actionDataPayloadConsolidated); + try { + updateActionsToEvalTree(tree, actionDataPayloadConsolidated); + } catch (error) { + errors.push({ + type: EvalErrorTypes.UNKNOWN_ERROR, + message: `Failed to update actions: ${(error as Error).message}`, + }); + }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
app/client/src/sagas/EvaluationsSaga.test.ts(3 hunks)app/client/src/sagas/EvaluationsSaga.ts(6 hunks)app/client/src/workers/Evaluation/handlers/evalTree.ts(4 hunks)app/client/src/workers/Evaluation/handlers/updateActionData.ts(2 hunks)app/client/src/workers/Evaluation/types.ts(2 hunks)app/client/src/workers/common/DataTreeEvaluator/index.ts(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- app/client/src/workers/Evaluation/types.ts
- app/client/src/sagas/EvaluationsSaga.ts
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: perform-test / rts-build / build
- GitHub Check: client-check-cyclic-deps / check-cyclic-dependencies
- GitHub Check: client-lint / client-lint
- GitHub Check: client-build / client-build
- GitHub Check: client-prettier / prettier-check
🔇 Additional comments (7)
app/client/src/workers/Evaluation/handlers/updateActionData.ts (2)
30-31: LGTM! Good separation of concerns.The extraction of action data update logic into a separate function improves code modularity.
47-50: LGTM! Clear function signature.The function signature clearly indicates the optional nature of the
actionsToUpdateparameter.app/client/src/sagas/EvaluationsSaga.test.ts (1)
79-79: LGTM! Comprehensive test coverage.The test cases properly verify the
actionDataPayloadConsolidatedparameter with different scenarios:
- Debug mode
- Non-debug mode
- JS object propagation
Also applies to: 125-125, 180-180
app/client/src/workers/common/DataTreeEvaluator/index.ts (2)
641-641: LGTM! Clear parameter addition.The optional
actionDataPayloadConsolidatedparameter is properly typed.
751-762: LGTM! Efficient path skipping implementation.The implementation properly maps and transforms paths for evaluation skipping.
app/client/src/workers/Evaluation/handlers/evalTree.ts (2)
37-37: LGTM! Clean import and destructuring.The changes properly import the new function and destructure the required data.
Also applies to: 74-74
221-221: LGTM! Proper parameter passing.The
actionDataPayloadConsolidatedis correctly passed tosetupUpdateTree.
|
🔴🔴🔴 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. |
vsvamsi1
requested review from
rajatagrawal
and removed request for
ApekshaBhosale
vsvamsi1
added
ok-to-test
|
🔴🔴🔴 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. |
Description
Whenever we have debounced updateActionData and an evalTree we will merge that to a single evaluation. Seeing a 10-20% reduction in LCP, for a configured customer app we are seeing the LCP reduce from 20-23 seconds to about 15-20 seconds.
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/12715835754
Commit: 693a3f8
Cypress dashboard.
Tags:
@tag.AllSpec:
Sat, 11 Jan 2025 16:27:52 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Chores
Technical Improvements