TW-4308 Add labels for several messages part 2#4431
Conversation
tddang-linagora
had a problem deploying
to
PR-4431
GitHub Actions
Failure
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
| if (session == null) { | ||
| emitFailure( | ||
| controller: this, | ||
| failure: AddListLabelsToListEmailsFailure( | ||
| exception: NotFoundSessionException(), | ||
| labelDisplays: labelDisplays, | ||
| ), | ||
| ); | ||
| return; | ||
| } | ||
|
|
||
| if (accountId == null) { | ||
| emitFailure( | ||
| controller: this, | ||
| failure: AddListLabelsToListEmailsFailure( | ||
| exception: NotFoundAccountIdException(), | ||
| labelDisplays: labelDisplays, | ||
| ), | ||
| ); | ||
| return; | ||
| } | ||
|
|
||
| if (labelKeywords.isEmpty) { | ||
| emitFailure( | ||
| controller: this, | ||
| failure: AddListLabelsToListEmailsFailure( | ||
| exception: const LabelKeywordIsNull(), | ||
| labelDisplays: labelDisplays, | ||
| ), | ||
| ); | ||
| return; | ||
| } |
There was a problem hiding this comment.
Avoid boilerplate by using a validate method:
Exception? _validateParams(List<KeyWordIdentifier> keywords) {
if (_labelController.session == null) return NotFoundSessionException();
if (_labelController.accountId == null) return NotFoundAccountIdException();
if (keywords.isEmpty) return const LabelKeywordIsNull();
return null;
}| if (success is AddListLabelsToListEmailsSuccess) { | ||
| toastManager.showMessageSuccess(success); | ||
| _pendingOnSync?.call(success.emailIds, success.labelKeywords, shouldRemove: false); | ||
| } else if (success is AddListLabelsToListEmailsHasSomeFailure) { | ||
| toastManager.showMessageSuccess(success); | ||
| _pendingOnSync?.call(success.emailIds, success.labelKeywords, shouldRemove: false); | ||
| } |
There was a problem hiding this comment.
Avoid boilerplate:
final (emailIds, labelKeywords) = switch (success) {
AddListLabelsToListEmailsSuccess s => (s.emailIds, s.labelKeywords),
AddListLabelsToListEmailsHasSomeFailure s => (s.emailIds, s.labelKeywords),
_ => return,
};
toastManager.showMessageSuccess(success);
_pendingOnSync?.call(emailIds, labelKeywords, shouldRemove: false);There was a problem hiding this comment.
else if clause deleted.
| } | ||
|
|
||
| // Store callback before consumeState (stream-based async) | ||
| _pendingOnSync = onSync; |
There was a problem hiding this comment.
Be careful with race condition:
_pendingOnSync is a single field. If the user opens the modal and calls _addLabels twice in a row (double-tap, or two different email lists), the second call will overwrite the _pendingOnSync of the first call. When the stream from the first call resolves, it will call the callback from the second call, resulting in incorrect data.
There was a problem hiding this comment.
_pendingOnSync is deleted.
| } | ||
|
|
||
| // Store callback before consumeState (stream-based async) | ||
| _pendingOnSync = onSync; |
There was a problem hiding this comment.
Beware of memory leaks.
_pendingOnSync stores a closure that can capture BuildContext, widget state, or large objects from the caller. If the consumeState stream doesn't emit (timeout, uncaught error, dispose early), this closure will be held indefinitely until onClose() is called. If the controller is reused (GetX singleton scope), onClose() may never be called at the right time.
There was a problem hiding this comment.
_pendingOnSync is deleted.
![codescene-delta-analysis[bot]](https://avatars.githubusercontent.com/in/53921?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
This PR has been deployed to https://linagora.github.io/tmail-flutter/4431. |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
lib/features/labels/presentation/widgets/choose_label_modal.dart (1)
169-180: Consider using consistent equality approach for label selection.The toggle logic uses
contains(selectedLabel)at line 172 (which uses object/reference equality) but removes bylabel.idat line 174. This works correctly because the sameLabelinstances fromwidget.labelsare used throughout, but usingid-based comparison for both would be more robust:♻️ Optional: Use id-based comparison consistently
void _onToggleLabel(Label selectedLabel) { final currentLabels = _selectedLabelStateNotifier.value; - if (currentLabels.contains(selectedLabel)) { + final isAlreadySelected = currentLabels.any((label) => label.id == selectedLabel.id); + if (isAlreadySelected) { _selectedLabelStateNotifier.value = currentLabels.where((label) => label.id != selectedLabel.id).toList(); } else { _selectedLabelStateNotifier.value = [...currentLabels, selectedLabel]; } _addLabelStateNotifier.value = _selectedLabelStateNotifier.value.isNotEmpty; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/features/labels/presentation/widgets/choose_label_modal.dart` around lines 169 - 180, The toggle logic in _onToggleLabel mixes object identity (contains(selectedLabel)) with id-based removal; make both checks id-based for robustness by replacing the contains check with an id comparison (e.g., check if any label in _selectedLabelStateNotifier.value has id == selectedLabel.id) and when adding ensure you append selectedLabel only if no existing label has the same id; keep the removal using label.id filtering and update _addLabelStateNotifier.value as before.lib/features/search/email/presentation/search_email_bindings.dart (1)
27-30: Register this delegate in one shared scope.
AddListLabelToListEmailsDelegateis nowGet.put(...)here and again inlib/features/thread/presentation/thread_bindings.dart, butdisposeBindings()still only tears downSearchEmailController. Either hoist the delegate into a shared parent binding or explicitly delete it here so its lifetime is unambiguous.Also applies to: 53-55
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/features/search/email/presentation/search_email_bindings.dart` around lines 27 - 30, The AddListLabelToListEmailsDelegate is being instantiated with Get.put in multiple places (search_email_bindings and thread_bindings) while disposeBindings only tears down SearchEmailController, causing ambiguous lifetime; fix by moving the Get.put(AddListLabelToListEmailsDelegate(...)) into a shared parent binding (so a single registration serves both scopes) or, if scoped to search, call Get.delete<AddListLabelToListEmailsDelegate>() in disposeBindings of SearchEmailController’s bindings to explicitly remove it; update the binding code that references AddListLabelToListEmailsDelegate, LabelController, and AddListLabelToListEmailsInteractor accordingly so there is a single clear owner of the delegate.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lib/features/thread/presentation/thread_controller.dart`:
- Around line 1316-1318: The current handler for EmailActionType.labelAs clears
the selection by calling cancelSelectEmail() before opening the label chooser,
which loses selection if the modal is dismissed; remove the immediate
cancelSelectEmail() call and rely on openChooseLabelModal(emails) to receive
onCancel: cancelSelectEmail so cleanup happens only after the modal flow
completes (i.e., ensure EmailActionType.labelAs invokes
openChooseLabelModal(emails) without preemptively calling cancelSelectEmail).
---
Nitpick comments:
In `@lib/features/labels/presentation/widgets/choose_label_modal.dart`:
- Around line 169-180: The toggle logic in _onToggleLabel mixes object identity
(contains(selectedLabel)) with id-based removal; make both checks id-based for
robustness by replacing the contains check with an id comparison (e.g., check if
any label in _selectedLabelStateNotifier.value has id == selectedLabel.id) and
when adding ensure you append selectedLabel only if no existing label has the
same id; keep the removal using label.id filtering and update
_addLabelStateNotifier.value as before.
In `@lib/features/search/email/presentation/search_email_bindings.dart`:
- Around line 27-30: The AddListLabelToListEmailsDelegate is being instantiated
with Get.put in multiple places (search_email_bindings and thread_bindings)
while disposeBindings only tears down SearchEmailController, causing ambiguous
lifetime; fix by moving the Get.put(AddListLabelToListEmailsDelegate(...)) into
a shared parent binding (so a single registration serves both scopes) or, if
scoped to search, call Get.delete<AddListLabelToListEmailsDelegate>() in
disposeBindings of SearchEmailController’s bindings to explicitly remove it;
update the binding code that references AddListLabelToListEmailsDelegate,
LabelController, and AddListLabelToListEmailsInteractor accordingly so there is
a single clear owner of the delegate.
🪄 Autofix (Beta)
✅ Autofix completed
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5466c47e-7046-4717-aed9-4c304dd9d388
📒 Files selected for processing (21)
core/lib/presentation/views/button/default_close_button_widget.dartlib/features/email/domain/state/labels/add_list_label_to_list_email_state.dartlib/features/email/domain/usecases/labels/add_list_label_to_list_emails_interactor.dartlib/features/labels/domain/model/add_list_labels_to_list_emails_params.dartlib/features/labels/presentation/delegates/add_list_label_to_list_emails_delegate.dartlib/features/labels/presentation/widgets/choose_label_modal.dartlib/features/mailbox/presentation/widgets/labels/label_list_item.dartlib/features/mailbox_dashboard/presentation/bindings/email_action_interactor_bindings.dartlib/features/mailbox_dashboard/presentation/extensions/labels/handle_logic_label_extension.dartlib/features/mailbox_dashboard/presentation/extensions/update_current_emails_flags_extension.dartlib/features/mailbox_dashboard/presentation/mailbox_dashboard_view_web.dartlib/features/mailbox_dashboard/presentation/widgets/top_bar_thread_selection.dartlib/features/search/email/presentation/extension/handle_press_email_selection_action.dartlib/features/search/email/presentation/search_email_bindings.dartlib/features/search/email/presentation/search_email_controller.dartlib/features/search/email/presentation/search_email_view.dartlib/features/thread/presentation/extensions/handle_press_email_selection_action.dartlib/features/thread/presentation/thread_bindings.dartlib/features/thread/presentation/thread_controller.dartlib/features/thread/presentation/thread_view.dartlib/features/thread_detail/presentation/extension/labels/add_label_to_thread_extension.dart
| EmailActionType.labelAs: (emails) { | ||
| cancelSelectEmail(); | ||
| openChooseLabelModal(emails); |
There was a problem hiding this comment.
Keep the selection until the label modal finishes.
Clearing selection before opening the chooser means a dismissed modal still drops the user's current selection. openChooseLabelModal() already passes onCancel: cancelSelectEmail, so cleanup can happen after the flow completes.
♻️ Suggested change
EmailActionType.labelAs: (emails) {
- cancelSelectEmail();
openChooseLabelModal(emails);
},🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/features/thread/presentation/thread_controller.dart` around lines 1316 -
1318, The current handler for EmailActionType.labelAs clears the selection by
calling cancelSelectEmail() before opening the label chooser, which loses
selection if the modal is dismissed; remove the immediate cancelSelectEmail()
call and rely on openChooseLabelModal(emails) to receive onCancel:
cancelSelectEmail so cleanup happens only after the modal flow completes (i.e.,
ensure EmailActionType.labelAs invokes openChooseLabelModal(emails) without
preemptively calling cancelSelectEmail).
|
Note Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it. Fixes Applied SuccessfullyFixed 1 file(s) based on 1 unresolved review comment. Files modified:
Commit: The changes have been pushed to the Time taken: |
Fixed 1 file(s) based on 1 unresolved review comment. Co-authored-by: CodeRabbit <noreply@coderabbit.ai>
There was a problem hiding this comment.
(1 files improve in Code Health)
Gates Passed
See analysis details in CodeScene
View Improvements
| File | Code Health Impact | Categories Improved |
|---|---|---|
| thread_controller.dart | 6.90 → 7.17 | Complex Method |
Absence of Expected Change Pattern
- tmail-flutter/lib/features/mailbox/presentation/widgets/labels/label_list_item.dart is usually changed with: tmail-flutter/lib/features/mailbox/presentation/widgets/labels/label_list_view.dart
Quality Gate Profile: The Bare Minimum
Install CodeScene MCP: safeguard and uplift AI-generated code. Catch issues early with our IDE extension and CLI tool.
2 participants
Issue
Demo
Screen.Recording.2026-02-09.at.16.46.27.mov
Summary by CodeRabbit