Fix account loading on general settings page #614
Conversation
WalkthroughRefactors GeneralSettingsScreen to load account data via providers with lazy loading, adds guarded fallback API loading, introduces loading/error state fields, listens for provider changes to refresh current account and metadata-derived ContactModel, updates UI to show loading/error states, and adjusts account switching/logout to use cached/provider data with toast-based error handling. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant Screen as GeneralSettingsScreen
participant Providers as Account/Metadata Providers
participant API as Accounts API
participant UI as UI/Toasts
User->>Screen: Open Settings
rect #eef7ff
note right of Screen: Initial load
Screen->>Providers: Get active account + metadata
alt Provider data available
Screen->>Screen: _refreshAccountData()<br/>Create ContactModel from metadata
Screen->>Providers: (async) Maybe fetch full accounts list
note over Screen,Providers: Lazy load other accounts' contact models
else Missing provider data
Screen->>API: _loadAccountsFromApi()
alt Success
Screen->>Screen: Populate accounts + current account
else Error
Screen->>UI: Show error toast
end
end
end
rect #f6fff0
note right of Screen: Provider updates
Providers-->>Screen: Active account / metadata changed
Screen->>Screen: _refreshAccountData()
Screen->>UI: Update current account tile
end
rect #fff7ee
note right of Screen: UI states
Screen->>UI: Show loading indicator or error message as needed
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Pre-merge checks (5 passed)✅ Passed checks (5 passed)
Poem
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. ✨ Finishing Touches🧪 Generate unit tests
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 |
untreu2
requested review from
erskingardner and
josefinalliende
and removed request for
erskingardner
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
lib/ui/settings/general_settings_screen.dart (2)
373-403: Fix “multiple accounts” logic and stale list after logout
- Condition
> 2is incorrect; should be> 1.- Using cached
_accountsbefore logout gives the wrong count post-logout. Fetch the remaining accounts after logout to decide the flow and refresh the cache.@@ - // Use cached accounts instead of making another bridge call - final hasMultipleAccounts = _accounts.length > 2; + // Determine remaining accounts after logout @@ - await authNotifier.logoutCurrentAccount(); + await authNotifier.logoutCurrentAccount(); @@ - // Check the final auth state after logout + // Check the final auth state after logout final finalAuthState = ref.read(authProvider); + // Refresh accounts and compute remaining count + final remainingAccounts = await getAccounts(); + final hasMultipleAccounts = remainingAccounts.length > 1; + if (mounted) { + setState(() { + _accounts = remainingAccounts; + }); + } @@ - if (hasMultipleAccounts) { + if (hasMultipleAccounts) { await _loadFromProviders();
302-310: Addpackage:collectionimport or bump SDK forIterable.firstOrNullIn lib/ui/settings/general_settings_screen.dart (lines 302 & 309), you call
.firstOrNullwithout importing the extension from package:collection. This will fail on CI unless your Dart SDK lower bound includes the built-infirstOrNull. Either:
- Add
to that file, orimport 'package:collection/collection.dart';- Raise your SDK constraint in pubspec.yaml to a version that natively provides
Iterable.firstOrNull.
🧹 Nitpick comments (9)
lib/ui/settings/general_settings_screen.dart (9)
44-48: Consolidate loading state into a single UI state (Freezed union) managed by RiverpodThree flags (
_isLoadingAccounts,_loadingError,_isLoadingInProgress) are easy to desync. Prefer aGeneralSettingsStateunion (loading | ready | error) in a provider; the widget subscribes to that state. Keeps logic out of the widget and follows our “Use Riverpod + Freezed to manage UI states” guideline.
58-71: Guard noisy refreshes when metadata instances change but values don’t
previous.value.metadata != newMetadatamay trigger on instance inequality even if content is equal. Ifmetadatais a Map or data class without deep equality, this will cause redundant_refreshAccountData()calls. Consider value equality (e.g., a hash orpackage:collectionDeepCollectionEquality) or expose a version/timestamp from the provider to compare.
83-118: Make loading flag transitions explicit inside provider-first pathWhen provider has no active account,
_loadAccountsFromApi()sets loading=true. When the provider already has an account, we should also ensure loading=false (see prior diff). Additionally, consider returning early ifactiveAccountStateisAsyncLoadingto avoid kicking the API fallback too eagerly.
120-139: Add a simple reentrancy guard and error surfacing for lazy loadIf this method is called twice quickly, it can race. Mirror the
_isLoadingInProgressguard here or use a local_isLoadingAccountsListflag. Also consider bubbling errors to_loadingErrorto render an error state rather than silentlydebugPrint.@@ - Future<void> _loadAccountsLazilyIfNeeded() async { - if (_accounts.isEmpty) { + Future<void> _loadAccountsLazilyIfNeeded() async { + if (_accounts.isEmpty && !_isLoadingInProgress) { + _isLoadingInProgress = true; try { final accounts = await getAccounts(); if (mounted) { setState(() { _accounts = accounts; }); _loadContactModelsForOtherAccounts(accounts); } } catch (e) { - // Silently handle - account switching might not work but current account will show - debugPrint('Failed to load accounts list: $e'); + debugPrint('Failed to load accounts list: $e'); + if (mounted) { + setState(() => _loadingError = e.toString()); + } } + _isLoadingInProgress = false; } }
140-163: Explicitly mark fire-and-forget futuresUse
unawaited(...)(dart:async) or a comment to silence lints and document intent for the background loads.@@ - userProfileDataNotifier + // unawaited: background prefetch + userProfileDataNotifier .getUserProfileData(account.pubkey)
201-233: Remove unnecessary async and ensure loading flag is cleared after refreshMethod doesn’t
await. Also ensure we end any loading state after applying fresh provider data.- void _refreshAccountData() async { + void _refreshAccountData() { @@ - setState(() { - _currentAccount = activeAccount; - }); + setState(() { + _currentAccount = activeAccount; + _isLoadingAccounts = false; + });
264-281: Name should start with a verbFollow guidelines: rename
_accountToContactModeltobuildContactModelForAccountand update call site.- ContactModel _accountToContactModel(Account account) { + ContactModel buildContactModelForAccount(Account account) { @@ - final contactModels = _accounts.map(_accountToContactModel).toList(); + final contactModels = _accounts.map(buildContactModelForAccount).toList();Also applies to: 280-281
316-320: Localize user-facing stringsReplace hardcoded English strings with
AppLocalizations(e.g., error toasts and error label).- ref.showErrorToast('Account not found. Please try switching account again.'); + ref.showErrorToast(AppLocalizations.of(context)!.accountNotFoundTryAgain); @@ - const Center(child: Text('Error loading account')) + Center(child: Text(AppLocalizations.of(context)!.errorLoadingAccount))Also applies to: 451-453
279-327: Disable “Switch Account” when accounts not yet loadedIf
_accountsis empty, the sheet opens blank and may lead to the “Account not found” toast. Consider disabling the button or showing a loading indicator until_accounts.isNotEmpty.- WnFilledButton( + WnFilledButton( label: 'Switch Account', size: WnButtonSize.small, visualState: WnButtonVisualState.secondary, - onPressed: () => _showAccountSwitcher(), + onPressed: _accounts.isEmpty ? null : () => _showAccountSwitcher(),
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
ios/Podfile.lockis excluded by!**/*.lock
📒 Files selected for processing (1)
lib/ui/settings/general_settings_screen.dart(6 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.dart
📄 CodeRabbit inference engine (.cursor/rules/flutter.mdc)
**/*.dart: Always declare the type of each variable and function (parameters and return value). Avoid using 'any'. Create necessary types.
Don't leave blank lines within a function.
One export per file.
Use PascalCase for classes.
Use camelCase for variables, functions, and methods.
Use underscores_case for file and directory names.
Use UPPERCASE for environment variables. Avoid magic numbers and define constants.
Start each function with a verb.
Use verbs for boolean variables. Example: isLoading, hasError, canDelete, etc.
Use complete words instead of abbreviations and correct spelling, except for standard and well-known abbreviations (API, URL, i, j, err, ctx, req, res, next).
Write short functions with a single purpose. Less than 20 instructions.
Name functions with a verb and something else. If it returns a boolean, use isX or hasX, canX, etc. If it doesn't return anything, use executeX or saveX, etc.
Avoid nesting blocks by early checks and returns, or extraction to utility functions.
Use higher-order functions (map, filter, reduce, etc.) to avoid function nesting. Use arrow functions for simple functions (less than 3 instructions). Use named functions for non-simple functions.
Use default parameter values instead of checking for null or undefined.
Reduce function parameters using RO-RO: use an object to pass multiple parameters and to return results. Declare necessary types for input arguments and output.
Use a single level of abstraction in functions.
Don't abuse primitive types and encapsulate data in composite types.
Avoid data validations in functions and use classes with internal validation.
Prefer immutability for data. Use readonly for data that doesn't change. Use 'as const' for literals that don't change.
Declare interfaces to define contracts.
Write small classes with a single purpose. Less than 200 instructions, less than 10 public methods, less than 10 properties.
Use exceptions to handle errors you don't expect. If you catch an exception, it sh...
Files:
lib/ui/settings/general_settings_screen.dart
🔇 Additional comments (1)
lib/ui/settings/general_settings_screen.dart (1)
40-40: Good addition: cache contact models per account pubkeyThis avoids repeat profile fetches and reduces UI jank when switching.
| _loadFromProviders(); | ||
| _loadPackageInfo(); | ||
| _activeAccountSubscription = ref.listenManual( |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Prevent the initial “No accounts found” flash on first frame
Because loading starts in a post-frame callback, the first build shows “No accounts found” until _isLoadingAccounts flips. Start in loading=true and clear it as soon as provider data sets _currentAccount.
Apply this diff:
@@
- bool _isLoadingAccounts = false;
+ bool _isLoadingAccounts = true;
@@
- setState(() {
- _currentAccount = activeAccount;
- });
+ setState(() {
+ _currentAccount = activeAccount;
+ _isLoadingAccounts = false;
+ });Also applies to: 449-454
| Future<void> _loadAccountsFromApi() async { | ||
| if (!mounted || _isLoadingInProgress) return; | ||
|
|
||
| _isLoadingInProgress = true; | ||
|
|
||
| setState(() { | ||
| _isLoadingAccounts = true; | ||
| _loadingError = null; | ||
| }); | ||
|
|
||
| try { | ||
| final accounts = await getAccounts(); | ||
| final activeAccountState = ref.read(activeAccountProvider); | ||
| final activeAccount = activeAccountState.value?.account; | ||
|
|
||
| if (mounted) { | ||
| setState(() { | ||
| _accounts = accounts; | ||
| _currentAccount = activeAccount; | ||
| _isLoadingAccounts = false; | ||
| }); | ||
| } | ||
| } catch (e) { | ||
| if (mounted) { | ||
| setState(() { | ||
| _loadingError = e.toString(); | ||
| _isLoadingAccounts = false; | ||
| }); | ||
| ref.showErrorToast('Failed to load accounts: $e'); | ||
| } | ||
| } finally {} | ||
| } finally { | ||
| _isLoadingInProgress = false; | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fallback should still show a valid account instead of “No accounts found”
If the provider hasn’t resolved yet, _currentAccount stays null even when the accounts list exists, which reintroduces the “No accounts found” UI. Default _currentAccount to the first account as a temporary selection until the provider catches up.
@@
- setState(() {
- _accounts = accounts;
- _currentAccount = activeAccount;
- _isLoadingAccounts = false;
- });
+ setState(() {
+ _accounts = accounts;
+ _currentAccount = activeAccount ?? (accounts.isNotEmpty ? accounts.first : null);
+ _isLoadingAccounts = false;
+ });📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| Future<void> _loadAccountsFromApi() async { | |
| if (!mounted || _isLoadingInProgress) return; | |
| _isLoadingInProgress = true; | |
| setState(() { | |
| _isLoadingAccounts = true; | |
| _loadingError = null; | |
| }); | |
| try { | |
| final accounts = await getAccounts(); | |
| final activeAccountState = ref.read(activeAccountProvider); | |
| final activeAccount = activeAccountState.value?.account; | |
| if (mounted) { | |
| setState(() { | |
| _accounts = accounts; | |
| _currentAccount = activeAccount; | |
| _isLoadingAccounts = false; | |
| }); | |
| } | |
| } catch (e) { | |
| if (mounted) { | |
| setState(() { | |
| _loadingError = e.toString(); | |
| _isLoadingAccounts = false; | |
| }); | |
| ref.showErrorToast('Failed to load accounts: $e'); | |
| } | |
| } finally {} | |
| } finally { | |
| _isLoadingInProgress = false; | |
| } | |
| } | |
| Future<void> _loadAccountsFromApi() async { | |
| if (!mounted || _isLoadingInProgress) return; | |
| _isLoadingInProgress = true; | |
| setState(() { | |
| _isLoadingAccounts = true; | |
| _loadingError = null; | |
| }); | |
| try { | |
| final accounts = await getAccounts(); | |
| final activeAccountState = ref.read(activeAccountProvider); | |
| final activeAccount = activeAccountState.value?.account; | |
| if (mounted) { | |
| setState(() { | |
| _accounts = accounts; | |
| _currentAccount = activeAccount ?? (accounts.isNotEmpty ? accounts.first : null); | |
| _isLoadingAccounts = false; | |
| }); | |
| } | |
| } catch (e) { | |
| if (mounted) { | |
| setState(() { | |
| _loadingError = e.toString(); | |
| _isLoadingAccounts = false; | |
| }); | |
| ref.showErrorToast('Failed to load accounts: $e'); | |
| } | |
| } finally { | |
| _isLoadingInProgress = false; | |
| } | |
| } |
🤖 Prompt for AI Agents
In lib/ui/settings/general_settings_screen.dart around lines 166 to 199, the
current implementation leaves _currentAccount null when the provider hasn’t
resolved yet, causing the UI to show “No accounts found” even though accounts
were fetched; update the success path after fetching accounts so that if
activeAccount is null but the returned accounts list is non-empty, set
_currentAccount = accounts.first as a temporary/default selection (and keep
updating when the provider resolves), ensure this assignment happens inside the
mounted setState block and only when _currentAccount is null to avoid
overwriting a user selection.
There was a problem hiding this comment.
I hate to say this, but found it a bit strange that it required lazy loading, so I took a deeper look into the bug, and here are my thoughts: I think the real issue isn’t with loading the accounts for the switcher, but with maintaining a separate current account state instead of relying directly on the active account provider.
Fetching all accounts when the screen first renders, or when switching between accounts, isn’t an expensive operation. It doesn’t hit a server, it only queries the Rust crate with data already stored locally on the device. So, requesting the accounts every now and then shouldn’t be a problem.
The real problem iI think s that we’re waiting to load all accounts just to display the current one, when we could simply read the active account directly from its provider.
It took me a bit to review because I wanted to be sure I wasn’t talking nonsense 😅. I tested it locally in a branch, and it worked for me without changing the account loads. I left my explorations in a branch called explore/no-accounts-bug. #616 Could you take a look? I don’t want to step over your work, just sharing in case it helps, and maybe I’m missing something.
|
Another idea that comes to mind is that loading all accounts in the general settings screen might be unnecessary. It should be enough to load them within the switcher widget, only when that widget is rendered... But that could be handled as a refactor after the release, it’s not a priority right now, but it would be nice to keep this screen simpler and move the logic into the components that actually use the data. |
josefinalliende
left a comment
josefinalliende
left a comment
There was a problem hiding this comment.
✅ Tried it locally and works, so lets merge this fix. I was wrong about that only watching the active account was needed, it was still needed to load accounts before rendering the switcher, thanks for taking the time to check my draft!
Description
Fixed the settings page showing "No accounts found" for 1-10 seconds by replacing redundant getAccounts() calls with provider-first approach.
Type of Change
Checklist
just precommitto ensure that formatting and linting are correctjust check-flutter-coverageto ensure that flutter coverage rules are passingCHANGELOG.mdfile with your changes (if they affect the user experience)Fixes #608
Summary by CodeRabbit