TF-4410 Propose ADR attach drive as attachment#4688
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
WalkthroughThis pull request adds ADR-0103 documenting Drive attachment behavior. It defines the Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![codescene-delta-analysis[bot]](https://avatars.githubusercontent.com/in/53921?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
Docker image published for this PR: linagora/tmail-web-pr:4688 |
1 similar comment
|
Docker image published for this PR: linagora/tmail-web-pr:4688 |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
Docker image published for this PR: linagora/tmail-web-pr:4688 |
|
|
||
| ## Decision | ||
|
|
||
| **Partitioning:** `DriveAttachmentHandler` splits picked `DriveDocument`s per-doc: `sharingLink != null` → HTML link (unchanged); `downloadLink != null` → download + attach (new). **Invariant:** both fields may be present on one doc (per ADR-0095's payload); when they are, `downloadLink` wins — download+attach only, enforced in the partition step, not call-site order. |
There was a problem hiding this comment.
IMO, sharingLink will win if both exists, user have more benefit from it than download.
|
|
||
| **Download (`workplace` package), streamed:** `DriveFileDatasourceImpl.openFileForUpload(doc)` branches on `kIsWeb`. **IO**: `Dio.get(responseType: stream)` piped directly as the upload request body (`FileInfo.byteStream`) — zero-copy. **Web**: XHR can't stream a body, so web buffers into `FileInfo.bytes`. A bounded worker pool (`kMaxConcurrentDriveTransfers = 3`, new constant, not tied to any existing limit) runs per-file pipelines with no "download all, then upload all" batching. A failing file logs/removes its chip; others continue. | ||
|
|
||
| **Upload pipeline contract:** the worker-isolate upload path (`FileUploader.uploadAttachment` → `UploadFileArguments`) only carries `filePath`/`bytes` across the isolate boundary — a one-shot `Stream<List<int>>` can't cross it. So **drive stream uploads run on the main isolate** instead, bypassing the worker-isolate branch. Local-file/paste/drop uploads are unaffected. |
There was a problem hiding this comment.
what happen if upload has token expired? can we retry?
There was a problem hiding this comment.
Yes, already in Retry policy section.
|
|
||
| **Download (`workplace` package), streamed:** `DriveFileDatasourceImpl.openFileForUpload(doc)` branches on `kIsWeb`. **IO**: `Dio.get(responseType: stream)` piped directly as the upload request body (`FileInfo.byteStream`) — zero-copy. **Web**: XHR can't stream a body, so web buffers into `FileInfo.bytes`. A bounded worker pool (`kMaxConcurrentDriveTransfers = 3`, new constant, not tied to any existing limit) runs per-file pipelines with no "download all, then upload all" batching. A failing file logs/removes its chip; others continue. | ||
|
|
||
| **Upload pipeline contract:** the worker-isolate upload path (`FileUploader.uploadAttachment` → `UploadFileArguments`) only carries `filePath`/`bytes` across the isolate boundary — a one-shot `Stream<List<int>>` can't cross it. So **drive stream uploads run on the main isolate** instead, bypassing the worker-isolate branch. Local-file/paste/drop uploads are unaffected. |
There was a problem hiding this comment.
in case of running in main isolate, what will be impacted?
There was a problem hiding this comment.
Explaination added.
| - A new attachment-eligibility condition is one new class + one line, not a change to every call site. | ||
| - Memory stays flat on IO regardless of file size/concurrent count (zero-copy pipe); web still buffers per file (XHR limitation). | ||
| - User sees live per-file progress immediately instead of a blocking wait. | ||
| - A streamed IO upload isn't 401-retry-safe (one-shot body can't be rebuilt) — accepted and made explicit via `nonRetryableUploadBody`: that file's chip fails and is removed, others continue (see Retry Policy above). |
There was a problem hiding this comment.
To follow zero-copy pipe, no. Explaination added.
|
|
||
| **Progress/cancel model:** `UploadFileStatus` gains `downloading`; `AttachmentUploadState` gains a matching `DownloadingAttachmentUploadState`; the composer progress widget gains a render branch for it. A single `CancelToken` per file spans both the download and upload calls, so cancelling (including deleting the chip mid-transfer) aborts whichever stage is active. | ||
|
|
||
| **Validation — minimal, condition-agnostic abstraction:** |
There was a problem hiding this comment.
I am not still clear about the flow starting from the time we got the document list? validators will be run first -> download -> upload? can you more detail about the flow?
|
|
||
| **Progress/cancel model:** `UploadFileStatus` gains `downloading`; `AttachmentUploadState` gains a matching `DownloadingAttachmentUploadState`; the composer progress widget gains a render branch for it. A single `CancelToken` per file spans both the download and upload calls, so cancelling (including deleting the chip mid-transfer) aborts whichever stage is active. | ||
|
|
||
| **Validation — minimal, condition-agnostic abstraction:** |
There was a problem hiding this comment.
how we handle the case:
- maxFileSize of a document is valid, but total size of files not valid
- totalSize of files is valid, but one file have not valid in file size
|
|
||
| **Visible progress:** each drive file gets a composer attachment chip immediately (`UploadFileStatus.downloading`, color-distinct from `uploading`), fed by `onDownloadProgress`/Dio's existing `onSendProgress`. IO shows one continuous phase (download+upload coupled by the pipe); web shows two sequential phases (download then upload). | ||
|
|
||
| **Progress/cancel model:** `UploadFileStatus` gains `downloading`; `AttachmentUploadState` gains a matching `DownloadingAttachmentUploadState`; the composer progress widget gains a render branch for it. A single `CancelToken` per file spans both the download and upload calls, so cancelling (including deleting the chip mid-transfer) aborts whichever stage is active. |
There was a problem hiding this comment.
not clear to me, how to indicate the download/upload in one progress bar, how to display exactly the progress?
There was a problem hiding this comment.
Answered by Visible progress section
|
|
||
| **Partitioning:** `DriveAttachmentHandler` splits picked `DriveDocument`s per-doc: `sharingLink != null` → HTML link (unchanged); `downloadLink != null` → download + attach (new). **Invariant:** both fields may be present on one doc (per ADR-0095's payload); when they are, `downloadLink` wins — download+attach only, enforced in the partition step, not call-site order. | ||
|
|
||
| **Download (`workplace` package), streamed:** `DriveFileDatasourceImpl.openFileForUpload(doc)` branches on `kIsWeb`. **IO**: `Dio.get(responseType: stream)` piped directly as the upload request body (`FileInfo.byteStream`) — zero-copy. **Web**: XHR can't stream a body, so web buffers into `FileInfo.bytes`. A bounded worker pool (`kMaxConcurrentDriveTransfers = 3`, new constant, not tied to any existing limit) runs per-file pipelines with no "download all, then upload all" batching. A failing file logs/removes its chip; others continue. |
There was a problem hiding this comment.
how about create an interface for DriveFileDownloader then implement for mobile (stream/file temp/ ....) and web?
There was a problem hiding this comment.
YAGNI at the moment. Explaination added.
| - IO shows only a single-phase "downloading"-styled bar for the whole transfer (download/upload are coupled in lockstep) — accepted over forcing IO to buffer for a two-phase visual. | ||
| - Stream receive-timeout is uncapped (`Duration.zero`); a stalled server relies on the user's manual cancel action. **Correction after checking Dio's actual IO adapter source (pinned `dio: 5.2.0`, and current 5.9.0):** `receiveTimeout` was never a whole-transfer watchdog on IO in the first place — it only guards time-to-first-byte/headers (`request.close()`), so the 10s default wasn't going to "kill a slow transfer mid-stream" either way. Setting `Duration.zero` still avoids a spurious timeout before a slow drive file starts streaming, just not for the reason originally stated. | ||
| - Concurrency fixed at 3 concurrent transfers, not user/config-configurable (YAGNI for now). | ||
| - **Web buffering is a browser platform limit, not a Dio-specific gap.** Verified via `dio_web_adapter` 2.1.1 source (current, ships with `dio: 5.9.0`) — still XHR-based, still fully buffers the request stream before `xhr.send()`. A hand-rolled `fetch()` client wouldn't help either: streaming *upload* request bodies only works on Chromium (Chrome/Edge/Opera, v105+, and only over HTTP/2+HTTPS with `duplex: 'half'`, always CORS-preflighted); Firefox and desktop Safari still buffer the full request body as of current stable channels (WebKit is adding it under Interop 2026, landed in a Safari 26.4 beta, not GA). Fetch response-body streaming (download side) is broadly supported, but doesn't remove the need to buffer before upload on non-Chromium engines. Conclusion: the web buffered fallback is correct for all three engines today, not an XHR-only workaround to revisit later. |
There was a problem hiding this comment.
keep it as risk with current choice is not right. WDYT?
|
|
||
| **Partitioning:** `DriveAttachmentHandler` splits picked `DriveDocument`s per-doc: `sharingLink != null` → HTML link (unchanged); `downloadLink != null` → download + attach (new). **Invariant:** both fields may be present on one doc (per ADR-0095's payload); when they are, `downloadLink` wins — download+attach only, enforced in the partition step, not call-site order. | ||
|
|
||
| **Download (`workplace` package), streamed:** `DriveFileDatasourceImpl.openFileForUpload(doc)` branches on `kIsWeb`. **IO**: `Dio.get(responseType: stream)` piped directly as the upload request body (`FileInfo.byteStream`) — zero-copy. **Web**: XHR can't stream a body, so web buffers into `FileInfo.bytes`. A bounded worker pool (`kMaxConcurrentDriveTransfers = 3`, new constant, not tied to any existing limit) runs per-file pipelines with no "download all, then upload all" batching. A failing file logs/removes its chip; others continue. |
There was a problem hiding this comment.
how about in web we use Origin Private File System instead of memory buffer for file?
There was a problem hiding this comment.
Considered but not adopted. Explaination added.
There was a problem hiding this comment.
No application code in the PR — skipped Code Health checks.
See analysis details in CodeScene
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.
|
Docker image published for this PR: linagora/tmail-web-pr:4688 |
4 participants
Issue
Summary by CodeRabbit
sharingLink(HTML link insertion) anddownloadLink(download+attach), including progress behavior across environments.