AI Scribe in the composer #4187
Conversation
AI_ENABLED allows to test AI features only in some wanted environment. AI_API_KEY and AI_API_URL allows to call the LLM while we wait for tmail backend endpoints. It will be removed when tmail backend endpoints are ready.
| } | ||
|
|
||
| static String correctGrammar(String text) { | ||
| return 'Correct grammar of the following text. Output only the result. No formatting. Text:\n\n$text'; |
There was a problem hiding this comment.
How about
| return 'Correct grammar of the following text. Output only the result. No formatting. Text:\n\n$text'; | |
| return 'Correct grammar of the following text. Output only the result. Preserve input formatting. Text:\n\n$text'; |
There was a problem hiding this comment.
I updated it.
When we will move this prompt somewhere else, we will need to find a proper way of evaluating these prompts.
| case AIScribeMenuAction.changeToneFriendly: | ||
| return translateTo(text, 'friendly'); | ||
| case AIScribeMenuAction.changeToneProfessionnal: | ||
| return translateTo(text, 'professionnal'); |
There was a problem hiding this comment.
| case AIScribeMenuAction.changeToneFriendly: | |
| return translateTo(text, 'friendly'); | |
| case AIScribeMenuAction.changeToneProfessionnal: | |
| return translateTo(text, 'professionnal'); | |
| case AIScribeMenuAction.changeToneFriendly: | |
| return changeToneTo(text, 'friendly'); | |
| case AIScribeMenuAction.changeToneProfessionnal: | |
| return changeToneTo(text, 'professionnal'); |
?
There was a problem hiding this comment.
Good catch, copy/paste mistake.
| ), | ||
| child: Text( | ||
| action.label, | ||
| style: AIScribeTextStyles.menuItem, |
There was a problem hiding this comment.
o.O
Too much ident level.
Is thi idiomatic from this code base?
| this.arguments, | ||
| this.contentViewState, | ||
| this.onEditorContentHeightChanged, | ||
| this.onTextSelectionChanged, |
There was a problem hiding this comment.
I would LOVE to have a separated commit for adding onTextSelectionChanged to the composer.
| onTapActionCallback: onOpenInsertLink, | ||
| ), | ||
| ), | ||
| if (onOpenAIScribe != null) ...[ |
There was a problem hiding this comment.
Two things:
- The button should be show if and only if the AI capability is advertized by the bac IMO
CF com:linagora:params:jmap:aibot is present in the JMAP capabilities
- I'd love to have one more indirection level so that regular tmail code has no direct dependencies onto AI components.
Acceptence criteria:
- All AI related changes are in the ai subfolder
- We actually only use declarative registration: plug the AI module.
(Idealy I would love to have a central slim code base and a code base with TWP extensions like AI, Drive integration, top bar integratins etc)
There was a problem hiding this comment.
I will try to go to this way 👍
| String get label { | ||
| switch (this) { | ||
| case AIScribeMenuAction.correctGrammar: | ||
| return 'Correct grammar'; |
|
|
||
| GenerateAITextInteractor(this._repository); | ||
|
|
||
| Future<Either<Failure, Success>> execute( |
There was a problem hiding this comment.
should use Stream, it makes UI more flexible to react
|
Hi @zatteo , To make it easier to support you, you should clean the history a bit:
For this, we can easily understand the logic behind scribe, and maybe can support you directly in UI components to make it a perfect fit with prototype. Moreover, it will help us easily to integrate with BE, when BE ready a capability for it. |
So basically move
OK
OK
What do you mean by use case here?
OK
OK will try to learn more about Getx |
This mixin leverages HTML callbacks that can be set by WebView editor to send text selection event to the composer so the composer will receive text selection event necessary to display buttons related to text selection.
|
Every comments have been answered. AI scribe UI is displayed only when env var AI_ENABLED is true. Because scribe will be potentially separated in another library later, I created an independant i18n for scribe. In another PR because this one is already too big, I will manage request to backend because it has been merged (see PR) and mobile version. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (5)
lib/features/composer/presentation/widgets/mobile/tablet_bottom_bar_composer_widget.dart (1)
78-89: Tooltip localization has been addressed.The tooltip now correctly uses
AppLocalizations.of(context).aiAssistantfor i18n support, consistent with other buttons in this widget.scribe/lib/scribe/ai/presentation/widgets/ai_scribe.dart (1)
15-23: Regarding the past review on DI: This design is acceptable.The
GenerateAITextInteractoris passed as a parameter, which is a valid form of dependency injection (method injection). This allows the caller (e.g., a GetX controller) to obtain the interactor via GetX and pass it here, keeping this utility function decoupled from the DI framework. This addresses the previous review concern about presentation calling data layer directly.scribe/lib/scribe/ai/presentation/model/ai_scribe_menu_action.dart (2)
18-46: LGTM - Localization properly implemented viaScribeLocalizations.This addresses the past review comments about using intl. Labels are now fetched from
ScribeLocalizations, enabling proper localization support.
4-77: LGTM - Methods defined directly in enum.This addresses the past review comment about not needing extensions for enums. The
getLabel,category, andgetFullLabelmethods are correctly defined within the enum body.scribe/lib/scribe/ai/presentation/model/ai_action.dart (1)
20-28: Localization support addresses past review comment.
CustomPromptAction.getLabelnow usesScribeLocalizations.of(context)!.customPromptAction, providing multi-language support as requested in the previous review.
🧹 Nitpick comments (15)
scribe/pubspec.yaml (2)
9-31: Verify all dependencies are actively used.Given the previous feedback to "import dependencies you need," please confirm each public dependency is actively used in the implementation:
dartz: functional Either/Option types?dio: HTTP requests to AI API?flutter_dotenv: environment configuration?get: state management and GetX bindings?pointer_interceptor: UI event handling?intl: localization strings?If any of these are unused or only conditionally imported, remove them or clearly document their purpose.
43-46: Pinintl_generatorto a specific commit hash instead ofmasterbranch.Pinning to the
masterbranch introduces a maintenance risk: the branch can change unexpectedly and potentially break reproducible builds. Although the comment explains the fork rationale (original abandoned), it's better to pin to a specific commit hash or release tag.Update the dependency to pin to a specific commit:
intl_generator: git: url: https://github.com/linagora/intl_generator.git - ref: master + ref: <specific-commit-hash-or-tag>Check the linagora/intl_generator repository for the latest stable commit or tag to use.
lib/main/localizations/app_localizations.dart (1)
5439-5445:aiAssistantgetter matches existing localization patterns; consider capitalization alignment.The new
aiAssistantgetter is correctly defined and consistent with otherIntl.messageusages. To avoid translator confusion, consider ensuring the English default text here uses the same capitalization as the correspondingaiAssistantentries in your ARB files (e.g. either all"AI Assistant"or all"AI assistant").scribe/lib/scribe/ai/l10n/intl_en.arb (1)
1-129: intl_en ARB structure and key set look solid; a few copy nits are optionalThe file is well-structured (keys + @metadata pairs, no placeholders needed) and the naming scheme is consistent and clear for the AI Scribe feature. Only optional UX wording tweaks you might consider:
actionTransformToBullets: something like “Transform into bullet list” or “Convert to bullet points” may read a bit more naturally.noDataReceived: “No response received from AI” or “No response received” can be clearer than the more technical-sounding “No data received”.These are purely copy choices; structurally the file is good to go.
scribe/lib/scribe.dart (1)
1-16: Consider separating public API from implementation exports.The library exports both abstractions (
ai_scribe_repository.dart) and implementations (ai_repository_impl.dart,ai_datasource.dart). For cleaner API boundaries, consider:
- A main export for consumers (domain + presentation only)
- A separate export file for DI/bindings (e.g.,
scribe_bindings.dart) containing implementationsThis is optional and depends on how tightly coupled the consumer is expected to be.
library scribe; +// Domain layer (public API) export 'scribe/ai/domain/usecases/generate_ai_text_interactor.dart'; export 'scribe/ai/domain/repository/ai_scribe_repository.dart'; export 'scribe/ai/domain/model/ai_response.dart'; export 'scribe/ai/domain/state/generate_ai_text_state.dart'; + +// Presentation layer (public API) export 'scribe/ai/presentation/widgets/ai_scribe.dart'; export 'scribe/ai/presentation/widgets/ai_scribe_bar.dart'; export 'scribe/ai/presentation/widgets/ai_scribe_button.dart'; export 'scribe/ai/presentation/widgets/ai_scribe_menu.dart'; export 'scribe/ai/presentation/widgets/ai_scribe_suggestion.dart'; export 'scribe/ai/presentation/model/ai_action.dart'; export 'scribe/ai/presentation/model/ai_scribe_menu_action.dart'; + +// Data layer (for DI wiring) export 'scribe/ai/data/repository/ai_repository_impl.dart'; export 'scribe/ai/data/datasource/ai_datasource.dart'; export 'scribe/ai/data/config/ai_config.dart';lib/features/composer/presentation/composer_view.dart (1)
557-595: RTL support properly implemented withPositionedDirectional.Good use of
PositionedDirectionalwithstartinstead ofPositionedwithleftfor RTL language support.Consider extracting the magic number to a style constant:
- // Account for the horizontal padding around the editor - const editorHorizontalPadding = 12.0; + // Account for the horizontal padding around the editor + final editorHorizontalPadding = ComposerStyle.mobileEditorPadding.horizontal / 2;Alternatively, define this value in
ComposerStyleif it's not already derivable from existing constants.scribe/lib/scribe/ai/presentation/widgets/ai_scribe_suggestion.dart (2)
174-189: Add user feedback after clipboard copy action.The copy button silently copies to clipboard without informing the user. Consider showing a snackbar or tooltip confirmation.
Widget _buildToolbar(String suggestion) { return Container( padding: const EdgeInsets.all(8), child: Row( children: [ - IconButton( - icon: const Icon(Icons.content_copy, size: 18), - color: Colors.grey[600], - onPressed: () { - Clipboard.setData(ClipboardData(text: suggestion)); - }, + Tooltip( + message: ScribeLocalizations.of(context)!.copyToClipboard, + child: IconButton( + icon: const Icon(Icons.content_copy, size: 18), + color: Colors.grey[600], + onPressed: () { + Clipboard.setData(ClipboardData(text: suggestion)); + ScaffoldMessenger.of(context).showSnackBar( + SnackBar( + content: Text(ScribeLocalizations.of(context)!.copiedToClipboard), + duration: const Duration(seconds: 2), + ), + ); + }, + ), ), ], ), ); }Note: You'll need to add the localization keys
copyToClipboardandcopiedToClipboardto your localization files.
27-29: State class naming inconsistency.The State class is named
_AIScribeSuggestionModalStatebut the widget isAIScribeSuggestion. Convention is to name it_AIScribeSuggestionState.@override - State<AIScribeSuggestion> createState() => _AIScribeSuggestionModalState(); + State<AIScribeSuggestion> createState() => _AIScribeSuggestionState(); } -class _AIScribeSuggestionModalState extends State<AIScribeSuggestion> { +class _AIScribeSuggestionState extends State<AIScribeSuggestion> {scribe/lib/scribe/ai/presentation/widgets/ai_scribe.dart (3)
39-41: Consider using theme colors instead of hardcodedColors.white.Hardcoded colors reduce flexibility for theming and dark mode support. Consider extracting these to
AIScribeColorsor usingTheme.of(context).- Material( - color: Colors.white, + Material( + color: Theme.of(context).colorScheme.surface,Also applies to: 55-58
30-31: Extract magic numbers to named constants.Values like
240.0,500.0,400.0,16, and10are scattered throughout. Consider defining them inAIScribeSizesfor consistency and maintainability.Also applies to: 135-135
180-195: Error handling loses original failure details.The
foldcallback wraps failures in a genericException, losing any structured error information. Consider defining a custom exception type or preserving the failure details for better debugging and error handling upstream.return result.fold( (failure) { - throw Exception('Failed to generate AI response: $failure'); + throw AIScribeException('Failed to generate AI response', failure); }, (success) { if (success is GenerateAITextSuccess) { final result = success.response.result; if (result == null) { - throw Exception('AI response result is null'); + throw AIScribeException('AI response result is null'); } return result; } else { - throw Exception('Unexpected success state: $success'); + throw AIScribeException('Unexpected success state: $success'); } }, );scribe/lib/scribe/ai/localizations/scribe_localizations.dart (1)
103-112:inputPlaceholderandcustomPromptActionhave identical default text.Both return
'Help me write'. If this is intentional for UX consistency, consider adding a comment. Otherwise,customPromptActionmay need a distinct message.scribe/lib/scribe/ai/presentation/widgets/ai_scribe_menu.dart (2)
220-223: HardcodedColors.white- same concern as ai_scribe.dart.Consider using theme colors or
AIScribeColorsfor consistency and theming support.
115-125: Edge case: category without actions.If
category.actionsis empty,onActionSelectedis never called and the tap is silently ignored. Consider logging or disabling the item when no actions exist.return _buildMenuItem( label: category.getLabel(context), onTap: () { if (category.actions.isNotEmpty) { widget.onActionSelected(category.actions.first); + } else { + // Consider logging or handling empty category case } }, );scribe/lib/scribe/ai/presentation/model/ai_scribe_menu_action.dart (1)
99-124:actionsgetter creates a new list on every call.Each invocation allocates a new list. If called frequently (e.g., in build methods), consider caching or using
constlists.List<AIScribeMenuAction> get actions { switch (this) { case AIScribeMenuCategory.correctGrammar: - return [AIScribeMenuAction.correctGrammar]; + return const [AIScribeMenuAction.correctGrammar]; case AIScribeMenuCategory.improve: - return [ + return const [ AIScribeMenuAction.improveMakeShorter, ... ]; // ... same for other cases } }
📜 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 (25)
lib/features/composer/presentation/composer_view.dart(5 hunks)lib/features/composer/presentation/composer_view_web.dart(10 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/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.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(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/model/ai_action.dart(1 hunks)scribe/lib/scribe/ai/presentation/model/ai_scribe_menu_action.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_menu.dart(1 hunks)scribe/lib/scribe/ai/presentation/widgets/ai_scribe_suggestion.dart(1 hunks)scribe/pubspec.yaml(1 hunks)scripts/prebuild.sh(2 hunks)
✅ Files skipped from review due to trivial changes (2)
- scribe/lib/scribe/ai/l10n/intl_fr.arb
- scribe/lib/scribe/ai/l10n/intl_vi.arb
🚧 Files skipped from review as they are similar to previous changes (4)
- scripts/prebuild.sh
- lib/l10n/intl_messages.arb
- scribe/lib/scribe/ai/presentation/widgets/ai_scribe_bar.dart
- lib/features/composer/presentation/widgets/web/bottom_bar_composer_widget.dart
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
- GitHub Check: analyze-test (contact)
- GitHub Check: Build web version and deploy
- GitHub Check: analyze-test (server_settings)
- GitHub Check: analyze-test (rule_filter)
- GitHub Check: analyze-test (forward)
- GitHub Check: analyze-test (email_recovery)
- GitHub Check: analyze-test (default)
- GitHub Check: analyze-test (fcm)
- GitHub Check: analyze-test (core)
- GitHub Check: analyze-test (model)
🔇 Additional comments (20)
scribe/lib/scribe/ai/l10n/intl_ru.arb (1)
1-129: AI Scribe RU ARB looks structurally correct and consistent.JSON structure, key naming, and
@metadata blocks are valid and aligned with the English AI-scribe keys; Russian labels are clear and use no placeholders, which matches the metadata.scribe/lib/scribe/ai/l10n/intl_messages.arb (1)
1-129: English AI Scribe ARB is well-formed and mirrors the RU Scribe keys.Key set and metadata are consistent with the RU Scribe ARB, making it a solid base for additional locales.
lib/l10n/intl_ru.arb (1)
5495-5500: RU translation foraiAssistantis correctly added.Key name and metadata follow the existing ARB conventions, and
"Помощник ИИ"is an appropriate Russian label for “AI assistant”.lib/main.dart (1)
12-12: Scribe localization delegate is correctly wired into the app.Importing
ScribeLocalizationsand addingScribeLocalizations.delegatealongside existing delegates is consistent with the current localization setup. Ensure that thescribepackage is properly declared inpubspec.yaml(as a path or local package) so this import resolves in all environments.lib/l10n/intl_vi.arb (1)
5482-5488: aiAssistant VI localization looks correct and consistentKey and metadata follow existing ARB conventions, and the translation “Trợ lý AI” is appropriate; no further changes needed.
lib/l10n/intl_fr.arb (1)
5476-5482: aiAssistant FR localization is well-formed and idiomaticString “Assistant IA” and its @Aiassistant metadata align with the existing FR ARB style; nothing to adjust.
lib/features/composer/presentation/widgets/mobile/tablet_bottom_bar_composer_widget.dart (1)
17-18: LGTM! Clean addition of optional AI Scribe parameters.The nullable fields and optional constructor parameters follow the existing widget pattern, allowing backward compatibility while enabling AI Scribe integration.
Also applies to: 30-31
lib/features/composer/presentation/composer_view.dart (2)
238-269: Good restructuring for AIScribe overlay integration.The Stack-based approach correctly layers the AIScribe selection button over the editor content while maintaining the existing scroll behavior.
472-473: LGTM! Proper feature flag guards for AI Scribe.The conditional assignment using
AIConfig.isAiEnabledensures the AI features are only wired when enabled.lib/features/composer/presentation/composer_view_web.dart (2)
270-271: LGTM! Text selection callback properly wired.The
onTextSelectionChangedcallback is correctly connected to the controller's handler across all responsive views.
562-563: LGTM! Consistent feature flag guards for AI Scribe.The pattern matches the mobile implementation with proper
AIConfig.isAiEnabledchecks.scribe/lib/scribe/ai/presentation/widgets/ai_scribe_suggestion.dart (1)
11-29: LGTM! Well-structured widget API.The widget has a clean, focused API with all required dependencies explicitly declared. The use of
Future<String>for the suggestion allows flexible async data fetching patterns.scribe/lib/scribe/ai/localizations/scribe_localizations.dart (3)
136-147: LGTM - StandardLocalizationsDelegateimplementation.The delegate correctly implements
isSupported,load, andshouldReload. The supported locales match the available ARB files.
11-20: GlobalIntl.defaultLocalemutation may cause side effects.Setting
Intl.defaultLocalein theload()method affects the entire application. If multiple localization modules use this pattern, the last one to load wins. Verify whether this is intentional or if the scribe module should rely on the app's already-set locale instead of mutating global state.
7-9: Nullable return type requires careful handling by callers.
of()returnsScribeLocalizations?, but callers use!(e.g., inai_scribe_menu_action.dartline 19). IfScribeLocalizations.delegateis not registered inMaterialApp.localizationsDelegates, this will throw at runtime. Ensure proper registration and consider adding a helpful assertion or fallback.scribe/lib/scribe/ai/presentation/widgets/ai_scribe_menu.dart (2)
27-34: LGTM - Proper initialization and key management.GlobalKeys are correctly initialized for each category in
initState, enabling accurate positioning for submenu overlays.
36-45: LGTM - Proper overlay cleanup in dispose.Calling
_removeSubmenu()indispose()ensures the overlay is removed when the widget is unmounted, preventing memory leaks and orphaned overlays.scribe/lib/scribe/ai/presentation/model/ai_scribe_menu_action.dart (1)
19-19: Potential null pointer ifScribeLocalizationsis not registered.Both
getLabelmethods useScribeLocalizations.of(context)!. If the delegate is not registered in the app'slocalizationsDelegates, this will throw. Consider a defensive fallback or ensure registration is verified at startup.Also applies to: 86-86
scribe/lib/scribe/ai/presentation/model/ai_action.dart (2)
5-9: LGTM - Well-designed sealed class hierarchy.Using
sealed classenables exhaustive pattern matching in Dart 3, ensuring all action types are handled. The abstractgetLabelmethod enforces consistent labeling across implementations.
27-27: Same force-unwrap concern as other files.
ScribeLocalizations.of(context)!will throw if the delegate is not registered. This is a recurring pattern across files - ensureScribeLocalizations.delegateis properly registered in the app.
Editor exposes html content (so with html tags) and we do not want to send them to AI backend
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
scribe/lib/scribe/ai/data/datasource_impl/ai_datasource_impl.dart (1)
12-18: Clarify ownership of API key/base URL betweenAIDataSourceImpl,AIApi, andAIConfig.You’re wiring
AIConfig.aiApiKey/AIConfig.aiApiUrlintoAIApihere. IfAIApiis always used with those same values, consider either:
- letting
AIApiown reading fromAIConfigin its default constructor, or- injecting
apiKey/baseUrl(or an already‑configuredAIApi) from the DI layer,to reduce coupling to static config and centralize this responsibility. This is essentially the same concern already raised in the previous review about where
apiKey/baseUrlshould live.
🧹 Nitpick comments (6)
scribe/lib/scribe/ai/data/datasource_impl/ai_datasource_impl.dart (1)
21-38: Preserve original error details instead of throwing a new genericException.Catching
DioError/ genericeand throwing a newExceptionstringifies the error and drops both the concrete type and original stack trace, which makes debugging and upstream handling harder.Consider either:
- rethrowing the original error after any logging, or
- wrapping it in a domain‑specific failure type while preserving the stack (e.g. via
Error.throwWithStackTrace) and, if useful, including status code / response body.This keeps your
AIDataSourcecontract clean while retaining enough information to diagnose issues.scribe/lib/scribe/ai/presentation/widgets/ai_scribe_suggestion.dart (1)
181-187: Consider adding user feedback after copying to clipboard.The clipboard operation succeeds silently. Users typically expect visual confirmation (e.g., a snackbar or tooltip) when content is copied.
IconButton( icon: const Icon(Icons.content_copy, size: 18), color: Colors.grey[600], onPressed: () { Clipboard.setData(ClipboardData(text: suggestion)); + ScaffoldMessenger.of(context).showSnackBar( + const SnackBar( + content: Text('Copied to clipboard'), + duration: Duration(seconds: 2), + ), + ); }, ),scribe/lib/scribe/ai/localizations/scribe_localizations.dart (1)
104-112:inputPlaceholderandcustomPromptActionhave identical values.Both getters return
'Help me write'with different message names. If these are intentionally the same, consider whether they need separate localization keys, or if one can reference the other to reduce duplication in translation files.lib/features/composer/presentation/composer_controller.dart (1)
871-883: HTML entity decoding is incomplete.The manual replacement handles only 6 common entities. Consider using a library or more comprehensive approach for proper HTML decoding, as content may contain other entities (e.g.,
—,©, numeric entities like—).+import 'package:html_unescape/html_unescape.dart'; + +final _htmlUnescape = HtmlUnescape(); + String convertHtmlContentToTextContent(String htmlContent) { String textContent = htmlContent.replaceAll(RegExp(r'<[^>]*>'), ''); - - textContent = textContent - .replaceAll(' ', ' ') - .replaceAll('&', '&') - .replaceAll('<', '<') - .replaceAll('>', '>') - .replaceAll('"', '"') - .replaceAll(''', "'"); - - return textContent.trim(); + return _htmlUnescape.convert(textContent).trim(); }Alternatively, if adding a dependency is not desired, at minimum consider adding a comment noting the limitation.
lib/features/composer/presentation/composer_view.dart (1)
238-269: Consider extracting the duplicated Stack pattern into a helper method.The mobile (lines 238-269) and tablet (lines 427-458) views have nearly identical Stack structures wrapping the editor with the AI scribe selection button. This could be refactored to reduce duplication.
Widget _buildEditorWithAIScribeOverlay({ required BuildContext context, required Widget editorView, required Widget? additionalContent, }) { return Stack( children: [ Column( children: [ editorView, if (additionalContent != null) additionalContent, SizedBox(height: MediaQuery.viewInsetsOf(context).bottom + 64), ], ), _buildAIScribeSelectionButton(context), ], ); }Also applies to: 427-458
scribe/lib/scribe.dart (1)
1-16: Consider tightening the public API surface of thescribepackageThe barrel currently exports domain, presentation, and all data-layer internals (e.g.,
ai_repository_impl.dart,ai_datasource.dart,ai_config.dart). That’s convenient but makes it harder to refactor internals later without breaking callers, and encourages outside code to couple directly to data implementations instead of the domain abstractions.If you want more flexibility long term, consider:
- Keeping only domain contracts, interactor(s), and presentation widgets public.
- Either not exporting data implementations at the top level, or re‑exporting them from a more “advanced/internal” sub‑barrel (e.g.,
scribe/data.dart) so typical consumers use only domain + UI.This is not a blocker, but worth deciding now since it defines the stable API of the module.
📜 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 (44)
lib/features/composer/presentation/composer_bindings.dart(2 hunks)lib/features/composer/presentation/composer_controller.dart(6 hunks)lib/features/composer/presentation/composer_view.dart(5 hunks)lib/features/composer/presentation/composer_view_web.dart(10 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/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.dart(2 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/data/config/ai_config.dart(1 hunks)scribe/lib/scribe/ai/data/datasource/ai_datasource.dart(1 hunks)scribe/lib/scribe/ai/data/datasource_impl/ai_datasource_impl.dart(1 hunks)scribe/lib/scribe/ai/data/model/ai_api_request.dart(1 hunks)scribe/lib/scribe/ai/data/model/ai_api_response.dart(1 hunks)scribe/lib/scribe/ai/data/model/ai_message.dart(1 hunks)scribe/lib/scribe/ai/data/network/ai_api.dart(1 hunks)scribe/lib/scribe/ai/data/repository/ai_repository_impl.dart(1 hunks)scribe/lib/scribe/ai/domain/constants/ai_prompts.dart(1 hunks)scribe/lib/scribe/ai/domain/model/ai_response.dart(1 hunks)scribe/lib/scribe/ai/domain/repository/ai_scribe_repository.dart(1 hunks)scribe/lib/scribe/ai/domain/state/generate_ai_text_state.dart(1 hunks)scribe/lib/scribe/ai/domain/usecases/generate_ai_text_interactor.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/bindings/ai_scribe_bindings.dart(1 hunks)scribe/lib/scribe/ai/presentation/model/ai_action.dart(1 hunks)scribe/lib/scribe/ai/presentation/model/ai_scribe_menu_action.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)scripts/prebuild.sh(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (29)
- scribe/lib/scribe/ai/data/datasource/ai_datasource.dart
- scripts/prebuild.sh
- scribe/lib/scribe/ai/data/network/ai_api.dart
- lib/features/composer/presentation/widgets/mobile/tablet_bottom_bar_composer_widget.dart
- scribe/lib/scribe/ai/data/config/ai_config.dart
- scribe/lib/scribe/ai/presentation/widgets/ai_scribe_button.dart
- lib/features/composer/presentation/composer_bindings.dart
- scribe/lib/scribe/ai/l10n/intl_vi.arb
- scribe/lib/scribe/ai/domain/repository/ai_scribe_repository.dart
- lib/features/composer/presentation/widgets/web/bottom_bar_composer_widget.dart
- scribe/lib/scribe/ai/l10n/intl_ru.arb
- scribe/lib/scribe/ai/data/model/ai_message.dart
- lib/main/localizations/app_localizations.dart
- scribe/lib/scribe/ai/domain/model/ai_response.dart
- lib/l10n/intl_ru.arb
- scribe/lib/scribe/ai/data/model/ai_api_response.dart
- scribe/lib/scribe/ai/data/model/ai_api_request.dart
- scribe/lib/scribe/ai/domain/usecases/generate_ai_text_interactor.dart
- scribe/lib/scribe/ai/presentation/widgets/ai_scribe_menu.dart
- scribe/lib/scribe/ai/data/repository/ai_repository_impl.dart
- lib/l10n/intl_messages.arb
- scribe/lib/scribe/ai/l10n/intl_en.arb
- lib/main.dart
- scribe/pubspec.yaml
- scribe/lib/scribe/ai/l10n/intl_fr.arb
- scribe/lib/scribe/ai/domain/constants/ai_prompts.dart
- scribe/lib/scribe/ai/presentation/bindings/ai_scribe_bindings.dart
- scribe/lib/scribe/ai/domain/state/generate_ai_text_state.dart
- scribe/lib/scribe/ai/presentation/styles/ai_scribe_styles.dart
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
- GitHub Check: analyze-test (server_settings)
- GitHub Check: analyze-test (email_recovery)
- GitHub Check: Build web version and deploy
- GitHub Check: analyze-test (fcm)
- GitHub Check: analyze-test (contact)
- GitHub Check: analyze-test (forward)
- GitHub Check: analyze-test (core)
- GitHub Check: analyze-test (model)
- GitHub Check: analyze-test (rule_filter)
- GitHub Check: analyze-test (default)
🔇 Additional comments (22)
lib/main/bindings/main_bindings.dart (1)
3-3: AIScribe DI wiring looks correct; just confirm behavior when AI is disabledImporting
AIScribeBindingsand invokingAIScribeBindings().dependencies()inMainBindings.dependencies()is consistent with the existing binding pattern and ordering.Since AI is feature‑toggled via
AI_ENABLED, please confirm thatAIScribeBindings().dependencies()is safe when AI is disabled or env vars are missing (no crashes, minimal overhead).Also applies to: 25-25
lib/l10n/intl_vi.arb (1)
5483-5487: NewaiAssistantVietnamese localization is consistentKey name, value
"Trợ lý AI", and@aiAssistantmetadata match existing ARB conventions and other locales. No issues.lib/l10n/intl_fr.arb (1)
5477-5481: NewaiAssistantFrench localization is well‑formed
"Assistant IA"plus the@aiAssistantmetadata follow the same pattern as other entries and align with terminology already used in the app.scribe/lib/scribe/ai/l10n/intl_messages.arb (1)
1-129: Scribe ARB definitions are coherent and ready for useThe new
intl_messages.arbfile is structurally valid, keys are clearly namespaced (category*,action*,language*, etc.), and all metadata blocks are consistent. This should integrate cleanly with the Scribe localization layer.scribe/lib/scribe/ai/presentation/widgets/ai_scribe_bar.dart (1)
1-103: LGTM!The widget implementation is clean and well-structured:
- Proper lifecycle management with listener removal and controller disposal
- Input validation with trim before callback invocation
- Dependency injection pattern followed for
imagePaths- Localization properly used for placeholder text
scribe/lib/scribe/ai/presentation/widgets/ai_scribe_suggestion.dart (1)
121-153: Error state layout has been corrected.The error state now correctly uses
Flexible(fit: FlexFit.loose, ...)matching the loading and success states, which addresses the previous review concern about layout issues withExpanded.lib/features/composer/presentation/composer_controller.dart (2)
908-927: Potential null context access after async gap.The
context.mountedcheck at line 917 is good, butshowAIScribeDialogis called withcontextwhich could theoretically become invalid between the check and usage. This is generally fine in practice but worth noting.The async flow and null safety handling are appropriate. The
context.mountedguard before callingshowAIScribeDialogprevents crashes if the widget is disposed during the async operation.
945-963: LGTM!The
handleTextSelectionmethod properly updates all related reactive state variables and handles the null case correctly by resetting all values.lib/features/composer/presentation/composer_view.dart (2)
557-595: LGTM!The
_buildAIScribeSelectionButtonimplementation is well-structured:
- Correctly uses
PositionedDirectionalfor RTL support (addresses past review comment)- Properly guards with
AIConfig.isAiEnabledcheck- Uses
Builderpattern appropriately to get the button's render context for position calculation- Handles the case where
renderBoxmight be null
472-473: AI feature gating is correctly applied.The
onOpenAIScribeandaiScribeButtonKeyare conditionally passed only whenAIConfig.isAiEnabledis true, ensuring the AI features are properly feature-flagged.scribe/lib/scribe/ai/localizations/scribe_localizations.dart (1)
11-20: SettingIntl.defaultLocaleinLocalizationsDelegate.load()follows Flutter best practices.This is the standard recommended pattern for Flutter localization delegates. The code correctly initializes messages before setting the locale. However, verify that
Intl.defaultLocaleis set from only one centralized place in the application to avoid conflicts. If the main app also manages locale changes, ensure they coordinate through a single source of truth rather than multiple scattered assignments.lib/features/composer/presentation/composer_view_web.dart (4)
32-32: LGTM! Text selection tracking properly integrated.The scribe package import and onTextSelectionChanged callbacks are correctly wired across all responsive variants (mobile, desktop, tablet), enabling text selection detection for the AI Scribe feature.
Also applies to: 270-270, 514-514, 791-791
336-336: LGTM! AI Scribe selection button consistently positioned.The AI Scribe selection button is correctly added to the Stack overlay in all three responsive variants, enabling contextual AI actions when text is selected.
Also applies to: 610-610, 885-885
562-563: LGTM! AI features properly gated in bottom bar.The onOpenAIScribe callback and aiScribeButtonKey are correctly conditionally passed to BottomBarComposerWidget when AI is enabled, maintaining proper feature gating.
Also applies to: 838-839
959-997: LGTM! RTL support correctly implemented.The method properly addresses the previous review concern by using
PositionedDirectionalwithstartinstead ofPositionedwithleft, ensuring correct behavior in RTL languages. The implementation includes:
- Proper feature gating with early return
- Reactive selection state tracking via Obx
- Button position calculation with fallback handling
- PointerInterceptor for web iframe interaction
scribe/lib/scribe/ai/presentation/model/ai_scribe_menu_action.dart (2)
4-77: LGTM! Past review feedback addressed.The implementation correctly:
- Uses
ScribeLocalizations(intl-based) for localization, addressing previous comments about intl support- Defines methods inside the enum rather than as extensions, as suggested in prior feedback
- Provides exhaustive switch coverage for all enum values
- Implements clean category grouping and submenu labeling logic
79-129: LGTM! Clean category implementation.The AIScribeMenuCategory enum follows the same localization pattern as AIScribeMenuAction, correctly groups actions by category, and provides clear submenu detection logic.
scribe/lib/scribe/ai/presentation/widgets/ai_scribe.dart (3)
15-23: LGTM! Dependency injection properly implemented.The function correctly receives
GenerateAITextInteractoras a parameter rather than instantiating data layer classes directly, addressing the previous review feedback about using dependency injection.
73-98: LGTM! Robust dialog positioning with bounds checking.The positioning logic correctly:
- Calculates dialog position relative to button
- Ensures dialogs stay within screen bounds with appropriate padding
- Uses responsive sizing (90% width on mobile, max 500px on larger screens)
- Provides fallback to centered positioning when buttonPosition is null
Also applies to: 130-155
173-196: Verify error handling in AIScribeSuggestion widget.The
_executeAIRequesthelper throws exceptions for failures and unexpected states, which will cause thesuggestionFuture(line 116) to complete with an error. Ensure thatAIScribeSuggestionproperly handles Future errors to display appropriate error messages to the user. Check that the widget uses FutureBuilder with an error state handler or implements catchError/onError callbacks.scribe/lib/scribe/ai/presentation/model/ai_action.dart (2)
5-9: LGTM! Clean sealed class abstraction.The sealed class pattern enables exhaustive checking of action types and ensures all implementations provide localized labels via the
getLabelmethod.
11-29: LGTM! Multi-language support implemented.Both
PredefinedActionandCustomPromptActioncorrectly implement localized labels viaScribeLocalizations, addressing the previous feedback about supporting multiple languages. The implementation cleanly separates predefined menu actions from custom prompt actions.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
scribe/lib/scribe/ai/presentation/widgets/ai_scribe.dart (1)
26-103: Consider extracting magic numbers and positioning logic.The dialog construction logic is solid, but could benefit from:
- Extracting magic numbers (240.0, 12, 8, 16, 10) as named constants for maintainability.
- The positioning logic (lines 73-98) is duplicated in the second dialog (lines 130-165) and could be extracted into a helper function.
Example refactor for constants:
// At the top of the file or in a constants file const double _kMenuWidth = 240.0; const double _kDialogElevation = 8.0; const double _kDialogBorderRadius = 12.0; const double _kDialogSpacing = 8.0; const double _kDialogMargin = 16.0; const double _kButtonOffset = 10.0;Example refactor for positioning:
Widget _positionDialog({ required Widget child, required Offset buttonPosition, required BuildContext context, required double dialogWidth, double? dialogHeight, }) { final screenSize = MediaQuery.of(context).size; double left = buttonPosition.dx; double bottom = screenSize.height - buttonPosition.dy + _kButtonOffset; // Horizontal bounds checking if (left + dialogWidth > screenSize.width) { left = screenSize.width - dialogWidth - _kDialogMargin; } if (left < _kDialogMargin) { left = _kDialogMargin; } // Vertical bounds checking if height provided if (dialogHeight != null) { if (bottom + dialogHeight > screenSize.height) { bottom = screenSize.height - dialogHeight - _kDialogMargin; } if (bottom < _kDialogMargin) { bottom = _kDialogMargin; } } return Stack( children: [ Positioned( left: left, bottom: bottom, child: child, ), ], ); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
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_menu.dart(1 hunks)scribe/lib/scribe/ai/presentation/widgets/ai_scribe_suggestion.dart(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- scribe/lib/scribe/ai/presentation/widgets/ai_scribe_suggestion.dart
- scribe/lib/scribe/ai/presentation/widgets/ai_scribe_menu.dart
- scribe/lib/scribe/ai/presentation/widgets/ai_scribe_bar.dart
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
- GitHub Check: analyze-test (server_settings)
- GitHub Check: analyze-test (core)
- GitHub Check: analyze-test (email_recovery)
- GitHub Check: analyze-test (rule_filter)
- GitHub Check: analyze-test (fcm)
- GitHub Check: analyze-test (forward)
- GitHub Check: analyze-test (model)
- GitHub Check: analyze-test (contact)
- GitHub Check: analyze-test (default)
- GitHub Check: Build web version and deploy
🔇 Additional comments (5)
scribe/lib/scribe/ai/presentation/widgets/ai_scribe.dart (5)
1-14: LGTM!The imports are appropriate for the AI Scribe dialog functionality, and the typedef provides clear semantics for the callback.
15-24: Good use of dependency injection.The interactor is now passed as a parameter rather than being instantiated directly, which properly addresses the previous review feedback about presentation/data layer separation.
105-110: LGTM!Good defensive programming with null checks and context.mounted verification before proceeding to the second dialog.
173-196: Error handling relies on consumer widget.The function correctly uses the Either pattern with
foldand throws descriptive exceptions. However, the error handling responsibility is delegated to the caller/consumer (AIScribeSuggestion widget). This is acceptable if that widget properly handles these exceptions.The null check on line 187 and the unexpected state check on line 191 are good defensive measures.
Note: This comment is related to the verification requested in the previous comment about AIScribeSuggestion error handling.
112-170: Verify error handling in AIScribeSuggestion widget.The
_executeAIRequestcall (line 116) can throw exceptions, but there's no error handling at this level. Ensure that theAIScribeSuggestionwidget properly handles future errors (e.g., usingFutureBuilderwith error state) to prevent the app from crashing when the AI request fails.
| } | ||
|
|
||
| static String improveMakeShorter(String text) { | ||
| return 'Make the following text shorter and more concise. Output only the result. Preserve input formatting. Text:\n\n$text'; |
There was a problem hiding this comment.
how about to make it as an env variable, IMO, user can easily to custom it then.
| // Get screen size to ensure modal stays within bounds | ||
| final screenSize = MediaQuery.of(context).size; | ||
| // Use responsive width: 90% on mobile, max 500px on larger screens | ||
| final modalWidth = screenSize.width < 600 ? screenSize.width * 0.9 : 500.0; |
| final screenSize = MediaQuery.of(context).size; | ||
| // Use responsive width: 90% on mobile, max 500px on larger screens | ||
| final modalWidth = screenSize.width < 600 ? screenSize.width * 0.9 : 500.0; | ||
| const modalHeight = 400.0; |
| controller: _controller, | ||
| decoration: InputDecoration( | ||
| hintText: 'Help me write', | ||
| hintStyle: TextStyle( |
| const SizedBox(width: 8), | ||
| TMailButtonWidget.fromIcon( | ||
| icon: ImagePaths().icSend, | ||
| iconSize: 16, |
There was a problem hiding this comment.
move to style file all magic number
There was a problem hiding this comment.
I managed all your last comments in last commits of the PR #4197
|
Follow up for the UI part: #4197 |
|
Closing because split in two PR |
4 participants
Follow the work of #4172. Mockups are finalized and the goal of this PR is to go to production contrary to the previous one which was more a draft to play and learn.
Still some work to do, but already creating the PR to get first feedbacks especially about usage of components, controllers, mixins, code sharing, respect of convention, etc.
Mockups
Current status
Screen.Recording.2025-12-02.at.16.19.26.mov
Todo
To start, I will not implement every features from the mockup. I consider enough for a first version if I add:
To play with it
Add in .env
Summary by CodeRabbit
New Features
Chores
✏️ Tip: You can customize this high-level summary in your review settings.