Conversation
|
This PR has been deployed to https://linagora.github.io/tmail-flutter/4182. |
dab246
force-pushed
the
feature/tf-4179-display-tag-list-in-sidebar
branch
from
8b64b6e to
63e7d93
Compare
dab246
force-pushed
the
feature/tf-4178-create-a-new-label
branch
from
efc6cde to
9669372
Compare
|
Caution Review failedThe pull request is closed. WalkthroughAdds a full "create label" feature and supporting UI: new color-selection widgets (ColorCircleWidget, ColorsMapWidget, ColorPickerModal) and exports; new label creation pipeline across datasource, API, repository, and interactor; presentation changes including CreateNewLabelModal, LabelController wiring and a web modal flow; shared widget import refactors to core; error-handling helper parseErrorForSetResponse and related tests; localization entries and toast handling for create-label flows; small API surface changes (ImagePaths, color constant, TMailButtonWidget factory param, ModalListActionButtonWidget flag) and a new dependency on flutter_hsvcolor_picker. Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro ⛔ Files ignored due to path filters (6)
📒 Files selected for processing (36)
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 review |
✅ Actions performedReview triggered.
|
hoangdat
changed the base branch from
feature/tf-4179-display-tag-list-in-sidebar
to
features/label
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (9)
labels/lib/extensions/list_label_extension.dart (1)
13-15:displayNameNotNullListimplementation is correct; optionally normalize whitespace-only namesLogic is sound and safe. If you want to also drop names that are only spaces, you can trim before checking emptiness.
- List<String> get displayNameNotNullList => map((label) => label.safeDisplayName) - .where((name) => name.isNotEmpty) - .toList(); + List<String> get displayNameNotNullList => map((label) => label.safeDisplayName) + .where((name) => name.trim().isNotEmpty) + .toList();core/lib/presentation/views/color/color_circle_widget.dart (1)
6-49: Clean implementation of a reusable color circle widget.The widget structure is well-organized with proper use of
Materialfor ink ripple effects on a circular shape. Consider addingSemanticsfor accessibility to describe this as a color selection button.@override Widget build(BuildContext context) { - return Material( + return Semantics( + button: true, + selected: isSelected, + label: 'Color option', + child: Material( type: MaterialType.transparency, child: InkWell( onTap: onTap, customBorder: const CircleBorder(), ... ), + ), ); }lib/features/labels/presentation/label_interactor_bindings.dart (1)
11-11: Cross-feature dependency onVerifyNameInteractor.This imports
VerifyNameInteractorfrommailbox_creatordomain. If label name validation requirements diverge from mailbox naming in the future, consider extracting a shared validation module tocoreor creating a label-specific validator. For now, reusing existing validation logic is pragmatic.lib/features/labels/presentation/widgets/create_new_label_modal.dart (1)
313-320: Consider consolidating color update methods.
_updateLabelColorand_onLabelColorChangedserve similar purposes. The distinction is thatColorsMapWidgethandles its own UI state whileColorPickerModalrequires external state update. Consider documenting this distinction or unifying to_onLabelColorChangedfor both callbacks for consistency.core/lib/presentation/views/color/colors_map_widget.dart (2)
26-52: Color list and selection state look correct; consider clarifying initial-selection behaviorThe
_setUpColorListlogic anddidUpdateWidgethandling ofcustomColorare consistent and safe. One behavioral nuance: whenevercustomColorchanges (including being set tonull),_selectedColoris reset tocustom. If the parent ever togglescustomColorindependently of the user’s selection, this will override any prior user choice. If that’s not desired, you might want to only update_colorListhere and leave_selectedColorunchanged, or make this behavior explicit in docs.Also applies to: 129-143
68-125: UI wiring and callbacks are sound; only micro-optimizations possibleThe Wrap layout, clear button, color circles, and color-picker trigger are all wired correctly and use
OnSelectColorCallbackconsistently. If you ever profile this, you could:
- Lift the
ValueListenableBuilderup one level to rebuild the row once instead of one builder per color.- Pass a
childintoValueListenableBuilder(theColorCircleWidgetwith fixed props) to avoid rebuilding icons on every selection change.These are purely optional; current code is correct and maintainable.
core/lib/presentation/views/color/color_picker_modal.dart (2)
56-62: Init state is fine; callingsuper.initState()first would be more idiomaticThe initialization of
_hsvColorNotifierand_hexColorInputControllerfrominitialColoris correct. For consistency with Flutter’s usual pattern, you may want to movesuper.initState()to the beginning of the method, but this is stylistic only.
181-248: Guard against invalid/partial hex values when callingtoColor
_onHexColorChangedand_onPositiveActionboth callvalue.toColor/hexColorText.toColoron arbitrary user input. IfStringExtension.toColorassumes a fully-formed hex string (e.g. 6–8 digits, optional#) and throws on partial or malformed values, typing into the field or saving an invalid value could crash the modal.Consider:
- Only updating
_hsvColorNotifierwhen the input length matches the supported formats, or- Wrapping
toColorin atry/catchand ignoring/rolling back invalid input, and- Optionally normalizing input (e.g. stripping
#, uppercasing) before parsing.This would harden the modal against unexpected input behavior.
void _onHexColorChanged(String value) { final hex = value.trim(); if (hex.isEmpty) return; // Example guard; adjust to what `toColor` supports. if (hex.length != 6 && hex.length != 7) { return; } try { _hsvColorNotifier.value = HSVColor.fromColor(hex.toColor); } catch (_) { // Optionally keep previous value or reset field. } }Also applies to: 317-324
lib/main/localizations/app_localizations.dart (1)
5454-5525: Localization getters correctly wired; consider copy/style consistencyThe new label-related getters are correctly defined and aligned with
intlusage and the ARB keys (including thelabelNameplaceholder increateLabelSuccessfullyMessage). Two minor polish points you might consider:
- The wording
"You successfully created $labelName label"mirrors folder strings but is slightly awkward;"You successfully created the $labelName label"would read more naturally if you’re open to adjusting both Dart and ARB.- The texts mix US and UK spelling (
colorvscolour) across these keys; aligning on one variant for this feature area would keep the UI more consistent.Functionally this looks good as-is.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (6)
assets/images/ic_color_picker.svgis excluded by!**/*.svgcontact/pubspec.lockis excluded by!**/*.lockcore/pubspec.lockis excluded by!**/*.locklabels/pubspec.lockis excluded by!**/*.lockmodel/pubspec.lockis excluded by!**/*.lockpubspec.lockis excluded by!**/*.lock
📒 Files selected for processing (34)
core/lib/core.dart(3 hunks)core/lib/presentation/extensions/color_extension.dart(1 hunks)core/lib/presentation/resources/image_paths.dart(1 hunks)core/lib/presentation/views/button/tmail_button_widget.dart(2 hunks)core/lib/presentation/views/color/color_circle_widget.dart(1 hunks)core/lib/presentation/views/color/color_picker_modal.dart(1 hunks)core/lib/presentation/views/color/colors_map_widget.dart(1 hunks)core/lib/presentation/views/dialog/modal_list_action_button_widget.dart(2 hunks)core/pubspec.yaml(1 hunks)labels/lib/extensions/list_label_extension.dart(1 hunks)lib/features/base/mixin/handle_error_mixin.dart(2 hunks)lib/features/email_recovery/presentation/widgets/email_recovery_form/email_recovery_form_desktop_builder.dart(1 hunks)lib/features/identity_creator/presentation/widgets/identity_creator_form_desktop_builder.dart(1 hunks)lib/features/labels/data/datasource/label_datasource.dart(1 hunks)lib/features/labels/data/datasource_impl/label_datasource_impl.dart(1 hunks)lib/features/labels/data/network/label_api.dart(2 hunks)lib/features/labels/data/repository/label_repository_impl.dart(1 hunks)lib/features/labels/domain/repository/label_repository.dart(1 hunks)lib/features/labels/domain/state/create_new_label_state.dart(1 hunks)lib/features/labels/domain/usecases/create_new_label_interactor.dart(1 hunks)lib/features/labels/presentation/label_controller.dart(6 hunks)lib/features/labels/presentation/label_interactor_bindings.dart(3 hunks)lib/features/labels/presentation/widgets/create_new_label_modal.dart(1 hunks)lib/features/mailbox/data/network/mailbox_api.dart(1 hunks)lib/features/mailbox/presentation/base_mailbox_view.dart(2 hunks)lib/features/mailbox_creator/presentation/extensions/validator_failure_extension.dart(1 hunks)lib/features/mailbox_creator/presentation/mailbox_creator_view.dart(1 hunks)lib/features/mailbox_dashboard/presentation/extensions/verify_display_overlay_view_on_iframe_extension.dart(1 hunks)lib/features/quotas/presentation/widget/quotas_banner_widget.dart(1 hunks)lib/features/rules_filter_creator/presentation/rules_filter_creator_view.dart(0 hunks)lib/l10n/intl_messages.arb(2 hunks)lib/main/localizations/app_localizations.dart(1 hunks)lib/main/utils/toast_manager.dart(4 hunks)test/features/composer/presentation/composer_controller_test.dart(5 hunks)
💤 Files with no reviewable changes (1)
- lib/features/rules_filter_creator/presentation/rules_filter_creator_view.dart
🧰 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:
test/features/composer/presentation/composer_controller_test.dartlib/features/labels/presentation/label_interactor_bindings.dart
🪛 RuboCop (1.81.7)
lib/l10n/intl_messages.arb
[convention] 5203-5203: Put a comma after the last item of a multiline array.
(Style/TrailingCommaInArrayLiteral)
🔇 Additional comments (33)
lib/features/email_recovery/presentation/widgets/email_recovery_form/email_recovery_form_desktop_builder.dart (1)
4-4: Core import forDefaultCloseButtonWidgetlooks correctCentralizing this widget under
core/presentation/views/buttonis consistent with the rest of the modal code and does not change behavior.lib/features/mailbox_creator/presentation/extensions/validator_failure_extension.dart (1)
52-62:getMessageLabelErroris consistent with existing failure mappersBranching mirrors other helpers, and the chosen localization keys fit the label use case. No issues from a correctness standpoint.
core/lib/presentation/views/dialog/modal_list_action_button_widget.dart (1)
11-20: NewisPositiveActionEnabledflag cleanly adds a disabled state; confirmConfirmDialogButtonsupports nullable callbacksThe flag and styling logic are straightforward and backward compatible (default
true). Disabling viaonTapAction: nullis a good pattern as long asConfirmDialogButton’sonTapActionparameter is nullable and treated as disabled.Please double‑check
ConfirmDialogButton’s constructor signature to ensureonTapActionis declared asVoidCallback?(or otherwise safely handlesnull) so this disabled state compiles and behaves as intended.Also applies to: 39-46
lib/features/mailbox_creator/presentation/mailbox_creator_view.dart (1)
6-7: Refactor to use core button/dialog widgets is soundSwitching to the core
DefaultCloseButtonWidgetandModalListActionButtonWidgetkeeps behavior unchanged while centralizing shared UI components.lib/features/labels/data/repository/label_repository_impl.dart (1)
16-19: RepositorycreateNewLabelpass‑through is correctThe method cleanly delegates to the datasource and matches the repository pattern established by
getAllLabels.lib/main/utils/toast_manager.dart (1)
18-18: Label creation failure/success toasts are wired correctlyDefaulting
CreateNewLabelFailuretocreateNewLabelFailureensures a user‑visible error, and usingnewLabel.safeDisplayNamein the success toast is a good defensive choice around label naming.Also applies to: 31-31, 201-204, 267-270
core/lib/presentation/extensions/color_extension.dart (1)
279-279: LGTM!The new color constant follows the established naming convention and is placed appropriately among other gray color definitions.
lib/features/identity_creator/presentation/widgets/identity_creator_form_desktop_builder.dart (1)
4-4: LGTM!The import path change aligns with the centralization of shared widgets in the core package, improving code organization and reusability.
lib/features/labels/data/datasource/label_datasource.dart (1)
6-7: LGTM!The interface extension follows the established pattern and provides a clear contract for label creation.
lib/features/labels/data/datasource_impl/label_datasource_impl.dart (1)
22-31: LGTM!The implementation correctly follows the established pattern used by
getAllLabels, maintaining consistency in error handling and API delegation.lib/features/labels/domain/repository/label_repository.dart (1)
6-7: LGTM!The repository interface extension mirrors the datasource contract, maintaining clean layer separation.
core/lib/presentation/resources/image_paths.dart (1)
265-265: LGTM!The new image path accessor follows the established pattern, and the SVG asset file exists at
assets/images/ic_color_picker.svg.lib/features/mailbox_dashboard/presentation/extensions/verify_display_overlay_view_on_iframe_extension.dart (1)
12-13: LGTM!The logic correctly extends overlay visibility to include the new label creation modal. Both
labelControllerandisCreateNewLabelModalVisibleare properly defined inLabelControllerand accessible throughMailboxDashBoardController.lib/features/mailbox/data/network/mailbox_api.dart (1)
199-199: LGTM!The refactoring to use
parseErrorForSetResponsefromHandleSetErrorMixinproperly centralizes error handling logic and reduces code duplication. The mixin method robustly handles all error scenarios:invalidArguments,invalidResultReference, and unknown errors, ensuring no error cases fall through unhandled.lib/features/quotas/presentation/widget/quotas_banner_widget.dart (1)
1-1: LGTM!The import path change aligns with the centralization of shared widgets in the core package, as reflected by the new export in
core/lib/core.dart.test/features/composer/presentation/composer_controller_test.dart (2)
102-111: LGTM!The
MockLabelControllercorrectly implements theLabelControllerinterface with appropriate Rx types for the label feature integration.
160-162: LGTM!The
labelControllergetter properly exposes theMockLabelControllerfor test scaffolding.core/lib/presentation/views/button/tmail_button_widget.dart (1)
113-141: LGTM!The
borderparameter addition tofromIconfactory brings consistency with the main constructor andfromTextfactory, enabling border customization for icon-only button use cases like the color picker widgets.core/lib/core.dart (3)
78-78: LGTM!Export addition for
default_close_button_widget.dartis appropriately placed with other button widgets.
91-91: LGTM!Export for
modal_list_action_button_widget.dartis logically grouped with dialog-related widgets.
119-121: LGTM!The new color-related widget exports (
color_circle_widget.dart,colors_map_widget.dart,color_picker_modal.dart) are well-organized together, supporting the label color picker feature.lib/features/labels/domain/usecases/create_new_label_interactor.dart (1)
9-28: LGTM!The
CreateNewLabelInteractorfollows the established pattern for interactors in this codebase:
- Emits loading state (
CreatingNewLabel) before the async operation- Properly wraps success/failure in
Eithertypes- Uses
async*generator for stream-based state emission- Preserves exception details in the failure state
lib/features/mailbox/presentation/base_mailbox_view.dart (1)
350-366: The null safety concern is already addressed. TheopenCreateNewLabelModalmethod acceptsAccountId?and the downstream_createNewLabelmethod explicitly checks for null (line 76 of label_controller.dart), returning aCreateNewLabelFailure(NotFoundAccountIdException())that displays an error to the user. No changes are required.lib/features/labels/presentation/label_interactor_bindings.dart (1)
33-37: LGTM!The new interactor bindings follow the established pattern in this file. The
VerifyNameInteractoris stateless, so instantiating it without a repository dependency is correct.lib/features/labels/domain/state/create_new_label_state.dart (1)
1-18: LGTM!The state classes follow the established pattern in the codebase. The
propsoverride ensures proper equality comparison for the success state.lib/features/labels/data/network/label_api.dart (1)
48-58: Verify server response completeness.The
copyWithto restoredisplayNameandcolorfrom input suggests the server may not return these fields in the created object. This is a reasonable defensive pattern, but verify this behavior matches the JMAP server implementation to ensure no data is lost.lib/features/labels/presentation/label_controller.dart (1)
54-71: LGTM!The modal visibility pattern with
whenCompleteensures proper cleanup regardless of how the modal is dismissed. The web-specific visibility flag is a good approach for platform-aware UI state.lib/features/labels/presentation/widgets/create_new_label_modal.dart (3)
294-306: Modal closes before confirming label creation success.The modal calls
popBack()immediately after invoking the callback without waiting for the creation result. Users won't see feedback within the modal if creation fails. Consider either:
- Showing a loading indicator and waiting for the result before closing
- Documenting that toast feedback will appear after modal dismissal (current behavior)
If the current UX is intentional (toast shows after modal closes), this is acceptable.
336-345: LGTM!All disposable resources (FocusNode, TextEditingController, ValueNotifiers) are properly cleaned up. The assignment
_labelDisplayNameList = []on line 343 is unnecessary since the state object is being disposed, but it's harmless.
42-44: DI lookups assume bindings are registered.
Get.find<ImagePaths>()andGet.find<VerifyNameInteractor>()are called during state initialization. This works becauseLabelController.openCreateNewLabelModalcallsinjectLabelsBindings()before showing this modal. Just ensure this dependency ordering is maintained.core/lib/presentation/views/color/color_picker_modal.dart (2)
150-179: HSV picker + notifier wiring is correctUsing
ValueListenableBuilder<HSVColor>aroundPaletteHuePickerand updating via_onHsvColorChangedis a clean pattern; there are no obvious re-entrancy or lifecycle issues here.
276-287: Mobile scaffold + backdrop tap behavior is reasonableThe nested
GestureDetectorpattern on mobile (outer close-tap, inner focus-clear) matches the common modal/backdrop UX and should behave correctly with Flutter’s gesture arena. No changes needed here.lib/l10n/intl_messages.arb (1)
2-2: New label-related ARB entries are consistent with the Dart localizationsThe added keys and their
@metadata (includingplaceholders_orderandplaceholdersforcreateLabelSuccessfullyMessage) correctly match the new getters inAppLocalizations. From an i18n tooling perspective, this block is well-formed and should integrate cleanly with the existing translations.Also applies to: 5144-5214
test/features/composer/presentation/composer_controller_test.dart
Outdated
Show resolved
Hide resolved
dab246
force-pushed
the
feature/tf-4178-create-a-new-label
branch
from
9669372 to
0d58d28
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
2 participants
Issue
#4178
Resolved
Screen.Recording.2025-11-26.at.16.59.22.mov
Summary by CodeRabbit
New Features
Improvements
Dependencies
✏️ Tip: You can customize this high-level summary in your review settings.