Closed
Conversation
WalkthroughAdds many new centralized route constants (AppRoutePaths) and replaces hard-coded route strings with those constants across GoRouter and multiple widgets. Introduces recovery key support: new localization entries, controller methods to fetch/copy the recovery key, UI items, an image asset getter, and integration test robot + test. Large set of UI files updated to cache L10n/color/theme values and use concise Dart shorthand syntax. Minor fixes include renaming a divider thickness constant and updating analyzer excludes. 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 unit tests (beta)
📝 Coding Plan for PR comments
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 |
9clg6
requested review from
dab246,
hoangdat,
nqhhdev and
tddang-linagora
as code owners
Contributor
|
This PR has been deployed to https://linagora.github.io/twake-on-matrix/2914 |
9clg6
force-pushed
the
fix/TW-2991-display-recovery-key-in-the-app
branch
from
2439904 to
a624e0c
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
Note
Due to the large number of review comments, Critical severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (8)
lib/pages/settings_dashboard/settings_multiple_emotes/settings_multiple_emotes_view.dart (1)
33-35:⚠️ Potential issue | 🟠 MajorURL-encode pack keys before interpolating into route path.
When
keys[i]is the empty string'', the navigation path becomesrooms/{roomId}/details/emotes/, losing the pack ID. Keys containing/,?, or#will also break routing without URL encoding.Also applies to: 56-57
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/pages/settings_dashboard/settings_multiple_emotes/settings_multiple_emotes_view.dart` around lines 33 - 35, URL-encode pack keys before using them in route interpolation: wrap occurrences of keys[i] (and any map keys like the empty-string entry in packs) with Uri.encodeComponent(...) when building the navigation path (the code that constructs "rooms/{roomId}/details/emotes/{packId}" around the navigation call at the locations referenced). This prevents slashes, ?, #, etc. from breaking routing and ensures the packId segment is safely encoded; apply the same encoding where you create/lookup packs[''] and in the navigation call(s) around lines 56-57.lib/pages/settings_dashboard/settings_3pid/settings_3pid.dart (1)
20-62:⚠️ Potential issue | 🟠 MajorGuard both async flows with
mountedbefore reusingcontextor callingsetState.These methods still read
Matrix.of(context)and callsetStateafter multiple awaits. If the user closes the screen mid-flow, this can hit a staleBuildContextorsetState()on a disposedState.🛡️ Suggested guard pattern
void add3PidAction() async { final l10n = L10n.of(context)!; @@ - if (input == null) return; + if (input == null || !mounted) return; + final client = Matrix.of(context).client; final clientSecret = DateTime.now().millisecondsSinceEpoch.toString(); final response = await TwakeDialog.showFutureLoadingDialogFullScreen( - future: () => Matrix.of(context).client.requestTokenToRegisterEmail( + future: () => client.requestTokenToRegisterEmail( clientSecret, input.single, Settings3Pid.sendAttempt++, ), ); - if (response.error != null) return; + if (response.error != null || !mounted) return; @@ final success = await TwakeDialog.showFutureLoadingDialogFullScreen( - future: () => Matrix.of(context).client.uiaRequestBackground( - (auth) => Matrix.of( - context, - ).client.add3PID(clientSecret, response.result!.sid, auth: auth), + future: () => client.uiaRequestBackground( + (auth) => client.add3PID(clientSecret, response.result!.sid, auth: auth), ), ); - if (success.error != null) return; + if (success.error != null || !mounted) return; setState(() => request = null); }Apply the same
if (!mounted) return;pattern indelete3Pidbefore post-awaitcontextaccess and beforesetState.Also applies to: 67-85
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/pages/settings_dashboard/settings_3pid/settings_3pid.dart` around lines 20 - 62, In add3PidAction (and similarly in delete3Pid) guard all post-await uses of the BuildContext and setState by checking the State's mounted flag: after each await that is followed by Matrix.of(context) or setState, add if (!mounted) return; and, where you need Matrix.of(context) across multiple lines, capture Matrix.of(context).client into a local variable only after verifying mounted to avoid using a disposed context; likewise ensure setState is only called after a mounted check.lib/pages/settings_dashboard/settings/settings.dart (2)
79-90:⚠️ Potential issue | 🟠 MajorGuard the null
routerKeycontext before showing the logout dialog.If
TwakeApp.routerKey.currentContextis unavailable, Line 86 still force-unwraps it and crashes the logout flow.🛠️ Minimal fix
final twakeContext = TwakeApp.routerKey.currentContext; if (twakeContext == null) { Logs().e('SettingsController()::logoutAction - Twake context is null'); + return; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/pages/settings_dashboard/settings/settings.dart` around lines 79 - 90, The code force-unwraps TwakeApp.routerKey.currentContext (twakeContext) when calling showConfirmAlertDialog in logoutAction, which can crash; update logoutAction to check twakeContext for null and bail out (or fall back to a safe context like the current method's context) before calling showConfirmAlertDialog, e.g., if twakeContext == null return (or use context) so showConfirmAlertDialog is only invoked with a non-null context.
262-265:⚠️ Potential issue | 🔴 CriticalAdd an explicit
breakafter the About action.Line 263 has a non-empty
casewith no terminating statement (nobreak,return, or similar control flow) before the nextcaseon line 264. Dart disallows this pattern and will report a compile-time error: "Switch case may fall through to the next case."Fix
case SettingEnum.about: PlatformInfos.showAboutDialogFullScreen(); + break; case SettingEnum.logout:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/pages/settings_dashboard/settings/settings.dart` around lines 262 - 265, The switch handling for SettingEnum.about currently calls PlatformInfos.showAboutDialogFullScreen() but falls through into SettingEnum.logout; add an explicit control transfer (e.g., a break or return) immediately after the call to PlatformInfos.showAboutDialogFullScreen() to prevent fall-through into logoutAction(); ensure the change is made in the switch that contains SettingEnum.about and SettingEnum.logout so logoutAction() only runs for the logout case.lib/pages/settings_dashboard/settings_contacts_visibility/settings_contacts_visibility_view.dart (3)
222-243:⚠️ Potential issue | 🔴 CriticalInvalid EdgeInsets shorthand in _buildVisibilityOptionItem.
🐛 Proposed fix
Padding( - padding: const .all(16), + padding: const EdgeInsets.all(16), child: Text( ... Padding( - padding: const .only(left: 8, right: 16), + padding: const EdgeInsets.only(left: 8, right: 16),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/pages/settings_dashboard/settings_contacts_visibility/settings_contacts_visibility_view.dart` around lines 222 - 243, The widget contains invalid shorthand tokens (e.g., "const .all(16)", "overflow: .ellipsis", "textAlign: .center", "const .only(left: 8, right: 16)") in _buildVisibilityOptionItem; replace these with the proper Flutter identifiers (EdgeInsets.all(16), TextOverflow.ellipsis, TextAlign.center, EdgeInsets.only(...)) so the Padding and Text properties compile correctly, and update any other similar occurrences in the same method (look for option.title, sysColor, enableDivider and Divider usage) to use the full names.
186-200:⚠️ Potential issue | 🔴 CriticalInvalid BorderRadius.only shorthand in switch cases.
While the enum values (
.public,.contacts,.private) are valid shorthand, theBorderRadius.onlyandRadius.circularrequire full class prefixes.🐛 Proposed fix
BorderRadius? _buildVisibilityOptionBorderRadius({ required SettingsContactsVisibilityEnum option, }) { switch (option) { case .public: - return const .only(topLeft: .circular(16), topRight: .circular(16)); + return const BorderRadius.only(topLeft: Radius.circular(16), topRight: Radius.circular(16)); case .contacts: return null; case .private: - return const .only( - bottomLeft: .circular(16), - bottomRight: .circular(16), + return const BorderRadius.only( + bottomLeft: Radius.circular(16), + bottomRight: Radius.circular(16), ); } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/pages/settings_dashboard/settings_contacts_visibility/settings_contacts_visibility_view.dart` around lines 186 - 200, In _buildVisibilityOptionBorderRadius (using SettingsContactsVisibilityEnum), replace the shorthand class invocations with fully qualified constructors: return const BorderRadius.only(...) and use Radius.circular(...) inside those calls (e.g., const BorderRadius.only(topLeft: Radius.circular(16), topRight: Radius.circular(16)) and const BorderRadius.only(bottomLeft: Radius.circular(16), bottomRight: Radius.circular(16))); keep the enum switch cases (e.g., .public, .contacts, .private) as-is and return null for the .contacts branch.
295-330:⚠️ Potential issue | 🔴 CriticalInvalid EdgeInsets shorthand in _buildVisibleFieldItem.
🐛 Proposed fix
Padding( - padding: const .all(16), + padding: const EdgeInsets.all(16), child: Column( ... Padding( - padding: const .only(left: 8, right: 16), + padding: const EdgeInsets.only(left: 8, right: 16),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/pages/settings_dashboard/settings_contacts_visibility/settings_contacts_visibility_view.dart` around lines 295 - 330, In _buildVisibleFieldItem, replace the invalid shorthand tokens with the proper Flutter enums and constructors: change "const .all(16)" and "const .only(...)" to "const EdgeInsets.all(...)" and "const EdgeInsets.only(...)", change "crossAxisAlignment: .start" to "crossAxisAlignment: CrossAxisAlignment.start", change "overflow: .ellipsis" to "overflow: TextOverflow.ellipsis", and change "textAlign: .center" / ".left" to "textAlign: TextAlign.center" / "TextAlign.left" so the Padding, Column, and Text widgets use correct types and constants.lib/pages/settings_dashboard/settings_profile/settings_profile_view_mobile.dart (1)
127-136:⚠️ Potential issue | 🔴 CriticalInvalid Dart syntax: named constructors require full class prefix.
.circular(...)and.all(...)are not valid shorthand in Dart.BorderRadius.circular()andBorder.all()are named constructors that require the full class prefix. The compiler cannot infer the type from the shorthand notation alone.🐛 Proposed fix
child: Container( decoration: BoxDecoration( color: colorScheme.primary, - borderRadius: .circular( + borderRadius: BorderRadius.circular( SettingsProfileViewMobileStyle .avatarSize, ), - border: .all( + border: Border.all( color: colorScheme.onPrimary, width: SettingsProfileViewMobileStyle .iconEditBorderWidth, ), ),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/pages/settings_dashboard/settings_profile/settings_profile_view_mobile.dart` around lines 127 - 136, The snippet uses shorthand constructors `.circular(...)` and `.all(...)` which are invalid in Dart; update the BorderRadius and Border usages to their full named constructors — replace `.circular(SettingsProfileViewMobileStyle.avatarSize)` with `BorderRadius.circular(SettingsProfileViewMobileStyle.avatarSize)` and replace `.all(color: colorScheme.onPrimary, width: SettingsProfileViewMobileStyle.iconEditBorderWidth)` with `Border.all(color: colorScheme.onPrimary, width: SettingsProfileViewMobileStyle.iconEditBorderWidth)` so the compiler recognizes the constructors.
🟠 Major comments (9)
lib/pages/settings_dashboard/settings_emotes/settings_emotes_view.dart-125-126 (1)
125-126:⚠️ Potential issue | 🟠 MajorMemory leak: TextEditingController created in itemBuilder without disposal.
A new
TextEditingControlleris instantiated on every call toitemBuilder. SinceitemBuilderis invoked each time the list rebuilds or scrolls, these controllers accumulate without being disposed, leaking native resources.Consider managing controllers at the parent level (e.g., a
Map<String, TextEditingController>keyed byimageCodewith proper disposal in the controller'sdispose()method), or wrap each list item in aStatefulWidgetthat creates and disposes its own controller.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/pages/settings_dashboard/settings_emotes/settings_emotes_view.dart` around lines 125 - 126, A TextEditingController is being created inside itemBuilder (TextEditingController and textEditingController.text = imageCode) causing leaked controllers; move controller lifecycle out of the build loop by creating and caching controllers keyed by imageCode (e.g., Map<String, TextEditingController>) at the parent widget state and reuse them in itemBuilder, and ensure all controllers are disposed in the parent State.dispose(); alternatively, wrap each list item in a StatefulWidget that instantiates a TextEditingController for that item and disposes it in its own dispose() method.lib/pages/settings_dashboard/settings_multiple_emotes/settings_multiple_emotes_view.dart-17-17 (1)
17-17:⚠️ Potential issue | 🟠 MajorGuard the room lookup instead of forcing two null assertions.
controller.roomIdis nullable here, andgetRoomById(...)can also returnnullif the room is no longer in cache. The current!/!turns both cases into a build-time crash.Proposed fix
- final room = Matrix.of(context).client.getRoomById(controller.roomId!)!; + final roomId = controller.roomId; + final room = roomId == null + ? null + : Matrix.of(context).client.getRoomById(roomId); + if (room == null) { + return const SizedBox.shrink(); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/pages/settings_dashboard/settings_multiple_emotes/settings_multiple_emotes_view.dart` at line 17, The direct double-null-assertion on controller.roomId and getRoomById causes a crash if either is null; change the lookup in settings_multiple_emotes_view.dart to guard both values (check controller.roomId != null, then call Matrix.of(context).client.getRoomById(controller.roomId!) and handle a null room) and return an appropriate fallback (e.g., early return widget, placeholder, or error UI) instead of using the two ! operators; update any downstream uses of room to account for the guarded/nullable value (or store a non-null local only after the guard) so you never crash on missing roomId or missing cached room.lib/pages/settings_dashboard/settings_security/settings_security.dart-148-156 (1)
148-156:⚠️ Potential issue | 🟠 MajorThe passcode validator will throw on non-digit input.
int.tryParse(text)!is force-unwrapped once the input reaches 4 chars. Pasting something like12a4will hit a null assertion error instead of returning the validation message.Suggested fix
DialogTextField( validator: (text) { - if (text!.isEmpty || - (text.length == 4 && int.tryParse(text)! >= 0)) { + final value = text ?? ''; + if (value.isEmpty || RegExp(r'^\d{4}$').hasMatch(value)) { return null; } return l10n.pleaseEnter4Digits; },🤖 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 148 - 156, The validator in DialogTextField currently force-unwraps int.tryParse(text) which throws on non-digit input; update the validator in the DialogTextField (the validator closure) to avoid the bang: first check text != null and text.length == 4, then call int.tryParse(text) and test that the result is non-null and >= 0 (e.g., final value = int.tryParse(text); if (value != null && value >= 0) return null;), otherwise return l10n.pleaseEnter4Digits; ensure no `!` is used on tryParse to prevent null assertion errors.lib/pages/settings_dashboard/settings_security/settings_security.dart-96-121 (1)
96-121:⚠️ Potential issue | 🟠 MajorFix password change dialog to properly collect and validate passwords.
The dialog labels (lines 106-112) show "new password" and "repeat password", but line 121 sends
input.firstasoldPasswordandinput.lastas the new password. This reverses the intended semantics and skips validation that both entries match. The dialog should either:
- Explicitly ask for the current password, new password, and confirmation, or
- If the old password is obtained elsewhere, collect only the new password and its confirmation with validation that they match
🤖 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 96 - 121, In changePasswordAccountAction, the dialog collects two password fields but passes them reversed to client.changePassword and never validates they match; update showTextInputDialog (in changePasswordAccountAction) to collect three fields (currentPassword, newPassword, confirmPassword) or at minimum collect newPassword and confirmPassword and validate equality before calling TwakeDialog.showFutureLoadingDialogFullScreen; then call client.changePassword(newPassword, oldPassword: currentPassword) (or if current password is obtained elsewhere, ensure you pass the correct oldPassword and validate input.first vs input.last properly) and show a validation error if confirmation != new password.lib/pages/settings_dashboard/settings_security/settings_security_view.dart-131-145 (1)
131-145:⚠️ Potential issue | 🟠 MajorDefer loading the plaintext recovery key until the user confirms.
Lines 131-145 fetch the actual secret as soon as this screen builds, even though the UI only renders a masked placeholder. That means the warning dialog only protects the clipboard write, not the secret fetch itself. Prefer a lightweight existence check for rendering this row, then load/copy the key after confirmation; if this
FutureBuilderstays, make sure the future is cached so rebuilds do not refetch the secret.🤖 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 131 - 145, The FutureBuilder currently triggers fetching the plaintext recovery key (controller.recoveryKeyFuture) as soon as the screen builds; change this to only perform a lightweight existence check for rendering the masked row and defer fetching/copying the real secret until after the user confirms in the warning dialog. Concretely: replace or wrap the current FutureBuilder usage around controller.recoveryKeyFuture with a fast existence-check future (or a boolean controller.hasRecoveryKey check) to decide whether to show the masked SettingsItemBuilder row, and move the actual call to fetch the plaintext key into the confirmation flow (the handler that opens the warning dialog and performs the clipboard write). If you keep using a future for the real key, ensure it is cached on the controller (e.g., controller.getRecoveryKeyCached()) so rebuilds do not refetch the secret; reference controller.recoveryKeyFuture, the FutureBuilder, and the SettingsItemBuilder when making these changes.assets/l10n/intl_en.arb-1791-1794 (1)
1791-1794:⚠️ Potential issue | 🟠 MajorLocalize the new recovery-key strings in every supported ARB, not just English.
The PR screenshot already shows this warning dialog in English on a French settings page. Because this dialog protects a security-sensitive action, shipping the new copy/warning strings untranslated is a real comprehension problem for non-English users. Please mirror these keys in the other locale ARBs before merging.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@assets/l10n/intl_en.arb` around lines 1791 - 1794, Add the two new keys "recoveryKeyWarningMessage" and "recoveryKeyCopiedToClipboard" (and their companion metadata entries "@recoveryKeyWarningMessage" and "@recoveryKeyCopiedToClipboard") to every non-English ARB file so users see localized text; translate the English strings into each supported language (or add translator notes/placeholders if you need a follow-up), keep the key names identical, mirror the metadata objects as in the English ARB, and ensure any punctuation/placeholder formatting matches existing locale entries.lib/pages/settings_dashboard/settings/settings_item_builder.dart-99-106 (1)
99-106:⚠️ Potential issue | 🟠 MajorPreserve the chevron when a trailing widget is provided.
This branch makes
trailingWidgetreplace the default chevron instead of augmenting it. That conflicts with the PR screenshots, which still show both a copy icon and a chevron on the recovery/public-key rows, and it changes the current API semantics becauseisHideTrailingIconis supposed to be the opt-out for hiding the chevron.
↔️ Proposed fix- if (trailingWidget != null) - trailingWidget! - else if (!isHideTrailingIcon) - Icon( - Icons.chevron_right_outlined, - size: SettingsViewStyle.iconSize, - color: tertiary30, - ), + if (trailingWidget != null) trailingWidget!, + if (!isHideTrailingIcon) ...[ + if (trailingWidget != null) const SizedBox(width: 4), + Icon( + Icons.chevron_right_outlined, + size: SettingsViewStyle.iconSize, + color: tertiary30, + ), + ],🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/pages/settings_dashboard/settings/settings_item_builder.dart` around lines 99 - 106, The current logic replaces the default chevron when trailingWidget is provided; change it so trailingWidget augments rather than replaces the chevron: in the widget that renders the trailing area (where trailingWidget, isHideTrailingIcon, Icon(Icons.chevron_right_outlined), SettingsViewStyle.iconSize, and tertiary30 are referenced) render trailingWidget first (if not null) and then, unless isHideTrailingIcon is true, render the chevron Icon after it (e.g., wrap them in a Row or similar), so the chevron remains visible by default and is only hidden when isHideTrailingIcon is true.lib/pages/settings_dashboard/settings_3pid/settings_3pid_view.dart-38-41 (1)
38-41:⚠️ Potential issue | 🟠 MajorDon't render raw exceptions in the settings UI.
snapshot.error.toString()exposes backend/internal failure details directly to end users here. Please log the exception and show a generic localized error state instead.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/pages/settings_dashboard/settings_3pid/settings_3pid_view.dart` around lines 38 - 41, The UI currently renders raw exceptions from the FutureBuilder snapshot (the branch that returns Center(child: Text(snapshot.error.toString(), ...))) which leaks internal details; instead, log the exception (use the app logger or error reporting used elsewhere) and replace the Text(snapshot.error.toString()) with a generic, localized error widget/message (e.g., a localized string like S.of(context).genericError or a SettingsErrorState widget) while keeping the same layout (Center). Locate the FutureBuilder/error-handling code in settings_3pid_view.dart (the snapshot.hasError branch) and change it to log snapshot.error and show the localized/generic error text to the user.lib/pages/settings_dashboard/settings_style/settings_style.dart-34-37 (1)
34-37:⚠️ Potential issue | 🟠 MajorDon't short-circuit the reset color option.
customColorsstill exposes anullswatch, and the view still renders it as selectable. Returning early here makes that option a no-op, so users can no longer clear the custom chat color once one is picked. If reset is no longer intended, remove thenullswatch instead.🛠️ Minimal fix
void setChatColor(Color? color) async { - if (color == null) return; AppConfig.colorSchemeSeed = color; controller.setPrimaryColor(color); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/pages/settings_dashboard/settings_style/settings_style.dart` around lines 34 - 37, The setChatColor method short-circuits when color is null, preventing the "reset" swatch (null entry in customColors) from working; instead allow null to flow through and clear the stored color and UI state. Modify setChatColor to handle a null argument by clearing AppConfig.colorSchemeSeed (set to null or default), calling controller.setPrimaryColor(null) or controller.resetPrimaryColor(), and updating any persisted state so the null swatch actually resets the chat color; keep current behavior for non-null values (AppConfig.colorSchemeSeed = color and controller.setPrimaryColor(color)).
🟡 Minor comments (1)
lib/pages/settings_dashboard/settings_security/settings_security.dart-184-194 (1)
184-194:⚠️ Potential issue | 🟡 MinorGuard
contextafter the confirmation dialog.The method uses
contextagain after awaitingshowOkCancelAlertDialogto show the loading dialog and accessMatrix.of(context). If the page is disposed between the dialog completion and these subsequent operations, this creates an async-gap context hazard that could cause errors.Suggested fix
final response = await showOkCancelAlertDialog( context: context, isDestructiveAction: true, title: l10n.dehydrate, message: l10n.dehydrateWarning, ); - if (response != OkCancelResult.ok) return; + if (!context.mounted || response != OkCancelResult.ok) return;🤖 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 184 - 194, After awaiting showOkCancelAlertDialog, guard against the widget being disposed before reusing context: add a mounted check immediately after the dialog response (e.g., if (!context.mounted) return;) before calling TwakeDialog.showFutureLoadingDialogFullScreen or Matrix.of(context) to avoid the async-gap context hazard; update the block around showOkCancelAlertDialog, TwakeDialog.showFutureLoadingDialogFullScreen, and Matrix.of(context) accordingly.
🧹 Nitpick comments (6)
lib/pages/settings_dashboard/settings_emotes/settings_emotes_view.dart (2)
26-29: Minor: Reuse cachedthemevariable.Line 27 calls
Theme.of(context)again whenthemeis already cached on line 25.♻️ Suggested fix
final textTheme = TextStyle( - color: Theme.of(context).colorScheme.secondary, + color: theme.colorScheme.secondary, fontWeight: FontWeight.bold, );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/pages/settings_dashboard/settings_emotes/settings_emotes_view.dart` around lines 26 - 29, The TextStyle construction in the variable textTheme calls Theme.of(context) again; use the already-cached theme variable instead by replacing Theme.of(context).colorScheme.secondary with theme.colorScheme.secondary in the textTheme declaration (variables: theme and textTheme) so the theme is reused rather than fetched twice.
127-127: Consider removingisDesktopcheck if desktop is unsupported.Based on learnings, desktop platforms (macOS, Linux, Windows) are not supported for this app. The
isDesktopbranch in this condition may be dead code. If desktop support is indeed excluded, simplify to justPlatformInfos.isWeb.♻️ Suggested simplification
- final useShortCuts = (PlatformInfos.isWeb || PlatformInfos.isDesktop); + final useShortCuts = PlatformInfos.isWeb;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/pages/settings_dashboard/settings_emotes/settings_emotes_view.dart` at line 127, The condition assigning useShortCuts uses PlatformInfos.isDesktop which appears to be dead if desktop platforms are unsupported; update the expression in settings_emotes_view.dart where useShortCuts is defined to remove the isDesktop check and only use PlatformInfos.isWeb (i.e., change the assignment of the useShortCuts variable to rely solely on PlatformInfos.isWeb), and run a quick search for any other usages of PlatformInfos.isDesktop in this file to remove or adjust related dead branches.lib/pages/chat/events/audio_message/audio_play_extension.dart (1)
6-6: Use a regular TODO comment here, not a doc comment.
///TODO(...)becomes the public docstring forMatrixFileAudioSource, so generated API docs will show the TODO instead of class documentation. This should be// TODO(...)unless you intend to document the class itself.🤖 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, The TODO is written as a documentation comment which makes it part of the public API docs for MatrixFileAudioSource; change the leading doc comment marker from ///TODO(...) to a non-doc single-line comment like // TODO(...) so the note remains a developer TODO and does not become the class's docstring (look for the TODO above MatrixFileAudioSource and replace the triple-slash with double-slash).lib/pages/settings_dashboard/settings_blocked_users/settings_blocked_users_view.dart (1)
134-158: Consider routing through the centralized path helpers here too.This callback still hardcodes the draft-chat and room paths. Since this PR is already moving navigation strings behind shared route definitions, using the same helper/constants here would reduce drift the next time those routes change.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/pages/settings_dashboard/settings_blocked_users/settings_blocked_users_view.dart` around lines 134 - 158, The onTap callback in settings_blocked_users_view uses hardcoded route strings ('/rooms/draftChat' and '/rooms/$roomId') when creating/entering chats; update it to use the centralized route helpers/constants instead: replace the hardcoded draft-chat path passed to context.go inside Router.neglect with the shared draft chat route constant/helper (used elsewhere in the app), and replace the '/rooms/$roomId' usages in both context.push and context.go with the room route builder/helper that accepts a roomId parameter (while keeping the same logic around Router.neglect, getDirectChatFromUserId, responsiveUtils.isMobile, and client.userID checks).lib/pages/settings_dashboard/settings_stories/settings_stories.dart (1)
86-86: Consider keeping thecontextparameter name inbuild.Using
Widget build(_)works but is unconventional for Flutter's build method. If future changes require accessingcontext(e.g., for navigation, themes, or localization), renaming back tocontextwill be needed. This is a minor style preference.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/pages/settings_dashboard/settings_stories/settings_stories.dart` at line 86, The build method currently uses an unnamed parameter (Widget build(_)) which is unconventional; change it to explicitly name the BuildContext parameter (e.g., Widget build(BuildContext context)) so the method signature is standard and future uses of context (navigation, theme, localization) are straightforward; update the method declaration where SettingsStories defines build (the override of build that returns SettingsStoriesView(this)) to use the named parameter.lib/pages/settings_dashboard/settings_contacts_visibility/settings_contacts_visibility.dart (1)
169-176: Inconsistent localization access.Line 175 uses
L10n.of(context)!directly whileupdateUserInfoVisibility(line 113) cachesl10nlocally. For consistency and slight performance benefit, consider cachingl10nat the start ofinitialGetUserInfoVisibilityas well.♻️ Suggested fix
void initialGetUserInfoVisibility() { + final l10n = L10n.of(context)!; if (client.userID == null) { return; } getUserInfoVisibilityStreamSub?.cancel(); getUserInfoVisibilityStreamSub = getIt .get<GetUserInfoVisibilityInteractor>() .execute(userId: client.userID!) .listen((either) { if (!mounted) return; getUserInfoVisibilityNotifier.value = either; either.fold( (failure) { if (failure is GetUserInfoVisibilityFailure) { TwakeDialog.hideLoadingDialog(context); TwakeSnackBar.show( context, - L10n.of(context)!.failedToGetContactsVisibility, + l10n.failedToGetContactsVisibility, ); } },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/pages/settings_dashboard/settings_contacts_visibility/settings_contacts_visibility.dart` around lines 169 - 176, In initialGetUserInfoVisibility, cache the localization instance like in updateUserInfoVisibility by assigning final l10n = L10n.of(context)! at the start and replace direct calls to L10n.of(context)! (e.g., the usage in the failure branch that shows TwakeSnackBar) with l10n; update all occurrences inside initialGetUserInfoVisibility to use this local l10n variable for consistency and minor performance gain.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@lib/pages/settings_dashboard/settings_contacts_visibility/settings_contacts_visibility_view.dart`:
- Around line 119-142: The code uses invalid shorthand member calls (e.g.,
".only", ".circular", ".all", ".center") inside the nested Column widgets;
update them to the proper constructors and enums: replace "const .only(...)"
with "EdgeInsets.only(...)", "borderRadius: .circular(16)" with
"BorderRadius.circular(16)", "border: .all(...)" with "Border.all(...)" and
"textAlign: .center" with "TextAlign.center"; apply these fixes around the
Padding/Text that references l10n.chooseWhichDetailsAreVisibleToOtherUsers and
the Container that uses refColor90 so the widgets compile correctly.
- Around line 262-272: In _buildVisibleFieldBorderRadius the Dart shorthand
".only" and ".circular" are invalid; replace them with the proper constructors:
return const BorderRadius.only(topLeft: Radius.circular(16), topRight:
Radius.circular(16)) for the phone case and return const
BorderRadius.only(bottomLeft: Radius.circular(16), bottomRight:
Radius.circular(16)) for the email case (use Radius.circular, not .circular, and
BorderRadius.only, not .only); ensure the switch over VisibleEnum (phone, email)
remains exhaustive or provide a default return if needed.
- Around line 47-74: The code uses invalid shorthand constructors (e.g. "const
.symmetric", ".only", ".circular", ".all") in the Padding, Text container
decoration and border; replace them with the full factory calls:
EdgeInsets.symmetric / EdgeInsets.only for padding in the Padding widgets,
BorderRadius.circular for the Container's borderRadius, and Border.all for the
Container's border. Update occurrences around the Padding and Container in
settings_contacts_visibility_view (the Padding child block and the Container
decoration) to use these proper constructors so the widget builds correctly.
In
`@lib/pages/settings_dashboard/settings_profile/settings_profile_item_style.dart`:
- Line 9: Replace the invalid omitted-type constructor calls on the abstract
EdgeInsetsGeometry with explicit EdgeInsets constructors: change the static
const declaration textPadding (currently using .symmetric) to use
EdgeInsets.symmetric(...) and also change the margin usage in
settings_profile_item (the property using .only(left: 40)) to
EdgeInsets.only(left: 40); refer to the symbol textPadding in
settings_profile_item_style.dart and the margin property in
settings_profile_item.dart to locate and update the offending lines.
In
`@lib/pages/settings_dashboard/settings_profile/settings_profile_view_mobile.dart`:
- Around line 158-170: The Container build uses invalid shorthand constructors
(e.g., "const .only", "const .all", "const .circular") — update these to the
correct Dart classes: replace margin: const .only(...) with margin: const
EdgeInsets.only(...), padding: const .all(...) with padding: const
EdgeInsets.all(...), and borderRadius: const .all(.circular(8)) with
borderRadius: BorderRadius.circular(8) or const
BorderRadius.all(Radius.circular(8)); adjust any other similar shorthand
occurrences in the Container/BoxDecoration block (look for the Container widget,
its margin/padding properties, and the BoxDecoration.borderRadius) so the file
compiles.
- Around line 339-341: The padding declaration uses an invalid shorthand;
replace the incorrect "const .symmetric(horizontal: 12)" with the proper
constructor "const EdgeInsets.symmetric(horizontal: 12)" so the widget's padding
is created correctly; update the code around the padding property (the widget
that contains "padding:" in settings_profile_view_mobile.dart) to call
EdgeInsets.symmetric instead of the ".symmetric" shorthand.
In
`@lib/pages/settings_dashboard/settings_profile/settings_profile_view_web.dart`:
- Around line 203-214: The Size.fromRadius constructor is referenced with an
invalid shorthand; in the widget returned for GetAvatarOnWebUIStateSuccess
replace the incorrect `const .fromRadius(...)` with the correct `const
Size.fromRadius(...)` where the SizedBox is created (the block that instantiates
SizedBox.fromSize using SettingsProfileViewWebStyle.radiusImageMemory and passes
matrixFile to StreamImageViewer with onImageLoaded).
- Around line 229-232: The BorderRadius shorthand ".circular" is invalid in the
RoundedRectangleBorder declaration; replace the incorrect
".circular(AvatarStyle.defaultSize)" with a proper BorderRadius constructor call
(e.g., BorderRadius.circular(AvatarStyle.defaultSize) or
BorderRadius.all(Radius.circular(AvatarStyle.defaultSize))) inside the shape
assigned to RoundedRectangleBorder so that the borderRadius parameter receives a
valid BorderRadius object for the widget using AvatarStyle.defaultSize.
- Around line 280-304: The EditMenuBtn code uses invalid shorthand (e.g.,
".circular" and ".all") for constructors; update the Widget tree in the builder
(where PopupMenuWidgetStyle.menuBorderRadius,
SettingsProfileViewWebStyle.avatarSize and
SettingsProfileViewWebStyle.iconEditBorderWidth are used) to call the proper
constructors: replace ".circular(PopupMenuWidgetStyle.menuBorderRadius)" with
"BorderRadius.circular(PopupMenuWidgetStyle.menuBorderRadius)" and replace
".circular(SettingsProfileViewWebStyle.avatarSize)" with
"BorderRadius.circular(SettingsProfileViewWebStyle.avatarSize)", and replace
".all(color: ..., width: ...)" with "Border.all(color: colorScheme.onPrimary,
width: SettingsProfileViewWebStyle.iconEditBorderWidth)" so valid Flutter
constructors are used.
- Around line 100-131: The code uses invalid Dart shorthand like .antiAlias,
.circular, .min and .start which are not resolved; update these to their full
enum/class-qualified forms: change clipBehavior: .antiAlias to clipBehavior:
Clip.antiAlias, change RoundedRectangleBorder(borderRadius: .circular(...)) to
RoundedRectangleBorder(borderRadius: BorderRadius.circular(...)), and change
Column(mainAxisSize: .min, crossAxisAlignment: .start) to Column(mainAxisSize:
MainAxisSize.min, crossAxisAlignment: CrossAxisAlignment.start) so the
Container, clipBehavior, RoundedRectangleBorder, borderRadius, and Column
properties (mainAxisSize, crossAxisAlignment) use the correct identifiers.
In `@lib/pages/settings_dashboard/settings_profile/settings_profile_view.dart`:
- Around line 73-77: The InkWell's borderRadius property is using the shorthand
`.circular(...)` without the class prefix; update the borderRadius assignment in
the InkWell (the widget using SettingsProfileViewStyle.borderRadius) to use the
proper class method by calling
BorderRadius.circular(SettingsProfileViewStyle.borderRadius) so the code
compiles and references the correct BorderRadius API.
In `@lib/pages/settings_dashboard/settings_security/settings_security_view.dart`:
- Line 144: Replace the invalid string multiplication used for the subtitle (the
expression '\u2022' * 32) with a generated string made by creating a list of 32
bullet characters and joining them; update the widget property named subtitle in
settings_security_view.dart (the line using '\u2022' * 32) to build the repeated
bullet string via List.filled and join instead of using the non-existent string
multiplication operator.
---
Outside diff comments:
In `@lib/pages/settings_dashboard/settings_3pid/settings_3pid.dart`:
- Around line 20-62: In add3PidAction (and similarly in delete3Pid) guard all
post-await uses of the BuildContext and setState by checking the State's mounted
flag: after each await that is followed by Matrix.of(context) or setState, add
if (!mounted) return; and, where you need Matrix.of(context) across multiple
lines, capture Matrix.of(context).client into a local variable only after
verifying mounted to avoid using a disposed context; likewise ensure setState is
only called after a mounted check.
In
`@lib/pages/settings_dashboard/settings_contacts_visibility/settings_contacts_visibility_view.dart`:
- Around line 222-243: The widget contains invalid shorthand tokens (e.g.,
"const .all(16)", "overflow: .ellipsis", "textAlign: .center", "const
.only(left: 8, right: 16)") in _buildVisibilityOptionItem; replace these with
the proper Flutter identifiers (EdgeInsets.all(16), TextOverflow.ellipsis,
TextAlign.center, EdgeInsets.only(...)) so the Padding and Text properties
compile correctly, and update any other similar occurrences in the same method
(look for option.title, sysColor, enableDivider and Divider usage) to use the
full names.
- Around line 186-200: In _buildVisibilityOptionBorderRadius (using
SettingsContactsVisibilityEnum), replace the shorthand class invocations with
fully qualified constructors: return const BorderRadius.only(...) and use
Radius.circular(...) inside those calls (e.g., const BorderRadius.only(topLeft:
Radius.circular(16), topRight: Radius.circular(16)) and const
BorderRadius.only(bottomLeft: Radius.circular(16), bottomRight:
Radius.circular(16))); keep the enum switch cases (e.g., .public, .contacts,
.private) as-is and return null for the .contacts branch.
- Around line 295-330: In _buildVisibleFieldItem, replace the invalid shorthand
tokens with the proper Flutter enums and constructors: change "const .all(16)"
and "const .only(...)" to "const EdgeInsets.all(...)" and "const
EdgeInsets.only(...)", change "crossAxisAlignment: .start" to
"crossAxisAlignment: CrossAxisAlignment.start", change "overflow: .ellipsis" to
"overflow: TextOverflow.ellipsis", and change "textAlign: .center" / ".left" to
"textAlign: TextAlign.center" / "TextAlign.left" so the Padding, Column, and
Text widgets use correct types and constants.
In
`@lib/pages/settings_dashboard/settings_multiple_emotes/settings_multiple_emotes_view.dart`:
- Around line 33-35: URL-encode pack keys before using them in route
interpolation: wrap occurrences of keys[i] (and any map keys like the
empty-string entry in packs) with Uri.encodeComponent(...) when building the
navigation path (the code that constructs
"rooms/{roomId}/details/emotes/{packId}" around the navigation call at the
locations referenced). This prevents slashes, ?, #, etc. from breaking routing
and ensures the packId segment is safely encoded; apply the same encoding where
you create/lookup packs[''] and in the navigation call(s) around lines 56-57.
In
`@lib/pages/settings_dashboard/settings_profile/settings_profile_view_mobile.dart`:
- Around line 127-136: The snippet uses shorthand constructors `.circular(...)`
and `.all(...)` which are invalid in Dart; update the BorderRadius and Border
usages to their full named constructors — replace
`.circular(SettingsProfileViewMobileStyle.avatarSize)` with
`BorderRadius.circular(SettingsProfileViewMobileStyle.avatarSize)` and replace
`.all(color: colorScheme.onPrimary, width:
SettingsProfileViewMobileStyle.iconEditBorderWidth)` with `Border.all(color:
colorScheme.onPrimary, width:
SettingsProfileViewMobileStyle.iconEditBorderWidth)` so the compiler recognizes
the constructors.
In `@lib/pages/settings_dashboard/settings/settings.dart`:
- Around line 79-90: The code force-unwraps TwakeApp.routerKey.currentContext
(twakeContext) when calling showConfirmAlertDialog in logoutAction, which can
crash; update logoutAction to check twakeContext for null and bail out (or fall
back to a safe context like the current method's context) before calling
showConfirmAlertDialog, e.g., if twakeContext == null return (or use context) so
showConfirmAlertDialog is only invoked with a non-null context.
- Around line 262-265: The switch handling for SettingEnum.about currently calls
PlatformInfos.showAboutDialogFullScreen() but falls through into
SettingEnum.logout; add an explicit control transfer (e.g., a break or return)
immediately after the call to PlatformInfos.showAboutDialogFullScreen() to
prevent fall-through into logoutAction(); ensure the change is made in the
switch that contains SettingEnum.about and SettingEnum.logout so logoutAction()
only runs for the logout case.
---
Major comments:
In `@assets/l10n/intl_en.arb`:
- Around line 1791-1794: Add the two new keys "recoveryKeyWarningMessage" and
"recoveryKeyCopiedToClipboard" (and their companion metadata entries
"@recoveryKeyWarningMessage" and "@recoveryKeyCopiedToClipboard") to every
non-English ARB file so users see localized text; translate the English strings
into each supported language (or add translator notes/placeholders if you need a
follow-up), keep the key names identical, mirror the metadata objects as in the
English ARB, and ensure any punctuation/placeholder formatting matches existing
locale entries.
In `@lib/pages/settings_dashboard/settings_3pid/settings_3pid_view.dart`:
- Around line 38-41: The UI currently renders raw exceptions from the
FutureBuilder snapshot (the branch that returns Center(child:
Text(snapshot.error.toString(), ...))) which leaks internal details; instead,
log the exception (use the app logger or error reporting used elsewhere) and
replace the Text(snapshot.error.toString()) with a generic, localized error
widget/message (e.g., a localized string like S.of(context).genericError or a
SettingsErrorState widget) while keeping the same layout (Center). Locate the
FutureBuilder/error-handling code in settings_3pid_view.dart (the
snapshot.hasError branch) and change it to log snapshot.error and show the
localized/generic error text to the user.
In `@lib/pages/settings_dashboard/settings_emotes/settings_emotes_view.dart`:
- Around line 125-126: A TextEditingController is being created inside
itemBuilder (TextEditingController and textEditingController.text = imageCode)
causing leaked controllers; move controller lifecycle out of the build loop by
creating and caching controllers keyed by imageCode (e.g., Map<String,
TextEditingController>) at the parent widget state and reuse them in
itemBuilder, and ensure all controllers are disposed in the parent
State.dispose(); alternatively, wrap each list item in a StatefulWidget that
instantiates a TextEditingController for that item and disposes it in its own
dispose() method.
In
`@lib/pages/settings_dashboard/settings_multiple_emotes/settings_multiple_emotes_view.dart`:
- Line 17: The direct double-null-assertion on controller.roomId and getRoomById
causes a crash if either is null; change the lookup in
settings_multiple_emotes_view.dart to guard both values (check controller.roomId
!= null, then call Matrix.of(context).client.getRoomById(controller.roomId!) and
handle a null room) and return an appropriate fallback (e.g., early return
widget, placeholder, or error UI) instead of using the two ! operators; update
any downstream uses of room to account for the guarded/nullable value (or store
a non-null local only after the guard) so you never crash on missing roomId or
missing cached room.
In `@lib/pages/settings_dashboard/settings_security/settings_security_view.dart`:
- Around line 131-145: The FutureBuilder currently triggers fetching the
plaintext recovery key (controller.recoveryKeyFuture) as soon as the screen
builds; change this to only perform a lightweight existence check for rendering
the masked row and defer fetching/copying the real secret until after the user
confirms in the warning dialog. Concretely: replace or wrap the current
FutureBuilder usage around controller.recoveryKeyFuture with a fast
existence-check future (or a boolean controller.hasRecoveryKey check) to decide
whether to show the masked SettingsItemBuilder row, and move the actual call to
fetch the plaintext key into the confirmation flow (the handler that opens the
warning dialog and performs the clipboard write). If you keep using a future for
the real key, ensure it is cached on the controller (e.g.,
controller.getRecoveryKeyCached()) so rebuilds do not refetch the secret;
reference controller.recoveryKeyFuture, the FutureBuilder, and the
SettingsItemBuilder when making these changes.
In `@lib/pages/settings_dashboard/settings_security/settings_security.dart`:
- Around line 148-156: The validator in DialogTextField currently force-unwraps
int.tryParse(text) which throws on non-digit input; update the validator in the
DialogTextField (the validator closure) to avoid the bang: first check text !=
null and text.length == 4, then call int.tryParse(text) and test that the result
is non-null and >= 0 (e.g., final value = int.tryParse(text); if (value != null
&& value >= 0) return null;), otherwise return l10n.pleaseEnter4Digits; ensure
no `!` is used on tryParse to prevent null assertion errors.
- Around line 96-121: In changePasswordAccountAction, the dialog collects two
password fields but passes them reversed to client.changePassword and never
validates they match; update showTextInputDialog (in
changePasswordAccountAction) to collect three fields (currentPassword,
newPassword, confirmPassword) or at minimum collect newPassword and
confirmPassword and validate equality before calling
TwakeDialog.showFutureLoadingDialogFullScreen; then call
client.changePassword(newPassword, oldPassword: currentPassword) (or if current
password is obtained elsewhere, ensure you pass the correct oldPassword and
validate input.first vs input.last properly) and show a validation error if
confirmation != new password.
In `@lib/pages/settings_dashboard/settings_style/settings_style.dart`:
- Around line 34-37: The setChatColor method short-circuits when color is null,
preventing the "reset" swatch (null entry in customColors) from working; instead
allow null to flow through and clear the stored color and UI state. Modify
setChatColor to handle a null argument by clearing AppConfig.colorSchemeSeed
(set to null or default), calling controller.setPrimaryColor(null) or
controller.resetPrimaryColor(), and updating any persisted state so the null
swatch actually resets the chat color; keep current behavior for non-null values
(AppConfig.colorSchemeSeed = color and controller.setPrimaryColor(color)).
In `@lib/pages/settings_dashboard/settings/settings_item_builder.dart`:
- Around line 99-106: The current logic replaces the default chevron when
trailingWidget is provided; change it so trailingWidget augments rather than
replaces the chevron: in the widget that renders the trailing area (where
trailingWidget, isHideTrailingIcon, Icon(Icons.chevron_right_outlined),
SettingsViewStyle.iconSize, and tertiary30 are referenced) render trailingWidget
first (if not null) and then, unless isHideTrailingIcon is true, render the
chevron Icon after it (e.g., wrap them in a Row or similar), so the chevron
remains visible by default and is only hidden when isHideTrailingIcon is true.
---
Minor comments:
In `@lib/pages/settings_dashboard/settings_security/settings_security.dart`:
- Around line 184-194: After awaiting showOkCancelAlertDialog, guard against the
widget being disposed before reusing context: add a mounted check immediately
after the dialog response (e.g., if (!context.mounted) return;) before calling
TwakeDialog.showFutureLoadingDialogFullScreen or Matrix.of(context) to avoid the
async-gap context hazard; update the block around showOkCancelAlertDialog,
TwakeDialog.showFutureLoadingDialogFullScreen, and Matrix.of(context)
accordingly.
---
Nitpick comments:
In `@lib/pages/chat/events/audio_message/audio_play_extension.dart`:
- Line 6: The TODO is written as a documentation comment which makes it part of
the public API docs for MatrixFileAudioSource; change the leading doc comment
marker from ///TODO(...) to a non-doc single-line comment like // TODO(...) so
the note remains a developer TODO and does not become the class's docstring
(look for the TODO above MatrixFileAudioSource and replace the triple-slash with
double-slash).
In
`@lib/pages/settings_dashboard/settings_blocked_users/settings_blocked_users_view.dart`:
- Around line 134-158: The onTap callback in settings_blocked_users_view uses
hardcoded route strings ('/rooms/draftChat' and '/rooms/$roomId') when
creating/entering chats; update it to use the centralized route
helpers/constants instead: replace the hardcoded draft-chat path passed to
context.go inside Router.neglect with the shared draft chat route
constant/helper (used elsewhere in the app), and replace the '/rooms/$roomId'
usages in both context.push and context.go with the room route builder/helper
that accepts a roomId parameter (while keeping the same logic around
Router.neglect, getDirectChatFromUserId, responsiveUtils.isMobile, and
client.userID checks).
In
`@lib/pages/settings_dashboard/settings_contacts_visibility/settings_contacts_visibility.dart`:
- Around line 169-176: In initialGetUserInfoVisibility, cache the localization
instance like in updateUserInfoVisibility by assigning final l10n =
L10n.of(context)! at the start and replace direct calls to L10n.of(context)!
(e.g., the usage in the failure branch that shows TwakeSnackBar) with l10n;
update all occurrences inside initialGetUserInfoVisibility to use this local
l10n variable for consistency and minor performance gain.
In `@lib/pages/settings_dashboard/settings_emotes/settings_emotes_view.dart`:
- Around line 26-29: The TextStyle construction in the variable textTheme calls
Theme.of(context) again; use the already-cached theme variable instead by
replacing Theme.of(context).colorScheme.secondary with
theme.colorScheme.secondary in the textTheme declaration (variables: theme and
textTheme) so the theme is reused rather than fetched twice.
- Line 127: The condition assigning useShortCuts uses PlatformInfos.isDesktop
which appears to be dead if desktop platforms are unsupported; update the
expression in settings_emotes_view.dart where useShortCuts is defined to remove
the isDesktop check and only use PlatformInfos.isWeb (i.e., change the
assignment of the useShortCuts variable to rely solely on PlatformInfos.isWeb),
and run a quick search for any other usages of PlatformInfos.isDesktop in this
file to remove or adjust related dead branches.
In `@lib/pages/settings_dashboard/settings_stories/settings_stories.dart`:
- Line 86: The build method currently uses an unnamed parameter (Widget
build(_)) which is unconventional; change it to explicitly name the BuildContext
parameter (e.g., Widget build(BuildContext context)) so the method signature is
standard and future uses of context (navigation, theme, localization) are
straightforward; update the method declaration where SettingsStories defines
build (the override of build that returns SettingsStoriesView(this)) to use the
named parameter.
...pages/settings_dashboard/settings_contacts_visibility/settings_contacts_visibility_view.dart
Show resolved
Hide resolved
...pages/settings_dashboard/settings_contacts_visibility/settings_contacts_visibility_view.dart
Show resolved
Hide resolved
...pages/settings_dashboard/settings_contacts_visibility/settings_contacts_visibility_view.dart
Show resolved
Hide resolved
lib/pages/settings_dashboard/settings_profile/settings_profile_item_style.dart
Show resolved
Hide resolved
lib/pages/settings_dashboard/settings_profile/settings_profile_view_mobile.dart
Show resolved
Hide resolved
lib/pages/settings_dashboard/settings_profile/settings_profile_view_web.dart
Show resolved
Hide resolved
lib/pages/settings_dashboard/settings_profile/settings_profile_view_web.dart
Show resolved
Hide resolved
lib/pages/settings_dashboard/settings_profile/settings_profile_view_web.dart
Show resolved
Hide resolved
lib/pages/settings_dashboard/settings_profile/settings_profile_view.dart
Show resolved
Hide resolved
lib/pages/settings_dashboard/settings_security/settings_security_view.dart
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
integration_test/robots/login_robot.dart (1)
191-218:⚠️ Potential issue | 🟠 MajorOverly broad exception handling masks real test failures.
The two
try/catchblocks catch all exceptions without distinguishing between the expected "browser closed" scenario and legitimate failures like element not found, tap timeout, or network errors. This will cause misleading test failures: when a real failure occurs, the test continues and eventually fails at$(ChatList).waitUntilVisible()inlogin_scenario.dart:36with a 120-second timeout, hiding the actual error.More critically, the second
catchblock (lines 213–217) doesn't return or rethrow—it silently swallows the exception, allowing execution to continue normally. If the$.native.tap()at line 214 fails, the error is completely masked, and the test proceeds as if the tap succeeded.Since
loginAndRunis used by all integration tests (pertest_base.dart), this affects the entire test suite's reliability.Narrow the exception handling to verify the browser actually closed and login succeeded before treating the exception as expected:
Suggested approach: verify browser state before assuming success
// tap on Sign in – the browser modal may close immediately after a // successful login, causing Patrol to report an error even though the // tap succeeded. We catch that error and let the flow continue. try { await $.native.tap(getSignInBtn(), appId: getBrowserAppId()); } catch (_) { - // Browser closed after successful SSO login – expected. - return; + // Browser may have closed after successful SSO login. + // Verify by checking if we're back in the app (ChatList visible or login page gone). + if ($(ChatList).exists || !await isSignInPageVisible()) { + return; // Login succeeded, browser closed – expected. + } + rethrow; // Actual failure – propagate for proper error reporting. }Apply the same pattern to the second
catchblock at lines 213–217.🤖 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 in login_robot.dart are too broad and swallow real failures; change them to only treat the "browser closed after successful SSO login" case as expected by (a) catching specific exceptions or by catching all errors but immediately verifying the post-login state (e.g., check that the browser app is no longer running via getBrowserAppId() or that the app reached the post-login screen such as ChatList visibility) before returning, and (b) in the second catch (around the retry tap for getSignInBtn()) either return when the browser-closed condition is confirmed or rethrow the error so genuine tap failures aren’t masked; use CoreRobot.existsOptionalNativeItems/getOKBtnInVerifyCaptchaDialog() and the existing login success check to decide whether to swallow or propagate the exception.
🧹 Nitpick comments (3)
lib/pages/settings_dashboard/settings_security/settings_security_view.dart (2)
181-181: Consider using list spread...[]instead of set spread...{}.While
...{}works (sets are iterable), using...[]is more idiomatic for spreading into a list context likeColumn.children.♻️ Optional: Use list spread for clarity
- if (client.encryption != null) ...{ + if (client.encryption != null) ...[ if (PlatformInfos.isMobile) Column( ... ), Padding( ... ), - }, + ],🤖 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` at line 181, The conditional block using a set spread "if (client.encryption != null) ...{ }" should use a list spread for clarity and idiomatic Flutter UI code; replace the set spread braces with list brackets so the children being added to the Column (or other Widget.children lists) use "...[ ... ]" instead of "...{ ... }" in the settings_security_view.dart around the "if (client.encryption != null)" block (update any occurrences of that pattern to use list spread).
150-156: Consider consistentColorFilterusage.Line 102 uses the shorthand
.mode(...)while this block uses the explicitColorFilter.mode(...). Both are valid, but consistency within the file would improve readability.♻️ Optional: Use shorthand for consistency
leadingWidget: SvgPicture.asset( ImagePaths.icRecoveryKey, - colorFilter: ColorFilter.mode( + colorFilter: .mode( refColorTertiary30 ?? sysColor.onSurface, .srcIn, ), ),🤖 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 150 - 156, The ColorFilter usage is inconsistent: in this SvgPicture.asset block using ImagePaths.icRecoveryKey you call ColorFilter.mode(...) explicitly while elsewhere the shorthand .mode(...) is used; update this block to use the shorthand constructor invocation (i.e., .mode(...)) to match the file style, keeping the same operands refColorTertiary30 ?? sysColor.onSurface and .srcIn so SvgPicture.asset and its color handling remain unchanged.integration_test/tests/setting/settings_recovery_key_test.dart (1)
35-51: These assertions don't prove the recovery key was copied.This still passes if the handler copies any other non-empty secret, e.g. the public key. If the fixture account has a deterministic recovery key, assert equality with that value, or surface it through the test harness, instead of only rejecting the masked placeholder.
🤖 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 35 - 51, The current assertions in settings_recovery_key_test.dart only check clipboardText is non-empty and not the masked bullets, which doesn't prove the real recovery key was copied; modify the test to compare clipboardText against the known deterministic recovery key from the fixture (e.g., assert clipboardText == expectedRecoveryKey) or expose the fixture's recovery key via the test harness (e.g., TestHarness.getRecoveryKey() or a getRecoveryKeyFromFixture() helper) and use that value in the expect call instead of only negating the mask; update the test to use the variable name (clipboardText) and the harness helper (expectedRecoveryKey/getRecoveryKeyFromFixture/TestHarness.getRecoveryKey) so the assertion unequivocally verifies the correct recovery key was copied.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@integration_test/tests/setting/settings_recovery_key_test.dart`:
- Around line 32-33: The test is flaky because the UI triggers copyRecoveryKey()
which shows the snackbar but does not await
TwakeClipboard.instance.copyText(key), so recoveryKeyRobot.getClipboardText()
can read the old/empty clipboard; either update copyRecoveryKey() in
settings_security.dart to await TwakeClipboard.instance.copyText(key) before
showing the snackbar, or change the test to poll/retry
recoveryKeyRobot.getClipboardText() (with a short timeout) until it contains the
expected key, ensuring the clipboard write has completed before asserting.
---
Outside diff comments:
In `@integration_test/robots/login_robot.dart`:
- Around line 191-218: The try/catch blocks around $.native.tap in
login_robot.dart are too broad and swallow real failures; change them to only
treat the "browser closed after successful SSO login" case as expected by (a)
catching specific exceptions or by catching all errors but immediately verifying
the post-login state (e.g., check that the browser app is no longer running via
getBrowserAppId() or that the app reached the post-login screen such as ChatList
visibility) before returning, and (b) in the second catch (around the retry tap
for getSignInBtn()) either return when the browser-closed condition is confirmed
or rethrow the error so genuine tap failures aren’t masked; use
CoreRobot.existsOptionalNativeItems/getOKBtnInVerifyCaptchaDialog() and the
existing login success check to decide whether to swallow or propagate the
exception.
---
Nitpick comments:
In `@integration_test/tests/setting/settings_recovery_key_test.dart`:
- Around line 35-51: The current assertions in settings_recovery_key_test.dart
only check clipboardText is non-empty and not the masked bullets, which doesn't
prove the real recovery key was copied; modify the test to compare clipboardText
against the known deterministic recovery key from the fixture (e.g., assert
clipboardText == expectedRecoveryKey) or expose the fixture's recovery key via
the test harness (e.g., TestHarness.getRecoveryKey() or a
getRecoveryKeyFromFixture() helper) and use that value in the expect call
instead of only negating the mask; update the test to use the variable name
(clipboardText) and the harness helper
(expectedRecoveryKey/getRecoveryKeyFromFixture/TestHarness.getRecoveryKey) so
the assertion unequivocally verifies the correct recovery key was copied.
In `@lib/pages/settings_dashboard/settings_security/settings_security_view.dart`:
- Line 181: The conditional block using a set spread "if (client.encryption !=
null) ...{ }" should use a list spread for clarity and idiomatic Flutter UI
code; replace the set spread braces with list brackets so the children being
added to the Column (or other Widget.children lists) use "...[ ... ]" instead of
"...{ ... }" in the settings_security_view.dart around the "if
(client.encryption != null)" block (update any occurrences of that pattern to
use list spread).
- Around line 150-156: The ColorFilter usage is inconsistent: in this
SvgPicture.asset block using ImagePaths.icRecoveryKey you call
ColorFilter.mode(...) explicitly while elsewhere the shorthand .mode(...) is
used; update this block to use the shorthand constructor invocation (i.e.,
.mode(...)) to match the file style, keeping the same operands
refColorTertiary30 ?? sysColor.onSurface and .srcIn so SvgPicture.asset and its
color handling remain unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 374e74d6-ec33-4527-a50f-bb85c96cedab
📒 Files selected for processing (4)
integration_test/robots/login_robot.dartintegration_test/robots/setting/settings_recovery_key_robot.dartintegration_test/tests/setting/settings_recovery_key_test.dartlib/pages/settings_dashboard/settings_security/settings_security_view.dart
tddang-linagora
requested changes
Mar 13, 2026
Collaborator
tddang-linagora
left a comment
tddang-linagora
left a comment
There was a problem hiding this comment.
Please:
- Add demo for mobile if this feature also involves mobile devices (Android OR iOS)
- Make all the changes necessary for the feature, and ONLY refactor the modified files. The code base is not perfected, but adding too much noise to the PR draws your collaborators time, which they can spend to add real value to the products, not beautify the code base.
hoangdat
reviewed
Mar 13, 2026
Member
There was a problem hiding this comment.
thank you for that, please separate the refactoring for route path in other PR, then the PR for recovery. It is better to review and make it merge
This was referenced Mar 19, 2026
Collaborator
Author
|
This PR has been split into two separate PRs:
Closing this one in favor of the two focused PRs above. |
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
3 participants
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.
Ticket
Related issue: TW-2991
Root cause
N/A — This is a feature, not a bug fix.
Solution
Display the user's recovery key in the Privacy & Security settings screen.
Fetch the recovery key from the ToM server via GetRecoveryWordsInteractor
Display it as a masked field (••••••••) in the security settings list
On tap or copy icon press, show a warning dialog informing the user that this key grants access to all encrypted messages
If confirmed, copy the key to the clipboard via TwakeClipboard
Add new SVG icon (ic_recovery_key.svg) and new i18n strings (recoveryKeyWarningMessage, recoveryKeyCopiedToClipboard)
Refactoring included:
Code hygiene, avoid repetition,
Adds a new entry in Privacy & Security settings allowing users to view and copy their recovery key, with a security warning before clipboard access.
Test recommendations
Verify the recovery key item appears in Settings > Privacy & Security when the user has a recovery key
Verify it is hidden when the recovery key is unavailable (API failure or no key)
Tap the copy icon → confirm the warning dialog appears → confirm the key is copied to clipboard
Dismiss the warning dialog → confirm nothing is copied
Verify the existing settings items (contacts visibility, blocked users, app lock, public key) still render and function correctly
Pre-merge
No.
Resolved
Attach screenshots or videos demonstrating the changes
Web:
Android:
IOS:
Summary by CodeRabbit
New Features
Improvements
Tests