[Scribe mobile] Refactoring to reuse components for Scribe mobile#4244
[Scribe mobile] Refactoring to reuse components for Scribe mobile#4244hoangdat merged 6 commits intoscribe-mobilefrom
Conversation
|
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 You can disable this status message by setting the WalkthroughThis change updates the scribe package's public exports and UI components for AI Scribe: it adds AiScribeSuggestionStateMixin and new menu-item widgets (AiScribeMenuItem, AiScribeMenuIcon, AiScribeMenuSubmenuIcon, AiScribeMenuText), replaces an exported submenu file with the new item widgets in scribe/lib/scribe.dart, refactors AiScribeSuggestionWidget to use the new mixin, simplifies AiScribeContextMenuItem to delegate to AiScribeMenuItem, and adds an optional boxShadow field to AIScribeBar. Only exported/public surface and UI wiring are modified. Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
|
This PR has been deployed to https://linagora.github.io/tmail-flutter/4244. |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![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
🤖 Fix all issues with AI agents
In
@scribe/lib/scribe/ai/presentation/utils/modal/ai_scribe_suggestion_state_mixin.dart:
- Around line 40-54: The public method loadSuggestion uses a potentially null
_interactor with a forced unwrap (_interactor!), which can cause a crash if
registration failed; guard against null by checking _interactor before calling
execute and set _suggestionState to a failure (or appropriate state) if it's
null, e.g., early-return when _interactor == null after optionally logging or
assigning a Left failure; keep the existing mounted check and continue using
aiAction and content when calling _interactor.execute.
🧹 Nitpick comments (2)
scribe/lib/scribe/ai/presentation/widgets/context_menu/ai_scribe_context_menu_item.dart (1)
64-75:_hoverController?.exit()is always a no-op for non-submenu items.Since
_hoverControlleris only initialized whenmenuAction.hasSubmenuis true (lines 33-36), it will always benullin this non-submenu branch. While harmless due to null-safety, this call is misleading and should be removed for clarity.Also, there's a minor formatting issue with extra whitespace on line 74.
Suggested cleanup
return AiScribeMenuItem( itemKey: _itemKey, menuAction: widget.menuAction, onSelectAction: widget.onSelectAction, imagePaths: widget.imagePaths, onHover: (_) { - _hoverController?.exit(); widget.onHoverOtherItem?.call(); } ); - }scribe/lib/scribe/ai/presentation/utils/modal/ai_scribe_suggestion_state_mixin.dart (1)
82-92: Unusedlocalizationsparameter inbuildLoadingStateandbuildErrorState.Both methods accept
ScribeLocalizations localizationsbut don't use it. If these are placeholders for future localization, consider adding a comment. Otherwise, the parameter could be removed to avoid confusion.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
scribe/lib/scribe.dartscribe/lib/scribe/ai/presentation/utils/modal/ai_scribe_suggestion_state_mixin.dartscribe/lib/scribe/ai/presentation/widgets/context_menu/ai_scribe_context_menu_item.dartscribe/lib/scribe/ai/presentation/widgets/items/ai_scribe_menu_item.dartscribe/lib/scribe/ai/presentation/widgets/items/ai_scribe_submenu_item.dartscribe/lib/scribe/ai/presentation/widgets/modal/ai_scribe_suggestion_widget.dartscribe/lib/scribe/ai/presentation/widgets/search/ai_scribe_bar.dart
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-12-09T09:36:45.349Z
Learnt from: dab246
Repo: linagora/tmail-flutter PR: 4194
File: lib/features/manage_account/presentation/manage_account_dashboard_controller.dart:174-176
Timestamp: 2025-12-09T09:36:45.349Z
Learning: In Dart/Flutter projects using GetX, do not wrap getBinding<T>() calls in try/catch since they return null when not found. Only wrap Get.find<T>() calls in try/catch because they throw if a dependency is unavailable. When a binding is optional, consider checking Get.isRegistered<T>() or handling the null/exception path gracefully instead of blindly catching, and document the expectation for failure modes where a dependency may not be registered.
Applied to files:
scribe/lib/scribe/ai/presentation/widgets/search/ai_scribe_bar.dartscribe/lib/scribe/ai/presentation/widgets/context_menu/ai_scribe_context_menu_item.dartscribe/lib/scribe/ai/presentation/widgets/items/ai_scribe_menu_item.dartscribe/lib/scribe/ai/presentation/utils/modal/ai_scribe_suggestion_state_mixin.dartscribe/lib/scribe/ai/presentation/widgets/modal/ai_scribe_suggestion_widget.dartscribe/lib/scribe.dart
📚 Learning: 2025-12-12T09:03:05.962Z
Learnt from: dab246
Repo: linagora/tmail-flutter PR: 4204
File: lib/features/email/presentation/model/popup_menu_item_email_action.dart:34-35
Timestamp: 2025-12-12T09:03:05.962Z
Learning: In the tmail-flutter codebase, popup menu items intentionally use a consistent hoverIcon (imagePaths.icThumbsUp) across all popup item types (email actions, mailbox actions, profile settings, etc.), while actionIcon varies per action type. This is a deliberate UI/UX design choice.
Applied to files:
scribe/lib/scribe/ai/presentation/widgets/context_menu/ai_scribe_context_menu_item.dartscribe/lib/scribe/ai/presentation/widgets/items/ai_scribe_menu_item.dart
🔇 Additional comments (8)
scribe/lib/scribe/ai/presentation/widgets/search/ai_scribe_bar.dart (1)
12-18: LGTM!The optional
boxShadowparameter is cleanly integrated with a sensible fallback toAIScribeShadows.modal. This approach maintains backward compatibility while enabling mobile-specific shadow customization.Also applies to: 67-67
scribe/lib/scribe.dart (1)
28-43: LGTM!The new exports are well-organized and correctly expose the refactored components (mixin, menu items, and suggestion UI widgets) for reuse across desktop and mobile implementations.
scribe/lib/scribe/ai/presentation/widgets/items/ai_scribe_menu_item.dart (2)
27-28: Verify: Should submenu category items triggeronSelectActionon tap?The
onTapcallback fires for all menu items, including those withhasSubmenu == true. Typically, category items with submenus only reveal their submenu on hover and don't perform an action on tap. If this is intentional (e.g., tapping navigates into the submenu on mobile), consider adding a comment. Otherwise, guard the tap:Suggested guard if tap should be disabled for submenu items
child: InkWell( - onTap: () => onSelectAction(menuAction), + onTap: menuAction.hasSubmenu ? null : () => onSelectAction(menuAction), hoverColor: AppColor.grayBackgroundColor,
36-69: LGTM on the menu item layout.The Row structure with conditional icon, flexible text label, and optional submenu arrow is clean and follows the existing UI patterns. The use of
Flexibleensures proper text truncation with ellipsis.scribe/lib/scribe/ai/presentation/widgets/modal/ai_scribe_suggestion_widget.dart (2)
35-48: LGTM!The mixin integration is clean. The getter overrides correctly proxy widget properties to satisfy the mixin's abstract requirements, enabling state logic reuse while keeping widget-specific layout in this file.
157-165: LGTM on click-outside handling.The dismiss logic correctly uses
suggestionStatefrom the mixin to determine if the modal should close (only when in failure or success state, not during loading).scribe/lib/scribe/ai/presentation/utils/modal/ai_scribe_suggestion_state_mixin.dart (2)
23-38: LGTM on initialization flow.The registration check with
Get.isRegistered<T>()beforeGet.find<T>()follows the project's GetX patterns (per learnings). The early return on registration failure with an appropriate error state is a good defensive approach.
107-111: LGTM on disposal.The
_suggestionStateValueNotifier is properly disposed to prevent memory leaks.
scribe/lib/scribe/ai/presentation/utils/modal/ai_scribe_suggestion_state_mixin.dart
Show resolved
Hide resolved
scribe/lib/scribe/ai/presentation/widgets/items/ai_scribe_menu_item.dart
Outdated
Show resolved
Hide resolved
scribe/lib/scribe/ai/presentation/widgets/items/ai_scribe_menu_item.dart
Outdated
Show resolved
Hide resolved
scribe/lib/scribe/ai/presentation/widgets/items/ai_scribe_menu_item.dart
Outdated
Show resolved
Hide resolved
scribe/lib/scribe/ai/presentation/widgets/items/ai_scribe_menu_item.dart
Outdated
Show resolved
Hide resolved
scribe/lib/scribe/ai/presentation/utils/modal/ai_scribe_suggestion_state_mixin.dart
Show resolved
Hide resolved
|
|
@coderabbitai Please review again |
|
@dab246, I'll perform a fresh review of the PR now, including checking for any outstanding nitpick comments that need to be addressed. ✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
scribe/lib/scribe/ai/presentation/utils/modal/ai_scribe_suggestion_state_mixin.dart (1)
40-64: Consider removing the unnecessarymountedcheck on line 44.The
mountedcheck at line 44 is redundant for the null interactor case. If_interactoris null, setting the failure state should happen immediately without checkingmounted. The guard at line 58 after the async call is correct.Suggested simplification
Future<void> loadSuggestion() async { _suggestionState.value = dartz.Right(GenerateAITextLoading()); if (_interactor == null) { - if (!mounted) return; _suggestionState.value = dartz.Left( GenerateAITextFailure( GenerateAITextInteractorIsNotRegisteredException(), ), ); return; }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
scribe/lib/scribe.dartscribe/lib/scribe/ai/presentation/utils/modal/ai_scribe_suggestion_state_mixin.dartscribe/lib/scribe/ai/presentation/widgets/context_menu/ai_scribe_context_menu_item.dartscribe/lib/scribe/ai/presentation/widgets/items/ai_scribe_menu_icon.dartscribe/lib/scribe/ai/presentation/widgets/items/ai_scribe_menu_item.dartscribe/lib/scribe/ai/presentation/widgets/items/ai_scribe_menu_submenu_icon.dartscribe/lib/scribe/ai/presentation/widgets/items/ai_scribe_menu_text.dartscribe/lib/scribe/ai/presentation/widgets/modal/ai_scribe_suggestion_widget.dart
🚧 Files skipped from review as they are similar to previous changes (1)
- scribe/lib/scribe/ai/presentation/widgets/context_menu/ai_scribe_context_menu_item.dart
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-12-09T09:36:45.349Z
Learnt from: dab246
Repo: linagora/tmail-flutter PR: 4194
File: lib/features/manage_account/presentation/manage_account_dashboard_controller.dart:174-176
Timestamp: 2025-12-09T09:36:45.349Z
Learning: In Dart/Flutter projects using GetX, do not wrap getBinding<T>() calls in try/catch since they return null when not found. Only wrap Get.find<T>() calls in try/catch because they throw if a dependency is unavailable. When a binding is optional, consider checking Get.isRegistered<T>() or handling the null/exception path gracefully instead of blindly catching, and document the expectation for failure modes where a dependency may not be registered.
Applied to files:
scribe/lib/scribe/ai/presentation/widgets/items/ai_scribe_menu_text.dartscribe/lib/scribe/ai/presentation/widgets/items/ai_scribe_menu_submenu_icon.dartscribe/lib/scribe/ai/presentation/widgets/modal/ai_scribe_suggestion_widget.dartscribe/lib/scribe/ai/presentation/widgets/items/ai_scribe_menu_icon.dartscribe/lib/scribe/ai/presentation/utils/modal/ai_scribe_suggestion_state_mixin.dartscribe/lib/scribe/ai/presentation/widgets/items/ai_scribe_menu_item.dartscribe/lib/scribe.dart
📚 Learning: 2025-12-12T09:03:05.962Z
Learnt from: dab246
Repo: linagora/tmail-flutter PR: 4204
File: lib/features/email/presentation/model/popup_menu_item_email_action.dart:34-35
Timestamp: 2025-12-12T09:03:05.962Z
Learning: In the tmail-flutter codebase, popup menu items intentionally use a consistent hoverIcon (imagePaths.icThumbsUp) across all popup item types (email actions, mailbox actions, profile settings, etc.), while actionIcon varies per action type. This is a deliberate UI/UX design choice.
Applied to files:
scribe/lib/scribe/ai/presentation/widgets/items/ai_scribe_menu_submenu_icon.dartscribe/lib/scribe/ai/presentation/widgets/items/ai_scribe_menu_icon.dartscribe/lib/scribe/ai/presentation/widgets/items/ai_scribe_menu_item.dart
🔇 Additional comments (13)
scribe/lib/scribe/ai/presentation/widgets/items/ai_scribe_menu_text.dart (1)
4-19: LGTM!Clean extraction of menu text rendering. The
Flexiblewrapper with single-line ellipsis overflow is appropriate for menu item labels in flex layouts.scribe/lib/scribe/ai/presentation/utils/modal/ai_scribe_suggestion_state_mixin.dart (2)
23-38: Good interactor registration pattern.The
Get.isRegisteredcheck beforeGet.findfollows the correct pattern for optional dependencies. Based on learnings, this avoids catching exceptions fromGet.findwhen the interactor might not be registered.
66-118: Well-structured state rendering and lifecycle management.The
ValueListenableBuilderpattern provides clean reactive UI updates. State widget factory methods (buildLoadingState,buildErrorState,buildSuccessState) are well-organized and properly dispose theValueNotifierindispose().scribe/lib/scribe/ai/presentation/widgets/modal/ai_scribe_suggestion_widget.dart (3)
35-48: Clean mixin integration with property delegation.The getter overrides provide a clear contract for the mixin to access widget properties. This pattern enables clean reuse of the state management logic across different widget implementations.
122-124: Good delegation to mixin for state content rendering.Replacing inline state handling with
buildStateContent(context)reduces duplication and centralizes the state rendering logic in the mixin.
157-165: Correct usage of mixin'ssuggestionStateproperty.The
_handleClickOutsidemethod properly accessessuggestionStatefrom the mixin to determine dismissibility.scribe/lib/scribe/ai/presentation/widgets/items/ai_scribe_menu_submenu_icon.dart (1)
6-23: LGTM!Clean extraction of submenu icon rendering. The RTL-aware
EdgeInsetsDirectionalpadding and consistent color filter application align with the codebase's icon styling patterns.scribe/lib/scribe/ai/presentation/widgets/items/ai_scribe_menu_icon.dart (1)
5-23: LGTM!Clean icon widget accepting a path string for per-action icon customization. The 72% alpha gray filter and RTL-aware padding are consistent with the codebase's styling patterns. Based on learnings, the varying
actionIconper action type is intentional.scribe/lib/scribe/ai/presentation/widgets/items/ai_scribe_menu_item.dart (3)
1-6: LGTM on imports and typedef.The imports are appropriate and the
OnHoverActiontypedef provides a clear signature for hover callbacks.
8-22: Well-structured widget API.The separation of
super.key(widget identity) anditemKey(container reference for layout calculations) is appropriate for scenarios where the menu item's position needs to be queried externally.
37-44: No changes needed — text overflow is already handled.
AiScribeMenuTextinternally wraps its content withFlexible, setsmaxLines: 1, and usesTextOverflow.ellipsis, so theRowlayout does not require an additionalExpandedwrapper. The text overflow concern is already addressed by the child widget.Likely an incorrect or invalid review comment.
scribe/lib/scribe.dart (2)
28-28: LGTM on mixin export.Adding the
AiScribeSuggestionStateMixinexport aligns with the PR objective to enable component reuse between desktop and mobile.
35-39: LGTM on new item widget exports.The new exports properly expose the extracted menu item components. The relocation from
context_menu/toitems/directory improves organization by separating reusable item widgets from context-menu-specific logic. Verification confirms the oldcontext_menu/ai_scribe_submenu_item.dartpath has been removed and no external imports of the old path exist in the codebase.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
scribe/lib/scribe/ai/presentation/utils/modal/ai_scribe_suggestion_state_mixin.dart (1)
65-87: Consider computinghasContentinside the builder.
hasContentis computed outside theValueListenableBuilder, so ifcontentchanges while only_suggestionStatetriggers a rebuild, the captured value may be stale. This is likely acceptable if content changes always trigger a full widget rebuild, but moving the computation inside the builder would be more robust.♻️ Optional: Compute hasContent inside builder
Widget buildStateContent( BuildContext context, ) { - final hasContent = content?.trim().isNotEmpty == true; - return ValueListenableBuilder<dartz.Either<Failure, Success>>( valueListenable: _suggestionState, builder: (_, stateValue, __) { return stateValue.fold( (failure) => buildErrorState(), (value) { if (value is GenerateAITextSuccess) { + final hasContent = content?.trim().isNotEmpty == true; return buildSuccessState( value.response.result, hasContent, ); } return buildLoadingState(); }, ); }, ); }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
scribe/lib/scribe/ai/presentation/utils/modal/ai_scribe_suggestion_state_mixin.dart
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-12-09T09:36:45.349Z
Learnt from: dab246
Repo: linagora/tmail-flutter PR: 4194
File: lib/features/manage_account/presentation/manage_account_dashboard_controller.dart:174-176
Timestamp: 2025-12-09T09:36:45.349Z
Learning: In Dart/Flutter projects using GetX, do not wrap getBinding<T>() calls in try/catch since they return null when not found. Only wrap Get.find<T>() calls in try/catch because they throw if a dependency is unavailable. When a binding is optional, consider checking Get.isRegistered<T>() or handling the null/exception path gracefully instead of blindly catching, and document the expectation for failure modes where a dependency may not be registered.
Applied to files:
scribe/lib/scribe/ai/presentation/utils/modal/ai_scribe_suggestion_state_mixin.dart
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
- GitHub Check: analyze-test (model)
- GitHub Check: analyze-test (email_recovery)
- GitHub Check: analyze-test (default)
- GitHub Check: analyze-test (contact)
- GitHub Check: Build web version and deploy
- GitHub Check: analyze-test (rule_filter)
- GitHub Check: analyze-test (fcm)
- GitHub Check: analyze-test (server_settings)
- GitHub Check: analyze-test (core)
- GitHub Check: analyze-test (forward)
🔇 Additional comments (3)
scribe/lib/scribe/ai/presentation/utils/modal/ai_scribe_suggestion_state_mixin.dart (3)
23-38: LGTM - Correct GetX dependency check pattern.The
Get.isRegistered<T>()check beforeGet.find<T>()correctly avoids exceptions when the interactor isn't registered. This follows the recommended pattern for optional GetX dependencies. Based on learnings.
40-63: Good defensive coding with mounted check.The
mountedcheck on line 57 correctly prevents state updates after widget disposal. The redundant null check for_interactoris appropriate sinceloadSuggestion()could be called from subclasses for retry functionality.
113-117: Proper cleanup of ValueNotifier.The
disposemethod correctly disposes_suggestionStatebefore callingsuper.dispose(), preventing potential memory leaks.
|
In Scribe desktop, the suggestion is displayed in a popup. In Scribe mobile, it will be displayed in a full screen modal. But it share the same logic about loading, success and error. So here I extract this logic and use it in the AI Scribe desktop to be able to use it later in AI Scribe mobile.
Scribe is always composed of menu (aka category) and submenu (aka action) : - submenu item UI was already in an UI only component. I moved it to items folder. - menu item UI was mixed in a component with desktop related UX. So I extract the UI to create an UI only component. I moved it also to items folder.
We will need a custom boxShadow for mobile version.
zatteo
force-pushed
the
feat/prepare-scribe-mobile
branch
from
b0236bf to
ffa3f81
Compare
3 participants
Summary by CodeRabbit
New Features
Improvements
✏️ Tip: You can customize this high-level summary in your review settings.