Skip to content

feat(dash): scrollbars on scrolling panels, with click-and-drag - #92

Merged
michaelroy-amd merged 4 commits into
mainfrom
feat/dash-scrollbars
Jul 16, 2026
Merged

feat(dash): scrollbars on scrolling panels, with click-and-drag#92
michaelroy-amd merged 4 commits into
mainfrom
feat/dash-scrollbars

Conversation

@michaelroy-amd

Copy link
Copy Markdown
Member

What

Panels gave no visual sign when their content overflowed, and there was no way to grab a scrollbar with the mouse.

  • Reusable panel::vertical_scrollbar / horizontal_scrollbar helpers: no-op when content fits, otherwise draw a bar and return the shrunk content rect.
  • Wired into the job console (vertical + horizontal), the LOGS dock, bench detail, chat transcript, and all seven list managers.
  • Click-and-drag: each drawn bar records a per-frame hit-test handle; a click on a track jumps to that position and arms a drag that keeps tracking the pointer until release. The tail-anchored LOGS dock offset is inverted so dragging down goes toward older lines.
  • Also clamps the job console's horizontal offset against the widest line.

Excluded on purpose: the list managers' bars are drag-to-jump-selection would be awkward (they're driven by ListState selection), so those show a bar but aren't draggable. Easy follow-on.

Tests

  • panel::tests: no-op-when-fits, shrink-on-overflow (V and H).
  • app::tests: proportional position mapping, full grab → drag → release cycle, dock inversion.
  • Full crate suite green (--test-threads=1), clippy -D warnings clean.

Notes

Part 2 of 3. Stacked on #91 (both touch the draw_job_console signature / call site) — please merge #91 first, then this retargets to main.

Job output was forwarded verbatim, so carriage-return progress redraws
(pip/tqdm/huggingface) accumulated into one long horizontal line. Keep only
the segment after the last CR at ingest, and show an animated braille spinner
plus the parsed percentage in the running-job header, driven by a per-tick
counter on the ~250ms repaint loop.

Adds a pure `ui::spinner` module (frame + percent parsing) with unit tests
and a CR-collapse test.

Signed-off-by: Michael Roy <michael.roy@amd.com>
Panels gave no sign when content overflowed. Add reusable `panel::vertical_scrollbar`
/ `horizontal_scrollbar` helpers (no-op when content fits, else draw a bar and
return the shrunk content rect) and wire them into the job console (V+H), the
LOGS dock, bench detail, chat transcript, and the list managers.

Scrollbars are also click-and-drag: each drawn bar records a hit-test handle;
a click on a track jumps to that position and arms a drag that keeps tracking
the pointer until release (inverting the tail-anchored dock). Also clamps the
job console's horizontal offset against the widest line.

Stacked on the progress-spinner branch (shared job-console render path).

Signed-off-by: Michael Roy <michael.roy@amd.com>

@rominf rominf 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.

LGTM. Shared scrollbar helper (no duplicated math), correct clamping (saturating_sub; div-by-zero guarded by max_pos == 0 / span <= 1 early returns), proportional offset math, no panics. Verified drag-when-bar-disappears, DockLogs tail-anchored inversion, and last-track-cell → max_pos mapping against the included unit tests.

Two minor notes:

  1. Horizontal drag clamps to content_w - vbody.width while the keyboard allows one extra column (content_w - 1) — a 1-cell inconsistency in max horizontal offset worth aligning.
  2. The manager-list scrollbars (engine/config/runtime/services/update/automations, folder_browser) render but aren't hit-tested (no record_scrollbar) and track selected rather than the scroll offset — looks intentional for this PR's scope, but the mix of grabbable/non-grabbable bars is a UX rough edge worth a note in the description.

Approving.

@volen-silo

Copy link
Copy Markdown
Collaborator

🔴 Automated review · pr-review-watcher · 2162f28

Summary

Adds reusable vertical/horizontal scrollbar helpers and wires them into the job console, LOGS dock, bench detail, chat transcript, and the seven list managers, with click-and-drag hit-testing for the first four. Verdict: Approve — this is a clean, well-tested change. Verified: cargo build/test(571 pass, --test-threads=1)/clippy -D warnings/fmt --check all clean; confirmed the drag position math (position_at proportional map + DockLogs tail inversion), the per-frame RefCell registry lifecycle (cleared+rebuilt every draw/draw_focused, mutually exclusive, no stale handles or borrow conflicts), and that the two initially-suspected "blocking" issues de-verify to pre-existing behavior / non-crashing visual imprecision. Blocking: 0 · Non-blocking: 4.

🚫 Blocking (must fix before merge)

None.

Non-blocking

  • ui/tabs/chat.rs:240, ui/tabs/bench.rs:458 — scrollbar is told content_len = lines.len() (pre-wrap) while the Paragraph renders with .wrap(Wrap{trim:false}), so when lines wrap the thumb under-reports extent (may not appear on real overflow) and chat drag maps to an imprecise offset; either measure wrapped rows for content_len or drop .wrap(). (Inherits a pre-existing scroll imprecision, now made visible.)
  • 7 list managers (e.g. ui/config_manager.rs:272) — these bars are draw-only (no record_scrollbar/ScrollTarget), so the PR summary bullet reading as "click-and-drag" for them is slightly ambiguous vs the body's noted exclusion; worth tightening the wording.
  • 7 list managers — pass selected (selection index) as the bar position, so the thumb tracks the cursor rather than the viewport top and sits mid-track when it should be near an edge; cosmetic only since these bars aren't hit-tested.
  • Test gaps: no unit test for the ScrollbarHandle::new no-op branch (drawn == area → None, guards against a phantom clickable region), and no drag/release test for the horizontal ConsoleH grab path (only vertical targets are exercised end-to-end).

Base automatically changed from feat/dash-progress-spinner to main July 16, 2026 15:50
Integrate parent PR #91 (braille progress spinner in the job console),
now merged to main as 63b5864, with PR #92's scrollbar + click-and-drag
support.

Sole conflict in crates/rocm-dash-tui/src/ui/job_console.rs was the
draw_job_console doc comment: kept #92's line documenting the returned
scrollbar handles. The function body composes both behaviors — the
running-job spinner/percentage header (#91) and the vertical/horizontal
scrollbar rendering that returns Vec<ScrollbarHandle> for mouse
hit-testing (#92). The ui/mod.rs callsite (auto-merged) passes
tick_count and records the returned handles for drag hit-testing.

Signed-off-by: Michael Roy <michael.roy@amd.com>
Final refresh: bring PR #92 up to main 8308450 (adds #104 decouple vLLM
Prometheus scraping from enable_docker, and #97 discover served model for
the configured chat endpoint).

Clean auto-merge with no conflicts — merge-tree predicted clean and only
crates/rocm-dash-tui/src/app/mod.rs was auto-merged (the #97 chat-endpoint
change; scrollbar hit-testing code untouched). The scrollbar + braille
spinner feature is unchanged: job_console.rs, ui/panel.rs and ui/spinner.rs
are byte-identical to the prior branch head.

Signed-off-by: Michael Roy <michael.roy@amd.com>
@michaelroy-amd
michaelroy-amd enabled auto-merge July 16, 2026 19:52
@michaelroy-amd
michaelroy-amd added this pull request to the merge queue Jul 16, 2026
Merged via the queue into main with commit fbb6f1a Jul 16, 2026
16 of 19 checks passed
@michaelroy-amd
michaelroy-amd deleted the feat/dash-scrollbars branch July 16, 2026 22:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants