POC Add File Picker POC in attachment#4448
Draft
zatteo wants to merge 1 commit into
Draft
Conversation
Contributor
WalkthroughThis pull request implements a new file attachment mechanism for the composer across multiple platforms. On Android, cleartext traffic over HTTP is enabled for local testing. The composer controller adds web-specific iframe-based file selection with session tracking and message handling, plus a mobile modal implementation using 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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}
There was a problem hiding this comment.
Gates Passed
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.
chibenwa
changed the title
|
This PR has been deployed to https://linagora.github.io/tmail-flutter/4448. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (1)
lib/features/composer/presentation/widgets/mobile/app_bar_composer_widget.dart (1)
86-94: Hide the cloud action when no handler is provided.
remoteAttachFileActionis nullable, but this button is shown whenever the network is available. A future caller can now render a dead control by forgetting the callback. Gate the button the same way the web bottom bar does.♻️ Suggested change
- TMailButtonWidget.fromIcon( - icon: imagePaths.icCloud, - iconColor: MobileAppBarComposerWidgetStyle.iconColor, - backgroundColor: Colors.transparent, - iconSize: MobileAppBarComposerWidgetStyle.iconSize, - tooltipMessage: AppLocalizations.of(context).browse, - onTapActionCallback: remoteAttachFileAction, - ), - const SizedBox(width: MobileAppBarComposerWidgetStyle.space), + if (remoteAttachFileAction != null) ...[ + TMailButtonWidget.fromIcon( + icon: imagePaths.icCloud, + iconColor: MobileAppBarComposerWidgetStyle.iconColor, + backgroundColor: Colors.transparent, + iconSize: MobileAppBarComposerWidgetStyle.iconSize, + tooltipMessage: AppLocalizations.of(context).browse, + onTapActionCallback: remoteAttachFileAction, + ), + const SizedBox(width: MobileAppBarComposerWidgetStyle.space), + ],🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/features/composer/presentation/widgets/mobile/app_bar_composer_widget.dart` around lines 86 - 94, The cloud button (TMailButtonWidget.fromIcon) is shown even when its callback remoteAttachFileAction is null; change the widget tree to only include/render the TMailButtonWidget.fromIcon (and its adjacent SizedBox) when remoteAttachFileAction != null so the control is hidden if no handler is provided (follow the same gating pattern used by the web bottom bar to avoid rendering a dead control).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@android/app/src/main/AndroidManifest.xml`:
- Around line 39-41: Remove the global android:usesCleartextTraffic="true" from
the main AndroidManifest.xml and instead enable cleartext only for debug builds
by either (A) moving that attribute into the debug manifest under
src/debug/AndroidManifest.xml (so the application tag in that manifest contains
android:usesCleartextTraffic="true"), or (B) implement a scoped
networkSecurityConfig XML that permits cleartext only for your local/test host
and reference it from the main manifest via
android:networkSecurityConfig="@xml/network_security_config"; update/create the
network_security_config XML to whitelist only the debug/local host. Ensure you
modify the Application tag (the one currently containing android:icon and
android:usesCleartextTraffic) or the debug manifest equivalent so release builds
do not allow cleartext.
In `@lib/features/composer/presentation/composer_controller.dart`:
- Around line 946-947: The controller currently hard-codes the picker endpoint
into the url string (see the url variable built with sessionId and clientUrl in
ComposerController), which should be replaced by a configurable value; refactor
to read the base picker host from environment/config (or only use a local debug
constant behind a debug-only flag like kDebugMode), construct the full URL with
Uri.encodeComponent for clientUrl and sessionId, and prefer https:// or respect
a configured scheme to avoid mixed-content issues; apply the same change for the
second occurrence that builds the picker URL.
- Around line 946-947: The iframe URL string for the picker constructs the id
parameter incorrectly (it contains id="+$sessionId+"&...), so update the url
declaration that uses sessionId and clientUrl to use proper Dart string
interpolation and remove the stray concatenation symbols; replace the current
value of the url variable with a properly interpolated string like
'http://.../browse?id=${sessionId}&clientUrl=${Uri.encodeComponent(clientUrl)}&type=payload&multiple=true'
so the id query receives the real sessionId (refer to the url variable and the
sessionId/clientUrl symbols).
- Around line 1017-1024: The JavaScript handler for 'postMessage' currently
calls _handlePostMessage and immediately _cleanupIframeSession for any non-empty
message, which closes the mobile picker prematurely; change the callback in
controller.addJavaScriptHandler (handlerName: 'postMessage') to parse/inspect
args[0] (the message payload) and only call _cleanupIframeSession(sessionId)
when the payload indicates the terminal state (e.g., payload['status'] == 'done'
or equivalent terminal flag), otherwise just call _handlePostMessage(args[0],
sessionId) and leave the session active so subsequent messages (like the actual
file selection) can arrive.
- Around line 1118-1129: Validate that the incoming message's id matches the
current per-dialog sessionId before processing attachments: in the message
handling block where you read messageData (the branch checking
messageData.containsKey('status') / 'id' / 'results'), compare messageData['id']
to the controller's sessionId and return/ignore the message if they differ; only
proceed with the existing status == 'done' && results is List flow when the ids
match to prevent cross-dialog injection via the global window.onMessage
listener.
- Around line 873-885: Remove the raw payload debugPrint calls in openNewModal()
(and the other noted ranges) and stop emitting full client URLs, postMessage
bodies, result maps, or file metadata to logs; instead either delete these
debugPrint lines or replace them with a minimal non-sensitive log (e.g., "iframe
session started" plus the session id) and, if a trace is required for Sentry,
use logTrace with a strictly sanitized payload (no attachments, file names,
URLs, or full message bodies). Update references in the function openNewModal()
and any other spots that set _activeIframeSessionId to only log the generated
session id (or nothing) and ensure any existing debugPrint usages are removed or
converted to logTrace with explicit redaction to prevent sensitive data leakage.
- Around line 1146-1148: The remote-file branch currently calls
uploadAttachmentsAction(pickedFiles: fileInfos) without running the
composer-side attachment-size validation, so oversized remote selections bypass
the guard; before calling uploadAttachmentsAction in composer_controller.dart,
run the same attachment-size validation used by the local picker/drop-zone flows
(the validation that checks fileInfos size limits and rejects/filters oversized
items) and only call uploadAttachmentsAction with the validated/filtered list
(or show the same error/feedback) to ensure consistency with other flows.
- Line 18: The import currently hides UserScript and UserScriptInjectionTime
which prevents their usage; update the import of
package:html_editor_enhanced/html_editor.dart in composer_controller.dart by
removing the "hide UserScript, UserScriptInjectionTime" clause so that the code
that instantiates UserScript and references UserScriptInjectionTime (lines where
UserScript is constructed and UserScriptInjectionTime is used) can compile.
In
`@lib/features/composer/presentation/widgets/web/bottom_bar_composer_widget.dart`:
- Around line 104-117: Replace the hard-coded tooltipMessage 'New Modal' with a
localized string sourced from the app localization (e.g., use AppLocalizations
or your l10n delegate) where BottomBarComposerWidget builds the
TMailButtonWidget.fromIcon; update the tooltipMessage to call the localization
getter (for example AppLocalizations.of(context).newModal or l10n.newModal) so
the label is translated, keeping the rest of the widget
(TMailButtonWidget.fromIcon, onOpenNewModalAction, AbsorbPointer) unchanged and
ensure the relevant localization key is added to the ARB/strings files.
---
Nitpick comments:
In
`@lib/features/composer/presentation/widgets/mobile/app_bar_composer_widget.dart`:
- Around line 86-94: The cloud button (TMailButtonWidget.fromIcon) is shown even
when its callback remoteAttachFileAction is null; change the widget tree to only
include/render the TMailButtonWidget.fromIcon (and its adjacent SizedBox) when
remoteAttachFileAction != null so the control is hidden if no handler is
provided (follow the same gating pattern used by the web bottom bar to avoid
rendering a dead control).
🪄 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: eeb91e7d-bb11-422e-8e90-0db9ff0a7946
📒 Files selected for processing (7)
android/app/src/main/AndroidManifest.xmllib/features/composer/presentation/composer_controller.dartlib/features/composer/presentation/composer_view.dartlib/features/composer/presentation/composer_view_web.dartlib/features/composer/presentation/widgets/mobile/app_bar_composer_widget.dartlib/features/composer/presentation/widgets/web/bottom_bar_composer_widget.dartlib/features/composer/presentation/widgets/web/web_editor_widget.dart
Comment on lines
+39
to
+41
| android:icon="@mipmap/ic_launcher" | ||
| android:usesCleartextTraffic="true"> | ||
| <!-- Necessary to test locally with http --> |
Contributor
There was a problem hiding this comment.
Scope cleartext traffic to debug/local hosts only.
android:usesCleartextTraffic="true" on the main application enables insecure HTTP for every build. For this POC, please move it to src/debug/AndroidManifest.xml or use a scoped networkSecurityConfig that only allows the local test host.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@android/app/src/main/AndroidManifest.xml` around lines 39 - 41, Remove the
global android:usesCleartextTraffic="true" from the main AndroidManifest.xml and
instead enable cleartext only for debug builds by either (A) moving that
attribute into the debug manifest under src/debug/AndroidManifest.xml (so the
application tag in that manifest contains android:usesCleartextTraffic="true"),
or (B) implement a scoped networkSecurityConfig XML that permits cleartext only
for your local/test host and reference it from the main manifest via
android:networkSecurityConfig="@xml/network_security_config"; update/create the
network_security_config XML to whitelist only the debug/local host. Ensure you
modify the Application tag (the one currently containing android:icon and
android:usesCleartextTraffic) or the debug manifest equivalent so release builds
do not allow cleartext.
| import 'package:flutter/services.dart'; | ||
| import 'package:get/get.dart'; | ||
| import 'package:html_editor_enhanced/html_editor.dart'; | ||
| import 'package:html_editor_enhanced/html_editor.dart' hide UserScript, UserScriptInjectionTime; |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -e
echo "== composer_controller imports =="
sed -n '1,40p' lib/features/composer/presentation/composer_controller.dart
echo
echo "== UserScript references in the repository =="
rg -n '\b(UserScript|UserScriptInjectionTime)\b' .
echo
echo "== Alternative imports/exports that could provide these symbols =="
rg -n "flutter_inappwebview|export .*UserScript|class UserScript|enum UserScriptInjectionTime" .Repository: linagora/tmail-flutter
Length of output: 11142
🏁 Script executed:
#!/bin/bash
# Check where UserScript/UserScriptInjectionTime are defined or imported from in the codebase
echo "== Searching for UserScript definition/export =="
rg 'class UserScript|enum UserScriptInjectionTime|export.*UserScript' --type dart
echo
echo "== Checking lines 1010-1015 in composer_controller.dart for UserScript usage =="
sed -n '1010,1015p' lib/features/composer/presentation/composer_controller.dart
echo
echo "== Checking if there's a re-export or alternate import of UserScript =="
rg "from 'package:html_editor_enhanced" lib/features/composer/presentation/composer_controller.dart -A 10Repository: linagora/tmail-flutter
Length of output: 526
🏁 Script executed:
#!/bin/bash
# Check if flutter_inappwebview package might export UserScript
echo "== Checking flutter_inappwebview imports in the codebase =="
rg "from 'package:flutter_inappwebview" lib/features/composer/presentation/composer_controller.dart
echo
echo "== Check all imports in composer_controller.dart to see if flutter_inappwebview is imported anywhere =="
sed -n '1,50p' lib/features/composer/presentation/composer_controller.dart | grep -i inappwebview
echo
echo "== Verify if html_editor_enhanced exports UserScript by checking package info =="
# Check pubspec.yaml to understand dependencies
cat pubspec.yaml | grep -A 2 html_editor_enhanced
echo
echo "== Check if there's an import statement that should bring in UserScript but is missing =="
git log --oneline -20 -- lib/features/composer/presentation/composer_controller.dartRepository: linagora/tmail-flutter
Length of output: 543
🏁 Script executed:
#!/bin/bash
# Check if UserScript might be imported from a different package entirely
echo "== Search entire codebase for any import that brings in UserScript =="
rg "import.*UserScript|from.*UserScript" . --type dart
echo
echo "== Check the actual html_editor_enhanced package to see what it exports =="
# Since it's a git dependency, let's check if there's documentation or if we can find usage patterns
rg "UserScript" . --type dart -B 2 -A 2 | head -30
echo
echo "== Check lines 1000-1020 for more context around UserScript usage =="
sed -n '1000,1020p' lib/features/composer/presentation/composer_controller.dartRepository: linagora/tmail-flutter
Length of output: 2861
🏁 Script executed:
#!/bin/bash
# Verify where InAppWebView, URLRequest, WebUri, etc. come from
echo "== Check if flutter_inappwebview is imported but maybe not shown in initial grep =="
sed -n '1,60p' lib/features/composer/presentation/composer_controller.dart | grep -i "inappwebview\|userscript\|inappwebview"
echo
echo "== Look for InAppWebView definition to confirm where it comes from =="
rg "InAppWebView|URLRequest|WebUri" lib/features/composer/presentation/composer_controller.dart --context 1 | head -20
echo
echo "== Check pubspec.yaml to see flutter_inappwebview dependency =="
grep -A 2 "flutter_inappwebview:" pubspec.yaml
echo
echo "== Verify: does html_editor_enhanced actually export UserScript and UserScriptInjectionTime? =="
# Check if there's any documentation or source that shows html_editor_enhanced exports these
rg "UserScript|UserScriptInjectionTime" . --type yamlRepository: linagora/tmail-flutter
Length of output: 857
🏁 Script executed:
#!/bin/bash
# Check if flutter_inappwebview should be imported
echo "== Search for where InAppWebView actually comes from =="
rg "export.*InAppWebView|class InAppWebView" . --type dart | head -10
echo
echo "== Check if html_editor_enhanced might re-export flutter_inappwebview symbols =="
# Let's look at what the html_editor_enhanced package actually exports
# by checking if it's available in pub.dev or GitHub
echo
echo "== Does the code actually compile? Check if there's an indirect import =="
# Look at all imports more carefully
sed -n '1,40p' lib/features/composer/presentation/composer_controller.dart
echo
echo "== Check rich_text_composer package - might re-export flutter_inappwebview =="
grep -r "flutter_inappwebview" lib/features/composer/ --include="*.dart" | head -20Repository: linagora/tmail-flutter
Length of output: 2782
🏁 Script executed:
#!/bin/bash
# Check the entire import section (might extend beyond line 40)
echo "== Get ALL import statements =="
sed -n '1,100p' lib/features/composer/presentation/composer_controller.dart | grep "^import\|^export"
echo
echo "== Check if flutter_inappwebview is imported anywhere in the file =="
grep -n "flutter_inappwebview" lib/features/composer/presentation/composer_controller.dart
echo
echo "== Check what package:core exports - might include flutter_inappwebview =="
find . -path "*/core/lib/*.dart" -name "*.dart" -exec grep -l "flutter_inappwebview\|UserScript" {} \;Repository: linagora/tmail-flutter
Length of output: 9065
🏁 Script executed:
#!/bin/bash
# Check if core/core.dart exports flutter_inappwebview symbols
echo "== Check core/lib/core.dart for re-exports =="
find core/lib -name "*.dart" | xargs grep -l "export.*flutter_inappwebview\|export.*UserScript"
echo
echo "== Check rich_text_composer exports =="
find . -path "*rich_text_composer*" -name "*.dart" | xargs grep -l "UserScript\|InAppWebView" 2>/dev/null | head -10
echo
echo "== Check pubspec.yaml dependencies - does core or rich_text_composer depend on flutter_inappwebview =="
grep -A 5 "flutter_inappwebview\|rich_text_composer" pubspec.yaml | head -20
echo
echo "== Look at the actual error - try to understand what's happening by checking if this is a known pattern =="
# Check if there's a pubspec.yaml in a subdirectory that might affect this
find . -maxdepth 3 -name "pubspec.yaml" | xargs grep -l "flutter_inappwebview" | head -5Repository: linagora/tmail-flutter
Length of output: 517
🏁 Script executed:
#!/bin/bash
# Check if flutter_inappwebview is re-exported from core
echo "== Check core/lib/core.dart main export file =="
cat core/lib/core.dart | head -50
echo
echo "== Check if flutter_inappwebview is in core dependencies =="
grep "flutter_inappwebview" core/pubspec.yaml
echo
echo "== The key question: where do InAppWebView, UserScript come from? =="
# InAppWebView is clearly being used but not imported. Check if it's from html_editor_enhanced
echo "Does html_editor_enhanced re-export flutter_inappwebview classes?"
rg "InAppWebView|URLRequest|WebUri" lib/features/composer/presentation/composer_controller.dart | head -3
echo
echo "== Final check: is this code even supposed to compile? Check for any compile_errors or TODOs =="
grep -n "TODO\|FIXME\|compile" lib/features/composer/presentation/composer_controller.dart | head -10Repository: linagora/tmail-flutter
Length of output: 3061
🏁 Script executed:
#!/bin/bash
# Final verification: check if this is a real compile error or if there's a working pattern somewhere
echo "== Search the entire codebase for other files using InAppWebView =="
rg "InAppWebView" lib/ --type dart -l
echo
echo "== Check if any of those files import flutter_inappwebview directly =="
rg "import.*flutter_inappwebview" lib/ --type dart
echo
echo "== Check the git history - was this code working before or recently added? =="
git log --oneline -30 lib/features/composer/presentation/composer_controller.dart
echo
echo "== Check the specific lines 1010-1020 to confirm UserScript/UserScriptInjectionTime are definitely being used =="
sed -n '1010,1020p' lib/features/composer/presentation/composer_controller.dart | cat -nRepository: linagora/tmail-flutter
Length of output: 1193
🏁 Script executed:
#!/bin/bash
# Check how mobile_editor_widget.dart handles the imports
echo "== Check mobile_editor_widget.dart imports =="
sed -n '1,50p' lib/features/composer/presentation/widgets/mobile/mobile_editor_widget.dart | grep -E "^import|^export"
echo
echo "== Check if mobile_editor_widget uses InAppWebView =="
grep -n "InAppWebView\|UserScript" lib/features/composer/presentation/widgets/mobile/mobile_editor_widget.dart
echo
echo "== Final confirmation: does html_editor_enhanced actually export these symbols? =="
# Since we can't access the package source directly, let's check documentation or comments
rg "html_editor_enhanced" . --type dart -A 5 -B 5 | grep -i "userscript\|inappwebview" | head -10
echo
echo "== Check if there's a note or comment explaining the hide clause =="
sed -n '10,30p' lib/features/composer/presentation/composer_controller.dartRepository: linagora/tmail-flutter
Length of output: 2179
Remove the hide clause to use UserScript and UserScriptInjectionTime.
Lines 1011-1014 instantiate UserScript and reference UserScriptInjectionTime, but the import on line 18 hides both symbols. This will cause a compile error.
Suggested fix
-import 'package:html_editor_enhanced/html_editor.dart' hide UserScript, UserScriptInjectionTime;
+import 'package:html_editor_enhanced/html_editor.dart';📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| import 'package:html_editor_enhanced/html_editor.dart' hide UserScript, UserScriptInjectionTime; | |
| import 'package:html_editor_enhanced/html_editor.dart'; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/features/composer/presentation/composer_controller.dart` at line 18, The
import currently hides UserScript and UserScriptInjectionTime which prevents
their usage; update the import of package:html_editor_enhanced/html_editor.dart
in composer_controller.dart by removing the "hide UserScript,
UserScriptInjectionTime" clause so that the code that instantiates UserScript
and references UserScriptInjectionTime (lines where UserScript is constructed
and UserScriptInjectionTime is used) can compile.
Comment on lines
+873
to
+885
| debugPrint('🚀 openNewModal() called - Starting iframe modal setup'); | ||
|
|
||
| if (PlatformInfo.isWeb) { | ||
| debugPrint('📱 Platform confirmed: Web environment detected'); | ||
|
|
||
| // Generate a unique ID for this iframe session | ||
| final String sessionId = | ||
| 'session-${DateTime.now().millisecondsSinceEpoch}'; | ||
| debugPrint('🔑 Generated session ID: $sessionId'); | ||
|
|
||
| // Store the ID so we can reference it later | ||
| _activeIframeSessionId = sessionId; | ||
| debugPrint('📝 Session ID stored in _activeIframeSessionId'); |
Contributor
There was a problem hiding this comment.
Remove the raw payload debug logs before merge.
This flow logs the client URL, full postMessage bodies, result maps, and file metadata. In practice that can expose attachment contents or other sensitive context in browser/device logs, and it is far noisier than the rest of the composer path.
Based on learnings: In the tmail-flutter repository, ensure that logTrace calls are used selectively for diagnostic purposes related to Sentry submissions and not for regular production logging. During reviews, verify that trace-level logs are placed only where they meaningfully aid diagnosis, avoid noisy production reports, and ensure no sensitive data is logged.
Also applies to: 891-895, 938-948, 954-963, 1060-1069, 1073-1111, 1115-1199
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/features/composer/presentation/composer_controller.dart` around lines 873
- 885, Remove the raw payload debugPrint calls in openNewModal() (and the other
noted ranges) and stop emitting full client URLs, postMessage bodies, result
maps, or file metadata to logs; instead either delete these debugPrint lines or
replace them with a minimal non-sensitive log (e.g., "iframe session started"
plus the session id) and, if a trace is required for Sentry, use logTrace with a
strictly sanitized payload (no attachments, file names, URLs, or full message
bodies). Update references in the function openNewModal() and any other spots
that set _activeIframeSessionId to only log the generated session id (or
nothing) and ensure any existing debugPrint usages are removed or converted to
logTrace with explicit redaction to prevent sensitive data leakage.
Comment on lines
+946
to
+947
| final String url = | ||
| 'http://10.4.0.32:5000/browse?id="+$sessionId+"&clientUrl=${Uri.encodeComponent(clientUrl)}&type=payload&multiple=true'; |
Contributor
There was a problem hiding this comment.
Don't hard-code a local HTTP picker endpoint in the controller.
Both paths are pinned to http://10.4.0.32:5000. That only works on one network, forces insecure transport, and will be blocked as mixed content when the web app is served over HTTPS. Please move this behind environment config or a debug-only flag.
Also applies to: 971-971
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/features/composer/presentation/composer_controller.dart` around lines 946
- 947, The controller currently hard-codes the picker endpoint into the url
string (see the url variable built with sessionId and clientUrl in
ComposerController), which should be replaced by a configurable value; refactor
to read the base picker host from environment/config (or only use a local debug
constant behind a debug-only flag like kDebugMode), construct the full URL with
Uri.encodeComponent for clientUrl and sessionId, and prefer https:// or respect
a configured scheme to avoid mixed-content issues; apply the same change for the
second occurrence that builds the picker URL.
The iframe URL builds the id query parameter incorrectly.
This string currently emits id="+$sessionId+"&..., so the picker never receives the real session id. That breaks the session handshake this flow relies on.
🐛 Suggested fix
- final String url =
- 'http://10.4.0.32:5000/browse?id="+$sessionId+"&clientUrl=${Uri.encodeComponent(clientUrl)}&type=payload&multiple=true';
+ final String url =
+ 'http://10.4.0.32:5000/browse?id=$sessionId&clientUrl=${Uri.encodeComponent(clientUrl)}&type=payload&multiple=true';📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| final String url = | |
| 'http://10.4.0.32:5000/browse?id="+$sessionId+"&clientUrl=${Uri.encodeComponent(clientUrl)}&type=payload&multiple=true'; | |
| final String url = | |
| 'http://10.4.0.32:5000/browse?id=$sessionId&clientUrl=${Uri.encodeComponent(clientUrl)}&type=payload&multiple=true'; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/features/composer/presentation/composer_controller.dart` around lines 946
- 947, The iframe URL string for the picker constructs the id parameter
incorrectly (it contains id="+$sessionId+"&...), so update the url declaration
that uses sessionId and clientUrl to use proper Dart string interpolation and
remove the stray concatenation symbols; replace the current value of the url
variable with a properly interpolated string like
'http://.../browse?id=${sessionId}&clientUrl=${Uri.encodeComponent(clientUrl)}&type=payload&multiple=true'
so the id query receives the real sessionId (refer to the url variable and the
sessionId/clientUrl symbols).
Comment on lines
+1017
to
+1024
| controller.addJavaScriptHandler( | ||
| handlerName: 'postMessage', | ||
| callback: (args) { | ||
| if (_activeIframeSessionId == sessionId && args.isNotEmpty) { | ||
| _handlePostMessage(args[0], sessionId); | ||
| _cleanupIframeSession(sessionId); | ||
| } | ||
| }, |
Contributor
There was a problem hiding this comment.
Don't tear down the mobile picker on the first message.
This closes the dialog after any non-empty postMessage, before verifying that the payload is the terminal status == done result for this session. If the embedded page sends an init/ready event first, the actual file selection never gets a chance to arrive.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/features/composer/presentation/composer_controller.dart` around lines
1017 - 1024, The JavaScript handler for 'postMessage' currently calls
_handlePostMessage and immediately _cleanupIframeSession for any non-empty
message, which closes the mobile picker prematurely; change the callback in
controller.addJavaScriptHandler (handlerName: 'postMessage') to parse/inspect
args[0] (the message payload) and only call _cleanupIframeSession(sessionId)
when the payload indicates the terminal state (e.g., payload['status'] == 'done'
or equivalent terminal flag), otherwise just call _handlePostMessage(args[0],
sessionId) and leave the session active so subsequent messages (like the actual
file selection) can arrive.
Comment on lines
+1118
to
+1129
| if (messageData.containsKey('status') && | ||
| messageData.containsKey('id') && | ||
| messageData.containsKey('results')) { | ||
|
|
||
| final status = messageData['status']; | ||
| final id = messageData['id']; | ||
| final results = messageData['results']; | ||
|
|
||
| debugPrint('📁 File selection message detected - status: $status, id: $id'); | ||
|
|
||
| if (status == 'done' && results is List) { | ||
| debugPrint('🎉 Processing ${results.length} file(s) from iframe'); |
Contributor
There was a problem hiding this comment.
Validate the incoming payload id against sessionId.
You generate a per-dialog session id and pass it to the picker, but the receive path never checks messageData['id'] == sessionId. With a global window.onMessage listener, any same-shaped payload can currently inject attachments into the composer while the modal is open.
🔒 Suggested fix
final status = messageData['status'];
final id = messageData['id'];
final results = messageData['results'];
debugPrint('📁 File selection message detected - status: $status, id: $id');
+
+ if (id != sessionId) {
+ return;
+ }
if (status == 'done' && results is List) {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if (messageData.containsKey('status') && | |
| messageData.containsKey('id') && | |
| messageData.containsKey('results')) { | |
| final status = messageData['status']; | |
| final id = messageData['id']; | |
| final results = messageData['results']; | |
| debugPrint('📁 File selection message detected - status: $status, id: $id'); | |
| if (status == 'done' && results is List) { | |
| debugPrint('🎉 Processing ${results.length} file(s) from iframe'); | |
| if (messageData.containsKey('status') && | |
| messageData.containsKey('id') && | |
| messageData.containsKey('results')) { | |
| final status = messageData['status']; | |
| final id = messageData['id']; | |
| final results = messageData['results']; | |
| debugPrint('📁 File selection message detected - status: $status, id: $id'); | |
| if (id != sessionId) { | |
| return; | |
| } | |
| if (status == 'done' && results is List) { | |
| debugPrint('🎉 Processing ${results.length} file(s) from iframe'); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/features/composer/presentation/composer_controller.dart` around lines
1118 - 1129, Validate that the incoming message's id matches the current
per-dialog sessionId before processing attachments: in the message handling
block where you read messageData (the branch checking
messageData.containsKey('status') / 'id' / 'results'), compare messageData['id']
to the controller's sessionId and return/ignore the message if they differ; only
proceed with the existing status == 'done' && results is List flow when the ids
match to prevent cross-dialog injection via the global window.onMessage
listener.
Comment on lines
+1146
to
+1148
| if (fileInfos.isNotEmpty) { | ||
| debugPrint('📤 Uploading ${fileInfos.length} file(s) to composer'); | ||
| uploadAttachmentsAction(pickedFiles: fileInfos); |
Contributor
There was a problem hiding this comment.
Run the normal attachment-size validation before uploading remote files.
This path jumps straight to uploadAttachmentsAction, unlike the local picker and drop-zone flows. Oversized remote selections will skip the composer-side guard and only fail later.
🧮 Suggested fix
- uploadAttachmentsAction(pickedFiles: fileInfos);
+ uploadController.validateTotalSizeAttachmentsBeforeUpload(
+ totalSizePreparedFiles: fileInfos.totalSize,
+ onValidationSuccess: () => uploadAttachmentsAction(pickedFiles: fileInfos),
+ );📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if (fileInfos.isNotEmpty) { | |
| debugPrint('📤 Uploading ${fileInfos.length} file(s) to composer'); | |
| uploadAttachmentsAction(pickedFiles: fileInfos); | |
| if (fileInfos.isNotEmpty) { | |
| debugPrint('📤 Uploading ${fileInfos.length} file(s) to composer'); | |
| uploadController.validateTotalSizeAttachmentsBeforeUpload( | |
| totalSizePreparedFiles: fileInfos.totalSize, | |
| onValidationSuccess: () => uploadAttachmentsAction(pickedFiles: fileInfos), | |
| ); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/features/composer/presentation/composer_controller.dart` around lines
1146 - 1148, The remote-file branch currently calls
uploadAttachmentsAction(pickedFiles: fileInfos) without running the
composer-side attachment-size validation, so oversized remote selections bypass
the guard; before calling uploadAttachmentsAction in composer_controller.dart,
run the same attachment-size validation used by the local picker/drop-zone flows
(the validation that checks fileInfos size limits and rejects/filters oversized
items) and only call uploadAttachmentsAction with the validated/filtered list
(or show the same error/feedback) to ensure consistency with other flows.
Comment on lines
+104
to
+117
| if (onOpenNewModalAction != null) | ||
| AbsorbPointer( | ||
| absorbing: isCodeViewEnabled, | ||
| child: TMailButtonWidget.fromIcon( | ||
| icon: imagePaths.icAttachFile, // Using same icon for now, can be changed later | ||
| iconColor: isCodeViewEnabled | ||
| ? BottomBarComposerWidgetStyle.disabledIconColor | ||
| : BottomBarComposerWidgetStyle.iconColor, | ||
| borderRadius: BottomBarComposerWidgetStyle.iconRadius, | ||
| backgroundColor: Colors.transparent, | ||
| padding: BottomBarComposerWidgetStyle.iconPadding, | ||
| iconSize: BottomBarComposerWidgetStyle.iconSize, | ||
| tooltipMessage: 'New Modal', // TODO: Add proper localization | ||
| onTapActionCallback: onOpenNewModalAction, |
Contributor
There was a problem hiding this comment.
Localize the new button label before merge.
'New Modal' is hard-coded user-facing text, so this control will bypass localization on every supported language. Please replace the TODO with an AppLocalizations entry before this ships.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@lib/features/composer/presentation/widgets/web/bottom_bar_composer_widget.dart`
around lines 104 - 117, Replace the hard-coded tooltipMessage 'New Modal' with a
localized string sourced from the app localization (e.g., use AppLocalizations
or your l10n delegate) where BottomBarComposerWidget builds the
TMailButtonWidget.fromIcon; update the tooltipMessage to call the localization
getter (for example AppLocalizations.of(context).newModal or l10n.newModal) so
the label is translated, keeping the rest of the widget
(TMailButtonWidget.fromIcon, onOpenNewModalAction, AbsorbPointer) unchanged and
ensure the relevant localization key is added to the ARB/strings files.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Do not merge. openburo hackathon work.
POC of iframe/webview of File Picker that returns a binary that is added as attachment.
Web
Screen.Recording.2026-04-10.at.13.13.13.mov
Mobile
mobile.mp4
Summary by CodeRabbit
New Features
Bug Fixes