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 <noreply@anthropic.com>
This commit is contained in:
@@ -153,6 +153,42 @@ new provider instance):
|
|||||||
(shares/activity/versions/restore) — stateless, so they don't need a
|
(shares/activity/versions/restore) — stateless, so they don't need a
|
||||||
controller of their own.
|
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
|
`sessionGeneration` (on `SessionController`, read by every other
|
||||||
controller) is incremented on every account switch so an in-flight fetch
|
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
|
from the account just left can recognize it's stale and discard its result
|
||||||
|
|||||||
@@ -27,8 +27,7 @@ enum LoginFlowStatus { idle, initiating, awaitingBrowser, error }
|
|||||||
/// full rationale.
|
/// full rationale.
|
||||||
class SessionController extends ChangeNotifier with WidgetsBindingObserver {
|
class SessionController extends ChangeNotifier with WidgetsBindingObserver {
|
||||||
final ConnectivityController connectivity;
|
final ConnectivityController connectivity;
|
||||||
final Future<SharedPreferences> prefsFuture =
|
final Future<SharedPreferences> prefsFuture = SharedPreferences.getInstance();
|
||||||
SharedPreferences.getInstance();
|
|
||||||
final AccountStore accountStore = AccountStore();
|
final AccountStore accountStore = AccountStore();
|
||||||
|
|
||||||
// True from the moment a saved session is restored (or a network
|
// True from the moment a saved session is restored (or a network
|
||||||
@@ -96,10 +95,32 @@ class SessionController extends ChangeNotifier with WidgetsBindingObserver {
|
|||||||
final List<VoidCallback> _accountReadyListeners = [];
|
final List<VoidCallback> _accountReadyListeners = [];
|
||||||
void addAccountClearedListener(VoidCallback cb) =>
|
void addAccountClearedListener(VoidCallback cb) =>
|
||||||
_accountClearedListeners.add(cb);
|
_accountClearedListeners.add(cb);
|
||||||
void addAccountActivatedListener(VoidCallback 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);
|
_accountActivatedListeners.add(cb);
|
||||||
void addAccountReadyListener(VoidCallback cb) =>
|
if (_isLoggedIn && !_isProvisionalLogin) cb();
|
||||||
|
}
|
||||||
|
|
||||||
|
void addAccountReadyListener(VoidCallback cb) {
|
||||||
_accountReadyListeners.add(cb);
|
_accountReadyListeners.add(cb);
|
||||||
|
if (_isLoggedIn) cb();
|
||||||
|
}
|
||||||
|
|
||||||
void _notifyAccountCleared() {
|
void _notifyAccountCleared() {
|
||||||
for (final cb in _accountClearedListeners) {
|
for (final cb in _accountClearedListeners) {
|
||||||
cb();
|
cb();
|
||||||
@@ -141,9 +162,7 @@ class SessionController extends ChangeNotifier with WidgetsBindingObserver {
|
|||||||
// one-time unlock at cold start would give the feature no real
|
// one-time unlock at cold start would give the feature no real
|
||||||
// security value, since the realistic threat is someone else picking
|
// security value, since the realistic threat is someone else picking
|
||||||
// up an already-running, unlocked phone.
|
// up an already-running, unlocked phone.
|
||||||
if (state == AppLifecycleState.paused &&
|
if (state == AppLifecycleState.paused && _loginLockEnabled && _isUnlocked) {
|
||||||
_loginLockEnabled &&
|
|
||||||
_isUnlocked) {
|
|
||||||
_isUnlocked = false;
|
_isUnlocked = false;
|
||||||
notifyListeners();
|
notifyListeners();
|
||||||
}
|
}
|
||||||
|
|||||||
@@ -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 <String, String>{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 <String>['none'];
|
||||||
|
return null;
|
||||||
|
});
|
||||||
|
TestDefaultBinaryMessengerBinding.instance.defaultBinaryMessenger
|
||||||
|
.setMockStreamHandler(
|
||||||
|
connectivityStatusChannel,
|
||||||
|
MockStreamHandler.inline(onListen: (arguments, events) {}),
|
||||||
|
);
|
||||||
|
});
|
||||||
|
|
||||||
|
Future<void> 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();
|
||||||
|
});
|
||||||
|
}
|
||||||
Reference in New Issue
Block a user