Skip to content

feat(dash): make the ROCm/Serving detail pane state-aware - #93

Merged
michaelroy-amd merged 1 commit into
mainfrom
feat/dash-aware-detail
Jul 15, 2026
Merged

feat(dash): make the ROCm/Serving detail pane state-aware#93
michaelroy-amd merged 1 commit into
mainfrom
feat/dash-aware-detail

Conversation

@michaelroy-amd

Copy link
Copy Markdown
Member

What

The Actions → Details pane on the ROCm and Serving tabs rendered fixed copy regardless of system state. Thread &AppState into pane::draw / draw_detail and render a live status block per verb:

  • Set up / Install ROCm — detected ROCm + driver versions and installed runtimes.
  • Serve a model — the known-model catalog.
  • Engines — Lemonade install status (vLLM deferred until the daemon surfaces detection).
  • Running instances — live instances inline (model / port / gen tps), no popup.

Falls back to honest not detected / none running when telemetry is absent, so it degrades correctly on machines without ROCm.

Tests

  • Tab tests for the populated install (detected ROCm version) and running-instances (inline, no modal) paths.
  • Full crate suite green (--test-threads=1), clippy -D warnings clean.

Notes

Part 3 of 3 for today's dash UX work. Independent of #91 / #92 (disjoint files) — mergeable in any order.

Follow-on (not in this PR): real downloaded-model detection and vLLM install detection need a new background fetch + AppState fields.

@michaelroy-amd
michaelroy-amd enabled auto-merge July 10, 2026 14:26

@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. The state→verb mapping is correct and the _ => Vec::new() catch-all is appropriate (documented; most KeyAction variants have no live view), no panics, lifetimes are fine (Vec<Line<'static>> coerces cleanly), both callers (rocm.rs, serving.rs) are updated, and regression tests are added.

Minor (pre-existing, not introduced here): the OpenServices live view iterates state.instances.values().take(5), so which 5 of N running instances appear is non-deterministic frame-to-frame — a stable sort by model/port would be a nice follow-up. The count header is always correct.

Approving.

@volen-silo

Copy link
Copy Markdown
Collaborator

🔴 Automated review · pr-review-watcher · 56b57ab

Summary

Threads &AppState into the shared ROCm/Serving pane::draw and renders a live status block per verb (installed ROCm/driver + runtimes, engine install status, model catalog, running instances inline) with honest not detected / none running fallbacks. Verdict: Approve — nits only. Verified: cargo build -p rocm-dash-tui clean and cargo test -p rocm-dash-tui --lib -- --test-threads=1 green (545 passed); confirmed all field/type references (GpuSystemInfo.{rocm,driver,lemond}_version, Instance.{model_name,port,gen_tps,status}, RuntimeSummary, ModelRecipeSummary.id, format::tps_opt) resolve, the live_lines match arms are correct with _ => Vec::new() covering the non-live verbs, the .take(4/5) + "…and N more" arithmetic is correct, both pane::draw call sites (rocm.rs, serving.rs) are updated with no missed callers, and the two new tests' selection indices map to the intended verbs. Blocking: 0 · Non-blocking: 4.

🚫 Blocking (must fix before merge)

None.

Non-blocking

  • pane.rs (OpenServices arm) — running instances come from state.instances.values() unsorted, then .take(5); HashMap order is nondeterministic, so with >5 running instances which 5 (and their order) can shuffle frame-to-frame. The codebase already solved this with tabs/instances.rs::sorted_instances() (used in 4 places) — reuse it / sort by container_name before .take(5).
  • draw_detail (pane.rs) — the detail Paragraph has no .scroll() and no height check; the added live lines (up to ~8 before "What you'll do") make it materially more likely the Start affordance / trailing steps clip off-screen on short terminals, with no scrollbar or "more below" cue. Consider capping the live block or making the pane scroll-aware.
  • Test gaps — the OpenEngineManager and OpenServeWizard live blocks, the not detected / none running fallbacks, and the "…and N more" truncation are all untested; and serving.rs:206 out.contains("8000") is a weak assertion (matches a bare digit run, not the rendered :8000).
  • pane.rs:91draw passes both focus and state into draw_detail even though focus == state.pane_focus; drop the redundant param.

The Actions -> Details pane rendered fixed copy. Thread `&AppState` into
`pane::draw`/`draw_detail` and render a live status block per verb:

- Set up / Install ROCm: detected ROCm + driver versions and installed runtimes
- Serve a model: the known-model catalog
- Engines: Lemonade install status (vLLM deferred until the daemon surfaces it)
- Running instances: live instances inline (model/port/gen tps) — no popup

Falls back to honest "not detected" / "none running" when telemetry is absent.
Adds tab tests for the populated install + running-instances paths.

Signed-off-by: Michael Roy <michael.roy@amd.com>
@michaelroy-amd
michaelroy-amd force-pushed the feat/dash-aware-detail branch from 56b57ab to 1bd8b34 Compare July 15, 2026 00:31
@michaelroy-amd
michaelroy-amd added this pull request to the merge queue Jul 15, 2026
Merged via the queue into main with commit 5b32c86 Jul 15, 2026
15 checks passed
@michaelroy-amd
michaelroy-amd deleted the feat/dash-aware-detail branch July 15, 2026 00:50
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