Skip to content

TF-4193 [Label] Config Label in Setting#4194

Merged
hoangdat merged 5 commits intofeatures/labelfrom
feature/tf-4193-setup-label-in-setting
Dec 10, 2025
Merged

TF-4193 [Label] Config Label in Setting#4194
hoangdat merged 5 commits intofeatures/labelfrom
feature/tf-4193-setup-label-in-setting

Conversation

@dab246
Copy link
Member

@dab246 dab246 commented Dec 9, 2025

Issue

#4193

Resolved

  • Web:
demo-web.mov
  • Mobile:
demo-mobile.webm

Summary by CodeRabbit

  • New Features

    • Added label visibility preference setting enabling users to display labels directly in email lists for easier categorization.
    • Labels now appear conditionally based on user preference and account capability.
    • Added settings UI with toggle control and explanatory text for label visibility management.
    • Added success notification when label visibility is enabled.
  • Localization

    • Added new UI text strings for label visibility settings and feature availability messaging.

✏️ Tip: You can customize this high-level summary in your review settings.

@dab246
Copy link
Member Author

dab246 commented Dec 9, 2025

@coderabbitai please review this

@coderabbitai
Copy link

coderabbitai bot commented Dec 9, 2025

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.

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

Walkthrough

This PR introduces a label visibility feature that gates label rendering behind both capability checks and user settings. It adds infrastructure for managing label visibility state through SharedPreferences, creates new state management classes and interactors, integrates label setting checks into email views and mailbox components, and extends the settings UI to allow users to toggle label visibility.

Changes

Cohort / File(s) Summary
Label Settings Infrastructure
lib/features/manage_account/data/local/setting_cache_manager.dart, lib/features/manage_account/data/local/preferences_setting_manager.dart, lib/features/manage_account/domain/model/preferences/label_config.dart, lib/features/manage_account/domain/repository/manage_account_repository.dart, lib/features/manage_account/data/datasource/manage_account_datasource.dart, lib/features/manage_account/data/datasource_impl/manage_account_datasource_impl.dart, lib/features/manage_account/data/repository/manage_account_repository_impl.dart
New SettingCacheManager class for label visibility persistence; LabelConfig model with JSON serialization; new methods in datasource/repository tiers for getLabelVisibility, saveLabelVisibility, and getLabelSettingState; extended PreferencesSettingManager with LabelConfig support.
Label Settings State & Interactors
lib/features/manage_account/domain/state/get_label_setting_state.dart, lib/features/manage_account/domain/state/get_label_visibility_state.dart, lib/features/manage_account/domain/state/save_label_visibility_state.dart, lib/features/manage_account/domain/usecases/get_label_setting_state_interactor.dart, lib/features/manage_account/domain/usecases/get_label_visibility_interactor.dart, lib/features/manage_account/domain/usecases/save_label_visibility_interactor.dart
New state classes for label setting/visibility operations (loading, success, failure states); interactors for fetching and saving label visibility with stream-based execution.
Label Settings Presentation Setup
lib/features/manage_account/presentation/bindings/setting_interactor_bindings.dart, lib/features/manage_account/presentation/bindings/preferences_interactors_bindings.dart, lib/main/bindings/local/local_bindings.dart
Dependency injection configuration for SettingCacheManager and new interactors; wired into ManageAccountDataSourceImpl.
Label Settings Management
lib/features/manage_account/presentation/manage_account_dashboard_controller.dart, lib/features/manage_account/presentation/extensions/handle_setup_label_visibility_in_setting_extension.dart, lib/features/labels/presentation/label_controller.dart
ManageAccountDashboardController extended with isLabelVisibility state and label interactor fields; new extension HandleSetupLabelVisibilityInSettingExtension for managing visibility setup and enable/disable actions; LabelController enhanced with GetLabelSettingState integration and isLabelSettingEnabled flag.
Settings UI Components
lib/features/manage_account/presentation/menu/settings/setting_app_bar.dart, lib/features/manage_account/presentation/menu/settings/setting_first_level_app_bar.dart, lib/features/manage_account/presentation/widgets/setting_header_widget.dart
New onMultiClickAction callback parameter added to SettingAppBar and SettingFirstLevelAppBar; MultiClickWidget conditional wrapping added to header and first-level app bar titles.
Preferences UI Integration
lib/features/manage_account/presentation/model/preferences_option_type.dart, lib/features/manage_account/presentation/preferences/preferences_controller.dart, lib/features/manage_account/presentation/preferences/preferences_view.dart, lib/features/manage_account/presentation/preferences/bindings/preferences_bindings.dart
New PreferencesOptionType.label enum variant with localization integration; preferences view and controller updated to handle label visibility toggling and dynamic option rendering.
Label Feature Gating in Email Views
lib/features/email/presentation/email_view.dart, lib/features/search/email/presentation/search_email_view.dart, lib/features/thread/presentation/thread_view.dart, lib/features/thread_detail/presentation/thread_detail_view.dart, lib/features/thread_detail/presentation/extension/get_thread_details_email_views.dart
Conditional label retrieval gated by isLabelCapabilitySupported and isLabelSettingEnabled checks; email labels passed as nullable values to email tile builders; getThreadDetailEmailViews extended with optional labels parameter.
Mailbox & Dashboard Integration
lib/features/mailbox/presentation/base_mailbox_view.dart, lib/features/mailbox_dashboard/presentation/controller/mailbox_dashboard_controller.dart
Conditional rendering of labels UI in buildLabelsBar and buildLabelsList requiring both capability and setting flags; dashboard controller updated to call checkLabelSettingState with null-guarded paths.
Localization & Dependencies
lib/l10n/intl_messages.arb, lib/main/localizations/app_localizations.dart, lib/features/manage_account/domain/model/preferences/preferences_setting.dart
New localization keys for label visibility and feature availability messages; AppLocalizations extended with corresponding getters; PreferencesSetting extended with labelConfig getter.
Tests
test/features/mailbox_dashboard/presentation/view/mailbox_dashboard_view_widget_test.dart
Test setup updated with isLabelSettingEnabled stub on labelController.

Sequence Diagram

sequenceDiagram
    participant UI as Settings UI
    participant Controller as ManageAccountDashboardController
    participant LabelCtrl as LabelController
    participant Interactor as GetLabelSettingStateInteractor
    participant Repo as ManageAccountRepository
    participant DS as DataSource<br/>(SettingCacheManager)

    UI->>Controller: setUpLabelVisibility()
    activate Controller
    Controller->>Interactor: execute(accountId)
    activate Interactor
    Interactor->>Repo: getLabelSettingState()
    activate Repo
    Repo->>DS: getLabelSettingState()
    activate DS
    DS-->>Repo: isEnabled: bool
    deactivate DS
    Repo-->>Interactor: isEnabled
    deactivate Repo
    Interactor-->>Controller: GetLabelSettingStateSuccess(isEnabled, accountId)
    deactivate Interactor
    
    Controller->>LabelCtrl: isLabelSettingEnabled.value = isEnabled
    Controller->>LabelCtrl: checkLabelSettingState(accountId)
    activate LabelCtrl
    Note over LabelCtrl: Fetch & cache labels
    LabelCtrl-->>Controller: (state updated)
    deactivate LabelCtrl
    
    Controller->>UI: Update isLabelSettingEnabled flag
    deactivate Controller
    
    Note over UI: Conditionally render<br/>labels in views based on<br/>isLabelCapabilitySupported &&<br/>isLabelSettingEnabled
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

  • Multiple new state management classes in domain layer (3 state files, 3 interactors) introduce observable state transitions that need validation for correctness and error handling paths
  • Data persistence layer adds new SettingCacheManager and extends PreferencesSettingManager with LabelConfig support; requires careful review of null handling and JSON serialization
  • Heterogeneous UI integration across 5+ email/thread views with conditional label rendering logic; each requires verification of null-safety and proper gating conditions
  • Circular dependency concern between LabelController and ManageAccountDashboardController for checkLabelSettingState/isLabelSettingEnabled state synchronization should be validated
  • Settings UI callback wiring for onMultiClickAction requires tracing from SettingsView → SettingAppBar → SettingFirstLevelAppBar → SettingHeaderWidget with proper event propagation

Possibly related PRs

  • PR #4181: Modifies label controller and mailbox views; overlaps in label feature integration and label controller state management.

Suggested reviewers

  • tddang-linagora
  • hoangdat

Poem

🐰 Hop, hop! Labels now dance with a setting,
No longer unconditional, users are getting
Control of their sight—toggle visibility on, off!
The feature persists with a SharedPreferences scoff. ✨📧

Pre-merge checks and finishing touches

✅ 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 clearly and specifically describes the main change: adding label configuration functionality to the settings screen, directly aligned with the extensive changes across label-related features.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

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

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

@coderabbitai
Copy link

coderabbitai bot commented Dec 9, 2025

@dab246: I'll review the changes in this PR right away.

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 3

🧹 Nitpick comments (19)
test/features/mailbox_dashboard/presentation/view/mailbox_dashboard_view_widget_test.dart (1)

451-455: Label setting stubs keep existing tests stable; consider centralizing setup

Stubbing labelController.isLabelSettingEnabled and labels to RxBool(false)/RxList([]) in each test correctly neutralizes the new label UI dependency so these thread‑behavior tests remain focused and stable. To reduce repetition and ease future changes, you could move this common stubbing into a small helper used by all affected tests.

Also applies to: 529-533, 594-598, 656-660, 714-718

lib/features/mailbox_dashboard/presentation/controller/mailbox_dashboard_controller.dart (1)

883-885: Label setting refresh hooks are correctly guarded

Calling labelController.checkLabelSettingState(...) only when isLabelCapabilitySupported is true and accountId is non‑null is a clean way to refresh label visibility both after initial session setup and when returning from settings/vacation flows. The three call sites are consistent and null‑safe. If this pattern expands further, you might consider a tiny helper like refreshLabelSettingStateIfSupported() to avoid duplication, but it’s fine as‑is.

Also applies to: 2056-2063, 2135-2142

lib/features/search/email/presentation/search_email_view.dart (1)

15-15: Search email tiles now respect label capability/setting without side effects

The per‑tile logic that derives emailLabels only when both capability and label setting are enabled is correct and matches the pattern used in other views. This keeps the search UI consistent with global label visibility while keeping the tiles self‑contained. If you find yourself repeating this block further, you could extract a small helper (e.g. on the controller) to resolve labels for a PresentationEmail, but that’s optional.

Also applies to: 731-759

lib/features/thread/presentation/thread_view.dart (1)

8-8: Thread list tiles correctly adopt gated label rendering

The new logic in _buildEmailItemNotDraggable that conditionally computes emailLabels based on isLabelCapabilitySupported and labelController.isLabelSettingEnabled.isTrue is consistent with other views and keeps label usage safely behind the new setting. The draggable feedback still omits labels, which is consistent with previous behavior; you might optionally extend the same label handling there later if you want identical visuals while dragging.

Also applies to: 597-627

lib/features/manage_account/presentation/preferences/preferences_controller.dart (1)

10-10: Label preference integration and controller source switch look consistent

The new PreferencesOptionType.label branch in _updateLocalPreferencesSetting reuses the existing pattern of constructing a config (here LabelConfig) with isEnabled: !isEnabled, matching how the other local options are toggled. Switching from the old private _manageAccountDashBoardController to accountDashboardController for accountId/sessionCurrent also preserves the previous behavior for server‑side updates, with the same null‑checks before invoking _updateServerSettingInteractor. If you want to make this more testable in the future, you could inject ManageAccountDashBoardController through the constructor instead of Get.find, but the current approach is consistent with the rest of the codebase.

Also applies to: 51-52, 121-129, 142-164, 186-203

lib/features/manage_account/domain/usecases/save_label_visibility_interactor.dart (1)

1-1: Remove unnecessary import.

The dart:core library is imported automatically by Dart.

-import 'dart:core';
-
 import 'package:core/presentation/state/failure.dart';
lib/features/manage_account/domain/usecases/get_label_setting_state_interactor.dart (1)

1-1: Remove unnecessary import.

The dart:core library is imported automatically by Dart.

-import 'dart:core';
-
 import 'package:core/presentation/state/failure.dart';
lib/features/manage_account/domain/usecases/get_label_visibility_interactor.dart (1)

1-1: Remove unnecessary import.

The dart:core library is imported automatically by Dart.

-import 'dart:core';
-
 import 'package:core/presentation/state/failure.dart';
lib/features/manage_account/data/local/setting_cache_manager.dart (1)

10-13: Consider removing reload() for better performance.

Calling reload() before every read forces a synchronous read from persistent storage. If getLabelVisibility() is called frequently (e.g., when rendering email lists or mailboxes), this could introduce unnecessary I/O overhead.

SharedPreferences maintains an in-memory cache that is automatically updated after writes. Consider relying on the in-memory cache and only calling reload() when you specifically need to refresh data that may have been modified by another isolate or process.

 Future<bool> getLabelVisibility() async {
-  await _sharedPreferences.reload();
   return _sharedPreferences.getBool(_labelVisibilitySettingKey) ?? false;
 }
lib/features/manage_account/presentation/preferences/preferences_view.dart (2)

54-76: Multi-click gating logic on the preferences header looks sound

You correctly disable the multi-click when either the label is already visible or the capability isn’t supported, and wire enableLabelVisibility only in the allowed case. This mirrors the SettingsView behavior and avoids repeated or invalid activations.

If this pattern will be reused elsewhere (and it already appears in SettingsView), consider extracting a small helper/extension that returns disableMultiClick from the dashboard controller to keep the logic centralized.


80-101: Label option visibility driven by isLabelVisibility

Excluding PreferencesOptionType.label from the generic local options and then conditionally appending it when isLabelVisibility.isTrue cleanly ties the option’s presence to the feature state. The mix of .value in the header and .isTrue here is technically fine for an RxBool, but you may want to standardize on one style for readability.

lib/features/manage_account/domain/state/save_label_visibility_state.dart (1)

1-10: State definitions align with existing pattern

SavingLabelVisibility, SaveLabelVisibilitySuccess, and SaveLabelVisibilityFailure follow the existing Loading/UI/Failure conventions and should drop in cleanly with the interactor. No issues spotted.

If other state classes in this layer use const constructors, you could mirror that for minor perf/readability benefits.

lib/features/manage_account/presentation/menu/settings/settings_view.dart (1)

32-62: Reactive app bar + multi-click wiring is correct, but duplicated with preferences header

Wrapping SettingAppBar in an Obx and driving onMultiClickAction from labelVisibility || !isLabelCapabilitySupported gives the right behavior: the multi-click is only available when it can meaningfully enable labels, and it deactivates once visibility is turned on.

Since the disableMultiClick computation is identical to the one in PreferencesView, consider centralizing it (e.g., as a getter/extension on the dashboard controller) to avoid future divergence.

lib/features/manage_account/domain/model/preferences/label_config.dart (1)

1-30: LabelConfig model is straightforward and consistent

LabelConfig cleanly extends PreferencesConfig with a single isEnabled flag, JSON support, initial() factory, and a copyWith extension. Equality via props matches the current field set.

You might consider making the constructor and initial() factory const (if that’s consistent with other configs) to allow compile-time instances.

lib/features/manage_account/presentation/extensions/handle_setup_label_visibility_in_setting_extension.dart (1)

11-45: Consider tying enable/setup flow to capability and interactor success before showing toast

The extension wiring is sound, but two aspects are worth tightening:

  • enableLabelVisibility() and setUpLabelVisibility() don’t consult isLabelCapabilitySupported, so a misplaced call could toggle local state and fire interactors even if labels aren’t supported.
  • The success toast in enableLabelVisibility() is shown unconditionally once the interactor is triggered, regardless of whether saving actually succeeds.

If feasible, consider:

  • Early‑returning when !isLabelCapabilitySupported, and/or
  • Emitting the “label feature is now available” toast only from the success branch of the save interactor’s state handling, rather than immediately after execute().

This would keep UI feedback aligned with actual capability and persistence state.

lib/features/manage_account/data/repository/manage_account_repository_impl.dart (1)

29-42: Repository delegations for label visibility and setting state look correct

The three new methods cleanly delegate to the data source and keep the interface in sync. Given both getLabelVisibility() and getLabelSettingState() return bool, a short doc comment in the interface clarifying how “visibility” (cached UI preference) differs from “setting state” (stored in preferences) could help future readers, but the implementation itself is fine.

lib/features/manage_account/data/local/preferences_setting_manager.dart (1)

6-23: LabelConfig integration into preferences manager is consistent

  • _preferencesSettingLabelKey and the new switch branch in loadPreferences() correctly deserialize stored label config as LabelConfig.
  • _getPreferencesConfigKey + the refactored savePreferences() keep per‑type key selection in one place, which should reduce future branching.
  • getLabelConfig() / updateLabel() follow the same pattern as the thread/spam/text-formatting helpers and safely default to LabelConfig.initial() when nothing is stored.

No functional issues here; the additions fit cleanly with the existing preferences infrastructure.

Also applies to: 40-61, 63-82, 144-160

lib/features/manage_account/data/datasource_impl/manage_account_datasource_impl.dart (1)

6-27: Data source wiring for label visibility and setting state is coherent

  • Injecting SettingCacheManager and branching on LabelConfig in toggleLocalSettingsState() is consistent with how other preference types are handled.
  • getLabelVisibility() / saveLabelVisibility() delegate to the cache manager with exception handling, and getLabelSettingState() reads LabelConfig.isEnabled via PreferencesSettingManager, which cleanly separates persisted preference from quick visibility cache.

You might consider:

  • Using the same .catchError(_exceptionThrower.throwException) style as other methods for consistency, and
  • Ensuring all DI bindings constructing ManageAccountDataSourceImpl are updated to pass the new SettingCacheManager parameter.

Functionally, though, this looks correct.

Also applies to: 37-59, 71-100

lib/features/manage_account/presentation/manage_account_dashboard_controller.dart (1)

77-77: Consider renaming for clarity.

The field name isLabelVisibility could be more explicit. Consider isLabelVisibilityEnabled or labelVisibilityEnabled to better convey that it represents an enabled/disabled state rather than just "visibility".

Apply this diff if you prefer a clearer name:

-  final isLabelVisibility = RxBool(false);
+  final isLabelVisibilityEnabled = RxBool(false);

Note: This would require updating references in the extension and any UI code that consumes this field.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 0d58d28 and 3ae8840.

📒 Files selected for processing (38)
  • lib/features/email/presentation/email_view.dart (2 hunks)
  • lib/features/labels/presentation/label_controller.dart (5 hunks)
  • lib/features/mailbox/presentation/base_mailbox_view.dart (2 hunks)
  • lib/features/mailbox_dashboard/presentation/controller/mailbox_dashboard_controller.dart (3 hunks)
  • lib/features/manage_account/data/datasource/manage_account_datasource.dart (1 hunks)
  • lib/features/manage_account/data/datasource_impl/manage_account_datasource_impl.dart (4 hunks)
  • lib/features/manage_account/data/local/preferences_setting_manager.dart (5 hunks)
  • lib/features/manage_account/data/local/setting_cache_manager.dart (1 hunks)
  • lib/features/manage_account/data/repository/manage_account_repository_impl.dart (1 hunks)
  • lib/features/manage_account/domain/model/preferences/label_config.dart (1 hunks)
  • lib/features/manage_account/domain/model/preferences/preferences_setting.dart (2 hunks)
  • lib/features/manage_account/domain/repository/manage_account_repository.dart (1 hunks)
  • lib/features/manage_account/domain/state/get_label_setting_state.dart (1 hunks)
  • lib/features/manage_account/domain/state/get_label_visibility_state.dart (1 hunks)
  • lib/features/manage_account/domain/state/save_label_visibility_state.dart (1 hunks)
  • lib/features/manage_account/domain/usecases/get_label_setting_state_interactor.dart (1 hunks)
  • lib/features/manage_account/domain/usecases/get_label_visibility_interactor.dart (1 hunks)
  • lib/features/manage_account/domain/usecases/save_label_visibility_interactor.dart (1 hunks)
  • lib/features/manage_account/presentation/bindings/setting_interactor_bindings.dart (2 hunks)
  • lib/features/manage_account/presentation/extensions/handle_setup_label_visibility_in_setting_extension.dart (1 hunks)
  • lib/features/manage_account/presentation/manage_account_dashboard_controller.dart (4 hunks)
  • lib/features/manage_account/presentation/menu/settings/setting_app_bar.dart (3 hunks)
  • lib/features/manage_account/presentation/menu/settings/setting_first_level_app_bar.dart (3 hunks)
  • lib/features/manage_account/presentation/menu/settings/settings_view.dart (2 hunks)
  • lib/features/manage_account/presentation/model/preferences_option_type.dart (5 hunks)
  • lib/features/manage_account/presentation/preferences/bindings/preferences_bindings.dart (1 hunks)
  • lib/features/manage_account/presentation/preferences/bindings/preferences_interactors_bindings.dart (2 hunks)
  • lib/features/manage_account/presentation/preferences/preferences_controller.dart (5 hunks)
  • lib/features/manage_account/presentation/preferences/preferences_view.dart (3 hunks)
  • lib/features/manage_account/presentation/widgets/setting_header_widget.dart (2 hunks)
  • lib/features/search/email/presentation/search_email_view.dart (2 hunks)
  • lib/features/thread/presentation/thread_view.dart (2 hunks)
  • lib/features/thread_detail/presentation/extension/get_thread_details_email_views.dart (3 hunks)
  • lib/features/thread_detail/presentation/thread_detail_view.dart (2 hunks)
  • lib/l10n/intl_messages.arb (2 hunks)
  • lib/main/bindings/local/local_bindings.dart (2 hunks)
  • lib/main/localizations/app_localizations.dart (1 hunks)
  • test/features/mailbox_dashboard/presentation/view/mailbox_dashboard_view_widget_test.dart (5 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 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/manage_account/presentation/bindings/setting_interactor_bindings.dart
  • lib/features/labels/presentation/label_controller.dart
🔇 Additional comments (28)
lib/main/bindings/local/local_bindings.dart (1)

39-39: LGTM!

The SettingCacheManager registration follows the established pattern used by other cache managers in this file (e.g., LocalSortOrderManager, LanguageCacheManager), correctly injecting SharedPreferences as its dependency.

Also applies to: 98-98

lib/features/manage_account/presentation/preferences/bindings/preferences_bindings.dart (1)

16-28: LGTM!

The binding order is correct—registering GetLocalSettingsInteractor and UpdateLocalSettingsInteractor before PreferencesController ensures the dependencies are available when the controller resolves them via Get.find().

lib/features/thread_detail/presentation/thread_detail_view.dart (1)

103-115: LGTM!

The conditional logic correctly gates label rendering behind both capability support and user setting enablement. The values are properly extracted inside the Obx block ensuring reactivity when isLabelSettingEnabled or labels change.

lib/features/thread_detail/presentation/extension/get_thread_details_email_views.dart (1)

49-51: LGTM!

The label logic mirrors the existing pattern for subject (line 56), where the last email's data is displayed on the first (collapsed) email in the thread detail view. The null-safe check labels?.isNotEmpty == true ensures the force-unwrap on labels! is safe.

lib/features/labels/presentation/label_controller.dart (4)

41-50: LGTM!

The checkLabelSettingState method follows the established pattern in this controller for handling optional interactors. The fallback behavior (disabling and clearing data) ensures a consistent state when the interactor is unavailable.


150-152: LGTM!

The failure handler correctly disables the label setting and clears data, maintaining a consistent fallback state when the setting cannot be retrieved.


158-164: LGTM!

The onClose cleanup properly nullifies the new _getLabelSettingStateInteractor reference, following the established pattern for other interactors in this controller.


122-129: LGTM!

The success handler correctly coordinates enabling the setting state, injecting bindings, and loading labels in sequence. The conditional if (isEnabled) prevents unnecessary binding injection and label fetching when the feature is disabled.

GetX's lazyPut safely guards against duplicate registrations by using service type as the key. When injectLabelsBindings() is called multiple times, the LabelInteractorBindings().dependencies() method will attempt to register services via lazyPut, but GetX's internal mechanism will skip re-registration for already-registered types, making this approach idempotent and safe.

lib/features/mailbox/presentation/base_mailbox_view.dart (1)

344-413: Label bar/list gating logic is consistent and safe

Using both isLabelCapabilitySupported and labelController.isLabelSettingEnabled.isTrue before rendering the label bar and list cleanly encapsulates the new visibility rule and avoids unnecessary label computations when disabled. The two builders are aligned and side‑effect free.

lib/features/manage_account/domain/model/preferences/preferences_setting.dart (1)

3-3: LabelConfig getter mirrors existing pattern and is safe

labelConfig follows the same firstWhereOrNull + initial() pattern as the other config getters, so callers always get a valid LabelConfig even when none is stored yet. No functional issues here.

Also applies to: 52-60

lib/features/email/presentation/email_view.dart (1)

12-12: Subject labels are now properly gated behind capability + setting

The conditional computation of emailLabels based on isLabelCapabilitySupported and labelController.isLabelSettingEnabled.isTrue is aligned with the new global label‑visibility rules, and avoids unnecessary work when labels are disabled. Passing the (possibly null) list into EmailSubjectWidget keeps the widget side responsible for rendering or omitting label chips.

Also applies to: 252-275

lib/features/manage_account/presentation/bindings/setting_interactor_bindings.dart (1)

7-7: LGTM! Clean dependency wiring.

The new dependencies are properly integrated:

  • SettingCacheManager correctly injected into the data source
  • Three new interactors follow the established binding pattern
  • Interactor names are descriptive and align with the learning about preferring full context over abbreviations

Also applies to: 10-12, 29-29, 36-46

lib/features/manage_account/domain/state/get_label_visibility_state.dart (1)

1-17: LGTM! State classes follow established patterns.

The three state classes (loading, success with visibility flag, failure with exception) are well-structured and consistent with similar state flows in the codebase.

lib/features/manage_account/domain/usecases/save_label_visibility_interactor.dart (1)

9-23: LGTM! Clean interactor implementation.

The execute method properly emits loading, success, and failure states with appropriate error handling. The default visible = true parameter is a reasonable choice for this save operation.

lib/features/manage_account/data/datasource/manage_account_datasource.dart (1)

13-17: LGTM! Clean interface extension.

The three new methods have clear, consistent signatures that align well with the existing data source interface pattern.

lib/features/manage_account/domain/repository/manage_account_repository.dart (1)

13-17: LGTM! Repository interface is consistent.

The three new methods align well with the data source interface and maintain consistent naming and return types.

lib/features/manage_account/domain/usecases/get_label_visibility_interactor.dart (1)

9-23: LGTM! Clean and consistent implementation.

The interactor follows the established pattern with proper state emission and error handling.

lib/features/manage_account/data/local/setting_cache_manager.dart (1)

3-18: LGTM! Clean cache manager implementation.

The implementation provides a straightforward interface for persisting label visibility. The default value of false is a safe choice, and the key naming follows good conventions.

lib/features/manage_account/presentation/menu/settings/setting_app_bar.dart (1)

16-27: Multi-click callback is only effective for non-universal settings

onMultiClickAction is correctly threaded into SettingFirstLevelAppBar while the universal path still ignores it. That fits a design where the “secret” multi-click only applies on first-level pages; just confirm that you don’t also need it on UniversalSettingAppBar.

Also applies to: 39-45

lib/features/manage_account/presentation/menu/settings/setting_first_level_app_bar.dart (1)

30-42: Clean integration of multi-click on title

The titleWidget refactor plus conditional MultiClickWidget wrapping is clear and keeps behavior unchanged when onMultiClickAction is null. Center alignment and padding remain intact.

Also applies to: 61-64

lib/features/manage_account/presentation/widgets/setting_header_widget.dart (1)

12-20: Header multi-click behavior is well-contained

The titleWidget extraction plus conditional MultiClickWidget wrapping integrates multi-click cleanly without affecting the explanation block or overall layout. Public API change (new onMultiClickAction) is minimal and backwards compatible.

Also applies to: 26-44

lib/main/localizations/app_localizations.dart (1)

5531-5558: Label visibility localization getters look consistent

The four new getters follow the existing Intl.message pattern, match the ARB keys, and the copy is clear and concise. No issues spotted.

lib/features/manage_account/domain/state/get_label_setting_state.dart (1)

1-19: Label setting state classes align with existing state pattern

Loading/success/failure states are structured consistently with other UIState/FeatureFailure usages (props implemented, AccountId carried in success). Looks good.

lib/features/manage_account/presentation/model/preferences_option_type.dart (1)

7-78: New label preference option is integrated consistently

The new label enum value is correctly handled in all switch methods and pulls its enabled state from preferencesSetting.labelConfig.isEnabled, matching the localization keys you introduced. No issues from this file’s side.

lib/l10n/intl_messages.arb (1)

5215-5238: Label visibility ARB entries match localization usage

The four new ARB keys and their metadata blocks are well-formed, use the same identifiers as in AppLocalizations, and require no placeholders. The updated @@last_modified is fine. Looks good.

lib/features/manage_account/presentation/manage_account_dashboard_controller.dart (3)

25-25: LGTM! Imports follow existing patterns.

The new imports for label visibility state, interactors, and extension are properly organized and consistent with the existing code structure.

Also applies to: 28-29, 34-34


109-110: LGTM! Success handling follows established patterns.

The handling of GetLabelVisibilitySuccess is consistent with other success state handlers in this method and properly delegates to the extension.


68-69: The interactors are properly initialized before use. SettingInteractorBindings().dependencies() is called in manage_account_dashboard_bindings.dart (line 13) before the controller is instantiated, and both the setUpLabelVisibility() and enableLabelVisibility() extension methods safely retrieve interactors using getBinding<>() with null checks before execution. Unlike vacation and quota interactors which use Get.find<>() with try-catch in injector methods, the label visibility pattern uses getBinding<>() upfront in the extension, which is a valid alternative approach with explicit null safety.

Likely an incorrect or invalid review comment.

@github-actions
Copy link

github-actions bot commented Dec 9, 2025

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

@dab246
Copy link
Member Author

dab246 commented Dec 10, 2025

@coderabbitai Please check and remove Nitpick comments unnecessary

@coderabbitai
Copy link

coderabbitai bot commented Dec 10, 2025

Tip

For best results, initiate chat on the files or code changes.

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Fetch all review comments on PR #4194 to identify nitpick comments
gh pr view 4194 --repo linagora/tmail-flutter --json reviews --jq '.reviews[] | select(.author.login == "coderabbitai") | {id: .id, state: .state, body: .body}'

Length of output: 33030


🏁 Script executed:

#!/bin/bash
# Also fetch review comments (line-level comments) on the PR
gh api repos/linagora/tmail-flutter/pulls/4194/comments --jq '.[] | select(.user.login == "coderabbitai[bot]") | {id: .id, path: .path, line: .line, body: .body, created_at: .created_at}'

Length of output: 16509


@dab246: I've reviewed all the nitpick comments on this PR. Here's my analysis of which ones could be considered unnecessary and resolved:

Recommendations for Resolution:

Definitely Unnecessary (Can Resolve):

  1. Three "Remove unnecessary import" comments for import 'dart:core'; in:

    • save_label_visibility_interactor.dart
    • get_label_setting_state_interactor.dart
    • get_label_visibility_interactor.dart

    While technically correct, these are auto-linting issues that should be caught by your linter config rather than manual review.

  2. Field naming suggestion (isLabelVisibilityisLabelVisibilityEnabled):

    • This is purely subjective. Your current naming is clear and consistent with the codebase.
  3. Const constructor suggestions:

    • State classes and LabelConfig "could use const" - these are micro-optimizations that don't impact functionality.

Optional but Valid (Your Choice):

  1. Centralization suggestions (test setup helper, label refresh helper, disableMultiClick helper):

    • These suggest extracting repeated patterns but current code is clear and works fine.
  2. Performance suggestion (removing reload() in SettingCacheManager):

    • Could be valid but needs benchmarking in your specific use case.
  3. Extension flow improvements (checking capability before enabling):

    • Already handled through UI gating, so additional checks may be redundant.

Would you like me to resolve the clearly unnecessary nitpick comments (dart:core imports and naming suggestions)? The rest are optional refactoring suggestions that you can address or dismiss based on your team's preferences.

@dab246
Copy link
Member Author

dab246 commented Dec 10, 2025

  1. Const constructor suggestions:

We don't use const here because the parent class itself doesn't have const, it will give an error when we add const

@dab246 dab246 force-pushed the feature/tf-4193-setup-label-in-setting branch from e62723c to 64170da Compare December 10, 2025 05:10
@dab246 dab246 changed the base branch from feature/tf-4178-create-a-new-label to features/label December 10, 2025 05:11
@chibenwa
Copy link
Member

https://github.com/coderabbitai Please check and remove Nitpick comments unnecessary

Seriously? What added value do we, as human, bring?

@hoangdat hoangdat merged commit 896f349 into features/label Dec 10, 2025
20 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants