feat(core): graceful 401 session expiry dialog

[syed-tp]

Replace silent auto-logout with a blocking SessionExpiredDialog overlay.
When a 401 is detected, show the backend's error message in a non-dismissible
dialog — user must tap "Login Again" to trigger logout explicitly.

• Add sessionExpiredProvider to hold the 401 error message globally
• Update AuthInterceptor to extract and pass the backend message
• Wire dioProvider to set sessionExpiredProvider on 401 instead of logging out
• Create SessionExpiredDialog using design tokens and l10n (EN, AR, ML, TA)
• Overlay dialog in _AppShellBuilder above the app shell via Stack
• Suppress 401 errors in course sync/search providers to prevent error banners
• Redirect directly to /login on logout, bypassing the onboarding splash screen
• Fix ApiException.extractApiMessage to recursively decode JSON string responses

[gemini-code-assist]

Caution

The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased.

[claude]

Claude finished @syed-tp's task in 4m 52s —— View job

---

PR Review: graceful 401 session expiry dialog

• Read repo guideline docs (AGENTS.md, ai_context.md, architecture.md)
• Check for relevant ADRs
• Fetch full PR diff
• Review OpenSpec compliance
• Review SDK boundary enforcement
• Review design system / widget usage
• Review accessibility semantics
• Review motion & animation
• Review platform neutrality
• Review navigation (GoRouter)
• Review Riverpod provider pattern
• Review data layer architecture
• Review generated files
• Review localization
• Review icon library usage
• Post structured review summary

---

PR Review Summary

✅ What's good

• Clean OpenSpec change (openspec/changes/graceful-401-session-dialog/) with proposal, design doc, and spec — good compliance with the mandatory spec-driven workflow for this new capability.
• SessionExpiredDialog (packages/core/lib/widgets/session_expired_dialog.dart) correctly uses Design.of(context) tokens, AppText/AppButton primitives, LucideIcons, and L10n.of(context) — no Material/Cupertino widgets, no static token imports.
• PopScope(canPop: false) correctly makes the dialog non-dismissible per the spec.
• AuthInterceptor boundary/import rules respected; _isLoggingOut guard correctly prevents duplicate dialog triggers, and auth-flow paths / logout requests are correctly excluded.
• ApiException.extractApiMessage recursive-JSON-string fix (packages/core/lib/data/exceptions/api_exception.dart:155-169) is a solid, well-contained bug fix.
• All 4 locales (EN/AR/ML/TA) updated consistently for the new strings, and generated app_localizations*.dart files match their .arb sources.

🔴 CRITICAL — Must fix before merge

1. Search UI gets stuck in a permanent loading state after a 401. In packages/courses/lib/providers/course_list_provider.dart:271-278 (CourseSearch._performSearch()), search() sets isLoading: true before calling _performSearch(). On a suppressed 401 (e is ApiException && e.type == ApiErrorType.unauthorized), the catch block does an early return without resetting isLoading back to false (unlike the non-401 path, which calls state.copyWith(error: e, isLoading: false)). This leaves CourseSearchState.isLoading stuck true forever whenever a session expires mid-search, so the search screen shows a permanent spinner even after the user signs back in. Note: the analogous CourseList._performSync() doesn't have this issue since its loading flags (isSyncingInitialPage/isSyncingMoreResults) are reset unconditionally in a finally block.
   Fix this →

🟡 WARNING — Should fix

1. Fallback session-expiry message is hardcoded, not localized. AuthInterceptor._fallbackMessage (packages/core/lib/network/auth_interceptor.dart:13-14) is a hardcoded English string used when the backend response has no extractable message. Meanwhile, a sessionExpiredFallbackMessage key was added to all four .arb files (app_en/ar/ml/ta.arb) and generated, but it's never referenced anywhere in the actual code (SessionExpiredDialog just renders whatever message it receives). So non-English users will see an English-only fallback message on session expiry, and the new l10n key is dead code. Consider resolving the fallback in SessionExpiredDialog via l10n.sessionExpiredFallbackMessage (e.g. pass a sentinel/null through instead of baking the English string into the interceptor) so it's localized at render time.
2. No test coverage added for any part of this change — no widget test for SessionExpiredDialog, no test for the AuthInterceptor.onSessionExpired callback rename/behavior, and no test for the AuthRoutes.redirect first-launch/logout-redirect logic. Given this touches auth/session flows, at least a widget test for the dialog and a unit test for the interceptor's 401 handling would be valuable.
3. AuthRoutes._isFirstLaunch (packages/testpress/lib/navigation/routes/auth_routes.dart:16) is mutable static state outside Riverpod. It tracks "has onboarding been shown yet" as a static class field rather than through a provider, which is inconsistent with how the rest of the app manages UI/navigation state and won't reset between widget tests running in the same isolate (risk of test pollution / flakiness). Consider a StateProvider<bool> or persisting this via existing app-state infra instead.

🔵 SUGGESTION — Nice to have

1. sessionExpiredProvider (packages/core/lib/data/auth/auth_provider.dart:49) is a manual StateProvider<String?>. This is consistent with existing simple UI-flag providers in the same file/pattern (e.g. isLogoutSheetOpenProvider), so it's not out of place, but since it's genuinely UI state (drives the dialog overlay) rather than DI/infrastructure wiring, consider migrating it (and its siblings) to @riverpod codegen for consistency with the project's stated provider preference.

Verdict

CHANGES REQUESTED

[syed-tp]

@claude review

[claude]

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

---

Claude finished @syed-tp's task — View job

• Read repo guideline docs (AGENTS.md, ai_context.md, architecture.md)
• Check for relevant ADRs
• Fetch full PR diff
• Review OpenSpec compliance
• Review SDK boundary enforcement
• Review design system / widget usage
• Review accessibility semantics
• Review motion & animation
• Review platform neutrality
• Review navigation (GoRouter)
• Review Riverpod provider pattern
• Review data layer architecture
• Review generated files
• Review localization
• Review icon library usage
• Post structured review summary

---

PR Review Summary

This is a re-review of the current HEAD (8cf821a). The two CRITICAL/WARNING items from the earlier automated pass have both been addressed since then:

• The CourseSearch._performSearch() stuck-spinner bug is fixed — packages/courses/lib/providers/course_list_provider.dart:275-277 now resets isLoading: false before the early return on a suppressed 401.
• The mutable static _isFirstLaunch field is gone — replaced with a real hasShownOnboardingProvider (StateProvider<bool>) in packages/core/lib/data/auth/auth_provider.dart:52.

✅ What's good

• sessionExpiredFallbackMessage is now actually wired up — SessionExpiredDialog (packages/core/lib/widgets/session_expired_dialog.dart:96) renders it when message.isEmpty, so the l10n key added in 4. is no longer dead code.
• SessionExpiredDialog correctly uses Design.of(context) tokens, AppText/AppButton primitives, LucideIcons.lock, L10n.of(context), and PopScope(canPop: false) for non-dismissibility — no Material/Cupertino widgets.
• AuthRoutes.redirect (packages/testpress/lib/navigation/routes/auth_routes.dart) now reads/writes hasShownOnboardingProvider through the Riverpod container instead of static mutable state — resolves the test-pollution risk flagged earlier.
• ApiException.extractApiMessage recursive-JSON-string fix (packages/core/lib/data/exceptions/api_exception.dart:153-169) is a solid, well-contained bug fix.
• All 4 locales (EN/AR/ML/TA) and generated app_localizations*.dart files stay in sync with the new keys.

🔴 CRITICAL — Must fix before merge

None found.

🟡 WARNING — Should fix

1. The new localized fallback path is effectively unreachable for real 401s, so non-English users still see hardcoded English text. ApiException.fromDioException (packages/core/lib/data/exceptions/api_exception.dart:86-94) always supplies a non-null, non-empty fallback for status 401 — backendMessage ?? 'You are not authorized to perform this action.'. AuthInterceptor.onError (packages/core/lib/network/auth_interceptor.dart:59-62) passes this apiException.message straight through, so message is never actually empty by the time it reaches SessionExpiredDialog. That means SessionExpiredDialog's message.isNotEmpty ? message : l10n.sessionExpiredFallbackMessage branch (session_expired_dialog.dart:96) never fires in practice — a session-expiry response with no backend message body still shows the hardcoded English string "You are not authorized to perform this action." to AR/ML/TA users, not the localized sessionExpiredFallbackMessage. Consider having AuthInterceptor pass apiException.data == null && backendMessage == null (or a dedicated "no backend message" signal) through as an empty string / null instead of relying on ApiException's own English fallback, so the dialog's localization path is actually reachable.
   Fix this →
2. No test coverage added for this change — no widget test for SessionExpiredDialog, no unit test for AuthInterceptor's 401→onSessionExpired behavior, and no test for the new AuthRoutes.redirect onboarding/login-redirect branching. Given this touches auth/session flows and has already needed two rounds of bug fixes (stuck spinner, static state), a couple of regression tests would materially reduce risk here.

🔵 SUGGESTION — Nice to have

1. Possible onboarding regression for first-time deep-linked users. AuthRoutes.redirect (packages/testpress/lib/navigation/routes/auth_routes.dart:27-30) now sends any unauthenticated user hitting a non-auth route straight to /login, and onboarding only shows via the special-cased path == '/onboarding' branch. Since initialLocation is hardcoded to /onboarding in app_router.dart:22, this works fine for a normal cold start — but if the platform ever resolves a deep link to something other than /onboarding on first launch (e.g. a push-notification link into /course/123), a brand-new user would skip onboarding entirely and land on /login. Worth double-checking against how deep links are dispatched on this app, if at all.
2. sessionExpiredProvider / hasShownOnboardingProvider remain manual StateProviders rather than @riverpod codegen. Consistent with sibling providers like isLogoutSheetOpenProvider in the same file, so not a blocker, but worth keeping in mind if/when this file migrates to codegen wholesale.

Verdict

CHANGES REQUESTED — the WARNING items (dead localization fallback, no test coverage) are worth addressing, but nothing CRITICAL remains; the two previously-flagged CRITICAL/WARNING issues (stuck search spinner, static onboarding flag) have both been fixed in the latest commit.
· branch fix/auto-logout

[syed-tp]

@claude review

[claude]

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

---

PR Review: graceful 401 session expiry dialog

• Read repo guideline docs (AGENTS.md, ai_context.md, architecture.md)
• Check for relevant ADRs
• Fetch full PR diff / review new commit since last pass
• Review OpenSpec compliance
• Review SDK boundary enforcement
• Review design system / widget usage
• Review accessibility semantics
• Review motion & animation
• Review platform neutrality
• Review navigation (GoRouter)
• Review Riverpod provider pattern
• Review data layer architecture
• Review generated files
• Review localization
• Review icon library usage
• Post structured review summary

---

This is a re-review of the current HEAD (4198477). Only one commit was added since the last pass, and it resolves the previously-flagged WARNING about the unreachable localized fallback message.

PR Review Summary

✅ What's good

• ApiException.fromDioException (packages/core/lib/data/exceptions/api_exception.dart:86-97) now returns backendMessage ?? '' for 401s instead of a hardcoded English fallback string. Combined with AuthInterceptor.onError passing apiException.message straight through (packages/core/lib/network/auth_interceptor.dart:59-63) and SessionExpiredDialog rendering l10n.sessionExpiredFallbackMessage whenever message.isEmpty (packages/core/lib/widgets/session_expired_dialog.dart:100-104), the localized fallback path is now actually reachable end-to-end for AR/ML/TA users. This closes out the last remaining WARNING from the prior review round.
• Verified the two earlier CRITICAL/WARNING fixes are still intact: CourseSearch._performSearch() resets isLoading: false before the early return on a suppressed 401 (packages/courses/lib/providers/course_list_provider.dart:275-277), and AuthRoutes.redirect uses the real hasShownOnboardingProvider (Riverpod) instead of static mutable state (packages/testpress/lib/navigation/routes/auth_routes.dart:28-45).
• SessionExpiredDialog continues to correctly use Design.of(context) tokens, AppText/AppButton primitives, LucideIcons.lock, L10n.of(context), and PopScope(canPop: false) for non-dismissibility — no Material/Cupertino widgets.
• All 4 locales (EN/AR/ML/TA) and generated app_localizations*.dart files remain in sync with the sessionExpiredFallbackMessage key.
• OpenSpec change (openspec/changes/graceful-401-session-dialog/) is present with proposal, design, and spec docs — good compliance for this new capability.

🔴 CRITICAL — Must fix before merge

None found.

🟡 WARNING — Should fix

1. Still no test coverage added for this change — no widget test for SessionExpiredDialog, no unit test for AuthInterceptor's 401→onSessionExpired behavior (including the new empty-string-fallback path), and no test for AuthRoutes.redirect's onboarding/login-redirect branching. This flow has already needed two rounds of bug fixes (stuck spinner, static state, unreachable fallback), which is itself a signal that regression tests would materially reduce risk here going forward.

🔵 SUGGESTION — Nice to have

1. Possible onboarding regression for first-time deep-linked users (unchanged from last review). AuthRoutes.redirect (packages/testpress/lib/navigation/routes/auth_routes.dart:27-29) sends any unauthenticated user hitting a non-auth route straight to /login; onboarding only shows via the special-cased path == '/onboarding' branch. Fine for a normal cold start since initialLocation is hardcoded to /onboarding, but worth double-checking against how deep links (e.g. push notifications) are dispatched on first launch, if at all.
2. sessionExpiredProvider / hasShownOnboardingProvider remain manual StateProviders rather than @riverpod codegen. Consistent with sibling providers like isLogoutSheetOpenProvider in the same file, so not a blocker.

Verdict

APPROVED — all CRITICAL and blocking WARNING items from earlier rounds are resolved. The one remaining WARNING (no test coverage) and the SUGGESTIONS are non-blocking but worth addressing in a fast-follow given this touches auth/session flows.
· branch fix/auto-logout