fix(grid): stretch cells with a min height to drop the 2px overflow (#141)#142
Conversation
…141) Follow-up to #139. WindEqualHeightRow re-laid each cell to a TIGHT height equal to the loose-measured row max. A cell whose flex flex-col content needs a hair more under the tight re-lay than the loose measure reported (sub-pixel text/flex rounding on real rendering, e.g. CanvasKit) then overflowed by ~2px ("RenderFlex overflowed by 2.0 pixels on the bottom") on the stretched row. Stretch with a MIN height instead of a tight one: a cell is laid out with minHeight = the row target and an unbounded (or the incoming) max, so it is never forced below its own content, and the row takes the tallest resulting height. Overflow is impossible by construction; the common (integer-metric) case still yields perfectly equal cells. Note: the 2px overflow is specific to fractional glyph metrics (CanvasKit) and is not reproducible in the flutter_test binding, so the guard test asserts no exception + equal heights; the fix is by construction (min-height never squeezes content). Post-change sync: grid_stretch_test #141 no-overflow case; doc/layout/grid.md, skill (tokens.md, version 2.8.2), CHANGELOG. Closes #141
|
Warning Review limit reached
Next review available in: 50 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughModifies WindEqualHeightRow's second layout pass to relay stretched cells with a minimum height constraint instead of a tight height constraint, preventing residual sub-pixel RenderFlex overflow. Adds a regression test, updates related documentation comments, tokens reference, changelog, and skill version metadata. ChangesEqual-Height Row Min-Height Fix
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related issues
Sequence Diagram(s)sequenceDiagram
participant Grid as WDiv Grid
participant Row as WindEqualHeightRow
participant Cell as Cell Widget
Grid->>Row: performLayout (Pass 1: loose measure)
Row->>Cell: layout with loose height
Cell-->>Row: measured height
Row->>Row: compute target = clamp(max measured height)
Row->>Cell: layout with minHeight=target (Pass 2)
Cell-->>Row: final height (>= target)
Row->>Row: rowHeight = max(target, child heights)
Row-->>Grid: positioned cells, no overflow
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
lib/src/widgets/wind_equal_height_row.dart (1)
122-139: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winEarlier-processed cells aren't re-stretched to the final
rowHeight.If a later cell in the loop needs more than
target(the sub-pixel case#141targets),rowHeightis raised, but cells already laid out earlier in the same pass keep their smallertarget-height layout. The row container ends uprowHeighttall, yet the earlier cell's own render box stays attarget, leaving a visible gap under it — reintroducing the "ragged row" problem (#126) for exactly the scenario this fix addresses, just without an overflow exception. The code comment above ("a grown cell simply sets a marginally taller row") acknowledges this but doesn't correct it.Consider a small normalization step: after the main loop, if
rowHeight > target, re-layout any child whosesize.height < rowHeightwithminHeight: rowHeightso every cell in the row actually matches the final height.♻️ Proposed fix: normalize cells to the final rowHeight
double rowHeight = target; double dx = 0; + final List<RenderBox> laidOutChildren = []; child = firstChild; while (child != null) { child.layout( BoxConstraints( minWidth: cellWidth, maxWidth: cellWidth, minHeight: target, maxHeight: constraints.maxHeight, ), parentUsesSize: true, ); if (child.size.height > rowHeight) rowHeight = child.size.height; + laidOutChildren.add(child); (child.parentData as _EqualHeightParentData).offset = Offset(dx, 0); dx += cellWidth + _spacing; child = childAfter(child); } + + // Rare sub-pixel case: a later cell needed more than `target`. Re-lay any + // cell still short of the final rowHeight so the row stays visually equal. + if (rowHeight > target) { + for (final c in laidOutChildren) { + if (c.size.height < rowHeight) { + c.layout( + BoxConstraints( + minWidth: cellWidth, + maxWidth: cellWidth, + minHeight: rowHeight, + maxHeight: constraints.maxHeight, + ), + parentUsesSize: true, + ); + } + } + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/src/widgets/wind_equal_height_row.dart` around lines 122 - 139, The row layout in `EqualHeightRow` leaves earlier children at the initial `target` height when a later child increases `rowHeight`, so the cells in the same row can end up visually uneven. Update the layout logic in the loop that walks `firstChild`/`childAfter` to perform a second normalization pass after `rowHeight` is finalized: if `rowHeight` exceeds `target`, re-layout any child whose height is still smaller using the final `rowHeight` so every child in the row matches the container height.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@lib/src/widgets/wind_equal_height_row.dart`:
- Around line 122-139: The row layout in `EqualHeightRow` leaves earlier
children at the initial `target` height when a later child increases
`rowHeight`, so the cells in the same row can end up visually uneven. Update the
layout logic in the loop that walks `firstChild`/`childAfter` to perform a
second normalization pass after `rowHeight` is finalized: if `rowHeight` exceeds
`target`, re-layout any child whose height is still smaller using the final
`rowHeight` so every child in the row matches the container height.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 0d70d9eb-6b2a-4303-9fa1-80095977fa18
📒 Files selected for processing (7)
CHANGELOG.mddoc/layout/grid.mdlib/src/widgets/w_div.dartlib/src/widgets/wind_equal_height_row.dartskills/wind-ui/SKILL.mdskills/wind-ui/references/tokens.mdtest/widgets/w_div/grid_stretch_test.dart
There was a problem hiding this comment.
Pull request overview
This PR fixes a residual RenderFlex overflowed by ~2px warning in grid ... items-stretch by changing WindEqualHeightRow to stretch children using a minimum height constraint rather than a tight height, preventing content from being forced below its own layout height (the #141 follow-up to #139).
Changes:
- Update
WindEqualHeightRowto re-layout cells withminHeight = target(not a tight height) and let the row grow to the tallest resulting child. - Add a regression test covering the #141 repro shape (asserts no exception + equal heights in the test environment).
- Sync documentation surfaces (doc page, skill reference, skill version bump) and add a CHANGELOG entry describing the fix.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| lib/src/widgets/wind_equal_height_row.dart | Changes equal-height row stretching logic to use min-height constraints to avoid residual overflows. |
| lib/src/widgets/w_div.dart | Updates Dartdoc describing how items-stretch grid rows are implemented. |
| test/widgets/w_div/grid_stretch_test.dart | Adds a widget test for the #141 regression shape (no exception + equal heights). |
| doc/layout/grid.md | Updates user docs to describe the min-height (non-tight) stretch behavior and overflow avoidance. |
| CHANGELOG.md | Adds a detailed Fixed entry for the #141 overflow warning regression. |
| skills/wind-ui/references/tokens.md | Updates token reference text to reflect the new min-height stretch semantics. |
| skills/wind-ui/SKILL.md | Bumps the skill version to 2.8.2 and updates the header comment accordingly. |
| double rowHeight = target; | ||
| double dx = 0; | ||
| child = firstChild; | ||
| while (child != null) { | ||
| child.layout( |
| // WindEqualHeightRow measures each cell with a real (loose-height) layout | ||
| // and re-lays it tight to the row max, instead of IntrinsicHeight's | ||
| // and re-lays it to at least the row max (min height), instead of the | ||
| // intrinsic query IntrinsicHeight would run, | ||
| // intrinsic query. A `flex flex-col` cell (which carries a LayoutBuilder) |
- Restore equal heights: after the min-height measure pass settles the true row height, a third pass re-lays every cell to that height (still via a MIN constraint, never tight), so a cell that measured shorter than a sibling which grew is brought up to match. All cells share the row height, and none is ever squeezed below its content (overflow-free). - Fix the duplicated 'intrinsic query ... intrinsic query.' fragment in the _buildStretchGrid dartdoc.
|
Both addressed:
@coderabbitai review |
🧩 Analysis chain🏁 Script executed: #!/bin/bash
fd wind_equal_height_row.dart
echo "---"
cat -n lib/src/widgets/wind_equal_height_row.dart | sed -n '1,200p'
echo "---div---"
rg -n "_buildStretchGrid" -A 30 lib/src/widgets/w_div.dart | head -80Length of output: 10670
This looks solid. Triggering a fresh review now. ✅ Action performedReview finished.
|
Summary
Fixes #141, a follow-up to #139.
grid ... items-stretchno longer crashes (thanks to #139'sWindEqualHeightRow), but it emitted a residualRenderFlex overflowed by 2.0 pixels on the bottomon the row that actually gets stretched.Cause
WindEqualHeightRowre-laid each cell to a tight height equal to the loose-measured row max. A cell whoseflex flex-colcontent needs a hair more under the tight re-lay than the loose pass reported (sub-pixel text/flex rounding on real rendering, e.g. CanvasKit) then overflowed by ~2px.Fix
Stretch with a min height instead of a tight one: each cell is laid out with
minHeight = row targetand an unbounded (or the incoming) max, so it is never forced below its own content, and the row takes the tallest resulting height. Overflow is impossible by construction; the common integer-metric case still yields perfectly equal cells. (This also folds the previous separate measure/relay passes into one positioning pass.)Tests / reproducibility
The 2px overflow is specific to fractional glyph metrics (CanvasKit) and is not reproducible in the
flutter_testbinding (integer test-font metrics), so I could not write a red→green test for the exact pixel. The added guard (grid_stretch_test.dart#141 case) asserts no exception + equal heights on the repro shape, and the fix is correct by construction (a min-height stretch cannot squeeze content below its own height, soRenderFlexcannot overflow).Definition of done
dart analyze: clean.dart format: no diff.flutter test: all pass, coverage 94.4% ≥ 90% (wind_equal_height_row.dartfully covered; no conditional re-lay branch left uncovered).doc/layout/grid.md, skill (tokens.md,SKILL.mdversion 2.8.2),CHANGELOG.md.WindEqualHeightRowstays internal (README unchanged).Summary by CodeRabbit
Bug Fixes
Documentation
Tests