TF-4190 Performance issue to edit thread favorites #4191
Conversation
|
This PR has been deployed to https://linagora.github.io/tmail-flutter/4191. |
|
@coderabbitai review this PR |
|
@dab246, I'll review the changes in this PR for you. ✅ Actions performedReview triggered.
|
WalkthroughConsolidates keyword and "star" handling into new extensions and moves in-memory update logic out of controllers. Adds HandleEmailActionExtension with public markAsEmailStarSuccess and multiple extensions to toggle keywords on Email, PresentationEmail, EmailLoaded, EmailInThreadDetailInfo, and maps. Changes map keyword utilities to a nullable Map<KeyWordIdentifier, bool>? extension (withKeyword/withoutKeyword/toMapString). Adds EmailLoaded.copyWith. Introduces an isThread flag in thread usecases/state and propagates it through controllers. Removes PresentationEmailExtension.resyncKeywords. Suggested labels
Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
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 help Please review this PR |
|
@dab246, I'll review the changes in this PR for you. The review will analyze the performance improvements made to address the thread favorites editing issue mentioned in #4190. I'll post my detailed findings shortly. ✅ Actions performedReview triggered.
|
lib/features/thread_detail/domain/extensions/presentation_email_map_extension.dart
Outdated
Show resolved
Hide resolved
|
@coderabbitai please review |
|
@hoangdat, I'll review the changes in this PR to analyze the performance improvements for editing thread favorites. ✅ Actions performedReview triggered.
|
![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
♻️ Duplicate comments (3)
lib/features/email/presentation/extensions/presentation_email_extension.dart (1)
158-172: Consider extracting keyword manipulation to a shared helper.The
star()andunstar()methods work correctly, but this keyword manipulation pattern is duplicated across multiple files in this PR:
presentation_email_extension.dart(here)presentation_email_map_extension.dartlist_email_in_thread_detail_info_extension.dartA previous reviewer suggested creating a
KeywordHelpersextension to reduce this duplication. This would centralize the keyword map manipulation logic and make the codebase more maintainable.lib/features/thread_detail/domain/extensions/presentation_email_map_extension.dart (1)
6-110: Reduce duplication across star/unstar operations.All six methods (
starAll,unstarAll,starByIds,unstarByIds,starOne,unstarOne) share nearly identical keyword manipulation logic:final updatedKeywords = Map<KeyWordIdentifier, bool>.from( email.keywords ?? {}, )..[KeyWordIdentifier.emailFlagged] = true; // or ..remove(KeyWordIdentifier.emailFlagged)This pattern is repeated 6 times in this file alone and appears across multiple files in this PR. As suggested in a previous review, extracting this to a helper extension would significantly improve maintainability and reduce the risk of inconsistencies.
lib/features/thread_detail/domain/extensions/list_email_in_thread_detail_info_extension.dart (1)
13-87: Extract repeated keyword manipulation pattern.Similar to the duplication found in other files, all six methods share the same keyword manipulation logic:
final updatedKeywords = Map<KeyWordIdentifier, bool>.from( email.keywords ?? {}, )..[KeyWordIdentifier.emailFlagged] = true; // or ..remove(...)This pattern appears 6 times in this file and is repeated across multiple files in the PR. Centralizing this logic would reduce code duplication and improve maintainability. Don't forget to add tests for these new methods.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
lib/features/email/presentation/controller/single_email_controller.dart(2 hunks)lib/features/email/presentation/extensions/email_loaded_extension.dart(1 hunks)lib/features/email/presentation/extensions/handle_email_action_extension.dart(1 hunks)lib/features/email/presentation/extensions/presentation_email_extension.dart(2 hunks)lib/features/email/presentation/model/email_loaded.dart(1 hunks)lib/features/mailbox_dashboard/presentation/controller/mailbox_dashboard_controller.dart(2 hunks)lib/features/thread/domain/state/mark_as_star_multiple_email_state.dart(2 hunks)lib/features/thread/domain/usecases/mark_as_star_multiple_email_interactor.dart(3 hunks)lib/features/thread_detail/domain/extensions/list_email_in_thread_detail_info_extension.dart(1 hunks)lib/features/thread_detail/domain/extensions/presentation_email_map_extension.dart(1 hunks)lib/features/thread_detail/presentation/extension/handle_thread_action_success.dart(1 hunks)lib/features/thread_detail/presentation/extension/on_thread_detail_action_click.dart(1 hunks)lib/features/thread_detail/presentation/thread_detail_controller.dart(3 hunks)model/lib/extensions/presentation_email_extension.dart(0 hunks)
💤 Files with no reviewable changes (1)
- model/lib/extensions/presentation_email_extension.dart
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-12-09T09:36:45.349Z
Learnt from: dab246
Repo: linagora/tmail-flutter PR: 4194
File: lib/features/manage_account/presentation/manage_account_dashboard_controller.dart:174-176
Timestamp: 2025-12-09T09:36:45.349Z
Learning: In Dart/Flutter projects using GetX, do not wrap getBinding<T>() calls in try/catch since they return null when not found. Only wrap Get.find<T>() calls in try/catch because they throw if a dependency is unavailable. When a binding is optional, consider checking Get.isRegistered<T>() or handling the null/exception path gracefully instead of blindly catching, and document the expectation for failure modes where a dependency may not be registered.
Applied to files:
lib/features/thread_detail/presentation/thread_detail_controller.dartlib/features/thread_detail/domain/extensions/presentation_email_map_extension.dartlib/features/email/presentation/extensions/email_loaded_extension.dartlib/features/email/presentation/extensions/handle_email_action_extension.dartlib/features/email/presentation/model/email_loaded.dartlib/features/thread_detail/presentation/extension/on_thread_detail_action_click.dartlib/features/thread_detail/domain/extensions/list_email_in_thread_detail_info_extension.dartlib/features/thread_detail/presentation/extension/handle_thread_action_success.dartlib/features/email/presentation/extensions/presentation_email_extension.dartlib/features/thread/domain/state/mark_as_star_multiple_email_state.dartlib/features/mailbox_dashboard/presentation/controller/mailbox_dashboard_controller.dartlib/features/email/presentation/controller/single_email_controller.dartlib/features/thread/domain/usecases/mark_as_star_multiple_email_interactor.dart
📚 Learning: 2025-12-09T12:47:45.861Z
Learnt from: zatteo
Repo: linagora/tmail-flutter PR: 4196
File: scribe/lib/scribe/ai/data/datasource_impl/ai_datasource_impl.dart:34-35
Timestamp: 2025-12-09T12:47:45.861Z
Learning: In the tmail-flutter repository, avoid suggesting replacing DioError with DioException unless there is a codebase-wide migration. This pattern applies to all Dart files; when reviewing, only propose a DioError-safe change (e.g., compatible error handling or conversion) unless a global migration is in place. Ensure consistency of DioError usage across modules and flag any deviations for a repo-wide decision.
Applied to files:
lib/features/thread_detail/presentation/thread_detail_controller.dartlib/features/thread_detail/domain/extensions/presentation_email_map_extension.dartlib/features/email/presentation/extensions/email_loaded_extension.dartlib/features/email/presentation/extensions/handle_email_action_extension.dartlib/features/email/presentation/model/email_loaded.dartlib/features/thread_detail/presentation/extension/on_thread_detail_action_click.dartlib/features/thread_detail/domain/extensions/list_email_in_thread_detail_info_extension.dartlib/features/thread_detail/presentation/extension/handle_thread_action_success.dartlib/features/email/presentation/extensions/presentation_email_extension.dartlib/features/thread/domain/state/mark_as_star_multiple_email_state.dartlib/features/mailbox_dashboard/presentation/controller/mailbox_dashboard_controller.dartlib/features/email/presentation/controller/single_email_controller.dartlib/features/thread/domain/usecases/mark_as_star_multiple_email_interactor.dart
🔇 Additional comments (15)
lib/features/email/presentation/model/email_loaded.dart (1)
20-32: LGTM! Clean immutability helper.The
copyWithmethod follows the standard Dart pattern for immutable updates. The implementation correctly preserves existing values when null is passed, enabling selective field updates while maintaining immutability semantics.lib/features/thread_detail/presentation/extension/handle_thread_action_success.dart (1)
12-55: LGTM! Well-structured extension for thread star actions.The extension cleanly handles thread-level star/unstar success states:
- Properly consumes state through the mailbox dashboard controller
- Updates both
emailIdsPresentationandemailsInThreadDetailInfoconsistently- Includes useful logging for debugging
- Clear separation between full success (
_handleMarkAll) and partial success (_handleMarkPartial) caseslib/features/thread/domain/state/mark_as_star_multiple_email_state.dart (2)
12-19: LGTM! Clean thread context propagation.The
isThreadflag addition enables distinguishing thread-level from mailbox-level star operations. The default value offalsemaintains backward compatibility, and the flag is properly included inpropsfor equality checks.Also applies to: 43-50
6-6: Align LoadingMarkAsStarMultipleEmailAll with similar state classes.
LoadingMarkAsStarMultipleEmailAllextendsLoadingState, but the parallelLoadingMarkAsMultipleEmailReadAllin mark_as_multiple_email_read_state.dart extendsUIState. Similar "mark as" operations should use consistent parent classes. Verify which pattern is correct for your state management architecture and align accordingly.lib/features/mailbox_dashboard/presentation/controller/mailbox_dashboard_controller.dart (1)
1273-1293: LGTM! Proper thread context handling.The
isThreadparameter correctly suppresses toast notifications when starring emails from the thread detail view (line 1280), allowing the thread detail controller to handle its own success feedback. The parameter is properly propagated from the success states (lines 458, 465), and the default value offalsemaintains backward compatibility.lib/features/thread_detail/presentation/thread_detail_controller.dart (1)
51-51: LGTM! Clean integration of thread star handling.The changes properly integrate the new thread-level star action handling:
- Import of the
handle_thread_action_successextension enables the functionality- Clearing
emailsInThreadDetailInfoinreset()maintains state consistency- Delegation to
handleMarkThreadAsStarredSuccessfollows the existing extension-based patternAlso applies to: 231-231, 264-266
lib/features/thread/domain/usecases/mark_as_star_multiple_email_interactor.dart (2)
20-22: LGTM! Well-designed parameter addition.The optional
isThreadparameter with a sensible default maintains backward compatibility while enabling context-aware handling downstream.
29-34: Good propagation of context flag.The
isThreadflag is correctly propagated to both success and partial-failure states, enabling downstream handlers to distinguish thread-level operations from mailbox-level operations for performance optimization.Also applies to: 38-43
lib/features/thread_detail/presentation/extension/on_thread_detail_action_click.dart (1)
63-71: Correct context flag usage.Passing
isThread: trueis appropriate here since this code executes in the thread-detail context, ensuring the success handlers can optimize updates accordingly.lib/features/email/presentation/extensions/email_loaded_extension.dart (2)
7-21: LGTM! Defensive keyword update logic.The
starByIdmethod correctly:
- Guards against null or mismatched email IDs
- Clones the keywords map before mutation (immutability)
- Returns an updated
EmailLoadedinstance viacopyWith
23-37: LGTM! Consistent with starById implementation.The
unstarByIdmethod mirrors thestarByIdpattern correctly, removing theemailFlaggedkeyword instead of adding it.lib/features/email/presentation/extensions/handle_email_action_extension.dart (2)
12-22: LGTM! Clear routing logic.The method correctly routes star/unstar success handling to the appropriate context (mailbox vs thread-detail) based on platform and feature flags, then displays a success toast.
50-81: LGTM! Thorough null checking.The thread-detail context handler properly checks for null
threadDetailControllerandcurrentEmailIdbefore proceeding, and correctly updates all relevant state (presentation IDs, thread detail info, loaded email, and dashboard flags).lib/features/email/presentation/controller/single_email_controller.dart (2)
67-67: Clean refactoring: extension import added.The import of
handle_email_action_extension.dartsupports the refactored star success handling now provided via extension method.
224-225: Clean refactoring: delegated to extension.Replacing the internal
_markAsEmailStarSuccesscall with the extension methodmarkAsEmailStarSuccessimproves separation of concerns and enables context-aware handling of star actions.
lib/features/email/presentation/extensions/handle_email_action_extension.dart
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
lib/features/email/presentation/extensions/email_extension.dart (1)
59-65: Add basic tests around keyword toggling (add/remove + null keywords map).# Suggested test cases to add (high level): # - Email(keywords: null).toggleKeyword(x, false) => keywords contains x:true # - Email(keywords: {x:true}).toggleKeyword(x, true) => keywords does not contain x # - Idempotency: remove when absent / add when present (ensure expected map content)
🧹 Nitpick comments (5)
lib/features/email/presentation/extensions/handle_email_action_extension.dart (2)
14-22: Consider moving the toast after in-memory sync (if sync can fail/early-return in future).
Right now success UI fires before the memory updates; if later you add guards/early returns, you may show success while state doesn’t update.
24-35:isMobileThreadDisablednaming/logic is OK, but it’s making downstream behavior easy to misread.
This flag is effectively “update mailbox-selected-email instead of thread-detail memory”. If that’s the intended contract, consider renaming to something likeshouldSyncSelectedEmailInMailboxContext(or add a short comment) to reduce future regressions.lib/features/email/presentation/extensions/email_extension.dart (1)
59-65: Consider avoiding the boolean “action flag” parameter (isRemoved).
This is easy to misuse at call sites; a named parameter ({required bool remove}) or a small enum can reduce mistakes.lib/features/thread_detail/domain/extensions/list_email_in_thread_detail_info_extension.dart (1)
14-45: Bulk toggle helpers are correct; optional no-op fast paths.
IftargetIdscan be empty (oremailIdnot found), consider an early return to avoid allocating/mapping.lib/features/thread_detail/presentation/extension/handle_thread_action_success.dart (1)
29-66: Consider extracting common update pattern.Both
_handleMarkAlland_handleMarkPartialfollow nearly identical patterns: capture current state, toggle keywords, update reactive values. While not essential, extracting a common helper could reduce duplication and improve maintainability.Example refactor:
void _updateEmailStates({ required Map<EmailId, PresentationEmail> Function(Map<EmailId, PresentationEmail>) updateIds, required List<EmailInThreadDetailInfo> Function(List<EmailInThreadDetailInfo>) updateDetails, }) { emailIdsPresentation.value = updateIds(emailIdsPresentation); emailsInThreadDetailInfo.value = updateDetails(emailsInThreadDetailInfo); } void _handleMarkAll(MarkStarAction action) { final isRemoved = action == MarkStarAction.unMarkStar; _updateEmailStates( updateIds: (current) => current.toggleEmailKeywords( keyword: KeyWordIdentifier.emailFlagged, isRemoved: isRemoved, ), updateDetails: (current) => current.toggleEmailKeywords( keyword: KeyWordIdentifier.emailFlagged, isRemoved: isRemoved, ), ); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
lib/features/email/presentation/extensions/email_extension.dart(3 hunks)lib/features/email/presentation/extensions/email_loaded_extension.dart(1 hunks)lib/features/email/presentation/extensions/handle_email_action_extension.dart(1 hunks)lib/features/email/presentation/extensions/presentation_email_extension.dart(2 hunks)lib/features/thread/data/extensions/map_keywords_extension.dart(1 hunks)lib/features/thread_detail/domain/extensions/email_in_thread_detail_info_extension.dart(1 hunks)lib/features/thread_detail/domain/extensions/list_email_in_thread_detail_info_extension.dart(1 hunks)lib/features/thread_detail/domain/extensions/presentation_email_map_extension.dart(1 hunks)lib/features/thread_detail/presentation/extension/handle_thread_action_success.dart(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- lib/features/email/presentation/extensions/email_loaded_extension.dart
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-12-09T09:36:45.349Z
Learnt from: dab246
Repo: linagora/tmail-flutter PR: 4194
File: lib/features/manage_account/presentation/manage_account_dashboard_controller.dart:174-176
Timestamp: 2025-12-09T09:36:45.349Z
Learning: In Dart/Flutter projects using GetX, do not wrap getBinding<T>() calls in try/catch since they return null when not found. Only wrap Get.find<T>() calls in try/catch because they throw if a dependency is unavailable. When a binding is optional, consider checking Get.isRegistered<T>() or handling the null/exception path gracefully instead of blindly catching, and document the expectation for failure modes where a dependency may not be registered.
Applied to files:
lib/features/email/presentation/extensions/email_extension.dartlib/features/thread_detail/domain/extensions/list_email_in_thread_detail_info_extension.dartlib/features/thread_detail/domain/extensions/email_in_thread_detail_info_extension.dartlib/features/thread/data/extensions/map_keywords_extension.dartlib/features/thread_detail/presentation/extension/handle_thread_action_success.dartlib/features/email/presentation/extensions/handle_email_action_extension.dartlib/features/email/presentation/extensions/presentation_email_extension.dartlib/features/thread_detail/domain/extensions/presentation_email_map_extension.dart
📚 Learning: 2025-12-09T12:47:45.861Z
Learnt from: zatteo
Repo: linagora/tmail-flutter PR: 4196
File: scribe/lib/scribe/ai/data/datasource_impl/ai_datasource_impl.dart:34-35
Timestamp: 2025-12-09T12:47:45.861Z
Learning: In the tmail-flutter repository, avoid suggesting replacing DioError with DioException unless there is a codebase-wide migration. This pattern applies to all Dart files; when reviewing, only propose a DioError-safe change (e.g., compatible error handling or conversion) unless a global migration is in place. Ensure consistency of DioError usage across modules and flag any deviations for a repo-wide decision.
Applied to files:
lib/features/email/presentation/extensions/email_extension.dartlib/features/thread_detail/domain/extensions/list_email_in_thread_detail_info_extension.dartlib/features/thread_detail/domain/extensions/email_in_thread_detail_info_extension.dartlib/features/thread/data/extensions/map_keywords_extension.dartlib/features/thread_detail/presentation/extension/handle_thread_action_success.dartlib/features/email/presentation/extensions/handle_email_action_extension.dartlib/features/email/presentation/extensions/presentation_email_extension.dartlib/features/thread_detail/domain/extensions/presentation_email_map_extension.dart
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
- GitHub Check: analyze-test (contact)
- GitHub Check: analyze-test (fcm)
- GitHub Check: analyze-test (email_recovery)
- GitHub Check: analyze-test (server_settings)
- GitHub Check: analyze-test (rule_filter)
- GitHub Check: analyze-test (core)
- GitHub Check: analyze-test (default)
- GitHub Check: analyze-test (forward)
- GitHub Check: analyze-test (model)
- GitHub Check: Build web version and deploy
🔇 Additional comments (8)
lib/features/thread/data/extensions/map_keywords_extension.dart (1)
9-15:withKeyword/withoutKeywordlook good for null-safe immutable updates.
TheMap<KeyWordIdentifier, bool>.from(this ?? {})pattern is consistent and keeps callers simple.lib/features/email/presentation/extensions/email_extension.dart (1)
42-57: Rename toemailAddressis clearer and preserves logic.lib/features/email/presentation/extensions/presentation_email_extension.dart (1)
159-165:PresentationEmail.toggleKeywordis consistent withEmail.toggleKeywordand keeps updates centralized.lib/features/thread_detail/domain/extensions/presentation_email_map_extension.dart (1)
6-55: Logic is correct and null-safe; watch out for full-map remap cost in hot paths.
If this is called frequently for single-id updates on large maps, consider measuring and optimizing (e.g., persistent collections / structural sharing, or alternative state shape).lib/features/thread_detail/domain/extensions/email_in_thread_detail_info_extension.dart (1)
1-16: Clean, consistenttoggleKeywordimplementation for thread-detail model.lib/features/thread_detail/domain/extensions/list_email_in_thread_detail_info_extension.dart (1)
8-12: Rename toemailInfoimproves readability; logic unchanged.lib/features/thread_detail/presentation/extension/handle_thread_action_success.dart (2)
1-11: LGTM!The imports are well-organized and all appear necessary for the implementation.
13-27: The null safety concern is not applicable here. ThemailboxDashBoardControlleris obtained viaGet.find<MailboxDashBoardController>()(line 112 of ThreadDetailController), which is the standard GetX dependency injection pattern.Get.find()does not return null; it throws an exception if the service is not registered. Combined with thefinal(non-nullable) type declaration, the controller is guaranteed to be non-null at runtime or fail during initialization.Likely an incorrect or invalid review comment.
lib/features/email/presentation/extensions/handle_email_action_extension.dart
Show resolved
Hide resolved
lib/features/thread_detail/presentation/extension/handle_thread_action_success.dart
Outdated
Show resolved
Hide resolved
lib/features/thread_detail/presentation/extension/handle_thread_action_success.dart
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
test/features/thread/presentation/extensions/map_keywords_extension_test.dart (2)
46-56: Test is fragile: avoid relying onKeyWordIdentifierequality/hashCode across instances
Line 47-51 creates two separateKeyWordIdentifier("\$flagged")objects; if equality/hashCode isn’t value-based, this test won’t actually validate “overwrite existing keyword”, and may miss duplicate-key regressions.test('does not mutate original map', () { - final map = { - KeyWordIdentifier("\$flagged"): false, - }; - final keyword = KeyWordIdentifier("\$flagged"); + final keyword = KeyWordIdentifier("\$flagged"); + final map = { + keyword: false, + }; final result = map.withKeyword(keyword); expect(result[keyword], true); expect(map[keyword], false); });
5-109: Missing coverage for nullable receiver (Map<KeyWordIdentifier, bool>?)
The extension API is nullable-receiver per summary, but all tests use non-null maps. Add explicit tests fornull.withKeyword(...)andnull.withoutKeyword(...)based on the intended contract (e.g., return empty map vsnull).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
test/features/thread/presentation/extensions/map_keywords_extension_test.dart(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-12-09T09:36:45.349Z
Learnt from: dab246
Repo: linagora/tmail-flutter PR: 4194
File: lib/features/manage_account/presentation/manage_account_dashboard_controller.dart:174-176
Timestamp: 2025-12-09T09:36:45.349Z
Learning: In Dart/Flutter projects using GetX, do not wrap getBinding<T>() calls in try/catch since they return null when not found. Only wrap Get.find<T>() calls in try/catch because they throw if a dependency is unavailable. When a binding is optional, consider checking Get.isRegistered<T>() or handling the null/exception path gracefully instead of blindly catching, and document the expectation for failure modes where a dependency may not be registered.
Applied to files:
test/features/thread/presentation/extensions/map_keywords_extension_test.dart
📚 Learning: 2025-12-09T12:47:45.861Z
Learnt from: zatteo
Repo: linagora/tmail-flutter PR: 4196
File: scribe/lib/scribe/ai/data/datasource_impl/ai_datasource_impl.dart:34-35
Timestamp: 2025-12-09T12:47:45.861Z
Learning: In the tmail-flutter repository, avoid suggesting replacing DioError with DioException unless there is a codebase-wide migration. This pattern applies to all Dart files; when reviewing, only propose a DioError-safe change (e.g., compatible error handling or conversion) unless a global migration is in place. Ensure consistency of DioError usage across modules and flag any deviations for a repo-wide decision.
Applied to files:
test/features/thread/presentation/extensions/map_keywords_extension_test.dart
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
- GitHub Check: analyze-test (contact)
- GitHub Check: analyze-test (default)
- GitHub Check: analyze-test (fcm)
- GitHub Check: analyze-test (rule_filter)
- GitHub Check: analyze-test (server_settings)
- GitHub Check: analyze-test (forward)
- GitHub Check: analyze-test (model)
- GitHub Check: analyze-test (email_recovery)
- GitHub Check: analyze-test (core)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
lib/features/thread_detail/domain/extensions/presentation_email_map_extension.dart (1)
40-54: Consider direct map access for single-ID updates.The current implementation iterates through the entire map (O(n)) to update a single entry. Since the PR addresses performance issues, consider using direct map access for O(1) lookup.
Apply this diff to optimize the method:
Map<EmailId, PresentationEmail?> toggleEmailKeywordById({ required EmailId emailId, required KeyWordIdentifier keyword, required bool remove, }) { - return map((id, email) { - if (id != emailId || email == null) { - return MapEntry(id, email); - } - return MapEntry( - id, - email.toggleKeyword(keyword, remove), - ); - }); + final email = this[emailId]; + if (email == null) return this; + return {...this, emailId: email.toggleKeyword(keyword, remove)}; }test/features/email/presentation/extensions/email_extension_test.dart (1)
281-368: Good coverage; please confirmkeywordsempty-state contract (nullvs{}).A couple tests (e.g., Line 310-312) assert
result.keywordsis non-null even after removing the only entry; if production intends to normalize “no keywords” tonull, these should be adjusted.Also, the immutability test (Line 353-367) checks instance identity only; consider additionally asserting the original
email.keywordscontents remain unchanged (deep immutability).lib/features/thread_detail/domain/extensions/list_email_in_thread_detail_info_extension.dart (1)
14-47: Keyword toggling extensions look solid; consider consistent immutability on empty input.
toggleEmailKeywordByIdsreturnsthiswhentargetIdsis empty (Line 27). If any caller expects a fresh list instance from these APIs (even for no-op calls), returningList.of(this)would make that behavior consistent.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
lib/features/email/presentation/extensions/email_loaded_extension.dart(1 hunks)lib/features/email/presentation/extensions/handle_email_action_extension.dart(1 hunks)lib/features/thread/data/extensions/map_keywords_extension.dart(1 hunks)lib/features/thread_detail/domain/extensions/list_email_in_thread_detail_info_extension.dart(1 hunks)lib/features/thread_detail/domain/extensions/presentation_email_map_extension.dart(1 hunks)lib/features/thread_detail/presentation/extension/handle_thread_action_success.dart(1 hunks)test/features/email/presentation/extensions/email_extension_test.dart(2 hunks)test/features/thread/presentation/extensions/map_keywords_extension_test.dart(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- lib/features/thread/data/extensions/map_keywords_extension.dart
- lib/features/email/presentation/extensions/email_loaded_extension.dart
- lib/features/email/presentation/extensions/handle_email_action_extension.dart
- test/features/thread/presentation/extensions/map_keywords_extension_test.dart
🧰 Additional context used
🧠 Learnings (5)
📓 Common learnings
Learnt from: dab246
Repo: linagora/tmail-flutter PR: 4191
File: lib/features/email/presentation/extensions/handle_email_action_extension.dart:37-80
Timestamp: 2025-12-12T04:54:05.432Z
Learning: In lib/features/email/presentation/extensions/handle_email_action_extension.dart, the mailboxDashBoardController.selectedEmail should only be synchronized when isMobileThreadDisabled is true. This is intentional behavior and should not be changed to update selectedEmail in non-mobile or thread-enabled contexts.
📚 Learning: 2025-12-12T04:54:05.432Z
Learnt from: dab246
Repo: linagora/tmail-flutter PR: 4191
File: lib/features/email/presentation/extensions/handle_email_action_extension.dart:37-80
Timestamp: 2025-12-12T04:54:05.432Z
Learning: In lib/features/email/presentation/extensions/handle_email_action_extension.dart, the mailboxDashBoardController.selectedEmail should only be synchronized when isMobileThreadDisabled is true. This is intentional behavior and should not be changed to update selectedEmail in non-mobile or thread-enabled contexts.
Applied to files:
lib/features/thread_detail/domain/extensions/presentation_email_map_extension.darttest/features/email/presentation/extensions/email_extension_test.dartlib/features/thread_detail/presentation/extension/handle_thread_action_success.dartlib/features/thread_detail/domain/extensions/list_email_in_thread_detail_info_extension.dart
📚 Learning: 2025-12-09T09:36:45.349Z
Learnt from: dab246
Repo: linagora/tmail-flutter PR: 4194
File: lib/features/manage_account/presentation/manage_account_dashboard_controller.dart:174-176
Timestamp: 2025-12-09T09:36:45.349Z
Learning: In Dart/Flutter projects using GetX, do not wrap getBinding<T>() calls in try/catch since they return null when not found. Only wrap Get.find<T>() calls in try/catch because they throw if a dependency is unavailable. When a binding is optional, consider checking Get.isRegistered<T>() or handling the null/exception path gracefully instead of blindly catching, and document the expectation for failure modes where a dependency may not be registered.
Applied to files:
lib/features/thread_detail/domain/extensions/presentation_email_map_extension.darttest/features/email/presentation/extensions/email_extension_test.dartlib/features/thread_detail/presentation/extension/handle_thread_action_success.dartlib/features/thread_detail/domain/extensions/list_email_in_thread_detail_info_extension.dart
📚 Learning: 2025-12-09T12:47:45.861Z
Learnt from: zatteo
Repo: linagora/tmail-flutter PR: 4196
File: scribe/lib/scribe/ai/data/datasource_impl/ai_datasource_impl.dart:34-35
Timestamp: 2025-12-09T12:47:45.861Z
Learning: In the tmail-flutter repository, avoid suggesting replacing DioError with DioException unless there is a codebase-wide migration. This pattern applies to all Dart files; when reviewing, only propose a DioError-safe change (e.g., compatible error handling or conversion) unless a global migration is in place. Ensure consistency of DioError usage across modules and flag any deviations for a repo-wide decision.
Applied to files:
lib/features/thread_detail/domain/extensions/presentation_email_map_extension.darttest/features/email/presentation/extensions/email_extension_test.dartlib/features/thread_detail/presentation/extension/handle_thread_action_success.dartlib/features/thread_detail/domain/extensions/list_email_in_thread_detail_info_extension.dart
📚 Learning: 2025-12-08T08:11:08.985Z
Learnt from: dab246
Repo: linagora/tmail-flutter PR: 4189
File: lib/features/login/presentation/extensions/handle_company_server_login_info_extension.dart:50-56
Timestamp: 2025-12-08T08:11:08.985Z
Learning: In the tmail-flutter codebase, prefer descriptive variable names that include full context rather than abbreviated names. For example, `removeLoginInfoInteractor` is preferred over shorter alternatives like `removeInteractor` or `interactor`, as it clearly conveys both the action (remove) and the specific data being operated on (loginInfo).
Applied to files:
lib/features/thread_detail/presentation/extension/handle_thread_action_success.dart
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
- GitHub Check: analyze-test (server_settings)
- GitHub Check: analyze-test (core)
- GitHub Check: analyze-test (email_recovery)
- GitHub Check: analyze-test (forward)
- GitHub Check: analyze-test (contact)
- GitHub Check: analyze-test (fcm)
- GitHub Check: analyze-test (model)
- GitHub Check: analyze-test (rule_filter)
- GitHub Check: analyze-test (default)
🔇 Additional comments (7)
lib/features/thread_detail/domain/extensions/presentation_email_map_extension.dart (3)
1-6: LGTM! Clean extension setup.The imports are appropriate and the extension on
Map<EmailId, PresentationEmail?>with nullable values aligns well with the keyword toggling functionality.
7-20: LGTM! Correctly handles bulk keyword toggling.The method properly handles null values and maintains immutability by returning a new map. The logic for toggling keywords across all entries is sound.
22-38: LGTM! Efficient ID-based filtering.Good use of
toSet()at line 27 for O(1) lookup when checking IDs. The method correctly toggles keywords only for specified IDs while preserving the rest of the map structure.test/features/email/presentation/extensions/email_extension_test.dart (1)
5-5: Import looks correct for new keyword API.lib/features/thread_detail/presentation/extension/handle_thread_action_success.dart (2)
38-77: Nice consolidation via_updateEmailStates(single place to keep map/list in sync).
21-36: Fixswitchcases: missingbreak/return(likely won’t compile).Each case (Line 26-33) should terminate; either add
break;or use=>shorthand.switch (success) { case MarkAsStarMultipleEmailAllSuccess(): _handleMarkAll(success.markStarAction); + break; case MarkAsStarMultipleEmailHasSomeEmailFailure(): _handleMarkPartial( success.markStarAction, success.successEmailIds, ); + break; default: break; }⛔ Skipped due to learnings
Learnt from: dab246 Repo: linagora/tmail-flutter PR: 4191 File: lib/features/email/presentation/extensions/handle_email_action_extension.dart:37-80 Timestamp: 2025-12-12T04:54:05.432Z Learning: In lib/features/email/presentation/extensions/handle_email_action_extension.dart, the mailboxDashBoardController.selectedEmail should only be synchronized when isMobileThreadDisabled is true. This is intentional behavior and should not be changed to update selectedEmail in non-mobile or thread-enabled contexts.lib/features/thread_detail/domain/extensions/list_email_in_thread_detail_info_extension.dart (1)
8-13: Renaming toemailInfoimproves readability.
|
@coderabbitai please review again |
|
@dab246, I'll review the changes in this PR again to analyze the performance improvements for editing thread favorites. ✅ Actions performedReview triggered.
|
lib/features/email/presentation/extensions/presentation_email_extension.dart
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
lib/features/thread_detail/domain/extensions/list_email_in_thread_detail_info_extension.dart (2)
24-36: Efficient multi‑ID toggle; consider semantics of returningthisfor empty targetsUsing
targetIds.toSet()and a singlemappass is efficient for bulk updates and avoids repeated.containson aList. The earlyif (targetIds.isEmpty) return this;is also a nice micro‑optimization; just ensure all callers are fine with sometimes getting the same list instance back and sometimes a new one. If consistent “new list” semantics are desired, you could instead always returntoList()even in the empty‑IDs case.
38-49: Single‑ID toggle is correct; minor optional optimization onlyThe per‑ID toggle correctly leaves non‑matching entries untouched and applies
toggleKeywordonly to the targeted email. It’s concise and readable. If this ends up in a very hot path, you could replacemapwith an indexedforloop that stops once the target is found, but that’s an optional micro‑optimization, not required for correctness.lib/features/email/presentation/extensions/email_loaded_extension.dart (1)
7-21: Implementation is correct; optional refactor suggestion available.The null-safety logic and ID matching check are correctly implemented. The method appropriately returns early if the email doesn't match.
The optional chaining operator on line 16 (
emailCurrent?.toggleKeyword) is technically redundant since line 12 already ensuresemailCurrentis not null when reaching that point. While this defensive programming is safe, you could simplify toemailCurrent!.toggleKeywordfor clarity if preferred.The
EmailLoaded.copyWithmethod correctly accepts theemailCurrentparameter as used in line 15.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
lib/features/email/presentation/extensions/email_extension.dart(3 hunks)lib/features/email/presentation/extensions/email_loaded_extension.dart(1 hunks)lib/features/email/presentation/extensions/handle_email_action_extension.dart(1 hunks)lib/features/email/presentation/extensions/presentation_email_extension.dart(2 hunks)lib/features/thread_detail/domain/extensions/email_in_thread_detail_info_extension.dart(1 hunks)lib/features/thread_detail/domain/extensions/list_email_in_thread_detail_info_extension.dart(1 hunks)lib/features/thread_detail/domain/extensions/presentation_email_map_extension.dart(1 hunks)test/features/email/presentation/extensions/email_extension_test.dart(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- lib/features/thread_detail/domain/extensions/presentation_email_map_extension.dart
- test/features/email/presentation/extensions/email_extension_test.dart
- lib/features/email/presentation/extensions/presentation_email_extension.dart
- lib/features/thread_detail/domain/extensions/email_in_thread_detail_info_extension.dart
- lib/features/email/presentation/extensions/handle_email_action_extension.dart
🧰 Additional context used
🧠 Learnings (7)
📓 Common learnings
Learnt from: dab246
Repo: linagora/tmail-flutter PR: 4204
File: lib/features/email/presentation/extensions/handle_label_for_email_extension.dart:94-148
Timestamp: 2025-12-12T07:43:26.643Z
Learning: In lib/features/email/presentation/extensions/handle_label_for_email_extension.dart and similar keyword synchronization code, the addKeyword() function is the preferred method for adding keywords to emails. The resyncKeywords() pattern is being phased out and will be replaced in favor of addKeyword().
Learnt from: dab246
Repo: linagora/tmail-flutter PR: 4191
File: lib/features/email/presentation/extensions/handle_email_action_extension.dart:37-80
Timestamp: 2025-12-12T04:54:11.121Z
Learning: In lib/features/email/presentation/extensions/handle_email_action_extension.dart, the mailboxDashBoardController.selectedEmail should only be synchronized when isMobileThreadDisabled is true. This is intentional behavior and should not be changed to update selectedEmail in non-mobile or thread-enabled contexts.
Learnt from: dab246
Repo: linagora/tmail-flutter PR: 4204
File: lib/features/email/data/datasource_impl/email_hive_cache_datasource_impl.dart:579-582
Timestamp: 2025-12-12T07:56:31.877Z
Learning: In lib/features/email/data/datasource_impl/email_hive_cache_datasource_impl.dart, the addLabelToEmail method intentionally throws UnimplementedError because label synchronization with the email cache is handled separately via extension methods (e.g., _autoSyncLabelToSelectedEmailOnMemory in handle_label_for_email_extension.dart). Implementing it in the cache datasource would cause data conflicts. This differs from other keyword operations like markAsRead, markAsStar, markAsAnswered, and markAsForwarded, which are implemented directly in the cache datasource.
📚 Learning: 2025-12-12T07:43:21.388Z
Learnt from: dab246
Repo: linagora/tmail-flutter PR: 4204
File: lib/features/email/presentation/extensions/handle_label_for_email_extension.dart:94-148
Timestamp: 2025-12-12T07:43:21.388Z
Learning: In lib/features/email/presentation/extensions, adopt addKeyword() as the preferred method for adding keywords to emails. The resyncKeywords() pattern is deprecated and should be replaced with addKeyword(). Apply this guideline to all Dart files in this directory that implement keyword synchronization, ensuring consistent usage and removal or refactoring of any resyncKeywords() implementations.
Applied to files:
lib/features/email/presentation/extensions/email_loaded_extension.dartlib/features/email/presentation/extensions/email_extension.dart
📚 Learning: 2025-12-12T04:54:11.121Z
Learnt from: dab246
Repo: linagora/tmail-flutter PR: 4191
File: lib/features/email/presentation/extensions/handle_email_action_extension.dart:37-80
Timestamp: 2025-12-12T04:54:11.121Z
Learning: In lib/features/email/presentation/extensions/handle_email_action_extension.dart, the mailboxDashBoardController.selectedEmail should only be synchronized when isMobileThreadDisabled is true. This is intentional behavior and should not be changed to update selectedEmail in non-mobile or thread-enabled contexts.
Applied to files:
lib/features/email/presentation/extensions/email_loaded_extension.dartlib/features/thread_detail/domain/extensions/list_email_in_thread_detail_info_extension.dartlib/features/email/presentation/extensions/email_extension.dart
📚 Learning: 2025-12-12T07:56:31.877Z
Learnt from: dab246
Repo: linagora/tmail-flutter PR: 4204
File: lib/features/email/data/datasource_impl/email_hive_cache_datasource_impl.dart:579-582
Timestamp: 2025-12-12T07:56:31.877Z
Learning: In lib/features/email/data/datasource_impl/email_hive_cache_datasource_impl.dart, the addLabelToEmail method intentionally throws UnimplementedError because label synchronization with the email cache is handled separately via extension methods (e.g., _autoSyncLabelToSelectedEmailOnMemory in handle_label_for_email_extension.dart). Implementing it in the cache datasource would cause data conflicts. This differs from other keyword operations like markAsRead, markAsStar, markAsAnswered, and markAsForwarded, which are implemented directly in the cache datasource.
Applied to files:
lib/features/email/presentation/extensions/email_loaded_extension.dartlib/features/thread_detail/domain/extensions/list_email_in_thread_detail_info_extension.dartlib/features/email/presentation/extensions/email_extension.dart
📚 Learning: 2025-12-09T09:36:45.349Z
Learnt from: dab246
Repo: linagora/tmail-flutter PR: 4194
File: lib/features/manage_account/presentation/manage_account_dashboard_controller.dart:174-176
Timestamp: 2025-12-09T09:36:45.349Z
Learning: In Dart/Flutter projects using GetX, do not wrap getBinding<T>() calls in try/catch since they return null when not found. Only wrap Get.find<T>() calls in try/catch because they throw if a dependency is unavailable. When a binding is optional, consider checking Get.isRegistered<T>() or handling the null/exception path gracefully instead of blindly catching, and document the expectation for failure modes where a dependency may not be registered.
Applied to files:
lib/features/email/presentation/extensions/email_loaded_extension.dartlib/features/thread_detail/domain/extensions/list_email_in_thread_detail_info_extension.dartlib/features/email/presentation/extensions/email_extension.dart
📚 Learning: 2025-12-09T12:47:45.861Z
Learnt from: zatteo
Repo: linagora/tmail-flutter PR: 4196
File: scribe/lib/scribe/ai/data/datasource_impl/ai_datasource_impl.dart:34-35
Timestamp: 2025-12-09T12:47:45.861Z
Learning: In the tmail-flutter repository, avoid suggesting replacing DioError with DioException unless there is a codebase-wide migration. This pattern applies to all Dart files; when reviewing, only propose a DioError-safe change (e.g., compatible error handling or conversion) unless a global migration is in place. Ensure consistency of DioError usage across modules and flag any deviations for a repo-wide decision.
Applied to files:
lib/features/email/presentation/extensions/email_loaded_extension.dartlib/features/thread_detail/domain/extensions/list_email_in_thread_detail_info_extension.dartlib/features/email/presentation/extensions/email_extension.dart
📚 Learning: 2025-12-12T07:43:26.643Z
Learnt from: dab246
Repo: linagora/tmail-flutter PR: 4204
File: lib/features/email/presentation/extensions/handle_label_for_email_extension.dart:94-148
Timestamp: 2025-12-12T07:43:26.643Z
Learning: In lib/features/email/presentation/extensions/handle_label_for_email_extension.dart and similar keyword synchronization code, the addKeyword() function is the preferred method for adding keywords to emails. The resyncKeywords() pattern is being phased out and will be replaced in favor of addKeyword().
Applied to files:
lib/features/thread_detail/domain/extensions/list_email_in_thread_detail_info_extension.dart
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
- GitHub Check: analyze-test (fcm)
- GitHub Check: Build web version and deploy
- GitHub Check: analyze-test (model)
- GitHub Check: analyze-test (core)
- GitHub Check: analyze-test (default)
- GitHub Check: analyze-test (rule_filter)
- GitHub Check: analyze-test (server_settings)
- GitHub Check: analyze-test (email_recovery)
- GitHub Check: analyze-test (forward)
- GitHub Check: analyze-test (contact)
🔇 Additional comments (6)
lib/features/thread_detail/domain/extensions/list_email_in_thread_detail_info_extension.dart (2)
2-12: Imports andemailIdsToDisplaylook correct and side‑effect freeUsing
EmailId/EmailInThreadDetailInfoconsistently and renaming the lambda parameter toemailInfoimproves clarity without changing behavior. TheemailIdsToDisplayhelper is straightforward and efficiently filters onisValidToDisplayonly when needed.
14-22:toggleEmailKeywordsis clear and aligns with immutable update patternThe method cleanly applies
toggleKeywordto every item and returns a newList<EmailInThreadDetailInfo>, which is consistent with functional/immutable update patterns and avoids in‑place surprises.lib/features/email/presentation/extensions/email_extension.dart (3)
4-4: LGTM!The new imports support the keyword toggling functionality introduced in this extension.
Also applies to: 10-10
44-44: LGTM!The variable naming corrections fix typos and ensure proper property access.
Also applies to: 55-55
59-68: All dependent methods and tests are in place.The
toggleKeywordimplementation correctly depends on:
withKeywordandwithoutKeywordextension methods inlib/features/thread/data/extensions/map_keywords_extension.dart—both properly implemented and thoroughly testedEmail.copyWithmethod which correctly accepts thekeywordsparameter- Comprehensive test coverage in
test/features/email/presentation/extensions/email_extension_test.dartincluding null keyword handling, keyword addition/removal, and immutabilityThe method is well-integrated and actively used across the codebase.
lib/features/email/presentation/extensions/email_loaded_extension.dart (1)
1-5: LGTM!The imports are appropriate and necessary for the extension functionality.
|
Please @dab246 can I get an executive summarry of what the issue was, and how it was solved? Thanks. |
|
@chibenwa Before
This implementation
|
Fixed |
3 participants
Issue
#4190
Resolved
Screen.Recording.2025-12-05.at.18.07.13.mov
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Tests
✏️ Tip: You can customize this high-level summary in your review settings.