Upgrade flutter_riverpod to 3.x and adopt @riverpod codegen per ADR-0092#4573
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThis PR upgrades Riverpod to v3.3.1 and migrates the codebase from manual provider declarations to code-generated Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
![codescene-delta-analysis[bot]](https://avatars.githubusercontent.com/in/53921?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
lib/features/composer/presentation/providers/composer_auto_save_notifier.dart (1)
50-62: 💤 Low valueOptional: Use
ref.mountedand drop the manual_mountedflagWith
flutter_riverpod: ^3.3.1(Riverpod 3.x),ref.mountedflips tofalsewhen the provider is disposed, so_mounted+ref.onDispose(() => _mounted = false)is redundant. Non-blocking—current implementation is correct.♻️ Proposed simplification
class ComposerAutoSaveNotifier extends _$ComposerAutoSaveNotifier { - bool _mounted = true; late final ResolveComposerCacheForRestoreInteractor _resolveInteractor; late final RemoveAllComposerCacheInteractor _removeInteractor; `@override` ComposerAutoSaveState build(String composerId) { _resolveInteractor = ref.read(resolveComposerCacheForRestoreProvider); _removeInteractor = ref.read(removeAllComposerCacheProvider); - ref.onDispose(() => _mounted = false); return const ComposerAutoSaveState(); } - bool get mounted => _mounted; + bool get mounted => ref.mounted;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/features/composer/presentation/providers/composer_auto_save_notifier.dart` around lines 50 - 62, The manual _mounted flag and ref.onDispose cleanup in ComposerAutoSaveNotifier.build should be removed and replaced by using ref.mounted; delete the _mounted field and its ref.onDispose call, update the mounted getter to return ref.mounted, and keep _resolveInteractor and _removeInteractor initialization inside build (referencing build, _mounted, ref.onDispose, and the mounted getter to locate the code).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In
`@lib/features/composer/presentation/providers/composer_auto_save_notifier.dart`:
- Around line 50-62: The manual _mounted flag and ref.onDispose cleanup in
ComposerAutoSaveNotifier.build should be removed and replaced by using
ref.mounted; delete the _mounted field and its ref.onDispose call, update the
mounted getter to return ref.mounted, and keep _resolveInteractor and
_removeInteractor initialization inside build (referencing build, _mounted,
ref.onDispose, and the mounted getter to locate the code).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6f8eba3c-53fe-42fe-9dbc-43c59ff93dd7
⛔ Files ignored due to path filters (1)
pubspec.lockis excluded by!**/*.lock
📒 Files selected for processing (21)
lib/features/caching/clients/composer_hive_cache_client.dartlib/features/composer/presentation/extensions/handle_mobile_auto_save_extension.dartlib/features/composer/presentation/providers/composer_auto_save_notifier.dartlib/features/composer/presentation/providers/composer_cache_providers.dartlib/features/mailbox_dashboard/data/datasource_impl/composer_persistent_cache_datasource_impl.dartlib/features/mailbox_dashboard/data/repository/composer_cache_repository_impl.dartlib/features/mailbox_dashboard/domain/usecases/mark_composer_cache_clean_close_interactor.dartlib/features/mailbox_dashboard/domain/usecases/remove_all_composer_cache_interactor.dartlib/features/mailbox_dashboard/domain/usecases/resolve_composer_cache_for_restore_interactor.dartlib/features/mailbox_dashboard/presentation/bindings/mailbox_dashboard_bindings.dartlib/features/mailbox_dashboard/presentation/extensions/handle_composer_restore_on_mobile_extension.dartlib/features/manage_account/presentation/preferences/preferences_controller.dartlib/features/manage_account/presentation/providers/local_settings_notifier.dartlib/features/manage_account/presentation/services/local_settings_service.dartlib/features/search/email/presentation/search_email_controller.dartlib/features/thread/presentation/thread_controller.dartlib/main.dartlib/main/exceptions/thrower/cache_exception_thrower.dartlib/main/providers/app_provider_container.dartpubspec.yamltest/features/composer/presentation/providers/composer_auto_save_notifier_test.dart
💤 Files with no reviewable changes (1)
- lib/features/composer/presentation/providers/composer_cache_providers.dart
|
This PR has been deployed to https://linagora.github.io/tmail-flutter/4573. |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
…d riverpod_generator
….family to @riverpod Notifier
dab246
force-pushed
the
feat/riverpod-3-upgrade
branch
from
a52bf22 to
6558afe
Compare
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lib/features/composer/presentation/providers/composer_auto_save_notifier.dart (1)
79-110:⚠️ Potential issue | 🟠 Major | ⚡ Quick winGuard
statemutations that occur afterawaitwith themountedcheck.Both
restoreandclearCachesetstateafter anawait. Per the doc comment (Lines 46–47), this provider is explicitly invalidated fromComposerController.onClose. If invalidation happens while one of these async calls is in flight, the post-awaitstate = ...will throw aStateErrorbecause the notifier has been disposed. The newmountedgetter exists but isn't used to guard these mutations — a regression from the previously_mounted-guardedStateNotifier.🛡️ Proposed fix to guard state after async gaps
(success) { if (success is! ResolveComposerCacheForRestoreSuccess) return null; final cache = success.cache; - if (cache != null) { + if (cache != null && mounted) { state = state.copyWith(hasRecoverableSnapshot: true); log('ComposerAutoSaveNotifier::restore: found snapshot'); } return cache; },(failure) => log('ComposerAutoSaveNotifier::clearCache: failure=${failure.runtimeType}'), (_) { - state = state.copyWith(hasRecoverableSnapshot: false); - log('ComposerAutoSaveNotifier::clearCache: cleared'); + if (mounted) { + state = state.copyWith(hasRecoverableSnapshot: false); + } + log('ComposerAutoSaveNotifier::clearCache: cleared'); },🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/features/composer/presentation/providers/composer_auto_save_notifier.dart` around lines 79 - 110, Both restore and clearCache mutate state after awaiting async interactor calls; guard those post-await state assignments with the notifier's mounted check. Specifically, in restore (method restore) after the await/result.fold and before any state = ... or log that assumes notifier alive, check if (!mounted) return null (or return early) before setting state or returning cache; in clearCache (method clearCache) do the same: after the await/result.fold return early if not mounted before calling state = state.copyWith(...). Use the existing mounted getter to avoid StateError when the provider was invalidated mid-flight.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In
`@lib/features/composer/presentation/providers/composer_auto_save_notifier.dart`:
- Around line 79-110: Both restore and clearCache mutate state after awaiting
async interactor calls; guard those post-await state assignments with the
notifier's mounted check. Specifically, in restore (method restore) after the
await/result.fold and before any state = ... or log that assumes notifier alive,
check if (!mounted) return null (or return early) before setting state or
returning cache; in clearCache (method clearCache) do the same: after the
await/result.fold return early if not mounted before calling state =
state.copyWith(...). Use the existing mounted getter to avoid StateError when
the provider was invalidated mid-flight.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 95187efe-2e69-4af2-8f94-64aa78bf3082
📒 Files selected for processing (2)
lib/features/composer/presentation/providers/composer_auto_save_notifier.dartpubspec.yaml
🚧 Files skipped from review as they are similar to previous changes (1)
- pubspec.yaml
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
9clg6
left a comment
9clg6
left a comment
There was a problem hiding this comment.
Well done !
Some comments but nothing huge
|
Glad to see some consensual changes ;-) I'm interested in @dab246 @tddang-linagora @hoangdat opinion:
|
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Signed-off-by: dab246 <tdvu@linagora.com>
There was a problem hiding this comment.
Our agent can fix these. Install it.
Gates Passed
Absence of Expected Change Pattern
- tmail-flutter/lib/features/mailbox_dashboard/data/repository/composer_cache_repository_impl.dart is usually changed with: tmail-flutter/lib/features/mailbox_dashboard/data/datasource_impl/composer_session_cache_datasource_impl.dart, tmail-flutter/lib/features/mailbox_dashboard/domain/repository/composer_cache_repository.dart
Quality Gate Profile: The Bare Minimum
Install CodeScene MCP: safeguard and uplift AI-generated code. Catch issues early with our IDE extension and CLI tool.
for me it not much more different, but we can move forward to have first step to apply riverpod. With these, we hope we can easily to develop new features with Riverpod than continue to depend on our ghost controller. But tbh, the very big challenge: all our UI depend on GetX (Obx), to completely apply Riverpod, we still have huge work to do.
We are trying to generalized the way to apply Riverpod. it is in progress, the first function we apply is #4576.
We will discover more and more during the way we apply Riverpod, hope we can soon generalize it to other functions |
|
Thanks for the honest feedback |
6 participants
Description
Implements ADR-0092.
Related
Dependency changes
flutter_riverpod^2.6.1^3.3.13.3.1riverpod_annotation^4.0.2riverpod_generator4.0.4-dev.3freezed_annotation^2.4.4^3.0.0Code changes
main.dartGetMaterialAppwithUncontrolledProviderScope(container: appProviderContainer)— shares container between widget tree and GetX layerLocalSettingsNotifierStateNotifier+ manualStateNotifierProvider→@Riverpod(keepAlive: true)Notifier. Provider namelocalSettingsProviderunchanged; 4 call sites unaffectedcomposer_cache_providers.dart. Move each provider inline to its class file with@Riverpod(keepAlive: true)andref.watchComposerAutoSaveNotifierStateNotifier+StateNotifierProvider.family→@Riverpod(keepAlive: true)Notifier family.mountedreplaced by_mountedviaref.onDispose. Interactors resolved aslate finalinbuild()ComposerAutoSaveNotifiertests switch from direct instantiation toProviderContainerwith overridesComposer cache provider chain (after)
composer_hive_cache_client.dartcomposerHiveCacheClientProvidercache_exception_thrower.dartcacheExceptionThrowerProvidercomposer_persistent_cache_datasource_impl.dartcomposerCacheDatasourceProvidercomposerHiveCacheClientProvider,cacheExceptionThrowerProvidercomposer_cache_repository_impl.dartcomposerCacheRepositoryProvidercomposerCacheDatasourceProviderresolve_composer_cache_for_restore_interactor.dartresolveComposerCacheForRestoreProvidercomposerCacheRepositoryProviderremove_all_composer_cache_interactor.dartremoveAllComposerCacheProvidercomposerCacheRepositoryProvidermark_composer_cache_clean_close_interactor.dartmarkComposerLocalCacheCleanCloseProvidercomposerCacheRepositoryProviderWhat is NOT in this PR
appProviderContaineris frozen — no new call sites. The 6 existing call sites below remain and will be removed in future ADRs as each controller migrates toConsumerWidget.LocalSettingsService.read(localSettingsProvider.notifier).update()PreferencesController.read(localSettingsProvider.notifier).update()ThreadController.read(localSettingsProvider)+.listen(...)SearchEmailController.read(localSettingsProvider)HandleMobileAutoSaveExtension.read(composerAutoSaveProvider(id).notifier)+.invalidate(...)HandleComposerRestoreOnMobileExtension.read(resolveComposerCacheForRestoreProvider)Summary by CodeRabbit
Release Notes
Refactor
Chores