Open
Conversation
WalkthroughThis pull request implements recovery key functionality across the application. It adds localization entries for recovery key-related messages, introduces a new integration test robot and test for recovery key copy workflows, updates the login robot to use localized strings, refactors settings UI components for caching and style consistency, implements recovery key fetching and copying in the security settings page, and adds supporting configuration changes. The modifications span analyzer configuration, localization assets, integration tests, and UI/settings code. 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. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
📝 Coding Plan
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 was referenced Mar 19, 2026
Contributor
|
This PR has been deployed to https://linagora.github.io/twake-on-matrix/2944 |
9clg6
requested review from
dab246,
hoangdat,
nqhhdev and
tddang-linagora
as code owners
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
lib/pages/chat/events/audio_message/audio_play_extension.dart (1)
6-6: Improve TODO comment clarity.The TODO comment could be more specific. Consider clarifying "ignore linter" to "ignore directive" or "linter suppression" for better precision.
📝 Suggested rewording
-///TODO(clement): Remove the ignore linter when the experimental member use is no longer needed. +///TODO(clement): Remove the ignore directive when the experimental member is no longer used or becomes stable.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/pages/chat/events/audio_message/audio_play_extension.dart` at line 6, Update the TODO in audio_play_extension.dart to be more specific: replace "ignore linter" with "linter suppression" or "ignore directive", state which specific ignore is used (e.g., the analyzer rule name) and add when it can be removed (e.g., "remove when experimental member use is stabilized/removed"), so the TODO in the file's top comment clearly indicates the suppression, the rule, and the removal condition.integration_test/robots/login_robot.dart (1)
191-218: Broad exception catching limits debuggability; logging would help identify failures.The
catch (_)blocks swallow all exceptions indiscriminately. While the downstream$(ChatList).waitUntilVisible()assertion inlogin_scenario.dartwill eventually fail for a broken login, transient failures are harder to diagnose without visibility into what exception was caught.Additionally, the early
returnat line 198 means if the first tap throws any exception, the Captcha dialog handling (lines 201–218) is skipped entirely. If the Captcha is shown but the first tap fails for an unrelated reason, the flow will exit prematurely.Patrol does not expose a specific exception type for native view lifecycle events (e.g., "view closed" or "disappeared"), so broader exception handling is the current pattern. Consider adding a log statement to the catch blocks for debugging visibility when test failures occur:
try { await $.native.tap(getSignInBtn(), appId: getBrowserAppId()); } catch (_) { // Browser closed after successful SSO login – expected. + // Log for debugging if SSO fails unexpectedly + // print('Sign-in tap threw exception: $_'); return; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@integration_test/robots/login_robot.dart` around lines 191 - 218, The try/catch blocks around $.native.tap(getSignInBtn(), appId: getBrowserAppId()) and the second tap currently swallow all exceptions and the first catch returns early, skipping the Captcha handling; update these catches to log the caught exception (include the exception message/stack via the test logger) rather than silently ignoring it, and remove the early return in the first catch so the code continues to check CoreRobot($).existsOptionalNativeItems(..., getOKBtnInVerifyCaptchaDialog(), ...) and attempt the Captcha flow; also add the same logging to the second catch so any unexpected failure during the retry is recorded (refer to getSignInBtn, getBrowserAppId, getOKBtnInVerifyCaptchaDialog, CoreRobot.existsOptionalNativeItems, and $.native.tap to find the locations).lib/pages/settings_dashboard/settings_security/settings_security_view.dart (1)
154-160: Keep the copy flow behind the explicit icon.
SettingsItemBuilderalready makes the whole tile tappable, so wiringonTap: controller.copyRecoveryKeyhere turns the entire recovery-key row into a copy action. Since this copies a secret, I'd keep the row non-destructive and reserve copying for the trailing control.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/pages/settings_dashboard/settings_security/settings_security_view.dart` around lines 154 - 160, The recovery-key row currently wires the tile-level onTap to controller.copyRecoveryKey, making the whole SettingsItemBuilder tappable; remove or null out the tile onTap (the onTap property currently set to controller.copyRecoveryKey) so only the trailingWidget InkWell (key: 'recovery_key_copy_button') triggers copyRecoveryKey, preserving non-destructive behavior for the row while keeping the trailing copy icon functional.integration_test/tests/setting/settings_recovery_key_test.dart (1)
26-51: Please cover the dismiss path too.This only proves the confirm flow. A regression where the warning dialog is dismissed but the clipboard is still written would slip through.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@integration_test/tests/setting/settings_recovery_key_test.dart` around lines 26 - 51, The test only exercises the confirm path via recoveryKeyRobot.tapCopyAndConfirm() so add a dismiss-path assertion: invoke the dialog dismiss action (e.g., a new or existing recoveryKeyRobot.tapCopyAndDismiss() or call to tap copy then tapCancel/dismiss on the dialog), then assert no confirmation snackbar via recoveryKeyRobot.verifySnackBarIsNotShown() (or invert verifySnackBarIsShown), and verify the clipboard remains null/empty or still contains the masked value by calling recoveryKeyRobot.getClipboardText() and asserting it equals null/empty or equals '\u2022' * 32; update or add helper methods on recoveryKeyRobot if needed to perform the dismiss action and negative snackbar check.lib/pages/settings_dashboard/settings_security/settings_security.dart (1)
50-55: Consider callingsuper.initState()first.Flutter's convention recommends calling
super.initState()before any other initialization logic. While the current ordering works, placing it first ensures any base class setup completes before subclass logic runs.🔧 Suggested reorder
`@override` void initState() { + super.initState(); listenIgnoredUser(); recoveryKeyFuture = _fetchRecoveryKey(); - super.initState(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/pages/settings_dashboard/settings_security/settings_security.dart` around lines 50 - 55, Move the call to super.initState() to the top of the initState method so base-class initialization runs before subclass work: in the SettingsSecurityState class's initState, call super.initState() first, then invoke listenIgnoredUser() and set recoveryKeyFuture = _fetchRecoveryKey(); to preserve current behavior while following Flutter conventions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@assets/l10n/intl_en.arb`:
- Around line 1742-1747: Add the three new localization keys recoveryKey,
recoveryKeyWarningMessage, and recoveryKeyCopiedToClipboard (and their
corresponding metadata entries `@recoveryKey`, `@recoveryKeyWarningMessage`,
`@recoveryKeyCopiedToClipboard`) to every remaining intl_*.arb locale file that
currently lacks them; ensure each file mirrors the exact English strings and
empty metadata objects from intl_en.arb (so non-English locales have the keys
present for validation/fallback), and also add the two missing keys to
intl_ie.arb to complete its partial entry.
---
Nitpick comments:
In `@integration_test/robots/login_robot.dart`:
- Around line 191-218: The try/catch blocks around $.native.tap(getSignInBtn(),
appId: getBrowserAppId()) and the second tap currently swallow all exceptions
and the first catch returns early, skipping the Captcha handling; update these
catches to log the caught exception (include the exception message/stack via the
test logger) rather than silently ignoring it, and remove the early return in
the first catch so the code continues to check
CoreRobot($).existsOptionalNativeItems(..., getOKBtnInVerifyCaptchaDialog(),
...) and attempt the Captcha flow; also add the same logging to the second catch
so any unexpected failure during the retry is recorded (refer to getSignInBtn,
getBrowserAppId, getOKBtnInVerifyCaptchaDialog,
CoreRobot.existsOptionalNativeItems, and $.native.tap to find the locations).
In `@integration_test/tests/setting/settings_recovery_key_test.dart`:
- Around line 26-51: The test only exercises the confirm path via
recoveryKeyRobot.tapCopyAndConfirm() so add a dismiss-path assertion: invoke the
dialog dismiss action (e.g., a new or existing
recoveryKeyRobot.tapCopyAndDismiss() or call to tap copy then tapCancel/dismiss
on the dialog), then assert no confirmation snackbar via
recoveryKeyRobot.verifySnackBarIsNotShown() (or invert verifySnackBarIsShown),
and verify the clipboard remains null/empty or still contains the masked value
by calling recoveryKeyRobot.getClipboardText() and asserting it equals
null/empty or equals '\u2022' * 32; update or add helper methods on
recoveryKeyRobot if needed to perform the dismiss action and negative snackbar
check.
In `@lib/pages/chat/events/audio_message/audio_play_extension.dart`:
- Line 6: Update the TODO in audio_play_extension.dart to be more specific:
replace "ignore linter" with "linter suppression" or "ignore directive", state
which specific ignore is used (e.g., the analyzer rule name) and add when it can
be removed (e.g., "remove when experimental member use is stabilized/removed"),
so the TODO in the file's top comment clearly indicates the suppression, the
rule, and the removal condition.
In `@lib/pages/settings_dashboard/settings_security/settings_security_view.dart`:
- Around line 154-160: The recovery-key row currently wires the tile-level onTap
to controller.copyRecoveryKey, making the whole SettingsItemBuilder tappable;
remove or null out the tile onTap (the onTap property currently set to
controller.copyRecoveryKey) so only the trailingWidget InkWell (key:
'recovery_key_copy_button') triggers copyRecoveryKey, preserving non-destructive
behavior for the row while keeping the trailing copy icon functional.
In `@lib/pages/settings_dashboard/settings_security/settings_security.dart`:
- Around line 50-55: Move the call to super.initState() to the top of the
initState method so base-class initialization runs before subclass work: in the
SettingsSecurityState class's initState, call super.initState() first, then
invoke listenIgnoredUser() and set recoveryKeyFuture = _fetchRecoveryKey(); to
preserve current behavior while following Flutter conventions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 033c4b52-5f98-4f8b-99db-26487e42c5bf
⛔ Files ignored due to path filters (1)
assets/images/ic_recovery_key.svgis excluded by!**/*.svg
📒 Files selected for processing (12)
analysis_options.yamlassets/l10n/intl_en.arbintegration_test/robots/login_robot.dartintegration_test/robots/setting/settings_recovery_key_robot.dartintegration_test/tests/setting/settings_recovery_key_test.dartlib/pages/chat/events/audio_message/audio_play_extension.dartlib/pages/settings_dashboard/settings/settings_item_builder.dartlib/pages/settings_dashboard/settings/settings_view.dartlib/pages/settings_dashboard/settings/settings_view_style.dartlib/pages/settings_dashboard/settings_security/settings_security.dartlib/pages/settings_dashboard/settings_security/settings_security_view.dartlib/resource/image_paths.dart
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.
Summary
Split from #2914 — this PR contains only the feature (no refactoring).
GetRecoveryWordsInteractoric_recovery_key.svg) and i18n stringsRelated PR (refacto): #2945
Ticket
Related issue: TW-2991
Test recommendations
Summary by CodeRabbit
Release Notes
New Features
Tests
Refactor
Chores