Repository navigation
Re-assert sidebar overlay scrollers when the preferred scroller style changes (#3241 sidebar scope) - #5955
Merged
Conversation
…red-style change AppKit resets every NSScrollView's scrollerStyle to the new system preference when NSScroller.preferredScrollerStyleDidChangeNotification fires (mouse connect/disconnect, System Settings "Show scroll bars"). That clobbers the sidebar's forced overlay configuration with a legacy, space-reserving scrollbar until some unrelated SwiftUI re-render happens to re-run the resolver (#3241 reopen, sidebar scope; browser-pane scope is PR #5847). Moves SidebarScrollViewResolverView from ContentView.swift into SidebarScrollViewConfigurator.swift unchanged (internal visibility) so the test can exercise it; no behavior change in this commit, so the new test fails: posting the style-change notification does not re-resolve. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
… changes SidebarScrollViewResolverView now observes NSScroller.preferredScrollerStyleDidChangeNotification and re-resolves, re-applying SidebarScrollViewConfigurator's overlay configuration. The async main hop in resolveScrollView() guarantees the re-apply runs after AppKit's own synchronous per-scroll-view style reset, regardless of observer registration order — mirroring the terminal's existing handlePreferredScrollerStyleChange treatment of the same notification. Both sidebar resolver call sites share this path, and the configurator's guarded writes keep the re-apply a no-op when nothing changed, so the knob-fade contract from the #3241 follow-up is preserved. Fixes the sidebar scope of the #3241 reopen (browser panes: PR #5847). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughExtracts SidebarScrollViewResolverView into a separate ChangesSidebar Scroller Resolver Module
Sequence DiagramsequenceDiagram
participant SwiftUI as SidebarEmptyArea (SwiftUI)
participant Resolver as SidebarScrollViewResolverView
participant Notif as NotificationCenter
participant Scroll as NSScrollView
SwiftUI->>Resolver: instantiate in view hierarchy
Resolver->>Notif: register preferredScrollerStyleDidChangeNotification (main queue)
Resolver->>Scroll: viewDidMoveToSuperview/viewDidMoveToWindow -> resolveScrollView()
Resolver->>Resolver: async main-actor hop
Resolver->>Scroll: onResolve(enclosingScrollView)
Notif->>Resolver: preferredScrollerStyleDidChangeNotification
Resolver->>Resolver: resolveScrollView() (re-run)
Resolver->>Scroll: onResolve(re-resolved NSScrollView)
Resolver->>Notif: deinit removes observer
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRsPoem
🚥 Pre-merge checks | ✅ 20 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (20 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. Comment |
Contributor
Greptile SummaryThis PR fixes the sidebar scope of issue #3241 by re-asserting the forced overlay scroller configuration whenever
Confidence Score: 5/5Safe to merge — targeted, well-tested re-assertion of an already-established overlay-scroller invariant using the same notification-observer pattern already present in GhosttyTerminalView. The observer registration is correct (queue: .main, weak capture, deinit cleanup), the deferred Task { @mainactor } hop preserves the post-AppKit-reset ordering guarantee that the DispatchQueue.main.async it replaced provided, and the guarded writes in SidebarScrollViewConfigurator keep re-application idempotent. The regression test is deterministic and exercises the full notification→re-resolve→re-assert path without wall-clock sleeps. No logic errors, no actor isolation issues, and no production patterns to flag. No files require special attention. Important Files Changed
Sequence DiagramsequenceDiagram
participant AppKit
participant NotificationCenter
participant SidebarScrollViewResolverView
participant MainActor as Main Actor (Task)
participant SidebarScrollViewConfigurator
Note over AppKit: Mouse connect/disconnect or System Settings scroller change
AppKit->>AppKit: "Reset NSScrollView.scrollerStyle = .legacy (sync, per-scrollview)"
AppKit->>NotificationCenter: post preferredScrollerStyleDidChangeNotification
NotificationCenter->>SidebarScrollViewResolverView: observer block (queue: .main)
SidebarScrollViewResolverView->>MainActor: "Task { @MainActor } enqueued after current sync work"
Note over MainActor: Runs after AppKit synchronous per-scroll-view reset completes
MainActor->>SidebarScrollViewResolverView: enclosingScrollView
SidebarScrollViewResolverView->>SidebarScrollViewConfigurator: apply(to: scrollView)
SidebarScrollViewConfigurator->>AppKit: "scrollerStyle = .overlay (guarded write, only if changed)"
Reviews (3): Last reviewed commit: "Express the resolver's deferred hop with..." | Re-trigger Greptile |
Comment on lines
+45
to
+51
| scrollerStyleObserver = NotificationCenter.default.addObserver( | ||
| forName: NSScroller.preferredScrollerStyleDidChangeNotification, | ||
| object: nil, | ||
| queue: nil | ||
| ) { [weak self] _ in | ||
| self?.resolveScrollView() | ||
| } |
Contributor
There was a problem hiding this comment.
queue: nil calls @MainActor-isolated method from background thread
NSScroller.preferredScrollerStyleDidChangeNotification is not guaranteed to be posted on the main thread — the analogous handler in GhosttyTerminalView.swift guards against this explicitly with Thread.isMainThread. With queue: nil, if AppKit fires the notification from a background thread the closure calls self?.resolveScrollView() off the main actor. At runtime the inner DispatchQueue.main.async keeps things correct, but Swift 6 strict concurrency will reject a call to an @MainActor-isolated NSView method from a non-isolated closure. Using queue: .main is semantically equivalent (the DispatchQueue.main.async inside resolveScrollView still provides the post-AppKit-reset ordering guarantee documented in the comment) and is Swift 6-safe.
Contributor
Author
There was a problem hiding this comment.
Fixed in 9ef8e6d — the observer now uses queue: .main so the block stays on the main thread for any posting thread. A main-thread post (AppKit's case) still runs it synchronously in place, so the re-apply ordering after AppKit's per-scroll-view reset is unchanged. (The code moved to Sources/SidebarScrollViewResolverView.swift in 21bcda4.)
— Claude Code
Autoreview: the extraction from ContentView.swift should follow the one-major-type-per-file architecture rule rather than sharing SidebarScrollViewConfigurator.swift with the configurator enum. No behavior change; wires the new file into the app target. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Greptile: queue: nil runs the observer block on the posting thread, so a background-thread post would call the NSView method off-main. Use .main — a main-thread post (AppKit's case) still executes the block synchronously in place, so the re-apply ordering after AppKit's own per-scroll-view reset is unchanged. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Aziz concurrency policy (autoreview): main-thread hops in new files should use @mainactor structured concurrency, not DispatchQueue.main .async — same modernization as the BrowserPanel download path (#5938). The Task { @mainactor } hop preserves the load-bearing ordering: it is enqueued synchronously and runs after the current runloop callout, i.e. after AppKit's own per-scroll-view scroller-style reset. The test drain switches from a main-queue round trip to a bounded main-actor yield loop so it makes no cross-mechanism FIFO assumptions. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This branch was successfully deployed
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
Sidebar scope of the #3241 reopen ("Verify the merged sidebar fix (#4767) actually holds in legacy mode — AppKit can re-tile/override on style changes"). The browser-pane / process-wide scope is handled in the parallel PR #5847, whose description points at this PR for the sidebar scroller config.
Mechanism
AppKit resets every
NSScrollView'sscrollerStyleto the new system preference whenNSScroller.preferredScrollerStyleDidChangeNotificationfires — a mouse is connected/disconnected, or System Settings → Appearance → "Show scroll bars" changes. That clobbers the sidebar's forced.overlayconfiguration (from #4767 /SidebarScrollViewConfigurator) with a legacy, space-reserving scrollbar track. The config is only re-applied when some unrelated SwiftUI re-render happens to re-runSidebarScrollViewResolver— with an idle sidebar the legacy track persists indefinitely.The terminal already handles exactly this notification (
handlePreferredScrollerStyleChangeinGhosttyTerminalView.swift, the #2997-class fix); the sidebar did not.Fix
SidebarScrollViewResolverView(moved unchanged fromContentView.swiftintoSidebarScrollViewConfigurator.swiftfor testability) now observesNSScroller.preferredScrollerStyleDidChangeNotificationand re-resolves. The existing async main hop inresolveScrollView()guarantees the re-apply runs after AppKit's own synchronous per-scroll-view reset, regardless of observer registration order.dragAutoScrollController.attachre-invocation is an already-established repeated path and idempotent.reapplyToConfiguredScrollViewWritesNothingtest still enforces it).Regression test (two-commit red/green)
resolverReassertsOverlayConfigurationAfterPreferredScrollerStyleChange, which simulates AppKit's reset (scrollerStyle = .legacy), posts the notification, and expects a re-resolve +.overlayre-asserted. Fails without the observer — the re-resolve-count assertion makes the red deterministic even on runners whose preferred style is already overlay.Test lives in the already-wired
cmuxTests/SidebarScrollViewConfiguratorTests.swift(no pbxproj changes needed).Localization audit
No user-facing strings added or changed — AppKit scroller configuration behavior only. Nothing to localize.
Part of #3241.
🤖 Generated with Claude Code
Need help on this PR? Tag
/codesmithwith what you need. Autofix is disabled.Summary by cubic
Keeps the sidebar’s overlay scrollbar style when the system scroller preference changes by re-resolving after AppKit resets scroll views. Fixes the legacy track sticking in idle sidebars (sidebar scope of #3241).
NSScroller.preferredScrollerStyleDidChangeNotificationon the main queue inSidebarScrollViewResolverViewand trigger a re-resolve.Task { @MainActor }) so it runs after AppKit’s reset; remains idempotent via guarded writes.SidebarScrollViewResolverViewintoSidebarScrollViewResolverView.swiftand added a deterministic regression test.Written for commit 8d60172. Summary will update on new commits.
Summary by CodeRabbit
New Features
Tests
Refactor