Merged
Conversation
Wide, translucent, full-length overlay scrollbars replacing the 6px
WebKit rail that users couldn't reliably grab. Click the track to
page-animate toward the click, drag the thumb, no layout shift, Firefox
parity.
Wraps plan-mode scroll containers (main viewer, annotation panel,
sidebar, settings, export modal) in a new <OverlayScrollArea> component
backed by overlayscrollbars-react. The library handles pointer capture,
click-to-jump, hover reveal, auto-hide, RTL, touch, momentum, and
cross-browser consistency. ClickScrollPlugin registered explicitly so
`clickScroll: true` actually pages (otherwise it silently no-ops).
Plumbing:
- New ScrollViewportContext + useScrollViewport() hook so descendants
(TableOfContents, PinpointOverlay, Viewer sticky observer) can reach
the real scrolling element instead of document.querySelector('main'),
which no longer returns the scroll node after wrapping.
- New useOverlayViewport() hook — canonical ref+state+callback pattern
bridging the library viewport into components that need both
imperative access and effect re-runs when the viewport mounts.
- useActiveSection gains an optional scrollElement arg so its
IntersectionObserver root re-attaches when the viewport becomes
available (ref mutations don't retrigger effects).
Theme:
- New .os-theme-plannotator tokens sourced from existing theme CSS
variables (translucent muted-foreground for handle, transparent track
at rest). 10px at rest, 14px on hover. Color + width transitions
only — deliberately does not reintroduce transform/opacity in global
transitions, preserving the abc952f scroll-jank fix.
- Firefox `scrollbar-width: thin` + `scrollbar-color` fallback for
unwrapped surfaces (micro-scrollers, future dockview).
- print.css hides .os-scrollbar during print.
ResizeHandle (#354 regression guard):
- `side="right"` touch area retuned from `-right-3 left-0` to
`-right-3 left-3` to clear the 14px hover scrollbar. Load-bearing
comment added explicitly warning against simplification because #354
has already regressed twice.
Intentionally left native: max-h-24 inline scrollers
(EditorAnnotationCard, AgentsTab, ThemeTab), dropdown menus — a 14px
overlay scrollbar would dominate those UI elements.
Fixes #354
Follow-up to #359, #465 (both fixed #354, which kept regressing)
Preserves #253 bidirectional annotation scroll
Preserves #452 file-switch reset (plan mode portion)
For provenance purposes, this commit was AI assisted.
Wrap the trivial code-review scroll containers in <OverlayScrollArea>: FileTree, ReviewSidebar content area, AITab chat history, PRCommentsTab timeline, and the ReviewPRSummary / ReviewPRChecks dock panels. None of these components read scrollTop / scrollHeight / scrollLeft directly — all descendant queries use containerRef.current.querySelector and all scroll-to-target calls use element.scrollIntoView, which walks up to the nearest scrollable ancestor (now the library viewport). No plumbing changes required. AITab was originally scoped to Commit B but an audit of its scroll effects (jump-to-question + auto-scroll-to-bottom) confirmed it only uses descendant scrollIntoView, so it ships here. DiffViewer and LiveLogViewer follow in a separate commit — they programmatically read/write scrollTop and need explicit viewport plumbing via useOverlayViewport. For provenance purposes, this commit was AI assisted.
Wrap DiffViewer's main scroll container and LiveLogViewer's log pane in <OverlayScrollArea>, plumbing containerRef through useOverlayViewport so imperative scroll reads/writes and IntersectionObserver roots land on the real library viewport, not the OverlayScrollArea host. DiffViewer: - `previousScrollFilePathRef` guard added: the file-switch reset (#452) now only advances the tracking ref once the scroll actually executed, closing a race where a file switch landing before the library viewport attached would leave the ref stale while the scrollTo silently no-oped. - `viewport` added to every effect dep that reads containerRef.current (file-switch reset, annotation scroll, search-highlight apply + swap, scroll-to-match) so they re-run when the viewport mounts. Without this they'd silently no-op on first paint. - Split-view sync via @pierre/diffs unaffected — its ScrollSyncManager attaches scroll listeners to its own internal codeDeletions / codeAdditions elements inside the content, not the outer scroller. LiveLogViewer: - React onScroll replaced with a native addEventListener('scroll', ...) inside a useEffect keyed on `viewport` because React's onScroll doesn't bubble across the library's wrapper layers. - Follow-tail heuristic (`scrollHeight - scrollTop - clientHeight < 40`) and the auto-scroll-to-bottom assignment both preserved verbatim — only the ref target changed from the raw div to the library viewport. Preserves #452 file-switch reset Preserves #253 bidirectional annotation scroll (diff side) For provenance purposes, this commit was AI assisted.
Two P1 bugs from the review, plus four correctness/cleanup items. P1: OverlayScrollbars viewport was never delivered to consumers. handleOsRef was reading osInstance() synchronously from the React ref callback, but `defer: true` queues the library's initialization for a later frame — at ref-callback time, the internal instance ref is still null. With a stable useCallback the ref never re-fires, so onViewportReady was never called with a real viewport. Every consumer of useOverlayViewport/useScrollViewport stayed permanently null: useActiveSection, TableOfContents.handleNavigate, Viewer sticky detection, PinpointOverlay, DiffViewer file-switch reset, LiveLogViewer follow-tail — all silently no-ops. Fix: deliver the viewport via the library's own `events.initialized` and `events.destroyed` callbacks, which fire exactly when elements are ready and when they're torn down. `getViewport()` prefers the tracked viewport ref over the imperative osInstance() path so late callers still work. P1: right-side ResizeHandle touch area was 0px wide. Touch area is an absolute-positioned child of a w-0 parent, so actual width = parent - left - right. With side='right' I'd set `left-3 -right-3`, which evaluates to `0 - 12 - (-12) = 0`. The annotation panel resize handle in plan mode and both right handles in code review had no draggable region. The visible 4px track has no event handlers, so the only affordance was cursor-style feedback — drag did nothing. Fix: revert to `left-0 -right-3` (12px wide, entirely right of the boundary, no left encroachment into the adjacent scrollbar zone — which was the original correct value before this branch). Rewrote the comment to explain the geometry trap instead of protecting the value that broke it. Additional fixes: - OverlayScrollArea: prefers-reduced-motion now reactive via useSyncExternalStore — OS toggle mid-session propagates to mounted instances instead of staying frozen at mount-time snapshot. - OverlayScrollArea: `ref as never` replaced with a narrow cast to `React.RefCallback<OverlayScrollbarsComponentRef<'div'>>` so future signature changes get type feedback. - PinpointOverlay: window resize listener moved above the scroll viewport guard so it attaches unconditionally. Scroll listener still requires the viewport (correct). Old code always registered resize on window; new code was accidentally skipping it when viewport was null. - TableOfContents: `className || default` changed to `className ?? default`. An explicit empty string from a caller (SidebarContainer passing className="" now that it wraps us in an OverlayScrollArea) should mean "no container styling", not "use the default". The old || operator treated "" as falsy and applied the default, leaving dead overflow-y-auto and unintended backdrop-blur on the nav. - useOverlayViewport: removed redundant double cast and `?? null` no-op in the ref setter. For provenance purposes, this commit was AI assisted.
When <main> is wrapped in OverlayScrollArea, the library adds its own
attribute-selector CSS rules: `[data-overlayscrollbars~="host"]` gets
`overflow: hidden !important` and `[data-overlayscrollbars-viewport]`
gets `overflow-x/y: hidden` (or scroll) with fixed viewport heights.
These beat our existing `main { overflow: visible !important }` print
override on specificity — attribute selectors outrank tag selectors
even when both use `!important`. Result: long plans printed only the
currently-visible viewport instead of flowing across pages.
Fix: add a print-scoped override that targets the library's attribute
selectors directly, setting overflow:visible, height:auto,
max-height:none, and display:block to defeat both the overflow clip
and the flex layout the library applies to host/padding wrappers.
Verified by printing a multi-page plan in the dev server.
For provenance purposes, this commit was AI assisted.
Switch autoHide from 'leave' to 'never'. The previous behavior faded the scrollbar 800ms after the pointer left the scroll host, which felt broken in a common interaction pattern: click a TOC entry (pointer in the sidebar) → trackpad-scroll (pointer still in sidebar) → 800ms idle → scrollbar disappears. User had to hover the right edge to bring it back on every interaction. Persistent visibility matches the pattern used by every editor-class technical app (VS Code, Zed, JetBrains, Xcode, Sublime) where the scrollbar is both a position indicator and a targeting surface for click-to-jump. Overlay scrollbars cost zero layout space, so "always visible" has no downside. Also removes the dead `.os-theme-plannotator .os-scrollbar` selector from the reduced-motion block. The library applies the theme class directly to the .os-scrollbar element itself (verified in the library runtime source, not just CSS), so a descendant selector with a space matches nothing. The track and handle selectors in the same block work correctly (they really are descendants) and are preserved. The component's own prefers-reduced-motion hook and helpers are now unused (autoHide is unconditionally 'never') and removed. Reduced motion is still honored for the hover color + width transitions on track/handle via the CSS @media (prefers-reduced-motion: reduce) block in theme.css, which the browser evaluates independently. For provenance purposes, this commit was AI assisted.
…r behavior The component header still described the old autoHide:'leave' behavior and implied the reduced-motion branch was in the component itself. Neither is true after cf137bf. Rewrite the jsdoc to describe the current behavior: always visible, no fade, reduced-motion handled via a CSS media query in theme.css rather than a React branch. For provenance purposes, this commit was AI assisted.
…ines) When pierre/diffs expanded context lines on a file whose diff previously fit inside the viewport, the scrollbar stayed hidden until the user manually scrolled or dragged the split-ratio handle to force a layout recalculation. Root cause: OverlayScrollbars' internal content observer doesn't see the mutation because pierre/diffs renders inside a shadow DOM, and MutationObserver does not pierce shadow DOM by default. The library's own host-level size observer doesn't help either — the host (our <main> / flex-1 container) has a fixed layout size that doesn't change when content grows. Fix: track the OverlayScrollbars instance in state and attach a ResizeObserver to the viewport's first element child in a useEffect. When the content's layout box grows — which happens even when the growth originates inside a shadow tree, because layout size propagates from shadow content to the shadow host — the observer fires and calls `instance.update(true)` to recompute scrollbar visibility. The call is debounced through requestAnimationFrame so the browser commits the new layout before OverlayScrollbars reads dimensions. Verified manually in the compiled binary against PR #509 itself: opening a small file, clicking pierre's expand-lines control, now reveals the scrollbar immediately. No regression observed in normal scroll / click-to-jump / file-switch / annotation-click paths. For provenance purposes, this commit was AI assisted.
Add the content-resize auto-recompute behavior to OverlayScrollArea's jsdoc header so the "what does this component do" summary is complete. The inline comment on the effect already explains the mechanism; this just surfaces it at the top. For provenance purposes, this commit was AI assisted.
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
Wide, translucent, full-length overlay scrollbars across plan mode and code review, replacing the 6px WebKit rail that users couldn't reliably click or grab. Click the track to page-animate toward the click, drag the thumb smoothly, no layout shift, Firefox parity, respects
prefers-reduced-motion, disappears in print.Backed by
overlayscrollbars-reactwrapped in a new<OverlayScrollArea>component. The library handles pointer capture, click-to-jump, hover reveal, auto-hide, RTL, touch, and momentum — we keep native scroll semantics on the viewport element so existingscrollIntoView/scrollTocalls still work.The marker layer (annotation ticks, diff-hunk ticks, search hits on the track) is deliberately deferred as a downstream feature; this PR is the foundation.
What ships
New
packages/ui/components/OverlayScrollArea.tsx— wrapper with a memoized options object, imperativegetViewport()handle,onViewportReadycallback, forwards all standard HTML props, honorsprefers-reduced-motion.packages/ui/hooks/useScrollViewport.ts— React context + hook so descendants can reach the real scrolling element withoutdocument.querySelector('main')(which no longer returns the scroll node after wrapping).packages/ui/hooks/useOverlayViewport.ts— canonical ref+state+callback pattern for components that need both imperative scroll access and effect re-runs when the library viewport mounts.Wrapped scroll containers
Plan mode:
packages/editor/App.tsx)Code review:
containerRef→ library viewport plumbing for file-switch reset, annotation scroll, search highlight, search scroll)Theme
.os-theme-plannotatorCSS variable set sourced from existing theme tokens — 10px at rest, 14px on hover, translucentmuted-foregroundhandle, transparent track at rest. Color + width transitions only — deliberately does not reintroducetransform/opacityin global transitions, preserving the abc952f scroll-jank fix.scrollbar-width: thin+scrollbar-colorfallback for unwrapped surfaces.print.csshides.os-scrollbarduring print.Not touched (intentionally native):
max-h-24/max-h-48inline scrollers (annotation original-text preview, agent prompt disclosure boxes, theme grid), the AskAI suggestion dropdown, the PRCommentsTab horizontal author filter, dockview's own tab-strip scrollers. A 14px overlay scrollbar would dominate these.Related issues and PRs
ResizeHandleside="right"touch area retune (-right-3 left-0→-right-3 left-3) clears it on both axes. A load-bearing comment is added toResizeHandle.tsxthat explicitly warns against simplification, because [BUG] Can't grab scrollbar #354 has regressed twice (first in fix: prevent resize handle from covering scrollbar #359's bundled feat: plan archive browser #369 simplification, then fixed again in fix(ui): right resize handle no longer covers scrollbar #465).scrollIntoViewon descendants walks up to the library viewport unchanged.DiffViewer. Additionally closes a race condition where a file switch landing before the library viewport attached would leavepreviousScrollFilePathRefstale while thescrollTosilently no-oped. The guard now only advances the tracking ref once the scroll actually executed.Architectural notes
ClickScrollPluginmust be registered explicitly. In overlayscrollbars v2,clickScroll: 'instant'works out of the box, butclickScroll: true(page-animate toward the click — the "like a website" behavior) silently no-ops unlessOverlayScrollbars.plugin(ClickScrollPlugin)runs at module load. Registered once inOverlayScrollArea.tsx.@pierre/diffssplit-view sync is unaffected. Verified pre-flight: itsScrollSyncManagerattaches scroll listeners to its own internalcodeDeletions/codeAdditionselements inside the content, not the outer container we wrap.overlayscrollbars+ the React wrapper. Acceptable given we already shiphighlight.jsanddockview-react.Test plan
ResizeHandlegrabs cleanly on bothside="left"andside="right"at both 10px rest and 14px hover widths