Skip to content

Commit 3742a6e

Browse files
committed
fix(flowr): harden listenable disposal
1 parent 84533b3 commit 3742a6e

10 files changed

Lines changed: 93 additions & 18 deletions

File tree

packages/flowr/lib/src/mixin/change_notifier.dart

Lines changed: 0 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -84,11 +84,4 @@ mixin FrChangeNotifierMx<M> on FrViewModel<M>, ChangeNotifier {
8484
mutexTag: mutexTag,
8585
);
8686
}
87-
88-
/// Disposes both ChangeNotifier listeners and FlowR resources.
89-
@override
90-
void dispose() {
91-
super.dispose();
92-
unawaited(close());
93-
}
9487
}

packages/flowr/lib/src/provider.dart

Lines changed: 18 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -227,6 +227,10 @@ FrProvider(
227227
/// A FlowR-named [ListenableProvider] for creating [ChangeNotifier] values.
228228
class FrListenableProvider<T extends ChangeNotifier>
229229
extends ListenableProvider<T> {
230+
/// Creates a [ChangeNotifier] and closes FlowR resources with it.
231+
///
232+
/// [dispose] is an optional hook that runs before automatic disposal. It
233+
/// must not dispose [T] itself.
230234
FrListenableProvider(
231235
Create<T> create, {
232236
super.key,
@@ -237,8 +241,20 @@ class FrListenableProvider<T extends ChangeNotifier>
237241
}) : super(
238242
create: create,
239243
dispose: (context, notifier) {
240-
dispose?.call(context, notifier);
241-
notifier.dispose();
244+
try {
245+
dispose?.call(context, notifier);
246+
} finally {
247+
try {
248+
notifier.dispose();
249+
} finally {
250+
if (notifier is Closable) {
251+
final closable = notifier as Closable;
252+
if (!closable.isClosed) {
253+
unawaited(Future<void>.sync(closable.close));
254+
}
255+
}
256+
}
257+
}
242258
},
243259
);
244260
}

packages/flowr/lib/src/support/provider/provider_support.dart

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,7 @@ import 'package:flowr/src/view_model.dart' show FrViewModel;
44
import 'package:flutter/foundation.dart';
55

66
/// support Provider-Consumer
7-
/// adapt ChangeNotifierProvider use ChangeNotifier
7+
/// adapt FrProvider.listenable use ChangeNotifier
88
abstract class FrChangeNotifierVM<M extends FrModel> extends FrViewModel<M>
99
with ChangeNotifier, FrChangeNotifierMx {
1010
FrChangeNotifierVM(super.initialState);

packages/flowr/test/mixin/change_notifier_test.dart

Lines changed: 59 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,5 @@
1+
import 'dart:async' show unawaited;
2+
13
import 'package:flowr/flowr_mvvm.dart';
24
import 'package:flowr/flowr_mvvm_support.dart';
35
import 'package:flutter/material.dart';
@@ -78,6 +80,16 @@ class LifecycleCounterVM extends CounterVM {
7880
}
7981
}
8082

83+
class SelfClosingCounterVM extends LifecycleCounterVM {
84+
SelfClosingCounterVM(super.events);
85+
86+
@override
87+
void dispose() {
88+
unawaited(close());
89+
super.dispose();
90+
}
91+
}
92+
8193
class MyChangNtfApp extends StatelessWidget {
8294
const MyChangNtfApp({super.key});
8395

@@ -261,5 +273,52 @@ main() {
261273
expect(vm.isClosed, isTrue);
262274
expect(() => vm.addListener(() {}), throwsFlutterError);
263275
});
276+
277+
testWidgets('still disposes when the dispose hook throws', (tester) async {
278+
late TrackedCounter counter;
279+
280+
await tester.pumpWidget(
281+
FrProvider.listenable<TrackedCounter>(
282+
(_) => counter = TrackedCounter(),
283+
dispose: (_, _) => throw StateError('hook failed'),
284+
child: Consumer<TrackedCounter>(
285+
builder:
286+
(_, value, _) =>
287+
Text('${value.count}', textDirection: TextDirection.ltr),
288+
),
289+
),
290+
);
291+
292+
await tester.pumpWidget(const SizedBox());
293+
294+
expect(tester.takeException(), isA<StateError>());
295+
expect(counter.disposeCalls, 1);
296+
expect(() => counter.addListener(() {}), throwsFlutterError);
297+
});
298+
299+
testWidgets('does not close an already self-closing FlowR twice', (
300+
tester,
301+
) async {
302+
late SelfClosingCounterVM vm;
303+
final events = <String>[];
304+
305+
await tester.pumpWidget(
306+
FrProvider.listenable<SelfClosingCounterVM>(
307+
(_) => vm = SelfClosingCounterVM(events),
308+
child: Consumer<SelfClosingCounterVM>(
309+
builder:
310+
(_, value, _) =>
311+
Text('${value.count}', textDirection: TextDirection.ltr),
312+
),
313+
),
314+
);
315+
316+
await tester.pumpWidget(const SizedBox());
317+
await tester.pump();
318+
319+
expect(vm.disposeCalls, 1);
320+
expect(vm.closeCalls, 1);
321+
expect(vm.isClosed, isTrue);
322+
});
264323
});
265324
}

skills/flowr-usage/references/flowr-install.md

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -71,6 +71,8 @@ class CounterPage extends StatelessWidget {
7171
state is page-scoped.
7272
- Use `FrProvider.value` for an existing instance and `FrProvider.multi` when a
7373
subtree owns multiple view models.
74+
- Use `FrProvider.listenable` instead of `ChangeNotifierProvider` for a
75+
`FrViewModel` mixed with `FrChangeNotifierMx`.
7476
- Use `FrViewModel<M>` for method-driven state and `FrBlocViewModel<E, M>` when
7577
callers should dispatch events with `add(event)`.
7678

skills/flowr-usage/references/flutter.md

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -44,6 +44,9 @@ FrProvider(
4444
- `FrProvider` owns and disposes instances created by its factory. Use
4545
`FrProvider.value` only for an existing instance; use `FrProvider.multi` for
4646
several owned providers.
47+
- Use `FrProvider.listenable` for a `ChangeNotifier`, especially a
48+
`FrViewModel` mixed with `FrChangeNotifierMx`; it disposes notifier listeners
49+
and closes FlowR resources together.
4750
- Use `autoDisposeNotifier(notifier)` for an owned `ChangeNotifier`, such as a
4851
`FocusNode` or `TextEditingController`.
4952
- `FrView` rebuilds UI from state. `FrListener` handles side effects,

skills/fr-mvvm-contract/SKILL.md

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -318,7 +318,7 @@ where the root Provider creates `Dio`; new scaffolds use
318318
`factory Type(Dio dio)` constructor; base URL ownership belongs to
319319
`AppEnv.apiBaseUrl` and `createAppDio(AppEnv)`, never project skill config or a
320320
service annotation/constructor. Provide `AppEnvViewModel` through
321-
`ChangeNotifierProvider` with `FrChangeNotifierMx`, then key the Dio/business
321+
`FrProvider.listenable` with `FrChangeNotifierMx`, then key the Dio/business
322322
runtime subtree by environment identity so an environment update disposes the
323323
old Dio and business state. Clear installed persistent token/cookie stores
324324
before publishing that update. After first

skills/fr-mvvm-contract/assets/acdd_scaffold/lib/core/providers.dart.tmpl

Lines changed: 6 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -15,12 +15,13 @@ class AppProviders extends StatelessWidget {
1515
final Dio? dio;
1616

1717
@override
18-
Widget build(BuildContext context) => ChangeNotifierProvider(
19-
create: (context) => AppEnvViewModel(),
20-
child: FrProvider.multi([
18+
Widget build(BuildContext context) => FrProvider.multi(
19+
[
20+
FrProvider.listenable<AppEnvViewModel>((context) => AppEnvViewModel()),
2121
FrProvider((context) => AppLocaleViewModel()),
2222
FrProvider((context) => AppThemeViewModel()),
23-
], child: Consumer<AppEnvViewModel>(
23+
],
24+
child: Consumer<AppEnvViewModel>(
2425
builder: (context, appEnvViewModel, child) => _AppEnvironmentScope(
2526
key: ValueKey(
2627
'${appEnvViewModel.state.env}|${appEnvViewModel.state.apiBaseUrl}',
@@ -30,7 +31,7 @@ class AppProviders extends StatelessWidget {
3031
child: child!,
3132
),
3233
child: child,
33-
)),
34+
),
3435
);
3536
}
3637

skills/fr-mvvm-contract/references/acdd_scaffold.md

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -83,7 +83,7 @@ Do not make application launch part of the scaffold command.
8383
providers. `AppEnv.apiBaseUrl` is the single runtime base URL source and
8484
`createAppDio(AppEnv)` applies it with `BaseOptions`.
8585
- `AppEnvViewModel` mixes in `FrChangeNotifierMx` and is registered with
86-
`ChangeNotifierProvider`. A `Consumer` keys the Dio/business runtime subtree
86+
`FrProvider.listenable`. A `Consumer` keys the Dio/business runtime subtree
8787
by environment identity. Changing environment therefore disposes the old
8888
internally owned Dio, connections, and business ViewModels while preserving
8989
application-level Env, Locale, and Theme state. An externally injected test

skills/fr-mvvm-contract/scripts/tests/test_acdd_scaffold.py

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -212,7 +212,8 @@ def fake_runner(step: scaffold.CommandStep) -> None:
212212
self.assertIn("Dio createAppDio(AppEnv env)", interceptors_text)
213213
self.assertIn("BaseOptions(baseUrl: env.apiBaseUrl)", interceptors_text)
214214
self.assertIn("FrChangeNotifierMx<AppEnv>", env_text)
215-
self.assertIn("ChangeNotifierProvider", providers_text)
215+
self.assertIn("FrProvider.listenable<AppEnvViewModel>", providers_text)
216+
self.assertNotIn("ChangeNotifierProvider", providers_text)
216217
self.assertIn("Consumer<AppEnvViewModel>", providers_text)
217218
self.assertIn("_AppEnvironmentScope", providers_text)
218219
self.assertIn("createAppDio(env)", providers_text)

0 commit comments

Comments
 (0)