chore: bypass immer for first evaluation, fixed cloneDeep issue and using mutative instead of immer#38993
Conversation
WalkthroughThis pull request refactors state management across the codebase by replacing the Changes
Sequence Diagram(s)sequenceDiagram
participant U as User Action
participant C as Component/Reducer
participant M as Mutative.create
participant S as Updated State
U->>C: Dispatch action (e.g., state update)
C->>M: Call create(currentState, updater)
M->>S: Return new state object
S-->>C: Updated state for render
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Tip 🌐 Web search-backed reviews and chat
✨ Finishing Touches
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
|
|
/build-deploy-preview skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/13133299569. |
|
Deploy-Preview-URL: https://ce-38993.dp.appsmith.com |
vsvamsi1
changed the title
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: 2
🧹 Nitpick comments (8)
app/client/src/utils/ReducerUtils.ts (1)
45-47: Rename the reducer to match the “mutative” library
The function is still calledcreateImmerReduceryet it now usescreatefrommutative. Consider renaming the function or adding clear comments to avoid confusion.-export const createImmerReducer = ( +export const createMutativeReducer = (app/client/src/reducers/evaluationReducers/treeReducer.ts (1)
29-33: Graceful handling of missing or empty update paths
Skipping invalid updates prevents unnecessary crashes. If needed, log or count how many updates are skipped for debugging.app/client/src/reducers/entityReducers/metaReducer/index.ts (1)
85-110: Maintain consistency in state update patterns.The reducer uses mutative's
createfor some cases but falls back to object spread operator forTABLE_PANE_MOVED. Consider using mutative consistently across all cases for better maintainability.-const next = { ...state }; -// TODO: Fix this the next time the file is edited -// eslint-disable-next-line @typescript-eslint/no-explicit-any -let widgetMetaProps: Record<string, any> = next[action.payload.widgetId]; +const next = create(state, draft => { + let widgetMetaProps = draft[action.payload.widgetId]; + + if (widgetMetaProps === undefined) { + draft[action.payload.widgetId] = { + isMoved: true, + position: { ...action.payload.position }, + }; + } else { + draft[action.payload.widgetId] = { + ...widgetMetaProps, + isMoved: true, + position: { ...action.payload.position }, + }; + } +});app/client/src/workers/common/DataTreeEvaluator/utils.test.ts (1)
190-192: Optimize performance by avoiding delete operator.The static analysis tool correctly flags the use of the delete operator which can impact performance.
-const deletedWidgetNameInAllKeys = create(initialAllKeys, (draft) => { - delete draft["WidgetName.name"]; -}); +const deletedWidgetNameInAllKeys = create(initialAllKeys, (draft) => { + draft["WidgetName.name"] = undefined; +});🧰 Tools
🪛 Biome (1.9.4)
[error] 191-191: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
app/client/src/components/editorComponents/WidgetQueryGeneratorForm/index.tsx (1)
2-2: State update logic looks good but consider adding error handling.The transition from
immertomutativemaintains the same state update logic. However, thecreatefunction's error handling could be improved.Consider wrapping the state update in a try-catch block:
setConfig( - create(config, (draftConfig) => { + try { + return create(config, (draftConfig) => { if ( property === "datasource" || (typeof property === "object" && Object.keys(property).includes("datasource")) ) { // ... existing code ... } // ... rest of the code ... }), + } catch (error) { + console.error("Failed to update config:", error); + return config; + } );Also applies to: 181-224
app/client/src/ce/pages/Editor/Explorer/hooks.tsx (2)
187-208: Consider optimizing the filtering logic in useActions.The current implementation iterates through all actions for each page. Consider using a more efficient filtering approach.
-const filteredActions = create(actions, (draft) => { +const filteredActions = create(actions, (draft) => { + const searchKeywordLower = searchKeyword.toLowerCase(); for (const [key, value] of Object.entries(draft)) { if (pageIds.includes(key)) { draft[key] = value; } else { - value.forEach((action, index) => { - const searchMatches = - action.config.name - .toLowerCase() - .indexOf(searchKeyword.toLowerCase()) > -1; - - if (searchMatches) { - draft[key][index] = action; - } else { - delete draft[key][index]; - } - }); + draft[key] = value.filter(action => + action.config.name.toLowerCase().includes(searchKeywordLower) + ); } - draft[key] = draft[key].filter(Boolean); } });
259-270: Optimize page filtering in usePageIds hook.The current implementation creates unnecessary array copies.
-const filteredPages = create(pages, (draft) => { - draft.forEach((page, index) => { - const searchMatches = - page.pageName.toLowerCase().indexOf(searchKeyword.toLowerCase()) > -1; - if (searchMatches) { - } else { - delete draft[index]; - } - }); -}); +const searchKeywordLower = searchKeyword.toLowerCase(); +const filteredPages = pages.filter(page => + page.pageName.toLowerCase().includes(searchKeywordLower) +);app/client/src/workers/Evaluation/__tests__/generateOpimisedUpdates.test.ts (1)
373-374: Consider using undefined assignment instead of delete operator.Using the delete operator can impact performance. Consider using undefined assignment instead:
- delete draft.Table1.pageSize; - delete draft.Table1.__evaluation__.errors.transientTableData; + draft.Table1.pageSize = undefined; + draft.Table1.__evaluation__.errors.transientTableData = undefined;Also applies to: 374-377
🧰 Tools
🪛 Biome (1.9.4)
[error] 373-374: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
app/client/yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (18)
app/client/package.json(1 hunks)app/client/src/WidgetProvider/factory/index.tsx(2 hunks)app/client/src/ce/pages/Editor/Explorer/hooks.tsx(4 hunks)app/client/src/ce/reducers/entityReducers/jsActionsReducer.tsx(2 hunks)app/client/src/ce/reducers/uiReducers/applicationsReducer.tsx(2 hunks)app/client/src/components/editorComponents/WidgetQueryGeneratorForm/index.tsx(2 hunks)app/client/src/components/propertyControls/ButtonTabControl.tsx(2 hunks)app/client/src/index.tsx(1 hunks)app/client/src/reducers/entityReducers/datasourceReducer.ts(7 hunks)app/client/src/reducers/entityReducers/metaReducer/index.ts(4 hunks)app/client/src/reducers/entityReducers/metaReducer/metaReducerUtils.ts(2 hunks)app/client/src/reducers/evaluationReducers/treeReducer.ts(3 hunks)app/client/src/sagas/WidgetAdditionSagas.ts(2 hunks)app/client/src/utils/ReducerUtils.ts(2 hunks)app/client/src/widgets/TableWidgetV2/widget/index.tsx(2 hunks)app/client/src/workers/Evaluation/__tests__/generateOpimisedUpdates.test.ts(27 hunks)app/client/src/workers/Evaluation/evalTreeWithChanges.test.ts(9 hunks)app/client/src/workers/common/DataTreeEvaluator/utils.test.ts(5 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
app/client/src/workers/common/DataTreeEvaluator/utils.test.ts
[error] 191-191: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
app/client/src/workers/Evaluation/__tests__/generateOpimisedUpdates.test.ts
[error] 373-374: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 374-377: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
🔇 Additional comments (17)
app/client/src/utils/ReducerUtils.ts (2)
35-43: Validate behavior when multiple updates are flagged as "newTree"
Returning after finding the first"newTree"means subsequent "newTree" updates, if any, won't be applied. Confirm this is intentional.
2-2: Double-check “mutative” library version compatibility
It looks fine to import and usecreatefrommutativehere. However, kindly ensure that the installed version aligns with project requirements and doesn’t introduce breaking changes.✅ Verification successful
Verification of mutative version compatibility
The installed version^1.1.0(resolved to1.1.0) is used consistently across the codebase, and there are no apparent compatibility issues with the import ofcreate.
- Confirmed in
app/client/package.json- Confirmed in
app/client/yarn.lock🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check the installed version of 'mutative' in package-lock.json or yarn.lock rg "mutative" -A 3Length of output: 7093
app/client/src/reducers/evaluationReducers/treeReducer.ts (2)
3-3: Confirm the import usage for “deep-diff”
UsingapplyChangeis valid, but if performance is a concern for large trees, consider profiling and compare with alternative approaches.
17-17: Switching to generic Diff<DataTree, DataTree>
This type update looks appropriate, given the broader changes. Ensure all references to the olderDiffWithNewTreeStateare removed or updated.app/client/src/index.tsx (1)
25-26: Removal of the autofreeze logic aligns with the mutative transition
DroppingsetAutoFreezeis consistent with moving away from immer.app/client/src/reducers/entityReducers/metaReducer/metaReducerUtils.ts (1)
84-91: Verify immutability guarantees with mutative.The transition from immer to mutative looks correct. However, let's verify that mutative's
createfunction provides the same immutability guarantees as immer'sproduce.✅ Verification successful
Immutability Guarantees Verified via Mutative's
create()The investigation confirms that Mutative's
create()function maintains the same core immutability guarantees as Immer'sproduce(). In both approaches, the original state is preserved and only a new state is returned after applying draft modifications. While configuration options and performance optimizations differ, the underlying immutability mechanism works equivalently for typical use cases.
- Core Guarantee: Both methods use a draft pattern to ensure that the base state remains unchanged.
- Advanced Options: Mutative offers additional configuration options (e.g., strict mode and patch support) without compromising immutability.
🏁 Scripts executed
The following scripts were executed for the analysis:
Web query:
Does mutative's create function provide the same immutability guarantees as immer's produce function?Length of output: 6032
app/client/src/ce/pages/Editor/Explorer/hooks.tsx (1)
228-241: Similar optimization needed in useWidgets hook.The widget filtering logic could be optimized for better performance.
Run this script to check the impact of filtering on large widget trees:
app/client/src/ce/reducers/entityReducers/jsActionsReducer.tsx (1)
403-418: LGTM! Efficient state update implementation.The state update logic using
createis well-implemented with efficient lookups usingkeyBy.app/client/src/sagas/WidgetAdditionSagas.ts (1)
114-120: LGTM! Good performance optimization.The switch from
immertomutativefor state management is a good optimization, as it reduces scripting time by approximately 50%. The logic for initializingprops.childrenremains correct.app/client/src/WidgetProvider/factory/index.tsx (1)
421-424: LGTM! Consistent state management update.The change from
immertomutativeis consistent with the codebase-wide optimization effort.app/client/src/reducers/entityReducers/datasourceReducer.ts (1)
469-477: LGTM! Consistent reducer updates.The migration from
immertomutativeacross all action handlers maintains the same state update logic while improving performance.Also applies to: 659-662, 667-674, 679-686, 688-691, 696-702, 707-713, 715-718, 723-729, 734-740
app/client/src/workers/Evaluation/__tests__/generateOpimisedUpdates.test.ts (1)
113-115: LGTM! Test coverage maintained.The migration from
immertomutativeacross all test cases maintains the same test coverage and assertions.Also applies to: 122-125, 134-136, 148-150, 165-167, 182-184, 185-190, 207-209, 210-215, 247-249, 263-265, 284-287, 305-308, 336-339, 380-386, 387-394, 420-426, 427-430, 446-448, 472-485, 491-493, 507-509, 531-539, 555-557, 572-574, 586-593, 616-623, 631-640, 661-668
app/client/src/ce/reducers/uiReducers/applicationsReducer.tsx (2)
25-25: LGTM! Import statement updated for new state management library.The change from
immertomutativeis consistent with the codebase migration.
533-542: LGTM! State update logic migrated to use mutative.The state update logic has been correctly migrated from
immer'sproducetomutative'screatewhile maintaining the same functionality.app/client/src/widgets/TableWidgetV2/widget/index.tsx (2)
144-144: LGTM! Added utility import for object key iteration.Using
objectKeysfrom@appsmith/utilsis a good practice for consistent key iteration.
934-948: LGTM! Improved column type migration logic.The implementation efficiently updates column types using
objectKeysandreduce, avoiding unnecessary deep cloning.app/client/package.json (1)
164-164: LGTM! Added mutative dependency.The addition of
mutative@1.1.0is required for the state management migration fromimmer.
| const updatedValues: string[] = create(values, (draft: string[]) => { | ||
| draft.push(value); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Simplify array update logic.
Using mutative's create for a simple array push operation is unnecessary overhead. A simpler spread operator would be more efficient here.
-const updatedValues: string[] = create(values, (draft: string[]) => {
- draft.push(value);
-});
+const updatedValues: string[] = [...values, value];📝 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.
| const updatedValues: string[] = create(values, (draft: string[]) => { | |
| draft.push(value); | |
| }); | |
| const updatedValues: string[] = [...values, value]; |
| const updatedLabelUnevalTree = create(unEvalTree, (draft: any) => { | ||
| draft.Text1.text = UPDATED_LABEL; | ||
| draft.Text1.label = UPDATED_LABEL; | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fix type casting and improve type safety.
Multiple instances of any type casting should be addressed as noted in the TODO comments.
Consider creating proper types for the draft state:
interface UnEvalTree {
Text1?: {
text?: string;
label?: string;
};
[key: string]: any; // for other properties if needed
}
// Then use it in create calls:
const updatedLabelUnevalTree = create(unEvalTree, (draft: UnEvalTree) => {
if (draft.Text1?.text) {
draft.Text1.text = UPDATED_LABEL;
}
});Also applies to: 314-316, 349-353, 389-393, 429-433, 459-463, 498-502, 536-540
vsvamsi1
dismissed stale reviews from znamenskii-ilia and rajatagrawal
via
4cb8217
vsvamsi1
added
ok-to-test
vsvamsi1
added
ok-to-test
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
app/client/src/utils/ReducerUtils.ts (1)
35-43: Consider adding type safety for updates array.The updates array handling looks correct but could benefit from type safety.
+interface Update { + kind: string; + rhs: unknown; +} + if (action?.payload?.updates) { - const updates = action?.payload?.updates; + const updates = action?.payload?.updates as Update[]; for (const update of updates) { if (update.kind === "newTree") { return update.rhs; } } }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
app/client/yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (18)
app/client/package.json(2 hunks)app/client/src/WidgetProvider/factory/index.tsx(2 hunks)app/client/src/ce/pages/Editor/Explorer/hooks.tsx(4 hunks)app/client/src/ce/reducers/entityReducers/jsActionsReducer.tsx(2 hunks)app/client/src/ce/reducers/uiReducers/applicationsReducer.tsx(2 hunks)app/client/src/components/editorComponents/WidgetQueryGeneratorForm/index.tsx(2 hunks)app/client/src/components/propertyControls/ButtonTabControl.tsx(2 hunks)app/client/src/index.tsx(1 hunks)app/client/src/reducers/entityReducers/datasourceReducer.ts(7 hunks)app/client/src/reducers/entityReducers/metaReducer/index.ts(4 hunks)app/client/src/reducers/entityReducers/metaReducer/metaReducerUtils.ts(2 hunks)app/client/src/reducers/evaluationReducers/treeReducer.ts(3 hunks)app/client/src/sagas/WidgetAdditionSagas.ts(2 hunks)app/client/src/utils/ReducerUtils.ts(2 hunks)app/client/src/widgets/TableWidgetV2/widget/index.tsx(2 hunks)app/client/src/workers/Evaluation/__tests__/generateOpimisedUpdates.test.ts(27 hunks)app/client/src/workers/Evaluation/evalTreeWithChanges.test.ts(9 hunks)app/client/src/workers/common/DataTreeEvaluator/utils.test.ts(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (15)
- app/client/src/ce/pages/Editor/Explorer/hooks.tsx
- app/client/package.json
- app/client/src/index.tsx
- app/client/src/components/editorComponents/WidgetQueryGeneratorForm/index.tsx
- app/client/src/reducers/entityReducers/datasourceReducer.ts
- app/client/src/ce/reducers/entityReducers/jsActionsReducer.tsx
- app/client/src/reducers/evaluationReducers/treeReducer.ts
- app/client/src/reducers/entityReducers/metaReducer/metaReducerUtils.ts
- app/client/src/components/propertyControls/ButtonTabControl.tsx
- app/client/src/sagas/WidgetAdditionSagas.ts
- app/client/src/WidgetProvider/factory/index.tsx
- app/client/src/ce/reducers/uiReducers/applicationsReducer.tsx
- app/client/src/workers/Evaluation/evalTreeWithChanges.test.ts
- app/client/src/reducers/entityReducers/metaReducer/index.ts
- app/client/src/widgets/TableWidgetV2/widget/index.tsx
🧰 Additional context used
🪛 Biome (1.9.4)
app/client/src/workers/Evaluation/__tests__/generateOpimisedUpdates.test.ts
[error] 373-374: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 374-377: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
app/client/src/workers/common/DataTreeEvaluator/utils.test.ts
[error] 191-191: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
🔇 Additional comments (4)
app/client/src/utils/ReducerUtils.ts (2)
2-2: LGTM: Import change aligns with PR objectives.The change from
immertomutativelibrary is consistent with the PR's goal of improving performance.
47-47: LGTM: State mutation using mutative's create function.The replacement of
producewithcreateis correctly implemented.app/client/src/workers/common/DataTreeEvaluator/utils.test.ts (1)
9-9: LGTM: Consistent replacement of immer with mutative across test cases.The transition from
producetocreateis consistently applied across all test cases while maintaining the original test logic and assertions.Also applies to: 173-174, 190-192, 207-209, 247-249, 263-265, 284-287, 305-308, 323-329, 336-339, 365-371, 372-375, 380-386, 387-394, 420-426, 427-430, 446-448, 475-481, 482-485, 491-493, 507-509, 532-538, 555-557, 572-574, 586-593, 616-623, 631-640, 661-668
app/client/src/workers/Evaluation/__tests__/generateOpimisedUpdates.test.ts (1)
3-3: LGTM: Comprehensive test coverage maintained with mutative.The transition from
producetocreateis consistently applied across all test cases while maintaining comprehensive test coverage for optimized updates generation, including edge cases with serialization, moment objects, and large collections.Also applies to: 113-115, 122-125, 134-136, 148-150, 165-167, 182-190, 207-215, 247-249, 263-265, 284-287, 305-308, 336-339, 365-371, 372-375, 380-386, 387-394, 420-426, 427-430, 446-448, 475-481, 482-485, 491-493, 507-509, 532-538, 555-557, 572-574, 586-593, 616-623, 631-640, 661-668
| "eslint-plugin-testing-library": "^6.2.0", | ||
| "factory.ts": "^0.5.1", | ||
| "husky": "^8.0.0", | ||
| "immer": "^9.0.6", |
There was a problem hiding this comment.
@vsvamsi1 Is immer still required in dependencies ?
There was a problem hiding this comment.
I have left it in the devDependendices not in the dependencies because there were cypress testcases using it. This won't show up on the client prod build. I tried substituting them in the cypress testcases with mutative, however im seeing some errors accessing the mutative packages in cypress commands. I will reach out to @sagar-qa007 to help resolve this and remove them completely. This part of it can be resolved in a separate PR.
There was a problem hiding this comment.
Sounds good. We can merge the changes for now. I have also added a backlog item for it here
What kind of error are we facing ?
There was a problem hiding this comment.
The mutative updates were getting ignored and all feature flag interception logic was not working. So all test cases are failing prematurely at the sidebar assertion itself. I thought it was headless chrome running these test cases does not support proxy, however the reducer updates using mutative were working. It seems specific cypress commands.
cc @sagar-qa007
…sing mutative instead of immer (appsmithorg#38993) ## Description - Using mutative instead of immer, this has reduced the main thread scripting by about 1 second. - Removed a cloneDeep during table onMount which saves about 70ms in main thread scripting. - Bypassed mutative when applying the first tree to reduce the overhead associated to mutative. Fixes #`Issue Number` _or_ Fixes `Issue URL` > [!WARNING] > _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 <!-- This is an auto-generated comment: Cypress test results --> > [!TIP] > 🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉 > Workflow run: <https://github.com/appsmithorg/appsmith/actions/runs/13164792224> > Commit: 4cb8217 > <a href="https://internal.appsmith.com/app/cypress-dashboard/rundetails-65890b3c81d7400d08fa9ee5?branch=master&workflowId=13164792224&attempt=2" target="_blank">Cypress dashboard</a>. > Tags: `@tag.All` > Spec: > <hr>Thu, 06 Feb 2025 04:21:41 UTC <!-- end of auto-generated comment: Cypress test results --> ## Communication Should the DevRel and Marketing teams inform users about this change? - [ ] Yes - [ ] No <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **New Dependency** - Integrated a new library to enhance overall state management. - **Refactor** - Updated state update mechanisms across interactive components and data flows. - Improved table widget processing for more consistent behavior. - **Chore** - Removed legacy development-only configuration settings. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
Suspect IssuesThis pull request was deployed and Sentry observed the following issues:
Did you find this useful? React with a 👍 or 👎 |
4 participants
Description
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/13164792224
Commit: 4cb8217
Cypress dashboard.
Tags:
@tag.AllSpec:
Thu, 06 Feb 2025 04:21:41 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
New Dependency
Refactor
Chore