Implement not include events filter for search#4451
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:
WalkthroughReplaces the previous keyword-driven “events” filter with an explicit boolean 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 |
![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.
Actionable comments posted: 1
🧹 Nitpick comments (1)
lib/features/mailbox_dashboard/presentation/model/search/search_email_filter.dart (1)
161-164: Consider guarding explicit Events inclusion beyondhasKeyword.Right now, only
hasKeyworddisables the defaultnotKeyword(events)clause. If Events can be selected via another path (e.g.,label.keyword), this can create contradictory filters. Consider centralizing anisEventsExplicitlyIncludedcheck.♻️ Suggested hardening
+ final isEventsExplicitlyIncluded = + hasKeyword.contains(KeyWordIdentifierExtension.eventsMail.value) || + label?.keyword?.value == KeyWordIdentifierExtension.eventsMail.value; + final listEmailCondition = { if (emailEmailFilterConditionShared.hasCondition) emailEmailFilterConditionShared, @@ - if (!hasKeyword.contains(KeyWordIdentifierExtension.eventsMail.value)) + if (!isEventsExplicitlyIncluded) EmailFilterCondition( notKeyword: KeyWordIdentifierExtension.eventsMail.value, ),🤖 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 161 - 164, The code currently only checks hasKeyword before adding an EmailFilterCondition(notKeyword: KeyWordIdentifierExtension.eventsMail.value), which can conflict if Events are included via another path (e.g., label.keyword); add a centralized boolean (e.g., isEventsExplicitlyIncluded) that returns true when hasKeyword OR any other explicit include (like label?.keyword == KeyWordIdentifierExtension.eventsMail.value or other selection flags) and then replace the hasKeyword check with !isEventsExplicitlyIncluded before emitting EmailFilterCondition(notKeyword: KeyWordIdentifierExtension.eventsMail.value); update the logic where EmailFilterCondition is created so all places consult isEventsExplicitlyIncluded to avoid contradictory filters.
🤖 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/mailbox_dashboard/presentation/model/search/search_email_filter.dart`:
- Around line 161-164: Update the tests to reflect that
mappingToEmailFilterCondition() now returns a non-null EmailFilterCondition for
the initial filter because Events (KeyWordIdentifierExtension.eventsMail) is
excluded by default; specifically modify the assertions in
test/features/search/search_email_filter_test.dart starting around line 81 to
expect a non-null EmailFilterCondition with notKeyword ==
KeyWordIdentifierExtension.eventsMail.value, and add a new test case that
verifies an explicit “include Events” scenario (i.e., when hasKeyword includes
KeyWordIdentifierExtension.eventsMail.value the mapping returns null or an
EmailFilterCondition without notKeyword as appropriate).
---
Nitpick comments:
In
`@lib/features/mailbox_dashboard/presentation/model/search/search_email_filter.dart`:
- Around line 161-164: The code currently only checks hasKeyword before adding
an EmailFilterCondition(notKeyword:
KeyWordIdentifierExtension.eventsMail.value), which can conflict if Events are
included via another path (e.g., label.keyword); add a centralized boolean
(e.g., isEventsExplicitlyIncluded) that returns true when hasKeyword OR any
other explicit include (like label?.keyword ==
KeyWordIdentifierExtension.eventsMail.value or other selection flags) and then
replace the hasKeyword check with !isEventsExplicitlyIncluded before emitting
EmailFilterCondition(notKeyword: KeyWordIdentifierExtension.eventsMail.value);
update the logic where EmailFilterCondition is created so all places consult
isEventsExplicitlyIncluded to avoid contradictory filters.
🪄 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: e61aa082-96e8-4da3-a030-22c8a4bfe505
📒 Files selected for processing (1)
lib/features/mailbox_dashboard/presentation/model/search/search_email_filter.dart
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: 1
🧹 Nitpick comments (1)
test/features/search/search_email_filter_test.dart (1)
121-135: Assert the events exclusion explicitly here.Line 135 only checks the condition count, so this can still pass if the third condition is unrelated. Since this hotfix is specifically about the default events exclusion, assert that one
EmailFilterConditionhasnotKeyword == KeyWordIdentifierExtension.eventsMail.value, not just that there are three entries.Suggested test hardening
expect(result, isA<LogicFilterOperator>()); final logicOperator = result as LogicFilterOperator; expect(logicOperator.operator, Operator.AND); expect(logicOperator.conditions.length, equals(3)); + expect( + logicOperator.conditions.whereType<EmailFilterCondition>().any( + (condition) => + condition.notKeyword == + KeyWordIdentifierExtension.eventsMail.value, + ), + isTrue, + );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/features/search/search_email_filter_test.dart` around lines 121 - 135, The test for SearchEmailFilter.mappingToEmailFilterCondition currently only checks there are three conditions but doesn't verify the default events exclusion; update the test to explicitly assert that among the LogicFilterOperator.conditions (from mappingToEmailFilterCondition) there exists an EmailFilterCondition whose notKeyword equals KeyWordIdentifierExtension.eventsMail.value, while keeping the existing checks for operator AND and total length; refer to SearchEmailFilter.mappingToEmailFilterCondition, LogicFilterOperator, EmailFilterCondition, and the notKeyword/KeyWordIdentifierExtension.eventsMail.value identifiers to locate and add the assertion.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/features/search/search_email_filter_test.dart`:
- Around line 246-260: Add two additional test cases around
SearchEmailFilter.mappingToEmailFilterCondition to catch membership bugs: (1)
when hasKeyword contains a non-events keyword (ensure
EmailFilterCondition.notKeyword is the events exclusion and hasKeyword reflects
only the provided non-events value), and (2) when hasKeyword contains eventsMail
plus another keyword (ensure eventsMail presence prevents setting notKeyword
while hasKeyword includes both values). Use
KeyWordIdentifierExtension.eventsMail.value and other keyword identifiers, and
assert EmailFilterCondition.hasKeyword and EmailFilterCondition.notKeyword
accordingly to validate membership rather than exact-set equality.
---
Nitpick comments:
In `@test/features/search/search_email_filter_test.dart`:
- Around line 121-135: The test for
SearchEmailFilter.mappingToEmailFilterCondition currently only checks there are
three conditions but doesn't verify the default events exclusion; update the
test to explicitly assert that among the LogicFilterOperator.conditions (from
mappingToEmailFilterCondition) there exists an EmailFilterCondition whose
notKeyword equals KeyWordIdentifierExtension.eventsMail.value, while keeping the
existing checks for operator AND and total length; refer to
SearchEmailFilter.mappingToEmailFilterCondition, LogicFilterOperator,
EmailFilterCondition, and the
notKeyword/KeyWordIdentifierExtension.eventsMail.value identifiers to locate and
add the assertion.
🪄 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: cf4925bc-fa87-4cb3-8559-09ecbed4620e
📒 Files selected for processing (1)
test/features/search/search_email_filter_test.dart
|
This PR has been deployed to https://linagora.github.io/tmail-flutter/4451. |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
Well I STRONGLY disagree with that change. I do believe that pleople are actually interested in email about event and might actually actively look for them. I thus challenge this functionnal requirement that I believe is misplaced. What are other email providers doing? Is GMail for instance actively filtering out events from my email search ? Cc @poupotte |
Signed-off-by: dab246 <tdvu@linagora.com>
dab246
force-pushed
the
hotfix/filter-event-in-mail-search
branch
from
9cd5bec to
8cd75a8
Compare
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
🧹 Nitpick comments (2)
lib/features/mailbox_dashboard/presentation/extensions/select_search_filter_action_extension.dart (1)
40-42: Consider renaming the stale delete helper.Line 40 now clears
notIncludeEventsOption, sodeleteNotIncludeEventsSearchFilter()would better match the behavior thandeleteEventsSearchFilter().Also applies to: 55-56
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/features/mailbox_dashboard/presentation/extensions/select_search_filter_action_extension.dart` around lines 40 - 42, The helper deleteEventsSearchFilter() is misnamed because it clears the notIncludeEventsOption; rename the function to deleteNotIncludeEventsSearchFilter and update all call sites and references accordingly; inside the function keep the call to searchController.updateFilterEmail(notIncludeEventsOption: const None()) unchanged and also rename the analogous helper at the other occurrence (lines 55-56) to the matching deleteNotIncludeEvents... name so names reflect behaviour.test/features/search/search_email_filter_test.dart (1)
25-29: Match the events exclusion explicitly in the helper.
firstWhere((c) => c.notKeyword != null)can pick anothernotKeywordcondition, such as unread’semailSeen, if this helper is reused on a broader AND filter. MatcheventsMaildirectly to avoid false failures.Suggested test helper hardening
void _assertEventsExclusionInAnd(LogicFilterOperator andFilter) { final eventsExclusion = andFilter.conditions .whereType<EmailFilterCondition>() - .firstWhere((c) => c.notKeyword != null); + .firstWhere( + (c) => c.notKeyword == KeyWordIdentifierExtension.eventsMail.value, + ); expect(eventsExclusion.notKeyword, KeyWordIdentifierExtension.eventsMail.value); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/features/search/search_email_filter_test.dart` around lines 25 - 29, The helper _assertEventsExclusionInAnd currently finds any EmailFilterCondition with a non-null notKeyword which can pick the wrong condition; update the predicate so it explicitly looks for the events exclusion by matching c.notKeyword == KeyWordIdentifierExtension.eventsMail.value (i.e., use firstWhere((c) => c.notKeyword == KeyWordIdentifierExtension.eventsMail.value)) to ensure the test asserts the eventsMail exclusion rather than any notKeyword.
🤖 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/search/email/presentation/extension/update_search_filter_extension.dart`:
- Around line 1-11: The current DeleteQuickSearchFilter implementation in the
UpdateSearchFilterExtension only handles QuickSearchFilter.labels, causing
deletions of QuickSearchFilter.starred, QuickSearchFilter.unread, and
QuickSearchFilter.notIncludeEvents to be ignored; update deleteQuickSearchFilter
to switch on the incoming QuickSearchFilter and for each case clear the
corresponding filter state (e.g., call updateSimpleSearchFilter with the
appropriate parameters for labels, remove/clear the starred flag, clear the
unread flag, and clear the not-include-events flag), then call
searchEmailAction() after clearing to re-run the search; reference the extension
UpdateSearchFilterExtension, the method deleteQuickSearchFilter, the
QuickSearchFilter enum values, updateSimpleSearchFilter, and searchEmailAction
when making the changes.
In `@test/features/search/verify_before_time_in_search_email_filter_test.dart`:
- Around line 178-188: _extractBeforeFromFilter currently calls firstWhere on
LogicFilterOperator.conditions which throws if no EmailFilterCondition has
before; change it to defensively search recursively: iterate over
filter.conditions in _extractBeforeFromFilter, for each condition return
condition.before if it's an EmailFilterCondition with before set, and if it's a
LogicFilterOperator call _extractBeforeFromFilter recursively and return the
non-null result; if none found return null. This avoids StateError and safely
handles nested LogicFilterOperator trees when before is absent.
---
Nitpick comments:
In
`@lib/features/mailbox_dashboard/presentation/extensions/select_search_filter_action_extension.dart`:
- Around line 40-42: The helper deleteEventsSearchFilter() is misnamed because
it clears the notIncludeEventsOption; rename the function to
deleteNotIncludeEventsSearchFilter and update all call sites and references
accordingly; inside the function keep the call to
searchController.updateFilterEmail(notIncludeEventsOption: const None())
unchanged and also rename the analogous helper at the other occurrence (lines
55-56) to the matching deleteNotIncludeEvents... name so names reflect
behaviour.
In `@test/features/search/search_email_filter_test.dart`:
- Around line 25-29: The helper _assertEventsExclusionInAnd currently finds any
EmailFilterCondition with a non-null notKeyword which can pick the wrong
condition; update the predicate so it explicitly looks for the events exclusion
by matching c.notKeyword == KeyWordIdentifierExtension.eventsMail.value (i.e.,
use firstWhere((c) => c.notKeyword ==
KeyWordIdentifierExtension.eventsMail.value)) to ensure the test asserts the
eventsMail exclusion rather than any notKeyword.
🪄 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: 972c36c9-d5bc-4539-8b47-a42dbdec5b93
📒 Files selected for processing (15)
lib/features/mailbox_dashboard/presentation/controller/advanced_filter_controller.dartlib/features/mailbox_dashboard/presentation/controller/mailbox_dashboard_controller.dartlib/features/mailbox_dashboard/presentation/controller/search_controller.dartlib/features/mailbox_dashboard/presentation/extensions/select_search_filter_action_extension.dartlib/features/mailbox_dashboard/presentation/mailbox_dashboard_view_web.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_filter_form_bottom_view.dartlib/features/search/email/presentation/extension/update_search_filter_extension.dartlib/features/search/email/presentation/search_email_controller.dartlib/features/search/email/presentation/search_email_view.dartlib/l10n/intl_messages.arblib/main/localizations/app_localizations.darttest/features/search/search_email_filter_test.darttest/features/search/verify_before_time_in_search_email_filter_test.dart
✅ Files skipped from review due to trivial changes (1)
- lib/features/mailbox_dashboard/presentation/controller/search_controller.dart
🚧 Files skipped from review as they are similar to previous changes (1)
- lib/features/mailbox_dashboard/presentation/model/search/search_email_filter.dart
Signed-off-by: dab246 <tdvu@linagora.com>
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
dab246
changed the title
not include events filter for search
| case QuickSearchFilter.unread: | ||
| case QuickSearchFilter.labels: | ||
| case QuickSearchFilter.events: | ||
| case QuickSearchFilter.notIncludeEvents: |
There was a problem hiding this comment.
Change spotted in mailbox_dashboard_controller.
Please elaborate on "why this is needed" and "what we can do to avoid this change"
There was a problem hiding this comment.
Why this is needed
This is an optional change; we modify it to clarify the function of each variable and avoid confusion during maintenance later, if another type of filter related to the event is needed.
What we can do to avoid this change
As explained above, we can keep it as is, but we should add a comment to clarify the function of this filter.
There was a problem hiding this comment.
Keep QuickSearchFilter.events
There was a problem hiding this comment.
We have searchController is the single source of truth for search state.
But in some other UI, which was controlled by other controller - mailbox_dashboard_controller in this case was updated to update the searchController. This is the reason why some controller were changed also.
There was a problem hiding this comment.
Reviewed.
No strong opposition on my side. We are touching with a simple code change 5+ controler and views, which lead me to think we have a view centered model, while there could be room for higher level abstractions...
Can we have a video of what is delivered, please?
Signed-off-by: dab246 <tdvu@linagora.com>
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Updated Screen.Recording.2026-04-20.at.16.51.10.mov |
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 |
|---|---|---|
| update_search_filter_extension.dart | 9.69 → 10.00 | Complex 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.
4 participants
Demo
Screen.Recording.2026-04-20.at.16.51.10.mov
Summary by CodeRabbit
New Features
UI
Bug Fixes
Tests