Conversation
WalkthroughThis pull request refactors the email move-to-trash workflow across multiple controllers and utilities. The changes replace the existing Possibly related PRs
Suggested labels
Suggested reviewers
🚥 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
lib/features/mailbox_dashboard/presentation/extensions/get_trash_mailbox_id_and_path_extension.dart (1)
12-15: Minor: Redundant type cast.The
as String?cast is unnecessary sincenullis already assignable toString?in Dart's type inference.✨ Suggested simplification
final defaultResult = ( trashId: mapDefaultMailboxIdByRole[PresentationMailbox.roleTrash], - trashPath: null as String?, + trashPath: null, );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/features/mailbox_dashboard/presentation/extensions/get_trash_mailbox_id_and_path_extension.dart` around lines 12 - 15, The tuple initializer `defaultResult` contains a redundant cast `null as String?`; remove the unnecessary `as String?` from the `trashPath` field (leave it as `trashPath: null`) so Dart's type inference assigns it to `String?` automatically; update the tuple expression where `defaultResult` is defined in get_trash_mailbox_id_and_path_extension.dart (the `defaultResult` binding that uses `trashId` and `trashPath`) to eliminate the cast.test/features/thread/presentation/mixin/get_trash_mailbox_id_and_path_test.dart (1)
68-98: Duplicate test cases.The test at lines 68-82 ("WHEN selectedMailbox is personal") and lines 84-98 ("WHEN selectedMailbox is not null") have identical test bodies and assertions. Consider consolidating them or differentiating the scenarios being tested.
For example, the second test could verify behavior when
selectedMailboxis a team mailbox instead to test a distinct code path.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/features/thread/presentation/mixin/get_trash_mailbox_id_and_path_test.dart` around lines 68 - 98, Two tests are duplicates: both call mockDashBoardController.getTrashMailboxIdAndPath with selectedMailbox set to Rxn(personalMailbox) and assert the same results; update the second test to cover a different scenario. Replace the second test's setup so mockDashBoardController.selectedMailbox returns Rxn(teamMailbox) (or null if you want to test fallback to emailMailbox) and adjust the expectations for trashId/trashPath accordingly (e.g., expect team-specific trash id/path instead of defaultTrashId or expect non-null trashPath), keeping references to mockDashBoardController, getTrashMailboxIdAndPath, personalMailbox, teamMailbox, and defaultTrashId to locate the code.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lib/features/email/presentation/controller/single_email_controller.dart`:
- Around line 764-765: The code uses
mailboxDashBoardController.getMailboxContain(email) which can return a
presentation-only virtual mailbox (e.g., selectedMailbox) and thus masks the
email's real team mailbox; update the logic in the single_email_controller to
detect virtual/presentation mailboxes (same guard used for bulk actions) and
when encountered call email.findMailboxContain(...) to resolve the actual
containing mailbox before doing the trash lookup — replace or fallback the
currentMailbox value from mailboxDashBoardController.getMailboxContain with the
result of email.findMailboxContain(...) when the mailbox is a
virtual/presentation mailbox.
In
`@lib/features/mailbox_dashboard/presentation/extensions/handle_action_type_for_email_selection.dart`:
- Around line 32-40: The moveToTrash branch in
handle_action_type_for_email_selection.dart currently resolves a single trash
destination from selectedMailbox regardless of selectedMailboxId or per-source
mailboxes; update the logic in the EmailActionType.moveToTrash branch (where
getTrashMailboxIdAndPath, getMailboxIdByRole, destinationMailboxId and
destinationFolderPath are used) to either (a) when selectedMailboxId is
provided, use that mailbox's trash (honor selectedMailboxId) or (b) if operating
on multiple source mailboxes (e.g. search/virtual-folder selections), resolve
the trash mailbox per source mailbox by calling getTrashMailboxIdAndPath for
each source mailboxId before building the request so team mailboxes map to their
own Trash and a chosen Trash in the destination picker is not overwritten.
In `@lib/features/thread/presentation/mixin/email_action_controller.dart`:
- Around line 92-93: The delete/trash lookup currently ignores the explicit
mailboxContain and uses mailboxDashBoardController.selectedMailbox.value; update
the logic so getTrashMailboxIdAndPath or this call honors the passed-in
mailboxContain: either modify getTrashMailboxIdAndPath to prefer its mailbox
argument over selectedMailbox.value, or resolve the correct trash namespace
locally in email_action_controller.dart by deriving the trashId/trashPath from
the mailboxContain before building the request (use the same resolution logic as
in getTrashMailboxIdAndPath but seeded with mailboxContain). Ensure you
reference getTrashMailboxIdAndPath, mailboxDashBoardController, and
mailboxContain when making the change so the explicit mailbox argument is always
preferred.
---
Nitpick comments:
In
`@lib/features/mailbox_dashboard/presentation/extensions/get_trash_mailbox_id_and_path_extension.dart`:
- Around line 12-15: The tuple initializer `defaultResult` contains a redundant
cast `null as String?`; remove the unnecessary `as String?` from the `trashPath`
field (leave it as `trashPath: null`) so Dart's type inference assigns it to
`String?` automatically; update the tuple expression where `defaultResult` is
defined in get_trash_mailbox_id_and_path_extension.dart (the `defaultResult`
binding that uses `trashId` and `trashPath`) to eliminate the cast.
In
`@test/features/thread/presentation/mixin/get_trash_mailbox_id_and_path_test.dart`:
- Around line 68-98: Two tests are duplicates: both call
mockDashBoardController.getTrashMailboxIdAndPath with selectedMailbox set to
Rxn(personalMailbox) and assert the same results; update the second test to
cover a different scenario. Replace the second test's setup so
mockDashBoardController.selectedMailbox returns Rxn(teamMailbox) (or null if you
want to test fallback to emailMailbox) and adjust the expectations for
trashId/trashPath accordingly (e.g., expect team-specific trash id/path instead
of defaultTrashId or expect non-null trashPath), keeping references to
mockDashBoardController, getTrashMailboxIdAndPath, personalMailbox, teamMailbox,
and defaultTrashId to locate the code.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: abd69154-7a02-4020-b1bc-ac99028a0e85
📒 Files selected for processing (12)
lib/features/composer/presentation/extensions/email_action_type_extension.dartlib/features/email/presentation/controller/single_email_controller.dartlib/features/email/presentation/utils/email_action_reactor/email_action_reactor.dartlib/features/mailbox/domain/exceptions/mailbox_exception.dartlib/features/mailbox/presentation/mixin/handle_team_mailbox_mixin.dartlib/features/mailbox/presentation/model/mailbox_node.dartlib/features/mailbox_dashboard/presentation/controller/mailbox_dashboard_controller.dartlib/features/mailbox_dashboard/presentation/extensions/get_trash_mailbox_id_and_path_extension.dartlib/features/mailbox_dashboard/presentation/extensions/handle_action_type_for_email_selection.dartlib/features/thread/presentation/mixin/email_action_controller.darttest/features/mailbox/presentation/extensions/handle_team_mailbox_extension_test.darttest/features/thread/presentation/mixin/get_trash_mailbox_id_and_path_test.dart
| final currentMailbox = mailboxDashBoardController.getMailboxContain(email); | ||
| if (currentMailbox == null) { |
There was a problem hiding this comment.
Virtual folders still mask the real mailbox here.
mailboxDashBoardController.getMailboxContain(email) returns selectedMailbox whenever search isn't running (lib/features/mailbox_dashboard/presentation/extensions/get_mailbox_contain_extension.dart:6-15). If the message is opened from Favorites or another virtual folder, currentMailbox becomes that presentation-only mailbox instead of the email’s actual team mailbox, so the trash lookup below falls back to personal Trash. Mirror the bulk-action virtual-folder guard here and resolve from email.findMailboxContain(...) in that case.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/features/email/presentation/controller/single_email_controller.dart`
around lines 764 - 765, The code uses
mailboxDashBoardController.getMailboxContain(email) which can return a
presentation-only virtual mailbox (e.g., selectedMailbox) and thus masks the
email's real team mailbox; update the logic in the single_email_controller to
detect virtual/presentation mailboxes (same guard used for bulk actions) and
when encountered call email.findMailboxContain(...) to resolve the actual
containing mailbox before doing the trash lookup — replace or fallback the
currentMailbox value from mailboxDashBoardController.getMailboxContain with the
result of email.findMailboxContain(...) when the mailbox is a
virtual/presentation mailbox.
| } else if (actionType == EmailActionType.moveToTrash) { | ||
| destinationMailboxId = getMailboxIdByRole(PresentationMailbox.roleTrash); | ||
| if (selectedMailbox.value?.isChildOfTeamMailboxes == true) { | ||
| final (:trashId, :trashPath) = | ||
| getTrashMailboxIdAndPath(selectedMailbox.value!); | ||
| destinationMailboxId = trashId; | ||
| destinationFolderPath = trashPath; | ||
| } else { | ||
| destinationMailboxId = | ||
| getMailboxIdByRole(PresentationMailbox.roleTrash); |
There was a problem hiding this comment.
Don't resolve one trash mailbox from selectedMailbox.
This branch ignores selectedMailboxId and picks a single destination from the current selection, but the loop below already handles search/virtual-folder selections per email. In global search or virtual folders, team-mailbox messages will still be sent to the personal Trash, and a Trash folder explicitly chosen in the destination picker can be silently replaced. Resolve the trash mailbox per source mailbox, or at least honor selectedMailboxId when it is provided, before building the request.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@lib/features/mailbox_dashboard/presentation/extensions/handle_action_type_for_email_selection.dart`
around lines 32 - 40, The moveToTrash branch in
handle_action_type_for_email_selection.dart currently resolves a single trash
destination from selectedMailbox regardless of selectedMailboxId or per-source
mailboxes; update the logic in the EmailActionType.moveToTrash branch (where
getTrashMailboxIdAndPath, getMailboxIdByRole, destinationMailboxId and
destinationFolderPath are used) to either (a) when selectedMailboxId is
provided, use that mailbox's trash (honor selectedMailboxId) or (b) if operating
on multiple source mailboxes (e.g. search/virtual-folder selections), resolve
the trash mailbox per source mailbox by calling getTrashMailboxIdAndPath for
each source mailboxId before building the request so team mailboxes map to their
own Trash and a chosen Trash in the destination picker is not overwritten.
| final (:trashId, :trashPath) = | ||
| mailboxDashBoardController.getTrashMailboxIdAndPath(mailboxContain); |
There was a problem hiding this comment.
Make the trash lookup honor mailboxContain.
getTrashMailboxIdAndPath() currently prefers selectedMailbox.value over the explicit argument (lib/features/mailbox_dashboard/presentation/extensions/get_trash_mailbox_id_and_path_extension.dart:17-22). In favorites or other virtual-selection contexts, this delete path can ignore the email’s real team mailbox and resolve the wrong trash namespace. Please make the helper prefer the method argument, or resolve the namespace locally here before building the request.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/features/thread/presentation/mixin/email_action_controller.dart` around
lines 92 - 93, The delete/trash lookup currently ignores the explicit
mailboxContain and uses mailboxDashBoardController.selectedMailbox.value; update
the logic so getTrashMailboxIdAndPath or this call honors the passed-in
mailboxContain: either modify getTrashMailboxIdAndPath to prefer its mailbox
argument over selectedMailbox.value, or resolve the correct trash namespace
locally in email_action_controller.dart by deriving the trashId/trashPath from
the mailboxContain before building the request (use the same resolution logic as
in getTrashMailboxIdAndPath but seeded with mailboxContain). Ensure you
reference getTrashMailboxIdAndPath, mailboxDashBoardController, and
mailboxContain when making the change so the explicit mailbox argument is always
preferred.
1 participant
Issue
#4392
User story 1:
Resolved
demo-move-trash.mov
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Tests