From a31c8a61c64be98da813feb8a2940125397a4778 Mon Sep 17 00:00:00 2001 From: Julian Dice <19397727+windoze95@users.noreply.github.com> Date: Sat, 27 Jun 2026 13:29:12 -0500 Subject: [PATCH] fix: smooth out iOS core-loop friction (401 re-auth, player exit, HQ swap) Three friction fixes in the core watch/auth loop. #11 - Global 401 -> graceful re-auth. The Dio onError interceptor now detects a real 401 on any protected (non-auth) endpoint and hands off to AuthNotifier.handleSessionExpired(), which resets to the profile picker (currentUser -> null drives the GoRouter redirect to '/'), clears the stored session, disconnects the websocket, and surfaces a "Session expired - sign in again" SnackBar via a new app-wide ScaffoldMessenger key. Auth/credential endpoints (everything under /api/auth/) are excluded so bad-PIN 401/403s still surface inline. A burst of concurrent 401s resets and notifies exactly once; connection errors (no response) and non-auth 403s do not trigger it. #12 + #15 - Instant player exit + single progress save. _navigateBack now fires the final /progress save unawaited and pops immediately, so leaving is instant regardless of network. A _progressSavedOnExit guard stops dispose() from sending a second save: teardown is exactly one PUT. #19 - Preview->HQ disposal crash. _switchToHq now defers the old preview controller's pause/dispose to a post-frame callback, so the outgoing VideoPlayer never reads a disposed controller in the same frame. Verified: dart format clean, flutter analyze (--fatal-infos --fatal-warnings) clean, flutter test green (72 tests, +3 for handleSessionExpired). Co-Authored-By: Claude Opus 4.8 (1M context) Claude-Session: https://claude.ai/code/session_01RXMKM1rDWn8wNh93MMUtxY --- lib/app.dart | 2 ++ lib/config/app_globals.dart | 16 +++++++++ lib/providers/auth_provider.dart | 29 ++++++++++++++++ lib/screens/video_player_screen.dart | 30 ++++++++++------ lib/services/api_service.dart | 22 ++++++++++++ test/unit/auth_notifier_test.dart | 52 ++++++++++++++++++++++++++++ 6 files changed, 140 insertions(+), 11 deletions(-) create mode 100644 lib/config/app_globals.dart diff --git a/lib/app.dart b/lib/app.dart index ddf5379..0f1c405 100644 --- a/lib/app.dart +++ b/lib/app.dart @@ -1,5 +1,6 @@ import 'package:flutter/material.dart'; import 'package:flutter_riverpod/flutter_riverpod.dart'; +import 'config/app_globals.dart'; import 'config/theme.dart'; import 'config/routes.dart'; @@ -12,6 +13,7 @@ class NullFeedApp extends ConsumerWidget { return MaterialApp.router( title: 'NullFeed', + scaffoldMessengerKey: scaffoldMessengerKey, theme: NullFeedTheme.darkTheme, darkTheme: NullFeedTheme.darkTheme, themeMode: ThemeMode.dark, diff --git a/lib/config/app_globals.dart b/lib/config/app_globals.dart new file mode 100644 index 0000000..929a950 --- /dev/null +++ b/lib/config/app_globals.dart @@ -0,0 +1,16 @@ +import 'package:flutter/material.dart'; + +/// App-wide [ScaffoldMessengerState] key. +/// +/// Lets code that has no [BuildContext] — e.g. the API layer reacting to a +/// 401, or a Riverpod notifier — surface a SnackBar. Wired into [MaterialApp] +/// via `scaffoldMessengerKey`. +final scaffoldMessengerKey = GlobalKey(); + +/// Shows [message] in a SnackBar via [scaffoldMessengerKey], replacing any +/// SnackBar already on screen. A no-op until the messenger is mounted. +void showGlobalSnackBar(String message) { + scaffoldMessengerKey.currentState + ?..hideCurrentSnackBar() + ..showSnackBar(SnackBar(content: Text(message))); +} diff --git a/lib/providers/auth_provider.dart b/lib/providers/auth_provider.dart index 2756393..f2e0cb5 100644 --- a/lib/providers/auth_provider.dart +++ b/lib/providers/auth_provider.dart @@ -1,4 +1,7 @@ +import 'dart:async'; + import 'package:flutter_riverpod/flutter_riverpod.dart'; +import '../config/app_globals.dart'; import '../models/user.dart'; import '../services/api_service.dart'; import '../services/storage_service.dart'; @@ -45,6 +48,9 @@ class AuthState { class AuthNotifier extends Notifier { @override AuthState build() { + // Let the API layer hand a dead-session 401 back to us so any screen can + // recover gracefully instead of dead-ending. + _api.onUnauthorized = _onApiUnauthorized; // Deferred so the synchronous part of _restoreSession (which writes // `state`) runs after build() returns and the provider is initialized. Future.microtask(_restoreSession); @@ -236,6 +242,29 @@ class AuthNotifier extends Notifier { } } + /// Wired to [ApiService.onUnauthorized]: resets to the picker and tells the + /// user once, even if several requests 401 at the same time. + void _onApiUnauthorized() { + if (handleSessionExpired()) { + showGlobalSnackBar('Session expired — sign in again'); + } + } + + /// Tears down a signed-in session locally after the server rejected it + /// (a 401 on a protected endpoint). Mirrors [signOut]'s local cleanup but + /// skips the logout round-trip — the session is already gone. Returns true + /// only when a signed-in session was actually cleared, so a burst of 401s + /// resets (and notifies) exactly once. + bool handleSessionExpired() { + if (state.currentUser == null) return false; + // Reset synchronously first so the router redirects and any concurrent + // 401 sees the cleared state and bails. + state = AuthState(profiles: state.profiles); + ref.read(webSocketServiceProvider).disconnect(); + unawaited(_storage.clearSession()); + return true; + } + Future signOut() async { try { // Best-effort: delete the session server-side. diff --git a/lib/screens/video_player_screen.dart b/lib/screens/video_player_screen.dart index 12f6ed2..8e66243 100644 --- a/lib/screens/video_player_screen.dart +++ b/lib/screens/video_player_screen.dart @@ -32,6 +32,7 @@ class _VideoPlayerScreenState extends ConsumerState { bool _isOfflinePlayback = false; bool _startingPlayback = false; bool _pendingHqSwitch = false; + bool _progressSavedOnExit = false; String? _error; late final ApiService _api; late final OfflineService _offline; @@ -321,8 +322,13 @@ class _VideoPlayerScreenState extends ConsumerState { _isPreviewMode = false; }); - oldController?.pause(); - oldController?.dispose(); + // Tear down the preview controller only after this frame has rendered + // with the HQ controller — disposing it synchronously can leave the + // outgoing VideoPlayer reading a disposed controller mid-frame. + WidgetsBinding.instance.addPostFrameCallback((_) { + oldController?.pause(); + oldController?.dispose(); + }); } catch (e) { // HQ switch failed — keep playing preview (silent fallback) debugPrint('HQ switch failed, continuing preview: $e'); @@ -375,13 +381,12 @@ class _VideoPlayerScreenState extends ConsumerState { _scheduleHideControls(); } - Future _navigateBack() async { - // Save progress before leaving, but never block navigation on a failure. - try { - await _saveProgress(); - } catch (_) { - // Ignore — progress save is best-effort. - } + void _navigateBack() { + // Fire-and-forget the final save so leaving is instant even on a slow or + // dead network. The guard stops dispose() from saving a second time, so + // teardown sends exactly one /progress PUT. + _progressSavedOnExit = true; + unawaited(_saveProgress()); _controller?.pause(); if (mounted) { Navigator.of(context).pop(); @@ -442,9 +447,12 @@ class _VideoPlayerScreenState extends ConsumerState { _previewTimeout?.cancel(); _previewPollTimer?.cancel(); // Save final position (fire-and-forget; a failed save must never throw - // out of dispose). + // out of dispose). Skipped when _navigateBack already fired the save on + // the way out, so teardown sends exactly one /progress PUT. final controller = _controller; - if (controller != null && controller.value.isInitialized) { + if (!_progressSavedOnExit && + controller != null && + controller.value.isInitialized) { final position = controller.value.position.inSeconds; if (position > 0) { if (_isOfflinePlayback) { diff --git a/lib/services/api_service.dart b/lib/services/api_service.dart index d5bb33a..5bb00b6 100644 --- a/lib/services/api_service.dart +++ b/lib/services/api_service.dart @@ -66,6 +66,13 @@ class ApiException implements Exception { class ApiService { final StorageService storage; + + /// Called when a protected (non-auth) endpoint returns 401, signalling the + /// stored session is dead server-side. Wired by [apiServiceProvider] to + /// reset auth and prompt re-sign-in. Auth endpoints are excluded — they + /// return 401/403 for bad credentials and are handled inline by callers. + void Function()? onUnauthorized; + late final Dio _dio; ApiService({required this.storage}) { @@ -90,6 +97,15 @@ class ApiService { handler.next(options); }, onError: (error, handler) { + // A real 401 response (connection failures carry no response) on a + // protected endpoint means the session is no longer valid + // server-side. Auth endpoints legitimately return 401/403 for bad + // credentials, so they're excluded and surface inline instead of + // forcing a global sign-out. + if (error.response?.statusCode == 401 && + !_isAuthEndpoint(error.requestOptions)) { + onUnauthorized?.call(); + } handler.next(error); }, ), @@ -98,6 +114,12 @@ class ApiService { String get _baseUrl => storage.getServerUrl() ?? 'http://localhost:8484'; + /// Auth/credential endpoints (sign-in, profile management, session probe) + /// legitimately return 401/403, so a 401 here must NOT trigger a global + /// session reset — callers surface it inline. + bool _isAuthEndpoint(RequestOptions options) => + options.uri.path.contains('${AppConstants.apiBase}/auth/'); + /// Generous timeout for endpoints that shell out to yt-dlp server-side. static const _slowReceiveTimeout = Duration(seconds: 90); diff --git a/test/unit/auth_notifier_test.dart b/test/unit/auth_notifier_test.dart index a1adc6b..0282a7c 100644 --- a/test/unit/auth_notifier_test.dart +++ b/test/unit/auth_notifier_test.dart @@ -299,6 +299,58 @@ void main() { }); }); + group('handleSessionExpired', () { + Future signedInContainer() async { + final alice = makeUser(); + when(() => api.getProfiles()).thenAnswer((_) async => [alice]); + when( + () => api.selectProfile('u1', pin: null), + ).thenAnswer((_) async => (user: alice, token: 'tok-1')); + final container = createContainer(); + final notifier = container.read(authStateProvider.notifier); + await notifier.loadProfiles(); + await notifier.selectProfile('u1'); + expect(container.read(authStateProvider).currentUser, alice); + return container; + } + + test('resets to the picker and clears storage on a server 401', () async { + final container = await signedInContainer(); + final notifier = container.read(authStateProvider.notifier); + + final didReset = notifier.handleSessionExpired(); + + expect(didReset, isTrue); + final state = container.read(authStateProvider); + expect(state.currentUser, isNull); + expect(state.profiles, hasLength(1), reason: 'profiles are kept'); + verify(() => webSocket.disconnect()).called(1); + verify(() => storage.clearSession()).called(1); + }); + + test('is a no-op when nobody is signed in', () { + final container = createContainer(); + final notifier = container.read(authStateProvider.notifier); + + expect(notifier.handleSessionExpired(), isFalse); + expect(container.read(authStateProvider).currentUser, isNull); + verifyNever(() => webSocket.disconnect()); + verifyNever(() => storage.clearSession()); + }); + + test('a burst of 401s resets exactly once', () async { + final container = await signedInContainer(); + final notifier = container.read(authStateProvider.notifier); + + expect(notifier.handleSessionExpired(), isTrue); + expect(notifier.handleSessionExpired(), isFalse); + expect(notifier.handleSessionExpired(), isFalse); + + verify(() => webSocket.disconnect()).called(1); + verify(() => storage.clearSession()).called(1); + }); + }); + group('deleteProfile', () { test('deleting the current user clears the session', () async { final alice = makeUser();