Add multi account support #78
Open
Conversation
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
|
Warning Rate limit exceeded
⌛ 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. WalkthroughAdds multi-account support: new SwitchProfile and AddProfile screens, a useAccounts hook, auth provider changes (logout returns next pubkey, new switchProfile, isAddingAccount flag), routing updates, a reusable auth-buttons widget, provider refactor for account pubkey, and extensive tests/mocks. Changes
Sequence Diagram(s)sequenceDiagram
participant User as User
participant UI as SwitchProfileScreen
participant Hook as useAccounts
participant Auth as AuthNotifier
participant Storage as SecureStorage
participant API as Accounts API
User->>UI: Open Switch Profile
UI->>Hook: init -> fetch accounts
Hook->>API: crateApiAccountsGetAccounts()
API-->>Hook: accounts list
Hook-->>UI: render accounts
User->>UI: Tap account (pubkey)
UI->>Hook: switchTo(pubkey)
Hook->>Auth: switchProfile(pubkey)
Auth->>API: crateApiAccountsGetAccount(pubkey) (optional)
Auth->>Storage: write active pubkey
Auth-->>Hook: success
Hook-->>UI: clear isSwitching & navigate back
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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
force-pushed
the
multi-account
branch
from
754c376 to
be61f66
Compare
✅ Coverage: 99.62% → 99.64% (+0.02%) |
1 similar comment
✅ Coverage: 99.62% → 99.64% (+0.02%) |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test/screens/sign_out_screen_test.dart (1)
102-111: Usefind.byKey()instead offind.byIcon()for icon assertions.Per coding guidelines, tests should use
find.byKey()instead offind.byIcon(). While this is existing code, consider updating to use keys for consistency. This applies to multiple icon lookups in this file (lines 106, 116, 125-133, 138).
🤖 Fix all issues with AI agents
In `@lib/hooks/use_accounts.dart`:
- Around line 30-35: The catch block currently shows a SnackBar with the raw
error string via ScaffoldMessenger.of(context).showSnackBar and
SnackBar(content: Text('Failed to switch profile: ${e.toString()}')); replace
this with a user-friendly message (e.g. 'Failed to switch profile. Please try
again.') for the SnackBar and move the raw error to a non-user-facing log or
error-report call (e.g. debugPrint or a reporting function) so the code in the
catch for the switch profile flow (the
ScaffoldMessenger.of(context).showSnackBar / SnackBar usage inside the catch in
use_accounts.dart) no longer surfaces internal error text to users.
In `@lib/providers/auth_provider.dart`:
- Around line 90-96: The code is doing fragile string matching on ApiError
messages in the catch blocks (used in the catch at switch handling and in
build())—define a single constant like _accountNotFoundError = 'Account not
found' at the top of the file and replace the inline contains(...) checks with a
reference to that constant (e.g., if (e is ApiError &&
e.message.contains(_accountNotFoundError))) so both the catch in the switch
logic (which calls _logger.warning, storage.delete and sets state = const
AsyncData(null)) and the check in build() use the same named constant for
consistency and easier future updates or replacement with a code-based check if
available.
- Around line 66-68: Remove the unnecessary Future.microtask wrapper in the
logout() and switchProfile() methods and assign state directly (e.g., state =
AsyncData(nextAccount.pubkey) or state = AsyncData(null)) just like in login()
and signup(); locate the Future.microtask(...) blocks inside logout() and
switchProfile() and replace them with direct state assignments to match the
provider's other methods and other providers in the codebase.
In `@lib/routes.dart`:
- Around line 59-62: The literal 'add-profile' is repeated; extract it into a
single constant (e.g., const String addProfileQuery = 'add-profile') in
lib/routes.dart and replace the inline occurrences: the
state.uri.queryParameters['from'] comparison that sets isAddingAccount and the
two places that push routes with query parameter 'from' (the push calls that
currently use the raw 'add-profile' string). Update references to use the new
constant everywhere to avoid duplication and typos.
In `@test/hooks/use_accounts_test.dart`:
- Around line 214-256: The hook is currently surfacing raw exception text
(e.toString()) in the user Snackbar; update the error handling in the
useAccounts hook (the switch/profile error path where the catch uses
e.toString()) to show the friendly message "Failed to switch profile. Please try
again." instead of the raw exception, and then update the test assertion in
test/hooks/use_accounts_test.dart (the failing test that expects the old raw
message) to expect "Failed to switch profile. Please try again." so the test
matches the new user-facing message.
In `@test/screens/add_profile_screen_test.dart`:
- Around line 25-38: The tests initialize a mocked Rust API globally but never
reset MockWnApi between tests, leaking StreamControllers; add a per-test reset
by inserting a setUp(() => MockWnApi.reset()) (or call the reset() on the
specific mock class used, e.g., _MockApi.reset() if that is the mock type)
alongside the existing setUpAll RustLib.initMock(...) so MockWnApi.reset() runs
before each test instead of relying on tearDownAll.
In `@test/screens/switch_profile_screen_test.dart`:
- Around line 149-158: The test "tapping different account switches profile"
uses four consecutive tester.pump() calls to wait for async UI updates; replace
those with a single await tester.pumpAndSettle() after the tap in the
testWidgets body (reference: testWidgets in switch_profile_screen_test.dart and
the helper pumpSwitchProfileScreen), so the test waits for all animations and
async work to finish reliably and more clearly.
- Around line 129-133: The test "displays checkmark for current account" uses
find.byIcon which violates the guideline—update the widget SwitchProfileScreen
to give the checkmark Icon a Key (e.g., Key('current_account_checkmark') on the
Icon with Icons.check_circle) and change the test (testWidgets 'displays
checkmark for current account') to locate the icon with find.byKey(const
Key('current_account_checkmark')) and assert findsOneWidget; ensure the unique
key string matches between the widget (SwitchProfileScreen) and the test.
be61f66 to
d519071
Compare
✅ Coverage: 99.62% → 99.64% (+0.02%) |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@lib/hooks/use_accounts.dart`:
- Around line 21-38: The catch block in switchTo currently only logs the error
(_logger.severe) and leaves the user without feedback; update the catch to show
a user-friendly SnackBar (e.g., via ScaffoldMessenger.of(context).showSnackBar)
when ref.read(authProvider.notifier).switchProfile(pubkey) throws, while keeping
the existing log and finally reset of isSwitching.value; also ensure you only
call the SnackBar when context.mounted is true and do not change Routes.goBack
or the existing flow for successful switches.
✅ Coverage: 99.62% → 99.64% (+0.02%) |
81 tasks
josefinalliende
requested changes
Jan 23, 2026
Collaborator
josefinalliende
left a comment
josefinalliende
left a comment
There was a problem hiding this comment.
Feature works great 💯 The add profile screen feels a bit too empty, but as the design is going to change soon, I wouldn't bother about changing it now.
Left some comments related to the query param approach to detect of the app in in a adding account state and also unresolved a code rabbit's comment related to saving an error state.
untreu2
force-pushed
the
multi-account
branch
3 times, most recently
from
0b9be9d to
e978d4c
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test/screens/sign_out_screen_test.dart (1)
106-106: Usefind.byKey()instead offind.byIcon()per coding guidelines.This test uses
find.byIcon(Icons.copy). Add aKeyto the copy icon in the widget and usefind.byKey()here. This also applies to lines 116, 125, 129, 131, and 138.♻️ Example fix
In the widget, add a key:
Icon(Icons.copy, key: const Key('copy_icon'))Then in the test:
- final copyButton = find.byIcon(Icons.copy); + final copyButton = find.byKey(const Key('copy_icon'));
🤖 Fix all issues with AI agents
In `@lib/hooks/use_accounts.dart`:
- Around line 42-62: Move the post-switch UI state update into the mounted check
to avoid updating widget-local state when the widget is unmounted: inside
switchTo, after await ref.read(authProvider.notifier).switchProfile(pubkey) and
the if (context.mounted) { Routes.goBack(context); } block, also call
state.value = state.value.copyWith(isSwitching: false) within that same mounted
check (instead of unconditionally afterward) so the isSwitching flag is only
cleared when the widget is still mounted.
In `@lib/providers/auth_provider.dart`:
- Around line 55-75: The logout method currently clears storage but leaves state
unchanged if accounts_api.getAccounts() or storage.write() throws; update logout
to wrap the account lookup and storage-write logic (the calls to
accounts_api.getAccounts and storage.write using _storageKey) in a try/catch and
ensure state is set to AsyncData(null) on any failure (or in a finally block) so
the in-memory state cannot remain the old pubkey after storage.delete; preserve
logging (use _logger.info/_logger.error) to record the error and still return
null (or the nextAccount.pubkey when successful).
In `@lib/screens/switch_profile_screen.dart`:
- Around line 136-200: The spinner appears on every non-current tile because it
only checks isSwitching; change the logic to track which pubkey is being
switched to in your accounts state (e.g., add switchingToPubkey to AccountsState
returned by useAccountsState/useUserMetadata) and update the tile rendering to
show the loader only when isSwitching && switchingToPubkey == pubkey &&
!isCurrent; update the code paths that start/stop switching to set/clear
switchingToPubkey accordingly so only the target _AccountTile (matching pubkey)
shows the CircularProgressIndicator.
In `@test/screens/sign_out_screen_test.dart`:
- Around line 160-186: In the test "signing out with another account returns to
previous screen" replace the three successive tester.pump() calls (used to wait
for the sign-out flow) with a single await tester.pumpAndSettle() to simplify
and robustly wait for animations/state changes before asserting
mockAuth.logoutCalled and that ChatListScreen is shown; update the calls in the
testWidgets block accordingly to use pumpAndSettle on the tester instance used
throughout the test.
♻️ Duplicate comments (1)
test/hooks/use_accounts_test.dart (1)
350-406: Add a test for successful profile switch completion.The test suite covers the error path and the "isSwitching during operation" state, but lacks a test verifying the successful completion of
switchTo. A past review comment flagged this missing coverage.🧪 Suggested test case
testWidgets('switchTo completes successfully and updates state', (tester) async { late Future<void> Function(String) capturedSwitchTo; late AccountsState capturedState; final mockAuthNotifier = _MockAuthNotifier('pubkey1'); mockApi.accounts = [ Account( pubkey: 'pubkey1', createdAt: DateTime.now(), updatedAt: DateTime.now(), ), Account( pubkey: 'pubkey2', createdAt: DateTime.now(), updatedAt: DateTime.now(), ), ]; setUpTestView(tester); await tester.pumpWidget( ProviderScope( overrides: [ authProvider.overrideWith(() => mockAuthNotifier), secureStorageProvider.overrideWithValue(MockSecureStorage()), ], child: MaterialApp( home: Scaffold( body: _TestWidget( currentPubkey: 'pubkey1', onBuild: (accounts, state, switchTo) { capturedSwitchTo = switchTo; capturedState = state; }, ), ), ), ), ); await tester.pumpAndSettle(); await capturedSwitchTo('pubkey2'); await tester.pumpAndSettle(); expect(capturedState.isSwitching, false); expect(capturedState.error, isNull); });
✅ Coverage: 99.6% → 99.62% (+0.02%) |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@test/hooks/use_accounts_test.dart`:
- Around line 250-300: After completing the switchProfileCompleter, pump the
widget tree and assert the final AccountsState: call
mockAuthNotifier.switchProfileCompleter!.complete(), then await
tester.pumpAndSettle() (or tester.pump()) and assert capturedState.isSwitching
is false and capturedState.error is null to verify switchTo finished
successfully; reference capturedSwitchTo and
mockAuthNotifier.switchProfileCompleter in the new assertions.
In `@test/widgets/wn_auth_buttons_container_test.dart`:
- Around line 9-39: Replace the local variable declarations like final widget =
const WnAuthButtonsContainer(); with a const variable declaration (e.g., const
widget = WnAuthButtonsContainer();) in each test case; update all occurrences in
tests that create WnAuthButtonsContainer so the variable itself is declared
const rather than final wrapping a const constructor, referencing the
WnAuthButtonsContainer variable named widget in each test.
♻️ Duplicate comments (3)
lib/providers/auth_provider.dart (1)
55-76: Guard logout state if account lookup/storage write fails.If
getAccounts()orstorage.write()throws,stateremains the old pubkey while storage is cleared, leaving the UI in a logged-in state. Ensure a safe fallback toAsyncData(null)on failure.🛠️ Suggested fix
await accounts_api.logout(pubkey: pubkey); await storage.delete(key: _storageKey); - final remainingAccounts = await accounts_api.getAccounts(); - if (remainingAccounts.isNotEmpty) { - final nextAccount = remainingAccounts.first; - await storage.write(key: _storageKey, value: nextAccount.pubkey); - state = AsyncData(nextAccount.pubkey); - _logger.info('Logout successful - switched to another account'); - return nextAccount.pubkey; - } - - state = const AsyncData(null); - _logger.info('Logout successful - no remaining accounts'); - return null; + try { + final remainingAccounts = await accounts_api.getAccounts(); + if (remainingAccounts.isNotEmpty) { + final nextAccount = remainingAccounts.first; + await storage.write(key: _storageKey, value: nextAccount.pubkey); + state = AsyncData(nextAccount.pubkey); + _logger.info('Logout successful - switched to another account'); + return nextAccount.pubkey; + } + } catch (e, st) { + _logger.warning('Logout completed but account switch failed', e, st); + } + + state = const AsyncData(null); + _logger.info('Logout successful - no remaining accounts'); + return null;lib/screens/switch_profile_screen.dart (1)
97-104: Loading indicator shows on all non-current tiles during switch.The
isSwitchingboolean is passed to all tiles, causing the spinner to appear on every non-current account when any switch is in progress. Consider tracking the target pubkey being switched to and only showing the spinner on that specific tile.test/screens/sign_out_screen_test.dart (1)
172-195: PreferpumpAndSettle()to replace multiplepump()calls.
This is the same pattern flagged earlier; a single settle call is clearer and more reliable here.♻️ Suggested simplification
await tester.tap(signOutButtons.last); - await tester.pump(); - await tester.pump(); - await tester.pump(const Duration(milliseconds: 100)); + await tester.pumpAndSettle();
untreu2
force-pushed
the
multi-account
branch
from
e978d4c to
469aa18
Compare
❌ Coverage: 99.6% → 99.59% (-0.01%) |
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@lib/hooks/use_accounts.dart`:
- Around line 38-39: The cached future created by useMemoized(() =>
accounts_api.getAccounts()) (accountsFuture) prevents re-fetching after profile
changes; modify the hook to allow explicit refresh by replacing the static
useMemoized with a refreshable mechanism (e.g., a stateful key or a
refreshTrigger state) so that calling refresh re-invokes
accounts_api.getAccounts() and updates the useFuture(accountsFuture) snapshot;
expose the refresh callback (named refresh) in the hook's return value alongside
the accounts snapshot so callers (e.g., AddProfile flow) can trigger an
immediate re-fetch.
In `@lib/providers/account_pubkey_provider.dart`:
- Around line 7-12: The listener on authProvider in accountPubkeyProvider only
sets state when pubkey != null, leaving a stale value on logout; change the
ref.listen callback in accountPubkeyProvider so that when next.value is null you
clear or invalidate the notifier instead of leaving the old state (e.g., call
ref.invalidateSelf() or ref.invalidate(accountPubkeyProvider) when pubkey is
null), keeping the Notifier<String> from exposing stale data to consumers.
In `@test/screens/switch_profile_screen_test.dart`:
- Around line 128-142: The test duplicates navigation/setup steps; update
pumpSwitchProfileScreen to accept an optional parameter (e.g., overrides or a
startingPubkey) so tests can mount the app and navigate into the Switch Profile
screen in one call. Modify pumpSwitchProfileScreen to apply provided overrides
(or set mockApi.accounts and authProvider state) instead of requiring
test-by-test calls to mountTestApp and
Routes.pushToSettings/Routes.pushToSwitchProfile; then simplify this test to
call pumpSwitchProfileScreen(tester, startingPubkey: 'pubkey1') after setting
mockApi.accounts = [].
In `@test/widgets/wn_auth_buttons_container_test.dart`:
- Around line 7-85: Add tests to verify default navigation in
WnAuthButtonsContainer when onLogin/onSignup are omitted: mount the widget
inside a test app (use mountTestApp or equivalent route setup), tap the 'Login'
and 'Sign Up' buttons, pumpAndSettle, and assert that navigation occurred by
checking either that the expected destination widget is present or that
Routes.pushToLogin / Routes.pushToSignup were invoked (mock or spy the Routes if
available). Target the WnAuthButtonsContainer constructor (no callbacks) and
reuse existing test patterns (tester.tap, pumpAndSettle, find.*) to keep tests
consistent with the file's style.
♻️ Duplicate comments (1)
lib/screens/switch_profile_screen.dart (1)
123-200: LGTM!The
_AccountTilewidget correctly usesnpubFromHexwith null fallback (line 170) per project conventions. The checkmark icon has a key for testability (line 182).
untreu2
force-pushed
the
multi-account
branch
from
469aa18 to
8102279
Compare
❌ Coverage: 99.6% → 99.59% (-0.01%) |
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Fix all issues with AI agents
In `@lib/providers/auth_provider.dart`:
- Around line 64-72: The current logout flow calls accounts_api.getAccounts()
and then picks remainingAccounts.first which can still include the
just-logged-out pubkey; update the logic in logout() to filter out the
logged-out pubkey from remainingAccounts (e.g. remove items where account.pubkey
== the pubkey passed to logout()), then select the next account from that
filtered list (handle the case where the filtered list is empty by clearing
storage/state appropriately). Ensure you update storage.write(key: _storageKey,
value: ...) and state = AsyncData(...) only when a truly different
nextAccount.pubkey is chosen.
In `@test/hooks/use_nsec_test.dart`:
- Around line 140-145: Extract the casted notifier into a local variable to
improve readability: call container.read(accountPubkeyProvider.notifier), assign
it to a local typed variable of MockAccountPubkeyNotifier, then call
.update('new_pubkey') on that variable and await tester.pumpAndSettle(); keep
the same behavior but replace the inline cast
`(container.read(accountPubkeyProvider.notifier) as
MockAccountPubkeyNotifier).update(...)` with the clearer two-step approach using
the MockAccountPubkeyNotifier-typed local.
In `@test/mocks/mock_wn_api.dart`:
- Around line 99-115: The mock method crateApiAccountsGetAccount currently
throws a generic Exception('Account not found') which differs from production
error typing; update the error thrown to ApiError.whitenoise (or the appropriate
ApiError variant) so tests exercise the same error-handling paths as production.
Locate crateApiAccountsGetAccount in test/mocks/mock_wn_api.dart (which uses
getAccountsCompleter and accounts) and replace the orElse throw to throw
ApiError.whitenoise (including any required constructor args) and import or
reference ApiError as needed.
In `@test/screens/add_profile_screen_test.dart`:
- Around line 19-25: The _MockAuthNotifier.build() currently assigns state =
const AsyncData('test_pubkey') and then returns 'test_pubkey', which can cause a
double-update; update the AuthNotifier mock so build() is async-compliant by
removing the explicit state assignment and simply returning 'test_pubkey' (or
awaited value) so the AsyncNotifier machinery wraps the result into AsyncData
automatically; adjust _MockAuthNotifier.build to be an async method that returns
the string without setting state.
In `@test/screens/sign_out_screen_test.dart`:
- Around line 172-195: Refactor the test helper pumpSignOutScreen to accept an
optional _MockAuthNotifier parameter so tests can pass a prepared mock instead
of duplicating setup: change pumpSignOutScreen to add an optional argument
(e.g., {_MockAuthNotifier? mockAuth}) and if null create the existing default
mock inside the helper; ensure the helper uses the passed mock when overriding
authProvider and secureStorageProvider (symbols: pumpSignOutScreen,
_MockAuthNotifier, authProvider); update this test to call
pumpSignOutScreen(tester, mockAuth: mockAuth) and remove the duplicated
mountTestApp/override logic so the test reuses the centralized setup.
In `@test/screens/switch_profile_screen_test.dart`:
- Around line 110-117: Replace the redundant await tester.pump() followed by
await tester.pumpAndSettle() in the 'tapping current account goes back' test
with a single await tester.pumpAndSettle(); locate the test named "tapping
current account goes back" (and the use of pumpSwitchProfileScreen and
tester.tap(find.text('Display pubkey1'))) and remove the extra await
tester.pump() so the test awaits only pumpAndSettle() to settle animations and
frames.
8102279 to
0dd44ab
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@lib/screens/sign_out_screen.dart`:
- Around line 40-54: The outer context.mounted check in signOut() is
redundant—remove the if (context.mounted) { ... } wrapper and always call
WidgetsBinding.instance.addPostFrameCallback after awaiting
ref.read(authProvider.notifier).logout(); keep the inner mounted check inside
the callback to guard navigation and preserve isLoggingOut.value = true and the
use of nextPubkey to choose between Routes.goBack(context) and
Routes.goToHome(context).
❌ Coverage: 99.6% → 99.59% (-0.01%) |
❌ Coverage: 99.6% → 99.59% (-0.01%) |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Description
Allow users to switch between multiple profiles and add new accounts without signing out.
Changes
Why accountPubkeyProvider is now a NotifierProvider?
A sync Provider wrapping an AsyncNotifierProvider doesn't reliably notify watchers when the source changes. This was never noticed before because the app only supported single account - the pubkey never changed at runtime. ChatListScreen stays mounted in the navigation stack, so it needs notifications to rebuild - unlike other screens which get rebuilt on navigation. Converting to NotifierProvider with ref.listen and state assignment ensures all watchers receive updates.
Type of Change
Checklist
just precommitto ensure that formatting and linting are correctCHANGELOG.mdfile with your changes (if they affect the user experience)Summary by CodeRabbit
New Features
Behavior Changes
Tests
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.
Closes #24