TF-4644 SSOT Part 3.5 — Apply Riverpod SSOT to Advanced Search#4694
TF-4644 SSOT Part 3.5 — Apply Riverpod SSOT to Advanced Search#4694dab246 wants to merge 4 commits into
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ 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
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/widgets/advanced_search/advanced_search_filter_form_bottom_view.dart (1)
73-95: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winMove this
FocusNodeout ofbuild(). Creating it inline allocates a new node on every rebuild and never disposes the old one.KeyboardListeneralso only sees Tab when its node has focus, so keep it long-lived and give it focus when this handler should run.🤖 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/widgets/advanced_search/advanced_search_filter_form_bottom_view.dart` around lines 73 - 95, Move the inline FocusNode used by KeyboardListener in advanced_search_filter_form_bottom_view out of build() and make it a long-lived member so it is not recreated on every rebuild or left undisposed. Update the widget state/class that contains the search button and _onClickSearchButton flow to own and dispose this node properly, then use that stored node in KeyboardListener. If Tab handling should run here, request focus on that node when needed so KeyboardListener can actually receive the key events.
🧹 Nitpick comments (2)
test/features/search/email/domain/notifier/search_filter_notifier_test.dart (1)
82-98: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd a test case for
setHasAttachment(false.asSearchFilterToggle()).
setHasAttachmentusesexactOption(returnsSome(false)for unselected), whilesetUnreadandsetNotIncludeEventsuseenabledOnlyOption(returnsNone()). This is a distinct code path —Some(false)vsNone()— but only thetruecase is tested forsetHasAttachment. A regression wheresetHasAttachment(false)accidentally usesenabledOnlyOptionwould go undetected.🧪 Suggested test addition
notifierOf().setHasAttachment(true.asSearchFilterToggle()); notifierOf().setUnread(true.asSearchFilterToggle()); notifierOf().setNotIncludeEvents(true.asSearchFilterToggle()); notifierOf().setSortOrder(EmailSortOrderType.oldest); expect(stateOf().hasAttachment, isTrue); expect(stateOf().unread, isTrue); expect(stateOf().notIncludeEvents, isTrue); expect(stateOf().sortOrderType, EmailSortOrderType.oldest); + notifierOf().setHasAttachment(false.asSearchFilterToggle()); notifierOf().setUnread(false.asSearchFilterToggle()); notifierOf().setNotIncludeEvents(false.asSearchFilterToggle()); + expect(stateOf().hasAttachment, isFalse); expect(stateOf().unread, isFalse); expect(stateOf().notIncludeEvents, isFalse);🤖 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/search/email/domain/notifier/search_filter_notifier_test.dart` around lines 82 - 98, Add a test case in search_filter_notifier_test.dart covering setHasAttachment(false.asSearchFilterToggle()) alongside the existing boolean and sort helpers test. The current test in search_filter_notifier_test only verifies the true path for setHasAttachment, so extend the notifierOf()/stateOf() assertions to confirm that calling setHasAttachment with false updates hasAttachment to false and exercises the exactOption path used by setHasAttachment. Keep the existing checks for setUnread and setNotIncludeEvents unchanged.test/features/mailbox_dashboard/presentation/controller/advanced_filter_controller_test.dart (1)
536-586: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPrefer stable keys over index-based checkbox lookups. The starred, unread, and notIncludeEvents tests in
test/features/mailbox_dashboard/presentation/controller/advanced_filter_controller_test.dartusefind.byType(CustomIconLabeledCheckbox).at(index), while the has-attachment checkbox already uses aValueKey. Add keys to the remaining checkboxes inlib/features/mailbox_dashboard/presentation/widgets/advanced_search/advanced_search_filter_form_bottom_view.dartand switch the tests tofind.byKeyso reordering or inserting a checkbox does not target the wrong field.🤖 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_dashboard/presentation/controller/advanced_filter_controller_test.dart` around lines 536 - 586, The starred, unread, and notIncludeEvents tests are using index-based checkbox lookup, which is fragile if the checkbox order changes. Add stable ValueKey identifiers to the corresponding checkbox widgets in advanced_search_filter_form_bottom_view and update the related tests to use find.byKey instead of find.byType(CustomIconLabeledCheckbox).at(...). Keep the existing has-attachment key pattern as the reference for locating the correct checkbox widgets.
🤖 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.
Inline comments:
In `@lib/features/search/email/domain/model/search_filter_input.dart`:
- Around line 40-44: The commaSeparatedSetOption getter currently trims and
deduplicates comma-separated values but still allows empty strings into the Set,
so update the mapping chain to filter out empty results before building the set.
Use the commaSeparatedSetOption symbol in SearchFilterInput and add a
where/exclude-empty step after splitting and trimming so inputs like
"apple,,banana" or "apple, ," do not produce a "" entry.
---
Outside diff comments:
In
`@lib/features/mailbox_dashboard/presentation/widgets/advanced_search/advanced_search_filter_form_bottom_view.dart`:
- Around line 73-95: Move the inline FocusNode used by KeyboardListener in
advanced_search_filter_form_bottom_view out of build() and make it a long-lived
member so it is not recreated on every rebuild or left undisposed. Update the
widget state/class that contains the search button and _onClickSearchButton flow
to own and dispose this node properly, then use that stored node in
KeyboardListener. If Tab handling should run here, request focus on that node
when needed so KeyboardListener can actually receive the key events.
---
Nitpick comments:
In
`@test/features/mailbox_dashboard/presentation/controller/advanced_filter_controller_test.dart`:
- Around line 536-586: The starred, unread, and notIncludeEvents tests are using
index-based checkbox lookup, which is fragile if the checkbox order changes. Add
stable ValueKey identifiers to the corresponding checkbox widgets in
advanced_search_filter_form_bottom_view and update the related tests to use
find.byKey instead of find.byType(CustomIconLabeledCheckbox).at(...). Keep the
existing has-attachment key pattern as the reference for locating the correct
checkbox widgets.
In `@test/features/search/email/domain/notifier/search_filter_notifier_test.dart`:
- Around line 82-98: Add a test case in search_filter_notifier_test.dart
covering setHasAttachment(false.asSearchFilterToggle()) alongside the existing
boolean and sort helpers test. The current test in search_filter_notifier_test
only verifies the true path for setHasAttachment, so extend the
notifierOf()/stateOf() assertions to confirm that calling setHasAttachment with
false updates hasAttachment to false and exercises the exactOption path used by
setHasAttachment. Keep the existing checks for setUnread and setNotIncludeEvents
unchanged.
🪄 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: 3f8c08ba-3575-406d-b3bb-444e6a783134
📒 Files selected for processing (12)
lib/features/mailbox_dashboard/presentation/controller/advanced_filter_controller.dartlib/features/mailbox_dashboard/presentation/controller/search_controller.dartlib/features/mailbox_dashboard/presentation/extensions/advanced_search/update_label_in_advanced_search_extension.dartlib/features/mailbox_dashboard/presentation/widgets/advanced_search/advanced_search_filter_form_bottom_view.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/domain/model/search_filter_input.dartlib/features/search/email/domain/notifier/search_filter_mutation.dartlib/features/search/email/domain/notifier/search_filter_notifier.darttest/features/mailbox_dashboard/presentation/controller/advanced_filter_controller_test.darttest/features/mailbox_dashboard/presentation/controller/mailbox_dashboard_controller_test.darttest/features/search/email/domain/notifier/search_filter_notifier_test.dart
💤 Files with no reviewable changes (1)
- lib/features/mailbox_dashboard/presentation/extensions/advanced_search/update_label_in_advanced_search_extension.dart
|
This PR has been deployed to https://linagora.github.io/tmail-flutter/4694. |
| @@ -172,19 +196,28 @@ void main() { | |||
| group('applyAdvancedSearchFilter::test', () { | |||
| test('SHOULD make sure memory search filter and search filter should be the same after applying', () async { | |||
There was a problem hiding this comment.
we should update the name of test more precise
There was a problem hiding this comment.
idem, now we dont have MemorySearchFilter
| import 'package:tmail_ui_user/features/search/email/domain/notifier/search_filter_notifier.dart'; | ||
| import 'package:tmail_ui_user/features/thread/domain/model/search_query.dart'; | ||
|
|
||
| void main() { |
There was a problem hiding this comment.
please add test for
- setMailbox
- toggleLabel
- setHasAttachment for the false branch
|
Signed-off-by: dab246 <tdvu@linagora.com>
…etHasAttachment false branch Signed-off-by: dab246 <tdvu@linagora.com>
There was a problem hiding this comment.
(1 files improve in Code Health)
Our agent can fix these. Install it.
Gates Passed
View Improvements
| File | Code Health Impact | Categories Improved |
|---|---|---|
| advanced_search_input_form.dart | 9.16 → 10.00 | 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.
added |
|
Docker image published for this PR: linagora/tmail-web-pr:4694 |
4 participants
Closes #4644
Summary
Move Advanced Search filter state to the committed
searchFilterProviderso the form, result chips, quick-search chips, and search execution read from the same source of truth.Changes
Consumerreads andSearchFilterNotifiermutations.searchFilterProviderinstead of maintaining duplicate mutable lists.Summary by CodeRabbit
New Features
Bug Fixes