docs: add migration docs for mvvm to flowmvi#220
docs: add migration docs for mvvm to flowmvi#220anshu7vyas wants to merge 2 commits intorespawn-app:masterfrom
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
✅ Files skipped from review due to trivial changes (2)
📝 WalkthroughWalkthroughAdds a new "Migration" documentation category and a comprehensive MVVM→FlowMVI migration guide; also reorders the existing "Misc" category position. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related issues
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a comprehensive migration guide from MVVM to FlowMVI, including a new category definition and a detailed markdown document. The guide covers concept mapping, side-by-side code comparisons, UI integration for Compose and Android Views, and incremental adoption strategies. Feedback focused on maintaining consistency in the code examples by standardizing the use of "BuildConfig.DEBUG" for build configuration references across the documentation.
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/docs/migration/mvvm.md`:
- Around line 367-377: The example uses ImmutableContainer<ProfileState,
ProfileIntent, ProfileAction> but then calls reduceLambdas(), which requires the
store's intent type to be LambdaIntent<ProfileState, ProfileAction>; change the
container/store intent type from ProfileIntent to LambdaIntent<ProfileState,
ProfileAction> so the types align (update the ImmutableContainer generic and any
usages like store.intent / loadProfile to use LambdaIntent), keeping
reduceLambdas(), lazyStore, and other symbols (ViewModel, lazyStore, store,
reduceLambdas, ImmutableContainer, ProfileState, ProfileAction) intact.
- Around line 128-133: The examples use unqualified sealed-class members (e.g.,
ClickedLoad, ClickedSave, ShowToast, Loading, Error, DisplayingProfile) which
prevents copy-paste compilation; update all occurrences to fully qualify them
with their enclosing sealed class names (e.g., ProfileIntent.ClickedLoad,
ProfileIntent.ClickedSave, ProfileAction.ShowToast, ProfileState.Loading,
ProfileState.Error, ProfileState.DisplayingProfile) so each match expression and
when branch references the exact symbol; apply this consistently across the
reduce, action, and state example blocks to ensure the snippets compile without
extra imports.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 6fd92127-bc81-4d7f-bbba-abd825055aeb
📒 Files selected for processing (2)
docs/docs/migration/_category_.jsondocs/docs/migration/mvvm.md
Nek-12
left a comment
Nek-12
left a comment
There was a problem hiding this comment.
Pretty good start, but this needs some human, non-ai love, and yes I had a pretty specific vision that i should have explained better beforehand. But this should be gtg after this one pass.
P.S. noticed CI is failing for some reason. I'll fix it on master.
|
|
||
| | MVVM | FlowMVI | Notes | | ||
| |------|---------|-------| | ||
| | `ViewModel` | `Container` / `ViewModel` implementing `ImmutableContainer` | You can keep your ViewModels on Android | |
There was a problem hiding this comment.
ViewModels are kmp, so we should remove "keep on android" and just leave "you can keep VMs or use ContainerViewModel.
| | `StateFlow.value` | `withState {}` | Thread-safe read access | | ||
| | Exposed `StateFlow` | `store.subscribe()` / Compose `subscribe {}` | Lifecycle-aware | | ||
| | Public ViewModel functions | `MVIIntent` sealed classes or `store.intent {}` lambdas | Choose one style per project | | ||
| | `SharedFlow` / `Channel` events | `MVIAction` side effects | Guaranteed delivery with `Distribute()` | |
There was a problem hiding this comment.
"MVIAction side effects" should say "action()" and consume when subscribing or smth similar. types are noise, we should point to functions that enable functionality. ditto everywhere in table
| | Public ViewModel functions | `MVIIntent` sealed classes or `store.intent {}` lambdas | Choose one style per project | | ||
| | `SharedFlow` / `Channel` events | `MVIAction` side effects | Guaranteed delivery with `Distribute()` | | ||
| | `init {}` block | `init` plugin | Runs each time the store starts | | ||
| | `viewModelScope.launch {}` | Logic in `reduce {}`, `init {}`, `whileSubscribed {}` | Structured pipeline | |
There was a problem hiding this comment.
that's wrong, we have PipelineContext.launch
|
|
||
| :::warning | ||
|
|
||
| `LambdaIntent` is a value class wrapping a lambda, which makes it inherently unstable for the Compose |
There was a problem hiding this comment.
this is wrong, the problem is not the instability of lambda intents, it's the instability and proliferation of doThis and doThat functions that are legacy from MVVM style (which ARE unstable).
|
|
||
| ### 5. Add plugins incrementally | ||
|
|
||
| Start with the essentials — `reduce`, `recover`, `init` — then layer in more as needed: |
There was a problem hiding this comment.
this is wrong, we should explain that once people set up DI, they should just create a reusable configuration to never remember about enableLogging crap again. And whileSubscribed is NOT optional - must be used day one. metrics shill is overkill here, we should mention more useful things.
There was a problem hiding this comment.
Removed metrics shill, updated in b65300d
|
@Nek-12 thank you for the detailed review, I have addressed all the comments and updated the doc as per your review. Please let me know if you have any further improvements/fixes in mind. |
2 participants
Summary
PipelineContextexplainer, MVI vs MVVM+ decision guide with Compose stability warning, expanded testing section with before/after comparisons, DI cross-reference, and Compose Multiplatform (KMP) sectionTest plan
docs/passes with no errors<details>blocks render correctly in browserdi.md,compose.md,testing.md,android.md,savedstate.md)Summary by CodeRabbit