Make PlayableActionSheet actions list scroll on small screens#178
Make PlayableActionSheet actions list scroll on small screens#178phanan wants to merge 1 commit into
Conversation
Wrap the inner ListView in Flexible and drop NeverScrollableScrollPhysics so the action list scrolls within the available height when content exceeds it, instead of overflowing the Column. The sheet's spaceBetween visual is preserved when content fits. Adds a regression test that mounts the sheet at iPhone SE size (320x568) and asserts no exception is raised.
📝 WalkthroughWalkthroughThe Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
test/ui/screens/playable_action_sheet_test.dart (1)
116-127: Optional: strengthen the small-screen assertion.
tester.takeException()only catches errors that have already been reported (e.g.RenderFlex overflowed) at pump time. It will pass even if a regression silently clips actions off-screen without throwing. Consider also asserting that an action item near the end of the list is present (and optionally reachable via scroll) so the test fails on hidden/clipped content as well.♻️ Suggested stronger assertion
await _mountSheet( tester, downloaded: true, surfaceSize: const Size(320, 568), ); expect(tester.takeException(), isNull); + // The last action should still be present (and scrollable into view) on small screens. + final addToPlaylist = find.text('Add to a Playlist…'); + expect(addToPlaylist, findsOneWidget); + await tester.ensureVisible(addToPlaylist); },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/ui/screens/playable_action_sheet_test.dart` around lines 116 - 127, The test currently only calls tester.takeException() which misses silent clipping; update the 'lays out without overflow on small screens (iPhone SE)' test (the testWidgets block that calls _mountSheet) to also assert that a known action item near the end of the list is present and visible: use a stable identifier from the sheet (e.g., the final action's text or Key used by the action widget) with expect(find.text('<LAST_ACTION_LABEL>') or find.byKey(ValueKey('<LAST_ACTION_KEY>')), findsOneWidget) and if it might be offscreen use tester.scrollUntilVisible or tester.ensureVisible on that finder before asserting to fail the test if the item is clipped or unreachable.lib/ui/screens/playable_action_sheet.dart (1)
94-273: LGTM —Flexible-wrapped scrollingListViewresolves the overflow correctly.With the modal opened via
isScrollControlled: true, the outerColumnreceives a bounded max height, soFlexible(loose fit) +shrinkWrap: truedoes the right thing in both cases:
- Content fits:
ListViewshrink-wraps to its intrinsic height, andMainAxisAlignment.spaceBetweenstill pins the actions to the bottom (behavior preserved).- Content overflows (small screens):
ListViewis capped at the remaining height and scrolls with default physics.Nit/optional: on small screens there's no affordance hinting that the actions are scrollable. Wrapping the
ListViewin aScrollbar(and giving theListViewaScrollController) would make overflow discoverable to users on iPhone SE-class viewports.♻️ Optional: add a Scrollbar for discoverability
- Flexible( - child: ListView( - shrinkWrap: true, - children: <Widget>[ + Flexible( + child: Scrollbar( + child: ListView( + shrinkWrap: true, + children: <Widget>[ ... - ], - ), - ), + ], + ), + ), + ),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/ui/screens/playable_action_sheet.dart` around lines 94 - 273, The ListView inside Flexible correctly fixes the overflow; to add the optional affordance, create a ScrollController (e.g., _actionsScrollController) and wrap the ListView in a Scrollbar that uses that controller; update the ListView to pass controller: _actionsScrollController (keeping shrinkWrap: true) and dispose the controller in the widget's State disposal; reference PlayableActionButton, Flexible, ListView, and shrinkWrap to locate where to wrap and wire the controller.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@lib/ui/screens/playable_action_sheet.dart`:
- Around line 94-273: The ListView inside Flexible correctly fixes the overflow;
to add the optional affordance, create a ScrollController (e.g.,
_actionsScrollController) and wrap the ListView in a Scrollbar that uses that
controller; update the ListView to pass controller: _actionsScrollController
(keeping shrinkWrap: true) and dispose the controller in the widget's State
disposal; reference PlayableActionButton, Flexible, ListView, and shrinkWrap to
locate where to wrap and wire the controller.
In `@test/ui/screens/playable_action_sheet_test.dart`:
- Around line 116-127: The test currently only calls tester.takeException()
which misses silent clipping; update the 'lays out without overflow on small
screens (iPhone SE)' test (the testWidgets block that calls _mountSheet) to also
assert that a known action item near the end of the list is present and visible:
use a stable identifier from the sheet (e.g., the final action's text or Key
used by the action widget) with expect(find.text('<LAST_ACTION_LABEL>') or
find.byKey(ValueKey('<LAST_ACTION_KEY>')), findsOneWidget) and if it might be
offscreen use tester.scrollUntilVisible or tester.ensureVisible on that finder
before asserting to fail the test if the item is clipped or unreachable.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 20058c1e-c73a-406d-9db9-6738e69c6ca9
📒 Files selected for processing (2)
lib/ui/screens/playable_action_sheet.darttest/ui/screens/playable_action_sheet_test.dart
|
Closing — the Flexible+scrollable wrap fixes the overflow but breaks drag-to-dismiss on the action list (the inner scrollable captures vertical drags even when content fits). PR #177's test surface bump remains the workaround for CI; the production sheet stays as it was. A proper fix would require DraggableScrollableSheet, which is a larger refactor of how the modal is shown. |
1 participant
Summary
The previous PR (#176) added a Download/Remove Download item to the song action sheet. On smaller screens (iPhone SE / X) the new item pushes total content past the available height, and because the inner `ListView` had `NeverScrollableScrollPhysics()` and `shrinkWrap: true`, the parent `Column` overflowed instead of letting the list scroll.
This wraps the `ListView` in `Flexible` and drops the never-scrollable physics. When content fits, the sheet looks identical (spaceBetween still pins the action list to the bottom). When content overflows, the action list scrolls within its allotment instead of throwing a render error.
Test plan
Summary by CodeRabbit
Bug Fixes
Tests