Fix form sheet buttons pushed off screen#155
Conversation
Use ConstrainedBox with maxHeight instead of fixed SizedBox height so the sheet sizes to content. Use Flexible instead of Expanded for the scroll area so it only takes what it needs, keeping buttons visible.
📝 WalkthroughWalkthroughThe Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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 |
Verify buttons are within the visible viewport for both short forms (few fields) and tall forms (many fields), and that the submit button is tappable.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
test/ui/widgets/form_sheet_test.dart (1)
11-134: Add a keyboard-inset regression test.The suite is strong, but it currently misses the keyboard-open case from the PR objective. Please add a test that simulates non-zero bottom
viewInsetsand re-checks that Cancel/Submit remain accessible.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/ui/widgets/form_sheet_test.dart` around lines 11 - 134, Add a new testWidgets named like 'buttons are visible with keyboard open' that uses buildTestApp and showFormSheet and simulates a non-zero bottom keyboard inset by setting the test binding window viewInsetsTestValue (e.g. TestWidgetsFlutterBinding.instance.window.viewInsetsTestValue or tester.binding.window.viewInsetsTestValue) to a bottom inset before pumping the widget, then pumpAndSettle, assert Cancel and Submit (or the submitLabel used) are present and their rect.bottom is within the visible screen bounds, and finally reset the viewInsetsTestValue to EdgeInsets.zero (or clear the test value) after the test so other tests are unaffected; keep references to showFormSheet, buildTestApp, and the binding/window viewInsets test value in the test.
🤖 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/widgets/form_sheet.dart`:
- Around line 21-24: The Column inside the FormSheet's build (inside the
ConstrainedBox that sets maxHeight using MediaQuery) is using the default
mainAxisSize which forces it to expand; update that Column widget to include
mainAxisSize: MainAxisSize.min so it will shrink-to-fit its children within the
ConstrainedBox instead of always filling the max height.
---
Nitpick comments:
In `@test/ui/widgets/form_sheet_test.dart`:
- Around line 11-134: Add a new testWidgets named like 'buttons are visible with
keyboard open' that uses buildTestApp and showFormSheet and simulates a non-zero
bottom keyboard inset by setting the test binding window viewInsetsTestValue
(e.g. TestWidgetsFlutterBinding.instance.window.viewInsetsTestValue or
tester.binding.window.viewInsetsTestValue) to a bottom inset before pumping the
widget, then pumpAndSettle, assert Cancel and Submit (or the submitLabel used)
are present and their rect.bottom is within the visible screen bounds, and
finally reset the viewInsetsTestValue to EdgeInsets.zero (or clear the test
value) after the test so other tests are unaffected; keep references to
showFormSheet, buildTestApp, and the binding/window viewInsets test value in the
test.
🪄 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: a91756ec-2601-4073-b832-664fb1c19ac0
📒 Files selected for processing (2)
lib/ui/widgets/form_sheet.darttest/ui/widgets/form_sheet_test.dart
| return ConstrainedBox( | ||
| constraints: BoxConstraints( | ||
| maxHeight: MediaQuery.of(context).size.height * 0.85, | ||
| ), |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's find and examine the form_sheet.dart file
fd form_sheet.dartRepository: koel/player
Length of output: 85
🏁 Script executed:
# Read the form_sheet.dart file to examine the code structure
cat -n lib/ui/widgets/form_sheet.dartRepository: koel/player
Length of output: 11405
Add mainAxisSize: MainAxisSize.min to the Column to enable shrink-to-content behavior.
The ConstrainedBox sets a maximum height constraint, but the Column at line 71 uses the default mainAxisSize: MainAxisSize.max, which causes it to expand to the full 85% height even for short forms. Without mainAxisSize: MainAxisSize.min, the sheet will remain unnecessarily tall.
Suggested fix
return Column(
+ mainAxisSize: MainAxisSize.min,
crossAxisAlignment: CrossAxisAlignment.stretch,
children: [🤖 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 21 - 24, The Column inside the
FormSheet's build (inside the ConstrainedBox that sets maxHeight using
MediaQuery) is using the default mainAxisSize which forces it to expand; update
that Column widget to include mainAxisSize: MainAxisSize.min so it will
shrink-to-fit its children within the ConstrainedBox instead of always filling
the max height.
1 participant
Summary
SizedBox(height: 85%)forced the sheet to always be tall, andExpandedon the scroll area consumed all remaining space, pushing Cancel/Create buttons below the visible areaConstrainedBox(maxHeight: 85%)so the sheet sizes to its contentExpandedtoFlexibleso the scroll area only takes what it needsAffects all form sheets: New Playlist, Create Folder, Add/Edit Radio Station.
Test plan
flutter testSummary by CodeRabbit
New Features
Tests