TF-4308 Add labels for several messages

[dab246]

Issue

#4308

Resolved

Screen.Recording.2026-02-09.at.16.46.27.mov

Summary by CodeRabbit

• New Features

  • Bulk label assignment: apply multiple labels to many emails at once.
  • Label selection modal for choosing labels during batch actions.
  • Top-bar label action to trigger labeling for selected emails.

• UI/UX Improvements

  • Visual feedback for selected labels in lists.
  • Configurable close-button alignment.
  • User-facing success/failure messages and localizations for batch labeling.

[coderabbitai]

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: b7ab369f-3a1c-4011-aca5-966bb852e4de

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

• 🔍 Trigger review

Walkthrough

Adds batch label-to-email functionality across the stack: new API/data-source/repository method addListLabelToListEmail, domain states for loading/success/partial-failure/failure, an interactor AddListLabelToListEmailsInteractor, presentation mixins/widgets (ChooseLabelModal, mailbox/dashboard/controller extensions and bindings), label UI updates (LabelListItem, TopBarThreadSelection), localization entries, toast handling, and a model extension to generate batch patch objects. Several datasource implementations remain unimplemented (throw UnimplementedError) and network/data-source wiring delegates to the new EmailAPI method.

Possibly related PRs

• TF-4236 Remove a label from an email #4239: Implements batch label operations and related model/API changes (same labels-to-multiple-emails flow).
• TF-4179 Display tag list in sidebar #4181: Modifies mailbox label UI including LabelListItem and related widgets touched by this PR.
• TF-4306 Update icon for Label in left menu #4307: Changes to LabelListItem widget assets/behavior that overlap this PR’s LabelListItem API/behavior updates.

Suggested reviewers

• tddang-linagora
• hoangdat

🚥 Pre-merge checks | ✅ 3

✅ Passed checks (3 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The PR title directly and clearly reflects the main objective: adding the ability to label multiple emails at once. The changeset implements new methods, state management, and UI components across the codebase to enable batch label assignment to several messages.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch features/tf-4308-add-labels-for-several-messages

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 3

🤖 Fix all issues with AI agents

In `@lib/features/mailbox/presentation/widgets/labels/label_list_item.dart`:
- Around line 202-224: Rename the misspelled parameter/field onSelectAcion to
onSelectAction in the _SelectedIcon StatelessWidget: update the final field
declaration, the constructor parameter (and its required named parameter), and
the usage passed to TMailButtonWidget.fromIcon (onTapActionCallback) so all
references match the new onSelectAction name; ensure constructor callers (where
_SelectedIcon is instantiated) are updated to use the corrected named parameter
as well.

In `@lib/main/utils/toast_manager.dart`:
- Around line 325-327: The current toast branch treats
AddListLabelsToListEmailsHasSomeFailure the same as full
AddListLabelsToListEmailsSuccess, which shows a misleading full-success message;
in the toast assignment logic in toast_manager.dart update the conditional
handling for AddListLabelsToListEmailsHasSomeFailure so it uses a distinct
partial-failure localization key (e.g.
appLocalizations.addListLabelToListEmailPartiallyFailedMessage) or falls back to
the failure message (e.g.
appLocalizations.addListLabelToListEmailFailureMessage) instead of
appLocalizations.addListLabelToListEmailSuccessfullyMessage; adjust the branch
that checks success is AddListLabelsToListEmailsSuccess ||
AddListLabelsToListEmailsHasSomeFailure to differentiate the two cases and
ensure the appropriate message is selected.

In `@model/lib/extensions/list_email_id_extension.dart`:
- Around line 66-75: The function generateMapUpdateObjectListLabel currently
emits duplicate keys (emailId.id) for each KeyWordIdentifier so only the last
keyword survives; fix it by building a single PatchObject per email: iterate
over each emailId in this, create a combined map of keyword patch entries (use
the same key format you expect, e.g.
'${PatchObject.keywordsProperty}/${key.value}'), set each entry to remove ? null
: true, then assign result[emailId.id] = PatchObject(combinedMap) and return the
result map; update references to labelKeywords and the remove flag accordingly
so all keywords for an email are merged instead of overwritten.

🧹 Nitpick comments (4)

lib/features/email/domain/state/labels/add_list_label_to_list_email_state.dart (1)

23-30: Consider overriding props to distinguish from the parent state.

AddListLabelsToListEmailsHasSomeFailure inherits props from AddListLabelsToListEmailsSuccess, so Equatable treats instances with the same data as equal regardless of type. If any downstream state deduplication (e.g., distinct() on streams) is applied, a partial-failure event could be dropped after a full-success event with the same payload. Adding a discriminator to props would make the distinction explicit.

♻️ Proposed fix

 class AddListLabelsToListEmailsHasSomeFailure
     extends AddListLabelsToListEmailsSuccess {
   AddListLabelsToListEmailsHasSomeFailure(
     super.emailIds,
     super.labelKeywords,
     super.labelDisplays,
   );
+
+  `@override`
+  List<Object> get props => [...super.props, 'hasSomeFailure'];
 }

lib/features/mailbox/presentation/widgets/labels/label_list_item.dart (1)

104-115: Verify: extra space before AppColor.primaryMain on Line 109.

Minor formatting inconsistency — there are two spaces before the ? ternary operator on Line 109. Not a functional issue.

                      ? AppColor.primaryMain

vs the line above:

                      ? widget.imagePaths.icCheckboxSelected

lib/features/labels/presentation/widgets/choose_label_modal.dart (1)

169-180: Consider using a Set for selected labels to improve toggle performance and clarity.

Using a List<Label> and calling contains + where on every toggle is O(n). A Set<Label> (or Set keyed by label id) would be more idiomatic for a selection model. This is minor for typical label list sizes.

lib/features/labels/presentation/mixin/add_list_labels_to_list_emails_mixin.dart (1)

160-170: Misleading parameter name success for a partial-failure state.

The parameter AddListLabelsToListEmailsHasSomeFailure success reads oddly since the type represents a partial failure. Consider renaming to partialFailure or hasSomeFailure for clarity.

Suggested rename

  void _handleAddListLabelsToListEmailsHasSomeFailure(
-   AddListLabelsToListEmailsHasSomeFailure success,
+   AddListLabelsToListEmailsHasSomeFailure hasSomeFailure,
  ) {
-   currentToastManager.showMessageSuccess(success);
+   currentToastManager.showMessageSuccess(hasSomeFailure);
 
    onSyncListLabelForListEmail?.call(
-     success.emailIds,
-     success.labelKeywords,
+     hasSomeFailure.emailIds,
+     hasSomeFailure.labelKeywords,
      shouldRemove: false,
    );
  }

[github-actions]

This PR has been deployed to https://linagora.github.io/tmail-flutter/4319.

[hoangdat]

• how to display the selected tag for a bunch of emails?

[hoangdat]

• opened email was not well updated if we tag multiple emails in email list in tablet mode

[hoangdat]

• close button in the top-left not follow other dialog

[hoangdat]

should check in case no keywords exist?

[hoangdat]

why not lazyPut?

[hoangdat]

message here and message at addListLabelToListEmailHasSomeFailureMessage are the same, maybe make user confused

[chibenwa]

...

That's too much

[chibenwa]

I disagree with this change which denotes clear misdesign of the overall application

[chibenwa]

● Yes, there is a real design problem here, but the comment needs to be precise to be actionable. Here's the breakdown:
                                                                                                                                                                                                            
  ---                                                                                                                                                                                                       
  What the change does
                                                                                                                                                                                                            
  The old AddLabelToListEmailsActionMixin (26 lines) just opened a modal. The new AddListLabelsToListEmailsMixin (177 lines) absorbs the entire interactor chain: session validation, account ID checks,  
  state consumption, toast display, and UI sync callback.                                                                                                                                                   
   
  ---                                                                                                                                                                                                       
  The concrete misdesign     
                        
  1. The self-reference anti-pattern — the strongest argument
                                                                                                                                                                                                            
  The mixin declares:
  BaseController get currentController;                                                                                                                                                                     
  And MailboxDashBoardController implements it as:
  BaseController get currentController => this;                                                                                                                                                             
                                                                                                                                                                                                            
  The mixin is mixed into a BaseController, so it could call consumeState(...) directly. Instead it routes through a getter that returns this. This is circular indirection with no benefit — it just       
  obscures the fact that the mixin could use this directly if it constrained its on clause to BaseController.                                                                                               
                             
  2. The mixin requires too many abstract contracts                                                                                                                                                         
                             
  mixin AddListLabelsToListEmailsMixin on EmitStateMixin {                                                                                                                                                  
    AccountId? get currentAccountId;                                                                                                                                                                        
    Session? get currentSession;                                                                                                                                                                            
    ToastManager get currentToastManager;                                                                                                                                                                   
    BaseController get currentController;         // self-reference                                                                                                                                         
    AddListLabelToListEmailsInteractor? get addListLabelToListEmailInteractor;                                                                                                                              
    OnSyncListLabelForListEmail? get onSyncListLabelForListEmail;                                                                                                                                           
  }                          
                                                                                                                                                                                                            
  6 abstract getters to mix in one behavior. This is the mixin pattern being used as a substitute for proper dependency injection. The consuming class is forced to wire all these manually, making the     
  mixin hard to reuse and easy to misuse.
                                                                                                                                                                                                            
  3. Cross-controller delegation for UI                                                                                                                                                                     
   
  Both ThreadController and SearchEmailController call:                                                                                                                                                     
  mailboxDashBoardController.openChooseLabelModal(...)
                                                                                                                                                                                                            
  The dashboard controller should not own "open a label modal" as a method. This creates a hidden dependency where thread/search features cannot function without the dashboard controller being alive and
  injected — a coupling that will bite in tests and in future refactors.                                                                                                                                    
   
  ---                                                                                                                                                                                                       
  Is "misdesign of the overall application" too strong?
                                                                                                                                                                                                            
  Partially. The self-reference (currentController => this) and the 6-getter contract are clear local misdesigns. The cross-controller delegation is a broader symptom of the app's pattern of routing UI
  actions through MailboxDashBoardController as a god object.                                                                                                                                               
                             

For the record...

[tddang-linagora]

That's too much

Split into 2:

• TW-4308 Add labels for several messages part 1 #4430
• TW-4308 Add labels for several messages part 2 #4431

misdesign

A new approach is in #4431

[chibenwa]

Please @tddang-linagora explain this new approach and why it shall be prefered.

Which difference with this work?

Thanks by advance!

[tddang-linagora]

Please @tddang-linagora explain this new approach and why it shall be prefered.

Which difference with this work?

Thanks by advance!

Instead of creating a mixin but inject a lot of information into it making it tightly coupled, my approach is like extending the capability of an existing feature without modify the code inside it, typical open-closed. It is fully testable, minimal dependency, and single responsibility. You can add endless feature to a core functionality this way.

[chibenwa]

Instead of creating a mixin but inject a lot of information into it making it tightly coupled, my approach is like extending the capability of an existing feature without modify the code inside it, typical open-closed. It is fully testable, minimal dependency, and single responsibility. You can add endless feature to a core functionality this way.

Ok

And part one is the refactoring? Part 2 the implem?

[tddang-linagora]

Instead of creating a mixin but inject a lot of information into it making it tightly coupled, my approach is like extending the capability of an existing feature without modify the code inside it, typical open-closed. It is fully testable, minimal dependency, and single responsibility. You can add endless feature to a core functionality this way.

Ok

And part one is the refactoring? Part 2 the implem?

yes exactly.

[chibenwa]

@dab246 opinion on approach 2?

If this is concensual then I have no problem to move forward with approach 2.

[dab246]

@dab246 opinion on approach 2?

If this is concensual then I have no problem to move forward with approach 2.

IMO, approach 2 doesn't significantly reduce the amount of code and file changes for this issue. Approach 2 simply uses a controller instead of a mixin, but we still have to manage it, initialize it before use, and release it when not in use. Nevertheless, approach 2 is still worth considering, and we can continue with that approach.