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 PR integrates AI Scribe functionality throughout the Composer module. Key changes include adding AIScribeInComposerMixin to ComposerController, implementing text selection monitoring in mobile and web editor views through callback propagation, introducing AIScribe UI components (button, menu, bar, suggestion dialog), establishing a centralized styling system for AIScribe, implementing dependency injection for AI services, adding localization strings across multiple languages, and providing a utility method to convert HTML editor content to plain text. The integration enables users to access AI-assisted text generation from the composer via full-text or selected-text workflows. Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
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.
|
|
This PR has been deployed to https://linagora.github.io/tmail-flutter/4197. |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (17)
scribe/pubspec.yaml (1)
27-28: GetX dependency additionAdding
get: 4.6.6here is reasonable for the AIScribe bindings/DI. Just confirm this version matches what the main app uses to avoid version skew across packages.lib/main/localizations/app_localizations.dart (1)
5440-5445:aiAssistantgetter wiring is correct; consider copy consistencyThe new getter correctly exposes the
aiAssistantkey for use in the UI. If you want strict consistency with other locales or marketing copy, you might consider capitalizing it as"AI Assistant"in the default string.scribe/lib/scribe/ai/presentation/widgets/ai_scribe_bar.dart (1)
1-89: AIScribeBar state & lifecycle look solid; minor UX tweak optional
- Controller setup/disposal and
_isButtonEnabledlogic are correct, and trimming before send avoids empty/whitespace prompts.- Button and
onSubmittedboth funnel through_onSendPressed, so behavior is consistent.Optional: you could add
textInputAction: TextInputAction.sendto theTextFieldto better align the keyboard affordance with the send action.scribe/lib/scribe/ai/presentation/widgets/ai_scribe_button.dart (1)
18-26: Consider using theme-aware colors for dark mode compatibility.The
backgroundColor: Colors.whiteis hardcoded. If the app supports dark mode or plans to, consider extracting this toAIScribeColorsor using theme-based colors for consistency.- backgroundColor: Colors.white, + backgroundColor: AIScribeColors.scribeButtonBackground,scribe/lib/scribe/ai/presentation/widgets/ai_scribe_suggestion.dart (2)
196-198: Add user feedback after copying to clipboard.The copy operation silently copies text without any visual confirmation. Users may not know if the copy succeeded.
onPressed: () { Clipboard.setData(ClipboardData(text: suggestion)); + ScaffoldMessenger.of(context).showSnackBar( + SnackBar(content: Text(ScribeLocalizations.of(context)!.copiedToClipboard)), + ); },
147-150: Redundant error display - showing both generic and raw error.The error state currently shows both a generic "Failed to generate" message and the raw error string. If you keep the raw error, it duplicates information; if you sanitize it (per earlier suggestion), this second text block becomes unnecessary.
lib/features/composer/presentation/widgets/mobile/mobile_editor_widget.dart (2)
53-56: Add defensive type checking for JavaScript callback data.The cast
args[0] as Map<dynamic, dynamic>will throw aTypeErrorif the JavaScript side sends unexpected data. Consider adding type checking.if (args.isNotEmpty) { - final data = args[0] as Map<dynamic, dynamic>; - handleSelectionChange(data); + final rawData = args[0]; + if (rawData is Map<dynamic, dynamic>) { + handleSelectionChange(rawData); + } }
65-69: Setting_editorApi = nulldoesn't remove the JavaScript handler.The JavaScript handler registered via
addJavaScriptHandlerpersists until the webview is destroyed. While themountedcheck prevents processing after disposal, consider whether explicit handler removal is needed for cleaner lifecycle management.lib/features/composer/presentation/composer_view.dart (2)
567-571: Hardcoded padding value should reference the actual style constant.The
editorHorizontalPadding = 12.0should referenceComposerStyle.mobileEditorPadding.horizontalor the actual constant to stay in sync if the padding changes.- // Account for the horizontal padding around the editor - const editorHorizontalPadding = 12.0; + // Account for the horizontal padding around the editor + final editorHorizontalPadding = ComposerStyle.mobileEditorPadding.left; return PositionedDirectional( start: coordinates.dx + editorHorizontalPadding,
239-270: Duplicated Stack/AIScribe overlay structure between mobile and tablet layouts.The Stack wrapping the editor with AIScribe selection button is duplicated. Consider extracting this into a shared builder method to reduce duplication and ensure consistency.
Widget _buildEditorWithAIScribeOverlay({ required BuildContext context, required Widget editorView, required Widget? viewEntireMessageWidget, }) { return Stack( children: [ Column( children: [ editorView, viewEntireMessageWidget ?? const SizedBox.shrink(), SizedBox(height: MediaQuery.viewInsetsOf(context).bottom + 64), ], ), _buildAIScribeSelectionButton(context), ], ); }Also applies to: 428-459
scribe/lib/scribe/ai/presentation/bindings/ai_scribe_bindings.dart (1)
19-27: Consider documenting dependency lifecycle expectations if instances may be disposed during app runtime.GetX's
lazyPutwithoutfenix: true(the default) removes the factory after instance disposal, preventing automatic recreation on subsequentGet.find()calls. If your application's SmartManagement disposes these dependencies (AIDataSourceImpl, AIDataSource, etc.) and they need to be recreated, addfenix: trueto preserve the factory for resurrection. Alternatively, usepermanent: trueif they should never be auto-disposed. Without visibility into the disposal patterns, clarify the intended lifecycle for these bindings.scribe/lib/scribe/ai/presentation/widgets/ai_scribe_menu.dart (2)
207-248: Submenu panel lacks dismiss-on-outside-tap behavior.The
_SubmenuPaneldisplays as a positioned overlay but doesn't handle taps outside the submenu to dismiss it. Users may expect tapping outside to close the submenu.Consider wrapping with a full-screen
GestureDetectorto capture outside taps:return Stack( children: [ + Positioned.fill( + child: GestureDetector( + onTap: widget.onDismiss, + behavior: HitTestBehavior.translucent, + child: const SizedBox.expand(), + ), + ), Positioned( left: adjustedLeft, top: adjustedTop,
118-128: Silent no-op when category has no actions.If a category without a submenu has an empty
actionslist,onTapdoes nothing. Consider adding a guard or logging for this edge case.scribe/lib/scribe/ai/presentation/widgets/ai_scribe.dart (2)
76-92: Consider extracting duplicated positioned dialog wrapper.The pattern of wrapping content with
PointerInterceptor,GestureDetector, andPositionedinside aStackis repeated for both dialog phases. A helper widget could reduce duplication.Also applies to: 137-153
201-216: Throwing generic exceptions loses error context.Using
throw Exception(...)discards typed failure information. Consider propagating the original failure or using a custom exception type for better error handling upstream.lib/features/composer/presentation/composer_view_web.dart (1)
563-564: Minor: RepeatedAIConfig.isAiEnabledchecks could be consolidated.The same ternary pattern appears twice. Consider caching the result or extracting to a local variable if the config is static.
Also applies to: 839-840
lib/features/composer/presentation/composer_controller.dart (1)
872-884: HTML-to-text conversion has limited entity support.The manual entity replacement handles only 6 common HTML entities. Consider using a dedicated HTML parsing library for more robust conversion, especially for user-generated content that may contain numeric entities (
{) or other named entities.+import 'package:html/parser.dart' as html_parser; + String convertHtmlContentToTextContent(String htmlContent) { - String textContent = htmlContent.replaceAll(RegExp(r'<[^>]*>'), ''); - - textContent = textContent - .replaceAll(' ', ' ') - .replaceAll('&', '&') - .replaceAll('<', '<') - .replaceAll('>', '>') - .replaceAll('"', '"') - .replaceAll(''', "'"); - - return textContent.trim(); + final document = html_parser.parse(htmlContent); + return document.body?.text ?? ''; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
scribe/pubspec.lockis excluded by!**/*.lock
📒 Files selected for processing (26)
lib/features/composer/presentation/composer_bindings.dart(2 hunks)lib/features/composer/presentation/composer_controller.dart(7 hunks)lib/features/composer/presentation/composer_view.dart(6 hunks)lib/features/composer/presentation/composer_view_web.dart(11 hunks)lib/features/composer/presentation/view/mobile/mobile_editor_view.dart(3 hunks)lib/features/composer/presentation/view/web/web_editor_view.dart(8 hunks)lib/features/composer/presentation/widgets/mobile/mobile_editor_widget.dart(2 hunks)lib/features/composer/presentation/widgets/mobile/tablet_bottom_bar_composer_widget.dart(3 hunks)lib/features/composer/presentation/widgets/web/bottom_bar_composer_widget.dart(3 hunks)lib/features/composer/presentation/widgets/web/web_editor_widget.dart(8 hunks)lib/l10n/intl_fr.arb(1 hunks)lib/l10n/intl_messages.arb(2 hunks)lib/l10n/intl_ru.arb(1 hunks)lib/l10n/intl_vi.arb(1 hunks)lib/main/bindings/main_bindings.dart(2 hunks)lib/main/localizations/app_localizations.dart(1 hunks)scribe/lib/scribe.dart(1 hunks)scribe/lib/scribe/ai/l10n/intl_messages.arb(1 hunks)scribe/lib/scribe/ai/presentation/bindings/ai_scribe_bindings.dart(1 hunks)scribe/lib/scribe/ai/presentation/styles/ai_scribe_styles.dart(1 hunks)scribe/lib/scribe/ai/presentation/widgets/ai_scribe.dart(1 hunks)scribe/lib/scribe/ai/presentation/widgets/ai_scribe_bar.dart(1 hunks)scribe/lib/scribe/ai/presentation/widgets/ai_scribe_button.dart(1 hunks)scribe/lib/scribe/ai/presentation/widgets/ai_scribe_menu.dart(1 hunks)scribe/lib/scribe/ai/presentation/widgets/ai_scribe_suggestion.dart(1 hunks)scribe/pubspec.yaml(1 hunks)
🧰 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.dartlib/features/composer/presentation/widgets/web/bottom_bar_composer_widget.dartlib/main/localizations/app_localizations.dartscribe/lib/scribe/ai/presentation/widgets/ai_scribe_bar.dartlib/features/composer/presentation/view/web/web_editor_view.dartscribe/lib/scribe/ai/presentation/widgets/ai_scribe_suggestion.dartlib/features/composer/presentation/composer_bindings.dartscribe/lib/scribe/ai/presentation/widgets/ai_scribe_button.dartlib/features/composer/presentation/widgets/mobile/tablet_bottom_bar_composer_widget.dartlib/features/composer/presentation/view/mobile/mobile_editor_view.dartscribe/lib/scribe/ai/presentation/widgets/ai_scribe_menu.dartscribe/lib/scribe/ai/presentation/widgets/ai_scribe.dartlib/features/composer/presentation/widgets/web/web_editor_widget.dartscribe/lib/scribe/ai/presentation/bindings/ai_scribe_bindings.dartscribe/lib/scribe/ai/presentation/styles/ai_scribe_styles.dartlib/main/bindings/main_bindings.dartlib/features/composer/presentation/widgets/mobile/mobile_editor_widget.dartlib/features/composer/presentation/composer_controller.dartlib/features/composer/presentation/composer_view_web.dartlib/features/composer/presentation/composer_view.dart
📚 Learning: 2025-12-09T12:47:45.861Z
Learnt from: zatteo
Repo: linagora/tmail-flutter PR: 4196
File: scribe/lib/scribe/ai/data/datasource_impl/ai_datasource_impl.dart:34-35
Timestamp: 2025-12-09T12:47:45.861Z
Learning: In the tmail-flutter repository, avoid suggesting replacing DioError with DioException unless there is a codebase-wide migration. This pattern applies to all Dart files; when reviewing, only propose a DioError-safe change (e.g., compatible error handling or conversion) unless a global migration is in place. Ensure consistency of DioError usage across modules and flag any deviations for a repo-wide decision.
Applied to files:
scribe/lib/scribe.dartlib/features/composer/presentation/widgets/web/bottom_bar_composer_widget.dartlib/main/localizations/app_localizations.dartscribe/lib/scribe/ai/presentation/widgets/ai_scribe_bar.dartlib/features/composer/presentation/view/web/web_editor_view.dartscribe/lib/scribe/ai/presentation/widgets/ai_scribe_suggestion.dartlib/features/composer/presentation/composer_bindings.dartscribe/lib/scribe/ai/presentation/widgets/ai_scribe_button.dartlib/features/composer/presentation/widgets/mobile/tablet_bottom_bar_composer_widget.dartlib/features/composer/presentation/view/mobile/mobile_editor_view.dartscribe/lib/scribe/ai/presentation/widgets/ai_scribe_menu.dartscribe/lib/scribe/ai/presentation/widgets/ai_scribe.dartlib/features/composer/presentation/widgets/web/web_editor_widget.dartscribe/lib/scribe/ai/presentation/bindings/ai_scribe_bindings.dartscribe/lib/scribe/ai/presentation/styles/ai_scribe_styles.dartlib/main/bindings/main_bindings.dartlib/features/composer/presentation/widgets/mobile/mobile_editor_widget.dartlib/features/composer/presentation/composer_controller.dartlib/features/composer/presentation/composer_view_web.dartlib/features/composer/presentation/composer_view.dart
🪛 GitHub Actions: Analyze and test
scribe/lib/scribe.dart
[error] 1-1: The process '/opt/hostedtoolcache/flutter/bin/cache/dart-sdk/bin/dart' failed with exit code 3
lib/features/composer/presentation/widgets/web/bottom_bar_composer_widget.dart
[error] 1-1: The process '/opt/hostedtoolcache/flutter/bin/cache/dart-sdk/bin/dart' failed with exit code 3
lib/main/localizations/app_localizations.dart
[error] 1-1: The process '/opt/hostedtoolcache/flutter/bin/cache/dart-sdk/bin/dart' failed with exit code 3
scribe/lib/scribe/ai/presentation/widgets/ai_scribe_bar.dart
[error] 1-1: The process '/opt/hostedtoolcache/flutter/bin/cache/dart-sdk/bin/dart' failed with exit code 3
lib/features/composer/presentation/view/web/web_editor_view.dart
[error] 1-1: The process '/opt/hostedtoolcache/flutter/bin/cache/dart-sdk/bin/dart' failed with exit code 3
scribe/lib/scribe/ai/presentation/widgets/ai_scribe_suggestion.dart
[error] 1-1: The process '/opt/hostedtoolcache/flutter/bin/cache/dart-sdk/bin/dart' failed with exit code 3
lib/features/composer/presentation/composer_bindings.dart
[error] 1-1: The process '/opt/hostedtoolcache/flutter/bin/cache/dart-sdk/bin/dart' failed with exit code 3
scribe/lib/scribe/ai/presentation/widgets/ai_scribe_button.dart
[error] 1-1: The process '/opt/hostedtoolcache/flutter/bin/cache/dart-sdk/bin/dart' failed with exit code 3
lib/features/composer/presentation/widgets/mobile/tablet_bottom_bar_composer_widget.dart
[error] 1-1: The process '/opt/hostedtoolcache/flutter/bin/cache/dart-sdk/bin/dart' failed with exit code 3
lib/features/composer/presentation/view/mobile/mobile_editor_view.dart
[error] 1-1: The process '/opt/hostedtoolcache/flutter/bin/cache/dart-sdk/bin/dart' failed with exit code 3
scribe/lib/scribe/ai/presentation/widgets/ai_scribe_menu.dart
[error] 1-1: The process '/opt/hostedtoolcache/flutter/bin/cache/dart-sdk/bin/dart' failed with exit code 3
scribe/lib/scribe/ai/presentation/widgets/ai_scribe.dart
[warning] 208-208: UNNECESSARY_NULL_COMPARISON: The operand can't be 'null', so the condition is always 'false'.
lib/features/composer/presentation/widgets/web/web_editor_widget.dart
[error] 1-1: The process '/opt/hostedtoolcache/flutter/bin/cache/dart-sdk/bin/dart' failed with exit code 3
scribe/lib/scribe/ai/presentation/bindings/ai_scribe_bindings.dart
[error] 1-1: The process '/opt/hostedtoolcache/flutter/bin/cache/dart-sdk/bin/dart' failed with exit code 3
scribe/lib/scribe/ai/presentation/styles/ai_scribe_styles.dart
[error] 1-1: The process '/opt/hostedtoolcache/flutter/bin/cache/dart-sdk/bin/dart' failed with exit code 3
lib/main/bindings/main_bindings.dart
[error] 1-1: The process '/opt/hostedtoolcache/flutter/bin/cache/dart-sdk/bin/dart' failed with exit code 3
lib/features/composer/presentation/widgets/mobile/mobile_editor_widget.dart
[error] 1-1: The process '/opt/hostedtoolcache/flutter/bin/cache/dart-sdk/bin/dart' failed with exit code 3
lib/features/composer/presentation/composer_controller.dart
[error] 1-1: The process '/opt/hostedtoolcache/flutter/bin/cache/dart-sdk/bin/dart' failed with exit code 3
lib/features/composer/presentation/composer_view_web.dart
[error] 1-1: The process '/opt/hostedtoolcache/flutter/bin/cache/dart-sdk/bin/dart' failed with exit code 3
lib/features/composer/presentation/composer_view.dart
[error] 1-1: The process '/opt/hostedtoolcache/flutter/bin/cache/dart-sdk/bin/dart' failed with exit code 3
⏰ 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). (1)
- GitHub Check: Build web version and deploy
🔇 Additional comments (31)
scribe/lib/scribe/ai/l10n/intl_messages.arb (1)
2-2: Timestamp update is fine
@@last_modifiedlooks consistent with normal ARB metadata updates; no functional impact.lib/l10n/intl_fr.arb (1)
5476-5482: FrenchaiAssistanttranslation looks goodKey/value
"aiAssistant": "Assistant IA"and its metadata follow the existing ARB pattern and should integrate cleanly with the new localization getter.scribe/lib/scribe.dart (1)
11-11: Export path is correct and file is properly integratedThe file
scribe/ai/data/config/ai_config.dartexists and is already used internally byai_datasource_impl.dart. The export follows the established pattern for data layer exports in this file and poses no buildability concerns.lib/l10n/intl_messages.arb (1)
2-2: NewaiAssistantlocalization correctly integratedThe
@@last_modifiedtimestamp update and the new"aiAssistant"entry with@aiAssistantmetadata follow the existing ARB structure and naming conventions. The corresponding getter is properly wired inAppLocalizations(lib/main/localizations/app_localizations.dart, line 5440) and is already being used in the UI components at lib/features/composer/presentation/widgets/web/bottom_bar_composer_widget.dart and lib/features/composer/presentation/widgets/mobile/tablet_bottom_bar_composer_widget.dart. Translations are present in multiple language files (Vietnamese, Russian, French).lib/l10n/intl_ru.arb (1)
5494-5500: LGTM!The new
aiAssistantlocalization entry follows the established ARB format and the Russian translation "Помощник ИИ" is appropriate for "AI Assistant".lib/features/composer/presentation/widgets/web/web_editor_widget.dart (5)
81-81: Mixin integration looks correct.The state class properly mixes in
TextSelectionMixinand overrides theonSelectionChangedgetter to forward to the widget's callback. This is a clean pattern for sharing selection handling logic.Also applies to: 96-97
115-126: Selection change listener follows the established drop listener pattern.The implementation mirrors the existing
_dropListenersetup, which is good for consistency. The listener properly:
- Checks for
MessageEventtype- Decodes JSON data
- Validates the message name before calling
handleSelectionChange
146-149: Proper cleanup prevents memory leaks.The dispose method correctly removes the selection change listener and nullifies the reference, mirroring the cleanup pattern used for the drop listener.
218-222: Registration guard prevents duplicate listener registration.The
_selectionChangeListenerRegisteredflag ensures the JavaScript listener is only registered once, consistent with the drop listener pattern.
9-9: Remove this review comment.The
TextSelectionMixinimport is correctly implemented. The mixin exists at the referenced path and properly exports thehandleSelectionChangemethod andonSelectionChangedgetter. The pipeline failure is unrelated to this import.Likely an incorrect or invalid review comment.
lib/features/composer/presentation/view/mobile/mobile_editor_view.dart (2)
22-22: LGTM - Clean callback pass-through.The
onTextSelectionChangedcallback is properly declared and accepted via constructor, following the existing pattern for other callbacks likeonEditorContentHeightChanged.Also applies to: 31-31
142-153: Callback correctly wired to MobileEditorWidget.The
buildEditormethod properly passes theonTextSelectionChangedcallback toMobileEditorWidget, enabling text selection events to propagate up to the composer.lib/features/composer/presentation/composer_bindings.dart (2)
77-77: No action needed — the scribe package is properly configured.The
scribepackage is declared as a local path dependency inpubspec.yaml(path: scribe), and the import path correctly references the existing file atscribe/lib/scribe/ai/domain/usecases/generate_ai_text_interactor.dart. This is not an external dependency issue.
342-342:GenerateAITextInteractoris registered globally — this is intentional and safe.The global registration (without
tag: composerId) is correct.GenerateAITextInteractoris registered inAIScribeBindingsand initialized viamain_bindings.dartbeforeComposerBindingsloads. No defensive checks are needed; the pattern is consistent with the codebase and guarantees availability.Likely an incorrect or invalid review comment.
lib/l10n/intl_vi.arb (1)
5477-5488: NewaiAssistantVI localization looks correctKey and metadata are consistent with the existing ARB structure, and
"Trợ lý AI"is an appropriate translation for the assistant label.lib/features/composer/presentation/widgets/web/bottom_bar_composer_widget.dart (1)
34-36: AIScribe button integration into bottom bar is consistent and safe
- Adding
onOpenAIScribeas nullable and gating the button withif (onOpenAIScribe != null)keeps the API backward‑compatible and avoids null issues when invoking the callback.- Using
AbsorbPointerand icon color changes tied toisCodeViewEnabledmatches the existing behavior of other formatting actions, so the UX remains coherent.- Passing
aiScribeButtonKeythrough as the widget key is a reasonable way to expose this control to parents that need positioning/measurement.No issues from a correctness standpoint.
Also applies to: 59-61, 134-152
lib/features/composer/presentation/view/web/web_editor_view.dart (1)
39-39: LGTM!The
onTextSelectionChangedcallback is correctly added as an optional field and consistently forwarded to allWebEditorWidgetinstantiations across all code paths (compose, edit draft, reply/forward, and default cases).Also applies to: 63-63, 96-96
lib/main/bindings/main_bindings.dart (1)
25-25: LGTM!
AIScribeBindingsis correctly integrated into the main dependency graph. The placement after network/session bindings and before platform-specific bindings is appropriate.lib/features/composer/presentation/widgets/mobile/tablet_bottom_bar_composer_widget.dart (1)
78-89: LGTM!The AI Scribe button is conditionally rendered with proper null checking and consistent styling. The spread operator pattern (
if ... [...,]) cleanly integrates the optional button into the row.scribe/lib/scribe/ai/presentation/bindings/ai_scribe_bindings.dart (1)
25-27: Potential issue:Get.findinsidelazyPutbuilder may throw if dependency isn't resolved yet.The
_bindingsDataSourceusesGet.find<AIDataSourceImpl>()inside the lazyPut builder. Since lazyPut defers creation until first access, ifAIDataSourceis requested beforeAIDataSourceImplis ever accessed, this could fail. The current call order independencies()registers them correctly, but the actual instantiation happens lazily.This should work correctly as long as consumers only access
GenerateAITextInteractor(which cascades the resolution), but verify the access patterns are correct.Based on learnings,
Get.find<T>()throws if a dependency is unavailable.lib/features/composer/presentation/composer_view.dart (1)
473-474: LGTM!The AI Scribe props are correctly passed conditionally based on
AIConfig.isAiEnabled, keeping the feature properly gated.scribe/lib/scribe/ai/presentation/widgets/ai_scribe_menu.dart (1)
28-46: LGTM! Proper overlay lifecycle management.The overlay cleanup in
dispose()prevents memory leaks, and the_removeSubmenu()helper correctly nullifies the reference after removal.scribe/lib/scribe/ai/presentation/widgets/ai_scribe.dart (1)
161-192: LGTM! Edge-fitting logic is well-implemented.The
_calculateModalPositionfunction correctly handles horizontal and vertical screen boundary constraints with appropriate padding.lib/features/composer/presentation/composer_view_web.dart (2)
271-272: LGTM! Consistent text selection callback wiring.The
onTextSelectionChangedcallback is correctly wired across all three responsive layouts (mobile, desktop, tablet), ensuring consistent AI Scribe behavior.Also applies to: 515-516, 792-793
960-998: LGTM! AIScribe selection button implementation is sound.The button correctly:
- Guards on
AIConfig.isAiEnabled- Uses
Obxfor reactive updates- Handles null coordinates gracefully
- Uses
Builderto obtain button context for position calculationscribe/lib/scribe/ai/presentation/styles/ai_scribe_styles.dart (2)
1-85: LGTM! Well-organized design token system.The centralized styling approach with clear categorization (colors, shadows, text styles, button styles, sizes) promotes consistency and maintainability across AIScribe UI components.
2-3: Remove the verification request—both imports are actively used.The imports
color_extension.dartandtheme_utils.dartare necessary and actively used throughout the file:
AppColoris referenced 5 times (lines 28, 29, 31, 35, 39)ThemeUtilsis referenced 5 times (lines 28, 29, 30, 31, 35)No action is needed on these imports.
lib/features/composer/presentation/composer_controller.dart (4)
162-164: LGTM! Reactive state for text selection tracking.Using
Rxn<String>andRxn<Offset>with GetX observables enables reactive UI updates when selection changes.
909-928: LGTM! Proper context.mounted check after async gap.The
context.mountedcheck at line 918 correctly guards against using a stale context after the asyncgetTextOnlyContentInEditor()call.
946-964: LGTM! Selection state management is well-structured.The
handleTextSelectionmethod cleanly handles both the selection present and absent cases, updating all three reactive fields consistently.
899-907: Simple newline-to-br conversion may not preserve all formatting.Replacing
\nwith<br>works for plain text but may produce unexpected results if the AI generates content with existing HTML. Consider whether the AI response should be sanitized or escaped first.
scribe/lib/scribe/ai/presentation/widgets/ai_scribe_suggestion.dart
Outdated
Show resolved
Hide resolved
- Button : the button with sparkle icon to display AI scribe - Menu : the menu where you can select the action (eg: translate) - Bar : the input where you can write a custom action - Suggestion : the wrapper of the result of the LLM
Editor exposes html content (so with html tags) and we do not want to send them to AI backend
|
| TMailButtonWidget.fromIcon( | ||
| key: aiScribeButtonKey, | ||
| icon: imagePaths.icSparkle, | ||
| borderRadius: TabletBottomBarComposerWidgetStyle.iconRadius, | ||
| padding: TabletBottomBarComposerWidgetStyle.iconPadding, | ||
| iconSize: TabletBottomBarComposerWidgetStyle.iconSize, | ||
| tooltipMessage: AppLocalizations.of(context).aiAssistant, | ||
| onTapActionCallback: onOpenAIScribe!, | ||
| ), |
There was a problem hiding this comment.
We should create a separate widget for it so we can reuse it in multiple places.
There was a problem hiding this comment.
Sure? I don't know, except mutualizing the icon, it does not mutualize a lot because it must have the style of bottom bar buttons.
There was a problem hiding this comment.
What if another feature needs to use this button? When we need to integrate it into other apps, do we call the widget again with so many parameters like this?
| final VoidCallback saveAsTemplateAction; | ||
| final VoidCallback onOpenInsertLink; | ||
| final VoidCallback? onOpenAIScribe; | ||
| final GlobalKey? aiScribeButtonKey; |
There was a problem hiding this comment.
IMO, the aiScribe widget should hold the GlobalKey so that it can manage its own key, preventing widgets holding keys from causing memory leaks.
There was a problem hiding this comment.
@dab246 Who is "the aiScribe widget"? In my case I need it at the top in the controller to position the scribe menu according the position of the bottom bar button.
There was a problem hiding this comment.
If you create a GlobalKey here, when you want to use the aiScribeButtonKey button elsewhere, you'll have to create a GlobalKey for the aiScribeButtonKey again. And if the aiScribeButtonKey isn't rendered, the GlobalKey will still exist in the controller and won't be released, potentially leading to a memory leak.
There was a problem hiding this comment.
Understood. WDYT about this totally different approach that removes the aiScribeButtonKey ? #4207
| if (event is MessageEvent) { | ||
| final data = jsonDecode(event.data); | ||
|
|
||
| if (data['name'] == HtmlUtils.registerSelectionChangeListener.name) { | ||
| handleSelectionChange(data); | ||
| } | ||
| } |
| } | ||
| }; | ||
| if (_selectionChangeListener != null) { | ||
| window.addEventListener("message", _selectionChangeListener!); |
There was a problem hiding this comment.
We should listen for the message event in one place to avoid calling it multiple times. We could reuse window.addEventListener("message", _dropListener!);, but it should be modified appropriately for general use, as we might have multiple features that need to be handled in this function.
| final selectedText = Rxn<String>(); | ||
| final hasTextSelection = false.obs; | ||
| final textSelectionCoordinates = Rxn<Offset>(); |
There was a problem hiding this comment.
IMO, these values are always set together, so we don't need too many values to observe here. If you absolutely need these three values, you should create an object to contain them and just observe this object to update the UI.
| String convertHtmlContentToTextContent(String htmlContent) { | ||
| String textContent = htmlContent.replaceAll(RegExp(r'<[^>]*>'), ''); | ||
|
|
||
| textContent = textContent | ||
| .replaceAll(' ', ' ') | ||
| .replaceAll('&', '&') | ||
| .replaceAll('<', '<') | ||
| .replaceAll('>', '>') | ||
| .replaceAll('"', '"') | ||
| .replaceAll(''', "'"); | ||
|
|
||
| return textContent.trim(); | ||
| } |
There was a problem hiding this comment.
It should be placed in the util class within the core module for reuse.
| static String convertHtmlContentToTextContent(String htmlContent) { | ||
| String textContent = htmlContent.replaceAll(RegExp(r'<[^>]*>'), ''); | ||
|
|
||
| textContent = textContent | ||
| .replaceAll(' ', ' ') | ||
| .replaceAll('&', '&') | ||
| .replaceAll('<', '<') | ||
| .replaceAll('>', '>') | ||
| .replaceAll('"', '"') | ||
| .replaceAll(''', "'"); | ||
|
|
There was a problem hiding this comment.
What do you think of this piece of Flutter code? Should I roast it?
🔍 What’s good
- The function is simple and readable.
- For very basic HTML, it might work.
But…
⚠️ The issues (yes, you can roast this code 😄)
1. Using a regex to strip HTML tags…
This is always fragile.
Problems you will eventually hit:
- Tags with attributes containing
> - Comments, scripts, styles
- Broken or unusual HTML
- Nested content where regex fails
👉 There’s a reason HTML parsers exist.
2. Only a few HTML entities are handled
There are thousands of HTML entities.
Your code handles maybe 5.
Anything like é, ©, …, etc. will remain unchanged.
3. No handling of <br>, <p>, etc.
All line breaks will disappear.
Your output becomes a single messy block:
"Line1Line2Line3"
4. Whitespace is not normalized
HTML often contains multiple spaces, newlines, indentation.
Your output might be weirdly spaced.
✔️ A proper solution in Flutter
Use the html package:
import 'package:html/parser.dart' as html;
String convertHtmlContentToTextContent(String htmlContent) {
final document = html.parse(htmlContent);
final String parsedString = document.body?.text ?? '';
return parsedString.trim();
}Why this is better:
- Correctly strips tags
- Correctly decodes all HTML entities (
é→é) - More robust
- Easier to maintain
🧠 Conclusion
Your current code works only for very simple HTML and is not reliable for real-world HTML content.
👉 Strong recommendation: use an HTML parser instead.
Before sending input to the LLM, we clean the composer input to remove html tags. We relied on regex that we not complete enough and could be bypassed easily. So let's use the html parser available in Dart instead. I also added some tests.
chibenwa
left a comment
There was a problem hiding this comment.
Still many changesout of lib/scribe but I'm unsure committing to more refactorings here on this specific feature is worth it.
|
@coderabbitai please review again |
|
@dab246, I'll review the changes in this PR. ✅ Actions performedReview triggered.
|
core/lib/utils/string_convert.dart
Outdated
| const blockTags = [ | ||
| 'p', | ||
| 'div', | ||
| 'li', | ||
| 'section', | ||
| 'article', | ||
| 'header', | ||
| 'footer', | ||
| 'h1','h2','h3','h4','h5','h6' | ||
| ]; |
There was a problem hiding this comment.
I did a more complete test by doing a lot of thing in the composer. As a result, I added blockquote in the composer but otherwise it is enough. I also added more tests to demonstrate it.
See 3393df3
core/lib/utils/string_convert.dart
Outdated
| for (final tag in blockTags) { | ||
| document.querySelectorAll(tag).forEach((element) { | ||
| element.append(Text('\n')); | ||
| }); | ||
| } |
There was a problem hiding this comment.
IMO, this process takes a considerable amount of time.
There was a problem hiding this comment.
Good point. I updated the behavior to use a single querySelecterAll with a comma nested selector in 3393df3 which must be better.
| group('StringConvert.convertHtmlContentToTextContent', () { | ||
| test('should preserve line breaks between paragraphs', () { | ||
| const htmlContent = '<p>First paragraph</p><p>Second paragraph</p><p>Third paragraph</p>'; | ||
| final result = StringConvert.convertHtmlContentToTextContent(htmlContent); | ||
|
|
||
| expect(result, 'First paragraph\nSecond paragraph\nThird paragraph'); | ||
| }); | ||
|
|
||
| test('should preserve line breaks for br', () { | ||
| const htmlContent = '<p>First paragraph</p><div><br></div><p>Second paragraph</p>'; | ||
| final result = StringConvert.convertHtmlContentToTextContent(htmlContent); | ||
|
|
||
| expect(result, 'First paragraph\n\nSecond paragraph'); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
scribe/lib/scribe/ai/presentation/widgets/ai_scribe.dart (1)
209-228: LGTM – Previous review addressed.The unnecessary null check flagged in the previous review has been removed. The current implementation correctly returns
success.response.resultdirectly forGenerateAITextSuccess, as the type system guaranteesresultis non-nullable.
🧹 Nitpick comments (1)
lib/features/composer/presentation/widgets/mobile/tablet_bottom_bar_composer_widget.dart (1)
78-89: Consider extracting the AI button to a separate reusable widget.Based on past review comments, there's a suggestion to extract the AI button into a dedicated widget for reusability across mobile and web bottom bars. This would reduce code duplication and simplify maintenance.
Per the past discussion, the team may defer this refactor, but it's worth considering for consistency with other bottom bar buttons.
Based on past review comments, this was previously discussed but not yet addressed.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
scribe/pubspec.lockis excluded by!**/*.lock
📒 Files selected for processing (36)
core/lib/utils/string_convert.dart(2 hunks)core/test/utils/string_convert_test.dart(1 hunks)lib/features/composer/presentation/composer_controller.dart(6 hunks)lib/features/composer/presentation/composer_view.dart(6 hunks)lib/features/composer/presentation/composer_view_web.dart(11 hunks)lib/features/composer/presentation/mixin/ai_scribe_in_composer_mixin.dart(1 hunks)lib/features/composer/presentation/view/mobile/mobile_editor_view.dart(3 hunks)lib/features/composer/presentation/view/web/web_editor_view.dart(8 hunks)lib/features/composer/presentation/widgets/mixins/text_selection_mixin.dart(1 hunks)lib/features/composer/presentation/widgets/mobile/mobile_editor_widget.dart(2 hunks)lib/features/composer/presentation/widgets/mobile/tablet_bottom_bar_composer_widget.dart(3 hunks)lib/features/composer/presentation/widgets/web/bottom_bar_composer_widget.dart(3 hunks)lib/features/composer/presentation/widgets/web/web_editor_widget.dart(7 hunks)lib/l10n/intl_fr.arb(1 hunks)lib/l10n/intl_messages.arb(2 hunks)lib/l10n/intl_ru.arb(1 hunks)lib/l10n/intl_vi.arb(1 hunks)lib/main/bindings/main_bindings.dart(2 hunks)lib/main/localizations/app_localizations.dart(1 hunks)scribe/lib/scribe.dart(1 hunks)scribe/lib/scribe/ai/l10n/intl_en.arb(0 hunks)scribe/lib/scribe/ai/l10n/intl_fr.arb(0 hunks)scribe/lib/scribe/ai/l10n/intl_messages.arb(1 hunks)scribe/lib/scribe/ai/l10n/intl_ru.arb(0 hunks)scribe/lib/scribe/ai/l10n/intl_vi.arb(0 hunks)scribe/lib/scribe/ai/localizations/scribe_localizations.dart(0 hunks)scribe/lib/scribe/ai/presentation/bindings/ai_scribe_bindings.dart(1 hunks)scribe/lib/scribe/ai/presentation/styles/ai_scribe_styles.dart(1 hunks)scribe/lib/scribe/ai/presentation/widgets/ai_scribe.dart(1 hunks)scribe/lib/scribe/ai/presentation/widgets/ai_scribe_bar.dart(1 hunks)scribe/lib/scribe/ai/presentation/widgets/ai_scribe_button.dart(1 hunks)scribe/lib/scribe/ai/presentation/widgets/ai_scribe_menu.dart(1 hunks)scribe/lib/scribe/ai/presentation/widgets/ai_scribe_suggestion.dart(1 hunks)scribe/pubspec.yaml(1 hunks)scribe/test/scribe/ai/presentation/widgets/ai_scribe_test.dart(1 hunks)test/features/composer/presentation/composer_controller_test.dart(4 hunks)
💤 Files with no reviewable changes (5)
- scribe/lib/scribe/ai/l10n/intl_en.arb
- scribe/lib/scribe/ai/localizations/scribe_localizations.dart
- scribe/lib/scribe/ai/l10n/intl_ru.arb
- scribe/lib/scribe/ai/l10n/intl_fr.arb
- scribe/lib/scribe/ai/l10n/intl_vi.arb
✅ Files skipped from review due to trivial changes (1)
- lib/features/composer/presentation/composer_view.dart
🚧 Files skipped from review as they are similar to previous changes (14)
- scribe/pubspec.yaml
- scribe/lib/scribe/ai/l10n/intl_messages.arb
- lib/main/bindings/main_bindings.dart
- lib/features/composer/presentation/view/web/web_editor_view.dart
- lib/features/composer/presentation/view/mobile/mobile_editor_view.dart
- lib/l10n/intl_messages.arb
- lib/features/composer/presentation/widgets/web/bottom_bar_composer_widget.dart
- scribe/lib/scribe.dart
- scribe/lib/scribe/ai/presentation/widgets/ai_scribe_suggestion.dart
- scribe/lib/scribe/ai/presentation/widgets/ai_scribe_menu.dart
- lib/l10n/intl_fr.arb
- lib/features/composer/presentation/widgets/web/web_editor_widget.dart
- lib/main/localizations/app_localizations.dart
- scribe/lib/scribe/ai/presentation/styles/ai_scribe_styles.dart
🧰 Additional context used
🧠 Learnings (7)
📚 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:
core/lib/utils/string_convert.dartscribe/lib/scribe/ai/presentation/bindings/ai_scribe_bindings.dartlib/features/composer/presentation/widgets/mobile/mobile_editor_widget.dartlib/features/composer/presentation/widgets/mobile/tablet_bottom_bar_composer_widget.dartlib/features/composer/presentation/widgets/mixins/text_selection_mixin.dartcore/test/utils/string_convert_test.dartscribe/test/scribe/ai/presentation/widgets/ai_scribe_test.darttest/features/composer/presentation/composer_controller_test.dartlib/features/composer/presentation/composer_view_web.dartlib/features/composer/presentation/mixin/ai_scribe_in_composer_mixin.dartlib/features/composer/presentation/composer_controller.dartscribe/lib/scribe/ai/presentation/widgets/ai_scribe_bar.dartscribe/lib/scribe/ai/presentation/widgets/ai_scribe.dartscribe/lib/scribe/ai/presentation/widgets/ai_scribe_button.dart
📚 Learning: 2025-12-09T12:47:45.861Z
Learnt from: zatteo
Repo: linagora/tmail-flutter PR: 4196
File: scribe/lib/scribe/ai/data/datasource_impl/ai_datasource_impl.dart:34-35
Timestamp: 2025-12-09T12:47:45.861Z
Learning: In the tmail-flutter repository, avoid suggesting replacing DioError with DioException unless there is a codebase-wide migration. This pattern applies to all Dart files; when reviewing, only propose a DioError-safe change (e.g., compatible error handling or conversion) unless a global migration is in place. Ensure consistency of DioError usage across modules and flag any deviations for a repo-wide decision.
Applied to files:
core/lib/utils/string_convert.dartscribe/lib/scribe/ai/presentation/bindings/ai_scribe_bindings.dartlib/features/composer/presentation/widgets/mobile/mobile_editor_widget.dartlib/features/composer/presentation/widgets/mobile/tablet_bottom_bar_composer_widget.dartlib/features/composer/presentation/widgets/mixins/text_selection_mixin.dartcore/test/utils/string_convert_test.dartscribe/test/scribe/ai/presentation/widgets/ai_scribe_test.darttest/features/composer/presentation/composer_controller_test.dartlib/features/composer/presentation/composer_view_web.dartlib/features/composer/presentation/mixin/ai_scribe_in_composer_mixin.dartlib/features/composer/presentation/composer_controller.dartscribe/lib/scribe/ai/presentation/widgets/ai_scribe_bar.dartscribe/lib/scribe/ai/presentation/widgets/ai_scribe.dartscribe/lib/scribe/ai/presentation/widgets/ai_scribe_button.dart
📚 Learning: 2025-12-12T04:54:11.121Z
Learnt from: dab246
Repo: linagora/tmail-flutter PR: 4191
File: lib/features/email/presentation/extensions/handle_email_action_extension.dart:37-80
Timestamp: 2025-12-12T04:54:11.121Z
Learning: In lib/features/email/presentation/extensions/handle_email_action_extension.dart, the mailboxDashBoardController.selectedEmail should only be synchronized when isMobileThreadDisabled is true. This is intentional behavior and should not be changed to update selectedEmail in non-mobile or thread-enabled contexts.
Applied to files:
lib/features/composer/presentation/widgets/mobile/mobile_editor_widget.dartlib/features/composer/presentation/composer_view_web.dartlib/features/composer/presentation/composer_controller.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:
lib/features/composer/presentation/widgets/mobile/tablet_bottom_bar_composer_widget.dartlib/features/composer/presentation/composer_view_web.dartscribe/lib/scribe/ai/presentation/widgets/ai_scribe_button.dart
📚 Learning: 2025-12-15T06:24:50.369Z
Learnt from: dab246
Repo: linagora/tmail-flutter PR: 4137
File: lib/features/mailbox_dashboard/presentation/controller/mailbox_dashboard_controller.dart:32-32
Timestamp: 2025-12-15T06:24:50.369Z
Learning: In lib/features/mailbox_dashboard/presentation/controller/mailbox_dashboard_controller.dart and lib/features/manage_account/presentation/manage_account_dashboard_controller.dart, SentryManager.instance.setUser() intentionally sends PII (account ID, display name, username, email) to Sentry for error tracking. The team plans to document this in the app's privacy policy for transparency and compliance.
Applied to files:
test/features/composer/presentation/composer_controller_test.dart
📚 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.dart
📚 Learning: 2025-12-12T07:43:26.643Z
Learnt from: dab246
Repo: linagora/tmail-flutter PR: 4204
File: lib/features/email/presentation/extensions/handle_label_for_email_extension.dart:94-148
Timestamp: 2025-12-12T07:43:26.643Z
Learning: In lib/features/email/presentation/extensions/handle_label_for_email_extension.dart and similar keyword synchronization code, the addKeyword() function is the preferred method for adding keywords to emails. The resyncKeywords() pattern is being phased out and will be replaced in favor of addKeyword().
Applied to files:
lib/features/composer/presentation/composer_view_web.dartlib/features/composer/presentation/composer_controller.dart
🔇 Additional comments (18)
lib/l10n/intl_vi.arb (1)
5482-5488: NewaiAssistantVI localization looks consistentKey, value, and metadata follow the existing ARB conventions (type
text, empty placeholders) and integrate cleanly at the file end. No changes needed.lib/l10n/intl_ru.arb (1)
5494-5500: NewaiAssistantRU localization is correctly wiredThe
aiAssistantkey and its@aiAssistantmetadata match the established ARB schema and appear ready for use by the localization layer.test/features/composer/presentation/composer_controller_test.dart (2)
183-183: LGTM!The MockSpec for GenerateAITextInteractor is correctly added to support testing of AI-related functionality.
11-11: Consider refactoring AI feature detection to use JMAP capabilities instead of .env variables.The codebase already uses JMAP capability detection elsewhere (Firebase, Forward, Contact capabilities). The current test setup with
dotenv.testLoad(AI_ENABLED: 'false')perpetuates an .env-based approach that is inconsistent with this pattern.Refactoring to mock JMAP capabilities for AI feature detection would improve architectural consistency and enable server-side control of AI support availability.
core/test/utils/string_convert_test.dart (1)
820-834: LGTM!The tests correctly validate HTML-to-text conversion with proper line break preservation. Coverage includes basic paragraph handling and nested structures with
<br>tags.scribe/test/scribe/ai/presentation/widgets/ai_scribe_test.dart (1)
1-202: LGTM!Comprehensive test coverage for AIScribe UI layout calculations. The tests properly validate height calculations for different configurations and positioning logic across all screen edges.
core/lib/utils/string_convert.dart (1)
201-227: LGTM!The HTML-to-text conversion properly uses an HTML parser instead of regex, addressing the ReDoS vulnerability concern raised in past reviews. The approach of inserting newlines after block-level tags is sound and covers common HTML structures.
Based on past review comments, this implementation resolves previous security concerns.
scribe/lib/scribe/ai/presentation/bindings/ai_scribe_bindings.dart (2)
9-9: Verify the architectural decision for AIScribeBindings.Based on past review comments, there's an unresolved discussion about whether this binding should extend
InteractorsBindingsor be moved tolib/features. The team hasn't reached consensus on the final approach.Consider clarifying the architectural decision before merging.
Based on past review comments, this architectural question remains open.
19-23: Verify that the dedicated Dio instance without auth interceptors is intentional.Line 21 creates a dedicated Dio instance without authorization interceptors. Ensure this is the correct security posture for the AI service endpoints—for example, if the AI service uses different authentication or is publicly accessible.
lib/features/composer/presentation/widgets/mixins/text_selection_mixin.dart (1)
72-82: LGTM!The EditorTextSelection class is well-structured with appropriate null-safety and a clear hasSelection helper method.
scribe/lib/scribe/ai/presentation/widgets/ai_scribe_bar.dart (1)
23-93: LGTM!The implementation properly uses
ValueNotifierfor button state management (following past feedback from dab246), correctly manages the TextEditingController lifecycle, and uses clean conditional expressions for callback handling.As per past review comments, ValueNotifier is preferred over setState for small widget updates.
scribe/lib/scribe/ai/presentation/widgets/ai_scribe_button.dart (1)
6-28: LGTM!The AIScribeButton is a clean composition widget that delegates to TMailButtonWidget with appropriate styling constants. The implementation is simple and correct.
lib/features/composer/presentation/widgets/mobile/mobile_editor_widget.dart (1)
15-92: LGTM!The conversion to StatefulWidget is well-executed. The implementation properly:
- Manages editor lifecycle with state
- Sets up text selection monitoring via JavaScript handler with mounted checks
- Cleans up resources in dispose()
- Accesses widget properties correctly throughout
lib/features/composer/presentation/mixin/ai_scribe_in_composer_mixin.dart (1)
11-84: LGTM!The mixin provides a clean integration surface for AI Scribe functionality. Implementation highlights:
- Proper platform branching for web vs. mobile/tablet controllers
- Context.mounted checks before showing dialogs
- Clean separation between full-text and selected-text workflows
- Appropriate use of GetX reactive state for text selection
scribe/lib/scribe/ai/presentation/widgets/ai_scribe.dart (1)
176-207: Position calculation logic is sound.The
calculateModalPositionfunction properly:
- Calculates position above the button
- Enforces horizontal bounds with screen edge padding
- Enforces vertical bounds when height is provided
- Uses record types appropriately for return value
lib/features/composer/presentation/composer_controller.dart (2)
870-882: LGTM!The
getTextOnlyContentInEditor()implementation correctly:
- Retrieves HTML content from the editor
- Converts HTML to plain text using
StringConvert.convertHtmlContentToTextContent- Handles errors gracefully by returning an empty string
- Uses appropriate try/catch for the async operation
265-266: Verify GenerateAITextInteractor DI registration.The getter uses
Get.find<GenerateAITextInteractor>(), which throws if the dependency is unavailable. Ensure thatGenerateAITextInteractoris properly registered in the dependency injection container before the composer controller is instantiated, or consider usingGet.isRegistered<GenerateAITextInteractor>()to provide a graceful fallback.lib/features/composer/presentation/composer_view_web.dart (1)
563-564: Consider implementing JMAP capability detection for AI features to align with backend-driven architecture.The current implementation uses
AIConfig.isAiEnabledwhich reads from.envconfiguration viadotenv. However, the codebase establishes a pattern of detecting feature support through JMAP capabilities (e.g.,AutocompleteCapability,WebSocketCapability,ContactSupportCapability) usingsession.getCapabilityProperties<T>().AI feature detection could benefit from following this pattern:
- Backend controls AI availability through JMAP capability advertisement rather than client-side
.env- Mobile clients automatically adapt when backend AI support changes
- Aligns with existing capability-based feature detection pattern in the codebase
No AI-specific JMAP capability was found, suggesting this would require backend implementation of an AI capability alongside frontend refactoring to check
session.getCapabilityProperties()instead ofAIConfig.isAiEnabled.
Improve efficiency and add more tests in convertHtmlContentToTextContent
|
All things will be addressed in #4203 |

Big Scribe PR #4187 has been split in two:
About prompts
A small PR will come soon that slighty update them to make them work as good as we can quickly. But later, we will handle differently prompts:
Video
Screen.Recording.2025-12-10.at.09.47.59.mov
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.