Merged
Conversation
…wrapper The drag-handle Stack used `StackFit.expand` + an inner `Align`, which gave the body tight max-height constraints and forced every sheet to fill the screen — even when the body opted into shrink-wrapping via `MainAxisSize.min`. Switch to default `StackFit.loose` and position the handle via the Stack's `alignment` instead of a child `Align`. The body now sizes naturally: shrink-wraps when it wants to (MainAxisSize.min), and still fills the screen when it uses `Expanded` / `MainAxisSize.max` from inside. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Reading `context.size` from `_handleDragEnd` after a setState elsewhere in the tree marked the render object dirty mid-gesture throws: > Cannot get size from a render object that has been marked dirty for > layout. The drag-end handler ran via `goBallistic` from the scrollable's position; in between the last drag-update layout and end-of-gesture, a state-driven rebuild (an upload-progress setState in our case) marked the parent stack dirty, so the size getter blew up. Cache the sheet height in both gesture-detector and scrollable-driven paths during `_handleDragUpdate` (where context is reliably laid out), and reuse the cached value in `_handleDragEnd`. Falls through to the existing safe defaults (`0` velocity / `1` height) when no update has fired. All existing showStreamSheet tests still pass. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Replaces the `context.size` reads in both drag-handler classes with a `_StreamSheetExtent` snapshotted by a `LayoutBuilder` in `StreamSheetRoute.buildPage`. Mirrors Flutter's `DraggableScrollableSheet` extent pattern, trimmed to just the height field we use: - `_StreamSheetExtent` is a plain value holder for `availableHeight`. - `StreamSheetRoute` owns one `_extent`. `buildPage` wraps the body in a `LayoutBuilder` that writes `_extent.availableHeight = constraints.maxHeight` on each layout pass — parent constraints are stable through layout, immune to the dirty-render-object propagation that happens when a body `setState` marks a `Stack(StackFit.loose)` parent dirty mid-gesture. - Both `_StreamDragGestureDetector` and `_StreamDraggableScrollableSheet` now take the extent as a constructor field. `_handleDragUpdate` and `_handleDragEnd` read `widget.extent.availableHeight` instead of `context.size`. The cached-height workaround in 409e33d is gone — we no longer need it because we never read the rendered size from the drag handlers. Existing showStreamSheet (11) and StreamSheetHeader (16) tests pass. Long-term, this opens the door to features the size-reading model couldn't easily support — multiple snap points, programmatic `animateTo(0.5)`, listenable size, etc. — by extending `_StreamSheetExtent` rather than retrofitting layout reads. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Three drifts from the design system landed at once:
- StreamAppBar background was unset, so Material's AppBar fell through
to ColorScheme.surface and picked up scrolled-under tint. Lock to
streamColorScheme.backgroundElevation1 (transparent surface tint)
so the bar stays the design's flat white. Per-call backgroundColor
still wins.
- StreamAppBar's auto-implied back leading used Material's BackButton,
which alternates between Icons.arrow_back (Android / web) and
Icons.arrow_back_ios_new (iOS / macOS). Now mirror Material's logic
in design-system icons:
fullscreenDialog → streamIcons.xmark
iOS / macOS → streamIcons.chevronLeft
other platforms → streamIcons.arrowLeft
automaticallyImplyLeading is set to false on the inner AppBar so it
doesn't insert a duplicate.
- StreamSheetHeader's "regular pushed page" branch hardcoded chevron;
align it with the AppBar's platform switch (sheet branches stay on
the chevron / cross convention because sheets are iOS-modal-style
on every platform).
Test changes:
- Renamed the sheet header test to "inserts arrow-left on a regular
pushed route on Android" and added a sibling iOS variant. Both use
debugDefaultTargetPlatformOverride wrapped in try/finally so the
framework's debug-vars-unset invariant still holds at end-of-test.
No public API change. All 16 sheet header tests + 11 sheet tests pass.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Replaces the Material AppBar wrapper with a slots-based StreamAppBar (leading / title / subtitle / trailing) that follows the same factory + theme + style pattern as the rest of the design system. Auto-implied leading is platform-aware: cross on fullscreen dialogs, chevron on iOS / macOS, arrow-left elsewhere. Extracts StreamHeaderToolbar — a 3-slot layout primitive shared by StreamAppBar and StreamSheetHeader that keeps the title geometrically centred even when leading and trailing widths differ. Replaces the StreamVisibility-based 48×48 spacer hack in StreamSheetHeader. StreamSheetHeader changes: - Now implements PreferredSizeWidget. - Uses StreamHeaderToolbar for layout. - Sheet branches (stacked sheet, deeper nested route) now use the platform-aware back affordance instead of always chevron, matching StreamAppBar's regular-pushed-page behaviour. - Subtitle colour: textTertiary → textSecondary. Tests: new stream_app_bar_test.dart with auto-implied leading + style precedence cases; sheet header tests pinned to iOS via a small _onPlatform helper for chevron-expecting cases (flutter_test defaults to Android so the platform-aware sheet branches need explicit pinning). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (2)
📒 Files selected for processing (1)
💤 Files with no reviewable changes (1)
📝 WalkthroughWalkthroughStreamAppBar is reimplemented as a slots-based widget with platform-aware auto-implied leading, backed by new theming primitives and a StreamHeaderToolbar three-slot layout. StreamSheetHeader is migrated to the toolbar, stream sheet drag math caches layout height, gallery demos/test coverage added, and exports/wiring updated. ChangesStreamAppBar Redesign & Theming
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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. Review rate limit: 0/1 reviews remaining, refill in 60 minutes.Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/stream_core_flutter/test/components/header/stream_sheet_header_test.dart (1)
417-419:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winTest comment references wrong color token.
The comment says
colorScheme.textTertiary, but the implementation instream_sheet_header.dart(line 412) usescolorScheme.textSecondary.📝 Proposed fix
// Subtitle falls through to defaults (neither props nor theme set it). - // Default subtitleTextStyle uses StreamTextTheme's captionDefault and - // colorScheme.textTertiary — non-null, non-zero font size. + // Default subtitleTextStyle uses StreamTextTheme's captionDefault and + // colorScheme.textSecondary — non-null, non-zero font size.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/stream_core_flutter/test/components/header/stream_sheet_header_test.dart` around lines 417 - 419, Update the incorrect test comment in stream_sheet_header_test.dart to reference the actual color token used by the implementation: change any mention of colorScheme.textTertiary to colorScheme.textSecondary to match the StreamSheetHeader implementation (see stream_sheet_header.dart where subtitle default style uses colorScheme.textSecondary); ensure the comment describes that the default subtitleTextStyle uses StreamTextTheme's captionDefault and colorScheme.textSecondary.
🧹 Nitpick comments (1)
packages/stream_core_flutter/test/components/header/stream_sheet_header_test.dart (1)
43-73: 💤 Low valueConsider using
_onPlatformhelper for consistency.These tests duplicate the try/finally pattern that
_onPlatformwas introduced to eliminate. The tests at lines 127, 170, 193, etc. already use the helper.♻️ Suggested refactor
- testWidgets('inserts arrow-left on a regular pushed route on Android', (tester) async { - debugDefaultTargetPlatformOverride = TargetPlatform.android; - try { - await tester.pumpWidget(_withStreamTheme(const _LauncherScreen())); - await tester.tap(find.text('Open')); - await tester.pumpAndSettle(); - - expect(find.byType(StreamButton), findsOneWidget); - expect(find.byIcon(StreamIconData.arrowLeft), findsOneWidget); - expect(find.byIcon(StreamIconData.chevronLeft), findsNothing); - expect(find.byIcon(StreamIconData.xmark), findsNothing); - } finally { - debugDefaultTargetPlatformOverride = null; - } - }); - - testWidgets('inserts back chevron on a regular pushed route on iOS', (tester) async { - debugDefaultTargetPlatformOverride = TargetPlatform.iOS; - try { - await tester.pumpWidget(_withStreamTheme(const _LauncherScreen())); - await tester.tap(find.text('Open')); - await tester.pumpAndSettle(); - - expect(find.byType(StreamButton), findsOneWidget); - expect(find.byIcon(StreamIconData.chevronLeft), findsOneWidget); - expect(find.byIcon(StreamIconData.arrowLeft), findsNothing); - expect(find.byIcon(StreamIconData.xmark), findsNothing); - } finally { - debugDefaultTargetPlatformOverride = null; - } + testWidgets( + 'inserts arrow-left on a regular pushed route on Android', + (tester) => _onPlatform(TargetPlatform.android, () async { + await tester.pumpWidget(_withStreamTheme(const _LauncherScreen())); + await tester.tap(find.text('Open')); + await tester.pumpAndSettle(); + + expect(find.byType(StreamButton), findsOneWidget); + expect(find.byIcon(StreamIconData.arrowLeft), findsOneWidget); + expect(find.byIcon(StreamIconData.chevronLeft), findsNothing); + expect(find.byIcon(StreamIconData.xmark), findsNothing); + }), + ); + + testWidgets( + 'inserts back chevron on a regular pushed route on iOS', + (tester) => _onPlatform(TargetPlatform.iOS, () async { + await tester.pumpWidget(_withStreamTheme(const _LauncherScreen())); + await tester.tap(find.text('Open')); + await tester.pumpAndSettle(); + + expect(find.byType(StreamButton), findsOneWidget); + expect(find.byIcon(StreamIconData.chevronLeft), findsOneWidget); + expect(find.byIcon(StreamIconData.arrowLeft), findsNothing); + expect(find.byIcon(StreamIconData.xmark), findsNothing); + }), + );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/stream_core_flutter/test/components/header/stream_sheet_header_test.dart` around lines 43 - 73, Replace the repeated try/finally platform override in the two widget tests with the existing _onPlatform helper to keep tests consistent and DRY: locate the tests that set debugDefaultTargetPlatformOverride = TargetPlatform.android/iOS and instead wrap the test body (pumpWidget, tap, pumpAndSettle, and expect assertions) inside a call to _onPlatform(TargetPlatform.android, () async { ... }) and _onPlatform(TargetPlatform.iOS, () async { ... }) respectively; keep the same assertions and references to _withStreamTheme and _LauncherScreen and remove the manual debugDefaultTargetPlatformOverride assignments and finally blocks.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In
`@packages/stream_core_flutter/test/components/header/stream_sheet_header_test.dart`:
- Around line 417-419: Update the incorrect test comment in
stream_sheet_header_test.dart to reference the actual color token used by the
implementation: change any mention of colorScheme.textTertiary to
colorScheme.textSecondary to match the StreamSheetHeader implementation (see
stream_sheet_header.dart where subtitle default style uses
colorScheme.textSecondary); ensure the comment describes that the default
subtitleTextStyle uses StreamTextTheme's captionDefault and
colorScheme.textSecondary.
---
Nitpick comments:
In
`@packages/stream_core_flutter/test/components/header/stream_sheet_header_test.dart`:
- Around line 43-73: Replace the repeated try/finally platform override in the
two widget tests with the existing _onPlatform helper to keep tests consistent
and DRY: locate the tests that set debugDefaultTargetPlatformOverride =
TargetPlatform.android/iOS and instead wrap the test body (pumpWidget, tap,
pumpAndSettle, and expect assertions) inside a call to
_onPlatform(TargetPlatform.android, () async { ... }) and
_onPlatform(TargetPlatform.iOS, () async { ... }) respectively; keep the same
assertions and references to _withStreamTheme and _LauncherScreen and remove the
manual debugDefaultTargetPlatformOverride assignments and finally blocks.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e5e9d331-9015-4cff-8a08-899b6e3440ad
⛔ Files ignored due to path filters (2)
packages/stream_core_flutter/test/components/header/goldens/ci/stream_sheet_header_dark_matrix.pngis excluded by!**/*.pngpackages/stream_core_flutter/test/components/header/goldens/ci/stream_sheet_header_light_matrix.pngis excluded by!**/*.png
📒 Files selected for processing (18)
apps/design_system_gallery/lib/app/gallery_app.directories.g.dartapps/design_system_gallery/lib/components/header/stream_app_bar.dartapps/design_system_gallery/lib/components/header/stream_sheet_header.dartpackages/stream_core_flutter/CHANGELOG.mdpackages/stream_core_flutter/lib/src/components.dartpackages/stream_core_flutter/lib/src/components/common/stream_app_bar.dartpackages/stream_core_flutter/lib/src/components/header/stream_app_bar.dartpackages/stream_core_flutter/lib/src/components/header/stream_header_toolbar.dartpackages/stream_core_flutter/lib/src/components/header/stream_sheet_header.dartpackages/stream_core_flutter/lib/src/components/sheet/stream_sheet.dartpackages/stream_core_flutter/lib/src/theme.dartpackages/stream_core_flutter/lib/src/theme/components/stream_app_bar_theme.dartpackages/stream_core_flutter/lib/src/theme/components/stream_app_bar_theme.g.theme.dartpackages/stream_core_flutter/lib/src/theme/stream_theme.dartpackages/stream_core_flutter/lib/src/theme/stream_theme.g.theme.dartpackages/stream_core_flutter/lib/src/theme/stream_theme_extensions.dartpackages/stream_core_flutter/test/components/header/stream_app_bar_test.dartpackages/stream_core_flutter/test/components/header/stream_sheet_header_test.dart
💤 Files with no reviewable changes (1)
- packages/stream_core_flutter/lib/src/components/common/stream_app_bar.dart
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #111 +/- ##
==========================================
+ Coverage 30.23% 31.38% +1.15%
==========================================
Files 161 163 +2
Lines 6232 6319 +87
==========================================
+ Hits 1884 1983 +99
+ Misses 4348 4336 -12 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
renefloor
approved these changes
May 5, 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
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 by CodeRabbit
New Features
Breaking Changes
Documentation