TF-4400 Enable collapse threads on Force Query path#4422
Open
TF-4400 Enable collapse threads on Force Query path#4422
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 (3)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (2)
WalkthroughAdds end-to-end thread-collapsing support: 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 |
dab246
force-pushed
the
features/tf-4400-enable-collapse-threads-on-force-query-path
branch
from
7374fea to
005d6ed
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: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
lib/features/thread/data/repository/thread_repository_impl.dart (2)
250-267:⚠️ Potential issue | 🟠 Major
_getFirstPage()drops the collapsed projection.This helper forwards
collapseThreads, but it still updates the cache withnetworkEmailResponse.emailListand returns the raw response. WhenrefreshChanges()falls back here, the first page will re-expand even though the query asked for collapsed threads. Normalize the response here the same way asforceQueryAllEmailsForWeb().Suggested fix
) async { final networkEmailResponse = await mapDataSource[DataSourceType.network]!.getAllEmail( session, accountId, limit: limit ?? ThreadConstants.defaultLimit, position: position, sort: sort, filter: filter ?? EmailFilterCondition(inMailbox: mailboxId), properties: propertiesCreated, collapseThreads: collapseThreads, ); + final emailListLoaded = collapseThreads == true + ? networkEmailResponse.threadEmails + : networkEmailResponse.emailList; await _updateEmailCache( accountId, session.username, - newCreated: networkEmailResponse.emailList, + newCreated: emailListLoaded, newDestroyed: networkEmailResponse.notFoundEmailIds, ); - return networkEmailResponse; + return networkEmailResponse.copyWith(emailList: emailListLoaded); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/features/thread/data/repository/thread_repository_impl.dart` around lines 250 - 267, _getFirstPage forwards collapseThreads but updates cache and returns the raw networkEmailResponse.emailList, causing collapsed queries to be re-expanded; modify _getFirstPage so that when collapseThreads is true it normalizes the network response the same way forceQueryAllEmailsForWeb() does (produce a collapsed projection/list and corresponding notFound ids), call _updateEmailCache with the normalized newCreated/newDestroyed values (instead of raw networkEmailResponse.emailList), and return the normalized response so refreshChanges() receives the collapsed view.
204-225:⚠️ Potential issue | 🟠 MajorCollapsed refresh still reuses the email-level change sync.
forceQueryAllEmailsForWeb()now cachesthreadEmailswhencollapseThreadsis enabled, butrefreshChanges()still runs_synchronizeCacheWithChanges()and then re-reads that cache without any thread reprojection step. That can leave stale thread counts or leak standalone emails back into a collapsed list after incremental changes. Consider short-circuiting to a freshEmail/querywhenevercollapseThreads == true, or making the change-sync path thread-aware.Also applies to: 376-432
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/features/thread/data/repository/thread_repository_impl.dart` around lines 204 - 225, forceQueryAllEmailsForWeb() caches serverResponse.threadEmails when collapseThreads==true but refreshChanges() still runs the email-level _synchronizeCacheWithChanges() and then re-reads that cache, causing stale thread counts or standalone emails to reappear; update refreshChanges() (and any incremental change path that calls _synchronizeCacheWithChanges()) to short-circuit to a fresh Email/query when collapseThreads==true (i.e., call forceQueryAllEmailsForWeb() or trigger a full Email/query reload) or alternatively make the change-sync path thread-aware by performing thread reprojection after _synchronizeCacheWithChanges() so _updateEmailCache() receives thread-scoped newCreated/newDestroyed (use the collapseThreads flag and threadEmails from forceQueryAllEmailsForWeb() to guide which path to take).
🧹 Nitpick comments (2)
lib/features/thread/data/extensions/reference_path_extension.dart (1)
3-5: Make the static constant immutable.The
listThreadIdsPathshould befinal(orconstifReferencePathsupports const construction) to prevent accidental reassignment.♻️ Proposed fix
extension ReferencePathExtension on ReferencePath { - static ReferencePath listThreadIdsPath = ReferencePath('/list/*/threadId'); + static final ReferencePath listThreadIdsPath = ReferencePath('/list/*/threadId'); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/features/thread/data/extensions/reference_path_extension.dart` around lines 3 - 5, Change the mutable static field listThreadIdsPath on the ReferencePathExtension to an immutable static final (or static const if ReferencePath has a const constructor) so it cannot be reassigned; update the declaration of listThreadIdsPath accordingly and, if using const, ensure ReferencePath('/list/*/threadId') is constructed with a const constructor.lib/features/manage_account/domain/model/preferences/preferences_setting.dart (1)
34-35: Simplify the boolean comparison.Since
ThreadDetailConfig.isEnabledis declared as a non-nullablebool(perthread_detail_config.dart), the== truecomparison is redundant.♻️ Proposed simplification
- bool get isThreadDetailEnabled => threadConfig.isEnabled == true; + bool get isThreadDetailEnabled => threadConfig.isEnabled;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/features/manage_account/domain/model/preferences/preferences_setting.dart` around lines 34 - 35, The getter isThreadDetailEnabled redundantly compares a non-nullable bool to true; update the getter in PreferencesSetting (the isThreadDetailEnabled accessor that reads threadConfig.isEnabled) to return threadConfig.isEnabled directly, removing the "== true" comparison to simplify the expression.
🤖 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/mixin/mail_api_mixin.dart`:
- Around line 192-198: The log force-unwraps notFoundEmailIds which can be null
when responseOfGetEmailMethod?.notFound is absent; update the fetchAllEmail
logging to avoid the null-assertion by safely handling
responseOfGetEmailMethod?.notFound (e.g., compute notFoundEmailIds as an empty
list or use a null-safe expression) and then use that safe value in the log call
(referencing notFoundEmailIds, responseOfGetEmailMethod, and
fetchAllEmail/runtimeType to locate the code).
- Around line 130-169: Derive a single boolean (e.g. shouldCollapseThreads) by
AND-ing the local collapseThreads flag with the session-level collapse-threads
support (use the Session object or capabilities API your code exposes, e.g.
session.supportsCollapseThreads or session.capabilities.collapseThreads), then
use that variable everywhere instead of directly checking collapseThreads: use
shouldCollapseThreads when setting properties (the block that sets
Properties({EmailProperty.threadId})), when calling
queryEmailMethod.addCollapseThreadsIfValid(...), and when deciding to
build/invoke GetThreadMethod and its reference ids (the
getThread/getThreadMethodCallId block); remove direct checks of collapseThreads
so the code only collapses when both session and local setting allow it.
In
`@lib/features/mailbox_dashboard/presentation/mixin/setup_preferences_setting_mixin.dart`:
- Around line 24-30: The collapseThreads getter currently returns true based
only on isThreadDetailEnabled and forceEmailQuery; update the mixin to accept a
Session capability flag (e.g., a boolean property named
sessionSupportsCollapsedQuery or similar) and fold that into the check so
collapseThreads returns true only when isThreadDetailEnabled == true,
forceEmailQuery == true, and sessionSupportsCollapsedQuery == true; add the new
session capability input to the mixin's constructor or as a setter so callers
can pass the server Session capability and update the collapseThreads logic
accordingly.
In `@model/lib/email/presentation_email.dart`:
- Line 27: The PresentationEmail class mixes in ThreadMixin but does not include
the ThreadMixin-provided emailIdsInThread in its Equatable props, so add the
emailIdsInThread field to the props getter in PresentationEmail (and the
corresponding props getter later in the file that mirrors this class) so
Equatable compares thread membership; locate the props getter in class
PresentationEmail and append emailIdsInThread to the returned list alongside the
existing properties.
---
Outside diff comments:
In `@lib/features/thread/data/repository/thread_repository_impl.dart`:
- Around line 250-267: _getFirstPage forwards collapseThreads but updates cache
and returns the raw networkEmailResponse.emailList, causing collapsed queries to
be re-expanded; modify _getFirstPage so that when collapseThreads is true it
normalizes the network response the same way forceQueryAllEmailsForWeb() does
(produce a collapsed projection/list and corresponding notFound ids), call
_updateEmailCache with the normalized newCreated/newDestroyed values (instead of
raw networkEmailResponse.emailList), and return the normalized response so
refreshChanges() receives the collapsed view.
- Around line 204-225: forceQueryAllEmailsForWeb() caches
serverResponse.threadEmails when collapseThreads==true but refreshChanges()
still runs the email-level _synchronizeCacheWithChanges() and then re-reads that
cache, causing stale thread counts or standalone emails to reappear; update
refreshChanges() (and any incremental change path that calls
_synchronizeCacheWithChanges()) to short-circuit to a fresh Email/query when
collapseThreads==true (i.e., call forceQueryAllEmailsForWeb() or trigger a full
Email/query reload) or alternatively make the change-sync path thread-aware by
performing thread reprojection after _synchronizeCacheWithChanges() so
_updateEmailCache() receives thread-scoped newCreated/newDestroyed (use the
collapseThreads flag and threadEmails from forceQueryAllEmailsForWeb() to guide
which path to take).
---
Nitpick comments:
In
`@lib/features/manage_account/domain/model/preferences/preferences_setting.dart`:
- Around line 34-35: The getter isThreadDetailEnabled redundantly compares a
non-nullable bool to true; update the getter in PreferencesSetting (the
isThreadDetailEnabled accessor that reads threadConfig.isEnabled) to return
threadConfig.isEnabled directly, removing the "== true" comparison to simplify
the expression.
In `@lib/features/thread/data/extensions/reference_path_extension.dart`:
- Around line 3-5: Change the mutable static field listThreadIdsPath on the
ReferencePathExtension to an immutable static final (or static const if
ReferencePath has a const constructor) so it cannot be reassigned; update the
declaration of listThreadIdsPath accordingly and, if using const, ensure
ReferencePath('/list/*/threadId') is constructed with a const constructor.
🪄 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: fa48c438-b320-40a0-b2f4-dcc3e94b0044
📒 Files selected for processing (33)
lib/features/base/mixin/mail_api_mixin.dartlib/features/email/presentation/action/email_ui_action.dartlib/features/mailbox_dashboard/presentation/controller/mailbox_dashboard_controller.dartlib/features/mailbox_dashboard/presentation/mixin/setup_preferences_setting_mixin.dartlib/features/manage_account/domain/model/preferences/preferences_setting.dartlib/features/manage_account/presentation/bindings/setting_interactor_bindings.dartlib/features/thread/data/datasource/thread_datasource.dartlib/features/thread/data/datasource_impl/local_thread_datasource_impl.dartlib/features/thread/data/datasource_impl/thread_datasource_impl.dartlib/features/thread/data/extensions/get_email_method_extension.dartlib/features/thread/data/extensions/query_email_method_extension.dartlib/features/thread/data/extensions/reference_path_extension.dartlib/features/thread/data/network/thread_api.dartlib/features/thread/data/repository/thread_repository_impl.dartlib/features/thread/domain/extensions/list_thread_extension.dartlib/features/thread/domain/model/email_response.dartlib/features/thread/domain/model/get_email_request.dartlib/features/thread/domain/model/thread_email.dartlib/features/thread/domain/repository/thread_repository.dartlib/features/thread/domain/usecases/get_emails_in_mailbox_interactor.dartlib/features/thread/domain/usecases/load_more_emails_in_mailbox_interactor.dartlib/features/thread/domain/usecases/refresh_changes_emails_in_mailbox_interactor.dartlib/features/thread/presentation/mixin/base_email_item_tile.dartlib/features/thread/presentation/thread_controller.dartlib/features/thread/presentation/thread_view.dartlib/features/thread/presentation/widgets/count_emails_in_thread_widget.dartlib/features/thread/presentation/widgets/email_tile_builder.dartlib/features/thread/presentation/widgets/email_tile_web_builder.dartlib/features/thread/presentation/widgets/web_tablet_body_email_item_widget.dartmodel/lib/email/presentation_email.dartmodel/lib/extensions/email_extension.dartmodel/lib/extensions/presentation_email_extension.dartmodel/lib/mixin/thread_mixin.dart
lib/features/mailbox_dashboard/presentation/mixin/setup_preferences_setting_mixin.dart
Show resolved
Hide resolved
|
This PR has been deployed to https://linagora.github.io/tmail-flutter/4422. |
…hen force email query = true
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 (2)
model/lib/email/presentation_email.dart (1)
231-296:⚠️ Potential issue | 🟠 MajorPreserve
emailIdsInThreadincopyWithto avoid dropping thread state.
copyWithcreates a newPresentationEmailbut does not carryemailIdsInThread. Since this field is now inprops(Line 228), anycopyWith()call can clear thread membership and break thread count/equality behavior.🔧 Proposed fix
PresentationEmail copyWith({ @@ MessageIdsHeaderValue? messageId, MessageIdsHeaderValue? references, + List<EmailId>? emailIdsInThread, }) { - return PresentationEmail( + return PresentationEmail( @@ messageId: messageId ?? this.messageId, references: references ?? this.references, - ); + )..emailIdsInThread = emailIdsInThread ?? this.emailIdsInThread; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@model/lib/email/presentation_email.dart` around lines 231 - 296, The copyWith method on PresentationEmail is missing the emailIdsInThread field, which causes thread membership to be lost; update PresentationEmail.copyWith to accept an optional parameter Map or Set (matching the type of emailIdsInThread) named emailIdsInThread and return it as emailIdsInThread: emailIdsInThread ?? this.emailIdsInThread when constructing the new PresentationEmail so the field is preserved across copies; ensure the new parameter name and the assignment match the existing emailIdsInThread property used in props.lib/features/thread/data/repository/thread_repository_impl.dart (1)
636-658:⚠️ Potential issue | 🟠 Major
loadAllEmailInFolderWithoutCachedoes not transform response whencollapseThreadsis true.Unlike
forceQueryAllEmailsForWeb,_getFirstPage, and_getAllEmailsWithoutLastEmailId, this method yieldsemailResponsedirectly without selectingthreadEmailswhencollapseThreads == true. This inconsistency may cause the caller to receiveemailListinstead ofthreadEmails, breaking the collapsed-thread view.🐛 Proposed fix to align with other methods
}) async* { final networkDataSource = mapDataSource[DataSourceType.network]!; final emailResponse = await networkDataSource.getAllEmail( session, accountId, limit: limit, position: position, sort: sort, filter: emailFilter?.filter, properties: propertiesCreated, collapseThreads: collapseThreads, ); - yield emailResponse; + final emailListLoaded = collapseThreads == true + ? emailResponse.threadEmails + : emailResponse.emailList; + yield emailResponse.copyWith(emailList: emailListLoaded); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/features/thread/data/repository/thread_repository_impl.dart` around lines 636 - 658, The method loadAllEmailInFolderWithoutCache yields the raw emailResponse even when collapseThreads is true, causing callers to get emailList instead of threadEmails; update loadAllEmailInFolderWithoutCache (same pattern used in forceQueryAllEmailsForWeb, _getFirstPage, and _getAllEmailsWithoutLastEmailId) to check collapseThreads and, if true, transform the network response by selecting response.threadEmails (or mapping threadEmails into the EmailsResponse emails field) before yielding so callers always receive the collapsed-thread payload when collapseThreads == true.
🧹 Nitpick comments (1)
lib/features/base/mixin/mail_api_mixin.dart (1)
140-143: Mutating caller-suppliedPropertiesobject may cause unintended side effects.When
collapseThreadsis true andpropertiesis non-null, line 142 mutates the caller'sProperties.valueset by addingthreadId. If the caller reuses this object, it will unexpectedly containthreadIdin subsequent calls.Consider creating a copy before modifying:
♻️ Proposed defensive copy
if (collapseThreads == true) { - properties ??= Properties({EmailProperty.threadId}); - properties.value.add(EmailProperty.threadId); + final updatedProperties = Properties({...?properties?.value, EmailProperty.threadId}); + properties = updatedProperties; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/features/base/mixin/mail_api_mixin.dart` around lines 140 - 143, The code in the collapseThreads branch mutates a caller-supplied Properties by adding EmailProperty.threadId to properties.value (in the mixin where collapseThreads is handled); instead, avoid mutating the original: if properties is non-null and collapseThreads is true, create a defensive copy of the Properties (copying its value set), modify that copy to add EmailProperty.threadId, and use the copy thereafter; if properties is null, continue to construct a new Properties as before. Locate the collapseThreads handling in mail_api_mixin.dart and update the logic around Properties to clone before modification.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@lib/features/thread/data/repository/thread_repository_impl.dart`:
- Around line 636-658: The method loadAllEmailInFolderWithoutCache yields the
raw emailResponse even when collapseThreads is true, causing callers to get
emailList instead of threadEmails; update loadAllEmailInFolderWithoutCache (same
pattern used in forceQueryAllEmailsForWeb, _getFirstPage, and
_getAllEmailsWithoutLastEmailId) to check collapseThreads and, if true,
transform the network response by selecting response.threadEmails (or mapping
threadEmails into the EmailsResponse emails field) before yielding so callers
always receive the collapsed-thread payload when collapseThreads == true.
In `@model/lib/email/presentation_email.dart`:
- Around line 231-296: The copyWith method on PresentationEmail is missing the
emailIdsInThread field, which causes thread membership to be lost; update
PresentationEmail.copyWith to accept an optional parameter Map or Set (matching
the type of emailIdsInThread) named emailIdsInThread and return it as
emailIdsInThread: emailIdsInThread ?? this.emailIdsInThread when constructing
the new PresentationEmail so the field is preserved across copies; ensure the
new parameter name and the assignment match the existing emailIdsInThread
property used in props.
---
Nitpick comments:
In `@lib/features/base/mixin/mail_api_mixin.dart`:
- Around line 140-143: The code in the collapseThreads branch mutates a
caller-supplied Properties by adding EmailProperty.threadId to properties.value
(in the mixin where collapseThreads is handled); instead, avoid mutating the
original: if properties is non-null and collapseThreads is true, create a
defensive copy of the Properties (copying its value set), modify that copy to
add EmailProperty.threadId, and use the copy thereafter; if properties is null,
continue to construct a new Properties as before. Locate the collapseThreads
handling in mail_api_mixin.dart and update the logic around Properties to clone
before modification.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b9b83466-a795-4d21-a046-7ef00289ce82
📒 Files selected for processing (6)
lib/features/base/mixin/mail_api_mixin.dartlib/features/manage_account/domain/model/preferences/preferences_setting.dartlib/features/thread/data/extensions/reference_path_extension.dartlib/features/thread/data/repository/thread_repository_impl.dartmodel/lib/email/presentation_email.darttest/features/thread/presentation/controller/thread_controller_test.dart
✅ Files skipped from review due to trivial changes (2)
- lib/features/manage_account/domain/model/preferences/preferences_setting.dart
- lib/features/thread/data/extensions/reference_path_extension.dart
…hange when force email query = true
Member
Author
Fixed |
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)
model/lib/email/presentation_email.dart (1)
263-297:⚠️ Potential issue | 🟡 Minor
copyWithcannot clearemailIdsInThreadonce set.Using
emailIdsInThread ?? this.emailIdsInThreadprevents resetting the field tonull, which can preserve stale thread state in refresh/toggle flows.🔧 Suggested fix
PresentationEmail copyWith({ @@ - List<EmailId>? emailIdsInThread, + List<EmailId>? emailIdsInThread, + bool clearEmailIdsInThread = false, }) { return PresentationEmail( @@ - )..emailIdsInThread = emailIdsInThread ?? this.emailIdsInThread; + )..emailIdsInThread = + clearEmailIdsInThread ? null : (emailIdsInThread ?? this.emailIdsInThread); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@model/lib/email/presentation_email.dart` around lines 263 - 297, The copyWith implementation in PresentationEmail currently uses "emailIdsInThread ?? this.emailIdsInThread" which prevents callers from clearing the field to null; update the copyWith in PresentationEmail so the cascade assignment uses the passed value directly (i.e., set emailIdsInThread = emailIdsInThread without the ?? fallback) so callers can explicitly clear the field by passing null; ensure the copyWith signature still accepts List<EmailId>? emailIdsInThread and adjust any call sites that relied on implicit preservation to instead pass the existing value when they want to keep it.
🧹 Nitpick comments (2)
test/features/mailbox_dashboard/presentation/controller/mailbox_dashboard_controller_test.dart (1)
497-500: Strengthenverify(...)assertions for new flags and filter reset.These checks currently only assert argument presence, not correctness. For this PR objective, consider asserting concrete values (especially
collapseThreads/forceEmailQuery) and restoring a concreteemailFilterexpectation where reset behavior is part of the test intent.Suggested tightening pattern
- emailFilter: anyNamed('emailFilter'), + emailFilter: threadController.getEmailFilterForLoadMailbox(), ... - useCache: anyNamed('useCache'), - forceEmailQuery: anyNamed('forceEmailQuery'), - collapseThreads: anyNamed('collapseThreads') + useCache: captureAnyNamed('useCache'), + forceEmailQuery: captureAnyNamed('forceEmailQuery'), + collapseThreads: captureAnyNamed('collapseThreads')Then assert captured values explicitly after
verify(...).Also applies to: 558-565
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/features/mailbox_dashboard/presentation/controller/mailbox_dashboard_controller_test.dart` around lines 497 - 500, The verify(...) calls that currently use anyNamed('useCache'), anyNamed('forceEmailQuery'), and anyNamed('collapseThreads') should assert concrete expected values instead of just presence: replace anyNamed for 'collapseThreads' and 'forceEmailQuery' with the expected boolean literals in the verify(...) on the same call that uses ThreadConstants.propertiesUpdatedDefault, and restore a concrete expectation for the emailFilter argument (e.g., null or an empty/default filter) rather than anyNamed; if the test needs to inspect the emailFilter object more deeply, capture the argument via a capture/argThat or an ArgumentCaptor and add explicit expect/assert statements for that captured emailFilter value and for collapseThreads/forceEmailQuery to ensure they match the intended behavior.test/features/search/verify_before_time_in_search_email_filter_test.dart (1)
854-867: Consider asserting the forwardedcollapseThreadsvalue, not only presence.
anyNamed('collapseThreads')inverify(...)can pass even if the wrong value is propagated. Tightening this to explicit expected value (or captured value assertion) would protect the force-query collapse behavior from regressions.Also applies to: 1022-1035, 1135-1148
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/features/search/verify_before_time_in_search_email_filter_test.dart` around lines 854 - 867, The verify currently uses anyNamed('collapseThreads') which only asserts presence; update the assertion to check the actual boolean passed for collapseThreads by replacing anyNamed('collapseThreads') with an explicit matcher (e.g., equals(expectedCollapseThreads)) or capture the argument and assert it equals the expected value from the test context (derive expectedCollapseThreads from threadController or the test setup) when verifying mockRefreshChangesEmailsInMailboxInteractor.execute in the test that references mailboxDashboardController.currentEmailState!, threadController.limitEmailFetched, and threadController.getEmailFilterForLoadMailbox().
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@model/lib/email/presentation_email.dart`:
- Around line 263-297: The copyWith implementation in PresentationEmail
currently uses "emailIdsInThread ?? this.emailIdsInThread" which prevents
callers from clearing the field to null; update the copyWith in
PresentationEmail so the cascade assignment uses the passed value directly
(i.e., set emailIdsInThread = emailIdsInThread without the ?? fallback) so
callers can explicitly clear the field by passing null; ensure the copyWith
signature still accepts List<EmailId>? emailIdsInThread and adjust any call
sites that relied on implicit preservation to instead pass the existing value
when they want to keep it.
---
Nitpick comments:
In
`@test/features/mailbox_dashboard/presentation/controller/mailbox_dashboard_controller_test.dart`:
- Around line 497-500: The verify(...) calls that currently use
anyNamed('useCache'), anyNamed('forceEmailQuery'), and
anyNamed('collapseThreads') should assert concrete expected values instead of
just presence: replace anyNamed for 'collapseThreads' and 'forceEmailQuery' with
the expected boolean literals in the verify(...) on the same call that uses
ThreadConstants.propertiesUpdatedDefault, and restore a concrete expectation for
the emailFilter argument (e.g., null or an empty/default filter) rather than
anyNamed; if the test needs to inspect the emailFilter object more deeply,
capture the argument via a capture/argThat or an ArgumentCaptor and add explicit
expect/assert statements for that captured emailFilter value and for
collapseThreads/forceEmailQuery to ensure they match the intended behavior.
In `@test/features/search/verify_before_time_in_search_email_filter_test.dart`:
- Around line 854-867: The verify currently uses anyNamed('collapseThreads')
which only asserts presence; update the assertion to check the actual boolean
passed for collapseThreads by replacing anyNamed('collapseThreads') with an
explicit matcher (e.g., equals(expectedCollapseThreads)) or capture the argument
and assert it equals the expected value from the test context (derive
expectedCollapseThreads from threadController or the test setup) when verifying
mockRefreshChangesEmailsInMailboxInteractor.execute in the test that references
mailboxDashboardController.currentEmailState!,
threadController.limitEmailFetched, and
threadController.getEmailFilterForLoadMailbox().
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 66d2ff2f-f06e-4725-b477-420838e5985e
📒 Files selected for processing (5)
lib/features/base/mixin/mail_api_mixin.dartlib/features/thread/data/repository/thread_repository_impl.dartmodel/lib/email/presentation_email.darttest/features/mailbox_dashboard/presentation/controller/mailbox_dashboard_controller_test.darttest/features/search/verify_before_time_in_search_email_filter_test.dart
✅ Files skipped from review due to trivial changes (1)
- lib/features/thread/data/repository/thread_repository_impl.dart
🚧 Files skipped from review as they are similar to previous changes (1)
- lib/features/base/mixin/mail_api_mixin.dart
…fresh change when force email query = true
Member
Author
Fixed |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Issue
#4400
Resolved
Screen.Recording.2026-03-27.at.17.02.05.mov
Summary by CodeRabbit
New Features
Refactor
Tests