Skip to content

AI scribe UI#4197

Merged
hoangdat merged 22 commits intofeatures/aifrom
ai-scribe-ui
Dec 18, 2025
Merged

AI scribe UI#4197
hoangdat merged 22 commits intofeatures/aifrom
ai-scribe-ui

Conversation

@zatteo
Copy link
Member

@zatteo zatteo commented Dec 10, 2025

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:

  • they will be fetched from a remote location
  • they will be managed by a dedicated library or SDK

Video

Screen.Recording.2025-12-10.at.09.47.59.mov

Summary by CodeRabbit

  • New Features
    • Added AI Scribe integration enabling users to generate text suggestions directly in the composer via AI-powered assistance.
    • Introduced AI assistant button in composer with menu for predefined actions and custom prompts.
    • Added multilingual support for AI assistant across French, Russian, and Vietnamese.

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

@coderabbitai
Copy link

coderabbitai bot commented Dec 10, 2025

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

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

Walkthrough

This PR 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

  • PR #4196: Introduces the core AI Scribe infrastructure and GenerateAITextInteractor dependency that this PR integrates into the Composer workflow.

Suggested reviewers

  • chibenwa
  • hoangdat

Pre-merge checks and finishing touches

❌ Failed checks (1 inconclusive)
Check name Status Explanation Resolution
Title check ❓ Inconclusive The title "AI scribe UI" is vague and generic; it does not clearly convey the specific changes being introduced. Consider a more specific title such as "Add AI Scribe UI integration for text generation" or "Implement AI Scribe dialog and editor selection UI" to better describe the primary change.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

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

@zatteo zatteo mentioned this pull request Dec 10, 2025
3 tasks
@zatteo
Copy link
Member Author

zatteo commented Dec 10, 2025

@coderabbitai review

@coderabbitai
Copy link

coderabbitai bot commented Dec 10, 2025

✅ Actions performed

Review triggered.

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

@github-actions
Copy link

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

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 2

🧹 Nitpick comments (17)
scribe/pubspec.yaml (1)

27-28: GetX dependency addition

Adding get: 4.6.6 here 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: aiAssistant getter wiring is correct; consider copy consistency

The new getter correctly exposes the aiAssistant key 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 _isButtonEnabled logic are correct, and trimming before send avoids empty/whitespace prompts.
  • Button and onSubmitted both funnel through _onSendPressed, so behavior is consistent.

Optional: you could add textInputAction: TextInputAction.send to the TextField to 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.white is hardcoded. If the app supports dark mode or plans to, consider extracting this to AIScribeColors or 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 a TypeError if 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 = null doesn't remove the JavaScript handler.

The JavaScript handler registered via addJavaScriptHandler persists until the webview is destroyed. While the mounted check 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.0 should reference ComposerStyle.mobileEditorPadding.horizontal or 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 lazyPut without fenix: true (the default) removes the factory after instance disposal, preventing automatic recreation on subsequent Get.find() calls. If your application's SmartManagement disposes these dependencies (AIDataSourceImpl, AIDataSource, etc.) and they need to be recreated, add fenix: true to preserve the factory for resurrection. Alternatively, use permanent: true if 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 _SubmenuPanel displays 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 GestureDetector to 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 actions list, onTap does 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, and Positioned inside a Stack is 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: Repeated AIConfig.isAiEnabled checks 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 (&#123;) 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('&nbsp;', ' ')
-      .replaceAll('&amp;', '&')
-      .replaceAll('&lt;', '<')
-      .replaceAll('&gt;', '>')
-      .replaceAll('&quot;', '"')
-      .replaceAll('&#39;', "'");
-
-    return textContent.trim();
+    final document = html_parser.parse(htmlContent);
+    return document.body?.text ?? '';
 }
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 986ddb9 and a5720ac.

⛔ Files ignored due to path filters (1)
  • scribe/pubspec.lock is 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.dart
  • lib/features/composer/presentation/widgets/web/bottom_bar_composer_widget.dart
  • lib/main/localizations/app_localizations.dart
  • scribe/lib/scribe/ai/presentation/widgets/ai_scribe_bar.dart
  • lib/features/composer/presentation/view/web/web_editor_view.dart
  • scribe/lib/scribe/ai/presentation/widgets/ai_scribe_suggestion.dart
  • lib/features/composer/presentation/composer_bindings.dart
  • scribe/lib/scribe/ai/presentation/widgets/ai_scribe_button.dart
  • lib/features/composer/presentation/widgets/mobile/tablet_bottom_bar_composer_widget.dart
  • lib/features/composer/presentation/view/mobile/mobile_editor_view.dart
  • scribe/lib/scribe/ai/presentation/widgets/ai_scribe_menu.dart
  • scribe/lib/scribe/ai/presentation/widgets/ai_scribe.dart
  • lib/features/composer/presentation/widgets/web/web_editor_widget.dart
  • scribe/lib/scribe/ai/presentation/bindings/ai_scribe_bindings.dart
  • scribe/lib/scribe/ai/presentation/styles/ai_scribe_styles.dart
  • lib/main/bindings/main_bindings.dart
  • lib/features/composer/presentation/widgets/mobile/mobile_editor_widget.dart
  • lib/features/composer/presentation/composer_controller.dart
  • lib/features/composer/presentation/composer_view_web.dart
  • lib/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.dart
  • lib/features/composer/presentation/widgets/web/bottom_bar_composer_widget.dart
  • lib/main/localizations/app_localizations.dart
  • scribe/lib/scribe/ai/presentation/widgets/ai_scribe_bar.dart
  • lib/features/composer/presentation/view/web/web_editor_view.dart
  • scribe/lib/scribe/ai/presentation/widgets/ai_scribe_suggestion.dart
  • lib/features/composer/presentation/composer_bindings.dart
  • scribe/lib/scribe/ai/presentation/widgets/ai_scribe_button.dart
  • lib/features/composer/presentation/widgets/mobile/tablet_bottom_bar_composer_widget.dart
  • lib/features/composer/presentation/view/mobile/mobile_editor_view.dart
  • scribe/lib/scribe/ai/presentation/widgets/ai_scribe_menu.dart
  • scribe/lib/scribe/ai/presentation/widgets/ai_scribe.dart
  • lib/features/composer/presentation/widgets/web/web_editor_widget.dart
  • scribe/lib/scribe/ai/presentation/bindings/ai_scribe_bindings.dart
  • scribe/lib/scribe/ai/presentation/styles/ai_scribe_styles.dart
  • lib/main/bindings/main_bindings.dart
  • lib/features/composer/presentation/widgets/mobile/mobile_editor_widget.dart
  • lib/features/composer/presentation/composer_controller.dart
  • lib/features/composer/presentation/composer_view_web.dart
  • lib/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_modified looks consistent with normal ARB metadata updates; no functional impact.

lib/l10n/intl_fr.arb (1)

5476-5482: French aiAssistant translation looks good

Key/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 integrated

The file scribe/ai/data/config/ai_config.dart exists and is already used internally by ai_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: New aiAssistant localization correctly integrated

The @@last_modified timestamp update and the new "aiAssistant" entry with @aiAssistant metadata follow the existing ARB structure and naming conventions. The corresponding getter is properly wired in AppLocalizations (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 aiAssistant localization 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 TextSelectionMixin and overrides the onSelectionChanged getter 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 _dropListener setup, which is good for consistency. The listener properly:

  1. Checks for MessageEvent type
  2. Decodes JSON data
  3. 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 _selectionChangeListenerRegistered flag ensures the JavaScript listener is only registered once, consistent with the drop listener pattern.


9-9: Remove this review comment.

The TextSelectionMixin import is correctly implemented. The mixin exists at the referenced path and properly exports the handleSelectionChange method and onSelectionChanged getter. 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 onTextSelectionChanged callback is properly declared and accepted via constructor, following the existing pattern for other callbacks like onEditorContentHeightChanged.

Also applies to: 31-31


142-153: Callback correctly wired to MobileEditorWidget.

The buildEditor method properly passes the onTextSelectionChanged callback to MobileEditorWidget, 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 scribe package is declared as a local path dependency in pubspec.yaml (path: scribe), and the import path correctly references the existing file at scribe/lib/scribe/ai/domain/usecases/generate_ai_text_interactor.dart. This is not an external dependency issue.


342-342: GenerateAITextInteractor is registered globally — this is intentional and safe.

The global registration (without tag: composerId) is correct. GenerateAITextInteractor is registered in AIScribeBindings and initialized via main_bindings.dart before ComposerBindings loads. 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: New aiAssistant VI localization looks correct

Key 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 onOpenAIScribe as nullable and gating the button with if (onOpenAIScribe != null) keeps the API backward‑compatible and avoids null issues when invoking the callback.
  • Using AbsorbPointer and icon color changes tied to isCodeViewEnabled matches the existing behavior of other formatting actions, so the UX remains coherent.
  • Passing aiScribeButtonKey through 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 onTextSelectionChanged callback is correctly added as an optional field and consistently forwarded to all WebEditorWidget instantiations 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!

AIScribeBindings is 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.find inside lazyPut builder may throw if dependency isn't resolved yet.

The _bindingsDataSource uses Get.find<AIDataSourceImpl>() inside the lazyPut builder. Since lazyPut defers creation until first access, if AIDataSource is requested before AIDataSourceImpl is ever accessed, this could fail. The current call order in dependencies() 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 _calculateModalPosition function 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 onTextSelectionChanged callback 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 Obx for reactive updates
  • Handles null coordinates gracefully
  • Uses Builder to obtain button context for position calculation
scribe/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.dart and theme_utils.dart are necessary and actively used throughout the file:

  • AppColor is referenced 5 times (lines 28, 29, 31, 35, 39)
  • ThemeUtils is 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> and Rxn<Offset> with GetX observables enables reactive UI updates when selection changes.


909-928: LGTM! Proper context.mounted check after async gap.

The context.mounted check at line 918 correctly guards against using a stale context after the async getTextOnlyContentInEditor() call.


946-964: LGTM! Selection state management is well-structured.

The handleTextSelection method 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 \n with <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.

- 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
@dab246
Copy link
Member

dab246 commented Dec 11, 2025

Screenshot 2025-12-11 at 13 13 41
  • IMO, We should show submenus when hovering over an item, instead of clicking, so users know which item is selected and which submenu it belongs to.

@dab246
Copy link
Member

dab246 commented Dec 11, 2025

  • What's about mobile?

Comment on lines +80 to +88
TMailButtonWidget.fromIcon(
key: aiScribeButtonKey,
icon: imagePaths.icSparkle,
borderRadius: TabletBottomBarComposerWidgetStyle.iconRadius,
padding: TabletBottomBarComposerWidgetStyle.iconPadding,
iconSize: TabletBottomBarComposerWidgetStyle.iconSize,
tooltipMessage: AppLocalizations.of(context).aiAssistant,
onTapActionCallback: onOpenAIScribe!,
),
Copy link
Member

Choose a reason for hiding this comment

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

We should create a separate widget for it so we can reuse it in multiple places.

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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;
Copy link
Member

Choose a reason for hiding this comment

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

IMO, the aiScribe widget should hold the GlobalKey so that it can manage its own key, preventing widgets holding keys from causing memory leaks.

Copy link
Member Author

Choose a reason for hiding this comment

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

@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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

Understood. WDYT about this totally different approach that removes the aiScribeButtonKey ? #4207

Comment on lines 116 to 122
if (event is MessageEvent) {
final data = jsonDecode(event.data);

if (data['name'] == HtmlUtils.registerSelectionChangeListener.name) {
handleSelectionChange(data);
}
}
Copy link
Member

Choose a reason for hiding this comment

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

Should use try/catch for this

}
};
if (_selectionChangeListener != null) {
window.addEventListener("message", _selectionChangeListener!);
Copy link
Member

Choose a reason for hiding this comment

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

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.

Comment on lines 162 to 164
final selectedText = Rxn<String>();
final hasTextSelection = false.obs;
final textSelectionCoordinates = Rxn<Offset>();
Copy link
Member

Choose a reason for hiding this comment

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

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.

Comment on lines 872 to 884
String convertHtmlContentToTextContent(String htmlContent) {
String textContent = htmlContent.replaceAll(RegExp(r'<[^>]*>'), '');

textContent = textContent
.replaceAll('&nbsp;', ' ')
.replaceAll('&amp;', '&')
.replaceAll('&lt;', '<')
.replaceAll('&gt;', '>')
.replaceAll('&quot;', '"')
.replaceAll('&#39;', "'");

return textContent.trim();
}
Copy link
Member

Choose a reason for hiding this comment

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

It should be placed in the util class within the core module for reuse.

Comment on lines 200 to 210
static String convertHtmlContentToTextContent(String htmlContent) {
String textContent = htmlContent.replaceAll(RegExp(r'<[^>]*>'), '');

textContent = textContent
.replaceAll('&nbsp;', ' ')
.replaceAll('&amp;', '&')
.replaceAll('&lt;', '<')
.replaceAll('&gt;', '>')
.replaceAll('&quot;', '"')
.replaceAll('&#39;', "'");

Copy link
Member

Choose a reason for hiding this comment

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

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 &eacute;, &copy;, &hellip;, 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 (&eacute;é)
  • 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.
Copy link
Member

@chibenwa chibenwa left a comment

Choose a reason for hiding this comment

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

Still many changesout of lib/scribe but I'm unsure committing to more refactorings here on this specific feature is worth it.

@dab246
Copy link
Member

dab246 commented Dec 15, 2025

@coderabbitai please review again

@coderabbitai
Copy link

coderabbitai bot commented Dec 15, 2025

@dab246, I'll review the changes in this PR.

✅ Actions performed

Review triggered.

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

Comment on lines 207 to 216
const blockTags = [
'p',
'div',
'li',
'section',
'article',
'header',
'footer',
'h1','h2','h3','h4','h5','h6'
];
Copy link
Member

Choose a reason for hiding this comment

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

Is this enough?

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Comment on lines 218 to 222
for (final tag in blockTags) {
document.querySelectorAll(tag).forEach((element) {
element.append(Text('\n'));
});
}
Copy link
Member

Choose a reason for hiding this comment

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

IMO, this process takes a considerable amount of time.

Copy link
Member Author

@zatteo zatteo Dec 15, 2025

Choose a reason for hiding this comment

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

Good point. I updated the behavior to use a single querySelecterAll with a comma nested selector in 3393df3 which must be better.

Comment on lines 820 to 834
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');
});
});
Copy link
Member

Choose a reason for hiding this comment

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

Write more test cases for it.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 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.result directly for GenerateAITextSuccess, as the type system guarantees result is 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

📥 Commits

Reviewing files that changed from the base of the PR and between a5720ac and 5f3b46d.

⛔ Files ignored due to path filters (1)
  • scribe/pubspec.lock is 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.dart
  • scribe/lib/scribe/ai/presentation/bindings/ai_scribe_bindings.dart
  • lib/features/composer/presentation/widgets/mobile/mobile_editor_widget.dart
  • lib/features/composer/presentation/widgets/mobile/tablet_bottom_bar_composer_widget.dart
  • lib/features/composer/presentation/widgets/mixins/text_selection_mixin.dart
  • core/test/utils/string_convert_test.dart
  • scribe/test/scribe/ai/presentation/widgets/ai_scribe_test.dart
  • test/features/composer/presentation/composer_controller_test.dart
  • lib/features/composer/presentation/composer_view_web.dart
  • lib/features/composer/presentation/mixin/ai_scribe_in_composer_mixin.dart
  • lib/features/composer/presentation/composer_controller.dart
  • scribe/lib/scribe/ai/presentation/widgets/ai_scribe_bar.dart
  • scribe/lib/scribe/ai/presentation/widgets/ai_scribe.dart
  • scribe/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.dart
  • scribe/lib/scribe/ai/presentation/bindings/ai_scribe_bindings.dart
  • lib/features/composer/presentation/widgets/mobile/mobile_editor_widget.dart
  • lib/features/composer/presentation/widgets/mobile/tablet_bottom_bar_composer_widget.dart
  • lib/features/composer/presentation/widgets/mixins/text_selection_mixin.dart
  • core/test/utils/string_convert_test.dart
  • scribe/test/scribe/ai/presentation/widgets/ai_scribe_test.dart
  • test/features/composer/presentation/composer_controller_test.dart
  • lib/features/composer/presentation/composer_view_web.dart
  • lib/features/composer/presentation/mixin/ai_scribe_in_composer_mixin.dart
  • lib/features/composer/presentation/composer_controller.dart
  • scribe/lib/scribe/ai/presentation/widgets/ai_scribe_bar.dart
  • scribe/lib/scribe/ai/presentation/widgets/ai_scribe.dart
  • scribe/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.dart
  • lib/features/composer/presentation/composer_view_web.dart
  • lib/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.dart
  • lib/features/composer/presentation/composer_view_web.dart
  • scribe/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.dart
  • lib/features/composer/presentation/composer_controller.dart
🔇 Additional comments (18)
lib/l10n/intl_vi.arb (1)

5482-5488: New aiAssistant VI localization looks consistent

Key, 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: New aiAssistant RU localization is correctly wired

The aiAssistant key and its @aiAssistant metadata 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 InteractorsBindings or be moved to lib/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 ValueNotifier for 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 calculateModalPosition function 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 that GenerateAITextInteractor is properly registered in the dependency injection container before the composer controller is instantiated, or consider using Get.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.isAiEnabled which reads from .env configuration via dotenv. However, the codebase establishes a pattern of detecting feature support through JMAP capabilities (e.g., AutocompleteCapability, WebSocketCapability, ContactSupportCapability) using session.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 of AIConfig.isAiEnabled.

Improve efficiency and add more tests in convertHtmlContentToTextContent
@hoangdat
Copy link
Member

All things will be addressed in #4203

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants