fix(router): eliminate first-run race condition on startup - #72
Merged
Conversation
Pre-read SharedPreferences in main() before runApp() and inject firstRunProvider and backupReminderProvider with synchronous initial values via ProviderScope.overrides. Previously both notifiers started as AsyncValue.loading(), causing the router to skip the walkthrough redirect on first launch and incorrectly routing bell taps to /walkthrough instead of /notifications.
Contributor
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 4 minutes and 37 seconds. ⌛ 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. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
WalkthroughProviders now accept optional synchronous initial values to avoid automatic async loads. main() preloads SharedPreferences and initializes IdentityService before nostr, derives initial flags, and starts the app with ProviderScope overrides for first-run and backup state. Account UI reads identity via IdentityService, fully masks mnemonics by default, and requires explicit user checkbox to confirm backup. Changes
Sequence Diagram(s)sequenceDiagram
participant App as App Startup
participant Prefs as SharedPreferences
participant Identity as IdentityService
participant Nostr as Nostr API
participant Providers as Riverpod Providers
participant UI as Router/Widgets
App->>Prefs: preload SharedPreferences
Prefs-->>App: return flags (firstRunComplete, backupDismissed, backupActive)
App->>App: compute backupPending = backupActive && !backupDismissed
App->>Identity: initialize()
Identity-->>App: mnemonic / identity ready
App->>Nostr: initialize(with identity keys)
Nostr-->>App: nostr ready
App->>Providers: ProviderScope(overrides: FirstRunNotifier(initialValue), BackupReminderNotifier(initialValue))
Providers-->>App: providers created with sync initial state
App->>UI: runApp (UI starts without initial loading)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~40 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
lib/features/account/providers/backup_reminder_provider.dart (1)
17-21: Consider setting_loaded = truewheninitialValueis provided.When
initialValueis non-null,load()is skipped in the constructor but_loadedremainsfalse. IfshowBackupReminder()is later called, it will invokeawait load()which will re-read from SharedPreferences and potentially overwrite the state with the same (or stale) values.While this likely won't cause functional issues since the values are pre-read from the same source, setting
_loaded = truewhen an initial value is provided would be more semantically correct and avoid unnecessary SharedPreferences reads.♻️ Suggested improvement
BackupReminderNotifier({bool? initialValue}) : super(initialValue ?? false) { - if (initialValue == null) load(); + if (initialValue == null) { + load(); + } else { + _loaded = true; + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/features/account/providers/backup_reminder_provider.dart` around lines 17 - 21, The constructor for BackupReminderNotifier currently skips load() when an initialValue is provided but leaves _loaded false; update the BackupReminderNotifier constructor so that when initialValue is non-null it also sets _loaded = true to reflect that state is already initialized, preventing later calls (e.g., showBackupReminder()) from needlessly invoking load() and re-reading SharedPreferences.lib/main.dart (1)
24-28: Extract SharedPreferences keys to shared constants to avoid duplication.The key strings (
'firstRunComplete','backupReminderDismissed','backupReminderActive') are duplicated here and in the provider files. If someone updates a constant in a provider file but forgets to updatemain.dart, the pre-read values won't match the values the providers read/write, potentially reintroducing the race condition or causing subtle bugs.Consider exporting the key constants from the provider files or defining them in a shared location.
♻️ Example: export constants from provider files
In
first_run_provider.dart:-const _kFirstRunComplete = 'firstRunComplete'; +const kFirstRunComplete = 'firstRunComplete';In
backup_reminder_provider.dart:-const _kBackupReminderDismissed = 'backupReminderDismissed'; -const _kBackupReminderActive = 'backupReminderActive'; +const kBackupReminderDismissed = 'backupReminderDismissed'; +const kBackupReminderActive = 'backupReminderActive';Then in
main.dart:- final firstRunComplete = prefs.getBool('firstRunComplete') ?? false; - final backupDismissed = prefs.getBool('backupReminderDismissed') ?? false; - final backupActive = prefs.getBool('backupReminderActive') ?? false; + final firstRunComplete = prefs.getBool(kFirstRunComplete) ?? false; + final backupDismissed = prefs.getBool(kBackupReminderDismissed) ?? false; + final backupActive = prefs.getBool(kBackupReminderActive) ?? false;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/main.dart` around lines 24 - 28, Extract the SharedPreferences key strings into shared constants and import them into main.dart instead of using literal strings: define/export constants (e.g. FIRST_RUN_COMPLETE_KEY, BACKUP_REMINDER_DISMISSED_KEY, BACKUP_REMINDER_ACTIVE_KEY) from the provider modules (first_run_provider.dart and backup_reminder_provider.dart) or a new shared constants file, update all prefs.getBool calls in main.dart (the block using SharedPreferences.getInstance() and prefs.getBool(...)) to use these constants, and update the providers to reference the same constants so there is a single source of truth for those keys.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@lib/features/account/providers/backup_reminder_provider.dart`:
- Around line 17-21: The constructor for BackupReminderNotifier currently skips
load() when an initialValue is provided but leaves _loaded false; update the
BackupReminderNotifier constructor so that when initialValue is non-null it also
sets _loaded = true to reflect that state is already initialized, preventing
later calls (e.g., showBackupReminder()) from needlessly invoking load() and
re-reading SharedPreferences.
In `@lib/main.dart`:
- Around line 24-28: Extract the SharedPreferences key strings into shared
constants and import them into main.dart instead of using literal strings:
define/export constants (e.g. FIRST_RUN_COMPLETE_KEY,
BACKUP_REMINDER_DISMISSED_KEY, BACKUP_REMINDER_ACTIVE_KEY) from the provider
modules (first_run_provider.dart and backup_reminder_provider.dart) or a new
shared constants file, update all prefs.getBool calls in main.dart (the block
using SharedPreferences.getInstance() and prefs.getBool(...)) to use these
constants, and update the providers to reference the same constants so there is
a single source of truth for those keys.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 970f5edf-7f98-4b97-895e-ecdba7b67fe3
📒 Files selected for processing (3)
lib/features/account/providers/backup_reminder_provider.dartlib/features/walkthrough/providers/first_run_provider.dartlib/main.dart
…flow - Add IdentityService to manage identity lifecycle (create on first launch, reload on subsequent launches) via flutter_secure_storage - Initialize identity in main() before Nostr relay pool setup - Account screen: display real mnemonic words from secure storage instead of placeholder, fully mask words by default - Show backup checkbox alongside words only when backup is still pending; checkbox is the sole confirmation method and is permanent once ticked - Reset words/checkbox state when generating a new identity
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
lib/features/account/screens/account_screen.dart (1)
305-321: Add error handling for identity regeneration to prevent inconsistent state.The sequence
deleteAll()→initialize()→showBackupReminder()can fail at any step. IfdeleteAll()succeeds butinitialize()fails, the user loses their identity with no recovery path. Consider wrapping in try-catch with user feedback and rollback strategy.🛡️ Proposed error handling
onPressed: () async { Navigator.pop(context); // Reset local state before generating new identity. setState(() { _wordsVisible = false; _showBackupCheckbox = false; _words = null; }); - // Delete old secure storage data, then generate + store new identity. - await IdentityService.deleteAll(); - await IdentityService.initialize(); - await ref - .read(backupReminderProvider.notifier) - .showBackupReminder(); + try { + // Delete old secure storage data, then generate + store new identity. + await IdentityService.deleteAll(); + await IdentityService.initialize(); + await ref + .read(backupReminderProvider.notifier) + .showBackupReminder(); + } catch (e) { + if (!context.mounted) return; + ScaffoldMessenger.of(context).showSnackBar( + SnackBar(content: Text('Failed to generate new identity: $e')), + ); + return; + } if (!context.mounted) return; context.go(AppRoute.walkthrough); },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/features/account/screens/account_screen.dart` around lines 305 - 321, Wrap the identity rotation sequence inside a try-catch and avoid mutating UI/navigation until it succeeds: in the onPressed handler, perform the operations in a guarded flow (call IdentityService.deleteAll() and IdentityService.initialize() inside try), and on success run the existing backupReminderProvider.showBackupReminder(), setState updates, Navigator.pop and context.go(AppRoute.walkthrough); on failure catch the exception, log it, present user-facing feedback (Snackbar/Dialog) and do not navigate away; attempt a recovery step such as calling IdentityService.initialize() again or invoking a restore/rollback method if available, and ensure setState does not clear _words/_showBackupCheckbox unless the new identity was created successfully.lib/core/services/identity_service.dart (1)
43-67: Consider adding error handling for secure storage operations.The storage read/write/delete operations can throw exceptions (e.g., if the keychain/keystore is locked or corrupted). Currently, these exceptions would propagate to callers. This is acceptable if callers handle errors, but consider wrapping in try-catch with logging for better diagnostics during debugging.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/core/services/identity_service.dart` around lines 43 - 67, Wrap all secure storage operations on _storage in try-catch blocks to capture and log exceptions (including the key and the caught exception) rather than letting them silently propagate; for getMnemonicWords (use _kMnemonic) catch errors, log them and return an empty list as a safe default, for saveTradeKeyIndex (use _kTradeKeyIndex) and savePrivacyMode (use _kPrivacyMode) catch and log errors and rethrow if you want callers to handle failures (or return/complete silently if you prefer non-fatal), and for deleteAll catch and log failures per delete call so one failing delete doesn't prevent attempting the others; use your existing logger or create a simple logger instance and include the function name (getMnemonicWords, saveTradeKeyIndex, savePrivacyMode, deleteAll) and key names in the log messages for diagnostics.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@CLAUDE.md`:
- Line 11: Remove the duplicated Active Technologies bullet that repeats the
exact text "Sembast (Dart, all platforms) for UI-layer state; SQLite via `sqlx`
(Rust, native) / IndexedDB via `indexed_db_futures` (Rust, web) for
protocol-layer persistence. Feature-gated via `#[cfg(target_arch =
\"wasm32\")]`. (004-mostro-p2p-client)"; keep a single instance, ensure
surrounding bullets/spacing remain consistent, and run a quick scan for any
other accidental verbatim duplicates in the file.
- Line 33: Remove the duplicated Recent Changes entry by deleting the extra line
that reads "- 004-mostro-p2p-client: Added Rust stable 1.94+ (core); Dart 3.x /
Flutter 3.x (UI shell)" so that only one identical entry remains in the Recent
Changes section; locate the duplicate string in the CLAUDE.md Recent Changes
block and remove the redundant occurrence.
In `@specs/004-mostro-p2p-client/plan.md`:
- Around line 225-227: The plan.md file describes Rust-side backup confirmation
APIs and persistence that do not exist in the current implementation, which
instead manages backup state entirely in Dart with SharedPreferences and the
BackupReminderNotifier. To fix this, clarify the plan by indicating that the
described Rust APIs and persistence are planned future work or update the
plan.md content so it accurately reflects the current Dart-based architecture
using BackupReminderNotifier and backupReminderProvider instead of Rust-side
features.
---
Nitpick comments:
In `@lib/core/services/identity_service.dart`:
- Around line 43-67: Wrap all secure storage operations on _storage in try-catch
blocks to capture and log exceptions (including the key and the caught
exception) rather than letting them silently propagate; for getMnemonicWords
(use _kMnemonic) catch errors, log them and return an empty list as a safe
default, for saveTradeKeyIndex (use _kTradeKeyIndex) and savePrivacyMode (use
_kPrivacyMode) catch and log errors and rethrow if you want callers to handle
failures (or return/complete silently if you prefer non-fatal), and for
deleteAll catch and log failures per delete call so one failing delete doesn't
prevent attempting the others; use your existing logger or create a simple
logger instance and include the function name (getMnemonicWords,
saveTradeKeyIndex, savePrivacyMode, deleteAll) and key names in the log messages
for diagnostics.
In `@lib/features/account/screens/account_screen.dart`:
- Around line 305-321: Wrap the identity rotation sequence inside a try-catch
and avoid mutating UI/navigation until it succeeds: in the onPressed handler,
perform the operations in a guarded flow (call IdentityService.deleteAll() and
IdentityService.initialize() inside try), and on success run the existing
backupReminderProvider.showBackupReminder(), setState updates, Navigator.pop and
context.go(AppRoute.walkthrough); on failure catch the exception, log it,
present user-facing feedback (Snackbar/Dialog) and do not navigate away; attempt
a recovery step such as calling IdentityService.initialize() again or invoking a
restore/rollback method if available, and ensure setState does not clear
_words/_showBackupCheckbox unless the new identity was created successfully.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3ea265bb-5f37-4366-807d-565da1cf583b
📒 Files selected for processing (6)
CLAUDE.mdlib/core/services/identity_service.dartlib/features/account/screens/account_screen.dartspecs/004-mostro-p2p-client/plan.mdspecs/004-mostro-p2p-client/spec.mdspecs/004-mostro-p2p-client/tasks.md
✅ Files skipped from review due to trivial changes (1)
- specs/004-mostro-p2p-client/tasks.md
- CLAUDE.md: remove duplicate Active Technologies and Recent Changes entries - plan.md: clarify backup_confirmed is currently Dart/SharedPreferences, Rust-side APIs are planned future work - IdentityService: wrap all secure storage calls in try-catch with diagnostic logging; deleteAll continues deleting remaining keys even if one fails - AccountScreen: guard identity rotation in try-catch; only clear state and navigate on success, show SnackBar on failure
…re-seeded - Expose kFirstRunCompleteKey, kBackupReminderDismissedKey, kBackupReminderActiveKey as public constants so main() shares a single source of truth instead of duplicating literal strings - Set _loaded = true in BackupReminderNotifier when initialValue is provided, preventing showBackupReminder() from re-reading SharedPreferences unnecessarily
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
lib/features/account/screens/account_screen.dart (1)
322-330: Consider showing a user-friendly error message instead of raw exception.Exposing the raw exception
$ein the SnackBar may leak internal implementation details or confusing technical jargon to users. ThedebugPrintalready logs the technical details for debugging.💡 Suggested improvement
} catch (e) { debugPrint('[account] generateNewUser error: $e'); if (!context.mounted) return; ScaffoldMessenger.of(context).showSnackBar( - SnackBar( - content: Text('Failed to generate new identity: $e'), - ), + const SnackBar( + content: Text('Failed to generate new identity. Please try again.'), + ), ); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/features/account/screens/account_screen.dart` around lines 322 - 330, The catch block handling generateNewUser currently shows the raw exception in the SnackBar; keep the debugPrint(debugPrint('[account] generateNewUser error: $e')) for diagnostics but change the ScaffoldMessenger.of(context).showSnackBar invocation to display a user-friendly message (e.g., "Failed to generate identity. Please try again.") instead of "$e"; optionally include the raw error only in debug builds (wrap extra details with kDebugMode) and keep the early context.mounted check as-is.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@lib/features/account/screens/account_screen.dart`:
- Around line 322-330: The catch block handling generateNewUser currently shows
the raw exception in the SnackBar; keep the debugPrint(debugPrint('[account]
generateNewUser error: $e')) for diagnostics but change the
ScaffoldMessenger.of(context).showSnackBar invocation to display a user-friendly
message (e.g., "Failed to generate identity. Please try again.") instead of
"$e"; optionally include the raw error only in debug builds (wrap extra details
with kDebugMode) and keep the early context.mounted check as-is.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 55f4bcb9-0912-49d4-be07-d3b3583702f6
📒 Files selected for processing (4)
CLAUDE.mdlib/core/services/identity_service.dartlib/features/account/screens/account_screen.dartspecs/004-mostro-p2p-client/plan.md
✅ Files skipped from review due to trivial changes (1)
- CLAUDE.md
🚧 Files skipped from review as they are similar to previous changes (2)
- specs/004-mostro-p2p-client/plan.md
- lib/core/services/identity_service.dart
identity_service.dart: - Write mnemonic to secure storage before metadata so a partial failure never leaves the user without recoverable keys - Add regenerate(): creates new identity and overwrites storage atomically, replacing the deleteAll→initialize sequence that had a data-loss window account_screen.dart: - _loadAndRevealWords: add try/catch; surface a SnackBar on error or when no identity is found instead of silently showing stale words - _confirmBackup: wrap in try/catch with SnackBar on failure - _confirmGenerateNewUser: use IdentityService.regenerate() so the old identity is only replaced once the new one is safely persisted - _BackupConfirmRow: change onConfirm to Future<void> Function(); only lock the checkbox on success, show a spinner while pending, leave unchecked on failure so the user can retry
spec.md: - Add FR-002a: define blocking error behavior when secure storage persistence fails on first launch plan.md: - Resolve contradiction between objectives (Dart) and Key Files (Rust): Key Files now documents the current Dart/SharedPreferences implementation and marks Rust API additions as planned future work - Update Constitution compliance to reflect Dart-based tracking - Update Acceptance Tests to reference Dart symbols tasks.md: - T147: add l10n key requirement for the backup checkbox label
Show a user-friendly message in production; include the exception detail only in debug builds via kDebugMode.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/features/account/screens/account_screen.dart`:
- Around line 54-59: In the catch blocks (e.g., in _loadAndRevealWords and the
other error handler around lines 73-75) stop showing the raw exception ($e) in
the SnackBar in release builds: import flutter/foundation and use kDebugMode to
decide message text — keep debugPrint or processLogger logging with the full
error, but pass a generic user-facing string (e.g., "Failed to load secret
words" or "An error occurred") to ScaffoldMessenger.of(context).showSnackBar
when not kDebugMode; when kDebugMode is true include the exception details in
the SnackBar for debugging.
- Around line 66-77: The try/catch in _confirmBackup is swallowing errors and
causing _BackupConfirmRowState._handleConfirm to treat failures as success;
remove the catch (or rethrow the caught exception) so that errors from
ref.read(backupReminderProvider.notifier).confirmBackupComplete() propagate to
the caller, and only update UI state (_showBackupCheckbox) after the awaited
call completes successfully; reference the _confirmBackup function,
confirmBackupComplete on backupReminderProvider.notifier, and the
_showBackupCheckbox state to locate and fix the logic.
In `@specs/004-mostro-p2p-client/plan.md`:
- Around line 284-285: Update the checklist so it reflects the new behavior
where the pending-backup state is already active after the first identity is
created: change the line referring to "Fresh install: identity is generated
before walkthrough renders; `backupReminderProvider` is `false`" to indicate
that `backupReminderProvider` is true (or mark that checklist item as already
satisfied/checked) so the spec matches the implementation that sets
pending-backup immediately after identity creation.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c3526449-7099-48d1-b44a-33c042e42f9e
📒 Files selected for processing (8)
lib/core/services/identity_service.dartlib/features/account/providers/backup_reminder_provider.dartlib/features/account/screens/account_screen.dartlib/features/walkthrough/providers/first_run_provider.dartlib/main.dartspecs/004-mostro-p2p-client/plan.mdspecs/004-mostro-p2p-client/spec.mdspecs/004-mostro-p2p-client/tasks.md
✅ Files skipped from review due to trivial changes (1)
- specs/004-mostro-p2p-client/tasks.md
🚧 Files skipped from review as they are similar to previous changes (3)
- lib/main.dart
- lib/features/account/providers/backup_reminder_provider.dart
- lib/features/walkthrough/providers/first_run_provider.dart
- _loadAndRevealWords catch: raw $e now only shown in debug builds - _confirmBackup: adds rethrow after showing the SnackBar — _handleConfirm now correctly sees the failure and leaves the checkbox unchecked for retry - plan.md line 284: checklist item corrected — backupReminderProvider is true (not false) after walkthrough completes
This was referenced Apr 1, 2026
4 tasks
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
1 participant
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.
Pre-read SharedPreferences in main() before runApp() and inject firstRunProvider and backupReminderProvider with synchronous initial values via ProviderScope.overrides.
Previously both notifiers started as AsyncValue.loading(), causing the router to skip the walkthrough redirect on first launch and incorrectly routing bell taps to /walkthrough instead of /notifications.
Summary by CodeRabbit
New Features
Refactor