Repository navigation
Restore sidebar edge fades - #3502
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Caution Review failedPull request was closed or merged during review 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:
📝 WalkthroughWalkthroughAdds a bottom-side scrim to the sidebar, refactors scrim rendering into a shared, edge-parameterized ChangesSidebar Bottom Scrim & Scrim Refactor
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 12 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (12 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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. Review rate limit: 3/8 reviews remaining, refill in 36 minutes and 17 seconds.Comment |
Greptile SummaryRestores the sidebar's top edge blur scrim and adds a matching bottom edge scrim. The implementation extracts a shared Confidence Score: 5/5Safe to merge; no actor isolation, blocking, or layout-correctness issues found. All changes are pure SwiftUI/AppKit UI rendering with no state ownership changes, no blocking primitives, no actor isolation issues, and no file-size rule violations. The new file is 68 lines with a single clear responsibility. The ZStack layout change is self-consistent: the 50 pt bottom safeAreaInset comfortably exceeds the ~34 pt footer height, so no content is hidden at rest or at scroll end. No files require special attention. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
ZStack["ZStack(alignment: .bottomLeading)"]
ZStack --> ScrollArea["workspaceScrollArea\n(GeometryReader + ScrollView)"]
ZStack --> Footer["SidebarFooter\n(overlays at bottom)"]
ScrollArea --> SAInsetTop[".safeAreaInset(.top)\nworkspaceScrollTopVisibilityInset"]
ScrollArea --> SAInsetBot[".safeAreaInset(.bottom)\nsidebarBottomScrimHeight = 50pt"]
ScrollArea --> OverlayTop[".overlay(.top)\nSidebarTopScrim"]
ScrollArea --> OverlayBot[".overlay(.bottom)\nSidebarBottomScrim"]
OverlayTop --> EdgeScrimTop["SidebarEdgeScrim(edge: .top)\nblack→clear gradient mask\n+ NSVisualEffectView blur"]
OverlayBot --> EdgeScrimBot["SidebarEdgeScrim(edge: .bottom)\nclear→black gradient mask\n+ NSVisualEffectView blur"]
EdgeScrimTop --> BlurEffect["SidebarEdgeBlurEffect\nNSVisualEffectView\n.withinWindow / .underWindowBackground"]
EdgeScrimBot --> BlurEffect
Reviews (5): Last reviewed commit: "Fix sidebar scrim review feedback" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| .overlay(alignment: .bottom) { | ||
| SidebarBottomScrim(height: sidebarBottomScrimHeight) | ||
| .allowsHitTesting(false) | ||
| } |
There was a problem hiding this comment.
The top scrim has a companion .safeAreaInset(edge: .top, spacing: 0) (line ~10014) that pushes scroll content down so rows aren't pinned under the fade. The bottom scrim has no equivalent. When the user scrolls all the way to the last row, the row's bottom edge sits flush with the scroll frame's bottom — i.e., directly behind the 28 pt fade — while the top half of that row remains fully visible. Adding a matching bottom inset would keep the last row fully clear of the fade at rest.
.safeAreaInset(edge: .bottom, spacing: 0) {
Color.clear
.frame(height: sidebarBottomScrimHeight)
.allowsHitTesting(false)
}
.overlay(alignment: .bottom) {
SidebarBottomScrim(height: sidebarBottomScrimHeight)
.allowsHitTesting(false)
}There was a problem hiding this comment.
Already fixed in the current branch: the scroll area now has a matching bottom safeAreaInset using sidebarBottomScrimHeight before the bottom overlay.
— Claude Code
8cb3aab to
6d05355
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: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Sources/ContentView.swift`:
- Around line 12454-12502: The file exceeds the Swift file-length budget because
the four small private types were added to ContentView.swift; extract
SidebarTopScrim, SidebarBottomScrim, SidebarEdgeScrim, and SidebarEdgeBlurEffect
into a new file (e.g., SidebarScrim.swift), add the necessary imports (import
SwiftUI and import AppKit), drop the private modifier so the structs are
internal (keep the same type names and implementations), and remove the
duplicate definitions from ContentView.swift so the build uses the new
module-scoped types.
In `@Sources/WindowChromeMetrics.swift`:
- Line 35: Replace the hardcoded literal in bottomScrimHeight with the shared
constant to make the relationship explicit: use
WindowChromeMetrics.sharedChromeBarHeight instead of 28 in the static let
bottomScrimHeight declaration (or, if the footer height is intentionally
independent, add a short comment above bottomScrimHeight explaining why 28 was
chosen and that it should not track sharedChromeBarHeight). Ensure you update
the bottomScrimHeight symbol accordingly so readers see the intended coupling
(or documented independence).
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: d1225be9-2b93-47ba-800d-d56f6c062418
📒 Files selected for processing (2)
Sources/ContentView.swiftSources/WindowChromeMetrics.swift
| static let firstRowTopOffset: CGFloat = MinimalModeChromeMetrics.titlebarHeight + 2 | ||
| static let rowVerticalPadding: CGFloat = 8 | ||
| static let topScrimHeight: CGFloat = firstRowTopOffset + 20 | ||
| static let bottomScrimHeight: CGFloat = 28 |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | ⚡ Quick win
Consider referencing WindowChromeMetrics.sharedChromeBarHeight instead of the raw literal 28.
bottomScrimHeight is hardcoded to 28, which happens to equal WindowChromeMetrics.sharedChromeBarHeight. If the help/footer area height is intentionally tied to the chrome bar height, using the named constant makes that relationship explicit and prevents silent drift when the base height changes later:
♻️ Proposed refactor
- static let bottomScrimHeight: CGFloat = 28
+ static let bottomScrimHeight: CGFloat = WindowChromeMetrics.sharedChromeBarHeightIf the footer height is intentionally independent of the chrome bar height, at least add a brief comment explaining the chosen value.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Sources/WindowChromeMetrics.swift` at line 35, Replace the hardcoded literal
in bottomScrimHeight with the shared constant to make the relationship explicit:
use WindowChromeMetrics.sharedChromeBarHeight instead of 28 in the static let
bottomScrimHeight declaration (or, if the footer height is intentionally
independent, add a short comment above bottomScrimHeight explaining why 28 was
chosen and that it should not track sharedChromeBarHeight). Ensure you update
the bottomScrimHeight symbol accordingly so readers see the intended coupling
(or documented independence).
6d05355 to
741fa7d
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
Sources/ContentView.swift (1)
12455-12503:⚠️ Potential issue | 🟠 Major | ⚡ Quick winCI still failing: file length budget exceeded.
The pipeline continues to report
actual=16030, budget=15966. The scrim types (SidebarTopScrim,SidebarBottomScrim,SidebarEdgeScrim,SidebarEdgeBlurEffect) have no dependency onContentViewinternals and should be extracted to a separate file (e.g.,Sources/SidebarScrim.swift) to bringContentView.swiftback under the 15,966-line budget.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/ContentView.swift` around lines 12455 - 12503, Move the scrim views out of ContentView by creating a new Swift file containing the SidebarTopScrim, SidebarBottomScrim, SidebarEdgeScrim, and SidebarEdgeBlurEffect types (copy their current definitions exactly, including private and enum Edge), add the necessary import (SwiftUI/AppKit as needed), and remove these declarations from ContentView.swift so the file length drops below the budget; ensure makeNSView/updateNSView signatures and property names (height, edge, gradientColors, etc.) remain unchanged so existing usages compile.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@Sources/ContentView.swift`:
- Around line 12455-12503: Move the scrim views out of ContentView by creating a
new Swift file containing the SidebarTopScrim, SidebarBottomScrim,
SidebarEdgeScrim, and SidebarEdgeBlurEffect types (copy their current
definitions exactly, including private and enum Edge), add the necessary import
(SwiftUI/AppKit as needed), and remove these declarations from ContentView.swift
so the file length drops below the budget; ensure makeNSView/updateNSView
signatures and property names (height, edge, gradientColors, etc.) remain
unchanged so existing usages compile.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 20c444a8-2528-4c12-a9c1-2a827ed63423
📒 Files selected for processing (2)
Sources/ContentView.swiftSources/WindowChromeMetrics.swift
741fa7d to
3c3b623
Compare
3c3b623 to
f3a29e8
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
Sources/ContentView.swift (1)
12454-12510:⚠️ Potential issue | 🟠 Major | ⚡ Quick winCI still failing: file-length budget exceeded — scrim types must be extracted.
The pipeline reports
actual=16037, budget=15966(+71 lines).SidebarBottomScrim,SidebarEdgeScrim, andSidebarEdgeBlurEffectwere added but not extracted to a separate file, which is exactly what is required to bringContentView.swiftback under budget.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/ContentView.swift` around lines 12454 - 12510, Extract the three scrim view types into a new Swift file: create a new file and move SidebarBottomScrim, SidebarEdgeScrim, and SidebarEdgeBlurEffect there (preserve their implementations exactly), then update their access level so they remain usable from ContentView.swift (remove or change the top-level private to an appropriate internal/fileprivate visibility in the new file), ensure any required imports (SwiftUI/AppKit) are present, and confirm that ContentView still references SidebarBottomScrim unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@Sources/ContentView.swift`:
- Around line 12454-12510: Extract the three scrim view types into a new Swift
file: create a new file and move SidebarBottomScrim, SidebarEdgeScrim, and
SidebarEdgeBlurEffect there (preserve their implementations exactly), then
update their access level so they remain usable from ContentView.swift (remove
or change the top-level private to an appropriate internal/fileprivate
visibility in the new file), ensure any required imports (SwiftUI/AppKit) are
present, and confirm that ContentView still references SidebarBottomScrim
unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 10cf61ef-eb05-4e30-a33d-b8364d53cd4a
📒 Files selected for processing (2)
Sources/ContentView.swiftSources/WindowChromeMetrics.swift
There was a problem hiding this comment.
♻️ Duplicate comments (1)
Sources/ContentView.swift (1)
12451-12507:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winCI still blocked: file-length budget exceeded (16034 > 15966).
The four new private types (
SidebarBottomScrim,SidebarEdgeScrim,SidebarEdgeBlurEffect, plus the refactoredSidebarTopScrim) pushed the file to 16,034 lines, 68 lines over the 15,966-line budget. The pipeline fails onscripts/swift_file_length_budget.py. The fix is to extract these self-contained types into a new file (e.g.Sources/SidebarScrim.swift).🛠️ Suggested extraction
-// ContentView.swift — remove these four private structs -private struct SidebarTopScrim: View { … } -private struct SidebarBottomScrim: View { … } -private struct SidebarEdgeScrim: View { … } -private struct SidebarEdgeBlurEffect: NSViewRepresentable { … }// Sources/SidebarScrim.swift (new file) import SwiftUI import AppKit struct SidebarTopScrim: View { let height: CGFloat var body: some View { SidebarEdgeScrim(height: height, edge: .top) } } struct SidebarBottomScrim: View { let height: CGFloat var body: some View { SidebarEdgeScrim(height: height, edge: .bottom) } } struct SidebarEdgeScrim: View { enum Edge { case top, bottom } let height: CGFloat let edge: Edge var body: some View { SidebarEdgeBlurEffect() .frame(height: height) .mask(LinearGradient(colors: gradientColors, startPoint: .top, endPoint: .bottom)) } private var gradientColors: [Color] { let colors: [Color] = [.black.opacity(0.95), .black.opacity(0.75), .black.opacity(0.35), .clear] return edge == .top ? colors : colors.reversed() } } struct SidebarEdgeBlurEffect: NSViewRepresentable { func makeNSView(context: Context) -> NSVisualEffectView { let view = NSVisualEffectView() view.blendingMode = .withinWindow view.material = .underWindowBackground view.state = .active view.isEmphasized = false return view } func updateNSView(_ nsView: NSVisualEffectView, context: Context) {} }Drop
privateso the types areinternaland remain visible within the module.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/ContentView.swift` around lines 12451 - 12507, The file exceeds the length budget because four small private types (SidebarBottomScrim, SidebarEdgeScrim, SidebarEdgeBlurEffect and the refactored SidebarTopScrim) were added; fix it by extracting these types into a new file (e.g. create Sources/SidebarScrim.swift) and move the implementations of SidebarBottomScrim, SidebarEdgeScrim, SidebarEdgeBlurEffect and SidebarTopScrim there, removing the private modifier so they remain internal to the module; ensure you import SwiftUI and AppKit in the new file and update any callers if necessary.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@Sources/ContentView.swift`:
- Around line 12451-12507: The file exceeds the length budget because four small
private types (SidebarBottomScrim, SidebarEdgeScrim, SidebarEdgeBlurEffect and
the refactored SidebarTopScrim) were added; fix it by extracting these types
into a new file (e.g. create Sources/SidebarScrim.swift) and move the
implementations of SidebarBottomScrim, SidebarEdgeScrim, SidebarEdgeBlurEffect
and SidebarTopScrim there, removing the private modifier so they remain internal
to the module; ensure you import SwiftUI and AppKit in the new file and update
any callers if necessary.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 908015a3-f137-4a8b-8859-c242f78aedb1
📒 Files selected for processing (2)
Sources/ContentView.swiftSources/WindowChromeMetrics.swift
Addressed in 3031fdb by moving sidebar scrim helpers into Sources/SidebarScrim.swift; workflow-guard-tests now passes and the CodeRabbit thread is resolved.
This branch was successfully deployed
1 participant
Summary
Testing
Issues
Summary by cubic
Restores the sidebar top fade and adds a matching bottom fade above the footer/help area to improve depth and readability without changing interactions. Uses a native blur scrim masked by a gradient at both edges.
SidebarTopScrimandSidebarBottomScrim, built on sharedSidebarEdgeScrim+SidebarEdgeBlurEffectwith a gradient mask (reversed for bottom).ZStackso the footer overlays; add top/bottom.safeAreaInsetpadding; disable hit testing on scrims/insets; setSidebarWorkspaceListMetrics.bottomScrimHeight=topScrimHeightfor consistent spacing.Written for commit 3031fdb. Summary will update on new commits.
Summary by CodeRabbit
New Features
Refactor