TF-4358 Remove mail in trash by default in search result#4361
TF-4358 Remove mail in trash by default in search result#4361
Conversation
|
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 (2)
WalkthroughAdds a new presentation mailbox Possibly related PRs
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 |
|
This PR has been deployed to https://linagora.github.io/tmail-flutter/4361. |
dab246
force-pushed
the
features/feb26_sprint
branch
from
391f304 to
e8ad5f4
Compare
dab246
force-pushed
the
features/tf-4358-remove-mail-in-trash-by-default-in-search-result
branch
from
154a21b to
d332deb
Compare
|
The diff is massive. 24 file change is scary. |
|
Is the diff too large for the subject? Yes. 24 files, +459/-186 for a feature that adds an "All Email, trash & spam" filter to search. The central commit (37f2eff, +376/-166 alone) bundles 3 distinct concerns. Concrete review suggestions
It mixes:
The refactoring is a good idea (better testability, no widget tree dependency), but it deserves its own isolated commit to make review easier.
This renames a value in a shared enum. Worth asking in review: are there any other usages of select elsewhere in the app that were missed? A grep should confirm no call site was left behind. The rename
The view was restructured to pass appLocalizations and isDesktop as parameters to _buildListMailbox / _buildListMailboxSearched. This is a full-view refactor for a minor functional addition — it
Worth checking: does this extension duplicate logic already in presentation_mailbox_extension.dart added in 37f2eff? Both files deal with mailboxes for search — risk of scattered responsibilities.
Only advanced_filter_controller_test.dart is touched (and shrunk: -7/+5). No tests for:
Summary: the implementation is directionally correct, but the PR should have been structured as 3 clean commits — (1) enum rename, (2) AppLocalizations propagation refactor, (3) the actual feature. |
|
Do not implement this as a fix but more as an info notice For what it is worth |
chibenwa
left a comment
chibenwa
left a comment
There was a problem hiding this comment.
I disagree with this change which denotes clear misdesign of the overall application
dab246
force-pushed
the
features/tf-4358-remove-mail-in-trash-by-default-in-search-result
branch
from
a55a756 to
45d4878
Compare
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
test/features/mailbox_dashboard/presentation/controller/advanced_filter_controller_test.dart (1)
207-217:⚠️ Potential issue | 🟡 MinorAssert the restored mailbox too.
initSearchFilterField()now rehydratesdestinationMailboxSelectedfrom_memorySearchFilter.mailbox, but this test never checks that state. Since this PR changes the special search-scope mailboxes, add an assertion here so a regression in the restored mailbox does not slip through.🧪 Suggested assertion
expect(advancedFilterController.receiveTimeType.value, equals(EmailReceiveTimeType.last7Days)); expect(advancedFilterController.sortOrderType.value, equals(EmailSortOrderType.oldest)); + expect( + advancedFilterController.destinationMailboxSelected.value?.id, + equals(memorySearchFilter.mailbox?.id), + ); expect(advancedFilterController.hasAttachment.value, equals(true));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/features/mailbox_dashboard/presentation/controller/advanced_filter_controller_test.dart` around lines 207 - 217, The test calls advancedFilterController.initSearchFilterField() but does not assert that the mailbox was rehydrated; add an assertion verifying advancedFilterController.destinationMailboxSelected equals the mailbox stored in the in-memory filter (the value set in _memorySearchFilter.mailbox or the specific mailbox used in the test setup) so changes to restored mailbox logic are covered. Ensure the assertion references advancedFilterController.destinationMailboxSelected and the same mailbox instance/value used in the test setup.lib/features/mailbox_dashboard/presentation/model/search/search_email_filter.dart (1)
208-260:⚠️ Potential issue | 🟠 MajorTreat
allEmailTrashAndSpamFolderas a real scope change.Line 216 and Line 231 currently use
isUnifiedMailbox, but that helper is only correct for the JMAP-shaping logic on Line 246.PresentationMailbox.allEmailTrashAndSpamFolderremoves the default Trash/Spam exclusion, so the query changes and any code gated byisApplied/isOnlyStarredAppliedwill think "nothing changed" for this new scope.💡 Suggested fix
bool get isApplied => from.isNotEmpty || to.isNotEmpty || text?.value.trim().isNotEmpty == true || subject?.trim().isNotEmpty == true || hasKeyword.isNotEmpty || notKeyword.isNotEmpty || emailReceiveTimeType != EmailReceiveTimeType.allTime || sortOrderType != SearchEmailFilter.defaultSortOrder || - (mailbox != null && mailbox?.isUnifiedMailbox != true) || + (mailbox != null && mailbox?.isAllEmail != true) || label != null || hasAttachment || unread; bool get isOnlyStarredApplied => from.isEmpty && to.isEmpty && text?.value.trim().isNotEmpty != true && subject?.trim().isNotEmpty != true && hasKeyword.firstOrNull == KeyWordIdentifier.emailFlagged.value && notKeyword.isEmpty && emailReceiveTimeType == EmailReceiveTimeType.allTime && sortOrderType == SearchEmailFilter.defaultSortOrder && - (mailbox == null || mailbox?.isUnifiedMailbox == true) && + (mailbox == null || mailbox?.isAllEmail == true) && label == null && !hasAttachment && !unread;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/features/mailbox_dashboard/presentation/model/search/search_email_filter.dart` around lines 208 - 260, The checks around mailbox scope should treat PresentationMailbox.allEmailTrashAndSpamFolder as a distinct scope change; update usages in isApplied and isOnlyStarredApplied (and the helper _getInMailboxField/_getInMailboxOtherThanField logic if needed) so they consider mailbox?.isAllEmailTrashAndSpamFolder == true as a non-default scope. Concretely, replace conditions that only test mailbox?.isUnifiedMailbox (e.g., "(mailbox != null && mailbox?.isUnifiedMailbox != true)" and "(mailbox == null || mailbox?.isUnifiedMailbox == true)") with logic that also treats isAllEmailTrashAndSpamFolder as a changed scope (for example: "(mailbox != null && (mailbox?.isUnifiedMailbox != true || mailbox?.isAllEmailTrashAndSpamFolder == true))" and the corresponding inverse for the only-starred check), and ensure _getInMailboxField()/_getInMailboxOtherThanField() similarly recognize isAllEmailTrashAndSpamFolder when deciding which mailbox IDs to return.
🤖 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/base/model/filter_field.dart`:
- Around line 37-38: The mailbox filter field was renamed to use “Search”
(FilterField.mailBox mapping in filter_field.dart) but the corresponding default
option still uses allFolders; update the default/option value (where you return
or map to allFolders) to use allEmail so terminology is consistent across the
advanced-search flow (locate references to FilterField.mailBox and the default
value/allFolders and replace with allEmail).
In
`@lib/features/destination_picker/presentation/destination_picker_controller.dart`:
- Around line 334-337: Add unit tests for the new special-mailbox branch in
selectMailboxAction(): create tests that call selectMailboxAction() with
presentationMailbox set to null, to PresentationMailbox.unifiedMailbox, and to
PresentationMailbox.allEmailTrashAndSpamFolder, and assert that
unAllSelectedMailboxNode() is invoked and the destination state is
updated/unselected accordingly; specifically mock or spy on
unAllSelectedMailboxNode() and verify it was called when presentationMailbox.id
== PresentationMailbox.allEmailTrashAndSpamFolder.id and that the controller's
destination selection state reflects the node being unselected. Ensure tests
reference selectMailboxAction(), unAllSelectedMailboxNode(), and
PresentationMailbox.allEmailTrashAndSpamFolder.
---
Outside diff comments:
In
`@lib/features/mailbox_dashboard/presentation/model/search/search_email_filter.dart`:
- Around line 208-260: The checks around mailbox scope should treat
PresentationMailbox.allEmailTrashAndSpamFolder as a distinct scope change;
update usages in isApplied and isOnlyStarredApplied (and the helper
_getInMailboxField/_getInMailboxOtherThanField logic if needed) so they consider
mailbox?.isAllEmailTrashAndSpamFolder == true as a non-default scope.
Concretely, replace conditions that only test mailbox?.isUnifiedMailbox (e.g.,
"(mailbox != null && mailbox?.isUnifiedMailbox != true)" and "(mailbox == null
|| mailbox?.isUnifiedMailbox == true)") with logic that also treats
isAllEmailTrashAndSpamFolder as a changed scope (for example: "(mailbox != null
&& (mailbox?.isUnifiedMailbox != true || mailbox?.isAllEmailTrashAndSpamFolder
== true))" and the corresponding inverse for the only-starred check), and ensure
_getInMailboxField()/_getInMailboxOtherThanField() similarly recognize
isAllEmailTrashAndSpamFolder when deciding which mailbox IDs to return.
In
`@test/features/mailbox_dashboard/presentation/controller/advanced_filter_controller_test.dart`:
- Around line 207-217: The test calls
advancedFilterController.initSearchFilterField() but does not assert that the
mailbox was rehydrated; add an assertion verifying
advancedFilterController.destinationMailboxSelected equals the mailbox stored in
the in-memory filter (the value set in _memorySearchFilter.mailbox or the
specific mailbox used in the test setup) so changes to restored mailbox logic
are covered. Ensure the assertion references
advancedFilterController.destinationMailboxSelected and the same mailbox
instance/value used in the test setup.
🪄 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: d10c1e59-f4f2-4d75-835d-bc1e9fb619ca
⛔ Files ignored due to path filters (1)
assets/images/ic_all_email.svgis excluded by!**/*.svg
📒 Files selected for processing (33)
core/lib/presentation/resources/image_paths.dartlib/features/base/model/filter_field.dartlib/features/base/widget/default_field/default_autocomplete_input_field_widget.dartlib/features/base/widget/default_field/default_autocomplete_tag_item_widget.dartlib/features/composer/presentation/extensions/prefix_email_address_extension.dartlib/features/composer/presentation/extensions/remove_draggable_email_address_between_recipient_fields_extension.dartlib/features/composer/presentation/model/draggable_email_address.dartlib/features/destination_picker/presentation/destination_picker_controller.dartlib/features/destination_picker/presentation/destination_picker_view.dartlib/features/destination_picker/presentation/widgets/destination_picker_folder_item.dartlib/features/destination_picker/presentation/widgets/destination_picker_search_mailbox_item_builder.dartlib/features/email_recovery/presentation/email_recovery_controller.dartlib/features/email_recovery/presentation/extension/handle_auto_create_email_address_tag_extension.dartlib/features/email_recovery/presentation/extension/handle_drag_email_tag_between_field_extension.dartlib/features/email_recovery/presentation/widgets/email_recovery_form/email_recovery_form_desktop_builder.dartlib/features/email_recovery/presentation/widgets/email_recovery_form/email_recovery_form_mobile_builder.dartlib/features/mailbox/presentation/extensions/presentation_mailbox_extension.dartlib/features/mailbox/presentation/model/mailbox_actions.dartlib/features/mailbox/presentation/widgets/mailbox_item_widget.dartlib/features/mailbox_dashboard/presentation/controller/advanced_filter_controller.dartlib/features/mailbox_dashboard/presentation/controller/mailbox_dashboard_controller.dartlib/features/mailbox_dashboard/presentation/model/search/quick_search_filter.dartlib/features/mailbox_dashboard/presentation/model/search/search_email_filter.dartlib/features/mailbox_dashboard/presentation/widgets/advanced_search/advanced_search_field_widget.dartlib/features/mailbox_dashboard/presentation/widgets/advanced_search/advanced_search_input_form.dartlib/features/mailbox_dashboard/presentation/widgets/advanced_search/icon_open_advanced_search_widget.dartlib/features/search/email/presentation/search_email_controller.dartlib/features/thread/presentation/thread_controller.dartlib/l10n/intl_messages.arblib/main/localizations/app_localizations.dartmodel/lib/mailbox/presentation_mailbox.darttest/features/mailbox_dashboard/presentation/controller/advanced_filter_controller_test.darttest/features/mailbox_dashboard/presentation/controller/mailbox_dashboard_controller_test.dart
lib/features/destination_picker/presentation/destination_picker_controller.dart
Show resolved
Hide resolved
dab246
force-pushed
the
features/tf-4358-remove-mail-in-trash-by-default-in-search-result
branch
from
45d4878 to
fe54ad8
Compare
|
Rebase needed. (today) We will get the code scene review. Let's decide the future of this work once we get the code scene review. |
dab246
force-pushed
the
features/tf-4358-remove-mail-in-trash-by-default-in-search-result
branch
from
fe54ad8 to
7cf61ac
Compare
![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.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 2
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/advanced_filter_controller.dart (1)
223-228:⚠️ Potential issue | 🟡 MinorVirtual folders can serialize through
mailboxIdSelected, but restoration is incomplete.The picker correctly displays virtual folder selections via ID comparison in the view (lines 516–520, 539–543 of destination_picker_view.dart), but it does not fully restore their selection state in the tree structure. When
generateMailboxTreeInUIruns with a virtual folder ID, the ID won't match any mailbox inallMailboxes, so the deactivation flag (line 69 of mailbox_tree_builder.dart) never applies. Additionally,selectMailboxActionskips callingselectMailboxNode()for virtual folders likeunifiedMailboxandallEmailTrashAndSpamFolder(lines 335–337 of destination_picker_controller.dart), leaving their tree node selection state unset.This works for UI display but leaves the picker in an inconsistent state: the view shows the virtual folder as selected, yet the internal tree nodes and controller state may not fully reflect this. Consider passing the full
PresentationMailboxobject (or a discriminator) instead of only theidto ensure virtual folders round-trip correctly through the picker's internal state management.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/features/mailbox_dashboard/presentation/controller/advanced_filter_controller.dart` around lines 223 - 228, The picker only serializes mailboxId via DestinationPickerArguments which breaks restoration for virtual folders; update DestinationPickerArguments to carry the full PresentationMailbox (or at least a type discriminator + id), then modify generateMailboxTreeInUI to accept and detect PresentationMailbox objects (falling back to id comparison against allMailboxes) so the deactivation flag in mailbox_tree_builder.dart can apply for virtual folders, and update selectMailboxAction in destination_picker_controller.dart to call selectMailboxNode() when the passed value is a virtual PresentationMailbox (including known cases like unifiedMailbox and allEmailTrashAndSpamFolder) so tree node selection state is set consistently.
🤖 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/destination_picker/presentation/destination_picker_view.dart`:
- Around line 516-533: The visual-selected boolean is derived from
mailboxIdSelected fallback and can be out of sync with the real source of truth
controller.mailboxDestination; update the logic in the
DestinationPickerFolderItem blocks (both occurrences around the shown snippet
and the 539-555 area) so you either (a) synchronize the controller state before
rendering by setting controller.mailboxDestination to
PresentationMailbox.unifiedMailbox when mailboxIdSelected is
null/unifiedMailbox, or (b) keep the painted state but change the tap guard to
check controller.mailboxDestination?.isAllEmail (and/or mailboxDestination ==
PresentationMailbox.unifiedMailbox) instead of the visual isSelected; ensure
taps trigger controller.selectMailboxAction(PresentationMailbox.unifiedMailbox)
and controller.dispatchSelectMailboxDestination(context) when
controller.mailboxDestination is not already the unified mailbox.
In
`@lib/features/mailbox_dashboard/presentation/controller/mailbox_dashboard_controller.dart`:
- Around line 969-974: The current getter trashSpamMailboxIds only uses
mapDefaultMailboxIdByRole (and spamMailboxId), which misses team system folders
that have no role; instead iterate the full real-mailbox collection used in this
controller (the same list used elsewhere, e.g., realMailboxes/mailboxes) and
build the exclusion set by adding any mailbox whose role equals
PresentationMailbox.roleTrash or PresentationMailbox.roleSpam and also by
matching known system folder names (e.g., "Trash", "Spam") for team mailboxes
that lack a role; keep spamMailboxId logic if present but remove reliance on
mapDefaultMailboxIdByRole so the set includes all trash/spam mailboxes
discovered in the complete mailbox collection.
---
Outside diff comments:
In
`@lib/features/mailbox_dashboard/presentation/controller/advanced_filter_controller.dart`:
- Around line 223-228: The picker only serializes mailboxId via
DestinationPickerArguments which breaks restoration for virtual folders; update
DestinationPickerArguments to carry the full PresentationMailbox (or at least a
type discriminator + id), then modify generateMailboxTreeInUI to accept and
detect PresentationMailbox objects (falling back to id comparison against
allMailboxes) so the deactivation flag in mailbox_tree_builder.dart can apply
for virtual folders, and update selectMailboxAction in
destination_picker_controller.dart to call selectMailboxNode() when the passed
value is a virtual PresentationMailbox (including known cases like
unifiedMailbox and allEmailTrashAndSpamFolder) so tree node selection state is
set consistently.
🪄 Autofix (Beta)
✅ Autofix completed
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4364d3f2-ed08-47cc-90cb-10ab6a79ac5d
⛔ Files ignored due to path filters (1)
assets/images/ic_all_email.svgis excluded by!**/*.svg
📒 Files selected for processing (16)
core/lib/presentation/resources/image_paths.dartlib/features/base/model/filter_filter.dartlib/features/destination_picker/presentation/destination_picker_controller.dartlib/features/destination_picker/presentation/destination_picker_view.dartlib/features/destination_picker/presentation/widgets/destination_picker_folder_item.dartlib/features/mailbox/presentation/extensions/presentation_mailbox_extension.dartlib/features/mailbox_dashboard/presentation/controller/advanced_filter_controller.dartlib/features/mailbox_dashboard/presentation/controller/mailbox_dashboard_controller.dartlib/features/mailbox_dashboard/presentation/model/search/quick_search_filter.dartlib/features/mailbox_dashboard/presentation/model/search/search_email_filter.dartlib/features/mailbox_dashboard/presentation/widgets/advanced_search/advanced_search_input_form.dartlib/features/search/email/presentation/search_email_controller.dartlib/features/thread/presentation/thread_controller.dartlib/l10n/intl_messages.arblib/main/localizations/app_localizations.dartmodel/lib/mailbox/presentation_mailbox.dart
✅ Files skipped from review due to trivial changes (1)
- lib/features/base/model/filter_filter.dart
🚧 Files skipped from review as they are similar to previous changes (8)
- core/lib/presentation/resources/image_paths.dart
- lib/features/mailbox_dashboard/presentation/model/search/quick_search_filter.dart
- lib/features/mailbox_dashboard/presentation/widgets/advanced_search/advanced_search_input_form.dart
- lib/features/thread/presentation/thread_controller.dart
- lib/main/localizations/app_localizations.dart
- model/lib/mailbox/presentation_mailbox.dart
- lib/l10n/intl_messages.arb
- lib/features/mailbox_dashboard/presentation/model/search/search_email_filter.dart
lib/features/destination_picker/presentation/destination_picker_view.dart
Show resolved
Hide resolved
lib/features/mailbox_dashboard/presentation/controller/mailbox_dashboard_controller.dart
Show resolved
Hide resolved
|
Note Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it. Fixes Applied SuccessfullyFixed 2 file(s) based on 2 unresolved review comments. Files modified:
Commit: The changes have been pushed to the Time taken: |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
dab246
force-pushed
the
features/tf-4358-remove-mail-in-trash-by-default-in-search-result
branch
from
f4bc7df to
e5e7374
Compare
There was a problem hiding this comment.
(1 files improve in Code Health)
Gates Passed
See analysis details in CodeScene
View Improvements
| File | Code Health Impact | Categories Improved |
|---|---|---|
| advanced_search_input_form.dart | 9.16 → 9.16 | Large Method |
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.
Per-file changes (branch vs
|
| File | Insertions | Deletions | Total |
|---|---|---|---|
ic_all_email.svg |
+5 | -0 | 5 |
image_paths.dart |
+1 | -0 | 1 |
filter_filter.dart |
+1 | -1 | 2 |
destination_picker_controller.dart |
+2 | -1 | 3 |
destination_picker_view.dart |
+67 | -80 | 147 |
destination_picker_folder_item.dart |
+102 | -0 | 102 |
presentation_mailbox_extension.dart |
+16 | -0 | 16 |
advanced_filter_controller.dart |
+8 | -24 | 32 |
mailbox_dashboard_controller.dart |
+8 | -6 | 14 |
quick_search_filter.dart |
+1 | -1 | 2 |
search_email_filter.dart |
+28 | -5 | 33 |
advanced_search_input_form.dart |
+2 | -2 | 4 |
search_email_controller.dart |
+13 | -9 | 22 |
thread_controller.dart |
+5 | -2 | 7 |
intl_messages.arb |
+12 | -0 | 12 |
app_localizations.dart |
+14 | -0 | 14 |
presentation_mailbox.dart |
+2 | -0 | 2 |
Summary
| Metric | Value |
|---|---|
| Files changed | 17 |
| Insertions | +287 |
| Deletions | -131 |
| Total | 418 |
|
Hello @dab246 Thanks for pushing this forward, and thanks for making this PR lighter. Can you describe the refactorings that allowed to make it lighter? Maybe other flutter team members have suggestions to further refactor things? |
|
3 participants
Issue
#4358
Resolved
Screen.Recording.2026-03-04.at.18.08.25.mov
Summary by CodeRabbit
New Features
Improvements