Conversation
WalkthroughThis set of changes introduces new error handling for Changes
Sequence Diagram(s)sequenceDiagram
participant UI
participant Saga
participant EvalWorker
participant Sentry
UI->>Saga: Trigger evaluation
Saga->>EvalWorker: Evaluate data tree
EvalWorker->>EvalWorker: Generate diffs & updates (using helpers)
alt Error: UPDATE_DATA_TREE_ERROR
EvalWorker->>Saga: Throw UPDATE_DATA_TREE_ERROR
Saga->>Sentry: Sentry.captureMessage(error)
Saga->>Saga: log.error("Evaluation Error: ...")
else Success
EvalWorker->>Saga: Return evaluation result & updates
end
Saga->>UI: Send evaluation response
Possibly related PRs
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)
🪧 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
|
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: 3
🧹 Nitpick comments (3)
app/client/src/workers/Evaluation/evalTreeWithChanges.ts (1)
156-162: Added support for differentiating update cycles.The new boolean parameter (
truein this case) indicates that this is an update cycle, enabling optimized diff generation.I recommend adding a brief comment explaining what this boolean parameter controls to improve code clarity for future developers.
const updates = generateOptimisedUpdatesAndSetPrevState( dataTree, dataTreeEvaluator, affectedNodePaths, additionalUpdates, - true, + true, // isUpdateCycle: true indicates this is an update to existing data tree, not initial creation );app/client/src/workers/Evaluation/helpers.ts (2)
528-560: Error handling for update application is a good additionThe
updatePrevStatefunction includes proper error handling for update application, which helps prevent cascading failures. However, consider logging these errors more extensively for debugging purposes.- dataTreeEvaluator.errors.push({ - type: EvalErrorTypes.UPDATE_DATA_TREE_ERROR, - message: error.message, - }); + dataTreeEvaluator.errors.push({ + type: EvalErrorTypes.UPDATE_DATA_TREE_ERROR, + message: error.message, + context: { + path: update.path, + updateKind: update.kind + } + });
508-508: Consider using nullish coalescing operatorFor better handling of falsy values, consider using the nullish coalescing operator (
??) instead of logical OR (||).- dataTreeEvaluator?.getPrevState() || {}, + dataTreeEvaluator?.getPrevState() ?? {},
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
app/client/src/sagas/EvalErrorHandler.ts(1 hunks)app/client/src/utils/DynamicBindingUtils.ts(1 hunks)app/client/src/workers/Evaluation/__tests__/generateOpimisedUpdates.test.ts(3 hunks)app/client/src/workers/Evaluation/evalTreeWithChanges.test.ts(6 hunks)app/client/src/workers/Evaluation/evalTreeWithChanges.ts(4 hunks)app/client/src/workers/Evaluation/handlers/evalTree.ts(7 hunks)app/client/src/workers/Evaluation/helpers.test.ts(2 hunks)app/client/src/workers/Evaluation/helpers.ts(6 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (3)
app/client/src/workers/Evaluation/evalTreeWithChanges.test.ts (1)
app/client/src/workers/Evaluation/helpers.ts (1)
updateEvalProps(562-580)
app/client/src/workers/Evaluation/helpers.test.ts (2)
app/client/src/workers/Evaluation/handlers/evalTree.ts (1)
dataTreeEvaluator(40-40)app/client/src/workers/Evaluation/helpers.ts (1)
updatePrevState(528-560)
app/client/src/workers/Evaluation/handlers/evalTree.ts (1)
app/client/src/workers/Evaluation/helpers.ts (1)
updateEvalProps(562-580)
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: perform-test / client-build / client-build
- GitHub Check: perform-test / rts-build / build
- GitHub Check: perform-test / server-build / server-unit-tests
- GitHub Check: client-unit-tests / client-unit-tests
- GitHub Check: client-lint / client-lint
- GitHub Check: client-check-cyclic-deps / check-cyclic-dependencies
- GitHub Check: client-build / client-build
- GitHub Check: client-prettier / prettier-check
🔇 Additional comments (26)
app/client/src/utils/DynamicBindingUtils.ts (1)
163-163: Added new error type for data tree update failures.The new
UPDATE_DATA_TREE_ERRORenum value will help track and handle errors specifically occurring during data tree updates.app/client/src/sagas/EvalErrorHandler.ts (1)
306-314: Appropriate error handling implementation for the new error type.The implementation follows the established pattern in the file - capturing the message in Sentry and logging locally with error details. This maintains consistency with other error handlers.
app/client/src/workers/Evaluation/evalTreeWithChanges.test.ts (4)
8-8: Updated imports for modified evaluation flow.The imports of
klonaandupdateEvalPropsalign with the changes in the evaluation flow that now uses these for data tree cloning and property updates.Also applies to: 14-14
206-209: Test setup refactored to use new evaluation helpers.The setup now leverages
updateEvalPropsto update evaluation properties and usesklonafor deep cloning, matching the new evaluation flow implementation.
379-387: Updated diff kind expectations to match new diff semantics.The test now correctly expects kind "E" (edit) instead of "N" (new), aligning with the refined diff semantics implemented in the evaluation helpers.
531-535: Updated test expectations for precise update generation.The test now verifies exact arrays of edits including dependent widget updates, which reflects the more precise update generation in the evaluation helpers.
app/client/src/workers/Evaluation/evalTreeWithChanges.ts (3)
15-16: Updated imports for new evaluation flow.Importing
updateEvalPropsand additional types to support the refactored evaluation process.Also applies to: 19-20
133-133: Replaced manual entity config handling with helper function.Using
updateEvalPropsinstead of (likely)makeEntityConfigsAsObjPropertiessimplifies the code and aligns with the modularized evaluation approach.
147-147: Improved type safety with explicit type assertion.The explicit type assertion for
additionalUpdatesmakes the code more type-safe and self-documenting.app/client/src/workers/Evaluation/helpers.test.ts (5)
1-3: Imports look clean. No concerns here.
5-96: Comprehensive tests forstringifyFnsInObject. Good coverage for nested and array scenarios.
98-129: Accurate test for non-update cycle scenario. ValidatessetPrevStateusage correctly.
130-174: Good coverage of serialized updates on update cycles. Verifies partial state updates thoroughly.
175-221: Solid error-handling test. Ensures errors are pushed todataTreeEvaluator.errorsas expected.app/client/src/workers/Evaluation/handlers/evalTree.ts (5)
22-22: New importupdateEvalProps. This utility looks well-integrated with existing helpers.
95-96: Introduction ofisUpdateCycleflag. Straightforward approach to track update cycles.
136-136: Usage ofupdateEvalProps. SettingdataTreefrom evaluator ensures correct property updates each cycle.Also applies to: 188-188, 246-246
192-192: AdjustingisUpdateCyclemid-flow. The on/off pattern is clear and synced to the evaluation cycle.Also applies to: 336-336
327-329: PassingisUpdateCycletogenerateOptimisedUpdatesAndSetPrevState. Maintains consistent logic for partial diffs.app/client/src/workers/Evaluation/__tests__/generateOpimisedUpdates.test.ts (4)
13-18: Added type imports &getReducedDataTrees. These imports align well with the new tests.
412-419: Refined checks for removed function properties. Ensures the correct diffs are generated and applied.
675-954: Robust “type change tests.” Thorough scenarios covering array-to-object, object-to-array transformations, and partial property diffs.
956-1114: Focused tests forgetReducedDataTrees. Properly validates selective reduction and special handling of__evaluation__properties.app/client/src/workers/Evaluation/helpers.ts (3)
185-266: Improved DataTree reduction with ancestor path handlingThe updated
getReducedDataTreesfunction is a significant improvement over the previous implementation. It now:
- Handles both old and new trees simultaneously
- Preserves error information in both trees
- Intelligently walks up the path hierarchy to find defined ancestors
- Optimizes the resulting trees to contain only the necessary data
This should improve the performance and accuracy of diffs.
276-281: Good implementation of reduced trees in diff generationUsing the new
getReducedDataTreesfunction here properly supports the implementation of the optimized diffing approach. This will reduce the amount of data being compared, making the diff generation more efficient.
499-526: Good refactoring of update generation into a named functionConverting the anonymous function to a named function with explicit parameters improves code readability and maintainability. The added
isUpdateCycleflag provides better control over state management.
|
|
||
| import { create } from "mutative"; | ||
| export const fn_keys: string = "__fn_keys__"; | ||
| import { klona } from "klona/json"; |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
The PR objective mentions removing klona but it's still imported and used
This PR's objective was to remove the last usage of "klona," but it's still imported here and used in the updateEvalProps function (line 574). Consider completing the klona removal by using an alternative clone mechanism.
🏁 Script executed:
#!/bin/bash
# Check if klona is used elsewhere in the codebase
rg -i "klona" --type=ts --type=js | grep -v "package.json"Length of output: 25609
Critical: Complete Removal of klona from helpers.ts
Our investigation confirms that the PR’s goal to remove klona has not been fully achieved. In app/client/src/workers/Evaluation/helpers.ts, klona is still being imported from "klona/json" (line 12) and used within the updateEvalProps function (line 574). Please replace these references with the approved alternative clone mechanism to align with the PR objective.
| export function updateEvalProps(dataTreeEvaluator: DataTreeEvaluator) { | ||
| if (!dataTreeEvaluator) return null; | ||
|
|
||
| const evalProps = dataTreeEvaluator.evalProps; | ||
|
|
||
| return create(dataTreeEvaluator.evalTree, (draft) => { | ||
| for (const [entityName, entityEvalProps] of Object.entries(evalProps)) { | ||
| if (!entityEvalProps.__evaluation__) continue; | ||
|
|
||
| set( | ||
| draft[entityName], | ||
| "__evaluation__", | ||
| klona({ | ||
| errors: entityEvalProps.__evaluation__.errors, | ||
| }), | ||
| ); | ||
| } | ||
| }); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider alternative to klona for deep cloning
The function correctly updates evaluation properties, but uses klona for cloning which contradicts the PR objective of removing klona usage.
- klona({
- errors: entityEvalProps.__evaluation__.errors,
- }),
+ {
+ errors: JSON.parse(JSON.stringify(entityEvalProps.__evaluation__.errors)),
+ },Alternatively, you could use the immutable approach with create from mutative:
- klona({
- errors: entityEvalProps.__evaluation__.errors,
- }),
+ create({}, (draft) => {
+ draft.errors = entityEvalProps.__evaluation__.errors;
+ }),📝 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.
| export function updateEvalProps(dataTreeEvaluator: DataTreeEvaluator) { | |
| if (!dataTreeEvaluator) return null; | |
| const evalProps = dataTreeEvaluator.evalProps; | |
| return create(dataTreeEvaluator.evalTree, (draft) => { | |
| for (const [entityName, entityEvalProps] of Object.entries(evalProps)) { | |
| if (!entityEvalProps.__evaluation__) continue; | |
| set( | |
| draft[entityName], | |
| "__evaluation__", | |
| klona({ | |
| errors: entityEvalProps.__evaluation__.errors, | |
| }), | |
| ); | |
| } | |
| }); | |
| } | |
| export function updateEvalProps(dataTreeEvaluator: DataTreeEvaluator) { | |
| if (!dataTreeEvaluator) return null; | |
| const evalProps = dataTreeEvaluator.evalProps; | |
| return create(dataTreeEvaluator.evalTree, (draft) => { | |
| for (const [entityName, entityEvalProps] of Object.entries(evalProps)) { | |
| if (!entityEvalProps.__evaluation__) continue; | |
| set( | |
| draft[entityName], | |
| "__evaluation__", | |
| { | |
| errors: JSON.parse(JSON.stringify(entityEvalProps.__evaluation__.errors)), | |
| }, | |
| ); | |
| } | |
| }); | |
| } |
| // serialise bigInt values and convert the updates to a string over here to minismise the cost of transfer | ||
| // to the main thread. In the main thread parse this object there. | ||
| return { serialisedUpdates: serialiseToBigInt(removedLhs) }; | ||
| return { serialisedUpdates: serialiseToBigInt(removedLhs), updates }; |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Return type extended to include raw updates
The function now returns both serialized updates and the raw updates array. Ensure all call sites have been updated to handle this change.
🏁 Script executed:
#!/bin/bash
# Find all places where generateSerialisedUpdates is called
# and check if they handle the new return value
rg "generateSerialisedUpdates" --type=ts --type=js -A 3 -B 1Length of output: 9247
Return type extended – Call sites must account for raw updates
The function now returns an object with both a serialisedUpdates property and a new updates array. However, our search indicates that many call sites (notably within app/client/src/workers/Evaluation/__tests__/generateOpimisedUpdates.test.ts) still only destructure and use serialisedUpdates (and in one case, error). Please update these call sites to either destructure the new updates field where needed or explicitly confirm that ignoring it is acceptable given the intended business logic.
- Action items:
- Review usages of
generateSerialisedUpdatesinapp/client/src/workers/Evaluation/helpers.tsand its tests. - Ensure that where the raw updates array is required, the caller properly extracts and handles the
updatesproperty.
- Review usages of
2 participants
Description
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/14475541362
Commit: 9d0b751
Cypress dashboard.
Tags:
@tag.AllSpec:
Tue, 15 Apr 2025 18:35:25 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
Summary by CodeRabbit
Bug Fixes
Tests
Refactor
New Features