diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 8dfa6eb..3ab6c93 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -10,6 +10,7 @@ jobs: analyze: name: Analyze code runs-on: ubuntu-latest + timeout-minutes: 15 steps: - uses: actions/checkout@v3 @@ -27,6 +28,7 @@ jobs: test: name: Run tests runs-on: ubuntu-latest + timeout-minutes: 15 steps: - uses: actions/checkout@v3 @@ -44,15 +46,10 @@ jobs: test_web: name: Run tests on web runs-on: ubuntu-latest + timeout-minutes: 15 steps: - uses: actions/checkout@v3 - - name: Setup Chrome - id: setup-chrome - uses: browser-actions/setup-chrome@v1 - with: - install-dependencies: true - - name: Setup Flutter uses: subosito/flutter-action@v2 with: @@ -62,14 +59,16 @@ jobs: run: flutter pub get # Guards web support: a `dart:io` import in lib/ fails to compile here. + # Chrome ships preinstalled on ubuntu-latest runners, so no setup action is + # needed — installing one shells out to apt, which can block on a dpkg lock + # and hang the job with no output. - name: Run tests run: flutter test --platform chrome - env: - CHROME_EXECUTABLE: ${{ steps.setup-chrome.outputs.chrome-path }} dry_run: name: Publish dry run runs-on: ubuntu-latest + timeout-minutes: 15 steps: - uses: actions/checkout@v3 diff --git a/CHANGELOG.md b/CHANGELOG.md index 0a47d6d..201a77f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -12,6 +12,10 @@ exhaustive `switch` statements over it need a `Cancelled` case added. `map()` and `flatMap()` pass cancellation through * Note: the task is not interrupted on cancellation — Dart futures cannot be cancelled, so it runs to completion and its result is discarded +* Fixed the returned future never completing when the hosting `Navigator` is + disposed while the task is in flight — for example a nested navigator removed + from the widget tree. The call now settles with `Cancelled` instead of leaving + the caller awaiting forever * Added Flutter Web support — `flutter_future_progress_dialog` no longer imports `dart:io`, so pub.dev now reports all six platforms * **Behaviour change:** `showAdaptiveProgressDialog` now selects its style from diff --git a/README.md b/README.md index 2f1f825..a2cdd7a 100644 --- a/README.md +++ b/README.md @@ -322,7 +322,6 @@ Yes, if you use the `BuildContext` afterwards. `showProgressDialog` closes its o - **The task cannot be aborted.** Cancellation dismisses the dialog, not the work. See [Can the user cancel the progress dialog?](#can-the-user-cancel-the-progress-dialog) - **No determinate progress.** The dialog shows an indeterminate spinner; there is no percentage or step reporting. -- **If the hosting `Navigator` is disposed while the task is in flight** — for example a nested navigator removed from the widget tree — the dialog is torn down without error, but the returned future never completes, so the `await` waits forever. ## Contributing diff --git a/lib/src/exactly_once.dart b/lib/src/exactly_once.dart index 3a6d649..01571a8 100644 --- a/lib/src/exactly_once.dart +++ b/lib/src/exactly_once.dart @@ -2,11 +2,23 @@ import 'dart:async'; import 'package:flutter/widgets.dart'; +/// Runs [callback] exactly once, after [child]'s first frame, and reports the +/// teardown of its own subtree through [onDispose]. +/// +/// [onDispose] is the only reliable signal that a route was torn down without +/// being popped — for example when its [Navigator] is removed from the tree. +/// The framework abandons `Route.popped` in that case, so nothing else fires. class ExactlyOnce extends StatefulWidget { final Future Function() callback; + final VoidCallback? onDispose; final Widget child; - const ExactlyOnce({super.key, required this.callback, required this.child}); + const ExactlyOnce({ + super.key, + required this.callback, + required this.child, + this.onDispose, + }); @override State createState() => _ExactlyOnceState(); @@ -21,6 +33,12 @@ class _ExactlyOnceState extends State { super.initState(); } + @override + void dispose() { + widget.onDispose?.call(); + super.dispose(); + } + @override Widget build(BuildContext context) { WidgetsBinding.instance.addPostFrameCallback((_) { diff --git a/lib/src/show_progress_dialog.dart b/lib/src/show_progress_dialog.dart index dd890d8..8af66a7 100644 --- a/lib/src/show_progress_dialog.dart +++ b/lib/src/show_progress_dialog.dart @@ -1,3 +1,5 @@ +import 'dart:async'; + import 'package:flutter/cupertino.dart' as c; import 'package:flutter/material.dart' as m; import 'package:flutter/widgets.dart' as w; @@ -12,14 +14,59 @@ Future _callback( w.NavigatorState navigator, Task task, w.Route> route, + _Settle settle, ) async { final result = await task.result(); + // Settle before removing the route: tearing the route down fires the dialog's + // onDispose, and the real result must win that race. + settle(result); if (!route.isActive) { return; } navigator.removeRoute(route, result); } +typedef _Settle = void Function(ProgressDialogResult result); + +/// Drives the returned future from whichever of three things happens first: the +/// task delivering a result, the route being popped, or the route's subtree +/// being torn down. +/// +/// The last case is why this exists. When a [w.Navigator] is disposed it force- +/// disposes its routes without completing `Route.popped`, so a future derived +/// from `Navigator.push` alone would never complete and the caller would await +/// forever. +class _Outcome { + final _completer = Completer>(); + + Future> get future => _completer.future; + + void settle(ProgressDialogResult result) { + if (!_completer.isCompleted) { + _completer.complete(result); + } + } + + /// Wires up the route-popped and never-installed paths, and returns the + /// future the caller should await. + Future> track( + w.NavigatorState navigator, + w.Route> route, + ) { + navigator.push>(route).then((result) => settle(result ?? Cancelled())).ignore(); + + // If the navigator is disposed before the route ever builds, the dialog's + // onDispose never fires because its state was never created. + w.WidgetsBinding.instance.addPostFrameCallback((_) { + if (route.navigator == null) { + settle(Cancelled()); + } + }); + + return future; + } +} + /// Shows a progress dialog while executing an asynchronous task. /// /// Displays a modal progress dialog that remains visible until [future] completes. @@ -86,11 +133,13 @@ Future> showProgressDialog({ ); late final m.Route> route; + final outcome = _Outcome(); route = m.DialogRoute>( context: context, builder: (context) => ExactlyOnce( - callback: () => _callback(navigator, future, route), + callback: () => _callback(navigator, future, route, outcome.settle), + onDispose: () => outcome.settle(Cancelled()), child: builder?.call(context) ?? const ProgressBarDialog(), ), barrierColor: barrierColor ?? @@ -109,9 +158,7 @@ Future> showProgressDialog({ fullscreenDialog: fullscreenDialog, ); - final result = await navigator.push>(route); - - return result ?? Cancelled(); + return outcome.track(navigator, route); } /// Shows a Cupertino-styled progress dialog while executing an asynchronous task. @@ -167,10 +214,12 @@ Future> showCupertinoProgressDialog({ final navigator = c.Navigator.of(context, rootNavigator: useRootNavigator); late final c.CupertinoDialogRoute> route; + final outcome = _Outcome(); route = c.CupertinoDialogRoute>( builder: (context) => ExactlyOnce( - callback: () => _callback(navigator, future, route), + callback: () => _callback(navigator, future, route, outcome.settle), + onDispose: () => outcome.settle(Cancelled()), child: builder?.call(context) ?? const CupertinoProgressBarDialog(), ), context: context, @@ -180,8 +229,7 @@ Future> showCupertinoProgressDialog({ anchorPoint: anchorPoint, requestFocus: requestFocus, ); - final result = await navigator.push>(route); - return result ?? Cancelled(); + return outcome.track(navigator, route); } /// Shows a platform-adaptive progress dialog while executing an asynchronous task. diff --git a/test/progress_dialog_test.dart b/test/progress_dialog_test.dart index 42487d0..c5b8100 100644 --- a/test/progress_dialog_test.dart +++ b/test/progress_dialog_test.dart @@ -42,17 +42,22 @@ void main() { expect((result as Success).value, 'ok'); }); - // Regression test: when the dialog is on a nested navigator and that - // navigator is removed from the tree while the task is in-flight, - // the dialog's context becomes unmounted. The original _callback would - // check context.mounted, return early, and never call removeRoute — - // causing push() to resolve with null and result! to throw. + // Regression test: when the dialog is on a nested navigator and that navigator + // is removed from the tree while the task is in-flight, the navigator force- + // disposes its routes without completing `Route.popped`. A result derived from + // `Navigator.push` alone would therefore never arrive and the caller would + // await forever. The dialog's own dispose signal settles it as Cancelled. + // + // Note this asserts on a captured value rather than awaiting the call: a + // regression here hangs rather than throws, so awaiting would stall the suite + // instead of failing it. testWidgets( - 'showProgressDialog does not crash when context unmounts before task completes', + 'showProgressDialog returns Cancelled when its navigator is disposed mid-task', (WidgetTester tester) async { final taskCompleter = Completer(); final showNestedNav = ValueNotifier(true); late BuildContext nestedContext; + ProgressDialogResult? outcome; await tester.pumpWidget( MaterialApp( @@ -82,7 +87,7 @@ void main() { return 'ok'; }, useRootNavigator: false, - ).ignore(); + ).then((result) => outcome = result); // Let the dialog build and ExactlyOnce fire _callback. // Can't use pumpAndSettle here — the pending task keeps the dialog @@ -92,17 +97,71 @@ void main() { await tester.pump(); // Remove the nested navigator from the tree — this disposes all its - // routes and unmounts the dialog's context while the task is running. + // routes and unmounts the dialog's subtree while the task is running. showNestedNav.value = false; await tester.pumpAndSettle(); - // Complete the task after context has been unmounted. + expect( + outcome, + isA>(), + reason: 'the caller must be released when the navigator goes away', + ); + + // The task is not interrupted; completing it later changes nothing. taskCompleter.complete(); await tester.pumpAndSettle(); + expect(outcome, isA>()); + expect(tester.takeException(), isNull); + }, + ); + + testWidgets( + 'showCupertinoProgressDialog returns Cancelled when its navigator is disposed mid-task', + (WidgetTester tester) async { + final taskCompleter = Completer(); + final showNestedNav = ValueNotifier(true); + late BuildContext nestedContext; + ProgressDialogResult? outcome; - // With the old code, _callback returns early (context.mounted == false), - // removeRoute is never called, and push() resolves with null causing - // result! to throw. + await tester.pumpWidget( + CupertinoApp( + home: ValueListenableBuilder( + valueListenable: showNestedNav, + builder: (_, show, __) { + if (!show) return const SizedBox(); + return Navigator( + onGenerateRoute: (_) => CupertinoPageRoute( + builder: (ctx) { + nestedContext = ctx; + return const SizedBox(); + }, + ), + ); + }, + ), + ), + ); + await tester.pumpAndSettle(); + + showCupertinoProgressDialog( + context: nestedContext, + future: () async { + await taskCompleter.future; + return 'ok'; + }, + useRootNavigator: false, + ).then((result) => outcome = result); + + await tester.pump(); + await tester.pump(); + + showNestedNav.value = false; + await tester.pumpAndSettle(); + + expect(outcome, isA>()); + + taskCompleter.complete(); + await tester.pumpAndSettle(); expect(tester.takeException(), isNull); }, );