[Label] Implement auto-sync of tags via WebSocket#4300
[Label] Implement auto-sync of tags via WebSocket#4300
Conversation
WalkthroughAdds session-aware label synchronization and batched label retrieval. Introduces SessionMixin (providing max-objects helpers) and BatchGetLabelProcessingMixin (batched GetLabel via JMAP). Label API, datasource, and repository APIs now surface label-change methods and return composite results containing labels plus optional State. Domain and presentation layers gain change-tracking types, a GetLabelChanges interactor, WebSocket queue handling, LabelChangeListener, and utilities to apply created/updated/destroyed label sets. Push notification mapping and related controllers are wired to emit and handle label-specific actions. Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
Important Action Needed: IP Allowlist UpdateIf your organization protects your Git platform with IP whitelisting, please add the new CodeRabbit IP address to your allowlist:
Reviews will stop working after February 8, 2026 if the new IP is not added to your allowlist. 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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
lib/features/base/mixin/mail_api_mixin.dart (1)
94-98:⚠️ Potential issue | 🟠 MajorVariable shadowing causes
mapErrorsto never accumulate errors.Line 95 declares a new
mapErrorsvariable that shadows the one declared at line 60. Line 98 then callsaddAllon this new local variable, which is immediately discarded. The outermapErrorsmap never receives the errors.🐛 Proposed fix
final listEmailIds = setEmailResponse?.updated?.keys.toEmailIds() ?? []; - final mapErrors = handleSetResponse([setEmailResponse]); + final batchErrors = handleSetResponse([setEmailResponse]); updatedEmailIds.addAll(listEmailIds); - mapErrors.addAll(mapErrors); + mapErrors.addAll(batchErrors);lib/features/push_notification/presentation/controller/push_base_controller.dart (1)
81-81:⚠️ Potential issue | 🟡 MinorCopy-paste bug: logging wrong variable.
The log statement references
listEmailActionsinstead oflistMailboxActions.🐛 Proposed fix
- log('PushBaseController::mappingTypeStateToAction():listMailboxActions: $listEmailActions'); + log('PushBaseController::mappingTypeStateToAction():listMailboxActions: $listMailboxActions');
🤖 Fix all issues with AI agents
In `@lib/features/base/mixin/session_mixin.dart`:
- Line 25: The log message incorrectly references a private method name
`_getMaxObjectsInSetMethod` but the actual method is `getMaxObjectsInSetMethod`;
update the log call that uses runtimeType to reference the correct method name
`getMaxObjectsInSetMethod` and keep the rest of the message intact (e.g.,
minOfMaxObjectsInSetMethod variable) so the log accurately reflects the method
being reported.
In `@lib/features/labels/data/repository/label_repository_impl.dart`:
- Around line 49-54: The loop condition uses newSinceState but the API call
passes the original sinceState, causing repeated fetches; update the call to
_labelDatasource.getLabelChanges(session, accountId, newSinceState) so it uses
newSinceState consistently (and ensure any subsequent assignment that updates
newSinceState remains correct inside the while loop, e.g., where newSinceState
is set from changesResponse).
In
`@lib/features/labels/presentation/extensions/handle_label_websocket_extension.dart`:
- Around line 34-41: The code currently calls
onDataFailureViewState(refreshState) even when
foldSuccessWithResult<GetLabelChangesSuccess>() returns null; change it to
null-check the result from foldSuccessWithResult: if it's non-null call await
_handleGetLabelChangesSuccess(refreshState), otherwise call
onDataFailureViewState with the actual Failure extracted from refreshViewState
by using the existing failure-fold helper (e.g. foldFailure or
foldFailureWithResult) so a non-null Failure is passed to onDataFailureViewState
instead of null.
🧹 Nitpick comments (4)
lib/features/labels/presentation/utils/label_utils.dart (1)
247-270: Consider documenting the in-place mutation behavior.The method mutates
currentLabelsdirectly rather than returning a new list. While this can be efficient, it may be unexpected for callers. Consider either:
- Adding a doc comment clarifying the mutation behavior, or
- Returning a new
List<Label>for a more predictable APIThe current implementation is functionally correct for the use case.
💡 Optional: Add documentation
+ /// Applies label changes to [currentLabels] **in place**. + /// + /// Removes labels matching [destroyedIds], [created], and [updated] IDs, + /// then appends [created] and [updated] labels. static void applyLabelChanges({ required List<Label> currentLabels, required List<Label> created, required List<Label> updated, required List<Id> destroyedIds, }) {lib/features/push_notification/presentation/listener/label_change_listener.dart (1)
14-19: Unnecessary try/catch aroundgetBinding<T>().Per project conventions,
getBinding<T>()returnsnullwhen not found rather than throwing. The try/catch is unnecessary here.♻️ Proposed fix
LabelChangeListener._internal() { - try { - _labelController = getBinding<LabelController>(); - } catch (e) { - logError( - 'LabelChangeListener::_internal(): IS NOT REGISTERED: ${e.toString()}'); - } + _labelController = getBinding<LabelController>(); + if (_labelController == null) { + logError('LabelChangeListener::_internal(): LabelController IS NOT REGISTERED'); + } }Based on learnings: "In Dart/Flutter projects using GetX, do not wrap getBinding() calls in try/catch since they return null when not found."
lib/features/base/mixin/batch_get_label_processing_mixin.dart (1)
34-35: Misleading variable name:totalEmailsshould betotalLabels.The variable refers to labels, not emails. This appears to be a copy-paste artifact.
♻️ Suggested fix
final maxObjects = getMaxObjectsInGetMethod(session, accountId); - final totalEmails = labelIds.length; - final batchSize = max(1, min(totalEmails, maxObjects)); + final totalLabels = labelIds.length; + final batchSize = max(1, min(totalLabels, maxObjects));Also update the loop on line 41:
- for (int start = 0; start < totalEmails; start += batchSize) { + for (int start = 0; start < totalLabels; start += batchSize) { int end = - (start + batchSize < totalEmails) ? start + batchSize : totalEmails; + (start + batchSize < totalLabels) ? start + batchSize : totalLabels;lib/features/labels/data/network/label_api.dart (1)
179-183: Consider extracting the magic number128as a named constant.The
maxChangesvalue of128is hardcoded. For better maintainability and consistency with other change methods, consider extracting this to a named constant.♻️ Suggested improvement
+ static const int _defaultMaxChanges = 128; + Future<ChangesLabelResponse> getLabelChanges( AccountId accountId, State sinceState, ) async { ... final changesLabelMethod = ChangesLabelMethod( accountId, sinceState, - maxChanges: UnsignedInt(128), + maxChanges: UnsignedInt(_defaultMaxChanges), );
lib/features/labels/presentation/extensions/handle_label_websocket_extension.dart
Show resolved
Hide resolved
|
This PR has been deployed to https://linagora.github.io/tmail-flutter/4300. |
1 participant
Issue
Implement auto-sync of tags via WebSocket
Resolved
Screen.Recording.2026-02-03.at.19.45.12.mov
Summary by CodeRabbit
New Features
Tests