TF-4660 Drive loading#4699
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:
WalkthroughThe Drive attachment flow now resolves intent data asynchronously in web and mobile modals, displays platform-specific skeleton loaders until readiness, and reports intent errors through callbacks. Modal layout options were expanded, picker error handling was centralized, and context-menu navigation closes before opening the picker. Message handling now gates early events and adds readiness timeout hooks and tests. The exchange-token request sends an 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.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@workplace/lib/presentation/mixin/drive_picker_state_mixin.dart`:
- Around line 58-64: Capture the localized attach-from-Drive failure message
while the picker context is still mounted, before Navigator.pop(context)
disposes the state, and pass that value through the picker flow into
_onIntentError or DrivePickFailure instead of resolving it there via
mounted/context.
In `@workplace/lib/presentation/view/drive_intent_web_view_modal_mobile.dart`:
- Around line 39-58: Ensure resolved intents are not dropped when the WebView is
created after the future completes: store the resolved intent in the state, then
centralize the data/URL loading logic and invoke it from both the future
completion handler and onWebViewCreated. Update the relevant intent state and
WebView initialization methods so whichever completes last performs the load
once the controller is available.
In `@workplace/lib/presentation/view/drive_intent_web_view_modal_web.dart`:
- Around line 48-60: Store the resolved intent in state when the
`widget.intentFuture.then` callback runs, rather than relying only on
`_iframeElement`. Update `onIframeCreated` to check for the buffered intent and
perform the `srcdoc` removal and URL navigation there; reuse a helper if
appropriate so both paths apply the same intent once the iframe exists.
🪄 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: 0ebca8cc-2854-48d1-b829-f1459ea637d6
📒 Files selected for processing (9)
workplace/lib/data/datasource_impl/workplace_datasource_impl.dartworkplace/lib/presentation/mixin/drive_intent_message_handler_mixin.dartworkplace/lib/presentation/mixin/drive_picker_state_mixin.dartworkplace/lib/presentation/notifier/drive_attachment_state.dartworkplace/lib/presentation/view/drive_intent_skeleton_page.dartworkplace/lib/presentation/view/drive_intent_web_view_modal_mobile.dartworkplace/lib/presentation/view/drive_intent_web_view_modal_shell.dartworkplace/lib/presentation/view/drive_intent_web_view_modal_web.dartworkplace/lib/presentation/widget/drive_attachment_context_menu_tile.dart
💤 Files with no reviewable changes (1)
- workplace/lib/presentation/notifier/drive_attachment_state.dart
|
This PR has been deployed to https://linagora.github.io/tmail-flutter/4699. |
|
Docker image published for this PR: linagora/tmail-web-pr:4699 |
|
The only cue for that is ready postMessage, but that would be web only at the moment, as mobile is still stuck with login screen |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
So let's fix it for the web version first; for the mobile version, please comment so we can fix it later. Also, update the demo to work correctly for the web version. |
Done and demos updated |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
workplace/lib/presentation/view/drive_intent_web_view_modal_web.dart (1)
49-58: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winIntent URL can be silently dropped if the future resolves before
onIframeCreated.
_handleIntentResolvedsets_iframeElement?.src, but_iframeElementis only assigned inonIframeCreated. IfintentFuturesettles first, the?.srcassignment is a no-op and nothing reapplies the URL afterward — the skeleton stays visible indefinitely. This was flagged in a prior review and remains unresolved.🔒 Proposed fix: buffer the resolved intent and apply from both paths
class _DriveIntentWebViewModalState extends State<DriveIntentWebViewModal> with DriveIntentMessageHandlerMixin, WebWindowMessageMixin<DriveIntentWebViewModal> { html.IFrameElement? _iframeElement; String _intentOrigin = 'null'; bool _showSkeleton = true; + WorkplaceIntent? _resolvedIntent;void _handleIntentResolved(WorkplaceIntent intent) { if (!mounted) return; - final origin = intent.intentUrl.scheme == 'data' ? 'null' : intent.intentUrl.origin; - _intentOrigin = origin; - initMessageHandler(intentId: intent.intentId, intentOrigin: origin); - _iframeElement?.src = intent.intentUrl.toString(); + _resolvedIntent = intent; + _applyIntent(); + } + + void _applyIntent() { + final intent = _resolvedIntent; + final iframe = _iframeElement; + if (intent == null || iframe == null) return; + final origin = intent.intentUrl.scheme == 'data' ? 'null' : intent.intentUrl.origin; + _intentOrigin = origin; + initMessageHandler(intentId: intent.intentId, intentOrigin: origin); + iframe.src = intent.intentUrl.toString(); }HtmlIframeWidget( key: const ValueKey('drive-intent-webview'), onIframeCreated: (iframe) { _iframeElement = iframe; + _applyIntent(); }, ),Also applies to: 94-96
🤖 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 `@workplace/lib/presentation/view/drive_intent_web_view_modal_web.dart` around lines 49 - 58, Buffer the resolved WorkplaceIntent in _handleIntentResolved instead of relying on _iframeElement being available, and add a shared helper to apply the buffered intent URL when the iframe exists. Invoke that helper from both _handleIntentResolved and onIframeCreated, ensuring the iframe receives the URL regardless of which callback runs first and preserving the existing origin/message-handler setup.
♻️ Duplicate comments (1)
workplace/lib/presentation/view/drive_intent_web_view_modal_mobile.dart (1)
40-58: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winIntent load can be dropped if the future resolves before
onWebViewCreatedfires.The
.thencallback calls_webViewController?.loadData/loadUrl, but_webViewControlleris only assigned inonWebViewCreated(line 79). On mobile,InAppWebViewcreation is asynchronous via platform channels, so the race is more likely than on web. If the controller isn't ready, the load is a no-op and the skeleton stays forever. This was flagged in a prior review and remains unresolved.🔒 Proposed fix: buffer the resolved intent and load from both paths
class _DriveIntentWebViewModalState extends State<DriveIntentWebViewModal> with DriveIntentMessageHandlerMixin { InAppWebViewController? _webViewController; String _intentOrigin = 'null'; bool _showSkeleton = true; + WorkplaceIntent? _resolvedIntent;widget.intentFuture.then((intent) { if (!mounted) return; - final isDataUri = intent.intentUrl.scheme == 'data'; - final origin = isDataUri ? 'null' : intent.intentUrl.origin; - _intentOrigin = origin; - initMessageHandler(intentId: intent.intentId, intentOrigin: origin); - if (isDataUri) { - _webViewController?.loadData( - data: DriveIntentFakePage.buildHtml(intent.intentId), - ); - } else { - _webViewController?.loadUrl( - urlRequest: URLRequest(url: WebUri.uri(intent.intentUrl)), - ); - } + _resolvedIntent = intent; + _applyIntent(); }).catchError((Object e) {+ void _applyIntent() { + final intent = _resolvedIntent; + final controller = _webViewController; + if (intent == null || controller == null) return; + final isDataUri = intent.intentUrl.scheme == 'data'; + final origin = isDataUri ? 'null' : intent.intentUrl.origin; + _intentOrigin = origin; + initMessageHandler(intentId: intent.intentId, intentOrigin: origin); + if (isDataUri) { + controller.loadData(data: DriveIntentFakePage.buildHtml(intent.intentId)); + } else { + controller.loadUrl(urlRequest: URLRequest(url: WebUri.uri(intent.intentUrl))); + } + } + `@override` Widget build(BuildContext context) {onWebViewCreated: (controller) { _webViewController = controller; controller.addJavaScriptHandler( handlerName: DriveIntentShims.handlerName, callback: (args) => onMessage( raw: args[0] as String, origin: args.length > 1 ? args[1] as String? : null, ), ); + _applyIntent(); },Also applies to: 78-79
🤖 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 `@workplace/lib/presentation/view/drive_intent_web_view_modal_mobile.dart` around lines 40 - 58, Buffer the resolved intent when the future completes before the WebView controller exists, instead of dropping the load. Update the intent-loading logic and onWebViewCreated callback to retain the resolved intent and invoke a shared load routine from both paths, ensuring it runs only when mounted and _webViewController is available while preserving error handling.
🤖 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.
Outside diff comments:
In `@workplace/lib/presentation/view/drive_intent_web_view_modal_web.dart`:
- Around line 49-58: Buffer the resolved WorkplaceIntent in
_handleIntentResolved instead of relying on _iframeElement being available, and
add a shared helper to apply the buffered intent URL when the iframe exists.
Invoke that helper from both _handleIntentResolved and onIframeCreated, ensuring
the iframe receives the URL regardless of which callback runs first and
preserving the existing origin/message-handler setup.
---
Duplicate comments:
In `@workplace/lib/presentation/view/drive_intent_web_view_modal_mobile.dart`:
- Around line 40-58: Buffer the resolved intent when the future completes before
the WebView controller exists, instead of dropping the load. Update the
intent-loading logic and onWebViewCreated callback to retain the resolved intent
and invoke a shared load routine from both paths, ensuring it runs only when
mounted and _webViewController is available while preserving error handling.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a2e3b5f6-07a6-46cc-a918-5d79176dd37c
📒 Files selected for processing (5)
workplace/lib/presentation/mixin/drive_intent_message_handler_mixin.dartworkplace/lib/presentation/mixin/drive_picker_state_mixin.dartworkplace/lib/presentation/view/drive_intent_skeleton_loader.dartworkplace/lib/presentation/view/drive_intent_web_view_modal_mobile.dartworkplace/lib/presentation/view/drive_intent_web_view_modal_web.dart
💤 Files with no reviewable changes (1)
- workplace/lib/presentation/mixin/drive_picker_state_mixin.dart
Currently: 
|
|
Docker image published for this PR: linagora/tmail-web-pr:4699 |
Radius added |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Screen.Recording.2026-07-23.at.2.48.34.PM.mov |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Screen.Recording.2026-07-23.at.3.13.03.PM.mov |
|
Docker image published for this PR: linagora/tmail-web-pr:4699 |
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:4699 |
|
|
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Done
untitled.webm |
|
Docker image published for this PR: linagora/tmail-web-pr:4699 |
|
Done |
There was a problem hiding this comment.
Our agent can fix these. Install it.
Gates Passed
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:4699 |
4 participants
Issue
Demo
Web
Screen.Recording.2026-07-22.at.3.47.19.PM.mov
Mobile
Mobile has a bug where drive is stuck at login stage, cannot end the loading, but as soon as it is fixed, the loading will go away without any further changes.
drive-mobile-loading.webm
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Tests