Skip to content

feat(dash): braille progress spinner in the job console - #91

Merged
michaelroy-amd merged 1 commit into
mainfrom
feat/dash-progress-spinner
Jul 16, 2026
Merged

feat(dash): braille progress spinner in the job console#91
michaelroy-amd merged 1 commit into
mainfrom
feat/dash-progress-spinner

Conversation

@michaelroy-amd

Copy link
Copy Markdown
Member

What

Long-running job output was forwarded verbatim, so carriage-return progress redraws (pip / tqdm / huggingface) piled up into one long horizontal line in the job console.

  • Keep only the segment after the last \r at ingest (jobs::last_cr_segment), collapsing a redraw sequence to its final visible state.
  • Show an animated braille spinner (⠋⠙⠹…) plus the parsed percentage in the running-job status header, driven by a tick_count advanced on the existing ~250ms repaint loop.
  • New pure ui::spinner module (spinner_frame + parse_progress_pct).

Tests

  • ui::spinner::tests: frame cycling, percent parsing (incl. rejection of out-of-range / bare %).
  • jobs::tests: CR-collapse + plain-line passthrough.
  • Full crate suite green (--test-threads=1), clippy -D warnings clean.

Notes

Part 1 of 3 for today's dash UX work. The scrollbars PR is stacked on this branch (shared job-console render path).

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>

@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. Spinner/percentage edge cases are correct — out-of-range and malformed % ("1.2.3%", bare %, "250% load") all resolve to None with no panics, and modulo frame indexing (tick % 10, wrapping_add) is safe. All draw_job_console call sites and both AppState struct-literal sites are updated, new ui/spinner.rs carries the correct MIT SPDX header, and both helpers are unit-tested.

Minor (non-blocking): job_console.rs rescans the output ring each frame for a trailing % — bounded (~1000 cheap ops at ~4 Hz), so fine. Also note last_cr_segment now drops pre-\r content for all emitted lines, not just progress lines — the intended terminal-redraw tradeoff.

Approving.

@volen-silo

Copy link
Copy Markdown
Collaborator

🔴 Automated review · pr-review-watcher · e80f8b8

Summary

Collapses \r-redraw progress lines to their final segment at ingest and adds an animated braille spinner + parsed percentage to the running-job console header, driven by a new per-tick AppState.tick_count. Verdict: Approve — clean, well-scoped, well-commented, one logical change. Verified: cargo test -p rocm-dash-tui --all-targets green (all suites, incl. the 5 new spinner/jobs tests) and cargo clippy -p rocm-dash-tui --all-targets -- -D warnings clean on my local checkout; confirmed no panics in parse_progress_pct/last_cr_segment/spinner_frame across adversarial inputs (multi-dot, out-of-range, multibyte-before-%, trailing \r, u64::MAX tick), all AppState construction sites carry tick_count, and the diff is genuinely limited to the spinner feature (the large chat/is_actionable_key churn visible in a main..HEAD compare is main moving ahead by 4 commits, not this PR). Blocking: 0 · Non-blocking: 4.

🚫 Blocking (must fix before merge)

None.

Non-blocking

  • src/ui/spinner.rs:31parse_progress_pct("… -50%") returns Some(50): the leading - isn't consumed by the digit walk, so a negative value is silently coerced to positive rather than rejected. Harmless for pip/tqdm/hf output but a wrong-but-plausible result; reject when the char before the digit run is -.
  • src/ui/spinner.rs:24rfind('%') means a trailing non-progress percent suppresses a valid earlier one: "50% done, cpu 250%"None (250 is out of range, real 50% missed). Acceptable given the target formats print one %, but worth a comment noting the last-%-wins assumption.
  • tests/wave0_job_bridge.rs:162 / src/ui/job_console.rs:123-148 — no behavioral test wires the spinner/percentage through draw_job_console: the snapshot test seeds no % line and renders at tick=0, so the percentage branch is never exercised and the glyph is never asserted. Consider seeding a 45% line + asserting out.contains("45%"), and rendering at two tick values to confirm the glyph advances.
  • feat/dash-progress-spinner is 4 commits behind main; rebasing before merge keeps the diff/CI honest (GitHub already compares against the merge-base, so the PR view is correct, but a local main..HEAD looks alarming).

@michaelroy-amd
michaelroy-amd added this pull request to the merge queue Jul 16, 2026
Merged via the queue into main with commit 63b5864 Jul 16, 2026
15 checks passed
@michaelroy-amd
michaelroy-amd deleted the feat/dash-progress-spinner branch July 16, 2026 15:50
michaelroy-amd added a commit that referenced this pull request Jul 16, 2026
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>
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