perf(auth): optimize session expiry transition to login

[syed-tp]

• Dismiss the session expired dialog immediately and update auth state upfront on logout.
• Runs database cleanup and network requests asynchronously to avoid UI jank.

Note: OpenSpec workflow was skipped intentionally as this is an internal performance and transition timing optimization with no changes to the public API or auth specifications.

[claude]

Claude finished @syed-tp's task in 2m 30s —— View job

---

PR Review Summary

✅ What's good

• The UX intent is solid: previously onSignIn awaited logout() before clearing sessionExpiredProvider, so a failed logout would leave the "session expired" dialog stuck open indefinitely. Dismissing it unconditionally first (app_router.dart:163-166) fixes that real bug.
• auth_repository.dart now skips the network logout call when there's no token (if (token != null && token.isNotEmpty)), avoiding a pointless failing request.
• Small, focused diff (7/-7 across 3 files), consistent with the "internal perf/timing optimization" framing — no OpenSpec change needed here per the exemption for refactors/perf changes that don't add new behavior/API surface.
• No SDK-boundary, design-system, accessibility, motion, or navigation-pattern violations — the diff only touches provider/repository logic and an existing callback wiring.

🔴 CRITICAL — Must fix before merge

1. Race condition: backgrounded logout cleanup can wipe a fast re-login's data. auth_provider.dart:130 now sets state = const AsyncData(false) before awaiting userRepo.clearCurrentUser(), resetUseCase.execute(), and _repository.logout(). bootstrap_provider.dart treats authState.valueOrNull == true as the only "authenticated" signal, so the router redirects to the login screen the instant this state flips — while cleanup is still running in the background.

   • UserRepository.clearCurrentUser() (packages/core/lib/data/repositories/user_repository.dart:16-18) does an unconditional _db.delete(_db.usersTable).go() (not scoped by user id).
   • AppResetUseCase.execute() (packages/core/lib/domain/usecases/app_reset_use_case.dart:21-24) purges all downloads and the entire local database.

   If the user (or a different user on a shared device) logs back in while this is still in flight, the stale cleanup from the previous logout can delete the new session's freshly-written user row, cached DB data, or downloads. The "Safety net" comment above clearCurrentUser() even acknowledges the full purge can fail mid-way — that failure window is now exposed to the UI/user instead of being hidden behind the auth-state transition. See inline comment on auth_provider.dart:130 for suggested fixes (await pending cleanup before a new login proceeds, or decouple the dialog-dismiss timing from the auth-state flip).

🟡 WARNING — Should fix

1. Offline data consistency (rule 16). Related to the above: the Repository layer is supposed to be the consistent source of truth for cached data, but this change creates a window where cache state can be corrupted by overlapping logout/login cleanup instead of being serialized.

🔵 SUGGESTION — Nice to have

1. Consider adding a regression test in packages/core/test/data/auth/auth_provider_test.dart asserting that authProvider state flips to false synchronously on logout() call (documenting the new intentional ordering), and/or a test guarding against the login-during-cleanup race once a fix is in place.

Verdict

CHANGES REQUESTED
· Branch

[syed-tp]

@claude review

[claude]

Claude finished @syed-tp's task in 1m 24s —— View job

---

Todo

• Read repo guidelines (AGENTS.md, core docs)
• Fetch PR diff and review changed files
• Check whether prior CRITICAL race-condition feedback was addressed
• Post review summary

PR Review Summary

✅ What's good

• The previous CRITICAL race condition (state flipping to AsyncData(false) before cleanup, allowing a fast re-login to collide with in-flight clearCurrentUser()/AppResetUseCase.execute()) has been properly fixed in auth_provider.dart:65-165: a _cleanupFuture is tracked on logout() and every login method (loginWithPassword, loginWithGoogle, register, verifyOtp) now awaits it before writing new session data. This closes the data-corruption window flagged earlier.
• The two new regression tests in auth_provider_test.dart:117-204 are well constructed — they use Completer-gated mocks to deterministically prove (a) state flips synchronously before cleanup finishes, and (b) a concurrent login genuinely blocks until cleanup completes, rather than just asserting end states.
• Moving the SessionExpiredDialog overlay from _AppShellBuilder (app_router.dart) into MaterialApp.router's builder in main.dart:129-149 is a real improvement — it now renders above the Navigator for every route (not just inside the authenticated shell), so it can't be clipped by a route transition or miss non-shell screens.
• onSignIn now clears sessionExpiredProvider before calling logout() (main.dart:139-141), so the dialog dismisses immediately regardless of how long cleanup takes — matches the PR's stated intent.
• auth_repository.dart:122 skipping the network logout() call when there's no token avoids a pointless failing request.
• AuthRepository.logout()'s local-then-remote ordering and the _runCleanup() try/catch with Sentry reporting (auth_provider.dart:146-164) is a reasonable safety net for the case cleanup partially fails.
• Small, focused diff; OpenSpec is correctly skipped here — this is a refactor/perf change to an already-specified feature (openspec/specs/session-expired-dialog/), not new behavior or public API.

🔴 CRITICAL — Must fix before merge

None found.

🟡 WARNING — Should fix

None found.

🔵 SUGGESTION — Nice to have

1. Overlapping logout() calls re-kick _runCleanup(). If logout() is invoked twice before the first cleanup finishes (e.g. the session-expired 401 fires again for an in-flight request while cleanup is still running, or a user could plausibly trigger both the session-expired dialog and the manual logout sheet in quick succession), a second _runCleanup() runs concurrently with the first — both calling clearCurrentUser()/resetUseCase.execute()/_repository.logout() at once. Harmless today since the operations are idempotent-ish, but worth a short-circuit (e.g. skip re-invoking _runCleanup if _cleanupFuture is already pending) if this pattern grows.

Verdict

APPROVED

Branch