diff --git a/.claude/context/architecture.md b/.claude/context/architecture.md index 7743f8b..2c0da71 100644 --- a/.claude/context/architecture.md +++ b/.claude/context/architecture.md @@ -153,6 +153,42 @@ new provider instance): (shares/activity/versions/restore) — stateless, so they don't need a controller of their own. +**Guardrail - the "empty list on first load" failure class.** This bug has +recurred more than once, always the same shape: a tab's list is empty after +login even though the account is fine, because the controller behind it +never ran its initial fetch. The cause is a race, not a network bug: +`addAccountActivatedListener`/`addAccountReadyListener` fire *once*, at the +moment login is verified/ready, to whichever listeners are registered at +that exact instant - and most per-domain controllers (`FilesController`, +`PhotosController`, `FavoritesController`, `TrashController`, +`SharesController`, `RecentController`) are lazy `ChangeNotifierProvider`s, +only constructed (and so only registering their listener) whenever +something first reads them. If that first read happens to land *after* +the one-shot event already fired - e.g. `ConnectivityController` +misreporting offline for its first couple of seconds after a cold Android +start (see its own doc comment) collapses `main.dart`'s bottom nav to just +the Offline tab, delaying construction of every other tab's controller +until connectivity corrects itself and login has already finished +verifying - that controller's listener registers too late and its initial +fetch simply never happens. `SessionController.addAccountActivatedListener`/ +`addAccountReadyListener` now close this at the root: registering either +one calls back **immediately** if the account is already in the state +being subscribed to, not just on the next fresh event, so a late-registering +controller always gets its initial fetch regardless of when its lazy +`Provider` happens to be built. Some views (`FilesView`/`PhotosView`, not +`FavoritesView` - see `FavoritesController`'s own doc comment) additionally +carry a build-time fallback (`_requestedInitialLoad` et al.: reload if +items are empty and not loading, once, after first build) predating this +fix; they're now redundant but harmless, and not worth touching for +cleanup alone. **Guardrail for new code:** any new network-fetching +controller that needs a one-time fetch on login must register through +`addAccountActivatedListener`/`addAccountReadyListener` in its constructor +and rely on their catch-up behavior - don't reach for a per-view +build-time "fetch if empty" fallback as the primary mechanism, since that +pattern is exactly what let this bug keep recurring silently (three +near-duplicate, slightly-diverging implementations, none of them fixing +the actual race). + `sessionGeneration` (on `SessionController`, read by every other controller) is incremented on every account switch so an in-flight fetch from the account just left can recognize it's stale and discard its result diff --git a/lib/providers/session_controller.dart b/lib/providers/session_controller.dart index e22d08e..2504d80 100644 --- a/lib/providers/session_controller.dart +++ b/lib/providers/session_controller.dart @@ -27,8 +27,7 @@ enum LoginFlowStatus { idle, initiating, awaitingBrowser, error } /// full rationale. class SessionController extends ChangeNotifier with WidgetsBindingObserver { final ConnectivityController connectivity; - final Future prefsFuture = - SharedPreferences.getInstance(); + final Future prefsFuture = SharedPreferences.getInstance(); final AccountStore accountStore = AccountStore(); // True from the moment a saved session is restored (or a network @@ -96,10 +95,32 @@ class SessionController extends ChangeNotifier with WidgetsBindingObserver { final List _accountReadyListeners = []; void addAccountClearedListener(VoidCallback cb) => _accountClearedListeners.add(cb); - void addAccountActivatedListener(VoidCallback cb) => - _accountActivatedListeners.add(cb); - void addAccountReadyListener(VoidCallback cb) => - _accountReadyListeners.add(cb); + + // Both `activated` and `ready` are one-shot, fire-and-forget calls, not a + // replayable stream - so a controller isn't guaranteed to be *listening* + // yet when the real event fires. Most controllers are registered with + // `lazy: false` in main.dart specifically so they exist before that can + // happen, but one that isn't (built lazily, on first read, like + // FilesController) can lose the race if nothing reads it until after + // login already finished verifying - e.g. `ConnectivityController` + // misreporting offline right at cold start collapses the bottom nav to + // just the Offline tab (see `main.dart`), so Files' tab, and the + // `FilesController` it lazily creates, never gets built during that + // window. Calling back immediately here if the account is *already* in + // the state being subscribed to closes that race for every current and + // future subscriber, without each one needing its own view-level + // fallback reload - see `.claude/context/architecture.md`'s "State + // management" section for the guardrail this exists to enforce. + void addAccountActivatedListener(VoidCallback cb) { + _accountActivatedListeners.add(cb); + if (_isLoggedIn && !_isProvisionalLogin) cb(); + } + + void addAccountReadyListener(VoidCallback cb) { + _accountReadyListeners.add(cb); + if (_isLoggedIn) cb(); + } + void _notifyAccountCleared() { for (final cb in _accountClearedListeners) { cb(); @@ -141,9 +162,7 @@ class SessionController extends ChangeNotifier with WidgetsBindingObserver { // one-time unlock at cold start would give the feature no real // security value, since the realistic threat is someone else picking // up an already-running, unlocked phone. - if (state == AppLifecycleState.paused && - _loginLockEnabled && - _isUnlocked) { + if (state == AppLifecycleState.paused && _loginLockEnabled && _isUnlocked) { _isUnlocked = false; notifyListeners(); } diff --git a/test/providers/session_controller_test.dart b/test/providers/session_controller_test.dart new file mode 100644 index 0000000..459977d --- /dev/null +++ b/test/providers/session_controller_test.dart @@ -0,0 +1,120 @@ +import 'dart:convert'; +import 'package:flutter/services.dart'; +import 'package:flutter_test/flutter_test.dart'; +import 'package:shared_preferences/shared_preferences.dart'; +import 'package:noo/models/saved_account.dart'; +import 'package:noo/providers/connectivity_controller.dart'; +import 'package:noo/providers/session_controller.dart'; + +/// Regression coverage for the "empty list on first load" bug class: a +/// controller built by a *lazy* `ChangeNotifierProvider` (FilesController, +/// PhotosController, ...) can be constructed after +/// [SessionController]'s one-shot account-activated/ready event already +/// fired - e.g. `ConnectivityController` misreporting offline for a couple +/// of seconds right after a cold Android start hides the Files tab from +/// `main.dart`'s bottom nav, delaying `FilesController`'s construction +/// until after login already finished. `addAccountActivatedListener`/ +/// `addAccountReadyListener` must call back immediately for a listener +/// that registers after the fact, not only fire for the *next* login. +void main() { + const secureStorageChannel = MethodChannel( + 'plugins.it_nomads.com/flutter_secure_storage', + ); + const connectivityChannel = MethodChannel( + 'dev.fluttercommunity.plus/connectivity', + ); + const connectivityStatusChannel = EventChannel( + 'dev.fluttercommunity.plus/connectivity_status', + ); + + const accountId = 'server_example_com__alice'; + const passwordKey = 'nc_app_password_$accountId'; + const password = 'app-password'; + + TestWidgetsFlutterBinding.ensureInitialized(); + + setUpAll(() { + TestDefaultBinaryMessengerBinding.instance.defaultBinaryMessenger + .setMockMethodCallHandler(secureStorageChannel, (call) async { + if (call.method == 'readAll') { + return {passwordKey: password}; + } + if (call.method == 'read') { + final key = (call.arguments as Map)['key'] as String?; + return key == passwordKey ? password : null; + } + return null; + }); + // Reports no network on every `checkConnectivity()` call, so + // `ConnectivityController` deterministically settles into `isOffline` + // and `SessionController` never attempts a real (and, in this + // sandboxed test environment, unreachable) HTTP call. + TestDefaultBinaryMessengerBinding.instance.defaultBinaryMessenger + .setMockMethodCallHandler(connectivityChannel, (call) async { + if (call.method == 'check') return ['none']; + return null; + }); + TestDefaultBinaryMessengerBinding.instance.defaultBinaryMessenger + .setMockStreamHandler( + connectivityStatusChannel, + MockStreamHandler.inline(onListen: (arguments, events) {}), + ); + }); + + Future pumpUntil(bool Function() condition) async { + for (var i = 0; i < 50 && !condition(); i++) { + await Future.delayed(Duration.zero); + } + } + + test('addAccountReadyListener calls back immediately for a listener that ' + 'registers after a provisional/offline login already happened', () async { + SharedPreferences.setMockInitialValues({ + 'account_migration_v1_done': true, + 'accounts_list': jsonEncode([ + const SavedAccount( + id: accountId, + serverUrl: 'https://server.example.com', + username: 'alice', + ).toJson(), + ]), + 'active_account_id': accountId, + }); + + final connectivity = ConnectivityController(); + await pumpUntil(() => connectivity.isOffline); + expect(connectivity.isOffline, true); + + final session = SessionController(connectivity); + await pumpUntil(() => session.isLoggedIn); + expect(session.isLoggedIn, true); + + // The fix under test: this mirrors what FilesController's/ + // OfflineController's own constructor does, but simulated as + // happening *after* the event above already fired - exactly the + // lazy-Provider race this test guards against. + var readyCalls = 0; + session.addAccountReadyListener(() => readyCalls++); + expect(readyCalls, 1); + + connectivity.dispose(); + session.dispose(); + }); + + test('addAccountReadyListener does not call back before any account is ' + 'active', () async { + SharedPreferences.setMockInitialValues({'account_migration_v1_done': true}); + + final connectivity = ConnectivityController(); + final session = SessionController(connectivity); + await pumpUntil(() => !session.isRestoringSession); + expect(session.isLoggedIn, false); + + var readyCalls = 0; + session.addAccountReadyListener(() => readyCalls++); + expect(readyCalls, 0); + + connectivity.dispose(); + session.dispose(); + }); +}