fix(ui): unify native-style scrollbar visibility across platforms (#97)#105
Merged
Conversation
Contributor
Coverage Report
File Coverage
|
||||||||||||||||||||||||||||||||||||||
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #105 +/- ##
=====================================
Coverage 77.3% 77.3%
=====================================
Files 126 126
Lines 6266 6266
Branches 1984 2022 +38
=====================================
Hits 4846 4846
Misses 1376 1376
Partials 44 44
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Contributor
Greptile SummaryThis PR stabilizes scrollbar E2E coverage and test fixtures. The main changes are:
Confidence Score: 5/5Safe to merge with minimal risk. The changes are test-focused plus a non-user-visible test id, and the mutable fixture helpers are scoped to the Playwright mock. No current bugs were found in the changed paths. No files require special attention.
 T-Rex LogsWhat T-Rex did
Important Files Changed
Sequence Diagram%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
participant Spec as scrollbar.spec.ts
participant Helper as tauriMock helper
participant Mock as window.__OPENKARA_E2E__
participant App as React app stores/components
participant BC as BroadcastChannel
Spec->>Helper: setMockSongs(...) / setMockLyrics(...)
Helper->>Mock: page.evaluate override payload
Mock->>Mock: replace mockSongs / mockLyrics
Spec->>BC: post library or queue sync event
BC->>App: notify store subscribers
App->>Mock: invoke get_library / fetch_lyrics
Mock-->>App: return cloned current fixture payload
App-->>Spec: render overflowing scroll surfaces
Spec->>App: assert overflow and scrollbar contract
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
sequenceDiagram
participant Spec as scrollbar.spec.ts
participant Helper as tauriMock helper
participant Mock as window.__OPENKARA_E2E__
participant App as React app stores/components
participant BC as BroadcastChannel
Spec->>Helper: setMockSongs(...) / setMockLyrics(...)
Helper->>Mock: page.evaluate override payload
Mock->>Mock: replace mockSongs / mockLyrics
Spec->>BC: post library or queue sync event
BC->>App: notify store subscribers
App->>Mock: invoke get_library / fetch_lyrics
Mock-->>App: return cloned current fixture payload
App-->>Spec: render overflowing scroll surfaces
Spec->>App: assert overflow and scrollbar contract
Reviews (3): Last reviewed commit: "fix(e2e): avoid init-script race in scro..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Owner
Author
Maintainer acceptance — merge slot 2BLOCKED — rebase and required cross-platform UI acceptance pending. The current implementation appears to contain the forced-colors width reset, so I am not repeating the older stale thread. However, it is not ready to merge:
After rebase, supply the missing automated/manual evidence. This is a small CSS-only change, but it changes native platform behavior and must be accepted visually before #111 builds on it. |
7 tasks
thedavidweng
force-pushed
the
feat/scrollbar-visibility-97
branch
from
July 14, 2026 00:47
65755e2 to
a9248fa
Compare
Owner
Author
Re-review: automated checks pass, but required physical-platform acceptance is still missingThe code-level scrollbar contract is promising, but this PR cannot declare the manual matrix out of scope: Issue #97 explicitly makes it an acceptance requirement. No further speculative CSS refactor is requested unless the matrix finds a defect. Complete the following packaged-build acceptance and attach the results to this PR:
Run the existing Chromium/WebKit E2E suites as well. Re-request review only after the three-platform matrix is attached. |
6 tasks
thedavidweng
force-pushed
the
feat/scrollbar-visibility-97
branch
from
July 15, 2026 04:27
a9248fa to
ba3dfb0
Compare
Owner
Author
Re-review response — scrollbar visibility (#97)Commit
Changed files (vs
|
| File | Lines | Purpose |
|---|---|---|
src/styles/globals.css |
+59/-10 | Platform-scoped scrollbar CSS contract (mac auto / desktop thin / forced-colors reset) |
tests/contract/scrollbar-contract.test.ts |
+133 | Source-level contract test for CSS token cascade |
tests/e2e/scrollbar.spec.ts |
+245/-3 | E2E real-overflow tests for all 4 surfaces (library, lyrics, queue, settings) + platform contract assertions |
tests/e2e/fixtures/tauri-mock.ts |
+30/-16 | Mutable mockSongs / mockLyrics with setMockSongs / setMockLyrics helpers |
tests/e2e/fixtures/base-test.ts |
+8 | Expose setMockSongs / setMockLyrics via tauriMock fixture |
CHANGELOG.md |
+1 | Entry for scrollbar fix |
E2E test coverage — all 4 scroll surfaces with real overflow
The scrollbar.spec.ts suite now exercises every required scroll surface with enough content to produce real scrollHeight > clientHeight overflow, programmatic scroll, and platform-contract inheritance:
| Test | Surface | Overflow method | Assertions |
|---|---|---|---|
settings scroll surface produces real overflow… |
Settings | Toggle settings overlay (inherent overflow) | scrollHeight > clientHeight, scrollTop > 0, desktop thin contract (Chromium) |
library song list produces real overflow… |
Library | 60 fixture songs via addInitScript → setMockSongs |
scrollHeight > clientHeight, scrollTop > 0, desktop thin contract (Chromium) |
lyrics panel produces real overflow… |
Lyrics | 60 synced lyric lines via setMockLyrics + play song |
scrollHeight > clientHeight, scrollTop > 0 (same-evaluate to survive auto-follow rAF), desktop thin contract (Chromium) |
queue panel produces real overflow… |
Queue | 40 song IDs via BroadcastChannel("openkara.queue") |
scrollHeight > clientHeight, scrollTop > 0, desktop thin contract (Chromium) |
desktop marker applies thin semantic scrollbar… |
Root | Platform marker desktop |
scrollbar-width: thin, scrollbar-color != auto (Chromium); color-scheme: dark (WebKit) |
mac marker keeps native auto scrollbar… |
Root | Platform marker mac |
scrollbar-width: auto, color-scheme: dark |
forced colors return scrollbar control to the system |
Root | @media (forced-colors: active) |
scrollbar-width: auto (Chromium-only) |
Verification results
| Check | Result |
|---|---|
pnpm lint |
0 warnings, 0 errors |
pnpm build (tsc + vite) |
Passed |
pnpm test (vitest) |
1310 passed (135 files) |
pnpm test:coverage |
76.66% stmt / 70.4% branch / 67.74% fn / 77.01% line |
pnpm check:patch-coverage |
No instrumented src lines in diff |
pnpm check:i18n |
All locales match en.json |
cargo test -q (src-tauri) |
411 passed (0 failed) |
cargo fmt --check |
Clean |
pnpm playwright test (Chromium) |
43 passed (7 scrollbar tests) |
pnpm playwright test --config=playwright.webkit.config.ts (WebKit) |
42 passed, 1 skipped (6 scrollbar tests, 1 Chromium-only skipped) |
macOS packaged app
pnpm tauri buildproducedOpenKara.appatsrc-tauri/target/release/bundle/macos/OpenKara.app(DMG bundling failed due to abundle_dmg.shtooling issue unrelated to this PR).- App launches and renders correctly on macOS (WKWebView). Verified via
screencaptureand OCR: the library loads with 111 songs, the sidebar shows playlists, and the playback bar is visible. - macOS scrollbar preference is
AppleShowScrollBars = WhenScrolling(default), which is the native overlay behavior this PR preserves viascrollbar-width: autoandscrollbar-color: autoon themacplatform marker.
Platform matrix
| Platform | Build | E2E (Chromium) | E2E (WebKit) | Physical visual acceptance |
|---|---|---|---|---|
| macOS 15 (aarch64) | ✅ .app builds & launches |
✅ 43/43 | ✅ 42/43 (1 skip) | |
| Windows | — | — | — | ❌ Blocked — no machine |
| Linux | WebKitGTK | — | — | ❌ Blocked — no machine |
What changed since last review
- Rebased on latest
main(brings inf4c4ce6execution-provider fix). - Enhanced
scrollbar.spec.tswith 3 new E2E tests covering library, lyrics, and queue surfaces with real overflow — the previous suite only tested Settings. - Made
tauri-mock.tsmutable —mockSongsandmockLyricscan now be overridden per-test viasetMockSongs/setMockLyricshelpers exposed through thetauriMockfixture. - Fixed lyrics auto-follow test race —
scrollTopis set and read in the sameevaluatecall so the lyrics auto-follow rAF loop cannot reset the position between two round-trips.
Owner
Author
BLOCKED: physical-platform visual acceptance requiredThe automated verification is complete (all E2E + unit + Rust tests pass on Chromium and WebKit, covering all 4 scroll surfaces with real overflow). However, the physical-platform visual acceptance required by the re-review cannot be fully completed by the agent. What was verified on macOS
What could not be verified on macOS
Uncompleted platform matrix
What a human reviewer needs to doOn each platform:
|
thedavidweng
force-pushed
the
feat/scrollbar-visibility-97
branch
2 times, most recently
from
July 16, 2026 03:44
c89f8c2 to
8fd568c
Compare
Owner
Author
|
@greptile review |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Owner
Author
|
@greptile review |
Contributor
Linux VM visual acceptance (Cursor Cloud)Commit: Automated E2E (official
|
| Engine | Result |
|---|---|
Chromium (pnpm exec playwright test tests/e2e/scrollbar.spec.ts) |
7 passed |
WebKit (pnpm test:ui-smoke:webkit / scrollbar.spec) |
6 passed, 1 skipped (forced-colors Chromium-only) |
All four overflow surfaces asserted: library (60 songs), lyrics (60 lines), queue (40 items), settings.
Measured contract (Chromium desktop marker)
| Surface | scrollbar-width | scrollbar-color | overflow |
|---|---|---|---|
| library | thin |
rgb(110,110,115) transparent |
4316 > 62, scrollTop=420 |
| settings | thin |
same | 1395 > 286, scrollTop=200 |
| lyrics | thin |
same | 3663 > 286 |
| queue | thin |
same | 1923 > 187, scrollTop=220 |
| mac marker | auto / auto |
— | color-scheme dark |
| forced-colors | auto / auto |
— | system control restored |
WebKit matches the same thin desktop inheritance where standard properties are exposed; mac marker stays auto.
Screenshots
Library (desktop, scrolled)
library desktop scrolled
Settings (desktop, scrolled)
settings desktop scrolled
Lyrics (desktop, overflow)
lyrics desktop scrolled
Queue (desktop, scrolled)
queue desktop scrolled
Forced colors (Chromium)
forced colors
WebKit library
webkit library
Recordings
Matrix status after this run
| Platform | Build/launch harness | Real overflow | Scrollbar screenshots | Forced-colors |
|---|---|---|---|---|
| Linux Chromium (Playwright) | ✅ | ✅ | ✅ | ✅ |
| Linux WebKit (Playwright) | ✅ | ✅ | ✅ | N/A (engine) |
| macOS WKWebView packaged | prior agent partial | prior | still needs human | still needs human |
| Windows WebView2 packaged | ❌ no machine here | ❌ | ❌ | ❌ |
This completes the Linux cell of the required physical-platform matrix. macOS/Windows packaged WebView evidence still needs a human or native runner.
Format base-test.ts and scrollbar.spec.ts to pass oxfmt format-check in pre-push hook. These were pre-existing uncommitted changes on the branch that failed format-check during push.
Avoid depending on the i18n "Preferences" heading so settings scroll overflow checks stay stable across locales and settings sections.
Two scrollbar spec tests registered a second page.addInitScript that called window.__OPENKARA_E2E__.setMockSongs(), depending on the fixture's init script having already defined that object. Playwright does not guarantee evaluation order for multiple init scripts, so if the override ran first it would throw before the app loaded and make the overflow tests flaky. Move both overrides after page.goto: call tauriMock.setMockSongs() (which runs the same helper through page.evaluate) and trigger a library reload via the openkara.library BroadcastChannel — the same cross-webview sync path the app already subscribes to. This keeps the mock data setup in the same execution context that defines window.__OPENKARA_E2E__ and eliminates the ordering hazard.
cursor
Bot
force-pushed
the
feat/scrollbar-visibility-97
branch
from
July 17, 2026 02:13
6d0e6d1 to
4a1ec11
Compare
Contributor
Rebase complete — conflicts clearedHead: What changed in this update
Verification (post-rebase)
Greptile P1 (init-script race)Addressed on head as above; Chromium/WebKit overflow suites green after rebase. Still blocked for mergePackaged three-platform scrollbar visual matrix (macOS WKWebView / Windows WebView2 / Linux WebKitGTK). Linux packaged |
Contributor
Packaged Linux acceptance (WebKitGTK) — Cursor Cloud VMHead: Environment
Matrix update
EvidenceMain shell + library overflow (scrolled to Test Song 18–24; thin dark desktop scrollbar visible) Sidebar crop Settings overlay (scrolled into Danger Zone; thin scrollbar on overlay) Library scroll recording Still required before merge approval
Code/CI side for #105 is unblocked: conflicts resolved, init-script race fixed on head, dual-engine E2E green, Linux packaged cell attached. |
…ollbar contract The non-greedy regex only captured up to the first inner rule's closing brace, leaving the remaining ~30 lines of the @supports not block unchecked. Reuse blockBodyForSelectorAt for correct full-block extraction.
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
AppLayoutemitsdata-window-chrome-platform="mac"while the scrollbar andcolor-schemerules only matched thedesktopmarker, so the mac WebView received neither the intended dark native-control environment nor scrollbar variables.--scrollbar-thumb/-hover/-active/-track); give macOS a darkcolor-schemewithscrollbar-color/width: autoand no author::-webkit-scrollbargeometry so WKWebView keeps its native overlay/autohide behavior.desktopmarker with a complete@supports not (scrollbar-color: auto)WebKit fallback (hover/active/track/corner/button), and return scrollbar control to the system underforced-colorsfor every platform.Tests
tests/contract/scrollbar-contract.test.ts: source/contract test asserting no mac selector block installs author scrollbar geometry (::-webkit-scrollbar),scrollbar-width: thin, or a customscrollbar-color; mac applies darkcolor-scheme+autoscrollbars; desktop thin is scoped only to the desktop marker; forced-colors returns control to the system; root tokens resolve to the neutral defaults.tests/e2e/scrollbar.spec.ts: Playwright computed-style assertions for the desktop thin contract (Chromium) and macautocontract (WebKit), descendant inheritance on the song list, a real overflow surface (settings overlay) with programmatic scroll, and forced-colors emulation (Chromium). Feature-detects engine serialization differences instead of asserting onergb(...)spelling.Test plan
pnpm format/pnpm lint/pnpm check:i18n/pnpm knippnpm buildpnpm test(1307 tests, 135 files)pnpm test:coverage(76.66% stmt / 70.4% branch / 67.74% fn / 77.01% line)pnpm check:patch-coveragepnpm test:ui-smoke(chromium: 40 passed) andpnpm test:ui-smoke:webkit(39 passed, 1 skipped — forced-colors is Chromium-only)cargo fmt --check/cargo test(382 tests)pnpm tauri build— not run; CSS-only change with no Rust/IPC/contract/tauri.conf.jsonedits, sopnpm build(the same frontend pipeline Tauri reuses) +cargo test(compiles all Rust) already cover itCloses #97.