[jussielo-amd]

Why

ROCMAI-817: after the dash-tui module split (#827) moved the popup geometry and the menu data out of crates/rocm-dash-tui/src/ui/modal.rs, the file still held the renderers for several unrelated modals (Help, global help, warnings, theme picker, Esc menu, command palette, Options) plus about 600 lines of their tests in one 1,483-line file. This PR splits it into focused modules as a pure, behaviour-preserving move, and delivers the whole ticket.

What changes

One commit per new module, each one compiling and passing clippy on its own, plus a last commit that keeps the crate's public surface as it was:

• ui/theme_picker.rs: draw_theme_picker, draw_theme_list, draw_theme_preview, badge. It has no tests of its own.
• ui/help_modal.rs: draw_help, draw_global_help and their helpers (help_title, popup_height_for, key_line, help_columns, help_group_lines), with the six tests that exercise them.
• ui/warnings_modal.rs: draw_warnings, warning_lines, WARNING_MAX_LINES, with the eleven warnings tests (including the painted_multi_line_warning helper). It imports popup_height_for from help_modal.rs, which does not import it back, so there is no new cycle.
• ui/menu.rs: draw_menu, draw_palette, draw_options, draw_logo, opt_row, the menu sizing constants and menu_fits, with their tests. p4_overlays_render_key_content also renders the global help, so it imports draw_global_help from help_modal.rs; I kept it whole rather than splitting it so the test inventory stays identical.

ui/modal.rs is now its module doc and pub use lines only: the four popup helpers from #827 (centered_rect, centered_width, draw_popup_frame, grey_overlay) and the ten renderer items that were public at ui::modal before this split (listed under the public surface below). I used the grouping the ticket proposes, since it matched the code: help and global help share the sizing helpers, the warnings modal shares only popup_height_for with them, and the menu, palette and Options panel share the logo and the settings row.

Everything besides the moved code is one of these edits, each also listed in the commit message that makes it:

• Each new module is a private mod in ui/mod.rs, and the call sites in ui/mod.rs use the new paths (rustfmt re-wrapped the longer theme picker call). The ten items that were pub in modal.rs stay pub in their new modules so that modal.rs can re-export them; popup_height_for is pub(super) because warnings_modal.rs calls it and it was never public.
• The test fixtures that the help, warnings and menu tests share (flat, rows, bottom_border_row) moved to the ui/mod.rs test module as pub(in crate::ui), next to the helpers ROCMAI-758: split the dash-tui app module, event loop and ui modules into focused files #827 already widened there. The test modules are named tests rather than ported_chrome_tests.
• Imports repointed: the new modules import the popup helpers from ui/panel.rs rather than through the modal.rs re-export, and modal.rs lost the imports it no longer needs.
• Comments and doc links: the comment in draw_help that names the test module, the doc links between draw_help, draw_warnings and popup_height_for (now explicit paths), the banner above the overlays in menu.rs (it listed the global Help, which is now in help_modal.rs), the doc link in panel.rs, the module doc of panel.rs, the comment in ui/mod.rs that names where the grey_overlay test lives, and the comment in the e2e dash step that names where the Esc menu items are.
• docs/architecture.md: one entry per new file, the modal.rs entry rewritten to say it is only re-exports, the panel.rs entry lists the four renderer files as importers of the popup geometry, and the app/types.rs entry now says ui/menu.rs renders the palette destinations and options tabs. The old text said modal.rs renders MENU_ITEMS, which was never true (the menu draws its own item list and count), so I corrected it rather than carrying it over.
• One new test, modal_renderers_stay_reachable_through_ui_modal in tests/dash_characterization.rs, described below.

Public surface

ui declares pub mod modal, so the ten items that were pub in it (draw_help, draw_global_help, draw_warnings, draw_theme_picker, draw_theme_preview, draw_menu, draw_palette, draw_options, draw_logo and opt_row) were reachable as rocm_dash_tui::ui::modal::*. The ticket says the crate's public surface stays as it is, so all ten are pub in their new modules and re-exported from modal.rs, next to the four popup helpers; every path that resolved before this PR still resolves. An earlier revision of this PR made them crate-private instead, which review correctly flagged as a narrowing; nothing in the workspace used those paths, but the ticket states the rule without that condition.

The new test calls all ten through ui::modal from outside the crate (as tests/dash_characterization.rs already does for the four popup helpers) and asserts the text only that renderer paints, so dropping a re-export fails to compile and swapping one for another renderer fails an assertion.

Verification

Run locally:

• Test inventory: cargo test -p rocm-dash-tui --all-targets -- --list and the same with --ignored, keeping lines ending : test, stripping the module path and sorting without -u, on origin/main and on this branch. The only difference is the one added test (1016 and 1017 names; the 4 ignored names are unchanged).
• cargo test -p rocm-dash-tui --all-targets passes (977 lib tests, 4 ignored, plus the integration targets, including the new test).
• The new test was shown to fail when its target is broken: re-exporting draw_palette under the name draw_menu from modal.rs fails the draw_menu assertion. The test names each of the ten through ui::modal, so a dropped re-export fails to compile; I did not run that mutation separately.
• cargo fmt --all --check, cargo clippy -p rocm-dash-tui --all-targets -- -D warnings, cargo check -p rocm, cargo xtask manifest --check and cargo xtask check-crate-edges all pass.
• A line-multiset comparison of the old modal.rs against the five resulting files, taken on the four move commits, shows only the edits listed above, and the leak scan of the diff against origin/main is clean. cargo doc --document-private-items reported no new warnings for the touched files on those commits (it still reports the unresolved gradient::lerp3_t link in the draw_logo doc, which was already unresolved in modal.rs).

Not run locally: the full workspace tests, the tests/e2e-cucumber suite and the Windows lane, which run in CI. The only e2e-visible change is a comment in a dash step. The pre-push hooks were skipped for the last push, so CI is the first full run of the workspace gate on the rebased branch.

Scope

A pure move of the Modal::* renderer bodies and their tests, with the visibility, re-export, import, comment and architecture-doc edits above and no behaviour change. Out of scope: any behaviour change and the other dash-tui and workspace refactoring tickets.

[Copilot]

🟢 Approval recommended

No active review issues remain for the extraction of dashboard modal renderers into focused modules.

No open issues 🎉

What changed in this PR

Completes ROCMAI-817 by splitting the remaining dashboard modal renderers into focused modules, building on #827’s extraction of shared popup geometry. Rendering logic remains unchanged, while the former public renderer paths are deliberately removed.

Changes:

• Moves help, warnings, theme-picker, and menu renderers into four modules, alongside their existing tests.
• Updates dispatch paths and visibility, centralizes shared test fixtures, and preserves the four public geometry-helper re-exports.
• Updates architecture documentation and source comments to reflect the new layout.

File	Description
tests/​e2e-cucumber/​tests/​e2e/​dash_steps.rs	Updates a comment naming the Esc-menu renderer’s location.
docs/​architecture.md	Documents the extracted modules and their dependencies.
crates/​rocm-dash-tui/​src/​ui/​warnings_modal.rs	Houses warning rendering, truncation helpers, and existing tests.
crates/​rocm-dash-tui/​src/​ui/​theme_picker.rs	Houses the theme list and live-preview rendering.
crates/​rocm-dash-tui/​src/​ui/​panel.rs	Updates documentation references to the relocated renderers.
crates/​rocm-dash-tui/​src/​ui/​modal.rs	Retains only documentation and four geometry-helper re-exports.
crates/​rocm-dash-tui/​src/​ui/​mod.rs	Registers modules, redirects rendering calls, and shares test fixtures.
crates/​rocm-dash-tui/​src/​ui/​menu.rs	Houses menu, palette, Options, logo helpers, and existing tests.
crates/​rocm-dash-tui/​src/​ui/​help_modal.rs	Houses both help renderers, shared content-height sizing, and existing tests.

_{Review effort: Balanced}

[jussielo-amd]

ROCMAI-817 says the crate's public surface stays as it is after the earlier dash-tui split. On main, rocm_dash_tui::ui::modal exposes ten public items: draw_help, draw_warnings, draw_theme_picker, draw_theme_preview, draw_menu, draw_palette, draw_options, draw_global_help, draw_logo and opt_row. In this PR modal.rs re-exports only the four panel helpers, and the seven draw_* renderers are pub(super) in modules that are private to ui, so those paths stop resolving for anything outside the crate. ui is a pub mod in lib.rs, so this is a change to the public API. I found no user of those paths inside the workspace, but that is not the rule the ticket sets.

The simplest fix that keeps the move pure is to widen the renderers to pub in their new modules and add pub use lines for all ten items in modal.rs, next to the four panel re-exports. If narrowing is intended instead, the PR body should say so as a deliberate API change and the ticket's "surface stays as it is" line should be reconciled.

Separately, the branch is based on f52cc5b0, which is older than current main, so it needs a rebase before it can merge.

[nowycondro]

🤖 Automated review (Claude Code) · 3482672

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 — needs work

Full review of the whole change.
Implements ROCMAI-817 (split ui/modal.rs into help/warnings/theme-picker/menu modules); the PR body states it "delivers the whole ticket," but the change violates one of the ticket's own explicit restated rules (see Blocking).

Blocking

• Public surface narrowed in violation of the owning ticket's explicit rule — crates/rocm-dash-tui/src/ui/modal.rs (all renderer pub fns removed), crates/rocm-dash-tui/src/ui/help_modal.rs:37,159,195, crates/rocm-dash-tui/src/ui/warnings_modal.rs:79, crates/rocm-dash-tui/src/ui/theme_picker.rs:24, crates/rocm-dash-tui/src/ui/menu.rs:45,87,129.
  Ticket ROCMAI-817's "Rules (epic ROCMAI-27, restated)" state verbatim: "Visibility widening and import-path edits are allowed; nothing else changes" and "The crate's public surface stays as it is after Phase 8.1-F." Before this PR, draw_help, draw_global_help, draw_warnings, draw_theme_picker, draw_menu, draw_palette, and draw_options were pub fn inside pub mod modal inside pub mod ui (crates/rocm-dash-tui/src/lib.rs), reachable from outside the crate as rocm_dash_tui::ui::modal::draw_help etc. This PR changes every one of them to pub(super), and draw_theme_preview/draw_logo/opt_row to private, removing that reachability — a narrowing, not a widening. The PR description discloses this directly ("Because the new modules are private and only the four geometry helpers are re-exported, those paths no longer exist... it is a surface change I would rather state than leave implicit") while the same description claims the PR "delivers the whole ticket." No in-repo consumer currently depends on the removed paths (confirmed: the only remaining user, tests/dash_characterization.rs, only reaches the four still-re-exported geometry helpers), so nothing breaks today — but the ticket states the rule unconditionally, not contingent on current consumers.
  Confidence 85 · architectural · Fix: either keep the renderer functions reachable at their pre-PR public path (re-export them from modal.rs alongside the four geometry helpers, or make the new modules pub mod with the renderers pub), or get explicit sign-off to amend ROCMAI-817's rule before merging and note the amendment in the PR body.

Non-blocking

None

Decisions for the author

• popup_height_for's placement in help_modal.rs rather than beside the other shared popup-geometry helpers in panel.rs — tradeoff. warnings_modal.rs reaches into help_modal.rs for this one piece of content-sizing math even though "warnings" isn't conceptually part of "help"; keeping it where draw_help/draw_global_help also use it avoids a third tiny module, but means one feature module depends on another for a shared utility rather than both depending on a neutral one. Confirm this is the intended shape as more content-sized modals are added.
• Shared test fixtures (flat, rows, bottom_border_row) now live in ui/mod.rs's own tests module as pub(in crate::ui) — non-blocking-improvement. This grows mod.rs's test module beyond its own tests. A dedicated ui::test_support-style module would keep it focused as more modules come to share fixtures, though at today's size it reads fine either way.

Positive signals

• The verification methodology in the PR body — a line-multiset comparison of old vs. new files, a before/after test-name-list diff, and one commit per module each independently compiling — is exactly the discipline a pure-move refactor needs; an independent review of all four new files plus the wiring confirmed the move is in fact byte-faithful (only visibility keywords, import paths, and doc-link wording differ from the original modal.rs).
• The PR candidly surfaces the public-surface narrowing itself rather than leaving a reviewer to discover it — the disclosure is accurate and specific, even though (see Blocking) it still conflicts with the ticket's stated rule.

Deployment notes

• rocm-dash-tui is workspace-internal (publish = false), so the public-surface narrowing above has no crates.io/semver exposure; it only matters to other code in this workspace or a downstream fork reaching into rocm_dash_tui::ui::modal::draw_*.
• At review time, two self-hosted E2E lanes (E2E tests (Strix Halo, Ubuntu, release, current) and (..., nightly, current)) showed CANCELLED, and two more (MI300X, W7900) were IN_PROGRESS/QUEUED; every other required check (clippy, coverage, Windows build/test, the rest of the E2E matrix, CodeQL, license/doc-link checks) was green. Cancelled/pending self-hosted runs are commonly infra scheduling rather than this change (a Rust-only, behavior-preserving move touching no e2e-visible surface beyond one comment), but confirm they finish green before merge.

What this covered

Every file in the diff, completely: crates/rocm-dash-tui/src/ui/{help_modal.rs, warnings_modal.rs, theme_picker.rs, menu.rs, modal.rs, mod.rs, panel.rs}, docs/architecture.md, and tests/e2e-cucumber/tests/e2e/dash_steps.rs, plus the surrounding code each touched function needed (the pre-split modal.rs, ui/theme.rs, ui/gradient.rs, ui/sparkline.rs, lib.rs's module tree, and tests/dash_characterization.rs). Reviewed as four independent fan-out passes grouped by coupling (help+warnings; theme-picker; menu; the mod.rs/modal.rs/panel.rs/docs wiring), each cross-checked against a line-multiset diff of the whole move run independently, plus a workspace-wide grep for any remaining reference to the pre-split paths (none found).
f52cc5b0ceb5d0afeb56daa669f0ee54f2df25ba…34826722f8996cd4eefa51783b925194dda8be7b — note the PR API's reported base sha (6eeec620c…) is main's current tip, which has advanced past this branch's fork point; the diff was taken against the true merge-base instead (the stacked/behind-branch case), and that recomputed base matches the PR's own reported file/line counts exactly (9 files, +1585/−1489).

Did not run: cargo test/cargo clippy/cargo doc locally (no Rust toolchain available in this review environment) — relied on static inspection plus the project's own CI results (read via gh pr view --json statusCheckRollup, taken at face value, not re-derived from workflow definitions), green on every completed required check at review time. History pass (git log on modal.rs/panel.rs) surfaced only the expected lineage (#827's prior split, used here as background only). Prior-changes pass: this PR's own comment thread and #827's review discussion were deliberately not read, per instructions. Agent-instruction pass checked the repo's AGENTS.md (doc-link format, module-privacy conventions, architecture-doc rules) with no violations found beyond the Blocking item above, which comes from the ticket rather than AGENTS.md.
Not reconciled against any discussion on this PR — its own comment thread was not read.

---

🤖 Generated with Claude Code

[volen-silo]

I read the whole diff at 3482672 and checked it as a pure move. I compared every non-blank, non-comment line of the old modal.rs against the new modules plus ui/mod.rs, ignoring imports and pub qualifiers: the only old line with no counterpart is super::draw_global_help(...), which is now the imported call the description mentions. The test count is unchanged (21 before, 21 after), and the architecture.md additions describe what the files now contain.

One thing for the author and ticket owner to decide, not a defect in the move: the seven render functions were pub at rocm_dash_tui::ui::modal::* and are now crate-private, which the description discloses. I confirmed the crate has publish = false and nothing in the repo reaches those paths (dash_characterization.rs only uses the four geometry helpers that are still re-exported), so nothing breaks today. If the ticket's "public surface stays as it is" rule is meant literally, re-exporting them from modal.rs is a one-line-per-function fix; otherwise the amendment is worth a line in the ticket.

This looks safe to approve after final validation. That is what a read of the diff can show; it is not a CI result or a runtime check, and I did not build or run it, so the remaining checks (the W7900 e2e lane was still running) and the maintainer's own pass still apply. This is not itself an approval.

[jussielo-amd]

Thanks for the reviews. Addressed in e407b24, pushed together with a rebase onto main as of d7faa84 (no conflicts; the four move commits are unchanged apart from the rebase). The existing approval was given on 3482672 and is still showing as the current review decision after the push.

The public surface: all three of you flagged that the renderers stopped being reachable at rocm_dash_tui::ui::modal::*, against the ticket's rule that the surface stays as it was. That was right, so I took the re-export route rather than amending the ticket. The ten items that were pub at that path (draw_help, draw_global_help, draw_warnings, draw_theme_picker, draw_theme_preview, draw_menu, draw_palette, draw_options, draw_logo and opt_row) are pub again in their new modules and re-exported from modal.rs next to the four popup helpers, so every path that resolved before this PR still does. popup_height_for stays pub(super) since it was never public.

To keep it from regressing, tests/dash_characterization.rs has a new test, modal_renderers_stay_reachable_through_ui_modal, that calls all ten through ui::modal from outside the crate and asserts text only that renderer paints. I checked it goes red on a wrong re-export: exporting draw_palette under the name draw_menu fails the draw_menu assertion. The test inventory against origin/main differs by exactly that one added test, and fmt, clippy, the crate's tests, cargo xtask manifest --check and cargo xtask check-crate-edges pass locally. I updated the PR description (the public-surface section, the visibility bullet and the verification numbers) and the modal.rs entry in docs/architecture.md to match.

On the two decisions in the automated review: I'm leaving popup_height_for in help_modal.rs and the shared test fixtures in ui/mod.rs as they are, since moving either would be a non-mechanical change in a PR that is meant to be a pure move; both are cheap to revisit when a third modal needs them.

One thing to know: I pushed this update with the pre-push hooks skipped, so CI is the first full run of the workspace gate on the rebased branch.

[tomastola]

Reviewed the whole change at e407b24. The four move commits are faithful relocations: line-by-line, the old modal.rs against the four new modules differs only in imports, visibility, module docs and repointed doc links, and the test inventory is unchanged. e407b24 restores every item that was public at ui::modal. I checked the new modal_renderers_stay_reachable_through_ui_modal test by swapping the draw_menu/draw_palette re-exports, the only mix-up that compiles, and it fails as intended. dash_characterization passes 22/22 locally on the head.

Two non-blocking notes:

• crates/rocm-dash-tui/src/ui/panel.rs:366: repointing the link to help_modal::draw_help left this doc line at 114 characters, against the 75–80 of the lines around it. rustfmt won't rewrap comments, so it needs a manual rewrap.
• crates/rocm-dash-tui/src/ui/menu.rs:198: [`gradient::lerp3_t`] doesn't resolve, because menu.rs has no gradient in scope, and cargo doc --document-private-items warns about it. This is pre-existing (your description already notes it), but since these lines are moving anyway, [`crate::ui::gradient::lerp3_t`] fixes it cheaply.

[nowycondro]

🤖 Automated review (Claude Code) · e407b24

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 (base 6eeec620…head e407b246, the review-fix commit on top of the previously-reviewed 34826722). Implements ROCMAI-817 (split ui/modal.rs into help_modal.rs/warnings_modal.rs/theme_picker.rs/menu.rs). The prior round's one blocking finding — this split narrowed the crate's public surface in violation of the ticket's own explicit "visibility widening only, public surface stays as it is" rule — was independently re-checked against the current code rather than assumed fixed.

Blocking

None. The public-surface narrowing is resolved: crates/rocm-dash-tui/src/ui/modal.rs at e407b246 now re-exports every renderer that was pub at ui::modal before the split (draw_help, draw_global_help, draw_menu, draw_palette, draw_options, draw_logo, opt_row, draw_warnings, draw_theme_picker, draw_theme_preview), alongside the four geometry helpers already covered — confirmed by reading the file directly at this commit, not taken from the prior round's prose.

Non-blocking

• crates/rocm-dash-tui/src/ui/panel.rs:366's doc-link line, after repointing to help_modal::draw_help, is 114 characters against the surrounding ~75-80. rustfmt does not rewrap comments, so this needs a manual rewrap.
  Confidence 80 · mechanical
• crates/rocm-dash-tui/src/ui/menu.rs:198's [`gradient::lerp3_t`] doc link does not resolve (menu.rs has no gradient in scope at either the pre- or post-split location, so cargo doc --document-private-items warns on it) — pre-existing, not introduced by this PR, but since these lines moved anyway, [`crate::ui::gradient::lerp3_t`] would fix it at negligible cost.
  Confidence 75 · mechanical

Decisions for the author

None outstanding — the two scope decisions already disclosed in the PR body (popup_height_for's placement in help_modal.rs rather than panel.rs; shared test fixtures growing ui/mod.rs's own test module) were already reasoned through in the prior round and are unaffected by this fix-up commit.

Positive signals

• Independently diffed menu.rs's relocated functions (draw_menu, draw_palette, draw_options, draw_logo, opt_row, menu_fits) and their five tests against the pre-split modal.rs body: function and test bodies are byte-for-byte identical apart from one legitimate cross-module call-path change (super::draw_global_help → crate::ui::help_modal::draw_global_help, matching draw_global_help's actual new location exactly).
• The consolidated test-only flat() buffer helper now lives once in crate::ui::tests::flat rather than being duplicated per split-out file — confirmed menu.rs imports it rather than carrying its own copy, and the import list was correctly pruned to exactly what the relocated functions use (no leftover Constraint/Direction/Layout/Wrap/GradientGauge imports that belonged to functions that didn't move here).
• modal_renderers_stay_reachable_through_ui_modal (per a prior approving reviewer's own verification, independently plausible from the re-export list above) is the regression guard against this exact class of re-export drift recurring.
• CI on this head: all completed required checks (build-and-test, clippy, Windows build/test, Test (affected crates), CodeQL, coverage) are green; only a few self-hosted GPU E2E lanes are still queued/in-progress, none failing.

Deployment notes

None — rocm-dash-tui is workspace-internal (publish = false), so even the now-resolved public-surface question had no crates.io/semver exposure.

What this covered

Independently re-verified at head e407b24635a43673109d5376a61837b505da6c3b: crates/rocm-dash-tui/src/ui/modal.rs (full file, fetched directly), confirming the re-export fix. One independent blind fan-out worker did a body-for-body diff of menu.rs against the pre-split modal.rs (functions, tests, imports, the shared flat() helper, and the gradient::lerp3_t doc-link question) and found no behavioral differences. I did not re-read help_modal.rs/warnings_modal.rs/theme_picker.rs/mod.rs/panel.rs/docs/architecture.md in full this round — the prior round (at 34826722) already read all of them completely as a faithful byte-level move, and the only thing that changed between that commit and this one is the modal.rs re-export fix, which this round verified directly.

Did not run: cargo build/test/clippy/cargo doc locally (no Rust toolchain in this environment) — relied on the prior round's direct inspection plus this round's targeted re-verification, and on CI's own green build/test/clippy/doc-link results for this exact commit. Not reconciled against this PR's own review discussion (including the two prior human approvals), which was not read beyond what the mandatory claim-check surfaced.

---

🤖 Generated with Claude Code