fix(navigation): eliminate cold-start splash overlap by reading auth state before runApp

[syed-tp]

• FlutterSecureStorage is async, so goRouterProvider had no way to know auth state at construction — initialLocation was always /onboarding. On fast devices, the router immediately redirected to /home, causing both screens to render simultaneously (the overlap).
• Read the auth token from FlutterSecureStorage in main() before runApp(). Pass the result via cachedAuthFlagProvider so goRouterProvider can set initialLocation = '/home' on the first frame for authenticated users. OnboardingScreen is never mounted on cold start — no transition, no overlap. authProvider still performs full async verification after launch.

[claude]

Claude finished @syed-tp's task in 3m 38s —— View job

---

Claude finished reviewing

Todo list:

• Read AGENTS.md and core docs (ai_context.md, architecture.md)
• Check ADRs / openspec context for the bootstrap/navigation architecture this touches
• Fetch full PR diff
• Review against SDK boundary, provider pattern, generated files, dependency, and general code quality rules
• Post structured review summary

PR Review Summary

✅ What's good

• Small, focused diff that targets a real, well-described cold-start UX bug.
• Keeps authProvider as the async source of truth for actual auth verification — the pre-boot read is only used as a fast-path hint, not a replacement.
• cachedAuthFlagProvider follows the existing manual-Provider DI pattern used by sharedPreferencesProvider right next to it (fine per this repo's Riverpod conventions).
• No Material/Cupertino widgets, design tokens, or SDK-boundary imports touched — this is plumbing-only, so most UI rubric items don't apply.

🔴 CRITICAL — Must fix before merge

1. The fix likely doesn't change actual cold-start behavior, because it doesn't account for an existing loading-gate. AuthRoutes.redirect (packages/testpress/lib/navigation/routes/auth_routes.dart:27-29, not itself changed in this PR but directly interacts with it) forces any non-/onboarding path back to /onboarding whenever bootstrapProvider is BootstrapState.loading. bootstrapProvider watches authProvider, whose build() (packages/core/lib/data/auth/auth_provider.dart:55-58) is genuinely async (await _repository.isUserLoggedIn(), itself an async secure-storage read). On cold start, this cannot have resolved by the time GoRouter evaluates its very first redirect call — so bootstrapState is loading at that point regardless of cachedAuthFlagProvider or device speed.
   With initialLocation now /home for cached-authenticated users, the very first redirect call sees path == '/home' while bootstrapState == loading and immediately forces it back to /onboarding — before the first frame paints. OnboardingScreen still mounts on cold start. Once bootstrapProvider resolves to authenticated, the router.refresh() listener (app_router.dart:54-56) fires and redirects /onboarding → /home again — the same flash this PR set out to remove.
   Fix needs one of: (a) make the loading branch in AuthRoutes.redirect respect cachedAuthFlagProvider and skip forcing /onboarding, or (b) seed authProvider's initial synchronous state from the same pre-boot check so bootstrapProvider isn't loading on the first pass. Please verify with a widget test asserting the first resolved route for a cached-authenticated cold start is /home, not /onboarding. (Left as an inline comment on auth_routes.dart, but GitHub couldn't attach it there since that file isn't part of this diff — restating here.)
2. Unguarded secure-storage read can crash app startup — app/lib/main.dart:14-19. secureStorage.read(key: 'auth_token') has no try/catch. AuthLocalDataSource.getToken() (packages/core/lib/data/auth/auth_local_data_source.dart:18-30) wraps the identical read specifically because flutter_secure_storage can throw (e.g. known Android Keystore invalidation after OS/backup restore). Since this runs before runApp(), an uncaught exception here means total launch failure for any affected user instead of a graceful fallback to unauthenticated. See inline comment on main.dart.

🟡 WARNING — Should fix

3. Duplicated auth-check logic — app/lib/main.dart:14-19 re-implements AuthLocalDataSource.isUserLoggedIn() from scratch (same hardcoded 'auth_token' key, same null/empty-trim check) instead of reusing it. Two copies of this logic can silently drift out of sync, which is exactly the kind of inconsistency this PR is trying to eliminate. Prefer await AuthLocalDataSource().isUserLoggedIn().
4. app/ performing raw platform I/O directly — per AGENTS.md, app/ is meant to be a consumer shell that depends on package:testpress, not talk to third-party plugins or duplicate packages/core internals directly. Consider exposing a small pre-boot helper from testpress/core instead of importing flutter_secure_storage straight into main.dart.
5. cachedAuthFlagProvider placed in shared_preferences_provider.dart (packages/core/lib/data/providers/shared_preferences_provider.dart) — unrelated to SharedPreferences; consider colocating with auth_provider.dart/auth_local_data_source.dart for discoverability.
6. No test coverage for the new cachedAuthFlagProvider → initialLocation behavior, despite an existing packages/testpress/test/navigation/app_router_test.dart. A test here would likely have caught finding feat(ui): implement LMS core primitives #1 above.
7. This changes runtime behavior of the app-bootstrap capability, which has a dedicated spec from a prior change (openspec/specs/app-bootstrap/spec.md, from openspec/changes/archive/2026-08-25-fix-splash-race) establishing "router blocks until bootstrap explicitly transitions out of loading." Since this is framed as a bug fix it's not required to go through OpenSpec, but the spec/design doc is now slightly stale relative to the new pre-boot signal — worth a follow-up note or spec update once feat(ui): implement LMS core primitives #1 is resolved.

🔵 SUGGESTION — Nice to have

8. flutter_secure_storage is promoted from transitive to direct dependency in app/pubspec.yaml — reasonable since it's already used transitively via packages/core, but a one-line justification in the PR description would match the repo's dependency-management guidance.

Verdict

CHANGES REQUESTED — primarily #1: as written, the router-level fix appears to be neutralized by the existing bootstrapProvider loading-gate, so the described overlap may not actually be resolved. Recommend confirming actual behavior on a physical/fast device (or via a widget test) before merging.

[syed-tp]

@claude review

[claude]

Claude finished @syed-tp's task in 3m 14s —— View job

---

Claude finished @syed-tp's review request — this is a re-review of the branch after the fixes pushed in commits 7027f00 and 201a616, which were made in response to the earlier automated review's CRITICAL findings.

Todo list:

• Read AGENTS.md and core docs (ai_context.md, architecture.md)
• Check ADRs / openspec context for the bootstrap/navigation architecture this touches
• Fetch full PR diff and review current state (previous review flagged critical issues — checking if addressed)
• Review against SDK boundary, provider pattern, generated files, dependency, and general code quality rules
• Post structured review summary

PR Review Summary

✅ What's good

• All previously-flagged CRITICAL issues are now resolved:
  • AuthRoutes.redirect (packages/testpress/lib/navigation/routes/auth_routes.dart:26-37) now checks cachedAuthFlagProvider inside the BootstrapState.loading branch, so a cached-authenticated cold start no longer gets bounced from /home back to /onboarding before authProvider resolves. This was the core bug that would have neutralized the fix — good catch and fix.
  • The pre-boot secure-storage read is no longer duplicated/unguarded. AuthLocalDataSource.checkCachedLogin() (packages/core/lib/data/auth/auth_local_data_source.dart:12-18) wraps the existing isUserLoggedIn() in try/catch and returns false on any storage failure (e.g. Keystore invalidation), so main() (app/lib/main.dart:13-16) can no longer crash app launch, and the token-key/empty-check logic lives in exactly one place.
• cachedAuthFlagProvider moved to auth_provider.dart (colocated with the auth state it feeds), and app/lib/main.dart no longer imports flutter_secure_storage directly — app/pubspec.yaml's direct dependency on it was reverted, keeping app/ as a package:testpress-only consumer per AGENTS.md.
• New tests in packages/testpress/test/navigation/app_router_test.dart cover both the initialLocation selection and the AuthRoutes.redirect loading-gate behavior, following the same Auth subclass mocking pattern as the existing bootstrap_provider_test.dart. This is exactly the regression test that would have caught the original bug.
• authProvider still performs full async verification post-launch — the pre-boot check is only a fast-path routing hint, consistent with the PR's stated design.
• SDK boundary is respected: AuthLocalDataSource/cachedAuthFlagProvider reach app/ only through package:testpress/testpress.dart → package:exams/exams.dart → package:core/data/data.dart (unfiltered re-export), the same existing chain sharedPreferencesProvider already relies on — not a new boundary violation.

🟡 WARNING — Should fix (non-blocking)

1. Expected UX edge case, not yet called out anywhere: if a user has a stale/invalid cached token (e.g. revoked server-side, expired, or logged out from another device), the router now shows /home immediately on cold start (via cachedAuthFlagProvider), then once authProvider resolves to unauthenticated, bootstrapProvider flips and the user is redirected /home → /login. This is a reasonable tradeoff for the fast path but is a visible (if brief) flash for that specific user segment — worth a short mention in the PR description so reviewers/QA know it's expected, and worth confirming /login (not a jarring full app reset) is an acceptable transition target.
2. This changes runtime behavior of the app-bootstrap capability, which has a spec at openspec/specs/app-bootstrap/spec.md (from the archived 2026-08-25-fix-splash-race change). The spec's "loading" scenario doesn't yet describe the new cached-auth fast path through the loading gate. Not required for this bug-fix PR, but worth a follow-up spec update so the spec doesn't drift from behavior.

🔵 SUGGESTION — Nice to have

3. app/pubspec.yaml has a stray extra blank line left between shared_preferences: ^2.3.0 and dev_dependencies: (a leftover from removing the flutter_secure_storage entry). Purely cosmetic.

Note on verification

flutter/dart CLI is not available in this environment, so I wasn't able to run flutter analyze or flutter test to confirm the new tests actually pass — I verified correctness by reading bootstrap_provider.dart, the generated auth_provider.g.dart (authProvider = AsyncNotifierProvider<Auth, bool>), and the existing bootstrap_provider_test.dart to confirm the new tests' mocking pattern and provider overrides are structurally consistent and should behave as intended. If you'd like this actually executed, either share CI output or grant test-running permissions.

Verdict

APPROVED — the CRITICAL router/loading-gate bug and the unguarded-storage-read crash risk from the previous review are both fixed, with regression tests added. Remaining items above are non-blocking follow-ups.
(branch)