Skip to content

fix: always show a thin scrollbar instead of revealing it on hover#864

Merged
qinxuye merged 4 commits into
xorbitsai:mainfrom
bluefish-08:fix/always-visible-scrollbar
Jul 13, 2026
Merged

fix: always show a thin scrollbar instead of revealing it on hover#864
qinxuye merged 4 commits into
xorbitsai:mainfrom
bluefish-08:fix/always-visible-scrollbar

Conversation

@bluefish-08

Copy link
Copy Markdown
Collaborator

Problem

Global .overflow-y-auto containers hid the scrollbar by default and only
revealed it on hover/focus. Moving the mouse into any scrollable area (e.g.
the home page) made the scrollbar flash in and out — visually jarring.

Change

frontend/src/app/globals.css: drop the hover-reveal logic; always show a
thin, subtle scrollbar.

Scope

All .overflow-y-auto scroll regions app-wide.

@XprobeBot XprobeBot added the bug Something isn't working label Jul 12, 2026

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request simplifies the native scrollbar styling in globals.css to make it always visible, thin, and subtle. The reviewer suggested using the specific background-color property instead of the shorthand background property to prevent resetting background-clip and to eliminate redundant declarations in the hover state.

Comment thread frontend/src/app/globals.css

@rogercloud rogercloud left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Summary

This PR fixes a jarring flash-in/flash-out of the scrollbar on global .overflow-y-auto containers, which previously hid the scrollbar and only revealed it on hover/focus/scroll. The change (in frontend/src/app/globals.css only) drops the reveal-on-interaction scheme entirely and always renders a thin, subtle scrollbar app-wide, using scrollbar-width/scrollbar-color for Firefox and ::-webkit-scrollbar* rules for Chrome/Safari. A border: 2px solid transparent + background-clip: content-box inset trick keeps the visible thumb narrow while preserving a wider hit-area.

Approach

Acceptable with reservations. "Always show a thin, subtle scrollbar" is the correct and simplest fix for a flash-on-hover problem — removing the reveal scheme entirely (rather than tuning its debounce/timing) is the right direction, and CSS-only is sufficient for the visual goal. The cross-browser CSS is correct and well-executed.

The reservation is scope: the PR description says it "drops the hover-reveal logic," but it only did so in CSS. See the finding below.

Findings

frontend/src/app/layout.tsx:131-158 — orphaned scroll listener (minor)

The inline <Script id="scrollbar-visibility-controller"> is a capture-phase document scroll listener that toggles a .scrolling class (with a 700ms debounced WeakMap timer) on .overflow-y-auto elements. That class existed solely to drive the old CSS reveal-on-scroll rule, which this PR deletes — post-change, globals.css has zero references to .scrolling. Since layout.tsx is untouched by this PR, the script is now fully orphaned: it still runs on every scroll event app-wide, still allocates and clears timers (classList.add("scrolling") at line 142, classList.remove("scrolling") at line 150), but produces no visible or functional effect.

This is not a functional bug (the class toggle is a harmless no-op), but it is exactly the mechanism this PR set out to retire. Suggest removing the <Script> block as part of this PR so the reveal machinery is retired in full — or, if you'd rather keep this PR CSS-only, explicitly note the script removal as tracked follow-up work.

Re-review: prior gemini-code-assist finding (globals.css:436)

Severity-adjusted — non-blocking. The bot flagged that the background shorthand resets background-clip, making the :hover rule's background-clip: content-box redundant, and suggested switching to background-color. On verification, background-clip is declared after background in the same rule (in both the base thumb rule and the :hover rule), so per the CSS cascade the later same-rule declaration overrides what the earlier shorthand implicitly reset — the code renders correctly as written, and there is no visual or functional bug. The suggested background-color rewrite is a valid, more defensive alternative (it doesn't rely on declaration order) but is optional polish, not a required fix. Not blocking.

Simplification opportunities

Lean already.

Test coverage

Not a gap here. This is presentational global CSS with no existing test file, consistent with how the codebase already handles similar styling — the separate Radix ScrollArea component keeps its own tests and is unaffected by this change.

Recommendation

Approve with a minor suggestion. The CSS fix is correct, cross-browser, and the right approach. The one confirmed finding (the now-orphaned scrollbar-visibility-controller script) is a minor, non-blocking cleanup — nice to fold into this PR since it's the exact machinery being retired, but it can be deferred as tracked follow-up. The prior bot finding is non-blocking. No changes are strictly required to merge.

@bluefish-08

Copy link
Copy Markdown
Collaborator Author

Thanks for the review. Addressed the confirmed finding:

  • Removed the orphaned scrollbar-visibility-controller <Script> in layout.tsx (and its now-unused next/script import) — the reveal-on-scroll machinery is now retired in full, both CSS and JS.

The prior gemini background-clip note is already handled in an earlier commit (switched to background-color), so no further change there.

@rogercloud rogercloud left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-review (round 2)

What changed since the last review

Two commits landed since the previous round:

  • 987776b (reviewed previously): switched .overflow-y-auto::-webkit-scrollbar-thumb and its :hover rule from the background shorthand to the background-color longhand.
  • c6524fe (new): removed the entire orphaned <Script id="scrollbar-visibility-controller"> block from frontend/src/app/layout.tsx, along with the now-unused import Script from "next/script".

Verdict: SOUND

The change is now a complete, coherent retirement of the old hover-reveal scrollbar mechanism: both the CSS (now an always-visible thin scrollbar) and the JS scroll-listener/timer that fed it are removed together, leaving no orphaned code, no dangling imports, and no leftover .scrolling references anywhere in frontend/src. The background-color longhand is correct — it doesn't reset background-clip/border the way the shorthand would, so dropping the redundant background-clip: content-box from the :hover rule is safe rather than a regression.

Prior findings

  1. FIXED — Orphaned scrollbar-visibility-controller Script in layout.tsx: the <Script> block and its import Script from "next/script" are both gone; a grep of frontend/src for next/script and scrolling turns up no remaining references.
  2. FIXEDbackground shorthand → background-color: both the base thumb rule and the :hover rule now use background-color, and the redundant background-clip: content-box in :hover was correctly dropped with no visual change.

Simplification opportunities

  • frontend/src/app/globals.css L419: yagniheight: 8px on .overflow-y-auto::-webkit-scrollbar sets the horizontal-scrollbar thickness, but .overflow-y-auto is only ever applied to vertically-scrolling elements (confirmed across ~67 usages in frontend/src); a horizontal bar can never render, so this is dead/speculative styling. Drop the height: 8px.

net: -1 line possible

Recommendation

Approve — no blocking issues. The height: 8px removal is an optional nice-to-have and can be picked up now or left for later.

@bluefish-08

Copy link
Copy Markdown
Collaborator Author

Picked up the optional simplification too: dropped the unused height: 8px on .overflow-y-auto::-webkit-scrollbar (feb1e8f3), since these containers only ever scroll vertically.

@rogercloud rogercloud left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review Round 3

What changed since last review: One new commit — feb1e8f ("fix: drop unused horizontal scrollbar height on vertical-only containers"), a single-line removal of height: 8px from .overflow-y-auto::-webkit-scrollbar in frontend/src/app/globals.css. Nothing else in the rule or file changed.

Design verdict: SOUND

This is now the correct minimal fix, and the PR has fully converged. Tracing the arc: the original hover-reveal scrollbar (JS scroll listener with WeakMap timers plus three parallel CSS state selectors) was over-engineered for the goal and produced the flash; it was replaced with an always-visible native CSS scrollbar (killing the whole problem class and the runtime cost), the inset-thumb background-clip: content-box trick was corrected, and this final commit drops the last speculative property. The resulting globals.css block is complete and self-contained (Firefox scrollbar-width/color + WebKit pseudo-elements + thumb hover-darken) with zero orphaned JS, zero dead selectors, and zero speculative properties. width: 8px alone is correct since .overflow-y-auto only ever governs vertical scroll.

Prior findings

  • FIXEDheight: 8px on .overflow-y-auto::-webkit-scrollbar was dead/speculative styling (these elements never scroll horizontally). The current rule now carries only width: 8px; the thumb's inset trick (border + background-clip: content-box) is independent of the removed property and unaffected.

Simplification opportunities

Lean already. The diff has been through two rounds of cleanup (dead JS removed earlier, unused CSS height removed here) — nothing further to simplify.

Recommendation

Approve. No blocking issues, no remaining action items — ready to merge.

@rogercloud rogercloud left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving after 3 rounds of review — all findings addressed, design is sound, and the PR has converged to a clean, minimal fix. Ready to merge.

@qinxuye qinxuye merged commit 2ca403c into xorbitsai:main Jul 13, 2026
10 checks passed
@bluefish-08 bluefish-08 deleted the fix/always-visible-scrollbar branch July 13, 2026 03:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants