fix(ui): floating improvements - #141
Conversation
|
Important Review skippedReview was skipped due to path filters ⛔ Files ignored due to path filters (1)
CodeRabbit blocks several paths by default. You can override this behavior by explicitly including those paths in the path filters. For example, including ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughAdds theme-backed ChangesElevation and floating components
Changelog placement enforcement
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Gallery
participant StreamButton
participant ButtonDefaults
participant BuildContext
Gallery->>StreamButton: pass isFloating from knob
StreamButton->>ButtonDefaults: pass interaction state
ButtonDefaults->>BuildContext: read streamElevation
BuildContext-->>ButtonDefaults: return themed elevation
ButtonDefaults-->>Gallery: render floating button
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
6d217ae to
756fb91
Compare
![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)
packages/stream_core_flutter/test/components/buttons/stream_button_golden_test.dart (1)
102-124: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd dark-theme coverage for labelled floating buttons.
This new matrix only exercises the light theme, while the changed floating appearance is a theme-sensitive visual state. Add a matching dark-theme matrix so outline/ghost fills and elevation shadows are covered in both primary themes.
As per coding guidelines, visible components require golden coverage for their primary theme variants.
🤖 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_core_flutter/test/components/buttons/stream_button_golden_test.dart` around lines 102 - 124, Add a second golden test matrix matching the existing “renders floating button correctly” coverage, but build its scenarios with the dark theme through _buildButtonInTheme or the established dark-theme helper. Keep the same StreamButtonStyle and StreamButtonType combinations, floating configuration, and scenario constraints so outline/ghost fills and elevation shadows are validated in both themes.Source: Coding guidelines
🤖 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_core_flutter/CHANGELOG.md`:
- Line 12: Move the changelog entry describing the default StreamButton
isFloating behavior from the 0.4.1 section into the package’s Upcoming section,
preserving its wording and formatting.
---
Nitpick comments:
In
`@packages/stream_core_flutter/test/components/buttons/stream_button_golden_test.dart`:
- Around line 102-124: Add a second golden test matrix matching the existing
“renders floating button correctly” coverage, but build its scenarios with the
dark theme through _buildButtonInTheme or the established dark-theme helper.
Keep the same StreamButtonStyle and StreamButtonType combinations, floating
configuration, and scenario constraints so outline/ghost fills and elevation
shadows are validated in both themes.
🪄 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 Plus
Run ID: 72a8acd7-9460-431c-b9ff-9017b59881a2
⛔ Files ignored due to path filters (1)
packages/stream_core_flutter/test/components/buttons/goldens/ci/stream_button_floating.pngis excluded by!**/*.png
📒 Files selected for processing (8)
STYLE_GUIDE.mdapps/design_system_gallery/lib/components/buttons/button.dartpackages/stream_core_flutter/CHANGELOG.mdpackages/stream_core_flutter/lib/core.dartpackages/stream_core_flutter/lib/src/components/avatar/stream_avatar.dartpackages/stream_core_flutter/lib/src/components/buttons/stream_button.dartpackages/stream_core_flutter/lib/src/theme/primitives/stream_elevation.dartpackages/stream_core_flutter/test/components/buttons/stream_button_golden_test.dart
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 @.github/workflows/pr_title.yml:
- Around line 88-93: Update the fetch function in the PR title workflow so only
a confirmed GitHub 404 creates an empty output file for a missing changelog;
propagate authentication, rate-limit, and other gh api failures instead of
masking them. Preserve successful raw-content fetching and ensure callers fail
closed when both revision requests cannot be retrieved.
- Around line 80-81: Update the jq filename filter in the changelogs collection
command to match only paths whose final filename is exactly CHANGELOG.md,
excluding names such as MYCHANGELOG.md while retaining changelog files in nested
directories.
🪄 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 Plus
Run ID: 1120b0d2-0577-4031-bc6a-2d1e079acafa
⛔ Files ignored due to path filters (1)
packages/stream_core_flutter/test/components/buttons/goldens/ci/stream_button_floating.pngis excluded by!**/*.png
📒 Files selected for processing (10)
.github/workflows/pr_title.yml.github/workflows/scripts/check-changelog-placement.shSTYLE_GUIDE.mdapps/design_system_gallery/lib/components/buttons/button.dartpackages/stream_core_flutter/CHANGELOG.mdpackages/stream_core_flutter/lib/core.dartpackages/stream_core_flutter/lib/src/components/avatar/stream_avatar.dartpackages/stream_core_flutter/lib/src/components/buttons/stream_button.dartpackages/stream_core_flutter/lib/src/theme/primitives/stream_elevation.dartpackages/stream_core_flutter/test/components/buttons/stream_button_golden_test.dart
🚧 Files skipped from review as they are similar to previous changes (2)
- STYLE_GUIDE.md
- packages/stream_core_flutter/CHANGELOG.md
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #141 +/- ##
==========================================
+ Coverage 53.06% 54.40% +1.33%
==========================================
Files 184 185 +1
Lines 7558 7569 +11
==========================================
+ Hits 4011 4118 +107
+ Misses 3547 3451 -96 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 @.github/workflows/pr_title.yml:
- Around line 72-75: Shorten the HEAD_BRANCH expression in the workflow while
preserving its existing fork-PR behavior: use the head ref only when the head
repository matches github.repository, otherwise use an empty string. Keep the
YAML line at or below 120 characters.
In `@packages/stream_core_flutter/lib/src/theme/stream_theme.dart`:
- Line 285: Update the deprecated brightness parameter in StreamTheme.raw to
remain nullable, and derive its stored value from colorScheme.brightness when
null is provided. Preserve explicitly supplied brightness values while ensuring
StreamTheme.raw(..., brightness: null) remains valid and dark color schemes do
not default to light.
In
`@packages/stream_core_flutter/test/components/buttons/stream_button_golden_test.dart`:
- Around line 126-148: Add the macOS golden asset for the disabled floating
button scenario covered by the goldenTest named “renders disabled floating
button correctly” and filename “stream_button_floating_disabled”. Ensure the
generated asset is placed in the existing macOS goldens collection and matches
all StreamButtonStyle and StreamButtonType scenarios.
🪄 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 Plus
Run ID: c6e1513a-ed49-4e18-8d88-54f5c9ac59b1
📒 Files selected for processing (11)
.github/workflows/pr_title.yml.github/workflows/scripts/check-changelog-placement.shSTYLE_GUIDE.mdpackages/stream_core_flutter/CHANGELOG.mdpackages/stream_core_flutter/lib/src/theme/primitives/stream_elevation.dartpackages/stream_core_flutter/lib/src/theme/primitives/stream_elevation.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/test/components/buttons/stream_button_golden_test.dartpackages/stream_core_flutter/test/components/buttons/stream_button_test.dartpackages/stream_core_flutter/test/theme/stream_theme_test.dart
🚧 Files skipped from review as they are similar to previous changes (2)
- STYLE_GUIDE.md
- packages/stream_core_flutter/CHANGELOG.md
| # Blank for a fork PR, so a fork cannot claim the release-branch | ||
| # exemption by naming its branch `release/...`. Releases are cut in | ||
| # this repo, so they always take the non-blank path. | ||
| HEAD_BRANCH: ${{ github.event.pull_request.head.repo.full_name == github.repository && github.event.pull_request.head.ref || '' }} |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Keep this YAML line within 120 characters.
Line 75 is 126 characters. As per coding guidelines, **/*.{dart,yaml} must use a maximum line width of 120 characters.
Proposed fix
- HEAD_BRANCH: ${{ github.event.pull_request.head.repo.full_name == github.repository && github.event.pull_request.head.ref || '' }}
+ HEAD_BRANCH: >-
+ ${{ github.event.pull_request.head.repo.full_name == github.repository &&
+ github.event.pull_request.head.ref || '' }}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # Blank for a fork PR, so a fork cannot claim the release-branch | |
| # exemption by naming its branch `release/...`. Releases are cut in | |
| # this repo, so they always take the non-blank path. | |
| HEAD_BRANCH: ${{ github.event.pull_request.head.repo.full_name == github.repository && github.event.pull_request.head.ref || '' }} | |
| # Blank for a fork PR, so a fork cannot claim the release-branch | |
| # exemption by naming its branch `release/...`. Releases are cut in | |
| # this repo, so they always take the non-blank path. | |
| HEAD_BRANCH: >- | |
| ${{ github.event.pull_request.head.repo.full_name == github.repository && | |
| github.event.pull_request.head.ref || '' }} |
🤖 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 @.github/workflows/pr_title.yml around lines 72 - 75, Shorten the HEAD_BRANCH
expression in the workflow while preserving its existing fork-PR behavior: use
the head ref only when the head repository matches github.repository, otherwise
use an empty string. Keep the YAML line at or below 120 characters.
Source: Coding guidelines
|
|
||
| const StreamTheme.raw({ | ||
| @Deprecated('Use colorScheme.brightness instead') Brightness? brightness, | ||
| @Deprecated('Use colorScheme.brightness instead') this.brightness = Brightness.light, |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
cd packages/stream_core_flutter/lib/src/theme
echo "## Style/test guide files"
for f in STYLE_GUIDE.md TESTING.md analysis_options.yaml; do
if [ -f "../../../$f" ]; then echo "FOUND ../../../$f"; else echo "MISSING ../../../$f"; fi
done
echo "## stream_theme.dart outline"
ast-grep outline stream_theme.dart --view compact || true
echo "## Relevant stream_theme.dart lines"
sed -n '240,390p' stream_theme.dart | nl -ba -v240
echo "## Companion generated file lines if present"
if [ -f generated-stream_theme.g.theme.dart ]; then
sed -n '240,390p' generated-stream_theme.g.theme.dart | nl -ba -v240
fi
echo "## Search for StreamTheme.raw usages/tests"
rg -n "StreamTheme\.raw|brightness:" . src/.. lib/.. tests/ test/ 2>/dev/null | head -200Repository: GetStream/stream-core-flutter
Length of output: 549
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "## Repo-wide StreamTheme.raw and raw brightness usage"
rg -n "StreamTheme\.raw|StreamTheme\.raw\(.*brightness|brightness\s*:" . --glob '!build/**' --glob '!dist/**' --glob '!node_modules/**' | head -300
echo "## AST/line count around dart theme constructor and constructor copyWith"
wc -l stream_theme.dart generated-stream_theme.g.theme.dart
sed -n '1,220p' stream_theme.dart | nl -ba -v1
echo "## Read-only behavioral probe: Dart syntax/field initializer normalization"
python3 - <<'PY'
from pathlib import Path
p = Path('stream_theme.dart')
s = p.read_text()
for needle in ['colorScheme.brightness', 'this.brightness = Brightness.light']:
print(needle, s.find(needle))
print("has constructor initializer for brightness before colorScheme:", s.find('this.brightness') < s.find('colorScheme.brightness') if ('this.brightness' in s and 'colorScheme.brightness' in s) else None)
PYRepository: GetStream/stream-core-flutter
Length of output: 14669
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "## Parse relevant constructor/instance field shapes from stream_theme.dart"
python3 - <<'PY'
from pathlib import Path
import re
text = Path('stream_theme.dart').read_text()
for m in re.finditer(r'(?:`@Deprecated`[^;\n]*;[ \t\n]*)*?((?:const )?StreamTheme\.[A-Za-z0-9_]+)\s*\([^\)]*\)', text, re.S):
start = m.start().rfind('\n') + 1
end = m.end() + text[m.end():].find('};') + 1 if '};' in text[m.end():] else m.end()
block = text[start:end]
if 'brightness' in block or 'colorScheme' in block:
print("\n--- " + m.group(1) + " ---")
for i, line in enumerate(block.splitlines(), start):
if line.strip().startswith('//'): continue
print(f"{i}:{line}")
# Check generated exports and copy/memo generated constructor-like lines for StreamTheme.raw brightness.
gpath = Path('generated-stream_theme.g.theme.dart')
if gpath.exists():
print("\n## generated occurrences")
for i, line in enumerate(gpath.read_text().splitlines(), 1):
if 'StreamTheme' in line or 'brightness' in line:
print(f"{i}:{line}")
PYRepository: GetStream/stream-core-flutter
Length of output: 770
🏁 Script executed:
#!/bin/bash
set -euo pipefail
cd packages/stream_core_flutter/lib/src/theme
echo "## File availability"
ls -l stream_theme.dart generated-stream_theme.g.theme.dart 2>&1 || true
echo "## Relevant stream_theme.dart lines"
sed -n '230,385p' stream_theme.dart
echo "## Relevant generated-copyWith behavior"
sed -n '55,165p' generated-stream_theme.g.theme.dart
echo "## StreamTheme.raw parameter occurrences"
rg -n "StreamTheme\.raw\(|brightness\b" stream_theme.dart
echo "## Theme tests relevant to raw brightness semantics"
sed -n '1,120p' ../../../test/theme/stream_theme_test.dart
echo "## changelog context"
sed -n '1,45p' ../../../CHANGELOG.mdRepository: GetStream/stream-core-flutter
Length of output: 6397
Preserve StreamTheme.raw’s nullable/derived brightness contract.
Making brightness non-null with a light default makes a StreamTheme.raw(..., brightness: null) constructor call fail and can expose a light brightness value for themes whose colorScheme.brightness is dark. Keep the parameter nullable and normalize it to colorScheme.brightness unless this is an intentional breaking API change.
🤖 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_core_flutter/lib/src/theme/stream_theme.dart` at line 285,
Update the deprecated brightness parameter in StreamTheme.raw to remain
nullable, and derive its stored value from colorScheme.brightness when null is
provided. Preserve explicitly supplied brightness values while ensuring
StreamTheme.raw(..., brightness: null) remains valid and dark color schemes do
not default to light.
There was a problem hiding this comment.
This is a follow up based on this PR: #131
The issue we introduced there was that in the generated copyWith the brightness is removed. But code generation did not run in that PR. We've re-introduced the brightness field here to prevent breaking changes. Making it nullable in the constructor and setting the value later like brightness ?? colorScheme.brightness can't happen inside the const constructor.
There was a problem hiding this comment.
@renefloor, thanks for the context—restoring brightness as a stored field to keep the generated copyWith API from regressing after #131 makes sense.
The const-constructor limitation is valid. However, the specific compatibility concern remains: the previous StreamTheme.raw signature accepted brightness: null, while the new non-nullable parameter rejects that existing call pattern and defaults a dark colorScheme to Brightness.light when omitted.
So I’d treat this as an intentional API tradeoff to preserve const StreamTheme.raw and generated copyWith, rather than a fully non-breaking restoration. The finding should remain open unless that source-compatibility change is acceptable for this release.
🐇🔎
✏️ Learnings added
Learnt from: renefloor
Repo: GetStream/stream-core-flutter PR: 141
File: packages/stream_core_flutter/lib/src/theme/stream_theme.dart:285-285
Timestamp: 2026-07-30T12:00:02.325Z
Learning: In `packages/stream_core_flutter`, PR `#131` failed to run theme-extension code generation, causing generated `StreamTheme.copyWith` to omit `brightness`. PR `#141` restores `StreamTheme.brightness` as a stored field so code generation preserves it. `StreamTheme.raw` remains const, so it cannot derive a non-null stored brightness from `colorScheme.brightness` when a nullable constructor argument is null.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
You are interacting with an AI system.
2 participants
Submit a pull request
Linear: FLU-502
CLA
Description of the pull request
This makes some improvements in the floating UI.
It also fixes the changelog and adds a check for future PR's.
It adds floating style to regular StreamButton
It adds a StreamElevation class for easier use of material elevation
It updates the style guide to prefer elevation tokens instead of shadows.
Summary by CodeRabbit
Summary by CodeRabbit
StreamElevationas a first-class theme primitive, now available via theme and context access.StreamButtonto accept nullableisFloatingfor consistent floating elevation behavior.StreamBoxShadowapplies.StreamButtonstates, plus expanded theme/elevation test coverage.