[jussielo-amd]

Why

The Instances Observe tab has drawn its per-instance table (draw_table) since the five-tab redesign replaced the older full-screen card grid, but the grid and everything behind it stayed in the crate. Nothing in production reaches it: instances::draw was referenced only by a #[cfg(test)] import in ui/instance_detail.rs, observe.rs calls only draw_table, the grid's hit_test is not wired into the mouse handling (which routes only the Rocm and Serving tabs), and ui::heatmap was used only by the grid's kv-cache block. No lint fires because ui is a pub module of a library crate, so the roughly 17 tests that exercised the grid were the only thing keeping it alive. This is part of the follow-up work from the modularization audit. Completes ROCMAI-853.

What changes

Four commits, each reviewable or droppable on its own; the fourth is comment-only.

The first deletes the card grid and the heatmap. From ui/tabs/instances.rs it removes draw, compute_heatmap_height, draw_card_grid, draw_kv_heatmap, label_width_for, build_kv_heatmap_rows, pick_cols, status_role, draw_card, compact_line, hit_test and point_in_rect, and updates the module doc and the sorted_instances doc to describe a table rather than a grid. It deletes the ui/heatmap.rs module (Heatmap, HeatmapRow, truncate_to) and its declaration in ui/mod.rs. Tests that only exercised the deleted code go with it: 13 in instances.rs and the 9 in heatmap.rs. Two tests also cover live code and were kept. The empty-state test (draw_empty_names_the_platform_reason_for_the_in_process_collector) now renders through draw_table, which shares draw_empty with the old entry point, and the instance-detail VRAM test keeps its detail-modal half, since draw_detail renders the same used/total string as the grid did. draw_table, draw_empty, sorted_instances, clamp_sel, status_meta and the fixtures other tests import are unchanged, and gen_tps_compact and mib_pair, which the card also used, still have other callers.

The second removes pct_opt and reqs_opt from ui/format.rs with their two tests: the card was their only production caller. pct and si, which they wrapped, are still used and stay.

The third removes BoxRole::from_tone and BoxRole::Danger, which the deletion newly orphans. from_tone (added recently when instance and service status colors were unified) was called only by the card's status_role, and Danger was constructed only by from_tone. It also removes their two tests, the now-unused StatusTone import in panel.rs, the sentence in the instance_status_tone doc that named from_tone, and the sentence in the shared-components list in AGENTS.md that described it as the way to derive a card border from a tone. instance_status_tone and Theme::instance_status_color are unchanged and still drive status text color. I kept this separate because from_tone is new and documents a convention in AGENTS.md: if a bordered status card is wanted again, drop this one commit and the rest of the PR still stands.

The fourth is a comment-only follow-up from review: it rewrites the doc comments that still named the deleted card grid or heatmap, in rocm-dash-daemon/src/demo.rs (the phase offset now says it staggers the Observe table curves and the per-GPU utilization sparklines, and the launch-args note names the detail modal), ui/widgets.rs (the power-threshold note now speaks of temperature and power colors, which is what temperature_style and power_style use them for), app/mod.rs (instance_sel is a cursor into the table), ui/instance_detail.rs and ui/format.rs. No code line changes in that commit.

Verification

Run locally on the final branch: cargo fmt --all --check (clean); cargo clippy -p rocm-dash-tui --all-targets -- -D warnings (clean, run once after the card-grid and formatter deletions and again after the third commit; after the fourth it was run for rocm-dash-daemon and rocm-dash-tui together, also clean); cargo test -p rocm-dash-tui (975 passed, 0 failed, 4 ignored, plus the crate's integration tests); cargo xtask manifest --check and cargo xtask check-crate-edges (both pass); and the repository's pre-commit hooks on all four commits (fmt, license headers, markdown links, commit signing, sign-off). A leak scan of the diff against the merge base and a conflict-marker grep are clean, and a merge dry-run against the current main reports no conflicts.

The dead-code check from the audit was re-run on the final tree: with pub mod ui narrowed to pub(crate) in a scratch edit (since reverted), cargo check -p rocm-dash-tui --lib reports nothing in instances.rs, format.rs or the removed heatmap module. Before the third commit it flagged exactly the two items that commit removes, BoxRole::from_tone and BoxRole::Danger, so nothing else was orphaned. It still flags items that are not part of this change: the skills.rs subsystem (its own ticket), dock::triptych, chat::transcript_lines and hardware::gpu_visible_count with ROWS_ABOVE_GPUS (all removed by #842), the ui/launcher.rs items (used by apps/rocm, so not dead), and some re-export imports in ui/modal.rs, which I left alone.

Not run

The full workspace tests, smoke_local, the e2e-cucumber suite and the Windows lane were not run; CI carries them. The coverage floor was not checked. cargo xtask coverage measures the whole workspace under instrumentation, which is far outside what I could run here, and a single-crate run would not be comparable to the committed floor. This change removes about 1,000 lines of well-covered code and 26 tests from a crate whose floor is 89.33% with a 0.2-point tolerance, so the floor may drop below tolerance. If the coverage job reports that, the right follow-up is cargo xtask coverage --bless with that justification in its commit, not weakening or restoring a test. I did not re-bless anything.

Overlaps and scope

#812 edits ui/tabs/instances.rs and ui/format.rs, so whichever of the two lands second will need a rebase. The other dead dash-TUI helpers from the same audit (dock::triptych, chat::transcript_lines, hardware::gpu_visible_count and ROWS_ABOVE_GPUS) are in #842, not here. No other open PR touches heatmap.rs.

There is no user-visible behavior change and no scenario is needed: the code removed is unreachable from the dashboard, and the live Observe table and detail modal keep their existing unit tests unchanged.

[Copilot]

Warning

Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.

Copilot review overview

1 open issue

• pub mod heatmap; is removed as part of deleting the heatmap module. Since ui is publicly… · New

What changed in this PR

Removes unreachable Instances Observe card-grid UI (and its heatmap) and cleans up now-orphaned UI helpers/APIs, leaving the production table-based Observe view and instance detail modal intact.

Changes:

• Deleted the old Instances card grid renderer, hit-testing, and the ui::heatmap widget module along with their dedicated tests.
• Removed format helpers (pct_opt, reqs_opt) that were only used by the deleted card grid.
• Removed BoxRole::from_tone and BoxRole::Danger now orphaned by the UI deletions, and updated docs accordingly.

File	Description
crates/​rocm-dash-tui/​src/​ui/​theme.rs	Updates tone documentation to remove references to deleted border-mapping API.
crates/​rocm-dash-tui/​src/​ui/​tabs/​instances.rs	Removes dead card-grid/heatmap paths and associated tests; table renderer remains.
crates/​rocm-dash-tui/​src/​ui/​panel.rs	Removes BoxRole::Danger and BoxRole::from_tone, plus their tests.
crates/​rocm-dash-tui/​src/​ui/​mod.rs	Removes pub mod heatmap; export after deleting the module.
crates/​rocm-dash-tui/​src/​ui/​instance_detail.rs	Removes test-only usage of deleted instances::draw entry point.
crates/​rocm-dash-tui/​src/​ui/​heatmap.rs	Deletes the heatmap widget implementation and its tests.
crates/​rocm-dash-tui/​src/​ui/​format.rs	Removes pct_opt / reqs_opt public helpers and their tests.
AGENTS.md	Updates guidance to remove references to BoxRole::from_tone.

_{Review effort: Lite}

[jussielo-amd]

Checked: the workspace sets publish = false for every first-party crate ("an application, not crates.io libraries" in the root Cargo.toml), and the only users of crate::ui::heatmap on main are the card-grid functions in ui/tabs/instances.rs that this PR deletes. rocm-dash-tui is used only as a dependency of the in-tree rocm binary, so there is no downstream consumer whose import could break, and no compatibility shim is kept.

[nowycondro]

🤖 Automated review (Claude Code) · 1dd71eb

This automation never files a GitHub approval, so no approving review will appear here whatever the outcome — the merge decision stays with a human reviewer.

Review — no blocking findings

Full review of the whole change (3 commits, 8 files, +11/-1038 lines), diffed against the true merge-base db7e5408 (the PR's reported base.sha, a7f84e8a, is main's current tip and is not an ancestor of the head commit — see note below; the file set and diffstat I reviewed match the PR's own description exactly, so this did not change what was reviewed).

Implements ROCMAI-853 in full ("Phase 8.1-H step 7, PR 7 of 7"). The ticket's own decision log ("Decision, 2026-11-04: delete, do not rewire") matches this PR's description and code precisely, down to the same list of 12 dead functions in instances.rs, the same orphaned ui/heatmap.rs and BoxRole::Danger, and the same explicit carve-outs (skills.rs, #842's items). No mismatch between the ticket and the change.

Blocking

None

Non-blocking

• Stale doc comment still describes deleted "card" code — crates/rocm-dash-tui/src/ui/instance_detail.rs:9
  The module doc reads: "...which stay single-sourced beside the table and card code that also use them". The card code (draw_card, draw_card_grid, status_role, etc.) is exactly what this PR deletes from instances.rs, and this PR already edits this same doc-comment-adjacent file (dropping the use …instances::draw; test import) and updates the analogous sentences in theme.rs, instances.rs's own module doc, and AGENTS.md — but missed this one instance in a file it was already touching for the same reason.
  Confidence 85 · mechanical · Fix: drop "and card" so it reads "...beside the table code that also use them."

Decisions for the author

• Manual dead-code audits vs. a structural guard — non-blocking-improvement
  The PR's own description notes this code stayed alive for as long as it did because ui is a pub module on a library crate, so no compiler/lint ever flagged it as dead; the audit that caught it was a manual, one-off pub(crate) scratch-narrowing. ui/launcher.rs needs the pub for apps/rocm, so blanket-narrowing isn't available, but a periodic/scheduled CI job that does the same pub(crate) + cargo check narrowing this PR did by hand would catch the next round of this class of drift automatically instead of waiting for another manual audit ticket.

Positive signals

• Impact analysis (this review's own repo-wide grep sweep, independently run across every removed symbol — draw, hit_test, compute_heatmap_height, draw_card_grid, draw_kv_heatmap, label_width_for, build_kv_heatmap_rows, pick_cols, status_role, draw_card, compact_line, point_in_rect, Heatmap, HeatmapRow, pct_opt, reqs_opt, BoxRole::Danger, BoxRole::from_tone) found zero remaining references anywhere in the workspace, including apps/rocm, the dash-TUI examples, and the e2e-cucumber test driver — confirming the PR's own "nothing else reaches this" claim rather than just trusting it.
• The three commits are genuinely independently droppable as claimed: commit 3 (BoxRole::from_tone/Danger) depends only on commit 1 having already removed status_role, and nothing depends on commit 3 in the other direction.
• sorted_instances's and instance_status_tone's doc comments were correctly updated to drop their grid/from_tone references, and no stray "heatmap"/"card grid" text was left in docs/architecture.md or anywhere else in docs/.

Deployment notes

None — no user-visible behavior change (confirmed: draw_table/draw_detail and their tests are untouched), no schema/config/migration involved.

What this covered

Fanned out to two independent background workers: one for the code-correctness + impact-analysis pass (all 8 touched files read in full at head 1dd71eb8, plus surrounding callers — app/scrollbar.rs's mouse dispatch, gradient.rs, theme.rs's StatusTone — and an exhaustive repo-wide grep sweep for every removed symbol), and one for the four source-reading passes (agent-instruction adherence against root AGENTS.md, git history/blame on the touched regions back to their origin, a search of all 116 local branches and PR #62/#850 for prior review comments on this same code, and a full re-read of every comment/doc-comment in the touched files). Both ran blind (no PR description, no discussion) and reported back independently; I verified the one surviving finding directly against the file myself before scoring it. CI was read, not re-derived: all functional checks (build-and-test, clippy, Test (affected crates), Coverage (rocm-dash crates, ratcheted), Windows build/lint, E2E consolidated report) show success on this exact head commit; the hardware-specific E2E matrix jobs show skipped, expected since this PR's head lives on a fork (jussielo-amd/rocm-cli) without access to self-hosted runners. Neither worker had a local Rust toolchain, so cargo test/clippy were read from CI rather than re-run locally — noted as a gap, not assumed.

Ticket ROCMAI-853 retrieved directly via its key (not via a link in the description) and read in full; see the intent line above for the reconciliation.

Not reconciled against this PR's existing review thread (the Copilot bot's single "medium severity" note about pub mod heatmap; being a public-API removal) — that comment was visible to me only incidentally during the required duplicate-review claim-check, was never passed to either blind worker, and this review's own independent impact-analysis (above) already covers the same ground and found no actual external caller of the removed heatmap module.

🤖 Generated with Claude Code

[juhovainio]

Deletion looks clean. No live references to the removed symbols remain at the head; the helpers the card shared (lerp3_t, mib_pair, gen_tps_compact, tokens_per_watt_cell, draw_empty, clamp_sel) still have callers; the removed tests covered only deleted code, and the empty-state test now runs through draw_table. Docs have no leftover references, the dash coverage-floor job is green, and the scope matches the ticket.

One non-blocking nit in a file this PR doesn't touch: crates/rocm-dash-daemon/src/demo.rs:189 still says "heatmap and per-GPU sparklines", which refers to the heatmap deleted here. Worth rewording in this PR or a follow-up.

[jussielo-amd]

Thanks for the approval and the note about demo.rs. You were right that the comment was stale: ContainerSpec::phase_s said it staggered the instances so "the heatmap and per-GPU sparklines" are not synchronised, and the heatmap is the thing this PR deletes. It now names what the offset actually feeds, the instance table columns (throughput, queue, kv-cache) and the per-GPU utilization sparklines. The fix is in 1ddc5c9, a comment-only commit.

I also grepped for the same kind of leftover and fixed the five others I found: the "heatmap/gauge colors" comment in widgets.rs (now "temperature and power colors"), "instance card / detail modal" in the launch_args doc, "Instances grid" in the instance_sel doc, "table and card code" in the instance_detail.rs module doc, and "for cards" in the gen_tps_compact doc. No code lines changed, and fmt and clippy are clean for both dash crates.