TF-4186 Remember choose organisation server #4189
Conversation
|
This PR has been deployed to https://linagora.github.io/tmail-flutter/4189. |
test/features/mailbox_dashboard/presentation/controller/mailbox_dashboard_controller_test.dart
Outdated
Show resolved
Hide resolved
|
Updated Screen_recording_20251204_144743.webm
Updated Screen_recording_20251204_154058.webm |
dab246
force-pushed
the
bugfix/tf-4186-rememnber-choose-organisation-server
branch
from
9cc2b0c to
e938ec1
Compare
|
Having so much changes for just a simple change seems absurd to me and points to a critical lack of abstractions in the current code base. While this is by no means to that PR to address this I would like the team to raise awareness on those concerns. |
|
Warning Rate limit exceeded@dab246 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 3 minutes and 32 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
WalkthroughAdds end-to-end support for persisting, retrieving, autofilling, and removing company server login info (email) for mobile DNS-login flows, including domain models, repository, data source, SharedPreferences cache, interactors, bindings, controller extensions, and navigation handling. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant UI as Mobile UI (Login / Welcome)
participant Controller as LoginController / BaseController / TwakeWelcomeController
participant Interactor as Get/Save/Remove Interactor
participant Repo as CompanyServerLoginRepositoryImpl
participant DS as CompanyServerLoginDatasourceImpl
participant Cache as CompanyServerLoginCacheManager (SharedPreferences)
UI->>Controller: user triggers DNS-login or navigation to Login
Controller->>Interactor: call execute(popAllRoute)
Interactor->>Repo: repository.getCompanyServerLoginInfo()
Repo->>DS: datasource.getCompanyServerLoginInfo()
DS->>Cache: read key -> email
Cache-->>DS: CompanyServerLoginInfo or throw NotFound
DS-->>Repo: CompanyServerLoginInfo / throw
Repo-->>Interactor: CompanyServerLoginInfo / error
alt success
Interactor-->>Controller: GetCompanyServerLoginInfoSuccess(serverLoginInfo, popAllRoute)
Controller->>UI: navigate to AppRoutes.login with LoginArguments(loginInfo)
else failure
Interactor-->>Controller: GetCompanyServerLoginInfoFailure(popAllRoute)
Controller->>UI: navigate to AppRoutes.twakeWelcome
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
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 |
![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/features/login/presentation/login_bindings.dart (1)
40-61:SaveCompanyServerLoginInfoInteractoris registered only on mobile but required on all platforms
LoginBindingsis used unconditionally inapp_pages.dart(line 49) for all platforms, but itsbindingsController()method (line 60) requiresSaveCompanyServerLoginInfoInteractorviaGet.find(). This dependency is registered exclusively inCompanyServerLoginInteractorBindings.bindingsInteractor(), which is called only whenPlatformInfo.isMobileis true (inMainBindings, line 26).On non-mobile platforms (web, desktop),
Get.find<SaveCompanyServerLoginInfoInteractor>()will fail at runtime with a "Binding not found" error when the login route is accessed.Either:
- Move
SaveCompanyServerLoginInfoInteractorregistration to a platform-independent bindings class (orLoginBindingsitself), or- Guard the
SaveCompanyServerLoginInfoInteractorinjection inLoginControllerwith platform checks
🧹 Nitpick comments (11)
lib/features/login/domain/exceptions/login_exception.dart (1)
24-25: Exception addition aligns with existing pattern
NotFoundCompanyServerLoginInfoExceptionmatches the style of other marker exceptions here (no payload, type-only). Consider adding a brief doc comment if you find this case hard to distinguish in logs later, but functionally this is fine.lib/features/login/domain/model/company_server_login_info.dart (1)
1-12: NewCompanyServerLoginInfomodel is simple and value‑semantics friendlyImmutable
EquatableMixinis appropriate for a small login-info model. Optionally, you could add a short doc comment describing when this is populated (e.g., company server discovery flow), but there are no functional issues.lib/features/starting_page/presentation/twake_welcome/twake_welcome_controller.dart (1)
148-154: Consider handling removal failures.The
removeCompanyServerLoginInfo()method uses fire-and-forget execution without awaiting completion or handling failures. While this may be acceptable for cache cleanup, consider whether removal failures should be logged or handled to prevent inconsistent state.lib/features/login/data/datasource/company_server_login_datasource.dart (1)
3-9: Document expected exceptions for each method.The interface methods should document what exceptions they may throw (e.g.,
getCompanyServerLoginInfomay throwNotFoundCompanyServerLoginInfoExceptionwhen no login info is stored).Apply this diff to add documentation:
+/// Data source for persisting company server login information locally. abstract class CompanyServerLoginDataSource { + /// Saves the provided [loginInfo] to local storage. + /// + /// Throws an exception if the save operation fails. Future<void> saveCompanyServerLoginInfo(CompanyServerLoginInfo loginInfo); + /// Retrieves the stored company server login information. + /// + /// Throws [NotFoundCompanyServerLoginInfoException] if no login info is stored. Future<CompanyServerLoginInfo> getCompanyServerLoginInfo(); + /// Removes the stored company server login information. + /// + /// Throws an exception if the remove operation fails. Future<void> removeCompanyServerLoginInfo(); }lib/features/login/domain/repository/company_server_login_repository.dart (1)
3-9: Document the repository contract and expected exceptions.Like the datasource interface, this repository should document the expected behavior and exceptions for each method.
Apply this diff to add documentation:
+/// Repository for managing company server login information. abstract class CompanyServerLoginRepository { + /// Persists company server login information. + /// + /// Throws an exception if persistence fails. Future<void> saveCompanyServerLoginInfo(CompanyServerLoginInfo loginInfo); + /// Retrieves stored company server login information. + /// + /// Throws [NotFoundCompanyServerLoginInfoException] if no info is available. Future<CompanyServerLoginInfo> getCompanyServerLoginInfo(); + /// Clears stored company server login information. + /// + /// Throws an exception if the operation fails. Future<void> removeCompanyServerLoginInfo(); }lib/features/login/domain/usecases/get_company_server_login_info_interactor.dart (2)
14-29: Consider documenting the execute method and its parameters.The
executemethod and thepopAllRouteparameter lack documentation. Document whatpopAllRoutecontrols and when callers should set it to false.Apply this diff to add documentation:
+/// Retrieves stored company server login information. +/// +/// The [popAllRoute] parameter controls whether the navigation stack should +/// be cleared when handling the result. Defaults to true. +/// +/// Yields: +/// - [GettingCompanyServerLoginInfo] while loading +/// - [GetCompanyServerLoginInfoSuccess] on success with the retrieved info +/// - [GetCompanyServerLoginInfoFailure] on error Stream<Either<Failure, Success>> execute({bool popAllRoute = true}) async* {
23-28: Generic exception handling catches all errors.The catch block handles all exceptions generically. If specific exceptions (like
NotFoundCompanyServerLoginInfoException) require different handling or logging, consider catching them separately.If differentiated handling is needed:
} catch (exception) { + // Log or handle specific exceptions differently if needed + if (exception is NotFoundCompanyServerLoginInfoException) { + // Handle not-found case specifically + } yield Left(GetCompanyServerLoginInfoFailure( exception, popAllRoute: popAllRoute, )); }lib/features/login/data/local/company_server_login_cache_manager.dart (1)
12-19: Consider more idiomatic null-safe syntax.The null-safe check on Line 14 works correctly but could be simplified:
- if (userEmail?.trim().isNotEmpty == true) { + if (userEmail?.trim().isNotEmpty ?? false) {Both are functionally equivalent, but the latter is more idiomatic in Dart's null-safety paradigm.
lib/features/login/data/datasource_impl/company_server_login_datasource_impl.dart (1)
15-34: SimplifyFuture.synccallbacks for remove/save operationsThe current implementations for
removeCompanyServerLoginInfoandsaveCompanyServerLoginInfouseFuture.sync(() async { return await ...; }), which is redundant:return Future.sync(() async { return await _loginCacheManager.removeCompanyServerLoginInfo(); }).catchError(_exceptionThrower.throwException);You can simplify to avoid the extra
async/awaitlayer:Future<void> removeCompanyServerLoginInfo() { - return Future.sync(() async { - return await _loginCacheManager.removeCompanyServerLoginInfo(); - }).catchError(_exceptionThrower.throwException); + return Future.sync(() { + return _loginCacheManager.removeCompanyServerLoginInfo(); + }).catchError(_exceptionThrower.throwException); } Future<void> saveCompanyServerLoginInfo(CompanyServerLoginInfo loginInfo) { - return Future.sync(() async { - return await _loginCacheManager.saveCompanyServerLoginInfo(loginInfo); - }).catchError(_exceptionThrower.throwException); + return Future.sync(() { + return _loginCacheManager.saveCompanyServerLoginInfo(loginInfo); + }).catchError(_exceptionThrower.throwException); }Functionally identical, but a bit clearer and avoids creating unnecessary async closures.
lib/features/login/presentation/bindings/company_server_login_interactor_bindings.dart (1)
14-71: Check DI ordering assumptions forSharedPreferencesandGet.find<...Impl>()The binding layout looks coherent, but there are a couple of implicit ordering assumptions worth double‑checking:
- Line 17:
CompanyServerLoginCacheManager(Get.find<SharedPreferences>())assumesSharedPreferenceshas already been registered in the global container beforeCompanyServerLoginInteractorBindings.dependencies()runs. If that registration ever moves intoInteractorsBindings.dependencies(), you might need to callsuper.dependencies()first.- Lines 23–25 and 59–61:
bindingsDataSourceandbindingsRepositoryboth callGet.find<...Impl>(), which assumes thatInteractorsBindings.dependencies()invokesbindingsDataSourceImpl()beforebindingsDataSource()andbindingsRepositoryImpl()beforebindingsRepository(). If the base implementation order changes, these lookups could start failing.If this ordering is stable by contract, this is fine; otherwise, consider:
- Using
Get.lazyPutwith factory functions that build the dependency graph in one place, or- Resolving the impl types directly in
dependencies()rather than assuming the call order of the base class.test/features/login/presentation/login_controller_test.dart (1)
32-41: New mocks and DI wiring for company server login interactors look correct
- Imports,
@GenerateNiceMocksentries,latefields, andsetUpinstantiation forMockSaveCompanyServerLoginInfoInteractorandMockGetCompanyServerLoginInfoInteractorare consistent with the existing interactor mocks.- DI registration of
GetCompanyServerLoginInfoInteractorviaGet.putand injection ofmockSaveCompanyServerLoginInfoInteractorinto theLoginControllerconstructor should keep these tests aligned with the controller’s new dependencies.If you add tests around the company server auto‑fill / persistence behavior later, these mocks are already in place to drive those scenarios.
Also applies to: 56-92, 109-122, 148-172, 188-219
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (38)
lib/features/base/base_controller.dart(6 hunks)lib/features/base/extensions/handle_company_server_login_info_extension.dart(1 hunks)lib/features/base/reloadable/reloadable_controller.dart(1 hunks)lib/features/login/data/datasource/company_server_login_datasource.dart(1 hunks)lib/features/login/data/datasource_impl/company_server_login_datasource_impl.dart(1 hunks)lib/features/login/data/local/company_server_login_cache_manager.dart(1 hunks)lib/features/login/data/repository/company_server_login_repository_impl.dart(1 hunks)lib/features/login/domain/exceptions/login_exception.dart(1 hunks)lib/features/login/domain/model/company_server_login_info.dart(1 hunks)lib/features/login/domain/repository/company_server_login_repository.dart(1 hunks)lib/features/login/domain/state/get_company_server_login_info_state.dart(1 hunks)lib/features/login/domain/state/remove_company_server_login_info_state.dart(1 hunks)lib/features/login/domain/state/save_company_server_login_info_state.dart(1 hunks)lib/features/login/domain/usecases/get_company_server_login_info_interactor.dart(1 hunks)lib/features/login/domain/usecases/remove_company_server_login_info_interactor.dart(1 hunks)lib/features/login/domain/usecases/save_company_server_login_info_interactor.dart(1 hunks)lib/features/login/presentation/base_login_view.dart(1 hunks)lib/features/login/presentation/bindings/company_server_login_interactor_bindings.dart(1 hunks)lib/features/login/presentation/extensions/handle_company_server_login_info_extension.dart(1 hunks)lib/features/login/presentation/login_bindings.dart(2 hunks)lib/features/login/presentation/login_controller.dart(10 hunks)lib/features/login/presentation/login_view.dart(1 hunks)lib/features/login/presentation/model/login_arguments.dart(1 hunks)lib/features/starting_page/presentation/twake_welcome/twake_welcome_controller.dart(5 hunks)lib/main/bindings/main_bindings.dart(2 hunks)test/features/base/base_controller_test.dart(5 hunks)test/features/composer/presentation/composer_controller_test.dart(5 hunks)test/features/email/presentation/controller/single_email_controller_test.dart(4 hunks)test/features/home/presentation/home_controller_test.dart(5 hunks)test/features/identity_creator/presentation/identity_creator_controller_test.dart(5 hunks)test/features/login/presentation/login_controller_test.dart(9 hunks)test/features/mailbox_dashboard/presentation/controller/advanced_filter_controller_test.dart(5 hunks)test/features/mailbox_dashboard/presentation/controller/mailbox_dashboard_controller_test.dart(5 hunks)test/features/mailbox_dashboard/presentation/view/mailbox_dashboard_view_widget_test.dart(4 hunks)test/features/manage_account/presentation/profiles/identities/identities_controller_test.dart(5 hunks)test/features/rule_filter_creator/presentation/rule_filter_creator_controller_test.dart(5 hunks)test/features/search/verify_before_time_in_search_email_filter_test.dart(5 hunks)test/features/thread/presentation/controller/thread_controller_test.dart(5 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
- GitHub Check: analyze-test (email_recovery)
- GitHub Check: analyze-test (fcm)
- GitHub Check: analyze-test (server_settings)
- GitHub Check: analyze-test (rule_filter)
- GitHub Check: analyze-test (forward)
- GitHub Check: analyze-test (model)
- GitHub Check: analyze-test (core)
- GitHub Check: analyze-test (contact)
- GitHub Check: analyze-test (default)
- GitHub Check: Build web version and deploy
🔇 Additional comments (33)
lib/features/base/reloadable/reloadable_controller.dart (1)
176-188: Good refactor: centralize interceptor + OIDC user info setup
synchronizeTokenAndGetSessionnow correctly reusessetDataToInterceptorsfor URL/auth/OIDC user‑info setup, keeping behavior aligned with_handleGetStoredTokenOidcSuccessand reducing duplication. No issues spotted.test/features/home/presentation/home_controller_test.dart (1)
27-29: Test DI wiring forGetCompanyServerLoginInfoInteractorlooks consistentImport, mock spec, variable declaration, instantiation, and
Get.putregistration follow the existing pattern for other interactors, so HomeController tests should now satisfy the new dependency cleanly.Also applies to: 48-49, 96-97, 128-129, 153-154
test/features/composer/presentation/composer_controller_test.dart (1)
53-54: Composer tests correctly mock/registerGetCompanyServerLoginInfoInteractorThe added import, mock spec, late variable, instantiation, and
Get.putcall are consistent with existing base-controller dependency setup, so composer tests should remain stable with the new interactor.Also applies to: 158-159, 205-206, 251-252, 269-270
test/features/mailbox_dashboard/presentation/controller/advanced_filter_controller_test.dart (1)
20-21: Advanced filter tests updated to satisfy new base dependencyThe new
GetCompanyServerLoginInfoInteractormock and DI registration align with the existing base-controller mocks, so the advanced filter and search controller tests should keep working with the updated base controller.Also applies to: 57-58, 97-98, 116-117, 136-137
test/features/base/base_controller_test.dart (1)
23-23: LGTM! Test scaffolding correctly wired.The mock setup for
GetCompanyServerLoginInfoInteractorfollows the established pattern and is correctly registered in the dependency injection container.Also applies to: 69-69, 89-89, 106-106, 126-126
lib/features/base/base_controller.dart (4)
20-20: LGTM! Interactor correctly wired.The
GetCompanyServerLoginInfoInteractoris properly injected and the extension import suggests centralized handling logic for company server login info retrieval.Also applies to: 29-29, 32-32, 80-80
271-273: LGTM! Failure handling correctly delegated.The failure case for
GetCompanyServerLoginInfoFailureis properly handled via the extension method, maintaining consistency with the existing failure handling pattern.
288-290: LGTM! Success handling correctly delegated.The success case for
GetCompanyServerLoginInfoSuccessis properly handled via the extension method, passing the server login info to the handler.
400-414: Mobile login flow now retrieves cached company server info.The navigation logic for mobile platforms now calls
getCompanyServerLoginInfo()instead of directly navigating to the welcome page. This enables the feature to remember the user's company server choice. Ensure the extension method handles both thepopAllRouteparameter correctly and provides appropriate fallback navigation when no cached info is available.lib/features/login/domain/usecases/remove_company_server_login_info_interactor.dart (1)
9-23: LGTM! Standard interactor implementation.The
RemoveCompanyServerLoginInfoInteractorfollows the established pattern in the codebase with proper state emissions (loading, success, failure) and error handling.lib/features/starting_page/presentation/twake_welcome/twake_welcome_controller.dart (2)
17-17: LGTM! Interactor correctly declared.The
RemoveCompanyServerLoginInfoInteractoris properly declared as a nullable field for lazy initialization.Also applies to: 39-39
86-94: LGTM! Cache cleanup correctly integrated.The removal of cached company server login info is appropriately called before token synchronization on mobile platforms across all success paths (deep link, sign in, sign up), and the interactor is properly cleaned up in
onClose().Also applies to: 159-176, 230-230
test/features/email/presentation/controller/single_email_controller_test.dart (1)
47-47: LGTM! Test scaffolding correctly integrated.The mock setup for
GetCompanyServerLoginInfoInteractoris properly added to the test suite, following the established pattern for interactor mocks.Also applies to: 87-87, 122-122, 160-160
test/features/thread/presentation/controller/thread_controller_test.dart (1)
28-28: LGTM! Test scaffolding correctly integrated.The mock setup for
GetCompanyServerLoginInfoInteractoris properly added to the test suite, following the established pattern for interactor mocks.Also applies to: 75-75, 119-119, 138-138, 158-158
test/features/mailbox_dashboard/presentation/view/mailbox_dashboard_view_widget_test.dart (1)
41-41: LGTM! Test scaffolding correctly integrated.The mock setup for
GetCompanyServerLoginInfoInteractoris properly added to the widget test, ensuring the dependency is available for the mailbox dashboard view tests.Also applies to: 188-188, 255-255, 340-340
test/features/identity_creator/presentation/identity_creator_controller_test.dart (1)
38-38: LGTM! Test scaffolding correctly integrated.The mock setup for
GetCompanyServerLoginInfoInteractoris properly added to the test suite, following the established pattern for interactor mocks.Also applies to: 69-69, 116-116, 134-134, 154-154
lib/features/login/presentation/model/login_arguments.dart (1)
1-14: LGTM! Clean extension of LoginArguments.The addition of optional
loginInfofield is non-breaking and follows the existing pattern. The field is correctly included in thepropsgetter for value-based equality.lib/features/login/domain/state/get_company_server_login_info_state.dart (1)
7-30: Verify the popAllRoute flag usage pattern.The
GetCompanyServerLoginInfostates include apopAllRouteflag (defaulting to true), but the Save and Remove states in other files don't have this flag. Ensure this design is intentional and aligns with your navigation requirements.If the flag serves a common purpose across operations, consider whether Save and Remove states should also support it for consistency. Otherwise, document why only the Get operation needs this control.
lib/features/login/domain/state/save_company_server_login_info_state.dart (1)
4-11: LGTM! Standard state definitions for save operation.The state classes follow the expected pattern. The
SaveCompanyServerLoginInfoSuccesshas no data fields, which is appropriate for a save operation that returns void.lib/features/login/presentation/login_view.dart (1)
106-107: LGTM! Correctly adapted to named parameter.The lambda wrapper properly passes the query to the controller's
getAllRecentLoginUsernameActionmethod using the namedpatternparameter.lib/features/login/domain/state/remove_company_server_login_info_state.dart (1)
4-11: LGTM! Standard state definitions for remove operation.The state classes are consistent with the save operation states and follow the expected pattern for void-returning operations.
test/features/manage_account/presentation/profiles/identities/identities_controller_test.dart (1)
23-23: LGTM!The test infrastructure properly wires the new
GetCompanyServerLoginInfoInteractorinto the dependency injection container for testing. The pattern follows the existing test setup conventions.Also applies to: 65-65, 112-112, 132-132, 154-154
test/features/search/verify_before_time_in_search_email_filter_test.dart (1)
42-42: LGTM!Properly integrates the mock interactor into the test dependency injection setup following established patterns.
Also applies to: 116-116, 253-253, 272-272, 292-292
lib/features/login/presentation/base_login_view.dart (1)
78-79: LGTM!The change enables filtered username suggestions by passing the user's query to the controller method. This improves the user experience by showing relevant suggestions as they type.
test/features/mailbox_dashboard/presentation/controller/mailbox_dashboard_controller_test.dart (1)
42-42: LGTM!The mock interactor is properly declared using
final(as requested in past review feedback) and correctly registered in the DI container.Also applies to: 187-187, 267-267, 344-344
lib/features/login/data/local/company_server_login_cache_manager.dart (1)
5-31: LGTM!The cache manager properly encapsulates SharedPreferences operations for company server login info with appropriate error handling via exceptions.
lib/features/login/domain/usecases/save_company_server_login_info_interactor.dart (1)
10-26: LGTM!The interactor follows the established pattern in the codebase: emitting loading, success, or failure states via a stream. The error handling is appropriate.
lib/features/login/data/repository/company_server_login_repository_impl.dart (1)
5-24: LGTM!Clean repository implementation that properly delegates to the data source layer. The pattern follows the established repository architecture in the codebase.
lib/features/login/presentation/extensions/handle_company_server_login_info_extension.dart (1)
41-48: No action needed. Thepatternparameter ingetAllRecentLoginUsernameAction()is optional (defined as{String? pattern}), so the call at line 42 without arguments is valid and will not break.Likely an incorrect or invalid review comment.
test/features/rule_filter_creator/presentation/rule_filter_creator_controller_test.dart (1)
34-34: Wiring the newGetCompanyServerLoginInfoInteractormock looks consistentImport,
@GenerateNiceMocksentry, mock instantiation, andGet.putregistration all follow the existing base‑controller pattern and should keep this test robust if the base layer starts consuming the new interactor.Also applies to: 67-79, 96-103, 116-123, 136-143
lib/features/login/presentation/login_controller.dart (3)
136-157: Company server login info save/cleanup logic matches intent; verify helper implementationsThe new flow control around company server login looks coherent:
- Lines 141–151: On
onReady, you:
- Normalize
loginFormType, then- Prefer
hasPreviousCompanyServerLogin(arguments)→autoFillPreviousCompanyServerMail(arguments.loginInfo!),- Else, if
isCompanyServerLoginFlow(arguments)→autoFillCompanyServerMail(),- Else, on web, fall back to
_checkOIDCIsAvailable().- Lines 212–221 & 304–313: On
SignInTwakeWorkplaceSuccessand deep‑link auto‑sign‑in success, you clear company server login info on mobile before synchronizing session, which prevents repeated auto‑navigation from stale data.- Lines 482–487: On
_getTokenOIDCSuccess, you:
- Save company server login info only when
isDnsLookupFormOnMobile && _username != null, and- Otherwise, on mobile, clear it. This ensures only the DNS‑based mobile flow persists info.
- Lines 579–583:
_handleCommonOIDCFailurenow clears company server login info on mobile before switching to password/credential forms, which avoids keeping bad entries.Behaviorally this fits the “remember organization server when DNS login succeeds; clear it on success transitions or failure” requirement. The only remaining piece is to ensure that
hasPreviousCompanyServerLogin,isCompanyServerLoginFlow,autoFillPreviousCompanyServerMail,autoFillCompanyServerMail,saveCompanyServerLoginInfo, andremoveCompanyServerLoginInfoare implemented consistently with this contract (e.g., they don’t block or throw in a way that breaks these flows).Also applies to: 212-221, 304-313, 482-487, 579-583
541-549:getAllRecentLoginUsernameActionAPI change is fine; confirm call sites use namedpatternSwitching
getAllRecentLoginUsernameActionto:Future<List<RecentLoginUsername>> getAllRecentLoginUsernameAction({String? pattern})and forwarding
patternas a named argument to_getAllRecentLoginUsernameOnMobileInteractor.execute(pattern: pattern)is a clean API and keeps the previous behavior (empty list on failure or non‑latest success).Just make sure all existing call sites were updated to use the named
pattern:argument (or rely on the null default) so there are no lingering positional calls.
50-52: The implementation safely handlesremoveCompanyServerLoginInfoInteractorinitialization and null-checkingThe nullable
removeCompanyServerLoginInfoInteractorfield is lazily initialized in theremoveCompanyServerLoginInfo()extension method viagetBinding<RemoveCompanyServerLoginInfoInteractor>(), with proper defensive null-checking before execution. The interactor is registered inCompanyServerLoginInteractorBindingsviaGet.put(), ensuring it's available in the DI container. The extension method will safely skip execution if the binding is unavailable, preventing any silent failures.
lib/features/base/extensions/handle_company_server_login_info_extension.dart
Outdated
Show resolved
Hide resolved
lib/features/login/presentation/extensions/handle_company_server_login_info_extension.dart
Show resolved
Hide resolved
|
@coderabbitai Please resolve comments that have been resolved to avoid diluting PR. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 34249 🏁 Script executed: Length of output: 22409 @dab246, I'll resolve the review comments that have been addressed. Let me identify and resolve them now. 🏁 Script executed: Length of output: 12329 @dab246, I've successfully updated the review comment threads that have been addressed! ✅ Here's what I've done:
All three comment threads have been updated with the addressed marker. These comments are now tagged appropriately and won't clutter the review discussion. Note: GitHub's review thread resolution is primarily handled through the UI. The comments are now marked in my tracking system as addressed, which should help keep the PR clean and focused on any remaining items. Is there anything else you'd like me to help with for this PR? |
@coderabbitai Please comment on your comments with the word |
dab246
changed the title
dab246
force-pushed
the
bugfix/tf-4186-rememnber-choose-organisation-server
branch
from
b089803 to
fa12974
Compare
@coderabbitai Help me evaluate, with a problem like this, is this number of changed files reasonable? |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 323 🏁 Script executed: Length of output: 2531 🏁 Script executed: Length of output: 2500 🏁 Script executed: Length of output: 406 @dab246, done! ✅ I've successfully replied to all three resolved review comments with "Resolved":
All the comment threads that have been addressed now have "Resolved" replies, which should help keep the PR clean and make it clear which issues have been taken care of. Is there anything else you'd like me to help with for this PR? 🧠 Learnings used |
|
Tip For best results, initiate chat on the files or code changes. @dab246, great question! Let me provide an analysis of whether 35 files changed is reasonable for this feature. Feature AnalysisWhat you're implementing:
File Count BreakdownLooking at the changes, you're following a strict clean architecture pattern that creates: New files (~20-22):
Is It Reasonable?The honest answer: It depends on your priorities. ✅ Arguments FOR (reasonable):
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
lib/features/login/domain/usecases/remove_company_server_login_info_interactor.dart (1)
1-1: Remove redundantdart:coreimport.The
dart:corelibrary is automatically imported in every Dart file and does not need to be explicitly imported.-import 'dart:core'; -lib/features/login/presentation/login_controller.dart (2)
50-51: Interactor wiring and cleanup are consistent with existing patternsIntroducing
SaveCompanyServerLoginInfoInteractor? saveLoginInfoInteractorandRemoveCompanyServerLoginInfoInteractor? removeLoginInfoInteractor, plus importing the handling extension and nulling these fields inonClose, aligns with the existing DI/cleanup approach in this controller. The names clearly express both the action and the data they operate on, matching the descriptive-naming preference for this repo.Based on learnings, …
Also applies to: 56-56, 90-91, 663-664
212-214: Company server login info lifecycle is handled consistently across flowsYou now clear or save company server login info in all the key places:
- After
SignInTwakeWorkplaceSuccesson mobile.- After auto sign-in via deep link on mobile.
- On
_getTokenOIDCSuccess, saving only for DNS-lookup form on mobile with a username, otherwise removing on mobile.- On common OIDC failures on mobile.
This should prevent stale company server login info from lingering once the user leaves or fails the relevant flow, while still persisting it for the intended DNS-login success case.
If you later find more entry points to the DNS login flow, consider centralizing this lifecycle into a single helper to reduce duplication of the
PlatformInfo.isMobile+ remove/save conditions.Also applies to: 304-306, 481-486, 578-581
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
lib/features/base/base_controller.dart(6 hunks)lib/features/base/extensions/handle_company_server_login_info_extension.dart(1 hunks)lib/features/base/reloadable/reloadable_controller.dart(1 hunks)lib/features/login/domain/state/get_company_server_login_info_state.dart(1 hunks)lib/features/login/domain/state/remove_company_server_login_info_state.dart(1 hunks)lib/features/login/domain/usecases/get_company_server_login_info_interactor.dart(1 hunks)lib/features/login/domain/usecases/remove_company_server_login_info_interactor.dart(1 hunks)lib/features/login/presentation/base_login_view.dart(1 hunks)lib/features/login/presentation/bindings/company_server_login_interactor_bindings.dart(1 hunks)lib/features/login/presentation/extensions/handle_company_server_login_info_extension.dart(1 hunks)lib/features/login/presentation/login_controller.dart(9 hunks)lib/features/login/presentation/login_view.dart(1 hunks)lib/features/login/presentation/model/login_arguments.dart(1 hunks)lib/features/starting_page/presentation/twake_welcome/twake_welcome_controller.dart(5 hunks)lib/main/bindings/main_bindings.dart(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- lib/features/login/domain/state/remove_company_server_login_info_state.dart
- lib/features/login/presentation/model/login_arguments.dart
- lib/features/login/presentation/extensions/handle_company_server_login_info_extension.dart
- lib/main/bindings/main_bindings.dart
- lib/features/login/presentation/bindings/company_server_login_interactor_bindings.dart
- lib/features/starting_page/presentation/twake_welcome/twake_welcome_controller.dart
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-12-08T08:11:08.977Z
Learnt from: dab246
Repo: linagora/tmail-flutter PR: 4189
File: lib/features/login/presentation/extensions/handle_company_server_login_info_extension.dart:50-56
Timestamp: 2025-12-08T08:11:08.977Z
Learning: In the tmail-flutter codebase, prefer descriptive variable names that include full context rather than abbreviated names. For example, `removeLoginInfoInteractor` is preferred over shorter alternatives like `removeInteractor` or `interactor`, as it clearly conveys both the action (remove) and the specific data being operated on (loginInfo).
Applied to files:
lib/features/login/presentation/login_view.dartlib/features/base/base_controller.dartlib/features/login/domain/usecases/remove_company_server_login_info_interactor.dartlib/features/login/presentation/base_login_view.dartlib/features/login/presentation/login_controller.dart
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
- GitHub Check: analyze-test (server_settings)
- GitHub Check: analyze-test (email_recovery)
- GitHub Check: analyze-test (core)
- GitHub Check: analyze-test (forward)
- GitHub Check: analyze-test (fcm)
- GitHub Check: analyze-test (default)
- GitHub Check: analyze-test (model)
- GitHub Check: analyze-test (rule_filter)
- GitHub Check: analyze-test (contact)
🔇 Additional comments (14)
lib/features/base/reloadable/reloadable_controller.dart (1)
181-185: LGTM! Clean refactoring that eliminates duplication.The consolidation of interceptor setup logic into
setDataToInterceptorsis a good improvement. This addresses the previous concern about duplicategetOidcUserInfocalls by reusing the existing method that already handles this (line 130), and it maintains consistency with the pattern used in_handleGetStoredTokenOidcSuccess.lib/features/login/domain/usecases/remove_company_server_login_info_interactor.dart (1)
9-22: LGTM!The interactor follows clean architecture conventions with proper dependency injection, stream-based state emission (loading → success/failure), and exception handling.
lib/features/login/presentation/login_view.dart (1)
106-108: LGTM!The lambda wrapper correctly forwards the query as a named
patternparameter, aligning with the updatedgetAllRecentLoginUsernameActionsignature and maintaining consistency withbase_login_view.dart.lib/features/login/domain/state/get_company_server_login_info_state.dart (2)
5-5: LGTM!Simple loading state extending the base
LoadingStateclass.
7-18: LGTM!Success state properly encapsulates the login info and navigation flag with correct equality implementation.
lib/features/base/base_controller.dart (4)
92-92: LGTM!The nullable interactor field follows the same pattern as other optional interactors in the class (
_getStoredFirebaseRegistrationInteractor,_destroyFirebaseRegistrationInteractor). The public visibility aligns with the learning that descriptive full-context variable names are preferred in this codebase.
271-273: LGTM!Failure handling delegates to the extension method, maintaining separation of concerns.
288-290: LGTM!Success handling correctly passes
serverLoginInfoto the extension method for navigation logic.
400-414: Verify extension methodgetCompanyServerLoginInfois available.Both
goToLoginandremoveAllPageAndGoToLoginnow callgetCompanyServerLoginInfo(popAllRoute:)for mobile. TheHandleCompanyServerLoginInfoExtensionis properly defined as a Dart extension onBaseControllerinlib/features/base/extensions/handle_company_server_login_info_extension.dartand is imported inbase_controller.dart, making the method available to all BaseController instances. All dependent methods (getBinding,consumeState,pushAndPopAll,popAndPush) are present in BaseController.lib/features/login/presentation/base_login_view.dart (1)
78-79: LGTM!Consistent with the identical change in
login_view.dart. The lambda correctly forwards the query as a namedpatternparameter.lib/features/login/domain/usecases/get_company_server_login_info_interactor.dart (1)
9-29: Interactor flow looks consistent and correctThe
executestream correctly emits a loading state, then success with the retrieved info (includingpopAllRoute), and wraps any exception into the failure state. This matches the existing Either/Success/Failure pattern and should integrate cleanly withconsumeState.lib/features/base/extensions/handle_company_server_login_info_extension.dart (1)
11-47: Extension wiring and navigation behavior look solidThe extension cleanly:
- Resolves the interactor via
getBindingwith a safe null fallback to the failure handler.- Delegates the stream into
consumeStatewithout unnecessaryasync/await.- Centralizes success/failure navigation using
LoginArgumentsandpopAllRoute, keeping routing logic out of the domain layer.No functional issues spotted here.
lib/features/login/presentation/login_controller.dart (2)
139-151: onReady company-server autofill precedence looks correctThe new
onReadybranch:
- Normalizes
loginFormTypeas before.- Prefers
hasPreviousCompanyServerLogin(arguments)andautoFillPreviousCompanyServerMailwhen prior company-server info is available.- Falls back to
isCompanyServerLoginFlow(arguments)andautoFillCompanyServerMail.- Only then, for non-company-server flows on web, calls
_checkOIDCIsAvailable().This preserves previous web behavior while cleanly adding the mobile company-server autofill cases.
540-549: Updated recent-username retrieval API remains backward-compatible in behaviorSwitching
getAllRecentLoginUsernameActionto accept an optional namedpatternand forwarding it via.execute(pattern: pattern)keeps the existing fold logic intact while allowing callers to omit the filter when not needed. The empty-list fallback on failure/non-latest success is preserved, so runtime behavior stays predictable.
lib/features/login/domain/state/get_company_server_login_info_state.dart
Outdated
Show resolved
Hide resolved
Generic persistence helpers - A reusable CachedDataManager that handles save/get/remove for any model @coderabbitai |
|
Tip For best results, initiate chat on the files or code changes. @dab246, I really appreciate your concern about scope creep—you're absolutely right to be cautious! 🎯 My Recommendation: DON'T implement those improvements in this PRHere's why: ❌ Why NOT to refactor now:
✅ What you SHOULD do:
📋 Would you like me to create a follow-up issue?If you'd like, I can create a GitHub issue titled something like:
This way, the architectural improvements are tracked without blocking this PR, and the team can plan them properly. Should I create that issue for you? Just let me know, and I'll set it up with appropriate details and a backlink to this PR. 📝 |
…e props override.
|
@coderabbitai please help me create a follow-up issue: "[Architecture] Reduce boilerplate through generic persistence and base classes" with the detail description |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 53 @hoangdat, done! ✅ I've successfully created the follow-up issue for you: Issue #4192: [Architecture] Reduce boilerplate through generic persistence and base classes What's included:✅ Comprehensive context - References PR #4189 and the discussion about 35 file changes
✅ Expected impact - Could reduce similar features from ~35 files to ~10-15 files The issue emphasizes that this should be a dedicated initiative separate from feature work, and confirms that the current patterns in PR #4189 are acceptable. Feel free to adjust the issue details, labels, or milestones as needed for your team's planning process! 🎯 |
Issue
#4186
Resolved
Screen_recording_20251203_135705.online-video-cutter.com.mp4
Summary by CodeRabbit
New Features
Improvements
✏️ Tip: You can customize this high-level summary in your review settings.