TF-4479 Clear trash subfolders with Riverpod state management#4576
TF-4479 Clear trash subfolders with Riverpod state management#4576dab246 wants to merge 10 commits into
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:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (36)
💤 Files with no reviewable changes (1)
✅ Files skipped from review due to trivial changes (2)
🚧 Files skipped from review as they are similar to previous changes (27)
WalkthroughThis PR refactors trash folder emptying from synchronous controller execution to an asynchronous stream-based provider pattern. It introduces an 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.
|
How about to completely move all the logic of clear trash into a specific provider? |
The execution logic is already fully contained in EmptyTrashNotifier. What remains in the widget are UI-only side effects: showing toasts (which require BuildContext) and dispatching GetX actions (which require the controller). These responsibilities cannot live in a pure Riverpod provider. The current separation, business logic in the provider and UI reactions in the widget, is both intentional and the correct architectural boundary. |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
This PR has been deployed to https://linagora.github.io/tmail-flutter/4576. |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
dab246
force-pushed
the
enhancement/tf-4479-remove-trash-sub-folders-r3
branch
from
88b255d to
cced3ce
Compare
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
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.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lib/features/mailbox_dashboard/presentation/controller/mailbox_dashboard_controller.dart (1)
1891-1893:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winResolve the selected team trash before falling back to personal trash.
This branch only accepts
selectedMailboxwhenisTrash == true. But the same controller enables the empty-trash flow forisTrashTeamMailboxon Lines 2417-2430, andDeleteActionType.allreaches this method without passingtrashMailboxon Line 1875. From a team-trash screen, this can fall through tomapDefaultMailboxIdByRole[roleTrash]and purge the personal trash instead of the selected team trash.Proposed fix
- final trashFolder = trashMailbox - ?? (selectedMailbox.value?.isTrash == true ? selectedMailbox.value : null) + final trashFolder = trashMailbox + ?? ((selectedMailbox.value?.isTrash == true || + (selectedMailbox.value?.isTrashTeamMailbox == true && + selectedMailbox.value?.myRights?.mayRemoveItems == true)) + ? selectedMailbox.value + : null) ?? mapMailboxById[mapDefaultMailboxIdByRole[PresentationMailbox.roleTrash]];🤖 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/mailbox_dashboard/presentation/controller/mailbox_dashboard_controller.dart` around lines 1891 - 1893, The current fallback for trashFolder ignores selected team trash and can pick the personal trash; update the selection logic so selectedMailbox is accepted when it's either a personal trash or a team trash. Concretely, change the conditional around selectedMailbox in the expression that builds trashFolder (the code referencing trashMailbox, selectedMailbox, isTrash, isTrashTeamMailbox, mapMailboxById and mapDefaultMailboxIdByRole[PresentationMailbox.roleTrash]) to treat selectedMailbox.value as valid when selectedMailbox.value?.isTrash == true || selectedMailbox.value?.isTrashTeamMailbox == true so team-trash screens resolve to the selected team trash before falling back to the default personal trash.
🧹 Nitpick comments (1)
test/features/mailbox/repository/mailbox_respository_test.dart (1)
289-301: ⚡ Quick winStrengthen this test to validate non-empty error passthrough.
The setup at Line 293-294 returns an empty network error map, so Line 300 only proves “empty stays empty.” It doesn’t validate the stated behavior (“original error map is returned”) when the map is non-empty.
Proposed test tightening
test( 'SHOULD return the original error map \n' 'WHEN local cache update throws', () async { - when(mailboxDataSource.deleteMultipleMailbox(sessionFixture, accountIdFixture, [mailboxIdA])) - .thenAnswer((_) async => {}); + final networkErrors = { + mailboxIdB.id: SetError(ErrorType('serverFail')), + }; + when(mailboxDataSource.deleteMultipleMailbox(sessionFixture, accountIdFixture, [mailboxIdA, mailboxIdB])) + .thenAnswer((_) async => networkErrors); when(mailboxCacheDataSourceImpl.update(accountIdFixture, userNameFixture, destroyed: anyNamed('destroyed'))) .thenThrow(Exception('cache error')); - final result = await mailboxRepository.deleteMultipleMailbox(sessionFixture, accountIdFixture, [mailboxIdA]); + final result = await mailboxRepository.deleteMultipleMailbox(sessionFixture, accountIdFixture, [mailboxIdA, mailboxIdB]); - expect(result, isEmpty); + expect(result, same(networkErrors)); + verify(mailboxCacheDataSourceImpl.update( + accountIdFixture, + userNameFixture, + destroyed: [mailboxIdA], + )).called(1); });🤖 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 `@test/features/mailbox/repository/mailbox_respository_test.dart` around lines 289 - 301, The test currently stubs mailboxDataSource.deleteMultipleMailbox to return an empty map so it only proves emptiness is preserved; change the stub for mailboxDataSource.deleteMultipleMailbox (used by mailboxRepository.deleteMultipleMailbox) to return a non-empty error map (e.g. {'someError': 'details'}) and keep mailboxCacheDataSourceImpl.update throwing Exception('cache error'), then assert that the result from mailboxRepository.deleteMultipleMailbox equals that same non-empty error map to verify proper passthrough; reference the mailboxRepository.deleteMultipleMailbox call and the stubs for mailboxDataSource.deleteMultipleMailbox and mailboxCacheDataSourceImpl.update when making this change.
🤖 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/mailbox_dashboard/presentation/controller/mailbox_dashboard_controller.dart`:
- Around line 1891-1893: The current fallback for trashFolder ignores selected
team trash and can pick the personal trash; update the selection logic so
selectedMailbox is accepted when it's either a personal trash or a team trash.
Concretely, change the conditional around selectedMailbox in the expression that
builds trashFolder (the code referencing trashMailbox, selectedMailbox, isTrash,
isTrashTeamMailbox, mapMailboxById and
mapDefaultMailboxIdByRole[PresentationMailbox.roleTrash]) to treat
selectedMailbox.value as valid when selectedMailbox.value?.isTrash == true ||
selectedMailbox.value?.isTrashTeamMailbox == true so team-trash screens resolve
to the selected team trash before falling back to the default personal trash.
---
Nitpick comments:
In `@test/features/mailbox/repository/mailbox_respository_test.dart`:
- Around line 289-301: The test currently stubs
mailboxDataSource.deleteMultipleMailbox to return an empty map so it only proves
emptiness is preserved; change the stub for
mailboxDataSource.deleteMultipleMailbox (used by
mailboxRepository.deleteMultipleMailbox) to return a non-empty error map (e.g.
{'someError': 'details'}) and keep mailboxCacheDataSourceImpl.update throwing
Exception('cache error'), then assert that the result from
mailboxRepository.deleteMultipleMailbox equals that same non-empty error map to
verify proper passthrough; reference the mailboxRepository.deleteMultipleMailbox
call and the stubs for mailboxDataSource.deleteMultipleMailbox and
mailboxCacheDataSourceImpl.update when making this change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2fa8f2f0-ee0f-4474-9a67-51ff4bbb57b5
📒 Files selected for processing (8)
lib/features/mailbox/data/repository/mailbox_repository_impl.dartlib/features/mailbox_dashboard/domain/state/empty_folder_state.dartlib/features/mailbox_dashboard/presentation/controller/mailbox_dashboard_controller.dartlib/features/mailbox_dashboard/presentation/delegates/dashboard_provider_listener_delegate.dartlib/features/mailbox_dashboard/presentation/delegates/empty_folder_provider_listener_delegate.dartlib/features/mailbox_dashboard/presentation/providers/empty_folder_provider.dartlib/features/mailbox_dashboard/presentation/riverpod_widgets/mailbox_dashboard_provider_listener_widget.darttest/features/mailbox/repository/mailbox_respository_test.dart
🚧 Files skipped from review as they are similar to previous changes (2)
- lib/features/mailbox_dashboard/presentation/delegates/dashboard_provider_listener_delegate.dart
- lib/features/mailbox_dashboard/presentation/delegates/empty_folder_provider_listener_delegate.dart
Fixed |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
hi @jbrnzl @C. it is in better shape, can you take a look and give us the feedback. Thank you |
| @@ -0,0 +1,7 @@ | |||
| import 'package:core/presentation/state/failure.dart'; | |||
|
|
|||
| abstract class UrgentExceptionHandler { | |||
There was a problem hiding this comment.
IMO, it is not in use, why not create a separate instance for it and use directly with EmptyFolderProviderListenerDelegate?
There was a problem hiding this comment.
It's still in use. It's used in the listener, and Riverpod uses it via GetX binding.
| import 'package:flutter_riverpod/flutter_riverpod.dart'; | ||
| import 'package:tmail_ui_user/features/mailbox_dashboard/presentation/delegates/dashboard_provider_listener_delegate.dart'; | ||
|
|
||
| class MailboxDashboardProviderListenerWidget extends ConsumerStatefulWidget { |
There was a problem hiding this comment.
with the idea of delegate, in the future controller will use as a place to keep the data, variable, ...?
There was a problem hiding this comment.
No. The controller will all be moved to Riverpod later.
| BuildContext context, { | ||
| required SubfoldersDeleteStatus subfoldersStatus, | ||
| }) { | ||
| final appToast = getBinding<AppToast>(); |
There was a problem hiding this comment.
how about we should minimize using getBinding to use riverpod more?
create AppToast with riverpod the use it here? wdyt?
There was a problem hiding this comment.
AppToast is independent of any classes. Therefore, we can convert it to RiverPod for use. Fixed
| import 'package:tmail_ui_user/main/routes/route_navigation.dart'; | ||
|
|
||
| extension ExecuteEmptyTrashExtension on MailboxDashBoardController { | ||
| List<MailboxId> childMailboxIds(PresentationMailbox parent) => mapMailboxById |
There was a problem hiding this comment.
should move it as a extension of Map, no need to make extension of controller bigger.
| .map((m) => m.id) | ||
| .toList(); | ||
|
|
||
| void requestEmptyTrash( |
There was a problem hiding this comment.
why need to define as a method in extension? we should find the way to reduce extension as much as possible.
how about to separate it as a dialog and have the callback? we can eliminate this extension?
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Signed-off-by: dab246 <tdvu@linagora.com>
…on and appToastProvider
dab246
force-pushed
the
enhancement/tf-4479-remove-trash-sub-folders-r3
branch
from
034ee22 to
cf6f0a6
Compare
There was a problem hiding this comment.
(4 files improve in Code Health)
Gates Failed
(1 hotspot with Lines of Code in a Single File)
Our agent can fix these. Install it.
Gates Passed
Reason for failure
| Prevent hotspot decline | Violations | Code Health Impact | |
|---|---|---|---|
| app_localizations.dart | 1 rule in this hotspot | 8.55 → 8.03 | Suppress |
View Improvements
| File | Code Health Impact | Categories Improved |
|---|---|---|
| mailbox_dashboard_controller.dart | 6.66 → 6.68 | Lines of Declarations in a Single File, Low Cohesion, Complex Method |
| mailbox_dashboard_view_web.dart | 6.75 → 6.78 | Overall Code Complexity |
| mailbox_dashboard_controller_test.dart | 9.00 → 9.00 | Large Method |
| mailbox_dashboard_view_widget_test.dart | 9.38 → 9.38 | Large Method |
Absence of Expected Change Pattern
- tmail-flutter/lib/features/mailbox/data/repository/mailbox_repository_impl.dart is usually changed with: tmail-flutter/lib/features/mailbox/data/datasource_impl/mailbox_cache_datasource_impl.dart, tmail-flutter/lib/features/mailbox/data/datasource_impl/mailbox_datasource_impl.dart, tmail-flutter/lib/features/mailbox/data/datasource/mailbox_datasource.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.
|
Why I saw this in a couple of code scene PRs and it gets me curious... |
I think we discussed this in a previous PR, perhaps you've forgotten (#4553 (comment)). Basically, it's because app_localizations.dart is the only file containing declarations for all Strings in the app, so the number of lines of code in this file is currently exceeding the Code Scene limit, leading to the error |
4 participants
Issue
#4479
Summary
DeleteTrashSubfoldersNotifier(@riverpod, sealed state hierarchy) to handle async deletion of trash subfoldersMailboxProviderListenerWidgetat mailbox view scope to listen to notifier state (toast + refresh)DeleteTrashSubfoldersNotifierSummary by CodeRabbit
Release Notes
New Features
Bug Fixes