Skip to content

Commit 454eb75

Browse files
phananphanan
authored
Fix form sheet not closing after submit (#192)
When an action sheet's row pops itself and immediately opens a form sheet (Edit Album/Artist/Playlist/Station, Add Podcast, Create Playlist/Folder), the form sheet's onSubmit closure captured the outer route's BuildContext. By the time onSubmit resumed after the network round-trip, that route was fully disposed, so Navigator.pop / showOverlay on it silently failed — the sheet stayed open with no feedback. showFormSheet now hands its own (always-mounted) context to onSubmit, and every caller uses that for pop and overlay. Adds a regression test that reproduces the action-sheet -> form-sheet flow.
1 parent 153af44 commit 454eb75

9 files changed

Lines changed: 161 additions & 40 deletions

‎lib/ui/screens/add_podcast_sheet.dart‎

Lines changed: 6 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -14,21 +14,23 @@ Future<void> showAddPodcastDialog(BuildContext context) async {
1414
title: 'Add Podcast',
1515
submitLabel: 'Add',
1616
canSubmit: () => urlController.text.trim().isNotEmpty,
17-
onSubmit: () async {
17+
onSubmit: (sheetContext) async {
1818
final url = urlController.text.trim();
1919
if (url.isEmpty) return;
2020

2121
try {
2222
await podcastProvider.add(url: url);
23-
Navigator.pop(context);
24-
showOverlay(context, caption: 'Podcast added');
23+
if (!sheetContext.mounted) return;
24+
Navigator.pop(sheetContext);
25+
showOverlay(sheetContext, caption: 'Podcast added');
2526
} catch (e) {
27+
if (!sheetContext.mounted) return;
2628
final message =
2729
e is HttpResponseException && e.response.statusCode == 409
2830
? 'You are already subscribed to this podcast.'
2931
: 'Something wrong happened. Please try again.';
3032
showOverlay(
31-
context,
33+
sheetContext,
3234
caption: 'Error',
3335
message: message,
3436
icon: CupertinoIcons.exclamationmark_triangle,

‎lib/ui/screens/create_playlist_folder_sheet.dart‎

Lines changed: 6 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -12,16 +12,18 @@ Future<void> showCreatePlaylistFolderDialog(BuildContext context) async {
1212
title: 'New Folder',
1313
submitLabel: 'Create',
1414
canSubmit: () => controller.text.trim().isNotEmpty,
15-
onSubmit: () async {
15+
onSubmit: (sheetContext) async {
1616
final name = controller.text.trim();
1717
if (name.isEmpty) return;
1818

1919
try {
2020
await folderProvider.create(name: name);
21-
Navigator.pop(context);
22-
showOverlay(context, caption: 'Folder created');
21+
if (!sheetContext.mounted) return;
22+
Navigator.pop(sheetContext);
23+
showOverlay(sheetContext, caption: 'Folder created');
2324
} catch (_) {
24-
showOverlay(context,
25+
if (!sheetContext.mounted) return;
26+
showOverlay(sheetContext,
2527
caption: 'Error',
2628
message: 'Could not create folder.',
2729
icon: Icons.error_outline,

‎lib/ui/screens/create_playlist_sheet.dart‎

Lines changed: 6 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -16,7 +16,7 @@ Future<void> showCreatePlaylistDialog(BuildContext context) async {
1616
title: 'New Playlist',
1717
submitLabel: 'Create',
1818
canSubmit: () => nameController.text.trim().isNotEmpty,
19-
onSubmit: () async {
19+
onSubmit: (sheetContext) async {
2020
final name = nameController.text.trim();
2121
if (name.isEmpty) return;
2222

@@ -26,10 +26,12 @@ Future<void> showCreatePlaylistDialog(BuildContext context) async {
2626
description: descController.text.trim(),
2727
folderId: selectedFolderId,
2828
);
29-
Navigator.pop(context);
30-
showOverlay(context, caption: 'Playlist added');
29+
if (!sheetContext.mounted) return;
30+
Navigator.pop(sheetContext);
31+
showOverlay(sheetContext, caption: 'Playlist added');
3132
} catch (_) {
32-
showOverlay(context,
33+
if (!sheetContext.mounted) return;
34+
showOverlay(sheetContext,
3335
caption: 'Error',
3436
message: 'Could not create playlist.',
3537
icon: Icons.error_outline,

‎lib/ui/screens/edit_album_sheet.dart‎

Lines changed: 6 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -19,7 +19,7 @@ Future<void> showEditAlbumDialog(
1919
title: 'Edit Album',
2020
submitLabel: 'Save',
2121
canSubmit: () => nameController.text.trim().isNotEmpty,
22-
onSubmit: () async {
22+
onSubmit: (sheetContext) async {
2323
final name = nameController.text.trim();
2424
if (name.isEmpty) return;
2525

@@ -28,11 +28,13 @@ Future<void> showEditAlbumDialog(
2828

2929
try {
3030
await albumProvider.update(album, name: name, year: year);
31-
Navigator.pop(context);
32-
showOverlay(context, caption: 'Album updated');
31+
if (!sheetContext.mounted) return;
32+
Navigator.pop(sheetContext);
33+
showOverlay(sheetContext, caption: 'Album updated');
3334
} catch (_) {
35+
if (!sheetContext.mounted) return;
3436
showOverlay(
35-
context,
37+
sheetContext,
3638
caption: 'Error',
3739
message: 'Could not update album.',
3840
icon: Icons.error_outline,

‎lib/ui/screens/edit_artist_sheet.dart‎

Lines changed: 6 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -16,17 +16,19 @@ Future<void> showEditArtistDialog(
1616
title: 'Edit Artist',
1717
submitLabel: 'Save',
1818
canSubmit: () => nameController.text.trim().isNotEmpty,
19-
onSubmit: () async {
19+
onSubmit: (sheetContext) async {
2020
final name = nameController.text.trim();
2121
if (name.isEmpty) return;
2222

2323
try {
2424
await artistProvider.update(artist, name: name);
25-
Navigator.pop(context);
26-
showOverlay(context, caption: 'Artist updated');
25+
if (!sheetContext.mounted) return;
26+
Navigator.pop(sheetContext);
27+
showOverlay(sheetContext, caption: 'Artist updated');
2728
} catch (_) {
29+
if (!sheetContext.mounted) return;
2830
showOverlay(
29-
context,
31+
sheetContext,
3032
caption: 'Error',
3133
message: 'Could not update artist.',
3234
icon: Icons.error_outline,

‎lib/ui/screens/edit_playlist_sheet.dart‎

Lines changed: 6 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -21,7 +21,7 @@ Future<void> showEditPlaylistDialog(
2121
title: 'Edit Playlist',
2222
submitLabel: 'Save',
2323
canSubmit: () => nameController.text.trim().isNotEmpty,
24-
onSubmit: () async {
24+
onSubmit: (sheetContext) async {
2525
final name = nameController.text.trim();
2626
if (name.isEmpty) return;
2727

@@ -32,11 +32,13 @@ Future<void> showEditPlaylistDialog(
3232
description: descController.text.trim(),
3333
folderId: selectedFolderId,
3434
);
35-
Navigator.pop(context);
36-
showOverlay(context, caption: 'Playlist updated');
35+
if (!sheetContext.mounted) return;
36+
Navigator.pop(sheetContext);
37+
showOverlay(sheetContext, caption: 'Playlist updated');
3738
} catch (_) {
39+
if (!sheetContext.mounted) return;
3840
showOverlay(
39-
context,
41+
sheetContext,
4042
caption: 'Error',
4143
message: 'Could not update playlist.',
4244
icon: Icons.error_outline,

‎lib/ui/screens/edit_radio_station_sheet.dart‎

Lines changed: 6 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -23,7 +23,7 @@ Future<void> showEditRadioStationDialog(
2323
canSubmit: () =>
2424
nameController.text.trim().isNotEmpty &&
2525
urlController.text.trim().isNotEmpty,
26-
onSubmit: () async {
26+
onSubmit: (sheetContext) async {
2727
final name = nameController.text.trim();
2828
final url = urlController.text.trim();
2929
if (name.isEmpty || url.isEmpty) return;
@@ -42,11 +42,6 @@ Future<void> showEditRadioStationDialog(
4242
isPublic: isPublic,
4343
);
4444

45-
// If we just edited the station that's currently on air, the
46-
// player is still streaming the old URL (its setUrl was called
47-
// at play time). Restart the stream when the URL changed;
48-
// otherwise just refresh the OS media-session metadata so the
49-
// lock screen / notification picks up the new name.
5045
if (radioPlayer.currentStation?.id == station.id) {
5146
if (url != oldUrl) {
5247
radioPlayer.play(station).catchError((_) {});
@@ -55,11 +50,13 @@ Future<void> showEditRadioStationDialog(
5550
}
5651
}
5752

58-
Navigator.pop(context);
59-
showOverlay(context, caption: 'Station updated');
53+
if (!sheetContext.mounted) return;
54+
Navigator.pop(sheetContext);
55+
showOverlay(sheetContext, caption: 'Station updated');
6056
} catch (_) {
57+
if (!sheetContext.mounted) return;
6158
showOverlay(
62-
context,
59+
sheetContext,
6360
caption: 'Error',
6461
message: 'Could not update station.',
6562
icon: Icons.error_outline,

‎lib/ui/widgets/form_sheet.dart‎

Lines changed: 8 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -6,12 +6,17 @@ import 'package:flutter/services.dart';
66

77
/// A reusable bottom sheet with a title, form fields, and action buttons.
88
/// Used for creating/editing playlists, folders, radio stations, etc.
9+
///
10+
/// `onSubmit` receives the form sheet's own [BuildContext] — this is the
11+
/// context callers should use for `Navigator.pop` / `showOverlay` after an
12+
/// awaited network call. The outer context that opened the sheet may belong
13+
/// to a route that's already been dismissed by the time `onSubmit` resumes.
914
Future<void> showFormSheet(
1015
BuildContext context, {
1116
required String title,
1217
required Widget Function(BuildContext context, StateSetter setState) builder,
1318
required String submitLabel,
14-
required Future<void> Function() onSubmit,
19+
required Future<void> Function(BuildContext context) onSubmit,
1520
bool Function()? canSubmit,
1621
}) async {
1722
await showModalBottomSheet(
@@ -39,7 +44,7 @@ class _FormSheet extends StatefulWidget {
3944
final String title;
4045
final Widget Function(BuildContext context, StateSetter setState) builder;
4146
final String submitLabel;
42-
final Future<void> Function() onSubmit;
47+
final Future<void> Function(BuildContext context) onSubmit;
4348
final bool Function()? canSubmit;
4449

4550
const _FormSheet({
@@ -126,7 +131,7 @@ class _FormSheetState extends State<_FormSheet> {
126131
: () async {
127132
setState(() => _submitting = true);
128133
try {
129-
await widget.onSubmit();
134+
await widget.onSubmit(context);
130135
} finally {
131136
if (mounted) {
132137
setState(() => _submitting = false);

‎test/ui/widgets/form_sheet_test.dart‎

Lines changed: 111 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -17,7 +17,7 @@ void main() {
1717
context,
1818
title: 'Test Form',
1919
submitLabel: 'Submit',
20-
onSubmit: () async {},
20+
onSubmit: (_) async {},
2121
builder: (context, setState) => Column(
2222
children: [
2323
TextField(decoration: InputDecoration(hintText: 'Field 1')),
@@ -55,7 +55,7 @@ void main() {
5555
context,
5656
title: 'Test Form',
5757
submitLabel: 'Save',
58-
onSubmit: () async {},
58+
onSubmit: (_) async {},
5959
builder: (context, setState) => Column(
6060
children: [
6161
TextField(decoration: InputDecoration(hintText: 'Field 1')),
@@ -111,9 +111,9 @@ void main() {
111111
title: 'Test Form',
112112
submitLabel: 'Go',
113113
canSubmit: () => true,
114-
onSubmit: () async {
114+
onSubmit: (sheetContext) async {
115115
submitted = true;
116-
Navigator.pop(context);
116+
Navigator.pop(sheetContext);
117117
},
118118
builder: (context, setState) =>
119119
TextField(decoration: InputDecoration(hintText: 'Name')),
@@ -132,4 +132,111 @@ void main() {
132132
expect(submitted, isTrue);
133133
});
134134
});
135+
136+
group('onSubmit context lifetime', () {
137+
testWidgets(
138+
'sheet closes via onSubmit context even after the route that opened '
139+
'it is already gone',
140+
(tester) async {
141+
// Reproduces the artist/album action-sheet flow:
142+
// 1. Action sheet's Edit row runs
143+
// Navigator.pop(actionSheetContext)
144+
// showEditDialog(actionSheetContext) // form sheet opens
145+
// 2. The form sheet captures the now-doomed actionSheetContext.
146+
// 3. Save tapped, awaits a network round-trip.
147+
// 4. By the time onSubmit's body runs, actionSheetContext is
148+
// defunct, so Navigator.pop on it silently fails.
149+
//
150+
// The fix: showFormSheet hands its own (always-mounted) context
151+
// to onSubmit, so the body uses that for pop/showOverlay.
152+
153+
await tester.pumpWidget(buildTestApp(
154+
child: Builder(
155+
builder: (rootContext) => ElevatedButton(
156+
onPressed: () {
157+
Navigator.of(rootContext).push(MaterialPageRoute<void>(
158+
builder: (innerContext) => Scaffold(
159+
body: Center(
160+
child: ElevatedButton(
161+
onPressed: () {
162+
Navigator.pop(innerContext);
163+
showFormSheet(
164+
innerContext,
165+
title: 'Edit',
166+
submitLabel: 'Save',
167+
canSubmit: () => true,
168+
onSubmit: (sheetContext) async {
169+
// Simulate a network round-trip; the inner
170+
// route finishes its dismissal animation
171+
// during this gap.
172+
await Future<void>.delayed(
173+
const Duration(milliseconds: 50),
174+
);
175+
if (!sheetContext.mounted) return;
176+
Navigator.pop(sheetContext);
177+
},
178+
builder: (_, __) => const SizedBox(),
179+
);
180+
},
181+
child: const Text('Edit'),
182+
),
183+
),
184+
),
185+
));
186+
},
187+
child: const Text('Open'),
188+
),
189+
),
190+
));
191+
192+
await tester.tap(find.text('Open'));
193+
await tester.pumpAndSettle();
194+
195+
await tester.tap(find.text('Edit'));
196+
await tester.pumpAndSettle();
197+
198+
expect(find.text('Save'), findsOneWidget,
199+
reason: 'form sheet should be open');
200+
201+
await tester.tap(find.text('Save'));
202+
await tester.pumpAndSettle();
203+
204+
expect(find.text('Save'), findsNothing,
205+
reason: 'form sheet must close after Save');
206+
},
207+
);
208+
209+
testWidgets('onSubmit receives a mounted context', (tester) async {
210+
BuildContext? captured;
211+
212+
await tester.pumpWidget(buildTestApp(
213+
child: Builder(
214+
builder: (context) => ElevatedButton(
215+
onPressed: () => showFormSheet(
216+
context,
217+
title: 'Test',
218+
submitLabel: 'Save',
219+
canSubmit: () => true,
220+
onSubmit: (sheetContext) async {
221+
captured = sheetContext;
222+
Navigator.pop(sheetContext);
223+
},
224+
builder: (_, __) => const SizedBox(),
225+
),
226+
child: const Text('Open'),
227+
),
228+
),
229+
));
230+
231+
await tester.tap(find.text('Open'));
232+
await tester.pumpAndSettle();
233+
await tester.tap(find.text('Save'));
234+
await tester.pumpAndSettle();
235+
236+
expect(captured, isNotNull);
237+
// After pop, the captured context should be unmounted — proving it
238+
// was the form sheet's own context, not some outer ancestor.
239+
expect(captured!.mounted, isFalse);
240+
});
241+
});
135242
}

0 commit comments

Comments
 (0)