TF-4195 Add a label to an email #4204
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 You can disable this status message by setting the WalkthroughThis PR implements email labeling functionality and refactors dialog routing patterns. Key changes include converting the Label model's keyword field from String? to KeyWordIdentifier?, introducing label-related UI components and state management, adding an addLabelToEmail method chain across data layers, and establishing popup submenu support via PopupSubmenuController. DialogRouter is refactored from static methods to a singleton factory pattern with instance invocations throughout the codebase. The KeywordIdentifier extension gains a generateLabelActionPath method, and the map keywords extension becomes nullable-aware with new mutation helpers. Localization strings for label operations are added alongside a new EmailActionType.labelAs action. Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
This PR has been deployed to https://linagora.github.io/tmail-flutter/4204. |
|
@coderabbitai please review this work. |
|
@chibenwa: I'll review the changes in this PR. Let me analyze the implementation of the "Add a label to an email" feature. ✅ 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: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
labels/test/method/get/get_label_method_test.dart (1)
209-230: Test description/type mismatch (“DioException” vsDioError).
The test name says “should throw DioException” but the assertion expectsDioError. Either rename the test or align the expectation with the project’s Dio error type conventions (avoiding a piecemeal migration). Based on learnings, don’t switch the whole file toDioExceptionunless there’s a repo-wide decision.lib/features/base/widget/popup_menu/popup_menu_action_group_widget.dart (1)
7-52: Ensure submenu overlay is always dismissed (not just on “hover other item”)
Right now the submenu is hidden ononHoverOtherItem, but it may remain visible if the popup menu closes via click selection or outside-tap/escape. Consider always hiding in afinallyaroundopenPopupMenuAction(...), and also beforeNavigator.pop(...).class PopupMenuActionGroupWidget with PopupContextMenuActionMixin { @@ Future<void> show( BuildContext context, RelativeRect position, ) async { @@ - return openPopupMenuAction(context, position, popupMenuItems); + try { + await openPopupMenuAction(context, position, popupMenuItems); + } finally { + submenuController?.hide(); + } } @@ child: PopupMenuItemActionWidget( menuAction: menuAction, menuActionClick: (menuAction) { + submenuController?.hide(); Navigator.pop(context); onActionSelected(menuAction); },Also applies to: 66-83
lib/features/mailbox_dashboard/presentation/extensions/update_current_emails_flags_extension.dart (1)
11-25: Potential no-op when adding a label ifpresentationEmail.keywordsis null. The_updateKeyword()method uses safe navigation (presentationEmail.keywords?[keyword] = true), which silently fails ifkeywordsis null. Additionally,isLabelAddeddefaults tofalse, making it easy to accidentally remove a label instead of adding it when passinglabelKeywordwithout explicitly settingisLabelAdded=true.Consider ensuring
keywordsis initialized before mutation, or adopt thecopyWithpattern already used elsewhere in the codebase (seepresentation_email_extension.dartline 162) to replace the email with updated keywords rather than mutating in-place.lib/main/routes/dialog_router.dart (1)
26-49: GateisRuleFilterDialogOpenedconsistently (avoid stale true on non-web).
_bindingDI()setsisRuleFilterDialogOpenedto true forAppRoutes.rulesFilterCreator, butpushGeneralDialog()only resets it on web. If any non-web code reads this flag, it can remain stucktrue.case AppRoutes.rulesFilterCreator: - isRuleFilterDialogOpened.value = true; + if (PlatformInfo.isWeb) { + isRuleFilterDialogOpened.value = true; + } RulesFilterCreatorBindings().dependencies(); break;Also applies to: 56-79
lib/features/email/presentation/controller/single_email_controller.dart (1)
222-274: Success/failure routing for label state looks fine; logging may be noisy.
If these logs are expected in production, OK; otherwise consider reducing to debug-only.
🧹 Nitpick comments (20)
model/lib/extensions/keyword_identifier_extension.dart (4)
9-14: Nice consolidation; clarify the “null means remove” contract for keyword patches
generatePath()+_boolPatch()removes a lot of duplication. One thing to make explicit (doc comment) is that passingnullis intended to unset/remove the keyword in JMAP patch semantics (vs writingfalse). That assumption is load-bearing forgenerateReadActionPath/generateMarkStarActionPath.
16-19: Simplify boolean-to-null logic for readability
return _boolPatch(isRead ? true : null);can be reduced toreturn _boolPatch(isRead ? true : null)→return _boolPatch(isRead ? true : null)is already short, but you can also avoid the ternary by inlining:return _boolPatch(isRead ? true : null);→return _boolPatch(isRead ? true : null);(orreturn _boolPatch(isRead ? true : null);). Concretely, I’d suggestreturn _boolPatch(isRead ? true : null);→return _boolPatch(isRead ? true : null);is fine; if you want an actual improvement:return _boolPatch(isRead ? true : null);→return _boolPatch(isRead ? true : null);isn’t one. Better: just inline the comparison:return _boolPatch(action == ReadActions.markAsRead ? true : null);.
21-24: Same simplification opportunity as read-action pathConsider inlining
isStarsimilarly to keep these generators uniform and compact.
11-14: Proposed tiny cleanup diff (optional)PatchObject generateReadActionPath(ReadActions action) { - final isRead = action == ReadActions.markAsRead; - return _boolPatch(isRead ? true : null); + return _boolPatch(action == ReadActions.markAsRead ? true : null); } PatchObject generateMarkStarActionPath(MarkStarAction action) { - final isStar = action == MarkStarAction.markStar; - return _boolPatch(isStar ? true : null); + return _boolPatch(action == MarkStarAction.markStar ? true : null); }Also applies to: 16-24
lib/features/labels/domain/exceptions/label_exceptions.dart (1)
1-1: Add a message/toString for debuggability.
Right now logs will just showInstance of 'LabelKeywordIsNull'. Consider@override String toString() => 'LabelKeywordIsNull: ...';.labels/lib/extensions/label_extension.dart (1)
30-41:copyWithcan’t explicitly clearkeywordto null.
If “unset keyword” is a valid operation, consider a wrapper (e.g.,Optional<KeyWordIdentifier?>) or abool clearKeyword = falseflag; currentkeyword ?? this.keywordonly supports “keep existing” vs “set non-null”.lib/features/email/presentation/bindings/email_interactor_bindings.dart (1)
19-19: Confirm DI registration order forEmailRepositorybeforeAddALabelToAnEmailInteractor.
AddALabelToAnEmailInteractoris created withGet.find<EmailRepository>(); ifbindingsInteractor()can run beforebindingsRepository(), this will throw at runtime. If the baseInteractorsBindings.dependencies()guarantees repository bindings first, then this is fine.Optional: consider typing the registration for consistency/readability:
- Get.lazyPut( - () => AddALabelToAnEmailInteractor(Get.find<EmailRepository>()), - ); + Get.lazyPut<AddALabelToAnEmailInteractor>( + () => AddALabelToAnEmailInteractor(Get.find<EmailRepository>()), + );Also applies to: 121-123
lib/features/mailbox/presentation/model/popup_menu_item_mailbox_action.dart (1)
29-32:hoverIconoverride: verify UI semantics + base contract.
ReturningicThumbsUpfor all mailbox actions on hover might be surprising UX (vs “same icon”, highlight, etc.). Also ensurePopupMenuItemActionRequiredIconexpects/useshoverIcon(otherwise dead code).lib/features/composer/presentation/composer_view_web.dart (1)
56-62: CacheDialogRouter()once inside theObxto avoid double instantiation/read skew.Even if it’s a factory singleton, prefer a single local reference for clarity and to guarantee consistent state reads:
final iframeOverlay = Obx(() { + final dialogRouter = DialogRouter(); bool isOverlayEnabled = controller.mailboxDashBoardController.isDisplayedOverlayViewOnIFrame || MessageDialogActionManager().isDialogOpened || EmailActionReactor.isDialogOpened || ColorDialogPicker().isOpened.isTrue || - DialogRouter().isRuleFilterDialogOpened.isTrue || - DialogRouter().isDialogOpened; + dialogRouter.isRuleFilterDialogOpened.isTrue || + dialogRouter.isDialogOpened;lib/features/manage_account/presentation/identities/widgets/signature_builder.dart (1)
31-35: Same here: cacheDialogRouter()once inObx.final iframeOverlay = Obx(() { - if (MessageDialogActionManager().isDialogOpened || - DialogRouter().isDialogOpened || - DialogRouter().isRuleFilterDialogOpened.isTrue) { + final dialogRouter = DialogRouter(); + if (MessageDialogActionManager().isDialogOpened || + dialogRouter.isDialogOpened || + dialogRouter.isRuleFilterDialogOpened.isTrue) {lib/features/email/data/datasource/email_datasource.dart (1)
206-213: Consider returning a result (e.g., success +SetErrors) instead ofvoidfor label mutation.Most other mutating email ops return
{ emailIdsSuccess, mapErrors }; having this returnvoidmakes partial failures harder to surface consistently. If the underlying JMAP call can return per-id errors, consider aligning the signature.lib/features/email/data/repository/email_repository_impl.dart (1)
484-495: Consider updating the cache layer for consistency.Unlike other email operations (
markAsRead,markAsStar,moveToMailbox), this method only updates the network datasource without propagating the change to thehiveCachedatasource. This could lead to stale cached data. Consider whether the cache should also be updated to reflect the label change, similar to the pattern used in lines 115-134 formarkAsRead.If cache updates are needed, apply a pattern similar to:
@override Future<void> addLabelToEmail( AccountId accountId, EmailId emailId, KeyWordIdentifier labelKeyword, ) { - return emailDataSource[DataSourceType.network]!.addLabelToEmail( + return Future.value().then((_) async { + await emailDataSource[DataSourceType.network]!.addLabelToEmail( accountId, emailId, labelKeyword, ); + try { + await emailDataSource[DataSourceType.hiveCache]!.addLabelToEmail( + accountId, + emailId, + labelKeyword, + ); + } catch (e) { + logError('EmailRepositoryImpl::addLabelToEmail:exception $e'); + } + }); }lib/features/labels/presentation/widgets/label_item_context_menu.dart (1)
1-62: Make text color/theme adaptive (avoid hard-codedColors.black)
UsingColors.blackwill likely break dark theme or themed popups. PreferTheme.of(context).colorScheme.onSurface(or a ThemeUtils variant that derives from theme).- style: ThemeUtils.textStyleBodyBody3(color: Colors.black), + style: ThemeUtils.textStyleBodyBody3( + color: Theme.of(context).colorScheme.onSurface, + ),lib/features/email/domain/usecases/add_a_label_to_an_email_interactor.dart (1)
15-38: Capture stack trace on failures to preserve debuggability
Catching onlyeloses stack context; most of your failure types benefit from retaining it (even if you just log it).- } catch (e) { + } catch (e, s) { yield Left(AddALabelToAnEmailFailure( exception: e, labelDisplay: labelDisplay, )); + // Optional: logError('AddALabelToAnEmailInteractor::execute', e, s); }lib/features/email/presentation/extensions/email_loaded_extension.dart (1)
8-18: Prefer non-null local var to avoid nullable chaining after the guard
AfteremailCurrent == nullis ruled out,emailCurrent?.…makes the code harder to reason about and can mask unexpected nulls.EmailLoaded addEmailKeyword({ @@ - final newKeyword = emailCurrent?.keywords?.withKeyword(keyword); - final updatedEmail = emailCurrent?.copyWith(keywords: newKeyword); + final current = emailCurrent!; + final newKeyword = current.keywords?.withKeyword(keyword); + final updatedEmail = current.copyWith(keywords: newKeyword); return copyWith(emailCurrent: updatedEmail); } @@ - final newKeyword = emailCurrent?.keywords?.withoutKeyword(keyword); - final updatedEmail = emailCurrent?.copyWith(keywords: newKeyword); + final current = emailCurrent!; + final newKeyword = current.keywords?.withoutKeyword(keyword); + final updatedEmail = current.copyWith(keywords: newKeyword); return copyWith(emailCurrent: updatedEmail); }Also applies to: 20-30
lib/features/labels/presentation/widgets/add_label_to_email_modal.dart (1)
15-19: Tighten callback typing + ensure GetX dependency availability for ImagePaths. Prefertypedef OnAddLabelToEmailCallback = void Function(EmailId emailId, Label label, bool isSelected);(type-safety) and please confirmImagePathsis always registered before this modal is built (sinceGet.find<ImagePaths>()will throw).Proposed diff:
-typedef OnAddLabelToEmailCallback = Function( +typedef OnAddLabelToEmailCallback = void Function( EmailId emailId, Label label, bool isSelected, );Also applies to: 40-41, 134-142
lib/features/thread/domain/extensions/presentation_email_map_extension.dart (1)
6-29: LGTM! Clean extension implementation.The immutable pattern returning a new map is appropriate. The logic correctly handles null emails and non-matching IDs.
One optional optimization consideration: iterating the entire map with
.map()is O(n) when you only need to update a single key. If performance becomes a concern with large email maps, you could optimize by directly copying and updating:Map<EmailId, PresentationEmail?> addEmailKeywordById({ required EmailId emailId, required KeyWordIdentifier keyword, }) { final newMap = Map<EmailId, PresentationEmail?>.from(this); final email = newMap[emailId]; if (email != null) { newMap[emailId] = email.addKeyword(keyword); } return newMap; }This is optional given typical email list sizes.
lib/features/email/presentation/controller/single_email_controller.dart (1)
821-907: GuardlabelAsaction by feature availability (defensive).
Right nowlabelAswill try to open the modal even if labels are disabled (or mis-triggered via shortcut/context).case EmailActionType.labelAs: - openAddLabelToEmailDialogModal(presentationEmail); + if (!isLabelFeatureEnabled) return; + openAddLabelToEmailDialogModal(presentationEmail); break;lib/features/email/presentation/extensions/handle_label_for_email_extension.dart (1)
24-40: Nice: feature gating + DialogRouter modal usage are straightforward; add a defensive gate too.
openAddLabelToEmailDialogModalshould probably early-return when!isLabelFeatureEnabled(in case the action is invoked unexpectedly).Future<void> openAddLabelToEmailDialogModal(PresentationEmail email) async { + if (!isLabelFeatureEnabled) return; final labels = mailboxDashBoardController.labelController.labels;Also applies to: 150-167
lib/features/base/widget/popup_menu/popup_menu_item_action_widget.dart (1)
29-41: Consider allocating_itemKey/_hoverItemNotifieronly for the submenu item.
Right now everyPopupMenuItemEmailActionpays the cost, even if it never shows a submenu.void initState() { super.initState(); - if (widget.menuAction is PopupMenuItemEmailAction) { + if (widget.menuAction is PopupMenuItemEmailAction && + widget.menuAction.action == EmailActionType.labelAs) { _itemKey = GlobalKey(); _hoverItemNotifier = ValueNotifier(false); } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
assets/images/ic_thumbs_up.svgis excluded by!**/*.svg
📒 Files selected for processing (66)
core/lib/presentation/resources/image_paths.dart(1 hunks)labels/lib/converter/keyword_identifier_nullable_converter.dart(1 hunks)labels/lib/extensions/label_extension.dart(2 hunks)labels/lib/model/label.dart(2 hunks)labels/test/method/get/get_label_method_test.dart(2 hunks)lib/features/base/base_mailbox_controller.dart(2 hunks)lib/features/base/model/popup_menu_item_action.dart(5 hunks)lib/features/base/widget/popup_menu/popup_menu_action_group_widget.dart(3 hunks)lib/features/base/widget/popup_menu/popup_menu_item_action_widget.dart(6 hunks)lib/features/base/widget/popup_menu/popup_submenu_controller.dart(1 hunks)lib/features/composer/presentation/composer_view_web.dart(1 hunks)lib/features/composer/presentation/extensions/email_action_type_extension.dart(2 hunks)lib/features/email/data/datasource/email_datasource.dart(2 hunks)lib/features/email/data/datasource_impl/email_datasource_impl.dart(2 hunks)lib/features/email/data/datasource_impl/email_hive_cache_datasource_impl.dart(1 hunks)lib/features/email/data/datasource_impl/email_local_storage_datasource_impl.dart(2 hunks)lib/features/email/data/datasource_impl/email_session_storage_datasource_impl.dart(2 hunks)lib/features/email/data/network/email_api.dart(1 hunks)lib/features/email/data/repository/email_repository_impl.dart(2 hunks)lib/features/email/domain/repository/email_repository.dart(2 hunks)lib/features/email/domain/state/add_a_label_to_an_email_state.dart(1 hunks)lib/features/email/domain/usecases/add_a_label_to_an_email_interactor.dart(1 hunks)lib/features/email/presentation/bindings/email_bindings.dart(2 hunks)lib/features/email/presentation/bindings/email_interactor_bindings.dart(2 hunks)lib/features/email/presentation/controller/single_email_controller.dart(9 hunks)lib/features/email/presentation/email_view.dart(4 hunks)lib/features/email/presentation/extensions/email_loaded_extension.dart(1 hunks)lib/features/email/presentation/extensions/handle_label_for_email_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/email/presentation/model/popup_menu_item_email_action.dart(2 hunks)lib/features/email/presentation/utils/email_action_reactor/email_action_reactor.dart(8 hunks)lib/features/labels/domain/exceptions/label_exceptions.dart(1 hunks)lib/features/labels/presentation/label_controller.dart(2 hunks)lib/features/labels/presentation/widgets/add_label_to_email_modal.dart(1 hunks)lib/features/labels/presentation/widgets/label_item_context_menu.dart(1 hunks)lib/features/labels/presentation/widgets/label_list_context_menu.dart(1 hunks)lib/features/mailbox/presentation/mailbox_controller.dart(1 hunks)lib/features/mailbox/presentation/model/popup_menu_item_mailbox_action.dart(1 hunks)lib/features/mailbox_dashboard/presentation/controller/advanced_filter_controller.dart(1 hunks)lib/features/mailbox_dashboard/presentation/controller/mailbox_dashboard_controller.dart(5 hunks)lib/features/mailbox_dashboard/presentation/extensions/handle_create_new_rule_filter.dart(1 hunks)lib/features/mailbox_dashboard/presentation/extensions/update_current_emails_flags_extension.dart(2 hunks)lib/features/mailbox_dashboard/presentation/extensions/verify_display_overlay_view_on_iframe_extension.dart(1 hunks)lib/features/mailbox_dashboard/presentation/model/profile_setting/popup_menu_item_profile_setting_type_action.dart(1 hunks)lib/features/manage_account/presentation/email_rules/email_rules_controller.dart(2 hunks)lib/features/manage_account/presentation/identities/identities_controller.dart(3 hunks)lib/features/manage_account/presentation/identities/widgets/signature_builder.dart(1 hunks)lib/features/rules_filter_creator/presentation/rules_filter_creator_controller.dart(1 hunks)lib/features/search/email/presentation/search_email_controller.dart(1 hunks)lib/features/search/mailbox/presentation/search_mailbox_controller.dart(1 hunks)lib/features/thread/data/extensions/map_keywords_extension.dart(1 hunks)lib/features/thread/domain/extensions/presentation_email_map_extension.dart(1 hunks)lib/features/thread/presentation/mixin/email_action_controller.dart(1 hunks)lib/features/thread_detail/domain/extensions/list_email_in_thread_detail_info_extension.dart(2 hunks)lib/features/thread_detail/presentation/extension/on_thread_detail_action_click.dart(1 hunks)lib/l10n/intl_messages.arb(2 hunks)lib/main/localizations/app_localizations.dart(1 hunks)lib/main/routes/dialog_router.dart(5 hunks)lib/main/utils/toast_manager.dart(4 hunks)model/lib/email/email_action_type.dart(1 hunks)model/lib/extensions/keyword_identifier_extension.dart(1 hunks)test/features/composer/presentation/composer_controller_test.dart(0 hunks)test/features/email/presentation/controller/single_email_controller_test.dart(4 hunks)test/features/thread/presentation/extensions/map_keywords_extension_test.dart(1 hunks)test/model/lib/extensions/get_label_list_in_keyword_email_test.dart(5 hunks)
💤 Files with no reviewable changes (1)
- test/features/composer/presentation/composer_controller_test.dart
🧰 Additional context used
🧠 Learnings (4)
📚 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/mailbox_dashboard/presentation/model/profile_setting/popup_menu_item_profile_setting_type_action.dartlib/features/email/data/datasource/email_datasource.dartlib/features/composer/presentation/extensions/email_action_type_extension.dartlib/features/email/data/network/email_api.dartlabels/lib/extensions/label_extension.dartmodel/lib/email/email_action_type.dartlib/features/labels/domain/exceptions/label_exceptions.dartlib/features/email/data/datasource_impl/email_session_storage_datasource_impl.dartcore/lib/presentation/resources/image_paths.dartlib/features/email/presentation/extensions/email_loaded_extension.dartlib/features/email/presentation/bindings/email_bindings.dartlib/features/email/data/datasource_impl/email_local_storage_datasource_impl.dartlib/features/rules_filter_creator/presentation/rules_filter_creator_controller.dartlabels/lib/converter/keyword_identifier_nullable_converter.dartlib/features/mailbox/presentation/mailbox_controller.dartlib/features/base/widget/popup_menu/popup_menu_action_group_widget.darttest/features/thread/presentation/extensions/map_keywords_extension_test.dartlib/features/base/widget/popup_menu/popup_submenu_controller.dartlib/features/labels/presentation/widgets/label_list_context_menu.dartlib/main/localizations/app_localizations.dartlib/features/mailbox/presentation/model/popup_menu_item_mailbox_action.dartlib/features/thread/data/extensions/map_keywords_extension.dartlib/features/search/mailbox/presentation/search_mailbox_controller.dartlib/features/email/data/repository/email_repository_impl.dartlib/features/manage_account/presentation/email_rules/email_rules_controller.dartlib/features/mailbox_dashboard/presentation/extensions/update_current_emails_flags_extension.dartlib/features/labels/presentation/widgets/add_label_to_email_modal.dartlib/features/email/data/datasource_impl/email_datasource_impl.dartlib/features/email/data/datasource_impl/email_hive_cache_datasource_impl.dartlib/features/thread_detail/domain/extensions/list_email_in_thread_detail_info_extension.dartlib/features/mailbox_dashboard/presentation/controller/mailbox_dashboard_controller.dartlib/features/email/domain/state/add_a_label_to_an_email_state.dartlib/features/composer/presentation/composer_view_web.dartlib/features/email/domain/usecases/add_a_label_to_an_email_interactor.dartlib/features/thread_detail/presentation/extension/on_thread_detail_action_click.dartlib/features/base/base_mailbox_controller.dartlib/features/email/domain/repository/email_repository.dartlabels/test/method/get/get_label_method_test.dartlib/features/labels/presentation/widgets/label_item_context_menu.dartlib/features/email/presentation/model/popup_menu_item_email_action.dartlib/features/mailbox_dashboard/presentation/extensions/verify_display_overlay_view_on_iframe_extension.dartmodel/lib/extensions/keyword_identifier_extension.dartlib/features/thread/presentation/mixin/email_action_controller.darttest/model/lib/extensions/get_label_list_in_keyword_email_test.dartlib/features/labels/presentation/label_controller.dartlib/features/thread/domain/extensions/presentation_email_map_extension.dartlib/features/email/presentation/bindings/email_interactor_bindings.dartlib/features/search/email/presentation/search_email_controller.dartlib/features/manage_account/presentation/identities/widgets/signature_builder.dartlib/features/email/presentation/utils/email_action_reactor/email_action_reactor.dartlib/features/base/widget/popup_menu/popup_menu_item_action_widget.darttest/features/email/presentation/controller/single_email_controller_test.dartlib/features/mailbox_dashboard/presentation/extensions/handle_create_new_rule_filter.dartlib/features/base/model/popup_menu_item_action.dartlib/features/email/presentation/model/email_loaded.dartlabels/lib/model/label.dartlib/features/email/presentation/extensions/handle_label_for_email_extension.dartlib/features/manage_account/presentation/identities/identities_controller.dartlib/features/email/presentation/controller/single_email_controller.dartlib/features/email/presentation/extensions/presentation_email_extension.dartlib/main/routes/dialog_router.dartlib/features/mailbox_dashboard/presentation/controller/advanced_filter_controller.dartlib/main/utils/toast_manager.dartlib/features/email/presentation/email_view.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/mailbox_dashboard/presentation/model/profile_setting/popup_menu_item_profile_setting_type_action.dartlib/features/email/data/datasource/email_datasource.dartlib/features/composer/presentation/extensions/email_action_type_extension.dartlib/features/email/data/network/email_api.dartlabels/lib/extensions/label_extension.dartmodel/lib/email/email_action_type.dartlib/features/labels/domain/exceptions/label_exceptions.dartlib/features/email/data/datasource_impl/email_session_storage_datasource_impl.dartcore/lib/presentation/resources/image_paths.dartlib/features/email/presentation/extensions/email_loaded_extension.dartlib/features/email/presentation/bindings/email_bindings.dartlib/features/email/data/datasource_impl/email_local_storage_datasource_impl.dartlib/features/rules_filter_creator/presentation/rules_filter_creator_controller.dartlabels/lib/converter/keyword_identifier_nullable_converter.dartlib/features/mailbox/presentation/mailbox_controller.dartlib/features/base/widget/popup_menu/popup_menu_action_group_widget.darttest/features/thread/presentation/extensions/map_keywords_extension_test.dartlib/features/base/widget/popup_menu/popup_submenu_controller.dartlib/features/labels/presentation/widgets/label_list_context_menu.dartlib/main/localizations/app_localizations.dartlib/features/mailbox/presentation/model/popup_menu_item_mailbox_action.dartlib/features/thread/data/extensions/map_keywords_extension.dartlib/features/search/mailbox/presentation/search_mailbox_controller.dartlib/features/email/data/repository/email_repository_impl.dartlib/features/manage_account/presentation/email_rules/email_rules_controller.dartlib/features/mailbox_dashboard/presentation/extensions/update_current_emails_flags_extension.dartlib/features/labels/presentation/widgets/add_label_to_email_modal.dartlib/features/email/data/datasource_impl/email_datasource_impl.dartlib/features/email/data/datasource_impl/email_hive_cache_datasource_impl.dartlib/features/thread_detail/domain/extensions/list_email_in_thread_detail_info_extension.dartlib/features/mailbox_dashboard/presentation/controller/mailbox_dashboard_controller.dartlib/features/email/domain/state/add_a_label_to_an_email_state.dartlib/features/composer/presentation/composer_view_web.dartlib/features/email/domain/usecases/add_a_label_to_an_email_interactor.dartlib/features/thread_detail/presentation/extension/on_thread_detail_action_click.dartlib/features/base/base_mailbox_controller.dartlib/features/email/domain/repository/email_repository.dartlabels/test/method/get/get_label_method_test.dartlib/features/labels/presentation/widgets/label_item_context_menu.dartlib/features/email/presentation/model/popup_menu_item_email_action.dartlib/features/mailbox_dashboard/presentation/extensions/verify_display_overlay_view_on_iframe_extension.dartmodel/lib/extensions/keyword_identifier_extension.dartlib/features/thread/presentation/mixin/email_action_controller.darttest/model/lib/extensions/get_label_list_in_keyword_email_test.dartlib/features/labels/presentation/label_controller.dartlib/features/thread/domain/extensions/presentation_email_map_extension.dartlib/features/email/presentation/bindings/email_interactor_bindings.dartlib/features/search/email/presentation/search_email_controller.dartlib/features/manage_account/presentation/identities/widgets/signature_builder.dartlib/features/email/presentation/utils/email_action_reactor/email_action_reactor.dartlib/features/base/widget/popup_menu/popup_menu_item_action_widget.darttest/features/email/presentation/controller/single_email_controller_test.dartlib/features/mailbox_dashboard/presentation/extensions/handle_create_new_rule_filter.dartlib/features/base/model/popup_menu_item_action.dartlib/features/email/presentation/model/email_loaded.dartlabels/lib/model/label.dartlib/features/email/presentation/extensions/handle_label_for_email_extension.dartlib/features/manage_account/presentation/identities/identities_controller.dartlib/features/email/presentation/controller/single_email_controller.dartlib/features/email/presentation/extensions/presentation_email_extension.dartlib/main/routes/dialog_router.dartlib/features/mailbox_dashboard/presentation/controller/advanced_filter_controller.dartlib/main/utils/toast_manager.dartlib/features/email/presentation/email_view.dart
📚 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/composer/presentation/extensions/email_action_type_extension.dartlib/features/email/presentation/extensions/email_loaded_extension.dartlib/features/email/presentation/bindings/email_bindings.dartlib/features/mailbox/presentation/mailbox_controller.dartlib/features/labels/presentation/widgets/label_list_context_menu.dartlib/features/mailbox/presentation/model/popup_menu_item_mailbox_action.dartlib/features/search/mailbox/presentation/search_mailbox_controller.dartlib/features/manage_account/presentation/email_rules/email_rules_controller.dartlib/features/mailbox_dashboard/presentation/extensions/update_current_emails_flags_extension.dartlib/features/labels/presentation/widgets/add_label_to_email_modal.dartlib/features/thread_detail/domain/extensions/list_email_in_thread_detail_info_extension.dartlib/features/mailbox_dashboard/presentation/controller/mailbox_dashboard_controller.dartlib/features/thread_detail/presentation/extension/on_thread_detail_action_click.dartlib/features/email/presentation/model/popup_menu_item_email_action.dartlib/features/mailbox_dashboard/presentation/extensions/verify_display_overlay_view_on_iframe_extension.dartlib/features/thread/presentation/mixin/email_action_controller.dartlib/features/thread/domain/extensions/presentation_email_map_extension.dartlib/features/email/presentation/bindings/email_interactor_bindings.dartlib/features/search/email/presentation/search_email_controller.dartlib/features/email/presentation/utils/email_action_reactor/email_action_reactor.darttest/features/email/presentation/controller/single_email_controller_test.dartlib/features/mailbox_dashboard/presentation/extensions/handle_create_new_rule_filter.dartlib/features/email/presentation/model/email_loaded.dartlib/features/email/presentation/extensions/handle_label_for_email_extension.dartlib/features/email/presentation/controller/single_email_controller.dartlib/features/email/presentation/extensions/presentation_email_extension.dartlib/features/mailbox_dashboard/presentation/controller/advanced_filter_controller.dartlib/main/utils/toast_manager.dartlib/features/email/presentation/email_view.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/email/presentation/bindings/email_bindings.dartlib/features/email/presentation/bindings/email_interactor_bindings.darttest/features/email/presentation/controller/single_email_controller_test.dartlib/features/email/presentation/controller/single_email_controller.dart
🪛 RuboCop (1.81.7)
lib/l10n/intl_messages.arb
[convention] 5249-5249: Put a comma after the last item of a multiline array.
(Style/TrailingCommaInArrayLiteral)
[convention] 5259-5259: Put a comma after the last item of a multiline array.
(Style/TrailingCommaInArrayLiteral)
🔇 Additional comments (54)
model/lib/extensions/keyword_identifier_extension.dart (1)
26-32: Action helpers look consistent; confirm “remove label” is handled elsewhereThe
=> _boolPatch(true)helpers (answered/forwarded/unsubscribe/label) look consistent. For labeling specifically: if the UI supports unlabeling, make sure there’s a corresponding path generator that unsets the keyword (likely via_boolPatch(null)) and that callers don’t misusefalsefor removal.lib/features/mailbox_dashboard/presentation/model/profile_setting/popup_menu_item_profile_setting_type_action.dart (1)
29-33:hoverIconoverride is consistent with the new hover-icon pattern.
Only note: double-check this icon choice matches profile-setting UX (a generic thumbs-up hover icon can be surprising outside “label” actions).lib/features/mailbox_dashboard/presentation/extensions/handle_create_new_rule_filter.dart (1)
46-54: DialogRouter instance call looks correct; please smoke-test web route stacking.
The migration toDialogRouter().pushGeneralDialog(...)is fine, but it’s worth verifying the dialog still anchors to the expected navigator/overlay in web builds.lib/features/email/domain/state/add_a_label_to_an_email_state.dart (1)
6-29: State objects look coherent and sufficiently descriptive for UI + error messaging.lib/features/thread_detail/presentation/extension/on_thread_detail_action_click.dart (1)
213-219: DialogRouter instance migration is consistent; keep an eye on web return types.
ThePresentationMailboxtype-guard is good—please confirm the web dialog path still returns that object (not a serialized/map payload) after the router refactor.core/lib/presentation/resources/image_paths.dart (1)
266-266: No action needed. The assetic_thumbs_up.svgis properly included in the asset bundle via theassets: - assets/images/configuration in pubspec.yaml, and the file exists at the expected location.lib/features/mailbox_dashboard/presentation/extensions/verify_display_overlay_view_on_iframe_extension.dart (1)
6-13: The propertylabelController.isCreateNewLabelModalVisibledoes not exist in the codebase and was never part of the overlay contract.The create-label modal is managed through
DialogRouter().openDialogModal(), which tracks modal visibility viaDialogRouter.isDialogOpened. However, theisDisplayedOverlayViewOnIFrameextension method does not include checks forDialogRouter().isDialogOpened. This is consistent with the codebase architecture: views likeemail_view.dartandcomposer_view_web.dartsupplement the extension method with additional dialog checks (DialogRouter().isDialogOpened,MessageDialogActionManager().isDialogOpened, etc.) rather than including them in the extension itself. If the create-label modal should block iframe interactions in views using this extension, those views are responsible for addingDialogRouter().isDialogOpenedto their iframe overlay conditions.Likely an incorrect or invalid review comment.
lib/features/mailbox/presentation/mailbox_controller.dart (1)
874-881: DialogRouter instance call: ensure behavior parity with previous static usage on web.
IfDialogRouter()is a factory/singleton, this is likely equivalent; just make sure no per-instance state was relied upon (e.g., navigator keys, stacked dialogs).labels/test/method/get/get_label_method_test.dart (1)
6-7: Label keyword migration in tests looks consistent.
SwitchingLabel.keywordtoKeyWordIdentifier(...)aligns expectations with the new model type.Also applies to: 43-54
lib/features/email/domain/repository/email_repository.dart (1)
14-15:addLabelToEmailsignature: confirm why it doesn’t requireSession.
Most repository network operations here acceptSession; if labeling is also a network call, double-check this won’t constrain multi-session/multi-account flows or force hidden globals in lower layers.Also applies to: 164-169
lib/features/manage_account/presentation/identities/identities_controller.dart (1)
232-235: DialogRouter instance call migration is consistent.
Looks like a mechanical refactor; just ensure DialogRouter’s factory/instance behavior matches the previous static usage across web dialogs.Also applies to: 384-387, 482-485
lib/features/email/presentation/bindings/email_bindings.dart (1)
1-4: DI wiring for label interactor intoSingleEmailControllerlooks correct.
SinceEmailInteractorBindings().dependencies()is called beforeGet.put(SingleEmailController(...)), the newGet.find<AddALabelToAnEmailInteractor>()should be available (assuming it’s registered there as added).Also applies to: 20-31
lib/features/composer/presentation/extensions/email_action_type_extension.dart (1)
137-183:labelAssupport in icon/title mapping is straightforward.
AssumingimagePaths.icTagandappLocalizations.labelAsare present everywhere, this looks good.Also applies to: 185-232
lib/features/search/mailbox/presentation/search_mailbox_controller.dart (1)
825-828: DialogRouter instance migration looks safe here (web-only).Web branch now uses
DialogRouter().pushGeneralDialog(...); flow/typing checks remain intact.lib/features/thread/presentation/mixin/email_action_controller.dart (1)
162-165: Web destination picker now uses instance-based DialogRouter; flow unchanged.lib/features/base/base_mailbox_controller.dart (2)
391-394: DialogRouter instance migration (move mailbox) looks good.
660-666: DialogRouter instance migration (move folder content) looks good.lib/features/email/data/datasource/email_datasource.dart (1)
14-15: Confirmkeyword_identifier.dart/KeyWordIdentifieris stable in your pinnedjmap_dart_client.This is a public API surface change relying on an external package type; worth double-checking the exact type name/export in the repo’s dependency lock.
model/lib/email/email_action_type.dart (1)
2-9: No action required. ThelabelAsenum value is properly handled throughout the codebase in all appropriate contexts. All switches have default cases, and the value is explicitly managed where needed (individual email actions) while being intentionally omitted from thread-level and search-level handlers where it's not applicable.lib/features/manage_account/presentation/email_rules/email_rules_controller.dart (1)
100-102: LGTM! DialogRouter refactoring applied consistently.The migration from static to instance-based DialogRouter invocation is correct and aligns with the broader refactoring pattern across the codebase.
lib/features/email/data/datasource_impl/email_session_storage_datasource_impl.dart (1)
269-272: LGTM! Stub implementation is appropriate.The
UnimplementedErrorforaddLabelToEmailin the session storage datasource is consistent with the pattern used for other unimplemented methods in this class, which delegates operations to other datasource implementations.lib/features/email/presentation/model/email_loaded.dart (1)
20-32: LGTM! Standard copyWith implementation.The
copyWithmethod follows the typical Dart pattern for immutable data classes and will support the label-related updates mentioned in the PR summary.lib/features/mailbox_dashboard/presentation/controller/advanced_filter_controller.dart (1)
223-227: LGTM! DialogRouter refactoring applied correctly.The instance-based DialogRouter invocation is consistent with the codebase-wide migration pattern.
lib/features/search/email/presentation/search_email_controller.dart (1)
707-709: LGTM! DialogRouter refactoring is consistent.The change maintains functionality while adopting the instance-based pattern.
lib/features/rules_filter_creator/presentation/rules_filter_creator_controller.dart (1)
443-445: LGTM! DialogRouter migration complete.The instance-based invocation is correctly applied.
test/features/email/presentation/controller/single_email_controller_test.dart (1)
30-30: LGTM! Test infrastructure updated correctly.The new
AddALabelToAnEmailInteractordependency is properly mocked and wired into the test setup, supporting the label feature addition.Also applies to: 77-77, 112-112, 180-180
lib/features/email/data/datasource_impl/email_local_storage_datasource_impl.dart (2)
24-24: LGTM!The KeyWordIdentifier import is correctly added to support the new labeling functionality.
355-358: LGTM!The UnimplementedError is appropriate for the local storage datasource, as label operations require network interaction and are not persisted locally.
lib/features/email/presentation/model/popup_menu_item_email_action.dart (1)
19-19: LGTM!The submenu parameter correctly extends the constructor to support the new popup submenu functionality.
lib/features/email/data/datasource_impl/email_datasource_impl.dart (2)
25-25: LGTM!The KeyWordIdentifier import is correctly added to support the new label functionality.
557-566: LGTM!The implementation correctly follows the established pattern for delegation to the EmailAPI layer with consistent error handling via the exception thrower.
labels/lib/converter/keyword_identifier_nullable_converter.dart (1)
1-14: LGTM!The converter correctly handles bidirectional conversion between
KeyWordIdentifier?andString?for JSON serialization, with proper null handling in both directions.lib/features/email/data/repository/email_repository_impl.dart (1)
16-16: LGTM!The KeyWordIdentifier import is correctly added.
test/model/lib/extensions/get_label_list_in_keyword_email_test.dart (1)
11-96: LGTM!The test updates correctly reflect the API change from
StringtoKeyWordIdentifiertype, maintaining comprehensive test coverage while adapting to the new type-safe keyword handling.test/features/thread/presentation/extensions/map_keywords_extension_test.dart (1)
1-109: LGTM!Comprehensive test coverage for the MapKeywordsExtension methods. The tests correctly verify immutability, edge cases (empty maps, non-existing keywords), and core functionality for both
withKeyword()andwithoutKeyword()methods.lib/features/mailbox_dashboard/presentation/controller/mailbox_dashboard_controller.dart (1)
1316-1318: Dialog routing invocation update looks consistent
Swapping toDialogRouter().pushGeneralDialog(...)in the web paths keeps the existing control-flow intact and aligns with the refactor described in the PR summary.Also applies to: 2174-2177, 2197-2200, 2221-2228, 3248-3254
lib/main/utils/toast_manager.dart (1)
25-25: Label add success/failure toast wiring looks consistent with existing patterns.Also applies to: 164-208, 275-279
lib/features/email/presentation/extensions/presentation_email_extension.dart (1)
2-2: Good migration to KeyWordIdentifier-based label detection + helper methods. Consider double-checkingkeywords.withKeyword(...)/keywords.withoutKeyword(...)semantics whenkeywordsis null (and whether they mutate vs copy), since these helpers will likely become widely used.Also applies to: 160-171, 173-183
lib/features/thread_detail/domain/extensions/list_email_in_thread_detail_info_extension.dart (1)
2-3: Nice immutable-style list update helpers (single-element copyWith) for thread detail keywords.Also applies to: 13-37
lib/features/labels/presentation/widgets/label_list_context_menu.dart (1)
22-40: LGTM: selection computed frompresentationEmail.getLabelList(labelList)keeps identity consistent forcontains.lib/main/localizations/app_localizations.dart (1)
5560-5588: New localization keys are straightforward and match the new label flow messaging needs.lib/features/thread/data/extensions/map_keywords_extension.dart (2)
20-22: Mutating method silently no-ops on null receiver.The
addKeywordmethod mutatesthisin place, but if the receiver isnull, it silently does nothing. This could mask bugs where callers expect the keyword to be added but the map was never initialized.Consider either:
- Making callers responsible for null-checking before calling
- Throwing an exception if
thisis null- Documenting this behavior clearly
If this silent behavior is intentional (e.g., the caller handles the null case separately), please add a brief doc comment to clarify.
12-18: Immutable keyword helpers look good.
withKeywordandwithoutKeywordcorrectly create new maps from a nullable receiver, defaulting to an empty map when null. This is a safe and predictable pattern.lib/features/base/model/popup_menu_item_action.dart (2)
6-22: Well-structured submenu support.The addition of
submenutoPopupMenuItemActionand its propagation through all derived classes is clean. Includingsubmenuinpropsensures proper equality comparisons for state management.
50-66: Good use of mixin for hover icon properties.The
OptionalPopupHoverIconmixin follows the existing pattern ofOptionalPopupIconandOptionalPopupSelectedIcon, providing consistent default values. Mixing it intoPopupMenuItemActionRequiredIconis the right approach.lib/features/email/presentation/utils/email_action_reactor/email_action_reactor.dart (3)
568-606: Label submenu integration looks good.The
PopupSubmenuControlleris properly scoped to the popup lifecycle and cleaned up viawhenComplete. The pattern of skippinglabelAsinonActionSelectedwhile delegating to the submenu is correct.
610-627: Helper method is clean and focused.
_getEmailActionSubmenucorrectly returnsnullfor non-label actions and handles the empty labels case. The null-checklabels?.isNotEmpty == trueis idiomatic Dart.
299-303: DialogRouter refactored to instance-based invocation.The change from static
DialogRouter.pushGeneralDialogtoDialogRouter().pushGeneralDialogcorrectly implements the singleton factory pattern. The refactoring is consistently applied across the codebase—all usages of DialogRouter (29+ instances found) use the instance-based invocation patternDialogRouter(). No remaining static method calls exist. The implementation at lines 299-303 and 743-744 aligns with this consistent pattern.lib/l10n/intl_messages.arb (1)
5239-5270: Localization entries are well-structured.The new label-related localization strings follow the existing ARB file patterns correctly. Placeholders for
labelNameare properly defined with matching metadata entries.Note: The static analysis hints about trailing commas in
placeholders_orderarrays (lines 5249, 5259) are style suggestions from RuboCop (which is a Ruby linter incorrectly applied to JSON/ARB). These can be safely ignored as ARB files don't require trailing commas.lib/main/routes/dialog_router.dart (1)
19-55: Singleton refactor looks coherent (instance Rx state + factory).
The factory singleton + instance members reads cleanly and keeps call-sites simple (DialogRouter()....).lib/features/base/widget/popup_menu/popup_menu_item_action_widget.dart (1)
246-253: Notifier disposal looks correct.
Good cleanup for the added ValueNotifier.lib/features/email/presentation/email_view.dart (2)
598-615: Overlay gating viaDialogRouter()is reasonable; ensure all dialog entry points toggle the flags.
Now that overlay depends onDialogRouter().isDialogOpened, make sure any dialog that should block iframe interactions goes throughDialogRouter(or also togglesMessageDialogActionManager/ other existing flags).
25-26: AvoidpresentationEmail.id!in label callbacks (defensive null-safety).
If an email ever reaches this path without an id (draft/placeholder), this will throw.- onSelectLabelAction: (label, isSelected) => - controller.toggleLabelToEmail( - presentationEmail.id!, - label, - isSelected, - ), + onSelectLabelAction: (label, isSelected) { + final id = presentationEmail.id; + if (id == null) return; + controller.toggleLabelToEmail(id, label, isSelected); + },Also applies to: 106-116, 319-329
⛔ 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/email/presentation/controller/single_email_controller.dart (1)
38-53: The constructor change forSingleEmailControllerhas already been properly handled. TheAddALabelToAnEmailInteractorparameter is present in the constructor signature (line 191) and is correctly provided in all instantiation points:
- email_bindings.dart (line 28):
Get.find<AddALabelToAnEmailInteractor>()is passed- single_email_controller_test.dart (line 180):
addALabelToAnEmailInteractoris passedNo binding locations are missing this parameter.
Likely an incorrect or invalid review comment.
Issue
#4195
Resolved
Screen.Recording.2025-12-12.at.10.online-video-cutter.com.mp4
demo-mobile.webm
Summary by CodeRabbit
Release Notes
✏️ Tip: You can customize this high-level summary in your review settings.