Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,9 @@ All notable changes to this project will be documented in this file.

### Fixed

- **A cold start with no working backend showed a blank window for as long as the client timeout, because `restore()` waited for a call whose answer the cache had already given.** `AuthServiceProvider.boot()` awaits `Auth.restore()`, which holds `Magic.init()`, which holds `runApp`, so everything `restore()` awaited was time the user spent looking at nothing. It awaited `_syncUserFromApi()` even after `loadCachedUser()` had produced a user and `setUser` had put it in place. Against a backend that accepts the connection and then says nothing (a captive portal, a dead mobile link, a hung server) that is the entire timeout: measured on an iPhone 17 simulator against an app configured for 120s as roughly two minutes of white screen, with the console stopping dead on `Auth: Cached user restored` and the theme's own boot logging not appearing until it let go. The class docblock has described the intent as "2. Sync from API in background" since it was written. The sync is now awaited only when the cache had nothing to show, because then there is nothing to render and no honest way to route; with a cached user the screen renders now and corrects itself when the sync lands, which is what `AuthRestored` already exists to announce. (`lib/src/auth/guards/base_guard.dart`, `test/auth/auth_test.dart`)
- **Losing the network signed the user out and destroyed the stored session.** `_syncUserFromApi()` treated any non-2xx as a rejected token and called `logout()`, and `DioNetworkDriver._handleError` reports a transport failure as `statusCode: 0`, because a timeout, a DNS miss or a dead link has no response to report. So a phone going through a tunnel during the restore call cleared the token and the cached user and dropped the app on the sign-in screen, while the log said `Auth: Token invalid` about a server that never spoke. Reproduced on a device: after one offline cold start the next launch logged `Auth: No token found in storage`. Only a `401` or a `403` ends a session now; every other failure keeps the cached one and logs what actually happened, including the status it saw. (`lib/src/auth/guards/base_guard.dart`, `test/auth/auth_test.dart`)

- **`Pick`'s two gallery fallbacks escaped their own error handling, and CI could not build until it was fixed.** `pickFromCamera` and `pickVideoFromCamera` returned the fallback future without awaiting it, so the future left the `try` block before completing: a failure inside the fallback never reached the `catch`, and the `onError` callback the caller supplied never fired. Flutter 3.47 added `unawaited_return_in_try_block`, which turned the latent bug into two analyzer warnings and a red `Lint & Test` job on every branch, including ones that never touch this file. Both are awaited now, which fixes the reporting and the build together. (`lib/src/facades/pick.dart`)
- **Every page this router built was anonymous, which silently disabled every screen-aware observer.** `GoRoute.name` names the ROUTE and never reaches `RouteSettings`, so a `NavigatorObserver` reading `route.settings.name` got `null` on every push and could not tell one screen from another. Analytics, breadcrumb trails and Sentry's Flutter Web release health all key on exactly that value, and the last one fails in the worst possible way: the transport keeps working, events keep arriving, and the session count sits at zero forever with nothing in any log to explain it, because `WebSessionHandler.startSession` only fires when the name CHANGES (or on the first navigation when it is exactly `/`). Measured on a deployed app before this fix: a browser with no ad blocker made zero requests to Sentry's ingest across three route changes while a forced `captureMessage` from the same page returned 200. All five pages the transition switch returns now carry `route.routeName ?? route.fullPath`. The fallback is the path rather than nothing, because `.name()` is optional and most routes never call it, so keying only on `routeName` would have left the common case exactly as broken as before; the path is always present, already unique per route, and on the root route it produces the `/` that the first-session rule wants. (`lib/src/routing/magic_router.dart`, `test/routing/page_route_name_test.dart`)

Expand Down
43 changes: 40 additions & 3 deletions lib/src/auth/guards/base_guard.dart
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import 'dart:async' show unawaited;
import 'dart:convert';

import 'package:flutter/foundation.dart';
Expand Down Expand Up @@ -264,7 +265,27 @@ abstract class BaseGuard implements Guard {
Log.debug('Auth: No cached user found');
}

// 2. Sync from API (fresh data)
// 2. Sync from API (fresh data).
//
// Awaited only when the cache had nothing to show. `AuthServiceProvider`
// awaits `restore()`, which holds `Magic.init()`, which holds `runApp`, so
// anything awaited here is time the user spends looking at a blank window.
// Against a backend that accepts the connection and then says nothing (a
// captive portal, a dead mobile link) that is the whole client timeout: on
// an app configured for 120s it measured as roughly two minutes of white
// screen on a cold start, with the console stopping dead on the line above.
//
// With a cached user already set the screen can render now and correct
// itself when the sync lands, which is what this class has documented as
// its cache strategy from the start ("2. Sync from API in background").
// Without one there is nothing to render and no honest way to route, so the
// API is the only answer and waiting for it is the point.
if (cachedUser != null) {
unawaited(_syncUserFromApi());

return;
}

await _syncUserFromApi();
}

Expand All @@ -282,8 +303,24 @@ abstract class BaseGuard implements Guard {
final response = await Http.get(userEndpoint!);

if (!response.successful) {
Log.warning('Auth: Token invalid, logging out');
await logout();
// Only the server may end a session. A transport failure (a timeout, a
// DNS miss, a dead mobile link) reaches here as statusCode 0, because
// `DioNetworkDriver._handleError` has no response to report: that is
// "nobody answered", not "your token is bad". Logging out on it threw
// away a valid session because the phone went through a tunnel, and
// said "Token invalid" about a server that never spoke.
if (response.statusCode == 401 || response.statusCode == 403) {
Log.warning('Auth: Token rejected by the server, logging out');
await logout();

return;
}

Log.warning(
'Auth: user sync failed (status ${response.statusCode}); '
'keeping the cached session',
);

return;
}

Expand Down
156 changes: 156 additions & 0 deletions test/auth/auth_test.dart
Original file line number Diff line number Diff line change
@@ -1,4 +1,7 @@
import 'package:flutter/foundation.dart';
import 'dart:async';
import 'dart:convert';

import 'package:flutter_test/flutter_test.dart';
import 'package:magic/magic.dart';

Expand Down Expand Up @@ -84,6 +87,56 @@ class MockGuard implements Guard {
}
}

/// A guard that keeps [BaseGuard.restore] rather than replacing it, so the real
/// cache-first path is what the test drives.
class _CacheFirstGuard extends BaseGuard {
_CacheFirstGuard()
: super(
userEndpoint: '/user',
userFactory: (data) => MockUser()..setRawAttributes(data, sync: true),
);

@override
Future<void> login(Map<String, dynamic> data, Authenticatable user) async {}
}

/// A driver whose GET never answers until the test opens the gate, standing in
/// for a backend that accepts the connection and then says nothing.
class _GatedDriver extends FakeNetworkDriver {
_GatedDriver(this.gate);

final Completer<void> gate;

@override
Future<MagicResponse> get(
String url, {
Map<String, dynamic>? query,
Map<String, String>? headers,
}) async {
await gate.future;

return super.get(url, query: query, headers: headers);
}
}

/// A driver whose GET answers the way [DioNetworkDriver] answers a transport
/// failure: no response, so `statusCode` is 0 rather than anything the server
/// said.
class _StatusDriver extends FakeNetworkDriver {
_StatusDriver(this.statusCode);

final int statusCode;

@override
Future<MagicResponse> get(
String url, {
Map<String, dynamic>? query,
Map<String, String>? headers,
}) async {
return MagicResponse(data: null, statusCode: statusCode, headers: {});
}
}

// ---------------------------------------------------------------------------
// Tests
// ---------------------------------------------------------------------------
Expand Down Expand Up @@ -404,4 +457,107 @@ void main() {
expect(result.errors['email'], ['Invalid email']);
});
});

// ---------------------------------------------------------------------------
// BaseGuard.restore: the cache answers, the API sync does not hold the boot
// ---------------------------------------------------------------------------

group('BaseGuard.restore cache-first contract', () {
setUp(() {
MagicApp.reset();
Magic.flush();
});

tearDown(() {
Vault.unfake();
Log.unfake();
MagicApp.reset();
Magic.flush();
});

test(
'returns once the cached user is in place, without waiting for the API',
() async {
// `AuthServiceProvider.boot()` awaits `restore()`, so anything `restore()`
// awaits holds `Magic.init()`, and nothing renders until it lets go. With
// a backend that accepts the connection and never answers (a captive
// portal, a dead mobile link), that is the whole client timeout: measured
// on an iPhone as roughly two minutes of blank white screen on a cold
// start, with the console stopping dead on "Auth: Cached user restored".
//
// The class docblock has always said "2. Sync from API in background".
Log.fake();
Vault.fake({
'auth_token': 'stored-token',
'auth_user': jsonEncode({'id': 7, 'name': 'Cached User'}),
});

final gate = Completer<void>();
Magic.singleton('network', () => _GatedDriver(gate));

final guard = _CacheFirstGuard();

// No timeout wrapper on purpose: if `restore()` waits for the gate this
// never completes and the case fails as a hang, which is exactly the
// shape of the defect.
await guard.restore();

expect(
guard.check(),
isTrue,
reason: 'the cached user is what makes the app renderable',
);
expect(guard.user<MockUser>()?.name, 'Cached User');
expect(
gate.isCompleted,
isFalse,
reason:
'the API has not answered yet, and must not have been waited on',
);

gate.complete();
},
);

test(
'a transport failure keeps the session, only the server may end it',
() async {
// `DioNetworkDriver._handleError` has no response to report on a
// timeout, a DNS failure or a dead link, so it returns statusCode 0.
// Reading that as "not successful" and logging out throws away a valid
// session because the phone went through a tunnel, and the log line
// said "Token invalid" about a server that never answered.
Log.fake();
Vault.fake({
'auth_token': 'stored-token',
'auth_user': jsonEncode({'id': 7, 'name': 'Cached User'}),
});
Magic.singleton('network', () => _StatusDriver(0));

final guard = _CacheFirstGuard();
await guard.restore();
// The sync no longer blocks restore, so let its microtask run.
await Future<void>.delayed(Duration.zero);

expect(guard.check(), isTrue);
expect(await Vault.get('auth_token'), 'stored-token');
},
);

test('a 401 does end the session, because the server said so', () async {
Log.fake();
Vault.fake({
'auth_token': 'stored-token',
'auth_user': jsonEncode({'id': 7, 'name': 'Cached User'}),
});
Magic.singleton('network', () => _StatusDriver(401));

final guard = _CacheFirstGuard();
await guard.restore();
await Future<void>.delayed(Duration.zero);

expect(guard.check(), isFalse);
expect(await Vault.get('auth_token'), isNull);
});
});
}
Loading