Convert form dialogs to bottom sheets#145
Merged
Merged
Conversation
|
Caution Review failedPull request was closed or merged during review Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughReplaces multiple Cupertino alert dialogs with a reusable bottom-sheet form API ( Changes
Sequence DiagramsequenceDiagram
participant Caller as Screen / Caller
participant API as showFormSheet()
participant Sheet as _FormSheet (widget)
participant Builder as Form Builder
participant Submit as onSubmit Handler
Caller->>API: call showFormSheet(context, title, builder, onSubmit, canSubmit)
API->>Sheet: open modal bottom sheet (transparent bg, 85% height)
Sheet->>Sheet: render header, drag handle, scrollable form
Sheet->>Builder: invoke builder(context, setState)
Builder->>Sheet: provide form widgets (FormTextField, FormDropdown)
User->>Builder: enter/modify data
Builder->>Sheet: onChanged -> setState()
Sheet->>Sheet: evaluate canSubmit()
User->>Sheet: tap Submit
Sheet->>Sheet: if canSubmit true -> set _submitting = true (show spinner)
Sheet->>Submit: await onSubmit()
Submit->>Sheet: return success or throw error
Sheet->>Sheet: set _submitting = false
Sheet->>Caller: Navigator.pop() (close sheet) on success
Sheet->>Caller: showOverlay(success/error)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
phanan
force-pushed
the
feat/form-sheets
branch
5 times, most recently
from
3b4b809 to
d961514
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (1)
test/ui/screens/create_playlist_folder_test.dart (1)
37-62: Add coverage for the disabledCreatestate.This suite exercises open/create/cancel, but it doesn't assert the new
canSubmitguard. Please verify thatCreatestarts disabled and only enables after non-whitespace input so the new behavior is protected.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/ui/screens/create_playlist_folder_test.dart` around lines 37 - 62, Add a new test (or update the existing 'creates a folder successfully') to assert the disabled Create state: after pumping the sheet (tap 'Open' and pumpAndSettle) verify the Create button (find.text('Create')) is initially disabled (cannot be tapped or has disabled semantics), then enter only whitespace into the CupertinoTextField and assert Create remains disabled, and finally enter non-whitespace text (e.g., 'My Folder') and assert Create becomes enabled and triggers folderProviderMock.create(name: 'My Folder') when tapped; reference the test name(s) and widgets CupertinoTextField, find.text('Create'), and folderProviderMock.create to locate where to add these assertions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lib/ui/screens/add_podcast_sheet.dart`:
- Around line 17-37: The onSubmit handler currently calls Navigator.pop(context)
in both success and the catch block which closes the sheet on error; update the
onSubmit logic so that Navigator.pop(context) is only invoked after a successful
await podcastProvider.add(url: url) (i.e. move the pop into the try after add
and before showOverlay for success) and remove the Navigator.pop call from the
catch block so the sheet remains open on failure; keep showOverlay usage and the
existing error message logic and continue to reference urlController,
podcastProvider.add, showOverlay, and Navigator.pop to locate the changes.
In `@lib/ui/screens/create_playlist_folder_sheet.dart`:
- Around line 20-26: The sheet is being dismissed inside the catch block which
discards the entered name and gives no feedback; update the folder creation flow
so only folderProvider.create(name: name) success path calls
Navigator.pop(context) and showOverlay(context, caption: 'Folder created'), and
in the catch block keep the sheet open and surface the error (e.g., showOverlay
or a visible error widget) instead of calling Navigator.pop; apply the same
change to the new radio and playlist submit handlers so they do not call
Navigator.pop on failure and instead display the error to the user.
In `@lib/ui/screens/create_playlist_sheet.dart`:
- Around line 20-34: The onSubmit flow currently allows the Cancel button
(controlled by _submitting in form_sheet.dart) to remain enabled and silently
pops the sheet on error; wrap the async playlistProvider.create call with
setting _submitting = true before awaiting and _submitting = false in a finally
block, disable the Cancel button when _submitting is true, only call
Navigator.pop(context) on successful creation, and in the catch block keep the
sheet open and call showOverlay(context, caption: <error message or
e.toString()>) so users see the failure and can retry; also ensure any
Navigator.pop is guarded (e.g., only after success) so we don't pop the
underlying page if the sheet was dismissed during the await.
In `@lib/ui/screens/playlists.dart`:
- Around line 562-564: The label builder currently calls folders.firstWhere((f)
=> f.id == playlist.folderId).name which can throw if playlist.folderId refers
to a deleted folder; move the defensive handling into that label builder by
replacing the direct firstWhere call with a safe lookup (e.g., use
folders.firstWhereOrNull((f) => f.id == playlist.folderId)?.name ?? '<deleted
folder>' or check folders.any((f)=>f.id==playlist.folderId) before calling
firstWhere) and keep the earlier selectedFolder/selectedFolderId logic
unchanged; update any references to selectedFolder/playlist.folderId in the
label-building code to use the safe result.
In `@lib/ui/widgets/form_sheet.dart`:
- Around line 20-29: The form body inside _FormSheet currently lives in a
fixed-height SizedBox and can overflow; modify the widget tree in _FormSheet
(likely its build method where you render title, builder(context) and the
actions row) so the central form content is wrapped with Expanded ->
SingleChildScrollView (add appropriate Padding/physics as needed) to make it
scrollable, while leaving the actions Row (submit/cancel buttons) as a sibling
pinned at the bottom; ensure you call the provided builder(...) inside the
SingleChildScrollView so submitLabel, onSubmit and canSubmit logic remains
unchanged.
- Around line 7-13: Change showFormSheet to give submit handlers the sheet's
BuildContext and to disable the Cancel button while submitting: update the
onSubmit parameter type from Future<void> Function() to Future<void>
Function(BuildContext sheetContext), add an isSubmitting state inside
showFormSheet (managed via the existing StateSetter), set the Cancel button's
onPressed to null when isSubmitting is true, and when the submit handler is
invoked call await onSubmit(sheetContext) so the sheet can be dismissed safely
from the sheet context (or the sheet can call Navigator.of(sheetContext).pop()
after await), ensuring callers update their onSubmit implementations to accept
the sheet context.
---
Nitpick comments:
In `@test/ui/screens/create_playlist_folder_test.dart`:
- Around line 37-62: Add a new test (or update the existing 'creates a folder
successfully') to assert the disabled Create state: after pumping the sheet (tap
'Open' and pumpAndSettle) verify the Create button (find.text('Create')) is
initially disabled (cannot be tapped or has disabled semantics), then enter only
whitespace into the CupertinoTextField and assert Create remains disabled, and
finally enter non-whitespace text (e.g., 'My Folder') and assert Create becomes
enabled and triggers folderProviderMock.create(name: 'My Folder') when tapped;
reference the test name(s) and widgets CupertinoTextField, find.text('Create'),
and folderProviderMock.create to locate where to add these assertions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c45fc2d9-e38a-4a2c-98ac-bc2185f78cf1
📒 Files selected for processing (10)
lib/router.dartlib/ui/screens/add_podcast_sheet.dartlib/ui/screens/create_playlist_folder_sheet.dartlib/ui/screens/create_playlist_sheet.dartlib/ui/screens/playlists.dartlib/ui/screens/radio_stations.dartlib/ui/widgets/form_sheet.dartlib/ui/widgets/widgets.darttest/ui/screens/create_playlist_folder_test.darttest/ui/screens/create_playlist_sheet_test.dart
Comment on lines
+20
to
+26
| try { | ||
| await folderProvider.create(name: name); | ||
| Navigator.pop(context); | ||
| showOverlay(context, caption: 'Folder created'); | ||
| } catch (_) { | ||
| Navigator.pop(context); | ||
| } |
There was a problem hiding this comment.
Don't dismiss the form on failed creates.
This catch drops the entered name and gives the user no feedback, so any transient failure forces them to start over. Please keep the sheet open, surface the error, and only pop after folderProvider.create succeeds. The same dismiss-on-error pattern also shows up in the new radio and playlist submit handlers in this PR.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/ui/screens/create_playlist_folder_sheet.dart` around lines 20 - 26, The
sheet is being dismissed inside the catch block which discards the entered name
and gives no feedback; update the folder creation flow so only
folderProvider.create(name: name) success path calls Navigator.pop(context) and
showOverlay(context, caption: 'Folder created'), and in the catch block keep the
sheet open and surface the error (e.g., showOverlay or a visible error widget)
instead of calling Navigator.pop; apply the same change to the new radio and
playlist submit handlers so they do not call Navigator.pop on failure and
instead display the error to the user.
Comment on lines
+7
to
+13
| Future<void> showFormSheet( | ||
| BuildContext context, { | ||
| required String title, | ||
| required Widget Function(BuildContext context, StateSetter setState) builder, | ||
| required String submitLabel, | ||
| required Future<void> Function() onSubmit, | ||
| bool Function()? canSubmit, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n lib/ui/widgets/form_sheet.dartRepository: koel/player
Length of output: 9894
Dismissal ownership and Cancel button behavior create a race condition risk.
onSubmit has no access to the sheet context, forcing callers to dismiss using the presenter context. Meanwhile, the Cancel button remains active during submission—a user can dismiss the sheet mid-save, and any subsequent context.pop() from onSubmit's completion handler will pop the underlying page instead. Either automatically dismiss the sheet after onSubmit completes, or pass the sheet's BuildContext to onSubmit and disable Cancel while submitting.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/ui/widgets/form_sheet.dart` around lines 7 - 13, Change showFormSheet to
give submit handlers the sheet's BuildContext and to disable the Cancel button
while submitting: update the onSubmit parameter type from Future<void>
Function() to Future<void> Function(BuildContext sheetContext), add an
isSubmitting state inside showFormSheet (managed via the existing StateSetter),
set the Cancel button's onPressed to null when isSubmitting is true, and when
the submit handler is invoked call await onSubmit(sheetContext) so the sheet can
be dismissed safely from the sheet context (or the sheet can call
Navigator.of(sheetContext).pop() after await), ensuring callers update their
onSubmit implementations to accept the sheet context.
d961514 to
81dad18
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
lib/ui/screens/playlists.dart (2)
543-554: Silent failure on error - consider user feedback.Consistent with the other form sheets, consider adding error feedback when playlist update fails.
♻️ Proposed fix to add error feedback
try { await provider.update( playlist, name: name, description: descController.text.trim(), folderId: selectedFolderId, ); Navigator.pop(context); showOverlay(context, caption: 'Playlist updated'); } catch (_) { Navigator.pop(context); + showOverlay( + context, + caption: 'Failed to update playlist', + icon: CupertinoIcons.exclamationmark_triangle, + ); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/ui/screens/playlists.dart` around lines 543 - 554, The catch block currently swallows errors silently after calling Navigator.pop; modify the try/catch around provider.update (the call to provider.update(playlist,...)) to catch the exception as a variable (e.g., catch (e)) and provide user feedback by calling showOverlay (or another UI error notifier) after Navigator.pop with a descriptive error caption such as "Failed to update playlist" and include the exception message (e.toString()) for clarity; ensure you still pop the sheet (Navigator.pop(context)) before showing the error overlay.
339-344: Missing success feedback for folder rename.Unlike the other form sheets (
Folder created,Playlist added,Playlist updated), the rename folder flow doesn't show a success overlay. Consider adding one for consistency.♻️ Proposed fix to add success feedback
onSubmit: () async { final name = controller.text.trim(); if (name.isEmpty) return; await provider.rename(folder, name: name); Navigator.pop(context); + showOverlay(context, caption: 'Folder renamed'); },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/ui/screens/playlists.dart` around lines 339 - 344, The rename flow in onSubmit calls provider.rename(...) and pops the sheet but lacks the success overlay shown elsewhere; after the await provider.rename(folder, name: name) call, invoke the same success feedback used by the other form sheets (the overlay/snackbar routine used for "Folder created"/"Playlist added"/"Playlist updated") with a message like "Folder renamed" and then call Navigator.pop(context); this will keep behavior consistent with the other flows.lib/ui/screens/create_playlist_sheet.dart (1)
23-33: Silent failure on error - consider user feedback.When playlist creation fails, the sheet dismisses without informing the user. For consistency with the podcast addition flow, consider showing an error overlay.
♻️ Proposed fix to add error feedback
try { await playlistProvider.create( name: name, description: descController.text.trim(), folderId: selectedFolderId, ); Navigator.pop(context); showOverlay(context, caption: 'Playlist added'); } catch (_) { Navigator.pop(context); + showOverlay( + context, + caption: 'Failed to create playlist', + icon: CupertinoIcons.exclamationmark_triangle, + ); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/ui/screens/create_playlist_sheet.dart` around lines 23 - 33, The catch block in the create playlist sheet silently dismisses the sheet on failure; update the error handling around playlistProvider.create (in create_playlist_sheet.dart) to present user feedback by calling showOverlay with an error caption (e.g., "Failed to add playlist" or include the exception message) before or after Navigator.pop(context), and optionally log the caught error; specifically modify the catch (_) to capture the exception (catch (e)) and call showOverlay(context, caption: 'Failed to add playlist: ${e.toString()}') or a safe generic message, keeping existing Navigator.pop(context) behavior.lib/ui/screens/create_playlist_folder_sheet.dart (1)
19-25: Silent failure on error - consider user feedback.When the folder creation fails, the sheet is dismissed without informing the user. Unlike the podcast addition flow which shows an error overlay, this silently swallows the exception. Consider adding error feedback for consistency.
♻️ Proposed fix to add error feedback
try { await folderProvider.create(name: name); Navigator.pop(context); showOverlay(context, caption: 'Folder created'); } catch (_) { Navigator.pop(context); + showOverlay( + context, + caption: 'Failed to create folder', + icon: CupertinoIcons.exclamationmark_triangle, + ); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/ui/screens/create_playlist_folder_sheet.dart` around lines 19 - 25, The catch block after calling folderProvider.create(name: name) currently swallows errors and just dismisses the sheet; update the catch to capture the exception (e.g., catch (e)) and show user-facing error feedback before/after calling Navigator.pop(context)—use the existing showOverlay function to display a clear error caption like "Failed to create folder: <error message>" (and mark it as an error if supported) so failure mirrors the podcast flow and surfaces the exception to the user.lib/ui/widgets/form_sheet.dart (1)
54-137: Consider clarifying state ownership between_FormSheetStateandStatefulBuilder.
_submittingis a field of_FormSheetState(line 55), but you're usingStatefulBuilder'ssetState(line 68) to trigger rebuilds when modifying it (lines 115, 120). This works because the assignment happens within the closure before the rebuild, but it conflates two state management approaches.One option is to capture a reference to the outer
setStateand use it for_submitting:♻️ Possible refactor to clarify state ownership
class _FormSheetState extends State<_FormSheet> { var _submitting = false; + late final StateSetter _outerSetState = setState; `@override` Widget build(BuildContext context) { return GradientDecoratedContainer( child: Padding( ... child: StatefulBuilder( - builder: (context, setState) { + builder: (context, innerSetState) { return Column( ... onPressed: _submitting || ... ? null : () async { - setState(() => _submitting = true); + _outerSetState(() => _submitting = true); try { await widget.onSubmit(); } finally { if (mounted) { - setState(() => _submitting = false); + _outerSetState(() => _submitting = false); } } }, ... - widget.builder(context, setState), + widget.builder(context, innerSetState),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/ui/widgets/form_sheet.dart` around lines 54 - 137, The StatefulBuilder's builder currently shadows the State.setState parameter named setState, causing ambiguity when updating the outer _submitting field; rename the builder's second parameter (e.g., to innerSetState) so widget.builder(context, innerSetState) keeps using the local builder state while calls that update _submitting use the outer setState (now unshadowed) — update the two places that set _submitting (setting true before await and setting false in finally) to call the outer setState so state ownership is clear.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lib/ui/screens/radio_stations.dart`:
- Around line 333-335: The edit flow currently swallows errors in the catch (_)
block (inside the edit sheet handler) by only calling Navigator.pop(context);
instead of notifying the user; change the catch to capture the exception (e.g.
catch (e) or catch (e, st)) and reuse the same error-extraction and display
logic used by _showAddStation (lines ~198-214) to surface the error
overlay/message to the user; keep the sheet open or close it only after showing
the overlay per the add flow so users see the failure instead of silently losing
their changes.
---
Nitpick comments:
In `@lib/ui/screens/create_playlist_folder_sheet.dart`:
- Around line 19-25: The catch block after calling folderProvider.create(name:
name) currently swallows errors and just dismisses the sheet; update the catch
to capture the exception (e.g., catch (e)) and show user-facing error feedback
before/after calling Navigator.pop(context)—use the existing showOverlay
function to display a clear error caption like "Failed to create folder: <error
message>" (and mark it as an error if supported) so failure mirrors the podcast
flow and surfaces the exception to the user.
In `@lib/ui/screens/create_playlist_sheet.dart`:
- Around line 23-33: The catch block in the create playlist sheet silently
dismisses the sheet on failure; update the error handling around
playlistProvider.create (in create_playlist_sheet.dart) to present user feedback
by calling showOverlay with an error caption (e.g., "Failed to add playlist" or
include the exception message) before or after Navigator.pop(context), and
optionally log the caught error; specifically modify the catch (_) to capture
the exception (catch (e)) and call showOverlay(context, caption: 'Failed to add
playlist: ${e.toString()}') or a safe generic message, keeping existing
Navigator.pop(context) behavior.
In `@lib/ui/screens/playlists.dart`:
- Around line 543-554: The catch block currently swallows errors silently after
calling Navigator.pop; modify the try/catch around provider.update (the call to
provider.update(playlist,...)) to catch the exception as a variable (e.g., catch
(e)) and provide user feedback by calling showOverlay (or another UI error
notifier) after Navigator.pop with a descriptive error caption such as "Failed
to update playlist" and include the exception message (e.toString()) for
clarity; ensure you still pop the sheet (Navigator.pop(context)) before showing
the error overlay.
- Around line 339-344: The rename flow in onSubmit calls provider.rename(...)
and pops the sheet but lacks the success overlay shown elsewhere; after the
await provider.rename(folder, name: name) call, invoke the same success feedback
used by the other form sheets (the overlay/snackbar routine used for "Folder
created"/"Playlist added"/"Playlist updated") with a message like "Folder
renamed" and then call Navigator.pop(context); this will keep behavior
consistent with the other flows.
In `@lib/ui/widgets/form_sheet.dart`:
- Around line 54-137: The StatefulBuilder's builder currently shadows the
State.setState parameter named setState, causing ambiguity when updating the
outer _submitting field; rename the builder's second parameter (e.g., to
innerSetState) so widget.builder(context, innerSetState) keeps using the local
builder state while calls that update _submitting use the outer setState (now
unshadowed) — update the two places that set _submitting (setting true before
await and setting false in finally) to call the outer setState so state
ownership is clear.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2c60d9f9-b2f0-4669-9112-41c4397e209d
⛔ Files ignored due to path filters (1)
pubspec.lockis excluded by!**/*.lock
📒 Files selected for processing (10)
lib/router.dartlib/ui/screens/add_podcast_sheet.dartlib/ui/screens/create_playlist_folder_sheet.dartlib/ui/screens/create_playlist_sheet.dartlib/ui/screens/playlists.dartlib/ui/screens/radio_stations.dartlib/ui/widgets/form_sheet.dartlib/ui/widgets/widgets.darttest/ui/screens/create_playlist_folder_test.darttest/ui/screens/create_playlist_sheet_test.dart
phanan
force-pushed
the
feat/form-sheets
branch
2 times, most recently
from
3eb6c4b to
d25cc62
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (4)
lib/ui/screens/playlists.dart (1)
531-532:⚠️ Potential issue | 🟡 MinorNormalize a missing folder id before binding the dropdown.
If
playlist.folderIdno longer exists infolderProvider.folders,selectedFolderIdis fed intoDropdownButtonas a value that is not present initems. That can assert in debug, and when there are no folders left it silently resubmits the stale id. Coerce the initial selection tonullwhen the folder is missing.🛡️ Minimal fix
final folders = folderProvider.folders; String? selectedFolderId = playlist.folderId; + if (selectedFolderId != null && + !folders.any((f) => f.id == selectedFolderId)) { + selectedFolderId = null; + }Also applies to: 548-549, 573-575
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/ui/screens/playlists.dart` around lines 531 - 532, Normalize the initial dropdown selection by coercing playlist.folderId to null when it is not present in the current folderProvider.folders before binding to the DropdownButton: replace direct use of String? selectedFolderId = playlist.folderId with logic that sets selectedFolderId = playlist.folderId if folderProvider.folders contains that id, otherwise selectedFolderId = null; apply the same fix to the other identical occurrences that read playlist.folderId for dropdown binding (the other DropdownButton / selectedFolderId patterns) so the DropdownButton value is never a stale id missing from folderProvider.folders.lib/ui/widgets/form_sheet.dart (2)
8-15:⚠️ Potential issue | 🟠 MajorPass the sheet context into
onSubmitand block dismissal while saving.Every new caller in this PR pops with the presenter context after awaiting provider work, and Cancel stays active while
_submitting. If the sheet is gone before the await completes, that later pop can hit the page underneath instead of the sheet.🔒 Safer API shape
- required Future<void> Function() onSubmit, + required Future<void> Function(BuildContext sheetContext) onSubmit, ... - final Future<void> Function() onSubmit; + final Future<void> Function(BuildContext sheetContext) onSubmit; ... - onPressed: () => Navigator.pop(context), + onPressed: + _submitting ? null : () => Navigator.pop(context), ... - await widget.onSubmit(); + await widget.onSubmit(context);Also applies to: 36-40, 97-125
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/ui/widgets/form_sheet.dart` around lines 8 - 15, Change the showFormSheet API so the onSubmit callback receives the sheet BuildContext (e.g. replace Future<void> Function() onSubmit with Future<void> Function(BuildContext sheetContext) onSubmit) and, inside showFormSheet (the submit button handler), set a local _submitting state true, call await onSubmit(sheetContext), and only reset and close after that completes; also prevent dismissal while saving by disabling barrierDismissible (or wrapping content in WillPopScope to block back) and disabling the Cancel button when _submitting is true. Apply the same pattern to the other sheet usages referenced (lines noted in the review) so callers use the provided sheetContext for any Navigator.pop calls to avoid popping the underlying page.
69-93:⚠️ Potential issue | 🟠 MajorMake the form body scrollable.
The sheet is fixed at 85% height, but the form content is inserted directly into a
Column. Multi-field sheets like playlist/radio edit will overflow once the keyboard or larger text scale reduces the available space.📱 Suggested layout fix
- return Column( - mainAxisSize: MainAxisSize.min, + return Column( + mainAxisSize: MainAxisSize.max, crossAxisAlignment: CrossAxisAlignment.stretch, children: [ @@ - widget.builder(context, setState), - const SizedBox(height: 20), + Expanded( + child: SingleChildScrollView( + padding: const EdgeInsets.only(bottom: 20), + child: widget.builder(context, setState), + ), + ),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/ui/widgets/form_sheet.dart` around lines 69 - 93, The Column currently places widget.builder(...) directly which causes overflow; wrap the variable-height form area in a scrollable flexible container so the sheet can scroll when space is tight. Specifically, in the FormSheet build where the Column is returned, replace the static insertion of widget.builder(context, setState) and the adjacent SizedBoxes with a Flexible (or Expanded) containing a SingleChildScrollView (and a Padding if needed) that hosts the builder output so the form body becomes scrollable and shrinks/grows with available space; keep the drag handle and title outside this scrollable region.lib/ui/screens/add_podcast_sheet.dart (1)
25-36:⚠️ Potential issue | 🟠 MajorKeep the podcast sheet open when add fails.
The
catchpath dismisses the sheet on every error, which throws away the RSS URL the user just entered.showOverlayis independent of the sheet, so remove the pop on failure and let the user retry in place.💡 Minimal fix
- } catch (e) { - Navigator.pop(context); + } catch (e) { final message = e is HttpResponseException && e.response.statusCode == 409 ? 'You are already subscribed to this podcast.'🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/ui/screens/add_podcast_sheet.dart` around lines 25 - 36, The catch block in add_podcast_sheet.dart currently calls Navigator.pop(context) which closes the Add Podcast sheet on any error and loses the entered RSS URL; remove the Navigator.pop(context) call from the error handling path (leave any Navigator.pop on the success path intact), so that on exception you only call showOverlay(...) with the error message (and still handle the 409-specific message), allowing the sheet to stay open for user retry.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lib/ui/screens/playlists.dart`:
- Around line 543-554: The catch block currently calls Navigator.pop(context)
which closes the edit sheet on failure and loses the user's edits; change the
error path so you do NOT call Navigator.pop(context) in the catch, and instead
surface the failure (e.g., call showOverlay or a SnackBar with the caught error
message) so the form (fields: name, descController, selectedFolderId) remains
open for retry; keep the successful-path Navigator.pop(context) and
showOverlay('Playlist updated') after provider.update(playlist, ...) only.
In `@lib/ui/screens/radio_stations.dart`:
- Around line 189-215: The catch blocks currently call Navigator.pop(context)
which closes the station form even on validation/network failure; remove the
Navigator.pop(context) calls from the catch handlers so the form remains open
and the user's input is preserved (keep the success path that calls
Navigator.pop after provider.create completes), still show the error overlay
with message/icon in the catch, and rely on the existing finally in
showFormSheet to reset the spinner; make the same change for the duplicate catch
at the other create/edit block that mirrors lines 323-335 (i.e., eliminate
popping on failure while keeping success pop and error overlay).
---
Duplicate comments:
In `@lib/ui/screens/add_podcast_sheet.dart`:
- Around line 25-36: The catch block in add_podcast_sheet.dart currently calls
Navigator.pop(context) which closes the Add Podcast sheet on any error and loses
the entered RSS URL; remove the Navigator.pop(context) call from the error
handling path (leave any Navigator.pop on the success path intact), so that on
exception you only call showOverlay(...) with the error message (and still
handle the 409-specific message), allowing the sheet to stay open for user
retry.
In `@lib/ui/screens/playlists.dart`:
- Around line 531-532: Normalize the initial dropdown selection by coercing
playlist.folderId to null when it is not present in the current
folderProvider.folders before binding to the DropdownButton: replace direct use
of String? selectedFolderId = playlist.folderId with logic that sets
selectedFolderId = playlist.folderId if folderProvider.folders contains that id,
otherwise selectedFolderId = null; apply the same fix to the other identical
occurrences that read playlist.folderId for dropdown binding (the other
DropdownButton / selectedFolderId patterns) so the DropdownButton value is never
a stale id missing from folderProvider.folders.
In `@lib/ui/widgets/form_sheet.dart`:
- Around line 8-15: Change the showFormSheet API so the onSubmit callback
receives the sheet BuildContext (e.g. replace Future<void> Function() onSubmit
with Future<void> Function(BuildContext sheetContext) onSubmit) and, inside
showFormSheet (the submit button handler), set a local _submitting state true,
call await onSubmit(sheetContext), and only reset and close after that
completes; also prevent dismissal while saving by disabling barrierDismissible
(or wrapping content in WillPopScope to block back) and disabling the Cancel
button when _submitting is true. Apply the same pattern to the other sheet
usages referenced (lines noted in the review) so callers use the provided
sheetContext for any Navigator.pop calls to avoid popping the underlying page.
- Around line 69-93: The Column currently places widget.builder(...) directly
which causes overflow; wrap the variable-height form area in a scrollable
flexible container so the sheet can scroll when space is tight. Specifically, in
the FormSheet build where the Column is returned, replace the static insertion
of widget.builder(context, setState) and the adjacent SizedBoxes with a Flexible
(or Expanded) containing a SingleChildScrollView (and a Padding if needed) that
hosts the builder output so the form body becomes scrollable and shrinks/grows
with available space; keep the drag handle and title outside this scrollable
region.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 969acdaf-3cde-4eb7-b71a-45a84090667d
⛔ Files ignored due to path filters (1)
pubspec.lockis excluded by!**/*.lock
📒 Files selected for processing (10)
lib/router.dartlib/ui/screens/add_podcast_sheet.dartlib/ui/screens/create_playlist_folder_sheet.dartlib/ui/screens/create_playlist_sheet.dartlib/ui/screens/playlists.dartlib/ui/screens/radio_stations.dartlib/ui/widgets/form_sheet.dartlib/ui/widgets/widgets.darttest/ui/screens/create_playlist_folder_test.darttest/ui/screens/create_playlist_sheet_test.dart
✅ Files skipped from review due to trivial changes (1)
- lib/ui/widgets/widgets.dart
🚧 Files skipped from review as they are similar to previous changes (5)
- lib/router.dart
- test/ui/screens/create_playlist_sheet_test.dart
- test/ui/screens/create_playlist_folder_test.dart
- lib/ui/screens/create_playlist_folder_sheet.dart
- lib/ui/screens/create_playlist_sheet.dart
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
lib/ui/widgets/form_sheet.dart (2)
222-225: Focus border color doesn't match PR objectives.The PR objectives specify
AppColors.highlightfor the focus border, but line 224 usesColors.white54. Consider usingAppColors.highlightfor consistency with the stated design:focusedBorder: OutlineInputBorder( borderRadius: BorderRadius.circular(8), - borderSide: const BorderSide(color: Colors.white54, width: 1), + borderSide: const BorderSide(color: AppColors.highlight, width: 1), ),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/ui/widgets/form_sheet.dart` around lines 222 - 225, The focusedBorder OutlineInputBorder currently uses Colors.white54 but should use the theme highlight color; update the BorderSide color in the focusedBorder (the OutlineInputBorder inside the FormSheet widget / the input decoration where focusedBorder is defined) to AppColors.highlight and ensure AppColors is imported or available in that file so the focus state uses the PR-specified AppColors.highlight.
112-125: State management mixes widget state withStatefulBuildersetState.
_submittingis declared on_FormSheetState(line 55) but mutated via theStatefulBuilder'ssetState(lines 117, 122). This works because the variable is captured in closure and StatefulBuilder rebuilds its subtree, but it's architecturally confusing. Consider either using_FormSheetState.setStatedirectly or moving_submittinginto aValueNotifierscoped to the builder.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/ui/widgets/form_sheet.dart` around lines 112 - 125, The code mutates the _submitting field of _FormSheetState from inside a StatefulBuilder's setState, which mixes state owners and is confusing; update the button handler to mutate _submitting via the owning State's setState (call _FormSheetState.setState) instead of the StatefulBuilder's setState, or alternatively move _submitting into a ValueNotifier<bool> scoped to the builder and update/listen to that notifier; ensure references to widget.canSubmit and widget.onSubmit remain unchanged and that the mounted check and final reset of _submitting (or notifier.value) happen after await widget.onSubmit() completes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lib/ui/screens/create_playlist_sheet.dart`:
- Around line 55-58: The labelBuilder closure currently uses
folders.firstWhere((f) => f.id == id) which can throw a StateError if folders
changes; update labelBuilder in create_playlist_sheet.dart to safely handle
missing matches by either using firstWhere with an orElse that returns a
fallback Folder (or null-handling) or by using collection's firstWhereOrNull and
then returning a default string (e.g., 'No folder' or empty) when the result is
null; ensure you reference the labelBuilder and folders variables and handle the
null/fallback case before accessing .name.
---
Nitpick comments:
In `@lib/ui/widgets/form_sheet.dart`:
- Around line 222-225: The focusedBorder OutlineInputBorder currently uses
Colors.white54 but should use the theme highlight color; update the BorderSide
color in the focusedBorder (the OutlineInputBorder inside the FormSheet widget /
the input decoration where focusedBorder is defined) to AppColors.highlight and
ensure AppColors is imported or available in that file so the focus state uses
the PR-specified AppColors.highlight.
- Around line 112-125: The code mutates the _submitting field of _FormSheetState
from inside a StatefulBuilder's setState, which mixes state owners and is
confusing; update the button handler to mutate _submitting via the owning
State's setState (call _FormSheetState.setState) instead of the
StatefulBuilder's setState, or alternatively move _submitting into a
ValueNotifier<bool> scoped to the builder and update/listen to that notifier;
ensure references to widget.canSubmit and widget.onSubmit remain unchanged and
that the mounted check and final reset of _submitting (or notifier.value) happen
after await widget.onSubmit() completes.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b0aaa770-5e84-4ab6-97d0-017ba85881f2
⛔ Files ignored due to path filters (1)
pubspec.lockis excluded by!**/*.lock
📒 Files selected for processing (10)
lib/router.dartlib/ui/screens/add_podcast_sheet.dartlib/ui/screens/create_playlist_folder_sheet.dartlib/ui/screens/create_playlist_sheet.dartlib/ui/screens/playlists.dartlib/ui/screens/radio_stations.dartlib/ui/widgets/form_sheet.dartlib/ui/widgets/widgets.darttest/ui/screens/create_playlist_folder_test.darttest/ui/screens/create_playlist_sheet_test.dart
✅ Files skipped from review due to trivial changes (1)
- lib/ui/widgets/widgets.dart
🚧 Files skipped from review as they are similar to previous changes (6)
- lib/router.dart
- test/ui/screens/create_playlist_sheet_test.dart
- lib/ui/screens/create_playlist_folder_sheet.dart
- test/ui/screens/create_playlist_folder_test.dart
- lib/ui/screens/playlists.dart
- lib/ui/screens/radio_stations.dart
phanan
force-pushed
the
feat/form-sheets
branch
from
d25cc62 to
e9624a2
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (9)
lib/ui/screens/add_podcast_sheet.dart (1)
25-37:⚠️ Potential issue | 🟠 MajorSheet dismisses on error, discarding the typed URL.
The
catchblock callsNavigator.pop(context)before showing the error overlay, losing the user's typed podcast URL. Keep the sheet open on failure so users can correct the URL and retry.🛠️ Suggested fix
} catch (e) { - Navigator.pop(context); final message = e is HttpResponseException && e.response.statusCode == 409 ? 'You are already subscribed to this podcast.' : 'Something wrong happened. Please try again.'; showOverlay( context, caption: 'Error', message: message, icon: CupertinoIcons.exclamationmark_triangle, ); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/ui/screens/add_podcast_sheet.dart` around lines 25 - 37, The catch block currently calls Navigator.pop(context) which closes the add-podcast sheet and discards the typed URL; remove that Navigator.pop(context) call in the error path and only call showOverlay(...) to display the error so the sheet remains open for the user to correct the URL; ensure any successful path still invokes Navigator.pop(context) as before and that the error handling around e, message, and showOverlay stays intact.lib/ui/screens/playlists.dart (2)
576-579:⚠️ Potential issue | 🟡 Minor
firstWherewithoutorElsecan throw if folder is deleted.Same issue as in
create_playlist_sheet.dart: ifplaylist.folderIdreferences a folder that no longer exists,firstWherethrowsStateError. Use a safe lookup.🛠️ Suggested fix
labelBuilder: (id) { if (id == null) return 'No folder'; - return folders.firstWhere((f) => f.id == id).name; + final folder = folders.cast<PlaylistFolder?>().firstWhere( + (f) => f?.id == id, + orElse: () => null, + ); + return folder?.name ?? 'Unknown folder'; },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/ui/screens/playlists.dart` around lines 576 - 579, The labelBuilder closure uses folders.firstWhere(...) which will throw a StateError if the referenced folder id is missing; change the lookup in labelBuilder to a safe lookup (e.g., use firstWhere with an orElse that returns null or a fallback object, or use folders.indexWhere and check for -1) and return a default string like 'Unknown folder' or 'No folder' when not found; update the labelBuilder lambda and any related code that assumes a non-null folder (referencing folders, folder.id, folder.name, and labelBuilder) to handle the missing-folder case safely.
552-554:⚠️ Potential issue | 🟠 MajorEdit playlist silently discards changes on error.
The
catchblock dismisses the sheet without feedback, causing users to lose their edited name, description, and folder selection with no indication of failure. Keep the sheet open and show an error message.🛠️ Suggested fix
} catch (_) { - Navigator.pop(context); + showOverlay( + context, + caption: 'Error', + message: 'Could not update playlist.', + icon: CupertinoIcons.exclamationmark_triangle, + ); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/ui/screens/playlists.dart` around lines 552 - 554, The catch block currently swallows errors and calls Navigator.pop(context), which closes the edit sheet and discards the user's edits; instead remove the Navigator.pop(context) from the catch, keep the sheet open, and display an error to the user (e.g. use ScaffoldMessenger.of(context).showSnackBar(...) or showDialog(...) with a clear message and optionally the error details). Update the catch handler that surrounds the edit/save logic in playlists.dart so it logs the error and shows the SnackBar/dialog (referencing the existing catch block that calls Navigator.pop(context)) rather than dismissing the sheet.lib/ui/screens/create_playlist_sheet.dart (2)
31-33:⚠️ Potential issue | 🟠 MajorSilent error handling discards user input without feedback.
The
catchblock dismisses the sheet without showing any error message, causing users to lose their entered name, description, and folder selection with no indication of what went wrong. Keep the sheet open on failure and display an error message so users can retry.🛠️ Suggested fix
} catch (_) { - Navigator.pop(context); + showOverlay( + context, + caption: 'Error', + message: 'Could not create playlist.', + icon: CupertinoIcons.exclamationmark_triangle, + ); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/ui/screens/create_playlist_sheet.dart` around lines 31 - 33, The catch block currently swallows errors and calls Navigator.pop(context), discarding user input; remove the Navigator.pop(context) from the catch and instead surface the error to the user (e.g., use ScaffoldMessenger.of(context).showSnackBar(...) or set an error state and call setState to display an inline error message in the CreatePlaylistSheet), leaving any TextEditingController values intact so the sheet stays open and the user can retry; update the catch that wraps the playlist creation logic (the block containing Navigator.pop(context)) to show the error message and preserve input rather than dismissing the sheet.
55-58:⚠️ Potential issue | 🟡 Minor
firstWherewithoutorElsecan throw if folder is deleted.If
foldersis mutated between whenitemsis built and whenlabelBuilderis called (e.g., a folder is deleted),firstWherethrowsStateError. Use a safe lookup pattern.🛠️ Suggested fix
labelBuilder: (id) { if (id == null) return 'No folder'; - return folders.firstWhere((f) => f.id == id).name; + final folder = folders.cast<PlaylistFolder?>().firstWhere( + (f) => f?.id == id, + orElse: () => null, + ); + return folder?.name ?? 'Unknown folder'; },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/ui/screens/create_playlist_sheet.dart` around lines 55 - 58, The labelBuilder closure risks throwing StateError when folders.firstWhere can't find a match; update labelBuilder (the closure using folders and id) to perform a safe lookup instead of plain firstWhere — e.g., use firstWhere with an orElse that returns null/placeholder or use indexWhere to check existence before accessing .name, and return a fallback like 'No folder' or 'Unknown folder' when no folder with the given id exists.lib/ui/widgets/form_sheet.dart (2)
69-93:⚠️ Potential issue | 🟠 MajorForm content lacks scroll mechanism and may overflow.
The
Columnat line 69 usesmainAxisSize: MainAxisSize.min, but it's constrained by a fixed 85% heightSizedBox. When the keyboard appears or with larger forms (multiple fields, multiline inputs), the content will overflow. Wrap the form content (line 92) inExpanded+SingleChildScrollViewwhile keeping the action buttons pinned at the bottom.🛠️ Suggested structure
return Column( - mainAxisSize: MainAxisSize.min, crossAxisAlignment: CrossAxisAlignment.stretch, children: [ Center( child: Container( width: 36, height: 4, decoration: BoxDecoration( color: Colors.white24, borderRadius: BorderRadius.circular(2), ), ), ), const SizedBox(height: 20), Text( widget.title, style: const TextStyle( fontSize: 24, fontWeight: FontWeight.bold, ), ), const SizedBox(height: 28), - widget.builder(context, setState), - const SizedBox(height: 32), + Expanded( + child: SingleChildScrollView( + child: widget.builder(context, setState), + ), + ), + const SizedBox(height: 16), Row(🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/ui/widgets/form_sheet.dart` around lines 69 - 93, The Column used to render the sheet header and body (the Container drag handle, title Text and widget.builder) can overflow because mainAxisSize is min inside the fixed-height sheet; wrap the variable form content returned by widget.builder in an Expanded (or Flexible) containing a SingleChildScrollView (or ListView) so the form becomes scrollable when the keyboard appears or the form is large, and keep the action buttons section outside that scrollable area (i.e., pinned after the Expanded) so buttons remain at the bottom; update the build that composes Column and widget.builder accordingly.
96-104:⚠️ Potential issue | 🟠 MajorCancel button remains active during submission, risking navigation issues.
The Cancel button's
onPressedis not disabled when_submittingis true. If the user taps Cancel whileonSubmitis awaiting, the sheet closes but any subsequentNavigator.pop(context)in the caller'sonSubmitcompletion will pop the underlying page instead of the already-dismissed sheet.🛠️ Suggested fix to disable Cancel during submission
Expanded( child: CupertinoButton( padding: const EdgeInsets.symmetric(vertical: 14), - onPressed: () => Navigator.pop(context), + onPressed: _submitting ? null : () => Navigator.pop(context), child: const Text( 'Cancel', style: TextStyle(color: Colors.white54), ), ), ),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/ui/widgets/form_sheet.dart` around lines 96 - 104, The Cancel button's onPressed must be disabled while _submitting is true to prevent premature Navigator.pop(context) and double-pop from onSubmit; update the CupertinoButton (the Cancel button) so its onPressed is conditional (e.g. onPressed: _submitting ? null : () => Navigator.pop(context)) or otherwise ignores taps when _submitting is true, ensuring the button is visually/behaviorally disabled during the async onSubmit flow.lib/ui/screens/radio_stations.dart (2)
333-335:⚠️ Potential issue | 🟠 MajorEdit flow silently discards changes on error.
Unlike the add flow which shows an error overlay, the edit flow's
catchblock only pops the sheet without any user feedback. Users won't know their update failed and will lose their edits. Show an error message and keep the sheet open.🛠️ Suggested fix
} catch (_) { - Navigator.pop(context); + showOverlay( + context, + caption: 'Error', + message: 'Could not update station.', + icon: CupertinoIcons.exclamationmark_triangle, + ); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/ui/screens/radio_stations.dart` around lines 333 - 335, The edit flow's catch block currently swallows errors and calls Navigator.pop(context) which closes the sheet without feedback; change the catch to capture the exception (e.g., catch (e, st)), remove the Navigator.pop(context) call, and instead show the same user-facing error overlay used by the add flow (replace with the project's error UI helper used elsewhere — e.g., showErrorOverlay or the add-flow's error call) passing the exception message/details so the user sees the failure and the sheet remains open for retry; ensure you update the catch surrounding the edit save/update routine (where Navigator.pop(context) is called) to implement this behavior.
198-215:⚠️ Potential issue | 🟠 MajorAdd flow dismisses sheet on error, discarding user input.
While error feedback is shown via
showOverlay, the sheet is still dismissed viaNavigator.pop(context)at line 199 before the overlay appears. This loses the user's typed station name, URL, and description. Keep the sheet open on failure so users can correct and retry.🛠️ Suggested fix
} catch (e) { - Navigator.pop(context); var message = 'Something went wrong.'; if (e is HttpResponseException) { try { final body = jsonDecode(e.response.body); if (body['message'] != null) { message = body['message']; } } catch (_) {} } showOverlay( context, caption: 'Error', message: message, icon: CupertinoIcons.exclamationmark_triangle, ); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/ui/screens/radio_stations.dart` around lines 198 - 215, The catch block currently calls Navigator.pop(context) which dismisses the input sheet before showing the error overlay and discarding user input; remove or move the Navigator.pop(context) out of this catch block and only call it after a successful save (e.g., where create/update completes), so that on exceptions (caught in this catch handling for HttpResponseException and others) the sheet remains open and showOverlay(...) displays the error to let the user correct inputs; look for the catch surrounding the save logic and the usages of Navigator.pop(context), showOverlay, and HttpResponseException to relocate the dismissal to the success path.
🧹 Nitpick comments (1)
test/ui/screens/create_playlist_folder_test.dart (1)
36-61: Consider adding a test for the disabled submit button when name is empty.The previous test "does not create with empty name" was removed. While
canSubmitnow prevents submission at the sheet level (the button is disabled when input is empty), there's no test verifying this behavior. Consider adding a test that confirms the Create button is disabled when the text field is empty.📝 Example test
testWidgets('submit button is disabled when name is empty', (tester) async { await tester.pumpWidget(buildTestApp()); await tester.tap(find.text('Open')); await tester.pumpAndSettle(); // Find the Create button and verify it's disabled final createButton = find.widgetWithText(CupertinoButton, 'Create'); final button = tester.widget<CupertinoButton>(createButton); expect(button.onPressed, isNull); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/ui/screens/create_playlist_folder_test.dart` around lines 36 - 61, Add a new widget test in create_playlist_folder_test.dart that verifies the Create button is disabled when the folder name is empty: open the sheet the same way the other tests do (tap 'Open' and pumpAndSettle), locate the Create button with find.widgetWithText(CupertinoButton, 'Create') and assert the widget's onPressed is null; reference the test name like 'submit button is disabled when name is empty' and the sheet's form that uses the canSubmit logic so the test covers the disabled-submit behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lib/ui/screens/playlists.dart`:
- Around line 339-344: The onSubmit handler for the rename sheet calls
provider.rename(folder, name: name) with no error handling; wrap that call in a
try-catch inside the onSubmit of _showRenameFolder (the closure passed to
showFormSheet) so that on success you still call Navigator.pop(context), but on
failure you catch the exception, surface a user-facing error (e.g., via
ScaffoldMessenger.of(context).showSnackBar or similar) and do not call
Navigator.pop so the sheet remains open for retry; reference the
controller.text, provider.rename(...) call and Navigator.pop(context) when
updating the handler.
---
Duplicate comments:
In `@lib/ui/screens/add_podcast_sheet.dart`:
- Around line 25-37: The catch block currently calls Navigator.pop(context)
which closes the add-podcast sheet and discards the typed URL; remove that
Navigator.pop(context) call in the error path and only call showOverlay(...) to
display the error so the sheet remains open for the user to correct the URL;
ensure any successful path still invokes Navigator.pop(context) as before and
that the error handling around e, message, and showOverlay stays intact.
In `@lib/ui/screens/create_playlist_sheet.dart`:
- Around line 31-33: The catch block currently swallows errors and calls
Navigator.pop(context), discarding user input; remove the Navigator.pop(context)
from the catch and instead surface the error to the user (e.g., use
ScaffoldMessenger.of(context).showSnackBar(...) or set an error state and call
setState to display an inline error message in the CreatePlaylistSheet), leaving
any TextEditingController values intact so the sheet stays open and the user can
retry; update the catch that wraps the playlist creation logic (the block
containing Navigator.pop(context)) to show the error message and preserve input
rather than dismissing the sheet.
- Around line 55-58: The labelBuilder closure risks throwing StateError when
folders.firstWhere can't find a match; update labelBuilder (the closure using
folders and id) to perform a safe lookup instead of plain firstWhere — e.g., use
firstWhere with an orElse that returns null/placeholder or use indexWhere to
check existence before accessing .name, and return a fallback like 'No folder'
or 'Unknown folder' when no folder with the given id exists.
In `@lib/ui/screens/playlists.dart`:
- Around line 576-579: The labelBuilder closure uses folders.firstWhere(...)
which will throw a StateError if the referenced folder id is missing; change the
lookup in labelBuilder to a safe lookup (e.g., use firstWhere with an orElse
that returns null or a fallback object, or use folders.indexWhere and check for
-1) and return a default string like 'Unknown folder' or 'No folder' when not
found; update the labelBuilder lambda and any related code that assumes a
non-null folder (referencing folders, folder.id, folder.name, and labelBuilder)
to handle the missing-folder case safely.
- Around line 552-554: The catch block currently swallows errors and calls
Navigator.pop(context), which closes the edit sheet and discards the user's
edits; instead remove the Navigator.pop(context) from the catch, keep the sheet
open, and display an error to the user (e.g. use
ScaffoldMessenger.of(context).showSnackBar(...) or showDialog(...) with a clear
message and optionally the error details). Update the catch handler that
surrounds the edit/save logic in playlists.dart so it logs the error and shows
the SnackBar/dialog (referencing the existing catch block that calls
Navigator.pop(context)) rather than dismissing the sheet.
In `@lib/ui/screens/radio_stations.dart`:
- Around line 333-335: The edit flow's catch block currently swallows errors and
calls Navigator.pop(context) which closes the sheet without feedback; change the
catch to capture the exception (e.g., catch (e, st)), remove the
Navigator.pop(context) call, and instead show the same user-facing error overlay
used by the add flow (replace with the project's error UI helper used elsewhere
— e.g., showErrorOverlay or the add-flow's error call) passing the exception
message/details so the user sees the failure and the sheet remains open for
retry; ensure you update the catch surrounding the edit save/update routine
(where Navigator.pop(context) is called) to implement this behavior.
- Around line 198-215: The catch block currently calls Navigator.pop(context)
which dismisses the input sheet before showing the error overlay and discarding
user input; remove or move the Navigator.pop(context) out of this catch block
and only call it after a successful save (e.g., where create/update completes),
so that on exceptions (caught in this catch handling for HttpResponseException
and others) the sheet remains open and showOverlay(...) displays the error to
let the user correct inputs; look for the catch surrounding the save logic and
the usages of Navigator.pop(context), showOverlay, and HttpResponseException to
relocate the dismissal to the success path.
In `@lib/ui/widgets/form_sheet.dart`:
- Around line 69-93: The Column used to render the sheet header and body (the
Container drag handle, title Text and widget.builder) can overflow because
mainAxisSize is min inside the fixed-height sheet; wrap the variable form
content returned by widget.builder in an Expanded (or Flexible) containing a
SingleChildScrollView (or ListView) so the form becomes scrollable when the
keyboard appears or the form is large, and keep the action buttons section
outside that scrollable area (i.e., pinned after the Expanded) so buttons remain
at the bottom; update the build that composes Column and widget.builder
accordingly.
- Around line 96-104: The Cancel button's onPressed must be disabled while
_submitting is true to prevent premature Navigator.pop(context) and double-pop
from onSubmit; update the CupertinoButton (the Cancel button) so its onPressed
is conditional (e.g. onPressed: _submitting ? null : () =>
Navigator.pop(context)) or otherwise ignores taps when _submitting is true,
ensuring the button is visually/behaviorally disabled during the async onSubmit
flow.
---
Nitpick comments:
In `@test/ui/screens/create_playlist_folder_test.dart`:
- Around line 36-61: Add a new widget test in create_playlist_folder_test.dart
that verifies the Create button is disabled when the folder name is empty: open
the sheet the same way the other tests do (tap 'Open' and pumpAndSettle), locate
the Create button with find.widgetWithText(CupertinoButton, 'Create') and assert
the widget's onPressed is null; reference the test name like 'submit button is
disabled when name is empty' and the sheet's form that uses the canSubmit logic
so the test covers the disabled-submit behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1b4c6ed5-874d-45ad-8f7f-d6b517206b62
⛔ Files ignored due to path filters (1)
pubspec.lockis excluded by!**/*.lock
📒 Files selected for processing (10)
lib/router.dartlib/ui/screens/add_podcast_sheet.dartlib/ui/screens/create_playlist_folder_sheet.dartlib/ui/screens/create_playlist_sheet.dartlib/ui/screens/playlists.dartlib/ui/screens/radio_stations.dartlib/ui/widgets/form_sheet.dartlib/ui/widgets/widgets.darttest/ui/screens/create_playlist_folder_test.darttest/ui/screens/create_playlist_sheet_test.dart
✅ Files skipped from review due to trivial changes (1)
- lib/ui/widgets/widgets.dart
🚧 Files skipped from review as they are similar to previous changes (2)
- test/ui/screens/create_playlist_sheet_test.dart
- lib/ui/screens/create_playlist_folder_sheet.dart
Replace CupertinoAlertDialog with a reusable showFormSheet bottom sheet for all create/edit forms: playlist, playlist folder, radio station, and podcast. Consistent style with text fields, combo box, and action buttons across all forms.
phanan
force-pushed
the
feat/form-sheets
branch
from
e9624a2 to
6206dd4
Compare
This was referenced Mar 28, 2026
6 tasks
This was referenced May 1, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
CupertinoAlertDialogwith a reusableshowFormSheetbottom sheet (85% height) for all create/edit formsFormTextFieldwidget: hint when unfocused, floating white label on focus, highlight focus border, white textFormDropdownwidget (inline combo box) replacing the folder selection action sheetTest plan
Summary by CodeRabbit
New Features
Refactor
Tests