[AI Scribe] Add "replace" and improve "insert" action #4201
Conversation
Collapsing a selection means removing it and keep the cursor at the end. It will be needed by the scribe for "Insert" action where we want to insert a text at the cursor even if the user selected text.
If at AI scribe creation, we pass a onReplace callback, it will show on "Replace" button next to "Insert" button.
By leveraging selection collapsing provided by JS in both composer, we can now differentiate two actions that will be used by the scribe: - inserting: always insert at end of cursor - replacing: replace the content (selection wide or editor wide)
If we open from selection: insert + replace (replace selection) If open from bottom bar with content : insert + replace (replace everything) If open from bottom bar with empty content : insert
WalkthroughThis pull request implements a replace functionality for the AI Scribe feature across the codebase. It adds a new JavaScript constant for collapsing text selections and introduces cross-platform editor manipulation utilities (collapse, clear, insert, replace) in the composer mixin. The web editor widget is updated to collapse selections on initialization. Localization strings for a "Replace" button are added across multiple language files (English, French, Russian, Vietnamese). The AI Scribe dialogs and suggestions are enhanced to support an optional replace callback, enabling a replace button in the UI alongside the existing insert button. Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
This PR has been deployed to https://linagora.github.io/tmail-flutter/4201. |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
lib/features/composer/presentation/mixin/ai_scribe_in_composer_mixin.dart (1)
53-67: Consider returningFuture<void>from async methods.Both
onInsertTextCallbackandonReplaceTextCallbackare declared asvoid ... async, which is an anti-pattern in Dart. This means callers cannot await them or handle errors. While this may be intentional for fire-and-forget callback usage, it silently swallows any exceptions from the awaited operations.If the callback typedef
OnInsertTextCallbackis constrained tovoid Function(String), this pattern is acceptable for callbacks, but any errors incollapseSelection()will be unobservable.🔎 Consider this pattern for better error observability:
- void onInsertTextCallback(String text) async { + Future<void> onInsertTextCallback(String text) async { await collapseSelection(); - insertTextInEditor(text); } - void onReplaceTextCallback(String text) async { + Future<void> onReplaceTextCallback(String text) async { final selection = editorTextSelection.value?.selectedText; if (selection == null || selection.isEmpty) { clearTextInEditor(); } - insertTextInEditor(text); }Note: This would also require updating
OnInsertTextCallbacktypedef if it currently constrains the return type tovoid.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
core/lib/utils/html/html_utils.dart(1 hunks)lib/features/composer/presentation/mixin/ai_scribe_in_composer_mixin.dart(4 hunks)lib/features/composer/presentation/widgets/web/web_editor_widget.dart(1 hunks)scribe/lib/scribe/ai/l10n/intl_en.arb(1 hunks)scribe/lib/scribe/ai/l10n/intl_fr.arb(1 hunks)scribe/lib/scribe/ai/l10n/intl_messages.arb(1 hunks)scribe/lib/scribe/ai/l10n/intl_ru.arb(1 hunks)scribe/lib/scribe/ai/l10n/intl_vi.arb(1 hunks)scribe/lib/scribe/ai/localizations/scribe_localizations.dart(1 hunks)scribe/lib/scribe/ai/presentation/widgets/ai_scribe.dart(2 hunks)scribe/lib/scribe/ai/presentation/widgets/ai_scribe_suggestion.dart(3 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/ai/localizations/scribe_localizations.dartscribe/lib/scribe/ai/presentation/widgets/ai_scribe.dartlib/features/composer/presentation/widgets/web/web_editor_widget.dartscribe/lib/scribe/ai/presentation/widgets/ai_scribe_suggestion.dartcore/lib/utils/html/html_utils.dartlib/features/composer/presentation/mixin/ai_scribe_in_composer_mixin.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/ai/localizations/scribe_localizations.dartscribe/lib/scribe/ai/presentation/widgets/ai_scribe.dartlib/features/composer/presentation/widgets/web/web_editor_widget.dartscribe/lib/scribe/ai/presentation/widgets/ai_scribe_suggestion.dartcore/lib/utils/html/html_utils.dartlib/features/composer/presentation/mixin/ai_scribe_in_composer_mixin.dart
🔇 Additional comments (14)
scribe/lib/scribe/ai/l10n/intl_vi.arb (1)
116-122: LGTM!The Vietnamese localization for the replace button follows the established pattern and structure used for other button translations in this file.
scribe/lib/scribe/ai/localizations/scribe_localizations.dart (1)
125-128: LGTM!The replaceButton getter is implemented correctly and follows the same pattern as other localization getters in this file.
scribe/lib/scribe/ai/l10n/intl_fr.arb (1)
116-122: LGTM!The French localization for the replace button is correct and follows the established pattern.
scribe/lib/scribe/ai/l10n/intl_en.arb (1)
116-122: LGTM!The English localization entry for the replace button is properly structured and follows the existing pattern.
core/lib/utils/html/html_utils.dart (1)
114-122: LGTM!The JavaScript snippet correctly uses the Selection API's
collapseToEnd()method with proper null checking. The IIFE pattern and naming convention are consistent with other constants in this file.lib/features/composer/presentation/widgets/web/web_editor_widget.dart (1)
188-191: LGTM!The collapseSelectionToEnd script is properly integrated into the web editor initialization scripts, following the same pattern as other WebScript entries.
scribe/lib/scribe/ai/presentation/widgets/ai_scribe.dart (2)
21-21: LGTM!The optional
onReplaceTextparameter correctly extends the API to support the replace functionality while maintaining backward compatibility.
128-131: LGTM!The conditional onReplace callback is correctly implemented. The replace action is only wired up when
onReplaceTextis provided, and it properly invokes the callback and closes the dialog.scribe/lib/scribe/ai/l10n/intl_messages.arb (1)
116-122: LGTM!The new
replaceButtonlocalization entry follows the established pattern and is consistent with the existinginsertButtonentry structure.scribe/lib/scribe/ai/presentation/widgets/ai_scribe_suggestion.dart (2)
16-27: LGTM!The optional
onReplacecallback is correctly added using the same type asonInsert, maintaining consistency while allowing the replace functionality to be opt-in.
189-209: LGTM!The footer correctly conditionally renders the replace button when
onReplaceis provided. The null-check guard at line 191 makes the force-unwrap at line 197 safe. The visual distinction between the transparent replace button and the styled insert button provides clear UX hierarchy.lib/features/composer/presentation/mixin/ai_scribe_in_composer_mixin.dart (3)
44-50: LGTM!The
clearTextInEditormethod correctly handles both platforms using null-safe optional chaining.
80-106: LGTM!The dialog integrations correctly wire up the new callbacks:
showAIScribeMenuForFullText: Conditionally providesonReplaceTextonly when there's content to replace (fullText.isEmpty ? null).showAIScribeMenuForSelectedText: Always providesonReplaceTextsince this path requires an existing selection (guarded at lines 93-95).This logic properly aligns with the PR's intent for insert vs. replace behavior.
33-42: HtmlUtils.collapseSelectionToEnd property access verified.Both
.nameand.scriptproperties exist onHtmlUtils.collapseSelectionToEndand are correctly used for their respective platforms. The implementation properly differentiates between web and mobile APIs.
| "placeholders_order": [], | ||
| "placeholders": {} | ||
| }, | ||
| "replaceButton": "заменить", |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cd scribe && cat -n lib/scribe/ai/l10n/intl_ru.arb | head -150Repository: linagora/tmail-flutter
Length of output: 3891
Fix capitalization for consistency with other button labels.
The Russian translation "заменить" is lowercase, while all other button and action labels in this file use capitalized first letters (e.g., "Вставить" on line 111). Change "заменить" to "Заменить" to match the established capitalization pattern.
🤖 Prompt for AI Agents
In scribe/lib/scribe/ai/l10n/intl_ru.arb around line 117, the value for
"replaceButton" is "заменить" (lowercase) which is inconsistent with other
button labels that use an initial capital letter; update the translation to
"Заменить" so the first letter is capitalized to match the file's established
capitalization pattern.
|
also in #4203 |
In some case, we want to be able with the result of the LLM to:
Here I add a "replace" optional button and correctly manage "replace" and "insert" action in all cases.
Screenshot
Summary by CodeRabbit
Release Notes
✏️ Tip: You can customize this high-level summary in your review settings.