Skip to content

Fix form sheet not closing after submit#192

Merged
phanan merged 1 commit into
masterfrom
fix/form-sheet-dead-context
May 2, 2026
Merged

Fix form sheet not closing after submit#192
phanan merged 1 commit into
masterfrom
fix/form-sheet-dead-context

Conversation

@phanan
Copy link
Copy Markdown
Member

@phanan phanan commented May 2, 2026

Summary

When a popup/action sheet's row pops itself and immediately opens a form sheet — e.g. Edit Album/Artist/Playlist/Radio 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. Navigator.pop / showOverlay on the dead context silently failed, so the form sheet stayed open with no feedback. The mutation itself succeeded server-side, but the UI never confirmed it.

showFormSheet now hands its own always-mounted context to onSubmit, and every caller uses that for Navigator.pop and showOverlay. Each caller also gates with sheetContext.mounted after the await as a belt-and-braces.

Adds a regression test in form_sheet_test.dart that reproduces the action-sheet -> form-sheet flow: opens an inner route, has it pop itself and open the form sheet, awaits a delay in onSubmit, then asserts the form sheet closes after Save.

Test plan

  • flutter test — all 288 tests pass, including the two new onSubmit context lifetime tests.
  • Manual: long-press an album with edit permission -> Edit -> change nothing -> Save. Sheet must close and "Album updated" toast must appear.
  • Same for Edit Playlist, Edit Radio Station, Add Podcast, Create Playlist, Create Folder.

Summary by CodeRabbit

  • Bug Fixes

    • Form dialogs now reliably close and display success/error messages even if navigation changes occur during async submission, preventing lost overlays or stuck sheets.
  • Tests

    • Added tests validating form sheet lifecycle and context handling to prevent edge cases with unmounted dialogs during async operations.

@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented May 2, 2026

Caution

Review failed

The pull request is closed.

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 2991af5f-66d9-4ce3-9387-21c619d1d7d4

📥 Commits

Reviewing files that changed from the base of the PR and between a8408f0 and 807d02c.

📒 Files selected for processing (9)
  • lib/ui/screens/add_podcast_sheet.dart
  • lib/ui/screens/create_playlist_folder_sheet.dart
  • lib/ui/screens/create_playlist_sheet.dart
  • lib/ui/screens/edit_album_sheet.dart
  • lib/ui/screens/edit_artist_sheet.dart
  • lib/ui/screens/edit_playlist_sheet.dart
  • lib/ui/screens/edit_radio_station_sheet.dart
  • lib/ui/widgets/form_sheet.dart
  • test/ui/widgets/form_sheet_test.dart

📝 Walkthrough

Walkthrough

The PR changes showFormSheet so its onSubmit receives the sheet's BuildContext; submit handlers in seven screens were updated to use that sheetContext with mounted guards for post-await UI work, and tests were adjusted to exercise context lifetime and closing via sheetContext.

Changes

Form Sheet Context Lifecycle

Layer / File(s) Summary
API Signature
lib/ui/widgets/form_sheet.dart
onSubmit type changed from Future<void> Function() to Future<void> Function(BuildContext); docs updated.
Core Implementation
lib/ui/widgets/form_sheet.dart
Submit button now calls await widget.onSubmit(context) passing the sheet's BuildContext.
Caller Updates
lib/ui/screens/add_podcast_sheet.dart, lib/ui/screens/create_playlist_folder_sheet.dart, lib/ui/screens/create_playlist_sheet.dart, lib/ui/screens/edit_album_sheet.dart, lib/ui/screens/edit_artist_sheet.dart, lib/ui/screens/edit_playlist_sheet.dart, lib/ui/screens/edit_radio_station_sheet.dart
Each onSubmit handler signature changed to accept sheetContext; success and error paths use if (!sheetContext.mounted) return;, call Navigator.pop(sheetContext) and show overlays via sheetContext.
Tests / Verification
test/ui/widgets/form_sheet_test.dart
Tests updated to accept sheetContext; added group verifying onSubmit context lifetime, that sheet is closed with Navigator.pop(sheetContext), and that captured sheetContext becomes unmounted after pop.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

Poem

🐰
I bounced into a sheet so neat,
Held my context snug and sweet.
Awaited long, still safe and sound,
Popped myself without a bound.
Mounted guards kept chaos small — hooray, neat feat!

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely summarizes the main problem being fixed—form sheets not closing after submit.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/form-sheet-dead-context

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share
Review rate limit: 6/8 reviews remaining, refill in 7 minutes and 32 seconds.

Comment @coderabbitai help to get the list of available commands and usage tips.

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.
@phanan phanan force-pushed the fix/form-sheet-dead-context branch from a8408f0 to 807d02c Compare May 2, 2026 16:03
@phanan phanan merged commit 454eb75 into master May 2, 2026
1 of 2 checks passed
@phanan phanan deleted the fix/form-sheet-dead-context branch May 2, 2026 16:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant