sidebar: Clip floating surface materials#5
Merged
Conversation
WalkthroughThe change updates the GPUI revision, initializes content masks with defaults, reworks surface noise tiling, and changes sidebar elevation defaults and rendering. Floating sidebar stories, tests, and documentation now expose and describe elevation settings. ChangesSidebar rendering
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Story as FloatingSidebarStory
participant FloatingSidebar
participant SidebarShell
participant SurfacePreset
Story->>FloatingSidebar: Apply selected elevation
FloatingSidebar->>SidebarShell: Render configured shell
SidebarShell->>SurfacePreset: Apply panel elevation
SurfacePreset-->>SidebarShell: Render elevated panel
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Owner
Author
|
@codex review this PR |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 `@crates/ui/src/sidebar_shell/mod.rs`:
- Around line 378-380: Update SidebarShell’s elevation handling so the
theme-controlled surface panel shadow remains the default: make the elevation
override optional and apply with_elevation only when explicitly provided. Update
FloatingSidebar to stop forwarding its default elevation as an override, while
preserving explicit elevation overrides.
In `@crates/ui/src/surface/mod.rs`:
- Around line 515-517: Update noise_tile_layout to clamp the rounded
tile_size_value to a minimum of 1px before converting it with px, preventing
zero-sized tiles at extreme scale factors. Add a regression test covering scale
factors above 256 and verify render_noise_tiles receives a bounded tile range.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 93bf5604-f762-4d42-b6cc-78d711466032
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (11)
Cargo.tomlcrates/story/src/stories/floating_sidebar_story.rscrates/ui/src/floating_sidebar/mod.rscrates/ui/src/scroll/scrollable_mask.rscrates/ui/src/scroll/scrollbar.rscrates/ui/src/sidebar_shell/mod.rscrates/ui/src/surface/mod.rscrates/ui/src/virtual_list.rscrates/webview/src/lib.rsdocs/docs/components/sidebar.mdvendor/gpui
Comment on lines
378
to
380
| let sidebar_surface = SurfacePreset::panel() | ||
| .with_elevation(self.elevation) | ||
| .wrap_with_bounds( |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== sidebar_shell outline ==\n'
ast-grep outline crates/ui/src/sidebar_shell/mod.rs --view expanded || true
printf '\n== relevant lines in sidebar_shell ==\n'
sed -n '320,450p' crates/ui/src/sidebar_shell/mod.rs
printf '\n== search for SurfacePreset definitions and with_elevation ==\n'
rg -n "struct SurfacePreset|impl SurfacePreset|with_elevation|use_theme_elevation_defaults|surface_panel_shadow|FloatingSidebar" crates/ui/src -SRepository: BumpyClock/gpui-component
Length of output: 11367
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== SurfacePreset implementation slice ==\n'
sed -n '191,470p' crates/ui/src/surface/mod.rs
printf '\n== FloatingSidebar render slice ==\n'
sed -n '230,340p' crates/ui/src/floating_sidebar/mod.rs
printf '\n== SidebarShell constructor/default slice ==\n'
sed -n '160,210p' crates/ui/src/sidebar_shell/mod.rs
printf '\n== tests around shadow defaults ==\n'
sed -n '480,620p' crates/ui/src/floating_sidebar/mod.rsRepository: BumpyClock/gpui-component
Length of output: 17975
Preserve the theme-controlled default elevation. SidebarShell always calls with_elevation(self.elevation), which disables use_theme_elevation_defaults and forces the sidebar shadow to ElevationToken::Sm instead of cx.theme().elevation.surface_panel_shadow. Make elevation an optional override and only apply it when explicitly set; FloatingSidebar should stop forwarding its default as an override too.
🤖 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 `@crates/ui/src/sidebar_shell/mod.rs` around lines 378 - 380, Update
SidebarShell’s elevation handling so the theme-controlled surface panel shadow
remains the default: make the elevation override optional and apply
with_elevation only when explicitly provided. Update FloatingSidebar to stop
forwarding its default elevation as an override, while preserving explicit
elevation overrides.
Comment on lines
+515
to
+517
| fn noise_tile_layout(width: Pixels, height: Pixels, scale_factor: f32) -> NoiseTileLayout { | ||
| let tile_size_value = (GLASS_NOISE_TILE_SIZE_BASE / scale_factor.max(1.0)).round(); | ||
| let tile_size = px(tile_size_value); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="crates/ui/src/surface/mod.rs"
echo "== outline =="
ast-grep outline "$FILE" --view expanded || true
echo
echo "== relevant lines =="
sed -n '500,560p' "$FILE" | cat -n
echo
echo "== search for noise_tile_layout uses =="
rg -n "noise_tile_layout|GLASS_NOISE_TILE_SIZE_BASE|tile_size_value|tile_size" "$FILE"Repository: BumpyClock/gpui-component
Length of output: 7125
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="crates/ui/src/surface/mod.rs"
echo "== render_noise_tiles section =="
sed -n '543,590p' "$FILE" | cat -n
echo
echo "== existing tests =="
sed -n '590,660p' "$FILE" | cat -n
echo
echo "== zero-tile-size behavior probe =="
python3 - <<'PY'
import math
base = 128.0
for scale in [128.0, 256.0, 257.0, 512.0, 1000.0]:
tile = round(base / max(scale, 1.0))
cols = math.ceil(200.0 / tile) if tile != 0 else "DIV0"
rows = math.ceil(150.0 / tile) if tile != 0 else "DIV0"
tiles = (cols * rows) if isinstance(cols, int) else "DIV0"
print(scale, tile, cols, rows, tiles)
PYRepository: BumpyClock/gpui-component
Length of output: 4246
Clamp the rounded tile size to at least 1px.
For scale factors above 256, this rounds to 0 and render_noise_tiles can expand to a near-unbounded tile range. Add a high-scale regression test.
Proposed fix
- let tile_size_value = (GLASS_NOISE_TILE_SIZE_BASE / scale_factor.max(1.0)).round();
+ let tile_size_value =
+ (GLASS_NOISE_TILE_SIZE_BASE / scale_factor.max(1.0))
+ .round()
+ .max(1.0);📝 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.
Suggested change
| fn noise_tile_layout(width: Pixels, height: Pixels, scale_factor: f32) -> NoiseTileLayout { | |
| let tile_size_value = (GLASS_NOISE_TILE_SIZE_BASE / scale_factor.max(1.0)).round(); | |
| let tile_size = px(tile_size_value); | |
| fn noise_tile_layout(width: Pixels, height: Pixels, scale_factor: f32) -> NoiseTileLayout { | |
| let tile_size_value = | |
| (GLASS_NOISE_TILE_SIZE_BASE / scale_factor.max(1.0)) | |
| .round() | |
| .max(1.0); | |
| let tile_size = px(tile_size_value); |
🤖 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 `@crates/ui/src/surface/mod.rs` around lines 515 - 517, Update
noise_tile_layout to clamp the rounded tile_size_value to a minimum of 1px
before converting it with px, preventing zero-sized tiles at extreme scale
factors. Add a regression test covering scale factors above 256 and verify
render_noise_tiles receives a bounded tile range.
4 tasks
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
1 participant
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
SurfacePresetown one rounded surface geometry for background, backdrop blur, noise, content, border, and elevationSidebarbackground transparentvendor/gpuito merged rounded-mask support from gpui: Support rounded content masks gpui#5 and the Windows retained-layer follow-up windows: Clip rounded retained layers gpui#6 (4332ea7deae4838c12bad6ea64292ca22a33cf98)Verification
cargo check -p gpui-component -p gpui-component-story -p gpui-wrycargo check -p gpui-component --testscargo fmt --all --checkgit diff --check main...HEADPlatform notes
AI assistance
Implementation and review were AI-assisted with Claude Code. CodeRabbit and an independent Codex review were also run before opening this PR.