[Scribe mobile] Hide keyboard and selection icons when Scribe mobile modal opened#4247
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThis pull request adds selection persistence functionality to the composer when using AI Scribe features. It introduces three new utility methods to HtmlUtils for saving, restoring, and clearing window selections on the web. The composer extension is enhanced with cross-platform APIs that abstract these utilities and add platform-specific behavior for mobile, including focus management before opening modals and cleanup after handling suggestions. The AI Scribe selection overlay is updated to make focus clearing asynchronous and invoke the new save-and-unfocus operation on mobile platforms. Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
|
This PR has been deployed to https://linagora.github.io/tmail-flutter/4247. |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![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
🤖 Fix all issues with AI agents
In
@lib/features/composer/presentation/extensions/ai_scribe/handle_ai_scribe_in_composer_extension.dart:
- Around line 103-109: The mobile branch may return null from
evaluateJavascript; update the return to coalesce null to false by converting
the result of
richTextMobileTabletController?.htmlEditorApi?.webViewController.evaluateJavascript(...)
(using HtmlUtils.restoreSelection.script) into a boolean with a null-coalescing
fallback of false so the method returns false instead of null (same fix pattern
used in saveSelection).
- Around line 83-88: The mobile branch returns the raw evaluateJavascript result
which can be null; update the call that uses
richTextMobileTabletController?.htmlEditorApi?.webViewController.evaluateJavascript(source:
HtmlUtils.saveSelection.script) to coalesce null to false (add ?? false) so the
method mirrors the web branch and always returns a non-null boolean.
In
@lib/features/composer/presentation/widgets/ai_scribe/composer_ai_scribe_selection_overlay.dart:
- Around line 33-40: The onTapFallback currently typed as VoidCallback doesn't
match the async _clearComposerInputFocus; change the callback type to
Future<void> Function()? in AiSelectionOverlay (ai_selection_overlay.dart) and
update all callers (notably where inline_ai_assist_button.dart invokes
onTapFallback?.call()) to await the call (e.g., await onTapFallback?.call();).
Keep _clearComposerInputFocus as Future<void> so saveAndUnfocusForModal() is
properly awaited and ensure any upstream callers handling the tap become async
to await the callback.
🧹 Nitpick comments (3)
core/lib/utils/html/html_utils.dart (2)
216-227: Remove debugconsole.logstatement.Line 222 contains a debug logging statement that should be removed before merging.
🧹 Proposed fix
static const saveSelection = ( script: ''' (() => { const selection = window.getSelection(); if (selection && selection.rangeCount > 0) { window._savedRange = selection.getRangeAt(0).cloneRange(); - console.log("savedRange", window._savedRange); return true; } return false; })();''', name: 'saveSelection');
229-249: Consider adding resilience for DOM changes.If the DOM structure changes between
saveSelectionandrestoreSelection(e.g., content edited in another way),addRangewith a stale range could throw or behave unexpectedly. Consider wrapping in try-catch for robustness.💡 Optional: Add error handling
static const restoreSelection = ( script: ''' (() => { if (window._savedRange) { const selection = window.getSelection(); if (selection) { - selection.removeAllRanges(); - selection.addRange(window._savedRange); - return true; + try { + selection.removeAllRanges(); + selection.addRange(window._savedRange); + return true; + } catch (e) { + console.warn('Failed to restore selection:', e); + return false; + } } } return false; })();''', name: 'restoreSelection');lib/features/composer/presentation/extensions/ai_scribe/handle_ai_scribe_in_composer_extension.dart (1)
203-225: Consider consolidatingclearSavedSelectioncalls.When using the modal flow,
clearSavedSelectionis called twice on mobile: once after the modal closes (line 205) and again after handling the suggestion (line 223). While harmless, this is redundant. Consider removing one of these calls or documenting why both are needed.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
core/lib/utils/html/html_utils.dartlib/features/composer/presentation/extensions/ai_scribe/handle_ai_scribe_in_composer_extension.dartlib/features/composer/presentation/widgets/ai_scribe/composer_ai_scribe_selection_overlay.dart
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: dab246
Repo: linagora/tmail-flutter PR: 4191
File: lib/features/email/presentation/extensions/handle_email_action_extension.dart:37-80
Timestamp: 2025-12-12T04:54:11.121Z
Learning: In lib/features/email/presentation/extensions/handle_email_action_extension.dart, the mailboxDashBoardController.selectedEmail should only be synchronized when isMobileThreadDisabled is true. This is intentional behavior and should not be changed to update selectedEmail in non-mobile or thread-enabled contexts.
📚 Learning: 2025-12-09T09:36:45.349Z
Learnt from: dab246
Repo: linagora/tmail-flutter PR: 4194
File: lib/features/manage_account/presentation/manage_account_dashboard_controller.dart:174-176
Timestamp: 2025-12-09T09:36:45.349Z
Learning: In Dart/Flutter projects using GetX, do not wrap getBinding<T>() calls in try/catch since they return null when not found. Only wrap Get.find<T>() calls in try/catch because they throw if a dependency is unavailable. When a binding is optional, consider checking Get.isRegistered<T>() or handling the null/exception path gracefully instead of blindly catching, and document the expectation for failure modes where a dependency may not be registered.
Applied to files:
core/lib/utils/html/html_utils.dartlib/features/composer/presentation/extensions/ai_scribe/handle_ai_scribe_in_composer_extension.dartlib/features/composer/presentation/widgets/ai_scribe/composer_ai_scribe_selection_overlay.dart
📚 Learning: 2025-12-12T04:54:11.121Z
Learnt from: dab246
Repo: linagora/tmail-flutter PR: 4191
File: lib/features/email/presentation/extensions/handle_email_action_extension.dart:37-80
Timestamp: 2025-12-12T04:54:11.121Z
Learning: In lib/features/email/presentation/extensions/handle_email_action_extension.dart, the mailboxDashBoardController.selectedEmail should only be synchronized when isMobileThreadDisabled is true. This is intentional behavior and should not be changed to update selectedEmail in non-mobile or thread-enabled contexts.
Applied to files:
lib/features/composer/presentation/extensions/ai_scribe/handle_ai_scribe_in_composer_extension.dartlib/features/composer/presentation/widgets/ai_scribe/composer_ai_scribe_selection_overlay.dart
🔇 Additional comments (2)
lib/features/composer/presentation/extensions/ai_scribe/handle_ai_scribe_in_composer_extension.dart (2)
134-148: LGTM!Good defensive coding - only unfocusing the editor when the selection was successfully saved ensures proper state management. The platform-specific handling for iOS vs Android keyboard behavior is appropriate.
169-181: LGTM!Restoring the selection on mobile before the replace operation ensures the text replacement targets the correct range that was saved when the modal was opened.
...tures/composer/presentation/extensions/ai_scribe/handle_ai_scribe_in_composer_extension.dart
Show resolved
Hide resolved
...tures/composer/presentation/extensions/ai_scribe/handle_ai_scribe_in_composer_extension.dart
Show resolved
Hide resolved
lib/features/composer/presentation/widgets/ai_scribe/composer_ai_scribe_selection_overlay.dart
Show resolved
Hide resolved
zatteo
force-pushed
the
feat/scribe-mobile-keyboard-management
branch
from
5c9e362 to
c78ee07
Compare
| Future<bool> saveSelection() async { | ||
| try { | ||
| if (PlatformInfo.isWeb) { | ||
| final result = await richTextWebController?.editorController.evaluateJavascriptWeb( | ||
| HtmlUtils.saveSelection.name, | ||
| hasReturnValue: true, | ||
| ) ?? false; | ||
| return result; | ||
| } else { | ||
| final result = await richTextMobileTabletController?.htmlEditorApi?.webViewController | ||
| .evaluateJavascript( | ||
| source: HtmlUtils.saveSelection.script, | ||
| ) ?? false; | ||
| return result; | ||
| } | ||
| } catch (e) { | ||
| logError('$runtimeType::saveSelection:Exception = $e'); | ||
| return false; | ||
| } | ||
| } |
There was a problem hiding this comment.
Is there a way to write unit tests for it?
There was a problem hiding this comment.
What do you imagine testing ? Checking that we call appropriate method from appropriate controller depending on platform ?
| Future<bool> restoreSelection() async { | ||
| try { | ||
| if (PlatformInfo.isWeb) { | ||
| final result = await richTextWebController?.editorController.evaluateJavascriptWeb( | ||
| HtmlUtils.restoreSelection.name, | ||
| hasReturnValue: true, | ||
| ) ?? false; | ||
| return result; | ||
| } else { | ||
| final result = await richTextMobileTabletController?.htmlEditorApi?.webViewController | ||
| .evaluateJavascript( | ||
| source: HtmlUtils.restoreSelection.script, | ||
| ) ?? false; | ||
| return result; | ||
| } |
| if (PlatformInfo.isIOS) { | ||
| await richTextMobileTabletController?.htmlEditorApi?.unfocus(); | ||
| } else if (PlatformInfo.isAndroid) { | ||
| await richTextMobileTabletController?.htmlEditorApi?.hideKeyboard(); | ||
| await richTextMobileTabletController?.htmlEditorApi?.unfocus(); | ||
| } |
There was a problem hiding this comment.
| if (PlatformInfo.isIOS) { | |
| await richTextMobileTabletController?.htmlEditorApi?.unfocus(); | |
| } else if (PlatformInfo.isAndroid) { | |
| await richTextMobileTabletController?.htmlEditorApi?.hideKeyboard(); | |
| await richTextMobileTabletController?.htmlEditorApi?.unfocus(); | |
| } | |
| final editorApi = richTextMobileTabletController?.htmlEditorApi; | |
| if (PlatformInfo.isIOS) { | |
| await editorApi?.unfocus(); | |
| } else if (PlatformInfo.isAndroid) { | |
| await editorApi?.hideKeyboard(); | |
| await editorApi?.unfocus(); | |
| } |
| } | ||
|
|
||
| onTapFallback?.call(); | ||
| await onTapFallback?.call(); |
zatteo
force-pushed
the
feat/scribe-mobile
branch
2 times, most recently
from
ce3632f to
7ed5cd8
Compare
zatteo
force-pushed
the
feat/scribe-mobile-keyboard-management
branch
from
c78ee07 to
ea78745
Compare
zatteo
force-pushed
the
feat/scribe-mobile
branch
from
7ed5cd8 to
c27fe98
Compare
When opening Scribe mobile modal, we now unfocus the composer to hide the keyboard and the selection start and end icons from the OS. Add new methods to save, restore, and clear text selection in HTML editor. These methods allow preserving and restoring user selection state when opening the Scribe mobile modal so that the "replace" action still works.
zatteo
force-pushed
the
feat/scribe-mobile-keyboard-management
branch
from
ea78745 to
21e7d8d
Compare
2 participants
When opening Scribe mobile modal, we now unfocus the composer to hide the keyboard and the selection start and end icons from the OS.
Add new methods to save, restore, and clear text selection in HTML editor. These methods allow preserving and restoring user selection state when opening the Scribe mobile modal so that the "replace" action still works.
With
Before
After
Summary by CodeRabbit
Release Notes
New Features
Improvements
✏️ Tip: You can customize this high-level summary in your review settings.