-
Notifications
You must be signed in to change notification settings - Fork 110
[AI Scribe] Use JMAP backend instead of calling directly an LLM #4203
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThis PR migrates AI Scribe integration from a static configuration model to a capability-based system. It removes the Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
This PR has been deployed to https://linagora.github.io/tmail-flutter/4203. |
|
@coderabbitai review please |
|
@chibenwa: I'll review the changes in this PR focusing on the JMAP backend integration for the AI Scribe feature. ✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🧹 Nitpick comments (2)
lib/features/mailbox_dashboard/presentation/controller/mailbox_dashboard_controller.dart (1)
828-857: LGTM! Clean implementation of AI scribe binding injection.The method correctly follows GetX patterns by checking
Get.isRegistered<T>()before deleting bindings, and the error handling with try/catch is consistent withinjectVacationBindings. Based on learnings, this approach is appropriate sinceGet.delete<T>()doesn't throw but checking registration first is defensive.One minor observation: consider extracting the binding types to a list to reduce repetition:
// Optional refactor for reduced repetition: final bindingTypes = [ AIDataSourceImpl, AIDataSource, AIScribeRepositoryImpl, AIScribeRepository, GenerateAITextInteractor, ]; for (final type in bindingTypes) { if (Get.isRegistered(tag: type.toString())) { Get.delete(tag: type.toString(), force: true); } }However, the current explicit approach is also valid and more type-safe.
scribe/lib/scribe/ai/data/network/ai_api.dart (1)
20-22: Consider using a more specific exception type.Throwing a generic
Exceptionmakes it harder for callers to handle this specific error case. Consider defining a custom exception (e.g.,AIApiNotAvailableException) or using an existing domain exception for better error handling upstream.+class AIApiNotAvailableException implements Exception { + final String message; + AIApiNotAvailableException([this.message = 'AI API is not available']); + @override + String toString() => message; +} if (url == null) { - throw Exception('AI API is not available'); + throw AIApiNotAvailableException(); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
lib/features/composer/presentation/composer_controller.dart(1 hunks)lib/features/composer/presentation/composer_view.dart(2 hunks)lib/features/composer/presentation/composer_view_web.dart(3 hunks)lib/features/composer/presentation/mixin/ai_scribe_in_composer_mixin.dart(2 hunks)lib/features/home/domain/extensions/session_extensions.dart(3 hunks)lib/features/mailbox_dashboard/presentation/controller/mailbox_dashboard_controller.dart(3 hunks)model/lib/ai/ai_capability.dart(1 hunks)model/lib/ai/capability_ai.dart(1 hunks)scribe/lib/scribe/ai/data/config/ai_config.dart(0 hunks)scribe/lib/scribe/ai/data/datasource_impl/ai_datasource_impl.dart(1 hunks)scribe/lib/scribe/ai/data/model/ai_api_request.dart(0 hunks)scribe/lib/scribe/ai/data/network/ai_api.dart(1 hunks)scribe/lib/scribe/ai/presentation/bindings/ai_scribe_bindings.dart(2 hunks)
💤 Files with no reviewable changes (2)
- scribe/lib/scribe/ai/data/model/ai_api_request.dart
- scribe/lib/scribe/ai/data/config/ai_config.dart
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-12-12T04:54:05.432Z
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:05.432Z
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/mixin/ai_scribe_in_composer_mixin.dartlib/features/composer/presentation/composer_controller.dartlib/features/composer/presentation/composer_view_web.dartlib/features/mailbox_dashboard/presentation/controller/mailbox_dashboard_controller.dart
📚 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:
lib/features/composer/presentation/mixin/ai_scribe_in_composer_mixin.dartlib/features/composer/presentation/composer_controller.dartlib/features/home/domain/extensions/session_extensions.dartmodel/lib/ai/ai_capability.dartlib/features/composer/presentation/composer_view_web.dartscribe/lib/scribe/ai/data/network/ai_api.dartmodel/lib/ai/capability_ai.dartlib/features/composer/presentation/composer_view.dartscribe/lib/scribe/ai/presentation/bindings/ai_scribe_bindings.dartlib/features/mailbox_dashboard/presentation/controller/mailbox_dashboard_controller.dartscribe/lib/scribe/ai/data/datasource_impl/ai_datasource_impl.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:
lib/features/composer/presentation/mixin/ai_scribe_in_composer_mixin.dartlib/features/composer/presentation/composer_controller.dartlib/features/home/domain/extensions/session_extensions.dartmodel/lib/ai/ai_capability.dartlib/features/composer/presentation/composer_view_web.dartscribe/lib/scribe/ai/data/network/ai_api.dartmodel/lib/ai/capability_ai.dartlib/features/composer/presentation/composer_view.dartscribe/lib/scribe/ai/presentation/bindings/ai_scribe_bindings.dartlib/features/mailbox_dashboard/presentation/controller/mailbox_dashboard_controller.dartscribe/lib/scribe/ai/data/datasource_impl/ai_datasource_impl.dart
🔇 Additional comments (13)
model/lib/ai/capability_ai.dart (1)
1-3: LGTM! Clean capability identifier definition.The capability identifier follows JMAP conventions and integrates cleanly with the capability infrastructure.
model/lib/ai/ai_capability.dart (1)
1-20: LGTM! Well-structured capability model.The AICapability class follows established patterns for capability properties with proper JSON serialization and equality support.
lib/features/home/domain/extensions/session_extensions.dart (1)
15-16: LGTM! Consistent capability integration.The AI capability follows the established pattern of other capability getters with appropriate error handling and logging.
Also applies to: 38-38, 172-184
lib/features/composer/presentation/mixin/ai_scribe_in_composer_mixin.dart (1)
25-35: Verify the dual-check logic for AI availability.The
isAIScribeAvailablechecks both the globalAIConfig.isAiEnabledflag (line 26) and the session capability (line 33). This means AI Scribe will only be available if both conditions are met.Is this intentional? The PR description states "Display AI Scribe features only if the JMAP capability exists," which suggests the capability check alone should be sufficient. The global config check might be:
- Intentional for a phased rollout or feature flag
- A defensive measure during migration
- An oversight
Please confirm whether both checks are needed or if the global config check can be removed in favor of relying solely on the JMAP capability.
lib/features/composer/presentation/composer_controller.dart (1)
271-275: LGTM! Clean implementation of session/accountId exposure.The getters appropriately delegate to the mailbox dashboard controller and correctly implement the mixin's abstract getters.
lib/features/composer/presentation/composer_view.dart (1)
472-473: LGTM! Proper UI gating based on capability availability.The UI correctly gates AIScribe features using
controller.isAIScribeAvailable, hiding the UI when the capability is not present.Also applies to: 558-560
scribe/lib/scribe/ai/presentation/bindings/ai_scribe_bindings.dart (1)
10-12: Verify behavior when endpoint is null.The
scribeEndpointparameter is optional (line 10), and it's passed directly toAIDataSourceImplwithout validation (line 26).Please verify:
- What happens when
endpointis null inAIDataSourceImpland subsequently inAIApi?- Is there a default endpoint that will be used?
- Should there be validation to ensure the endpoint is provided when AI features are enabled?
Without clear fallback behavior, initializing AIScribe bindings without an endpoint could lead to runtime errors when attempting to make AI requests.
Consider adding validation or documentation about the expected behavior when
scribeEndpointis null.Also applies to: 24-27
scribe/lib/scribe/ai/data/datasource_impl/ai_datasource_impl.dart (1)
11-17: Verify AIApi behavior with null endpoint.The
endpointparameter is optional and passed directly toAIApi(line 16). This change removes the previous hardcodedAIConfig.aiApiKeyandAIConfig.aiApiUrlusage in favor of a configurable endpoint.Please ensure that
AIApihandles a null endpoint appropriately:
- Does
AIApihave a default endpoint when none is provided?- Will it throw a clear error if endpoint is null and a request is attempted?
- Should there be a runtime check here to fail fast if endpoint is null?
This relates to the same concern raised in
ai_scribe_bindings.dartabout null endpoint handling.lib/features/mailbox_dashboard/presentation/controller/mailbox_dashboard_controller.dart (1)
898-898: Verify binding injection order.The
injectAIScribeBindingscall is placed afterinjectPreferencesBindingsand before the mobile FCM bindings. Ensure this ordering doesn't create any dependency issues if AI scribe bindings rely on preferences or other injected dependencies.lib/features/composer/presentation/composer_view_web.dart (3)
562-563: LGTM! Consistent capability-based AI scribe gating.The conditional logic correctly disables the AI scribe button when the capability is unavailable by passing
nullfor both the callback and the key. This is a clean pattern for feature gating.
838-839: Tablet view follows the same pattern - LGTM.Consistent with the desktop implementation.
959-962: Selection button guard is correct.Early return when
isAIScribeAvailableis false prevents unnecessary widget building and Obx subscription.scribe/lib/scribe/ai/data/network/ai_api.dart (1)
5-13: LGTM on the endpoint-based API restructure.The simplification from separate API key and base URL to a single endpoint aligns with the JMAP capability-driven approach where the endpoint is provided by the session capability.
lib/features/composer/presentation/mixin/ai_scribe_in_composer_mixin.dart
Outdated
Show resolved
Hide resolved
lib/features/composer/presentation/mixin/ai_scribe_in_composer_mixin.dart
Outdated
Show resolved
Hide resolved
lib/features/mailbox_dashboard/presentation/controller/mailbox_dashboard_controller.dart
Outdated
Show resolved
Hide resolved
lib/features/mailbox_dashboard/presentation/controller/mailbox_dashboard_controller.dart
Outdated
Show resolved
Hide resolved
lib/features/mailbox_dashboard/presentation/controller/mailbox_dashboard_controller.dart
Outdated
Show resolved
Hide resolved
2f2ac2e to
fe624a0
Compare
Check JMAP capability AND if AI is enabled in env.file and use this method everywhere where we checked env.file
No more API key and url needed because everything is managed by the backend. Also backend does not accept a model parameter for the moment.
It was used at the beginning for configuration purpose but now we have the JMAP capability, no need anymore.
5289b0d to
86d9551
Compare
…ode in a single file
Fixed |
…w email without content
Also cannot reproduce |
Fixed Screen.Recording.2025-12-18.at.17.01.52.mov |
Screen.Recording.2025-12-18.at.5.39.49.PM.mov |
|
@zatteo I built image: |
|
Summary of feedbacks from everyone who tested:
=> #4216
Screen.Recording.2025-12-18.at.17.12.04.mov
I checked this one and it is fixed by #4212 or #4214
Screen.Recording.2025-12-18.at.17.17.16.mov
=> #4215 |
Fixed |
… of composer or response have long result but at the bottom of composer
Fixed Screen.Recording.2025-12-19.at.11.36.18.mov |
as other web AI do. Fx: chatgpt Screen.Recording.2025-12-19.at.12.00.50.PM.mov |




Rely on the work of the backend team to get the JMAP capability:
Summary by CodeRabbit
Release Notes
New Features
Improvements
✏️ Tip: You can customize this high-level summary in your review settings.