[WIP] TF-4449 Implement external drive attachments

[tddang-linagora]

Issue

• TF-4449 ADR External drive attachments #4569

[coderabbitai]

Important

Review skipped

Draft detected.

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.

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: cdbdb830-da8a-4895-a43a-a3e61ca48dc6

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

Use the checkbox below for a quick retry:

• 🔍 Trigger review

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch feature/TF-4449-External-drive-attachments

---

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

• X
• Mastodon
• Reddit
• LinkedIn

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

[github-actions]

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

[dab246]

Using Dio with an interceptor already in place. Why do we need to set authentication in the header? What about token refresh cases?

[hoangdat]

This is token for other service not for jmap, so we must have a dedicated http client for this.
Yes, good question for refreshToken here @zatteo

[zatteo]

Good question, we will need to get and manage refreshToken or we can take hypothesis that these are one shot call that must be done directly after token_exchange. I am writing this in backend questions.

[tddang-linagora]

Pending until further progress regarding token for external drive's service.

[dab246]

We should create a model for payload data.

[hoangdat]

Agree, now 1 case of intent, but in the future we have variety intents

[tddang-linagora]

Done

[dab246]

Should we return an error earlier if parsed.data.attributes.services is empty?

[tddang-linagora]

If services is empty, .first will throw and the function will stop here.

[chibenwa]

IMO design can be refined

[chibenwa]

Is this drive button displayed only on environment that supports it?

[tddang-linagora]

Current implementation: If you have workplaceFqdn url, you have the button.
After discussed with the team and Zoe: The button will have 2 more gates, one in env.file file, the other in Preferences toggle (experimental flag, hide behind 7 taps on the title of settings).

[chibenwa]

No I disagree with this !

The concept of drive attachment should not leak into our main model.

We should have the "drive layer" do the translation and provide the "create_email_request" properties that we need!

[tddang-linagora]

Done

[chibenwa]

(if spotted - if your remeber what I said yesterday during the MOB it may ring a bell)

[hoangdat]

Yeah a good point to invest: how we can plug and play the UI component?

[chibenwa]

Same for Scribe !

[chibenwa]

Can we avoid this direct dependency?

In Java I would...

• define an interface with the behaviour / info needed
• implement the interface in my extension
• Use the interface in the other part of the code
• And do the link with dependency injection

That avoids direct dependency and allow pluging alternative mechanisms in alternative extensions eg JMAP file extension. At a very minimal cost.

[tddang-linagora]

Until there are multiple behavior that can be injected to this particular button of external drive, it is moved into its function's module, and we respect YAGNI in the meantime.

[chibenwa]

Same ! Have an interface...

[tddang-linagora]

Same also

[chibenwa]

Why do we need changes in JMAP dart client ?

[hoangdat]

To query header of linked file

[chibenwa]

Why do we need to overload this ?

Can't we just have a list of headers and associated tyes we pass to the jmap client ?

[tddang-linagora]

@chibenwa does "X-LinkedFile:all:asText" need to be individual header that needs header: prefix or it can be inside headers field? If individual header, jmap-dart-client's current implementation only allows specified header to be parsed from and to json. And need at least 1 commit to allow dynamically adding more.

[tddang-linagora]

If it can be inside headers field then the PR for "feature/linked-file-header" can be closed.

[chibenwa]

We cannot set headers and individual header indiviually i't one or the other.

I'm fine with the proposal to add extra headers (though I want us to improve the user experience of it)

[tddang-linagora]

Now I understood what you meant. For now we will not update jmap-dart-client dependency of tmail-flutter and inject/parse the desired header from headers field.

Nevermind. it is still needed after all.

[chibenwa]

@coderabbitai review this please

[dab246]

Should use icon in design figma

[coderabbitai]

Tip

For best results, initiate chat on the files or code changes.

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Fetch PR details
gh pr view 4581 --repo linagora/tmail-flutter --json title,body,state,additions,deletions,changedFiles,labels,reviewRequests

echo "=== FILES CHANGED ==="
gh pr diff 4581 --repo linagora/tmail-flutter --name-only

echo "=== FULL DIFF ==="
gh pr diff 4581 --repo linagora/tmail-flutter

Length of output: 71484

---

---

@chibenwa: Here is a full review of the PR. Since it's marked [WIP] I've tried to flag both blocking issues and things worth addressing before merge.

---

Overall Summary

The PR introduces a new drive_attachment package that wires a Cozy Drive file-picker (intent API + token exchange) into the TMail composer. The architecture is clean (data / domain / presentation split, Riverpod state), but there are several crash-level bugs, a security/architecture concern around the shared Dio instance, and a few logic errors that should be fixed before this ships.

---

🔴 Blocking Issues

1. Unguarded .first on services list — RangeError crash

File: drive_attachment/lib/drive_attachment/data/datasource_impl/drive_attachment_datasource_impl.dart

// current – throws RangeError when services is empty
final href = parsed.data.attributes.services.first.href;

Add a guard:

final services = parsed.data.attributes.services;
if (services.isEmpty) {
  throw StateError('createIntent: no services returned in intent response');
}
final href = services.first.href;

---

2. Unsafe cast on token-exchange response — TypeError crash

File: same file, exchangeToken

return response.data['access_token'] as String;   // throws if key absent or value is null

Safer:

final token = response.data['access_token'];
if (token is! String) throw StateError('exchangeToken: missing access_token in response');
return token;

---

3. DriveIntentParseException is thrown but never caught — unhandled exception

File: drive_attachment/lib/drive_attachment/presentation/mixin/drive_intent_message_handler_mixin.dart

try {
  msg = DriveIntentMessage.parse(_intentId, raw);
} catch (_) {
  throw DriveIntentParseException(raw);   // ← re-thrown, no one catches it
}

This propagates out of the addJavaScriptHandler / window.addEventListener callback and can crash the app silently. Log and return instead:

} catch (_) {
  log('driveIntent: failed to parse message: $raw');
  return;
}

---

4. Shared Dio instance + drive's own Authorization header = double-header conflict

File: lib/main/bindings/network/network_bindings.dart + datasource_impl

setupDriveAttachment(Get.find<Dio>()) passes the app-wide Dio instance (which already has AuthorizationInterceptors adding a JMAP Bearer token), yet createIntent also manually sets 'Authorization': 'Bearer $accessToken'. On requests where both fire, the drive token will likely be overwritten by the JMAP interceptor, or vice versa depending on interceptor order.

Recommendation: Create a separate, bare Dio instance for drive attachment requests (no JMAP interceptors):

final driveDio = Dio();          // no JMAP auth interceptor
setupDriveAttachment(driveDio);

---

5. Web modal timeout starts at iframe creation, not page load

File: drive_attachment/lib/drive_attachment/presentation/view/drive_intent_web_view_modal_web.dart

onElementCreated: (element) {
  ...
  onWebViewLoaded();   // fires immediately when the DOM element is created, not after load
},

There is no onLoad event hook for HtmlElementView, but calling onWebViewLoaded() here starts the 30-second ready timer before the network request even begins, making the timeout effectively shorter than intended. Consider:

• not calling onWebViewLoaded() on web (remove the call), or
• starting the timer only after the intent-*:ready message is not received within 30 s of the iframe first becoming interactive (i.e., reset the timer approach).

---

🟡 Important / Should Fix

6. Global mutable state for Dio and OIDC getter — hurts testability

File: drive_attachment/lib/drive_attachment/presentation/provider/drive_attachment_providers.dart

Dio? _dio;
String? Function()? _oidcIdTokenGetter;

Package-level mutables are an anti-pattern in Riverpod apps. Use provider overrides or a Ref-scoped approach instead:

// Expose via a ProviderScope override at app startup
final driveAttachmentDioProvider = Provider<Dio>((ref) => throw UnimplementedError());
final driveOidcTokenGetterProvider = Provider<String? Function()>((ref) => throw UnimplementedError());

Then override them in ProviderScope (or via appProviderContainer) at startup.

---

7. DriveAttachmentNotifier constructor pattern is not idiomatic for Riverpod 3 families

File: drive_attachment/lib/drive_attachment/presentation/notifier/drive_attachment_notifier.dart

With Riverpod ≥ 3, family notifiers should extend FamilyNotifier<State, Arg> and access the family argument via this.arg:

class DriveAttachmentNotifier
    extends FamilyNotifier<DriveAttachmentState, String> {
  // no constructor arg needed; use `arg` property
  `@override`
  DriveAttachmentState build(String composerId) { ... }
}

final driveAttachmentNotifierProvider = NotifierProvider
    .family<DriveAttachmentNotifier, DriveAttachmentState, String>(
  DriveAttachmentNotifier.new,
);

The current pattern compiles but bypasses Riverpod's lifecycle contract.

---

8. URL-construction logic duplicated in three places

fqdn.startsWith('http') ? fqdn : 'https://$fqdn' appears in:

• drive_access_token_notifier.dart
• workplace_fqdn_notifier.dart
• drive_attachment_button.dart

Extract to a single helper, e.g.:

// In workplace_fqdn_notifier.dart or a utils file
Uri fqdnToUri(String fqdn) =>
    Uri.parse(fqdn.startsWith('http') ? fqdn : 'https://$fqdn');

---

9. composerId ?? '' fallback creates a shared/wrong provider key

File: lib/features/composer/presentation/composer_view.dart, composer_view_web.dart

composerId: controller.composerId ?? '',

If composerId is null, all null-composer instances share the '' provider key, potentially mixing attachments across different composers. Either assert non-null earlier, or guard:

if (controller.composerId == null) return const SizedBox.shrink();
DriveAttachmentButton(composerId: controller.composerId!, ...)

---

10. _modalOpen flag is not race-safe — double-tap can open two modals

File: lib/features/composer/presentation/widgets/drive_attachment_button.dart

_openModal is async; a second ref.listen callback could fire before the first await showDialog returns. Consider disabling the button while _modalOpen is true, or guarding earlier in _onTap.

---

11. DriveIntentDoneMessage.fromJson reads 'document' (singular) — verify against API

File: drive_attachment/lib/drive_attachment/domain/message/drive_intent_message.dart

(json['document'] as List? ?? [])

If the Cozy intent API actually sends "documents" (plural), this silently produces an empty list and no files are ever selected. Please confirm the key name against the Cozy intent spec.

---

🟢 Minor / Nits

12. DriveAttachmentListWidget rendered twice in mobile composer

File: lib/features/composer/presentation/composer_view.dart

DriveAttachmentListWidget is added in both the tablet portrait branch (line ~241) and the phone portrait branch (line ~435). Confirm this is intentional and that neither layout causes it to appear twice on the same screen.

---

13. Missing unit tests

There is no test/ directory in drive_attachment/. At a minimum, the following are important:

• DriveIntentMessage.parse for all message types (including malformed JSON)
• DriveAttachmentNotifier state transitions
• WorkplaceFqdnNotifier.setFqdn validation logic

---

14. sendAck uses string interpolation in JS — fragile

File: drive_intent_web_view_modal_mobile.dart

_webViewController?.evaluateJavascript(source: '''
  window.dispatchEvent(new MessageEvent('message', {
    data: '$payload',      // JS injection if payload ever contains '
    ...
  }));
''');

Since payload is always jsonEncode({}) = "{}" this is safe now, but consider using jsonEncode(payload) and proper JS escaping to future-proof it.

---

15. jmap-dart-client pinned to a feature branch in pubspec.lock

ref: "feature/linked-file-header"
resolved-ref: "6d730da063638080a8fe96939c4d0c9f8b3d4275"

This should be switched to a tagged release or merged to main before the PR is marked ready.

[hoangdat]

Why we need this method?

[tddang-linagora]

to generate expected format for the header

[hoangdat]

It quite orphan? It will keep dio for this provider?

[tddang-linagora]

Yes

[codescene-delta-analysis]

Gates Failed
New code is healthy (1 new file with code health below 10.00)

Our agent can fix these. Install it.

Gates Passed
2 Quality Gates Passed

Reason for failure

New code is healthy	Violations	Code Health Impact
drive_intent_message_handler_mixin.dart	1 rule	9.69	Suppress

See analysis details in CodeScene

Absence of Expected Change Pattern

• tmail-flutter/lib/features/composer/presentation/widgets/mobile/app_bar_composer_widget.dart is usually changed with: tmail-flutter/lib/features/composer/presentation/widgets/mobile/landscape_app_bar_composer_widget.dart

Quality Gate Profile: The Bare Minimum
Install CodeScene MCP: safeguard and uplift AI-generated code. Catch issues early with our IDE extension and CLI tool.

[tddang-linagora]

This PR will stop develop here and move to #4587 and #4588.