[AI Scribe] Use JMAP backend instead of calling directly an LLM #4203
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 WalkthroughThe pull request introduces a capability-driven approach to AI Scribe feature availability. It adds AI capability retrieval to sessions, replaces global AI config checks with per-controller availability checks based on session capabilities, makes the AI service layer endpoint configurable, and refactors the composer and mailbox dashboard controllers to expose session/accountId and dynamically inject AI bindings. The AIConfig static accessors for API key and URL are removed in favor of endpoint-based configuration passed during binding initialization. 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.
|
![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 (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.
| final response = await _dio.post( | ||
| '$_baseUrl/chat/completions', | ||
| options: Options( | ||
| headers: { | ||
| 'Authorization': 'Bearer $_apiKey', | ||
| 'Content-Type': 'application/json', | ||
| }, | ||
| ), | ||
| url, | ||
| data: request.toJson(), | ||
| ); |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Dio Flutter automatically set Content-Type header POST JSON data
💡 Result:
Short answer: Recent Dio versions no longer infer Content-Type for you — you must set it (or set options.contentType) and/or send JSON-encoded body yourself.
How to do it:
- Dio v5+ (recommended)
- Set options.contentType or Headers.jsonContentType, and pass a JSON string if needed:
- dio.options.contentType = Headers.jsonContentType;
- await dio.post(url, data: jsonEncode(bodyMap));
- Or set per-request:
- await dio.post(url, data: jsonEncode(bodyMap), options: Options(contentType: Headers.jsonContentType));
- Set options.contentType or Headers.jsonContentType, and pass a JSON string if needed:
- Dio v4 (older behavior): passing a Map could result in application/x-www-form-urlencoded; to force JSON set header/contentType and/or pass jsonEncode(bodyMap).
Examples:
- Per-request:
- await dio.post(url, data: jsonEncode(bodyMap), options: Options(headers: {'Content-Type':'application/json'}));
- Global:
- dio.options.headers['Content-Type'] = 'application/json';
- await dio.post(url, data: jsonEncode(bodyMap));
Notes:
- If you pass a Map and Dio treats it as form data, JSON encoding (jsonEncode) plus Content-Type: application/json ensures a JSON body.
- Dio v5 intentionally removed automatic content-type inference — set it manually. (github.com)
If you want, tell me which Dio version you use and I’ll give the exact snippet.
Citations:
🏁 Script executed:
# Search for Dio configuration in the codebase
fd -e dart -e yaml | head -20Repository: linagora/tmail-flutter
Length of output: 1159
🏁 Script executed:
# Look for JMAP backend references and Dio setup
rg -i "jmap|content.type|dio" --type dart -A 3 -B 3 | head -100Repository: linagora/tmail-flutter
Length of output: 8876
🏁 Script executed:
# Find the _dio initialization to see if headers are configured
rg "_dio\s*=" -A 10 -B 2 --type dartRepository: linagora/tmail-flutter
Length of output: 942
🏁 Script executed:
cat scribe/pubspec.yamlRepository: linagora/tmail-flutter
Length of output: 2123
🏁 Script executed:
# Look for Dio configuration in bindings or initialization
rg "dio|headers|Content-Type" scribe/lib/scribe/ai/data/network/ -A 3 -B 3 --type dartRepository: linagora/tmail-flutter
Length of output: 1591
🏁 Script executed:
# Check for any Dio interceptors or global configuration
rg "Dio\(|BaseOptions|interceptors|Headers" scribe/lib/ -A 5 --type dartRepository: linagora/tmail-flutter
Length of output: 48
Add explicit Content-Type header for Dio 5.0.0 compatibility.
Dio 5.0.0 no longer automatically sets the Content-Type header for JSON payloads. Passing data: request.toJson() (a Map) without an explicit Content-Type: application/json header will result in the request being sent as application/x-www-form-urlencoded, causing the JMAP backend request to fail.
Set the Content-Type header explicitly:
- Add
options: Options(contentType: Headers.jsonContentType)to the post call, or - Configure globally via
dio.options.contentType = Headers.jsonContentType
🤖 Prompt for AI Agents
In scribe/lib/scribe/ai/data/network/ai_api.dart around lines 24 to 27, the POST
call sends a Map payload without an explicit Content-Type so Dio 5 defaults to
application/x-www-form-urlencoded; explicitly set Content-Type to
application/json by adding options: Options(contentType:
Headers.jsonContentType) to the _dio.post(...) call (or alternatively set
dio.options.contentType = Headers.jsonContentType globally during Dio
initialization) to ensure the request is sent as JSON.
There was a problem hiding this comment.
Seems untrue in network browser logs
There was a problem hiding this comment.
@zatteo, thank you for checking the network logs! If the Content-Type header is being set correctly in the actual requests, then my concern was unfounded. Dio may be handling the Map-to-JSON conversion and header setting appropriately in your configuration. I appreciate you verifying the behavior empirically.
🧠 Learnings used
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:50.713Z
Learning: The tmail-flutter codebase uses `DioError` consistently across all modules, even with Dio 5.0.0. Do not suggest replacing `DioError` with `DioException` unless it's part of a codebase-wide migration.
| Session? get session; | ||
| AccountId? get accountId; | ||
|
|
||
| bool get isAIScribeAvailable { |
There was a problem hiding this comment.
ai stuff in lib/features/composer ?
There was a problem hiding this comment.
Right this makes not a lot of sense here.
There was a problem hiding this comment.
Actually it makes no sense in scribe module. So it is maybe more a tmail generic ai stuff.
| Session? get session => mailboxDashBoardController.sessionCurrent; | ||
|
|
||
| @override | ||
| AccountId? get accountId => mailboxDashBoardController.accountId.value; |
There was a problem hiding this comment.
I found it hard, if more and more mixin also need session or accountId
There was a problem hiding this comment.
Agree. I am not very fond of this part. Open to suggestion or rework.
|
|
||
| bool get isAIScribeAvailable { | ||
| if (!AIConfig.isAiEnabled) return false; | ||
|
|
There was a problem hiding this comment.
| if (PlatformInfo.isMobile) return false; |
|
|
||
| void injectAIScribeBindings(Session? session, AccountId? accountId) { | ||
| try { | ||
| if (session == null || accountId == null) return; |
There was a problem hiding this comment.
if (!PlatformInfo.isWeb) return;
| if (Get.isRegistered<GenerateAITextInteractor>()) { | ||
| Get.delete<GenerateAITextInteractor>(force: true); | ||
| } | ||
|
|
There was a problem hiding this comment.
how about ?
if (scribeEndpoint == null || scribeEndpoint.isEmpty) return;
| } | ||
|
|
||
| // Reinitialize with the correct endpoint | ||
| AIScribeBindings(scribeEndpoint: scribeEndpoint).dependencies(); |
There was a problem hiding this comment.
how about to change the approach, instead of check and delete then re-initialize why not using DioInterceptor to setup endpoint for dio, then we don't care about isRegistered?
| class AIConfig { | ||
| const AIConfig._(); | ||
|
|
||
| static bool get isAiEnabled => dotenv.get('AI_ENABLED', fallback: 'false') == 'true'; |
There was a problem hiding this comment.
do you think we still need it?
| final url = _endpoint; | ||
|
|
||
| if (url == null) { | ||
| throw Exception('AI API is not available'); |
There was a problem hiding this comment.
how about strong type an exception for this?
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.
zatteo
force-pushed
the
ai-scribe-jmap
branch
from
2f2ac2e to
fe624a0
Compare
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.