From 4e6477a3ae26273fd87708468fafcd6043c99b2d Mon Sep 17 00:00:00 2001 From: ayushya Date: Mon, 28 Sep 2026 23:51:16 -0400 Subject: [PATCH] Fix recurring empty-list-on-login race at the root addAccountActivatedListener/addAccountReadyListener fired once, to whichever listeners were registered at that instant. Most per-tab controllers are lazy providers, only constructed (and so only registering) whenever something first reads them - if that happened after the one-shot event already fired (e.g. a cold-start connectivity misdetection delaying a tab's construction until after login verified), that controller's initial fetch never ran, leaving its list permanently empty. Both listeners now call back immediately on registration if the account is already in the state being subscribed to, closing the race regardless of construction timing. Documents the failure class and the guardrail for future controllers in architecture.md. Co-Authored-By: Claude Sonnet 5 --- .claude/context/architecture.md | 36 ++++++ lib/providers/session_controller.dart | 37 ++++-- test/providers/session_controller_test.dart | 120 ++++++++++++++++++++ 3 files changed, 184 insertions(+), 9 deletions(-) create mode 100644 test/providers/session_controller_test.dart 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(); + }); +}