TF-4099 Mobile adding an attachment causes composer UI issues#4100
TF-4099 Mobile adding an attachment causes composer UI issues#4100
Conversation
|
This PR has been deployed to https://linagora.github.io/tmail-flutter/4100. |
|
what is the progress for this ticket? |
In the process of being completely resolved, the error is related to the device's RAM usage rights. Only occurs on low-configuration devices, the error does not appear frequently. |
|
This fix do not fully solve the issue However it delivers an improvments. I propose to rebase it, merge it, but keep the issue open. |
…w process to keep more RAM
hoangdat
force-pushed
the
bugfix/tf-4099-mobile-adding-an-attachment-causes-composer-ui-issues
branch
from
d345f0a to
ee75e14
Compare
WalkthroughThis pull request implements WebView lifecycle management for the composer feature on mobile platforms. The Android manifest now includes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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.
![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 the current code and only fix it if needed.
Inline comments:
In `@android/app/src/main/AndroidManifest.xml`:
- Around line 45-49: The android:largeHeap="true" attribute is currently on the
<activity> element (MainActivity) where it's ignored; remove android:largeHeap
from the <activity> element and add android:largeHeap="true" to the
<application> element instead (ensuring you don't duplicate the attribute if
already present) so the large heap request is applied correctly at the
application level.
In `@lib/features/composer/presentation/composer_controller.dart`:
- Around line 1162-1167: The calls to webViewLifecycleManager.pause() are not
awaited, causing a race when launching pickers; update openFilePickerByType and
insertImage to be async, await
richTextMobileTabletController?.webViewLifecycleManager?.pause() before calling
popBack() (if applicable) and before invoking
_localFilePickerInteractor.execute(...) or any picker logic so the WebView is
fully paused; ensure consumeState(...) is still used but only after awaiting
pause().
In `@lib/features/upload/domain/usecases/local_file_picker_interactor.dart`:
- Line 2: The unconditional import of dart:io and use of ProcessInfo in
LocalFilePickerInteractor causes web build failures; replace the direct import
by moving the ProcessInfo-dependent diagnostic behind a conditional import or
platform stub: create an IO implementation (e.g.,
local_file_picker_interactor_io.dart) that imports dart:io and exposes a
function or getter used by LocalFilePickerInteractor to fetch process info, and
a web stub (e.g., local_file_picker_interactor_web.dart) that returns a safe
no-op or null; then use Dart conditional imports in
local_file_picker_interactor.dart (import ... if (dart.library.io) ...) or
remove the diagnostic call entirely. Update ComposerBindings to continue binding
LocalFilePickerInteractor as before. Ensure references to ProcessInfo are only
in the IO-specific file.
🪄 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: a468b1d9-441d-4210-bd1c-467037b33a8c
📒 Files selected for processing (5)
android/app/src/main/AndroidManifest.xmllib/features/composer/presentation/composer_controller.dartlib/features/composer/presentation/controller/rich_text_mobile_tablet_controller.dartlib/features/composer/presentation/controller/web_view_lifecycle_manager.dartlib/features/upload/domain/usecases/local_file_picker_interactor.dart
lib/features/upload/domain/usecases/local_file_picker_interactor.dart
Outdated
Show resolved
Hide resolved
lib/features/upload/domain/usecases/local_file_picker_interactor.dart
Outdated
Show resolved
Hide resolved
…or allow process to keep more RAM Signed-off-by: dab246 <tdvu@linagora.com>
There was a problem hiding this comment.
Gates Passed
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.
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)
lib/features/composer/presentation/composer_controller.dart (1)
1191-1235:⚠️ Potential issue | 🟠 MajorAsync handlers called from void context run unawaited.
These handlers (
_handlePickFileSuccess,_handlePickFileFailure,_handlePickImageSuccess,_handlePickImageFailure) are nowFuture<void>but are invoked fromhandleSuccessViewState/handleFailureViewState, which arevoidmethods called viaconsumeState'sStream.listen(onData). SinceonDatais typed asvoid Function(...), the returned Futures are discarded, meaning:
await resume()andawait _restoreEditorContentIfBlank()execute fire-and-forget- Any exceptions not caught within the handlers won't propagate
The existing try-catch in
_restoreEditorContentIfBlankprovides some protection, but errors inresume()could go unobserved.Consider wrapping the async block in the handlers with an additional safety layer:
🛡️ Optional defensive wrapper
Future<void> _handlePickFileSuccess(LocalFilePickerSuccess success) async { if (PlatformInfo.isMobile) { - await richTextMobileTabletController?.webViewLifecycleManager?.resume(); - await _restoreEditorContentIfBlank(); + try { + await richTextMobileTabletController?.webViewLifecycleManager?.resume(); + await _restoreEditorContentIfBlank(); + } catch (e) { + logWarning('ComposerController::_handlePickFileSuccess:WebView recovery failed: $e'); + } } // ... rest of handler }Alternatively, a more robust long-term fix would be to refactor
consumeStateto support async handlers, but that's a larger architectural change.🤖 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 1191 - 1235, The handlers (_handlePickFileSuccess, _handlePickFileFailure, _handlePickImageSuccess, _handlePickImageFailure) perform awaited async work but are invoked from void callbacks so their Futures are dropped; wrap the internal async work in a detached Future and ensure errors are caught and logged so exceptions don't go unobserved. Concretely, change each handler to start the work with unawaited(Future(() async { /* existing mobile resume(), _restoreEditorContentIfBlank(), and uploadController.validate... calls */ })). Inside that detached Future keep the awaits and add try/catch around resume(), _restoreEditorContentIfBlank(), and the uploadController calls to report errors (e.g., via appToast or logging) so failures are handled instead of being dropped.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@lib/features/composer/presentation/composer_controller.dart`:
- Around line 1191-1235: The handlers (_handlePickFileSuccess,
_handlePickFileFailure, _handlePickImageSuccess, _handlePickImageFailure)
perform awaited async work but are invoked from void callbacks so their Futures
are dropped; wrap the internal async work in a detached Future and ensure errors
are caught and logged so exceptions don't go unobserved. Concretely, change each
handler to start the work with unawaited(Future(() async { /* existing mobile
resume(), _restoreEditorContentIfBlank(), and uploadController.validate... calls
*/ })). Inside that detached Future keep the awaits and add try/catch around
resume(), _restoreEditorContentIfBlank(), and the uploadController calls to
report errors (e.g., via appToast or logging) so failures are handled instead of
being dropped.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e96a8f99-6d90-4c01-a873-f2aa3e6177b4
📒 Files selected for processing (2)
android/app/src/main/AndroidManifest.xmllib/features/composer/presentation/composer_controller.dart
✅ Files skipped from review due to trivial changes (1)
- android/app/src/main/AndroidManifest.xml
It's not necessary, as both functions already have try/catch handling built in. |
|
|
||
| List<Attachment> initialAttachments = <Attachment>[]; | ||
| String? _textEditorWeb; | ||
| String? _savedEditorContentBeforePicker; |
There was a problem hiding this comment.
IMO, please remove this mechanism, because the problem is system acquire memory from app, so it restart the app. we should find the better way to cache the content.
|
it still not fix the problem, will verify it along side with #3598 |
4 participants
Issue
#4099
Root cause
This is a typical crash of Android WebView renderer being
OOM (Out Of Memory)or being force-killed due tolow-memorypolicy.Special:
Confirmed MIUI (system_server) killed the process to reclaim memory, as the device recognized it as a “lowmemory device”.
flutter_inappwebviewcreates its own sandbox renderer (Chromium engine) -> consumes a lot of RAM.When file_picker turns on
Intent.ACTION_OPEN_DOCUMENT, Android temporarily switches foreground to another app → causing MIUI or Android low-memory killer to think your app is in the background → kill WebView renderer.Then WebView cannot recover because Chromium sandbox is dead → Flutter is terminated.
Workaround
android:largeHeap="true"in AndroidManifest => Allow process to keep more RAM, limit OOM kill when DocumentsUI is open.Summary by CodeRabbit
New Features
Bug Fixes
Chores