fix(ui): stop StreamMessageListView from tripping RenderViewport layout-cycle limit#2703
Merged
Merged
Conversation
Contributor
|
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:
📝 WalkthroughWalkthroughExpands padding API to accept EdgeInsetsGeometry (resolved via Directionality), implements axis-aware padding and content-edge scroll adjustments, preserves visual anchor alignment without resetting scroll pixels, refactors item-position calculations, and relaxes viewport layout-cycle convergence checks. ChangesScrollablePositionedList API and Behavior Update
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
🚥 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 |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #2703 +/- ##
==========================================
+ Coverage 65.66% 65.67% +0.01%
==========================================
Files 423 423
Lines 26666 26694 +28
==========================================
+ Hits 17509 17532 +23
- Misses 9157 9162 +5 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/stream_chat_flutter/lib/scrollable_positioned_list/src/scrollable_positioned_list.dart (1)
222-231:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
isScrollingListenableis not actually safe to subscribe to before attach.Returning a noop object here drops any listener added before
_attach, so the documented “register at any time” contract is false. This needs a stable controller-owned notifier/proxy instead of swapping the listenable instance on attachment.🤖 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 `@packages/stream_chat_flutter/lib/scrollable_positioned_list/src/scrollable_positioned_list.dart` around lines 222 - 231, The getter isScrollingListenable currently returns a _NoopListenable until _attach, which drops listeners added early; fix by creating a persistent controller-owned proxy/listenable (e.g. a private ValueNotifier<bool> or proxy Listenable field) that the getter always returns instead of swapping instances, then when _attach is called subscribe the attached state's _isScrollingListenable (or _isScrolling notifier) and forward its value/notifications into that proxy; ensure you wire up detach/dispose to remove subscriptions and keep symbol references: isScrollingListenable getter, _scrollableListState, _isScrollingListenable, _NoopListenable (remove use), and the new controller-owned proxy field so listeners registered before attach are preserved.
🤖 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.
Inline comments:
In
`@packages/stream_chat_flutter/lib/scrollable_positioned_list/src/positioned_list.dart`:
- Around line 51-54: The constructor assertion in PositionedList currently
allows negative positionedIndex values; update the assertion in PositionedList
(the constructor in positioned_list.dart) to reject negatives by requiring
positionedIndex >= 0 and positionedIndex < itemCount (i.e., tighten the lower
bound so invalid negative indices fail at the API boundary rather than during
layout).
In
`@packages/stream_chat_flutter/lib/scrollable_positioned_list/src/scrollable_positioned_list.dart`:
- Around line 680-684: The helper _resolveLeadingEndPaddingAdjust currently
computes isAtLeadingEnd as "widget.reverse ? index == 0 : index ==
widget.itemCount - 1", which is inverted; for non-reversed lists the leading end
is index == 0 and for reversed lists it is index == widget.itemCount - 1. Update
the condition in _resolveLeadingEndPaddingAdjust (the isAtLeadingEnd
calculation) to the correct form (e.g. widget.reverse ? index ==
widget.itemCount - 1 : index == 0) so the leading-padding correction applies to
the actual leading item.
---
Outside diff comments:
In
`@packages/stream_chat_flutter/lib/scrollable_positioned_list/src/scrollable_positioned_list.dart`:
- Around line 222-231: The getter isScrollingListenable currently returns a
_NoopListenable until _attach, which drops listeners added early; fix by
creating a persistent controller-owned proxy/listenable (e.g. a private
ValueNotifier<bool> or proxy Listenable field) that the getter always returns
instead of swapping instances, then when _attach is called subscribe the
attached state's _isScrollingListenable (or _isScrolling notifier) and forward
its value/notifications into that proxy; ensure you wire up detach/dispose to
remove subscriptions and keep symbol references: isScrollingListenable getter,
_scrollableListState, _isScrollingListenable, _NoopListenable (remove use), and
the new controller-owned proxy field so listeners registered before attach are
preserved.
🪄 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: 58985ee7-2ff9-4d5e-a5b3-3a80d518934c
📒 Files selected for processing (4)
packages/stream_chat_flutter/CHANGELOG.mdpackages/stream_chat_flutter/lib/scrollable_positioned_list/src/positioned_list.dartpackages/stream_chat_flutter/lib/scrollable_positioned_list/src/scrollable_positioned_list.dartpackages/stream_chat_flutter/lib/scrollable_positioned_list/src/viewport.dart
xsahil03x
force-pushed
the
fix/spl-render-viewport-layout-cycles
branch
3 times, most recently
from
7d34ab2 to
8a2f56d
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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.
Inline comments:
In
`@packages/stream_chat_flutter/lib/scrollable_positioned_list/src/positioned_list.dart`:
- Around line 504-529: The center sliver padding logic drops one main-axis edge
when widget.itemCount == 0 because isLast becomes false; update
_centerSliverPadding to preserve both edges for empty lists by checking
widget.itemCount == 0 (or equivalently treating isFirst and isLast as true) and
returning the full _resolvedPadding in that case before the AxisDirection
switch; modify the function around _centerSliverPadding/_resolvedPadding and
usages of widget.positionedIndex and widget.itemCount to handle the empty-list
early return.
- Around line 592-606: The leading/trailing edge math for horizontal items
currently branches on widget.reverse causing RTL to be flipped; update the logic
that builds ItemPosition (itemLeadingEdge/itemTrailingEdge) to branch on
_axisDirection instead, using AxisDirection.right to compute leading =
itemOffset and trailing = itemOffset + box.size.width, and AxisDirection.left to
compute leading = viewportDimension - (itemOffset + box.size.width) and trailing
= viewportDimension - itemOffset (still divide by
scrollController.position.viewportDimension and round as before); change the
conditional checks around itemOffset/box.size.width in the ItemPosition
construction to reference _axisDirection (AxisDirection.left/right) rather than
widget.reverse so RTL is correct.
🪄 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: bf3e7d48-67b0-495c-9fcc-1eefeb430630
📒 Files selected for processing (6)
.fvmrc.gitignorepackages/stream_chat_flutter/CHANGELOG.mdpackages/stream_chat_flutter/lib/scrollable_positioned_list/src/positioned_list.dartpackages/stream_chat_flutter/lib/scrollable_positioned_list/src/scrollable_positioned_list.dartpackages/stream_chat_flutter/lib/scrollable_positioned_list/src/viewport.dart
✅ Files skipped from review due to trivial changes (3)
- .fvmrc
- .gitignore
- packages/stream_chat_flutter/CHANGELOG.md
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/stream_chat_flutter/lib/scrollable_positioned_list/src/scrollable_positioned_list.dart
xsahil03x
force-pushed
the
fix/spl-render-viewport-layout-cycles
branch
2 times, most recently
from
8d883ce to
219f2c3
Compare
xsahil03x
force-pushed
the
fix/spl-render-viewport-layout-cycles
branch
3 times, most recently
from
8ba11f0 to
76b2f66
Compare
…ut-cycle limit ScrollablePositionedList's reanchor used to reset pixels to 0, which forced the underlying slivers to walk back through their children one layout cycle at a time on mid-list anchored layouts — exhausting RenderViewport's 10-cycle budget. The reanchor now folds the existing pixels into the new viewport anchor so no walk-back is needed. UnboundedRenderViewport also gains a precisionErrorTolerance guard on sliver corrections and a higher cycle ceiling as belt-and-suspenders. Also pulls the v10 padding fix forward: ScrollablePositionedList.padding now accepts EdgeInsetsGeometry, and scrollTo adjusts for leading padding so the target lands at the content-area edge. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Adds .fvmrc with the same Flutter floor the workspace declares in melos.yaml (>=3.27.4) so contributors running fvm pick up the right SDK automatically. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
xsahil03x
force-pushed
the
fix/spl-render-viewport-layout-cycles
branch
from
76b2f66 to
448ad4b
Compare
xsahil03x
added a commit
that referenced
this pull request
Jun 1, 2026
Resolves SPL conflicts in scrollable_positioned_list and positioned_list: - Take master's preserve-pixels `_updateFirstVisibleItemIfNeeded` (PR #2703) - Take master's empty-list `_centerSliverPadding` guard - Take master's RTL `_axisDirection`-based `_updatePositions` branching - Take master's `_resolveLeadingEndPaddingAdjust` `index != 0` check - Drop orphaned `message_card_test.dart` (widget deleted in v10 design refresh); master's MessageCard fix from #2702 ported to StreamMessageContent. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
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
2 participants
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
ScrollablePositionedListreanchor (_updateFirstVisibleItemIfNeeded) no longer resetspixelsto 0 after a list mutation; it folds the existing scroll offset into the new viewport anchor so the underlying slivers don't have to walk back through their children one layout cycle at a time.UnboundedRenderViewportignores sub-precisionErrorTolerancesliver corrections (which can't actually movepixels) and raises its per-frame cycle ceiling from 10 to 100 as a safety net.ScrollablePositionedList.paddingnow acceptsEdgeInsetsGeometry(resolved againstDirectionality), andscrollTolands the target at the content-area edge by adjusting for leading padding. Also fixes a few edge cases in that forward-port flagged in review (empty-list padding symmetry, RTLItemPositionmath,scrollTopadding-adjust for non-reversed lists)..fvmrcpinning the workspace to the melos-declared Flutter floor so contributors running fvm pick up the right SDK automatically.Background
StreamMessageListViewwas hittingA RenderViewport exceeded its maximum number of layout cyclesunder mid-list anchored layout — repro'd in production. Each time_updateFirstVisibleItemIfNeededresetpixelsto 0 after an item-set change, the underlying slivers had to walk back through their kept-alive children one correction per frame; with a high enough startingfirstChildindex, the viewport's 10-cycle budget was exhausted.The math for the fix: solve
newAnchor * viewport - pixels == currentAlignment * viewportso the anchored item lands at the same visual position without touchingpixels. No walk-back, no cycle budget pressure.Test plan
flutter test packages/stream_chat_flutter/test/scrollable_positioned_list/— 194/194 pass (2 pre-existing skips)melos run analyze— no new issues from these changesitemKeyBuilderset + a stream of mutations doesn't hit the assertscrollTowith padding lands the target at the content-area edge (not the viewport edge)🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Improvements
Chores