using mutative #38950
using mutative #38950
Conversation
vsvamsi1
requested review from
ApekshaBhosale,
ayushpahwa,
a team and
KelvinOm
as code owners
WalkthroughThis pull request replaces the usage of the Changes
Sequence Diagram(s)sequenceDiagram
participant Code as Application Code
participant Mutative as mutative Library
participant Reducer as Reducer/State Manager
participant Test as Automated Tests
Code->>Mutative: Call create(state, draft => {...})
Mutative-->>Code: Return updated state
Code->>Reducer: Update state using new mechanism
Reducer-->>Code: State updated response
Test->>Mutative: Invoke create in test cases
Mutative-->>Test: Provide drafted state for validation
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
✨ 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
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
app/client/src/utils/ReducerUtils.ts (1)
23-42: Consider renamingcreateImmerReducerto reflect the use of mutative.The function name is now misleading as it uses the
mutativelibrary instead ofimmer.-export const createImmerReducer = ( +export const createMutativeReducer = (🧰 Tools
🪛 Biome (1.9.4)
[error] 34-34: Do not access Object.prototype method 'hasOwnProperty' from target object.
It's recommended using Object.hasOwn() instead of using Object.hasOwnProperty().
See MDN web docs for more details.(lint/suspicious/noPrototypeBuiltins)
app/client/src/components/propertyControls/ButtonTabControl.tsx (1)
65-67: Simplify the mutative array update.The current implementation can be simplified by returning the new array directly.
- const updatedValues: string[] = create(values, (draft: string[]) => { - draft.push(value); - }); + const updatedValues = create(values, (draft: string[]) => { + return [...draft, value]; + });
📜 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 (16)
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/sagas/WidgetAdditionSagas.ts(2 hunks)app/client/src/utils/ReducerUtils.ts(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/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)
⏰ 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-check-cyclic-deps / check-cyclic-dependencies
- GitHub Check: client-build / client-build
- GitHub Check: client-prettier / prettier-check
- GitHub Check: client-lint / client-lint
🔇 Additional comments (15)
app/client/src/utils/ReducerUtils.ts (1)
35-37: LGTM! Clean migration to mutative.The migration pattern correctly preserves immutability semantics while switching to the
mutativelibrary.app/client/src/index.tsx (1)
25-26: Verify development-time mutation detection after removing autofreeze.With the removal of Immer's autofreeze, ensure that
mutativeprovides equivalent development-time safeguards against accidental mutations.app/client/src/reducers/entityReducers/metaReducer/metaReducerUtils.ts (1)
85-91: LGTM! Clean migration preserving nested update logic.The migration to
mutativecorrectly maintains the complex state update logic using path-based setters.app/client/src/reducers/entityReducers/metaReducer/index.ts (1)
14-14: LGTM! State updates look correct with mutative.The transition from
immer'sproducetomutative'screateis implemented correctly across all reducer cases. The immutability pattern is preserved.Also applies to: 41-51, 57-67, 73-83
app/client/src/workers/common/DataTreeEvaluator/utils.test.ts (1)
9-9: LGTM! Test cases correctly updated to use mutative.The transition to
mutativeis correctly implemented in test cases. While there's a static analysis warning about thedeleteoperator, it's acceptable in test code.Also applies to: 173-176, 190-192, 198-201, 216-218, 224-227, 242-245
app/client/src/components/editorComponents/WidgetQueryGeneratorForm/index.tsx (1)
2-2: LGTM! State updates in updateConfig are correctly implemented.The transition to
mutativemaintains the same state update behavior while preserving all edge case handling.Also applies to: 182-223
app/client/src/ce/pages/Editor/Explorer/hooks.tsx (1)
9-9: LGTM! Hooks correctly updated to use mutative.The transition to
mutativepreserves the existing filtering logic and performance measurements. Array mutations are handled correctly.Also applies to: 187-208, 228-241, 259-270
app/client/src/ce/reducers/entityReducers/jsActionsReducer.tsx (1)
9-9: LGTM! Clean migration from immer to mutative.The migration maintains the same immutable state update pattern and functionality.
Also applies to: 403-418
app/client/src/workers/Evaluation/evalTreeWithChanges.test.ts (1)
7-7: LGTM! Consistent migration in test cases.All test cases have been updated to use
mutativewhile maintaining the same test coverage and assertions.Also applies to: 262-265, 314-316, 349-353, 389-393, 429-433, 459-463, 498-502, 536-540
app/client/src/sagas/WidgetAdditionSagas.ts (1)
23-23: LGTM! Clean migration in widget props handling.The state update logic for widget props remains intact with the new library.
Also applies to: 115-119
app/client/src/WidgetProvider/factory/index.tsx (1)
38-38: LGTM! Clean migration in widget property pane configuration.The property pane configuration update logic remains intact with the new library.
Also applies to: 421-423
app/client/package.json (1)
164-164: Verify mutative version compatibility.The addition of mutative alongside immer could lead to potential state management conflicts. Consider removing immer after the migration is complete.
Run the following script to check for any version conflicts or security advisories:
✅ Verification successful
Mutative version compatibility verified.
- The npm query confirms that mutative is correctly set to version 1.1.0.
- No security advisories or reported vulnerabilities were found.
- Although both mutative and immer coexist, no inherent version conflicts were detected. Consider removing immer after the migration is complete if it's no longer needed.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for version conflicts and security advisories # Check npm for latest version and any security advisories curl -s "https://registry.npmjs.org/mutative" | jq '.["dist-tags"].latest,.time.modified' # Check for any reported vulnerabilities gh api graphql -f query=' { securityVulnerabilities(first: 5, ecosystem: NPM, package: "mutative") { nodes { advisory { summary severity publishedAt } vulnerableVersionRange firstPatchedVersion { identifier } } } }'Length of output: 466
app/client/src/reducers/entityReducers/datasourceReducer.ts (1)
469-476: LGTM! Clean migration to mutative.The state updates using mutative's create function are implemented correctly, maintaining immutability and type safety.
Also applies to: 659-661, 667-673, 679-685, 688-690, 696-701, 707-712, 715-717, 723-728, 734-739
app/client/src/workers/Evaluation/__tests__/generateOpimisedUpdates.test.ts (1)
113-115: LGTM! Test cases properly updated.The migration to mutative's create function is thorough and maintains test coverage. The use of delete operator in tests is acceptable.
Also applies to: 122-125, 134-136, 148-150, 182-184, 207-209, 210-215, 247-249, 305-308, 336-339, 380-386, 387-394, 446-448, 475-481, 482-485, 491-493, 507-509, 555-557, 586-593, 631-640, 661-668
app/client/src/ce/reducers/uiReducers/applicationsReducer.tsx (1)
533-542: LGTM! Proper null checks in place.The state update using mutative's create function is implemented correctly with proper null checks for optional navigationSetting.
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/13095907366
Commit: f99a60c
Cypress dashboard.
Tags:
@tag.AllSpec:
Sun, 02 Feb 2025 07:18:51 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
Chores / Refactor
Tests
These changes improve maintainability and internal performance while keeping user-facing functionality consistent.