TW-2910: add connection to a matrix server though username password#2911
Open
nqhhdev wants to merge 8 commits intosprint/march26from
Open
TW-2910: add connection to a matrix server though username password#2911nqhhdev wants to merge 8 commits intosprint/march26from
nqhhdev wants to merge 8 commits intosprint/march26from
Conversation
nqhhdev
requested review from
dab246,
hoangdat and
tddang-linagora
as code owners
|
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 ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughThis PR adds password-login UI and flow support: new HomeserverState.passwordLoginMethod, homeserver-picker logic to navigate to the password login path, localized strings ( Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
nqhhdev
force-pushed
the
TW-2910-Add-connection-to-a-Matrix-server-though-username-password
branch
from
e58eaa6 to
f814676
Compare
Contributor
|
This PR has been deployed to https://linagora.github.io/twake-on-matrix/2911 |
Member
|
@CodeRabbit please review |
|
The PR description image shows an Xcode test results window (
✅ Actions performedReview triggered.
|
hoangdat
reviewed
Mar 19, 2026
|
The PR description image shows an Xcode test results window (
✅ Actions performedFull review 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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lib/pages/homeserver_picker/homeserver_picker.dart (1)
190-192:⚠️ Potential issue | 🔴 CriticalRouting mismatch:
/loginroute does not exist.Based on the router configuration in
lib/config/go_routes/go_router.dart, only/home/loginis registered (as a nested route under/home). The route/loginat line 192 is not defined, which would cause a navigation error or undefined behavior.This should be consistent with line 212 which correctly uses
/home/login.🐛 Proposed fix
if (!ssoSupported && matrix.loginRegistrationSupported == false) { // Server does not support SSO or registration. We can skip to login page: - context.push('/login'); + context.push('/home/login'); } else if (ssoSupported && matrix.loginRegistrationSupported == false) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/pages/homeserver_picker/homeserver_picker.dart` around lines 190 - 192, The routing call uses a non-existent '/login' path; update the navigation in the homeserver picker where context.push('/login') is called to use the correct registered route '/home/login' (ensure the same change is applied wherever context.push('/login') appears), locating the call by searching for the context.push invocation in homeserver_picker.dart and the related ssoSupported / matrix.loginRegistrationSupported conditional to keep routing consistent with the router configuration.
🧹 Nitpick comments (5)
lib/pages/homeserver_picker/homeserver_picker_view.dart (1)
133-142: Consider consolidating switch cases that return the same value.The
ssoLoginServer,wrongServerName, andpasswordLoginMethodcases all return the sameText(l10n.continueProcess, ...). This could be simplified.♻️ Optional simplification
switch (controller.state) { case HomeserverState.otherLoginMethod: - return Text(l10n.continueProcess, style: textStyle); - case HomeserverState.loading: - return const Expanded(child: LinearProgressIndicator()); case HomeserverState.ssoLoginServer: case HomeserverState.wrongServerName: case HomeserverState.passwordLoginMethod: return Text(l10n.continueProcess, style: textStyle); + case HomeserverState.loading: + return const Expanded(child: LinearProgressIndicator()); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/pages/homeserver_picker/homeserver_picker_view.dart` around lines 133 - 142, The switch in homeserver_picker_view.dart repeats the same return for HomeserverState.ssoLoginServer, HomeserverState.wrongServerName, and HomeserverState.passwordLoginMethod; consolidate them into a single case group to reduce duplication by grouping the enum values (e.g., case HomeserverState.ssoLoginServer: case HomeserverState.wrongServerName: case HomeserverState.passwordLoginMethod:) and keep the existing return Text(l10n.continueProcess, style: textStyle); leaving other cases (otherLoginMethod and loading) unchanged; update the switch over controller.state accordingly.integration_test/robots/login_robot.dart (2)
211-224: Index-based field selection is fragile.Using
$(TextField).at(0)and$(TextField).at(1)depends on the order of TextField widgets in the widget tree. If the UI changes (e.g., a search field is added, or fields are reordered), these indices would break silently.Consider using more specific selectors if possible, such as finding by widget key or by ancestor widget type. However, I recognize this follows patterns elsewhere in this file.
♻️ Alternative approach (if widget keys are available)
// If LoginView adds keys to its TextFields: Future<void> enterUsername(String username) async { final usernameField = $(TextField).containing('Email or username'); await $.enterText(usernameField, username); } Future<void> enterPassword(String password) async { final passwordField = $(TextField).containing('Password'); await $.enterText(passwordField, password); }Alternatively, add
Keywidgets to the TextFields inLoginViewand select by key.🤖 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 211 - 224, The current enterUsername and enterPassword methods use fragile index-based selectors ($(TextField).at(0) / at(1)) which will break if TextField order changes; update these to use more specific selectors (e.g., select by label/text via $(TextField).containing('Email or username') and $(TextField).containing('Password') or by Key if LoginView adds Keys) in the enterUsername and enterPassword functions, and keep tapSignInButton as-is (or switch its selector to a keyed ElevatedButton if you add Keys); modify LoginView to add unique Keys to the username/password fields if necessary and then update the robots to use those Keys for stable selection.
240-246: Async not needed for synchronous existence checks.
isLoginViewVisible()andisChatListVisible()simply check.existswhich is synchronous. Theasynckeyword andFuture<bool>return type add unnecessary overhead.♻️ Simplify to synchronous methods
- Future<bool> isLoginViewVisible() async { - return $(LoginView).exists; - } + bool isLoginViewVisible() { + return $(LoginView).exists; + } - Future<bool> isChatListVisible() async { - return $(ChatList).exists; - } + bool isChatListVisible() { + return $(ChatList).exists; + }Note: This would require updating the test to remove
awaitbefore these calls.🤖 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 240 - 246, The two methods isLoginViewVisible and isChatListVisible perform synchronous checks using $(LoginView).exists and $(ChatList).exists respectively, so remove the async keyword and change their signatures to return bool (not Future<bool>), returning the .exists result directly; also update any tests or call sites that currently await these methods to call them synchronously (remove await) to match the new return type.lib/pages/login/login_view.dart (1)
163-171: Hardcoded icon color may cause visibility issues in dark mode.The visibility toggle icon uses
Colors.blackwhich won't adapt to theme changes. Consider using a theme-aware color.♻️ Proposed fix
suffixIcon: IconButton( onPressed: controller.toggleShowPassword, icon: Icon( controller.showPassword ? Icons.visibility_off_outlined : Icons.visibility_outlined, - color: Colors.black, + color: Theme.of(context).colorScheme.onSurface, ), ),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/pages/login/login_view.dart` around lines 163 - 171, Replace the hardcoded Colors.black in the suffixIcon's Icon with a theme-aware color so the visibility toggle adapts to dark/light themes; inside the IconButton used in the suffixIcon (the widget using controller.toggleShowPassword and controller.showPassword with Icons.visibility_off_outlined / Icons.visibility_outlined) change the Icon's color to use the current theme (for example Theme.of(context).iconTheme.color or IconTheme.of(context).color) or omit the color to inherit the surrounding IconTheme.integration_test/tests/login/login_with_password_test.dart (1)
31-46: Consider validating environment variables before proceeding.If
USERNAME,PASSWORD, orSERVER_URLare not provided (empty strings from missing--dart-define), the test will proceed and fail with unclear errors. Adding early validation would improve debuggability.♻️ Proposed validation
const username = String.fromEnvironment('USERNAME'); const password = String.fromEnvironment('PASSWORD'); const serverUrl = String.fromEnvironment('SERVER_URL'); + if (username.isEmpty || password.isEmpty || serverUrl.isEmpty) { + fail( + 'Missing required environment variables. ' + 'Ensure USERNAME, PASSWORD, and SERVER_URL are set via --dart-define-from-file.', + ); + } + // Navigate through welcome screen to login page🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@integration_test/tests/login/login_with_password_test.dart` around lines 31 - 46, Add explicit validation for the environment vars username, password, and serverUrl before using them: check that the constants username, password, and serverUrl are non-empty (e.g., using isEmpty) and if any are missing fail fast with a clear message (throw a TestFailure or call fail) so the test stops with a descriptive error; update the setup in login_with_password_test.dart before the welcome/login navigation and before calling loginRobot.loginWithPassword to perform these checks and surface which variable is missing.
🤖 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/login/login.dart`:
- Around line 226-231: Dispose currently cancels controllers and the
_clientLoginStateChangedSubscription but doesn't cancel the pending homeserver
lookup timer _coolDown, allowing _checkWellKnown() to run after teardown; update
dispose() to also cancel the timer (call _coolDown?.cancel() and optionally null
it) before calling super.dispose() so the timer cannot fire and invoke
_checkWellKnown() using a disposed context/Navigator.
- Around line 113-132: This listener should be single-shot: at the top of
_listenClientLoginStateChanged check if _loginCompleter.isCompleted and return
early to ignore duplicate events, and when you accept the event mark the
completer completed immediately (call _loginCompleter.complete()) before
invoking _handleFirstLoggedIn or _handleAddAnotherAccount so the handlers cannot
run twice for the same login; use the existing symbols
_listenClientLoginStateChanged, _loginCompleter, _handleFirstLoggedIn,
_handleAddAnotherAccount and MultipleAccountLoginType to implement this guard.
- Around line 90-97: The catch blocks for MatrixException and the generic
exception call TwakeDialog.hideLoadingDialog(context) and setState(...) without
checking whether the State is still mounted; wrap the UI-touching logic in each
catch (MatrixException and generic) with a mounted guard (e.g., return early if
not mounted) so you don't call hideLoadingDialog(context) or setState when the
widget has been disposed; ensure the mounted check happens before calling
TwakeDialog.hideLoadingDialog and before modifying passwordError/loading via
setState.
---
Outside diff comments:
In `@lib/pages/homeserver_picker/homeserver_picker.dart`:
- Around line 190-192: The routing call uses a non-existent '/login' path;
update the navigation in the homeserver picker where context.push('/login') is
called to use the correct registered route '/home/login' (ensure the same change
is applied wherever context.push('/login') appears), locating the call by
searching for the context.push invocation in homeserver_picker.dart and the
related ssoSupported / matrix.loginRegistrationSupported conditional to keep
routing consistent with the router configuration.
---
Nitpick comments:
In `@integration_test/robots/login_robot.dart`:
- Around line 211-224: The current enterUsername and enterPassword methods use
fragile index-based selectors ($(TextField).at(0) / at(1)) which will break if
TextField order changes; update these to use more specific selectors (e.g.,
select by label/text via $(TextField).containing('Email or username') and
$(TextField).containing('Password') or by Key if LoginView adds Keys) in the
enterUsername and enterPassword functions, and keep tapSignInButton as-is (or
switch its selector to a keyed ElevatedButton if you add Keys); modify LoginView
to add unique Keys to the username/password fields if necessary and then update
the robots to use those Keys for stable selection.
- Around line 240-246: The two methods isLoginViewVisible and isChatListVisible
perform synchronous checks using $(LoginView).exists and $(ChatList).exists
respectively, so remove the async keyword and change their signatures to return
bool (not Future<bool>), returning the .exists result directly; also update any
tests or call sites that currently await these methods to call them
synchronously (remove await) to match the new return type.
In `@integration_test/tests/login/login_with_password_test.dart`:
- Around line 31-46: Add explicit validation for the environment vars username,
password, and serverUrl before using them: check that the constants username,
password, and serverUrl are non-empty (e.g., using isEmpty) and if any are
missing fail fast with a clear message (throw a TestFailure or call fail) so the
test stops with a descriptive error; update the setup in
login_with_password_test.dart before the welcome/login navigation and before
calling loginRobot.loginWithPassword to perform these checks and surface which
variable is missing.
In `@lib/pages/homeserver_picker/homeserver_picker_view.dart`:
- Around line 133-142: The switch in homeserver_picker_view.dart repeats the
same return for HomeserverState.ssoLoginServer, HomeserverState.wrongServerName,
and HomeserverState.passwordLoginMethod; consolidate them into a single case
group to reduce duplication by grouping the enum values (e.g., case
HomeserverState.ssoLoginServer: case HomeserverState.wrongServerName: case
HomeserverState.passwordLoginMethod:) and keep the existing return
Text(l10n.continueProcess, style: textStyle); leaving other cases
(otherLoginMethod and loading) unchanged; update the switch over
controller.state accordingly.
In `@lib/pages/login/login_view.dart`:
- Around line 163-171: Replace the hardcoded Colors.black in the suffixIcon's
Icon with a theme-aware color so the visibility toggle adapts to dark/light
themes; inside the IconButton used in the suffixIcon (the widget using
controller.toggleShowPassword and controller.showPassword with
Icons.visibility_off_outlined / Icons.visibility_outlined) change the Icon's
color to use the current theme (for example Theme.of(context).iconTheme.color or
IconTheme.of(context).color) or omit the color to inherit the surrounding
IconTheme.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4a324672-b6de-4569-af97-28295c1a85d9
📒 Files selected for processing (9)
assets/l10n/intl_en.arbintegration_test/robots/login_robot.dartintegration_test/tests/login/login_with_password_test.dartlib/pages/homeserver_picker/homeserver_picker.dartlib/pages/homeserver_picker/homeserver_picker_view.dartlib/pages/homeserver_picker/homeserver_state.dartlib/pages/login/login.dartlib/pages/login/login_view.dartlib/presentation/mixins/connect_page_mixin.dart
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (4)
lib/pages/login/login.dart (3)
226-231:⚠️ Potential issue | 🟠 MajorCancel
_coolDownindispose().The pending timer can still fire after teardown, and
_checkWellKnown()readscontext/Navigator after async work. Dispose should cancel it with the other async resources.Possible fix
`@override` void dispose() { + _coolDown?.cancel(); usernameController.dispose(); passwordController.dispose(); _clientLoginStateChangedSubscription?.cancel(); super.dispose(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/pages/login/login.dart` around lines 226 - 231, Dispose currently cancels usernameController, passwordController and _clientLoginStateChangedSubscription but does not cancel the pending Timer _coolDown, which can fire after teardown and cause _checkWellKnown() to access context/Navigator; update dispose() to also cancel (and null out) _coolDown (the Timer) alongside the other resources so any scheduled callback is prevented from running after the widget is disposed.
90-97:⚠️ Potential issue | 🟠 MajorGuard the async error paths with
mountedbefore touching UI.After the awaited login call, this state may already be disposed.
TwakeDialog.hideLoadingDialog(context)andsetState(...)can then pop the wrong route or throw after dispose.Possible fix
} on MatrixException catch (exception) { + if (!mounted) return; TwakeDialog.hideLoadingDialog(context); - setState(() => passwordError = exception.errorMessage); - return setState(() => loading = false); + setState(() { + passwordError = exception.errorMessage; + loading = false; + }); + return; } catch (exception) { + if (!mounted) return; TwakeDialog.hideLoadingDialog(context); - setState(() => passwordError = exception.toString()); - return setState(() => loading = false); + setState(() { + passwordError = exception.toString(); + loading = false; + }); + return; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/pages/login/login.dart` around lines 90 - 97, The error handlers in the login flow call TwakeDialog.hideLoadingDialog(context) and setState(...) unconditionally which can touch UI after the State has been disposed; before calling TwakeDialog.hideLoadingDialog or any setState in the catch blocks of the login logic (the MatrixException catch and generic catch), guard with the widget's mounted property and bail out if not mounted (e.g., return early) so you never call hideLoadingDialog(context) or setState on an unmounted State.
113-132:⚠️ Potential issue | 🟠 MajorMake the login-state listener actually single-shot.
_loginCompleteris completed, but never checked before handling the event. If the stream emits twice for the same login, both handlers can dismiss the dialog and route to/roomstwice.Possible fix
- final Completer _loginCompleter = Completer(); + final Completer<void> _loginCompleter = Completer<void>(); void _listenClientLoginStateChanged(ClientLoginStateEvent event) { + if (!mounted || _loginCompleter.isCompleted) return; + Logs().i( 'StreamDialogBuilder::_listenClientLoginStateChanged - ${event.multipleAccountLoginType}', ); + _loginCompleter.complete(); if (event.multipleAccountLoginType == MultipleAccountLoginType.firstLoggedIn) { _handleFirstLoggedIn(event.client); - if (!_loginCompleter.isCompleted) _loginCompleter.complete(); return; } if (event.multipleAccountLoginType == MultipleAccountLoginType.otherAccountLoggedIn) { _handleAddAnotherAccount(event.client); - if (!_loginCompleter.isCompleted) _loginCompleter.complete(); return; } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/pages/login/login.dart` around lines 113 - 132, The listener _listenClientLoginStateChanged currently completes _loginCompleter but still processes events multiple times; make it single-shot by checking _loginCompleter.isCompleted at the top of _listenClientLoginStateChanged (or immediately before any handling) and returning early if already completed, so _handleFirstLoggedIn and _handleAddAnotherAccount are only invoked once for a given login event; ensure you still call _loginCompleter.complete() after the first handled branch (firstLoggedIn or otherAccountLoggedIn).lib/pages/homeserver_picker/homeserver_picker.dart (1)
202-213:⚠️ Potential issue | 🟠 MajorAdd a fallback for non-single SSO provider sets.
This branch now only handles exactly one IdP or password fallback. If
identityProviders()returns[]or several providers andsupportsLogin(context)is false, execution falls through withstatestillloading, so the picker never recovers.Possible fix
if (supportsSso(context) && identitiesProvider?.length == 1) { final result = await ssoLoginAction( context: context, id: identitiesProvider!.single.id!, ); if (result == SsoLoginState.error) { state = HomeserverState.ssoLoginServer; } } else if (supportsLogin(context)) { state = HomeserverState.passwordLoginMethod; context.push('/home/login'); + } else { + state = HomeserverState.otherLoginMethod; + context.push('/connect'); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/pages/homeserver_picker/homeserver_picker.dart` around lines 202 - 213, The current branch handles only the case of exactly one SSO provider or a password login, so when identitiesProvider is empty or has multiple entries and supportsLogin(context) is false the state remains HomeserverState.loading; update the logic in the block using supportsSso(context), identitiesProvider, ssoLoginAction and supportsLogin(context) to set a sensible fallback state (e.g., HomeserverState.ssoProviderPicker or HomeserverState.noAuthMethods) and navigate appropriately if needed; specifically, when supportsSso(context) is true but identitiesProvider?.length != 1, assign a non-loading state so the UI can present the provider list or an error, and ensure the else branch still handles the supportsLogin(path) behavior with state = HomeserverState.passwordLoginMethod and context.push('/home/login').
🤖 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/login/login_with_password_test.dart`:
- Around line 23-26: The test currently force-unwraps and replaces
FlutterError.onError via originalOnError = FlutterError.onError! and never
guarantees restoration; change this to safely capture the current handler
(allowing null) and register a restoration in the test teardown (e.g., using
addTearDown or tearDown) so FlutterError.onError is always set back to the saved
original handler even if the test fails; reference the existing
variables/places: FlutterError.onError and originalOnError and ensure you remove
the force-unwrap (!) and restore the handler in a finally/teardown hook.
- Around line 31-33: After reading username, password, and serverUrl via
String.fromEnvironment in login_with_password_test.dart, add explicit guards
that check for empty or null values for username, password, and serverUrl and
immediately fail the test with a clear message; locate the variables (username,
password, serverUrl) and insert validations right after their declarations
(e.g., throw a StateError or call the test framework's fail with a message like
"Missing required --dart-define: USERNAME/PASSWORD/SERVER_URL") so the test
fails fast on misconfigured --dart-define inputs instead of attempting a login
with empty credentials.
In `@lib/pages/login/login_view.dart`:
- Around line 23-235: The form currently uses a centered Column inside
AutofillGroup that can overflow; wrap the Column (the child of AutofillGroup) in
a SingleChildScrollView (or use LayoutBuilder+SingleChildScrollView) and add
bottom padding equal to MediaQuery.of(context).viewInsets.bottom so the keyboard
inset is respected; keep the AutofillGroup and the existing widgets (TextFields
using controller.usernameController, controller.passwordController,
controller.login(), controller.loading, controller.loginClientFuture, etc.)
intact and ensure the SingleChildScrollView uses physics:
AlwaysScrollableScrollPhysics() and a ConstrainedBox/Intrinsics to preserve
centering on large screens while allowing scrolling on small screens.
---
Duplicate comments:
In `@lib/pages/homeserver_picker/homeserver_picker.dart`:
- Around line 202-213: The current branch handles only the case of exactly one
SSO provider or a password login, so when identitiesProvider is empty or has
multiple entries and supportsLogin(context) is false the state remains
HomeserverState.loading; update the logic in the block using
supportsSso(context), identitiesProvider, ssoLoginAction and
supportsLogin(context) to set a sensible fallback state (e.g.,
HomeserverState.ssoProviderPicker or HomeserverState.noAuthMethods) and navigate
appropriately if needed; specifically, when supportsSso(context) is true but
identitiesProvider?.length != 1, assign a non-loading state so the UI can
present the provider list or an error, and ensure the else branch still handles
the supportsLogin(path) behavior with state =
HomeserverState.passwordLoginMethod and context.push('/home/login').
In `@lib/pages/login/login.dart`:
- Around line 226-231: Dispose currently cancels usernameController,
passwordController and _clientLoginStateChangedSubscription but does not cancel
the pending Timer _coolDown, which can fire after teardown and cause
_checkWellKnown() to access context/Navigator; update dispose() to also cancel
(and null out) _coolDown (the Timer) alongside the other resources so any
scheduled callback is prevented from running after the widget is disposed.
- Around line 90-97: The error handlers in the login flow call
TwakeDialog.hideLoadingDialog(context) and setState(...) unconditionally which
can touch UI after the State has been disposed; before calling
TwakeDialog.hideLoadingDialog or any setState in the catch blocks of the login
logic (the MatrixException catch and generic catch), guard with the widget's
mounted property and bail out if not mounted (e.g., return early) so you never
call hideLoadingDialog(context) or setState on an unmounted State.
- Around line 113-132: The listener _listenClientLoginStateChanged currently
completes _loginCompleter but still processes events multiple times; make it
single-shot by checking _loginCompleter.isCompleted at the top of
_listenClientLoginStateChanged (or immediately before any handling) and
returning early if already completed, so _handleFirstLoggedIn and
_handleAddAnotherAccount are only invoked once for a given login event; ensure
you still call _loginCompleter.complete() after the first handled branch
(firstLoggedIn or otherAccountLoggedIn).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0c0b8297-5f83-4f7a-a1da-cd2348857996
📒 Files selected for processing (9)
assets/l10n/intl_en.arbintegration_test/robots/login_robot.dartintegration_test/tests/login/login_with_password_test.dartlib/pages/homeserver_picker/homeserver_picker.dartlib/pages/homeserver_picker/homeserver_picker_view.dartlib/pages/homeserver_picker/homeserver_state.dartlib/pages/login/login.dartlib/pages/login/login_view.dartlib/presentation/mixins/connect_page_mixin.dart
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
lib/pages/login/login.dart (1)
57-58:⚠️ Potential issue | 🟠 MajorKeep the loading dialog and
loadingflag on the same lifecycle.Line 57 opens a modal, but the exception paths at Lines 90-103 only clear
loading, and Line 106 clearsloadingbefore the login-state listener closes the dialog. A failed login will leave the modal stuck on screen, and the form can be re-enabled before post-login setup finishes. Dismiss the dialog in both catch blocks and let the success path finish from_handleFirstLoggedIn/_handleAddAnotherAccount.Suggested fix
} on MatrixException catch (exception) { if (!mounted) return; + TwakeDialog.hideLoadingDialog(context); setState(() { passwordError = exception.errorMessage; loading = false; }); return; } catch (exception) { if (!mounted) return; + TwakeDialog.hideLoadingDialog(context); setState(() { passwordError = exception.toString(); loading = false; }); return; } - - if (mounted) setState(() => loading = false);Also applies to: 90-106
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/pages/login/login.dart` around lines 57 - 58, The loading modal opened with TwakeDialog.showLoadingDialog(context) is not always dismissed on error; update the error handling in the login flow so both catch blocks explicitly call TwakeDialog.dismissLoadingDialog(context) before or while clearing the loading flag, and remove the premature loading=false set that occurs before the login-state listener finishes; keep the dismissal of the dialog on the success path handled by the existing _handleFirstLoggedIn / _handleAddAnotherAccount flows (do not dismiss there prematurely), so the dialog and the loading boolean share the same lifecycle.
🤖 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/login/login_view.dart`:
- Around line 87-122: The error text below the username field is not associated
with the TextField for screen readers; update the widget tree around the
username TextField and its error display (the InputDecoration and
controller.usernameError usage) to merge semantics so assistive tech reads the
error as part of the field: wrap the TextField and the error Text
(controller.usernameError) in a MergeSemantics (or replace the TextField with a
FormField that manages its own error semantics), and wrap the error Text with a
Semantics widget setting enabled: true and label to the error string so it is
announced in context; ensure you remove any isolated semantics nodes and keep
InputDecoration.errorText behavior consistent with the merged semantics
approach.
---
Duplicate comments:
In `@lib/pages/login/login.dart`:
- Around line 57-58: The loading modal opened with
TwakeDialog.showLoadingDialog(context) is not always dismissed on error; update
the error handling in the login flow so both catch blocks explicitly call
TwakeDialog.dismissLoadingDialog(context) before or while clearing the loading
flag, and remove the premature loading=false set that occurs before the
login-state listener finishes; keep the dismissal of the dialog on the success
path handled by the existing _handleFirstLoggedIn / _handleAddAnotherAccount
flows (do not dismiss there prematurely), so the dialog and the loading boolean
share the same lifecycle.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2960625c-41e6-4c8a-a1b4-2d04aa6ba698
📒 Files selected for processing (3)
integration_test/tests/login/login_with_password_test.dartlib/pages/login/login.dartlib/pages/login/login_view.dart
✅ Files skipped from review due to trivial changes (1)
- integration_test/tests/login/login_with_password_test.dart
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/login/login.dart`:
- Around line 121-137: This handler is firing for unrelated app-level login
events; restrict it to the password-login started by this page by correlating
events: add a field (e.g., _expectedLoginClient or _expectedLoginRequestId) set
when you initiate the password login, then in _listenClientLoginStateChanged
return early unless event.client == _expectedLoginClient (or event.requestId ==
_expectedLoginRequestId); only then complete _loginCompleter and call
_handleFirstLoggedIn/_handleAddAnotherAccount. Apply the same correlation check
to the other listener usage at lines 226-228 so only the matching login attempt
can complete the completer or dismiss the dialog.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0abefa57-d84a-4c26-b3a8-76474463ee5a
📒 Files selected for processing (3)
integration_test/tests/login/login_with_password_test.dartlib/pages/login/login.dartlib/pages/login/login_view.dart
🚧 Files skipped from review as they are similar to previous changes (2)
- integration_test/tests/login/login_with_password_test.dart
- lib/pages/login/login_view.dart
…h username+password
hoangdat
reviewed
Mar 23, 2026
| Logs().i( | ||
| 'StreamDialogBuilder::_listenClientLoginStateChanged - ${event.multipleAccountLoginType}', | ||
| ); | ||
| _loginCompleter.complete(); |
Member
There was a problem hiding this comment.
should clear _pendingLoginClient?
Member
Author
|
Demo add multiple account: Simulator.Screen.Recording.-.iPhone.17.-.2026-03-23.at.12.06.35.mov |
…r though username+password
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lib/pages/login/login.dart (1)
41-113:⚠️ Potential issue | 🟠 MajorKeep the login flow single-flight until the matching success event is handled.
Line 113 drops
loadingback tofalsebefore the stream-driven success path finishes, and there is no separate in-flight guard. A second submit in that window can start another attempt; then one attempt’s error path can clear_pendingLoginClientor dismiss the dialog for the other, and the eventual success event gets ignored.🛠️ Proposed fix
void login() async { + if (_pendingLoginClient != null || loading) return; + if (usernameController.text.isEmpty) { setState(() => usernameError = L10n.of(context)!.pleaseEnterYourUsername); } else { setState(() => usernameError = null); } @@ void _handleFirstLoggedIn(Client client) { if (!mounted) return; + _pendingLoginClient = null; TwakeDialog.hideLoadingDialog(context); TwakeApp.router.go( @@ void _handleAddAnotherAccount(Client client) { if (!mounted) return; + _pendingLoginClient = null; TwakeDialog.hideLoadingDialog(context); TwakeApp.router.go(Also applies to: 145-165
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/pages/login/login.dart` around lines 41 - 113, The login() method currently clears loading (setState(() => loading = false)) at the end, allowing a second concurrent submission; instead make the flow single-flight by: add an early guard in login() that returns if loading is true; keep loading true and retain _pendingLoginClient until the asynchronous, stream-driven success/failure handlers clear them; remove the final setState(() => loading = false) at the end of login() and ensure only the stream success handler and the catch/MatrixException handlers clear _pendingLoginClient, call TwakeDialog.hideLoadingDialog(context), and set loading = false; apply the same change to the analogous block referenced at the other occurrence (the 145-165 section).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@lib/pages/login/login.dart`:
- Around line 41-113: The login() method currently clears loading (setState(()
=> loading = false)) at the end, allowing a second concurrent submission;
instead make the flow single-flight by: add an early guard in login() that
returns if loading is true; keep loading true and retain _pendingLoginClient
until the asynchronous, stream-driven success/failure handlers clear them;
remove the final setState(() => loading = false) at the end of login() and
ensure only the stream success handler and the catch/MatrixException handlers
clear _pendingLoginClient, call TwakeDialog.hideLoadingDialog(context), and set
loading = false; apply the same change to the analogous block referenced at the
other occurrence (the 145-165 section).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d4189000-b159-424a-ba2b-b9c5b0f8f088
📒 Files selected for processing (2)
integration_test/tests/login/login_with_password_test.dartlib/pages/login/login.dart
…x server though username+password
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
lib/pages/homeserver_picker/homeserver_picker.dart (1)
220-222:⚠️ Potential issue | 🟠 MajorHandle the non-single-IdP fallback explicitly.
When
identityProviders()returns anything other than exactly one provider andsupportsLogin(context)is false, this branch just falls through withstate == HomeserverState.loading, so the picker never recovers. Send those cases to/connect(or reset the state) instead of depending only on the password-login arm.Possible fallback
- } else if (supportsLogin(context)) { - handlePasswordLogin(); - } + } else if (supportsLogin(context)) { + handlePasswordLogin(); + } else { + state = HomeserverState.otherLoginMethod; + context.push('/connect'); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/pages/homeserver_picker/homeserver_picker.dart` around lines 220 - 222, The current branch after checking identityProviders() and supportsLogin(context) leaves the picker stuck in HomeserverState.loading when there is not exactly one IdP and supportsLogin returns false; update the logic in the homeserver picker (the block that calls identityProviders(), supportsLogin(context) and handlePasswordLogin()) to explicitly handle the non-single-IdP fallback by either navigating to the connect flow (e.g. pushRoute to "/connect") or resetting the homeserver state (set state from HomeserverState.loading to an appropriate value) so the UI recovers instead of remaining stuck; modify the branch that currently only calls handlePasswordLogin() to include an else branch that performs this redirect or state reset.
🤖 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/homeserver_picker/homeserver_picker.dart`:
- Around line 237-244: The branch that handles password-only homeservers
currently calls context.push('/login') which is unreachable with the new routes;
instead reuse the existing helper handlePasswordLogin to perform the correct
route selection (it already pushes '/home/login' or
'/rooms/addaccount/homeserverpicker/login' based on widget.arg.type). Locate the
earlier condition that checks !ssoSupported && matrix.loginRegistrationSupported
== false and replace the direct context.push('/login') call with a call to
handlePasswordLogin (or extract the routing logic from handlePasswordLogin into
a shared private method and invoke it from both places) so both code paths use
the same routing logic.
---
Duplicate comments:
In `@lib/pages/homeserver_picker/homeserver_picker.dart`:
- Around line 220-222: The current branch after checking identityProviders() and
supportsLogin(context) leaves the picker stuck in HomeserverState.loading when
there is not exactly one IdP and supportsLogin returns false; update the logic
in the homeserver picker (the block that calls identityProviders(),
supportsLogin(context) and handlePasswordLogin()) to explicitly handle the
non-single-IdP fallback by either navigating to the connect flow (e.g. pushRoute
to "/connect") or resetting the homeserver state (set state from
HomeserverState.loading to an appropriate value) so the UI recovers instead of
remaining stuck; modify the branch that currently only calls
handlePasswordLogin() to include an else branch that performs this redirect or
state reset.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 7b203282-a9cc-4ebf-a6e4-301f81f9ebfb
📒 Files selected for processing (4)
integration_test/tests/login/login_with_password_test.dartlib/config/go_routes/go_router.dartlib/pages/homeserver_picker/homeserver_picker.dartlib/pages/login/login.dart
✅ Files skipped from review due to trivial changes (1)
- integration_test/tests/login/login_with_password_test.dart
…a Matrix server though username+password
There was a problem hiding this comment.
♻️ Duplicate comments (1)
lib/pages/homeserver_picker/homeserver_picker.dart (1)
219-220:⚠️ Potential issue | 🟠 MajorAdd a fallback when multiple IdPs exist and password login is unavailable.
At Line 219, when
identitiesProvider?.length != 1andsupportsLogin(context)is false, this branch performs no transition and can leave the view stuck inHomeserverState.loading.Suggested fix
- } else if (supportsLogin(context)) { - handlePasswordLogin(); - } + } else if (supportsLogin(context)) { + handlePasswordLogin(); + } else { + state = HomeserverState.otherLoginMethod; + context.push('/connect'); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/pages/homeserver_picker/homeserver_picker.dart` around lines 219 - 220, When identitiesProvider?.length != 1 and supportsLogin(context) is false the flow never transitions and the view can remain in HomeserverState.loading; update the branch that currently calls handlePasswordLogin() to add a fallback that handles multiple IdPs by invoking the IdP selection flow (e.g., call the existing method that shows the IdP picker or set the homeserver state to the IdP chooser) so the UI leaves loading and presents the identity providers instead of stalling; specifically modify the branch around supportsLogin(context) / handlePasswordLogin() to call the IdP picker handler (or set HomeserverState.chooseIdp) when supportsLogin(context) is false and identitiesProvider?.length > 1.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@lib/pages/homeserver_picker/homeserver_picker.dart`:
- Around line 219-220: When identitiesProvider?.length != 1 and
supportsLogin(context) is false the flow never transitions and the view can
remain in HomeserverState.loading; update the branch that currently calls
handlePasswordLogin() to add a fallback that handles multiple IdPs by invoking
the IdP selection flow (e.g., call the existing method that shows the IdP picker
or set the homeserver state to the IdP chooser) so the UI leaves loading and
presents the identity providers instead of stalling; specifically modify the
branch around supportsLogin(context) / handlePasswordLogin() to call the IdP
picker handler (or set HomeserverState.chooseIdp) when supportsLogin(context) is
false and identitiesProvider?.length > 1.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e6cfc5c3-65e6-4d08-9488-9fd7f1cd4bea
📒 Files selected for processing (4)
integration_test/tests/login/login_with_password_test.dartlib/config/go_routes/go_router.dartlib/pages/homeserver_picker/homeserver_picker.dartlib/pages/login/login.dart
🚧 Files skipped from review as they are similar to previous changes (1)
- lib/pages/login/login.dart
…ion to a Matrix server though username+password
tddang-linagora
approved these changes
Mar 23, 2026
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
Resolved
Attach screenshots or videos demonstrating the changes
Simulator.Screen.Recording.-.iPhone.17.-.2026-03-12.at.16.43.12.mov
E2E:
Summary by CodeRabbit
New Features
Improvements
Bug Fixes
Tests