TF-4449 ADR External drive attachments#4569
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds ADRs 92–94 defining an intent-based external-drive attachment flow: derive the drive platform URL from OIDC workplaceFqdn (https enforced) and expose via Riverpod, exchange id_token for an in-memory drive access token, POST /intents to create a PICK intent (validate https URL), render the intent URL in a modal (iframe on web, InAppWebView on mobile) guarded by per-session IDs, perform a strict postMessage handshake (intent-{id}:ready → ack → done|error|cancel) with origin and intent-id binding and a 30s ready timeout, and map done payload items to DriveAttachment (sharingLink preferred). Implementation constrained to a drive_attachment package. Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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.
chibenwa
left a comment
chibenwa
left a comment
There was a problem hiding this comment.
Please in term of code manage to get drive related stuff in a dedicate folder / module.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
docs/adr/0092-external-drive-file-picker-integration.md (1)
50-65: ⚡ Quick winAdd language specifier to fenced code block.
The code block should specify
httpas the language for proper syntax highlighting and linting compliance.📝 Proposed fix
-``` +```http POST https://<platformUrl>/intents Authorization: Bearer <drive-access-token> Content-Type: application/json🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/adr/0092-external-drive-file-picker-integration.md` around lines 50 - 65, The fenced code block containing the HTTP example starting with "POST https://<platformUrl>/intents" should include a language specifier for proper highlighting and linting; update the opening fence from ``` to ```http for that block (the POST request example and its headers/body) so the snippet is marked as HTTP.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/adr/0092-external-drive-file-picker-integration.md`:
- Around line 111-118: The markdown code fence is malformed (`A``dart`) causing
rendering issues; update the opening fence to a proper triple-backtick with
language `dart` and ensure the matching closing triple-backtick is present
around the snippet that calls controller.evaluateJavascript(...) so the block
containing the MessageEvent dispatch (using $jsonPayload and $intentOrigin) is
rendered correctly.
- Line 185: Clarify the ADR by choosing one of two behaviors and documenting it
explicitly: either (A) require the `done` payload to always include at least one
of `sharingLink` or `downloadLink` and state that implementations MUST
reject/flag items lacking both before mapping to `DriveAttachment`, or (B)
specify the fallback download flow (include a documented fallback endpoint URL
pattern, auth method, expected request/response shape, error handling, and
rate-limit/timeout behavior) and describe how the downloaded bytes are converted
into a regular attachment and attached to the composer attachment panel;
reference `done` payload, `sharingLink`, `downloadLink`, `DriveAttachment`, and
the composer attachment panel in the updated text so readers can find the exact
behavior.
---
Nitpick comments:
In `@docs/adr/0092-external-drive-file-picker-integration.md`:
- Around line 50-65: The fenced code block containing the HTTP example starting
with "POST https://<platformUrl>/intents" should include a language specifier
for proper highlighting and linting; update the opening fence from ``` to
```http for that block (the POST request example and its headers/body) so the
snippet is marked as HTTP.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0a5040c6-14c4-4029-a36b-a7e86bd2eb8a
📒 Files selected for processing (1)
docs/adr/0092-external-drive-file-picker-integration.md
| ```javascript | ||
| (function() { | ||
| window.parent = { | ||
| postMessage: function(data, targetOrigin) { |
There was a problem hiding this comment.
how about strong type the data of postMessage? we want to ensure the data in compile time to reduce the risk of message exchange? WDYT?
There was a problem hiding this comment.
Yes updated
Updated and addressed in the ADR |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
@zatteo It would help if we have your knowledge and opinion on this matter, as if I'm not wrong, cozy drive will be the first to be implemented for this, and you already have a POC regarding this feature. |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
docs/adr/0092-external-drive-file-picker-integration.md (2)
48-71: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winSpecify response validation beyond HTTP errors.
Step 3 parses
data.attributes.services[0].hrefanddata.idfrom the intent creation response, but does not specify what happens if the response is HTTP 200 but malformed (e.g.,servicesarray is empty,services[0].hrefis missing, ordata.idis missing). The error matrix covers HTTP errors but not structural validation failures. Consider adding a note about validating the response structure and specifying the action when required fields are absent.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/adr/0092-external-drive-file-picker-integration.md` around lines 48 - 71, Update Step 3 to validate the intent creation response structure beyond HTTP status: ensure data exists, data.id is present, and data.attributes.services is a non-empty array with services[0].href present and starting with "https://"; if any of these checks fail, treat it as an error path (reject the intent) and surface a clear error message indicating which field is missing (e.g., missing data.id or missing services[0].href) so callers can handle malformed 200 responses consistently.
40-46:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd token exchange error handling to the error matrix.
Step 2 describes calling
POST https://<platformUrl>/auth/token_exchange, but the error handling matrix in Step 7 (lines 95-102) does not cover token exchange failures. Token exchange can fail due to invalidid_token, network issues, platform unavailability, or HTTP 401/403 responses. Please add a row to the error matrix specifying the action for token exchange failures.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/adr/0092-external-drive-file-picker-integration.md` around lines 40 - 46, Add a row to the error matrix in Step 7 that covers failures of the token exchange call described in Step 2 (POST https://<platformUrl>/auth/token_exchange), listing causes (invalid id_token, network/platform unavailable, HTTP 401/403) and the remediation: retry with exponential backoff for transient/network errors, surface a clear user-facing message prompting re-authentication when receiving 401/403 or invalid id_token, and log/debug details for platform errors; reference the "token_exchange" flow and Step 2 in the matrix entry so reviewers can locate the related behavior.
🧹 Nitpick comments (4)
docs/adr/0092-external-drive-file-picker-integration.md (3)
147-147: 💤 Low valueConsider using "use cases" (two words) in prose.
Line 147 uses "usecases" as one word. While acceptable in code identifiers, documentation typically uses "use cases" (two words) when referring to the architectural concept in prose.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/adr/0092-external-drive-file-picker-integration.md` at line 147, The prose uses the concatenated term "usecases"—please change it to the two-word phrase "use cases" wherever it appears in this document (notably in the sentence describing that ComposerController calls through domain usecases and does not import drive presentation classes); leave code identifiers like drive_attachment and ComposerController unchanged, only update the human-readable documentation text to "use cases".
104-121: 💤 Low valueClarify the user-facing behavior when attachment operation fails.
Line 121 states "If neither is present, consider the attaching operation failed," but does not specify the user-visible outcome. Should the UI show an error message, silently drop the item, or log a warning? Adding one sentence to clarify the expected behavior would help implementers.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/adr/0092-external-drive-file-picker-integration.md` around lines 104 - 121, Update the Step 8 — Attach Documents section to specify the user-facing behavior when attachment fails: state that if neither sharingLink nor downloadLink is present for an item in the done payload, the client should surface a clear inline error message in the composer attachment panel (and optionally a toast) indicating the attachment failed, do not attach the item to the message, and log a warning for diagnostics; reference the DriveAttachment mapping and the sharingLink/downloadLink fields so implementers know where to enforce this.
127-145: 💤 Low valueAdd language specifier to the code fence.
The code fence at line 127 is missing a language specifier, which causes a markdown linting warning. Consider adding a language identifier like
textorplaintextto improve rendering consistency.📝 Proposed fix
-``` +```text drive_attachment/ pubspec.yaml # name: drive_attachment, resolution: workspace lib/🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/adr/0092-external-drive-file-picker-integration.md` around lines 127 - 145, The markdown code fence that contains the directory tree starting with "drive_attachment/" is missing a language specifier; update the opening triple-backtick fence for that block (the fenced block showing the drive_attachment/ tree in the ADR) to include a language identifier such as text or plaintext (e.g., change ``` to ```text) so the markdown linter warning is resolved and rendering is consistent.docs/adr/0094-render-intent-url-in-webview-modal.md (1)
100-110: 💤 Low valueClarify listener removal terminology.
Line 106 mentions "Unregister the web
window.onMessagelistener" but based on ADR-0093 line 69 and 77, the implementation usesweb.window.addEventListener('message', _listener)andremoveEventListener. Consider rephrasing to "Remove the webwindowmessage event listener" for consistency with the actual addEventListener/removeEventListener API.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/adr/0094-render-intent-url-in-webview-modal.md` around lines 100 - 110, Change the wording in the lifecycle/cleanup step to match the actual DOM API: instead of "Unregister the web `window.onMessage` listener" use phrasing like "Remove the web `window` message event listener (added via window.addEventListener('message', _listener) and removed via window.removeEventListener('message', _listener))" so it consistently reflects the addEventListener/removeEventListener pattern used by the implementation and keep the rest of the steps referencing canceling the ready-timeout timer, nulling `_activeSessionId`, calling `Navigator.pop(result)`, and invoking `_closeModal(result)` for teardown.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/adr/0093-postmessage-handshake-for-drive-intent-webview.md`:
- Around line 138-142: The origin check in _onMessage currently allows a null
origin to pass (if (origin != null && origin != _intentOrigin)), which lets
messages with a null origin through and can be spoofed by a postMessage without
targetOrigin; change the validation to require a non-null origin that matches
_intentOrigin (e.g., treat origin == null as invalid) so that _onMessage rejects
null or mismatched origins; update any related comments or tests for the shim
that forwards targetOrigin to ensure _intentOrigin remains authoritative (refer
to the _onMessage handler and the _intentOrigin value used for validation).
- Line 13: Update the incorrect platform statement: replace the sentence
claiming "On web flutter_inappwebview renders an <iframe>" with a corrected
description that flutter_inappwebview is mobile-only and that on web Flutter's
HtmlElementView creates the iframe (per ADR-0094 §2 and the document's later
text around the handshake). Edit the sentence that references
flutter_inappwebview to mention mobile/tablet usage only and explicitly state
that HtmlElementView is responsible for the web iframe behavior so the doc
aligns with ADR-0094 and the content at the later paragraph (around the existing
line 178).
- Around line 32-40: The shim for window.parent.postMessage should not fall back
to '*'—remove the targetOrigin || '*' fallback in the postMessage shim and
instead require a valid origin: if targetOrigin is absent throw or assert and
use the known _intentOrigin as the enforced origin; update the shim around the
callHandler('driveIntentMessage', ...) in the window.parent.postMessage
implementation to pass either the provided targetOrigin or _intentOrigin (or
raise an error when neither is present) so the drive app always supplies and
uses a restrictive origin rather than "*".
---
Outside diff comments:
In `@docs/adr/0092-external-drive-file-picker-integration.md`:
- Around line 48-71: Update Step 3 to validate the intent creation response
structure beyond HTTP status: ensure data exists, data.id is present, and
data.attributes.services is a non-empty array with services[0].href present and
starting with "https://"; if any of these checks fail, treat it as an error path
(reject the intent) and surface a clear error message indicating which field is
missing (e.g., missing data.id or missing services[0].href) so callers can
handle malformed 200 responses consistently.
- Around line 40-46: Add a row to the error matrix in Step 7 that covers
failures of the token exchange call described in Step 2 (POST
https://<platformUrl>/auth/token_exchange), listing causes (invalid id_token,
network/platform unavailable, HTTP 401/403) and the remediation: retry with
exponential backoff for transient/network errors, surface a clear user-facing
message prompting re-authentication when receiving 401/403 or invalid id_token,
and log/debug details for platform errors; reference the "token_exchange" flow
and Step 2 in the matrix entry so reviewers can locate the related behavior.
---
Nitpick comments:
In `@docs/adr/0092-external-drive-file-picker-integration.md`:
- Line 147: The prose uses the concatenated term "usecases"—please change it to
the two-word phrase "use cases" wherever it appears in this document (notably in
the sentence describing that ComposerController calls through domain usecases
and does not import drive presentation classes); leave code identifiers like
drive_attachment and ComposerController unchanged, only update the
human-readable documentation text to "use cases".
- Around line 104-121: Update the Step 8 — Attach Documents section to specify
the user-facing behavior when attachment fails: state that if neither
sharingLink nor downloadLink is present for an item in the done payload, the
client should surface a clear inline error message in the composer attachment
panel (and optionally a toast) indicating the attachment failed, do not attach
the item to the message, and log a warning for diagnostics; reference the
DriveAttachment mapping and the sharingLink/downloadLink fields so implementers
know where to enforce this.
- Around line 127-145: The markdown code fence that contains the directory tree
starting with "drive_attachment/" is missing a language specifier; update the
opening triple-backtick fence for that block (the fenced block showing the
drive_attachment/ tree in the ADR) to include a language identifier such as text
or plaintext (e.g., change ``` to ```text) so the markdown linter warning is
resolved and rendering is consistent.
In `@docs/adr/0094-render-intent-url-in-webview-modal.md`:
- Around line 100-110: Change the wording in the lifecycle/cleanup step to match
the actual DOM API: instead of "Unregister the web `window.onMessage` listener"
use phrasing like "Remove the web `window` message event listener (added via
window.addEventListener('message', _listener) and removed via
window.removeEventListener('message', _listener))" so it consistently reflects
the addEventListener/removeEventListener pattern used by the implementation and
keep the rest of the steps referencing canceling the ready-timeout timer,
nulling `_activeSessionId`, calling `Navigator.pop(result)`, and invoking
`_closeModal(result)` for teardown.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 15284ddb-cfdb-4df6-af49-e488d2e1fe68
📒 Files selected for processing (3)
docs/adr/0092-external-drive-file-picker-integration.mddocs/adr/0093-postmessage-handshake-for-drive-intent-webview.mddocs/adr/0094-render-intent-url-in-webview-modal.md
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
|
||
| Feature is shown only when `workplaceFqdn` is non-null in the current session. | ||
|
|
||
| ### Step 2 — Platform Access Token |
There was a problem hiding this comment.
@chibenwa do you think it still a generic way to not depend on any specific Drive?
There was a problem hiding this comment.
I think for easier debug having a way to configure the file picker URL is nice.
Other than this I am fine to start with platform approache and add non platform bypass when needed. If ever needed.
| window.flutter_inappwebview.callHandler( | ||
| 'driveIntentMessage', | ||
| typeof data === 'string' ? data : JSON.stringify(data), | ||
| targetOrigin | ||
| ); |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/adr/0092-external-drive-file-picker-integration.md`:
- Around line 146-148: The fenced code block containing the HTTP header example
(the block showing "X-LinkedFile: url=...") is missing a language identifier;
update that fenced block in
docs/adr/0092-external-drive-file-picker-integration.md to include a language
(e.g., "http") so it renders and lints correctly — locate the fenced block with
the X-LinkedFile header and change the opening fence from ``` to ```http.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c6645b62-76ed-4c03-b6a5-a3f56e6ca993
📒 Files selected for processing (3)
docs/adr/0092-external-drive-file-picker-integration.mddocs/adr/0093-postmessage-handshake-for-drive-intent-webview.mddocs/adr/0094-render-intent-url-in-webview-modal.md
✅ Files skipped from review due to trivial changes (2)
- docs/adr/0094-render-intent-url-in-webview-modal.md
- docs/adr/0093-postmessage-handshake-for-drive-intent-webview.md
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Our agent can fix these. Install it.
No application code in the PR — skipped Code Health checks.
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.
|
Two tech/product questions that could imply params to send to the intent:
cc @chibenwa |
Not especially.
Yes. JMAP adverize a maximum attachment size that should be honorred when attaching a file to a mail. However if we share by link such limit do not exist.
A classic mail thing is eg to forbid sending Write now we can enforce this backend side when processing the mail but we lack the tools to do it upon sending (both SMTP and JMAP) + we indeed need to avertize forbidden file extensions to the front. That could make a VERY nice JMAP extension BTW :-) As off today let's say we do not push such restrictions (yet!) |
5 participants
Summary
Adds ADR 92, 93 and 94 documenting the design for external drive file attachment flow in the composer.
Related
What the ADR covers
workplaceFqdnin existing OIDC userinfo (no new network call)window.addEventListener)Open Questions
EmailBodyParthas no URL-link field — external-link attachment encoding unresolved (proprietary James extension vs HTML<a href>insertion)sharingLinkrequires a drive account the recipient doesn't haveSummary by CodeRabbit
#92–Add bitrise.yml for the CI job #94) for external drive file picker via an intent protocol.