Fix sidebar edge fade background - #4610
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.
|
📝 WalkthroughWalkthroughReplaces AppKit-backed scrim/blur overlays with a SwiftUI gradient-based edge fade mask and applies it to the workspace list and extension sidebar timeline scroll areas, using configured top and bottom scrim heights. ChangesEdge Fade Mask Replacement
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
🚥 Pre-merge checks | ✅ 4 | ❌ 13❌ Failed checks (1 warning, 12 inconclusive)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the 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 |
Greptile SummaryReplaces the two-layer overlay approach (blur
Confidence Score: 5/5Safe to merge — the change removes an NSViewRepresentable and the observer machinery it required, replacing both call sites with a pure SwiftUI mask. No logic is at risk. The diff deletes more than it adds: the NSVisualEffectView representable, the separate top/bottom scrim overlay views, and the associated layout are all gone. The replacement mask view is a straightforward VStack of two gradients and a solid Rectangle with correctly inverted opacity stops for mask semantics. No async work, no shared state, no actor crossings, and no user-facing strings are introduced. No files require special attention. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A["VerticalTabsSidebar scroll content\n(.mask applied)"]
subgraph OLD["Old approach (removed)"]
B1["SidebarTopScrim overlay\n(SidebarEdgeBlurEffect + gradient mask)"]
B2["SidebarBottomScrim overlay\n(SidebarEdgeBlurEffect + gradient mask)"]
end
subgraph NEW["New approach"]
C["SidebarWorkspaceScrollEdgeFadeMask\n(SwiftUI .mask)"]
C1["SidebarEdgeFadeGradient(edge: .top)\n.frame(height: topHeight)\nopacity: 0.05 → 1.0"]
C2["Rectangle().fill(.black)\n(fully opaque — content visible)"]
C3["SidebarEdgeFadeGradient(edge: .bottom)\n.frame(height: bottomHeight)\nopacity: 1.0 → 0.05"]
C --> C1
C --> C2
C --> C3
end
A --> NEW
Reviews (3): Last reviewed commit: "Fix sidebar edge fade background" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| func updateNSView(_ nsView: SidebarScrollEdgeObserverView, context: Context) { | ||
| nsView.configure(currentVisibility: visibility) { newVisibility in | ||
| visibility = newVisibility | ||
| } | ||
| nsView.resolveScrollView() | ||
| } |
There was a problem hiding this comment.
updateNSView calls resolveScrollView() on every SwiftUI pass, which enqueues a DispatchQueue.main.async each time the parent redraws (including every scroll-triggered state update). The view-lifecycle overrides viewDidMoveToSuperview and viewDidMoveToWindow already handle initial discovery and any subsequent move to a different scroll view, so the extra dispatch in updateNSView only adds churn. If the concern is a SwiftUI hierarchy rebuild that leaves the view in place while swapping the enclosing scroll view, a targeted resolveScrollView() could be guarded to only run once the observer isn't already installed.
| func updateNSView(_ nsView: SidebarScrollEdgeObserverView, context: Context) { | |
| nsView.configure(currentVisibility: visibility) { newVisibility in | |
| visibility = newVisibility | |
| } | |
| nsView.resolveScrollView() | |
| } | |
| func updateNSView(_ nsView: SidebarScrollEdgeObserverView, context: Context) { | |
| nsView.configure(currentVisibility: visibility) { newVisibility in | |
| visibility = newVisibility | |
| } | |
| // resolveScrollView is already called from viewDidMoveToSuperview/viewDidMoveToWindow. | |
| // Only re-run discovery if no scroll view has been found yet (e.g. first pass | |
| // before the hierarchy stabilises), not on every SwiftUI invalidation. | |
| if !nsView.hasObservedScrollView { | |
| nsView.resolveScrollView() | |
| } | |
| } |
| let clipView = scrollView.contentView | ||
| clipView.postsBoundsChangedNotifications = true | ||
| clipView.postsFrameChangedNotifications = true | ||
| observers.append(NotificationCenter.default.addObserver( | ||
| forName: NSView.boundsDidChangeNotification, | ||
| object: clipView, | ||
| queue: .main | ||
| ) { [weak self] _ in | ||
| self?.updateVisibility() | ||
| }) | ||
| observers.append(NotificationCenter.default.addObserver( | ||
| forName: NSView.frameDidChangeNotification, | ||
| object: clipView, | ||
| queue: .main | ||
| ) { [weak self] _ in | ||
| self?.updateVisibility() | ||
| }) | ||
|
|
||
| if let documentView { | ||
| documentView.postsBoundsChangedNotifications = true | ||
| documentView.postsFrameChangedNotifications = true | ||
| observers.append(NotificationCenter.default.addObserver( | ||
| forName: NSView.boundsDidChangeNotification, | ||
| object: documentView, | ||
| queue: .main | ||
| ) { [weak self] _ in | ||
| self?.updateVisibility() | ||
| }) | ||
| observers.append(NotificationCenter.default.addObserver( | ||
| forName: NSView.frameDidChangeNotification, | ||
| object: documentView, | ||
| queue: .main | ||
| ) { [weak self] _ in | ||
| self?.updateVisibility() | ||
| }) | ||
| } |
There was a problem hiding this comment.
postsBoundsChangedNotifications not restored on cleanup
installObservers enables postsBoundsChangedNotifications = true and postsFrameChangedNotifications = true on both the clip view and the document view, but removeObservers() only removes the NSObjectProtocol tokens — it never resets these flags. If the sidebar's scroll view is replaced (triggering removeObservers for the old one), the old clip and document views keep posting notifications to any future listener. If another component in the codebase assumes those flags are false for those views, this becomes a silent source of unexpected callbacks. Capture the previous values before setting them and restore them in removeObservers.
| } | ||
|
|
||
| final class SidebarScrollEdgeVisibilityTests: XCTestCase { | ||
| func testHiddenWhenDocumentDoesNotOverflowViewport() { | ||
| let visibility = SidebarScrollEdgeVisibility.resolve( | ||
| visibleRect: CGRect(x: 0, y: 0, width: 200, height: 500), | ||
| documentBounds: CGRect(x: 0, y: 0, width: 200, height: 500), | ||
| documentIsFlipped: true | ||
| ) | ||
|
|
||
| XCTAssertEqual(visibility, .hidden) | ||
| } | ||
|
|
||
| func testFlippedDocumentShowsOnlyBottomScrimAtTop() { | ||
| let visibility = SidebarScrollEdgeVisibility.resolve( | ||
| visibleRect: CGRect(x: 0, y: 0, width: 200, height: 400), | ||
| documentBounds: CGRect(x: 0, y: 0, width: 200, height: 1_000), | ||
| documentIsFlipped: true | ||
| ) | ||
|
|
||
| XCTAssertFalse(visibility.showsTopScrim) | ||
| XCTAssertTrue(visibility.showsBottomScrim) | ||
| } | ||
|
|
||
| func testFlippedDocumentShowsBothScrimsInMiddle() { | ||
| let visibility = SidebarScrollEdgeVisibility.resolve( | ||
| visibleRect: CGRect(x: 0, y: 200, width: 200, height: 400), | ||
| documentBounds: CGRect(x: 0, y: 0, width: 200, height: 1_000), | ||
| documentIsFlipped: true | ||
| ) | ||
|
|
||
| XCTAssertTrue(visibility.showsTopScrim) | ||
| XCTAssertTrue(visibility.showsBottomScrim) | ||
| } | ||
|
|
||
| func testFlippedDocumentShowsOnlyTopScrimAtBottom() { | ||
| let visibility = SidebarScrollEdgeVisibility.resolve( | ||
| visibleRect: CGRect(x: 0, y: 600, width: 200, height: 400), | ||
| documentBounds: CGRect(x: 0, y: 0, width: 200, height: 1_000), | ||
| documentIsFlipped: true | ||
| ) | ||
|
|
||
| XCTAssertTrue(visibility.showsTopScrim) | ||
| XCTAssertFalse(visibility.showsBottomScrim) | ||
| } | ||
|
|
||
| func testNonFlippedDocumentUsesOppositeYAxisOrigin() { | ||
| let topVisibility = SidebarScrollEdgeVisibility.resolve( | ||
| visibleRect: CGRect(x: 0, y: 600, width: 200, height: 400), | ||
| documentBounds: CGRect(x: 0, y: 0, width: 200, height: 1_000), | ||
| documentIsFlipped: false | ||
| ) | ||
| let bottomVisibility = SidebarScrollEdgeVisibility.resolve( | ||
| visibleRect: CGRect(x: 0, y: 0, width: 200, height: 400), | ||
| documentBounds: CGRect(x: 0, y: 0, width: 200, height: 1_000), | ||
| documentIsFlipped: false | ||
| ) | ||
|
|
||
| XCTAssertFalse(topVisibility.showsTopScrim) | ||
| XCTAssertTrue(topVisibility.showsBottomScrim) | ||
| XCTAssertTrue(bottomVisibility.showsTopScrim) | ||
| XCTAssertFalse(bottomVisibility.showsBottomScrim) | ||
| } | ||
| } |
There was a problem hiding this comment.
SidebarScrollEdgeVisibilityTests tests SidebarScrollEdgeVisibility.resolve(...), which lives in SidebarScrim.swift and has no connection to window-appearance snapshots. Placing it in WindowAppearanceSnapshotTests.swift grows the file's scope and makes the tests harder to discover. A dedicated SidebarScrollEdgeVisibilityTests.swift (or SidebarScrimTests.swift) alongside the other unit tests would be more consistent with the repository's naming conventions.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
c33906c to
b1b5a5f
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
b1b5a5f to
8c429e2
Compare
lawrencecchen
changed the title
This branch was successfully deployed
1 participant
Summary:
Tests:
git diff --checkxcrun swiftc -parse Sources/SidebarScrim.swift Sources/ContentView.swift cmuxTests/WindowAppearanceSnapshotTests.swift./scripts/reload.sh --tag sidegrad8c429e2d0after rerunning transientweb-typechecksetup failure.Dogfood:
sidegradis built from commit8c429e2d0for visual approval before merge.