Repository navigation
ROCMAI-758: split the dash-tui app module, event loop and ui modules into focused files - #827
Conversation
…test Move the slash-command, chat-agent factory, slash-tool summary, resolved-args and focused-draw tests out of app/mod.rs into the test modules of slash.rs, chat.rs, summary.rs, types.rs and ui/mod.rs, and the three quit-confirm services-key tests from ui/mod.rs into app/mod.rs, next to the reducer method they exercise. The args_with_anthropic_key helper moves to app/test_support.rs and st() widens to pub(crate) so ui tests can reach it. Test-only: no production code changes. Signed-off-by: Jussi Elo <jussi.elo@amd.com>
…a into app/types.rs ui/modal.rs sat upstream of the shared components that depend on it: the approval and job-console overlays imported popup geometry from a file of specific modals, and app/actions.rs read the menu, palette and options constants from a render file. Move centered_rect, centered_width, draw_popup_frame and grey_overlay (with their private helpers) into ui/panel.rs beside panel::popup, and MENU_ITEMS, PALETTE_DESTS and OPTIONS_TABS into app/types.rs, re-exported from app/mod.rs. Importers are repointed rather than re-exported, so the reversed edge is gone. Moved geometry tests go with the code; the grey_overlay test stays beside the modal-only logo it composes. Signed-off-by: Jussi Elo <jussi.elo@amd.com>
…app submodules Move the quit-confirm gates and prompt handling into app/quit_confirm.rs and the plan-to-approval hand-off and verdict routing into app/tool_approval.rs, each a further impl AppState block in a child module so the reducer's file shrinks without adding a module cycle. The module is named tool_approval to avoid colliding with ui::approval. Tests move by subject with their recording-executor fixtures, and the comment pins in apps/rocm and the e2e cucumber sources that name the collapsed-envelope test now point at its new file. Signed-off-by: Jussi Elo <jussi.elo@amd.com>
…s into app submodules Move the chat consent/detect/submit methods into app/chat_state.rs, the overlay bookkeeping and input-gating predicates into app/overlays.rs, and the scroll-offset methods into app/scroll.rs, each a further impl AppState block in a child module. close_overlays and set_chat_scroll widen to pub(super) because sibling modules call them. app/scroll.rs is deliberately not named scrollbar, which the actions.rs import guard matches. Tests move by the method they exercise, the app/mod.rs module doc is rewritten to describe the new layout, and the comment pins that name moved symbols now point at their new files. Signed-off-by: Jussi Elo <jussi.elo@amd.com>
…event_loop.rs Move should_skip_daemon, focused_close_key_blocked, open_overlay_for_focus and apply_startup_focus into app/focus.rs, and the Updates-tile check (refresh_update_status, reduce_update_json and their constants) into app/update_check.rs. Both are leaves the loop calls into, so no cycle is introduced; app/mod.rs keeps its re-export of HOME_UPDATE_CHECK_JOB_ID for the Home tab. Their tests move with them, and the comment in apps/rocm that names the startup-focus gate test now points at focus.rs. Signed-off-by: Jussi Elo <jussi.elo@amd.com>
…p/terminal.rs
Move terminal restoration, the shutdown latch, the termination-signal
watcher (with its cfg(unix) and cfg(windows) TerminationSignals variants and
exit codes) and the Ctrl+C key helpers out of event_loop.rs into a leaf
module, so the loop file keeps only run, the loop itself and the frame gate.
app/mod.rs re-exports the same names, so crate::app::* and the public
rocm_dash_tui::app::{run, spawn_termination_watcher} paths are unchanged.
claim_shutdown and write_restore_sequences widen to pub(super) for the tests
that stay beside the loop. The signal and restore tests move with the code,
including the platform-gated ones.
Signed-off-by: Jussi Elo <jussi.elo@amd.com>
…t of ui/mod.rs Move the quit-confirm prompt content into ui/quit_confirm.rs, draw_header into ui/header.rs, and Seg plus draw_footer into ui/footer.rs. ui/mod.rs re-exports quit_confirm_request so the launcher's crate::ui path is unchanged; the new modules are private, so the crate's public surface is unchanged. Tests move by subject, the two helpers shared with a test that stays behind widen to pub(in crate::ui), and the comments that name the moved functions by path now point at the new files. Signed-off-by: Jussi Elo <jussi.elo@amd.com>
…etail.rs Move draw_detail and its render_summary, render_scrollable_pane, render_body and render_footer helpers out of ui/tabs/instances.rs. The new module depends on instances.rs only, for sorted_instances, clamp_sel and status_meta, which stay single-sourced there and widen to pub(in crate::ui); draw_detail is not re-exported from instances.rs, so no cycle appears, and ui/mod.rs dispatches to it directly. The detail tests move with it and reach the shared fixtures through the widened instances test module. The dead card-grid code stays where it is. Signed-off-by: Jussi Elo <jussi.elo@amd.com>
There was a problem hiding this comment.
🟡 Changes recommended
A private import makes four previously public popup helpers inaccessible to library consumers, breaking existing API paths unless the helpers are publicly re-exported.
1 open issue
What changed in this PR
This PR splits dashboard state, event-loop helpers, and rendering into focused modules, reducing overlap between unrelated changes while aiming to preserve existing behavior.
Changes:
- Extract cohesive state and rendering responsibilities, with existing tests colocated beside them.
- Move popup helpers into
ui/panel.rsand navigation constants intoapp/types.rs, updating callers. - Update architecture documentation and comments to reflect the new locations.
Validation here was limited to source inspection; builds and tests were not run.
| File | Description |
|---|---|
tests/e2e-cucumber/tests/e2e/dash_steps.rs |
Update comments naming moved helpers. |
tests/e2e-cucumber/tests/e2e/comfyui_steps.rs |
Update the approval-test location in comments. |
tests/e2e-cucumber/features/dash.feature |
Update test-location comments. |
tests/e2e-cucumber/features/comfyui.feature |
Update the approval-test location in comments. |
docs/architecture.md |
Document extracted modules and responsibilities. |
crates/rocm-dash-tui/src/ui/runtime_manager.rs |
Import popup helpers from panel. |
crates/rocm-dash-tui/src/ui/quit_confirm.rs |
Colocate quit-prompt content and tests. |
crates/rocm-dash-tui/src/ui/panel.rs |
House shared popup helpers and geometry tests. |
crates/rocm-dash-tui/src/ui/onboarding.rs |
Update overlay-gating references in comments. |
crates/rocm-dash-tui/src/ui/model_picker.rs |
Import popup helpers from panel. |
crates/rocm-dash-tui/src/ui/modal.rs |
Delegate geometry and navigation data to shared modules. |
crates/rocm-dash-tui/src/ui/launcher.rs |
Use relocated backdrop helper and update comments. |
crates/rocm-dash-tui/src/ui/job_console.rs |
Import popup helpers from panel. |
crates/rocm-dash-tui/src/ui/header.rs |
Colocate header rendering and badge tests. |
crates/rocm-dash-tui/src/ui/format.rs |
Update a quit-prompt reference in documentation. |
crates/rocm-dash-tui/src/ui/folder_browser.rs |
Import popup helpers from panel. |
crates/rocm-dash-tui/src/ui/approval.rs |
Import popup helpers from panel. |
crates/rocm-dash-tui/src/app/update_check.rs |
Extract background update checks and tests. |
crates/rocm-dash-tui/src/app/types.rs |
House navigation constants and a relocated argument test. |
crates/rocm-dash-tui/src/app/tool_approval.rs |
Extract tool-approval state handling and tests. |
crates/rocm-dash-tui/src/app/test_support.rs |
Share fixtures across relocated tests. |
crates/rocm-dash-tui/src/app/summary.rs |
Colocate the tool-summary test. |
crates/rocm-dash-tui/src/app/scrollbar.rs |
Update rendering-location comments. |
crates/rocm-dash-tui/src/app/scroll.rs |
Extract scroll-state methods and tests. |
crates/rocm-dash-tui/src/app/quit_confirm.rs |
Extract quit-confirmation gates, handlers, and tests. |
crates/rocm-dash-tui/src/app/overlays.rs |
Extract overlay bookkeeping, input gates, and tests. |
crates/rocm-dash-tui/src/app/focus.rs |
Extract focused-host policy and tests. |
crates/rocm-dash-tui/src/app/chat.rs |
Colocate agent-factory and configuration tests. |
crates/rocm-dash-tui/src/app/chat_state.rs |
Extract chat-state methods and tests. |
crates/rocm-dash-tui/src/app/actions.rs |
Use navigation constants from application types. |
apps/rocm/src/main.rs |
Update a startup-focus test reference in comments. |
apps/rocm/src/dash_seam.rs |
Update approval-test references in comments. |
AGENTS.md |
Document the shared popup helpers’ location. |
Review effort: Balanced
jussielo-amd
changed the title
The popup geometry moved into ui/panel.rs, which dropped the four helpers (centered_rect, centered_width, draw_popup_frame, grey_overlay) from their public rocm_dash_tui::ui::modal path. Re-export them from modal.rs so the pre-split paths still resolve; in-crate callers keep importing from panel, so the dependency direction is unchanged. An integration test names them through ui::modal from outside the crate, so dropping the re-export fails to compile. Signed-off-by: Jussi Elo <jussi.elo@amd.com>
|
Re the review summary: it carries one finding, the private import that made the four popup helpers inaccessible at rocm_dash_tui::ui::modal, and it is answered in its inline thread. It is fixed in 7c23b46 by re-exporting them from ui/modal.rs, with an integration test that names them through that path from outside the crate. |
juhovainio
left a comment
juhovainio
left a comment
There was a problem hiding this comment.
Verified the split is behavior-preserving: every removed line has a matching added line across the moved files, no function or test is dropped or duplicated, both the cfg(unix) and cfg(windows) TerminationSignals variants are retained, and the docs and e2e comment pins are updated. Required CI is green. Three non-blocking notes inline.
| use crate::ui::panel::{self, BoxRole}; | ||
| // Re-exported, not just imported: these four were public at this path before | ||
| // the geometry moved into `panel`, so the pre-split public paths stay valid. | ||
| pub use crate::ui::panel::{centered_rect, centered_width, draw_popup_frame, grey_overlay}; |
There was a problem hiding this comment.
The ticket (PR 2) says no re-export from modal.rs so the old path doesn't keep the reversed edge, but this adds one (plus a pinning test). Intentional? If so, consider also re-exporting MENU_ITEMS/PALETTE_DESTS/OPTIONS_TABS, which were public at this path too, or call out in the PR that those paths break.
There was a problem hiding this comment.
Yes, intentional. Copilot pointed out that these four helpers were public at rocm_dash_tui::ui::modal before the split, so dropping the path was an API break the ticket did not ask for; the re-export keeps it, while the in-crate callers still import from ui::panel, so the reversed dependency edge is unchanged. On the menu constants: nothing in the workspace used MENU_ITEMS, PALETTE_DESTS or OPTIONS_TABS at the old path and the ticket places them in app::types, so I left them moved and have now said in the PR description that their old ui::modal paths no longer resolve, and that the re-export is a deliberate exception to the ticket's instruction.
| //! `handle_key`, `apply_action`), `app/scrollbar.rs` (mouse hit-testing), | ||
| //! `app/event_loop.rs` (the tick loop), `app/terminal.rs` (terminal restore, | ||
| //! shutdown latch, signal watcher), `app/focus.rs` (focused-host policy), | ||
| //! `app/update_check.rs` (the Updates-tile check) and `app/summary.rs`. `crate::app::*` paths for the public surface moved out |
There was a problem hiding this comment.
Doc line is overlong after the reflow; wrap it like the rest of the module doc.
There was a problem hiding this comment.
Fixed in 9b17c1b: the line is wrapped like the rest of the module doc, and the sentence about the re-exports (which the reflow had run together with the previous one) is restored to its original wording.
| /// Draws the instance Detail modal and returns the max scroll offset for its | ||
| /// body (see `render_body`), so the caller can write it back to | ||
| /// `AppState::instance_detail_max_scroll`. | ||
| pub(super) fn draw_detail(f: &mut Frame, area: Rect, state: &AppState, theme: &Theme) -> u16 { |
There was a problem hiding this comment.
draw_detail was pub in ui::tabs::instances and is now pub(super). Fine if nothing external used it (PR body says so), but it is an API removal in a pub module, so worth stating as a deliberate non-move change.
There was a problem hiding this comment.
Agreed. The PR description now lists it, together with the three menu constants, as one of two deliberate non-move changes to the public surface: draw_detail was pub in ui::tabs::instances and is now pub(super) in ui/instance_detail.rs, and nothing in the workspace outside the crate used it.
The reflow left one line of the module doc far over the width of the rest of the comment and ran two sentences together. Wrap it like the surrounding lines and keep the sentence about the re-exports intact. Signed-off-by: Jussi Elo <jussi.elo@amd.com>
nowycondro
left a comment
nowycondro
left a comment
There was a problem hiding this comment.
🤖 Automated review (Claude Code) · 9b17c1b
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: all 42 changed files, base 2e5143254e9094a92c0a4ab8ec6583dbcf5cd480 → head 9b17c1bd09bd7485dc7b5573125b59ae982679d9.
Implements PR-steps 1–8 of ROCMAI-758's own documented PR-by-PR plan (a pure, behaviour-preserving split of dash-tui's app/ui modules). PR-step 9 (optionally splitting ui/modal.rs's remaining Modal::* renderers) is correctly deferred to ROCMAI-817, matching a ticket comment that confirms this PR is the one intended to close ROCMAI-758.
Blocking
None
Non-blocking
None
Decisions for the author
ui/modal.rs's own renderers call the four re-exported popup helpers (centered_rect,centered_width,draw_popup_frame,grey_overlay) unqualified rather than via an explicitpanel::prefix — non-blocking-improvement
This compiles and behaves identically either way (thepub use crate::ui::panel::{...}line is still the one source of these names), so it isn't a defect. But the PR's own stated goal for step 2 was that "the in-crate callers still import them fromui::panel" so the reversed dependency direction reads as unchanged;modal.rs's remaining renderers are themselves an in-crate caller, and qualifying their call sites aspanel::centered_rect(...)etc. (rather than relying on the local re-exported binding) would make that invariant visible in the code itself, not just true after name resolution. Worth a follow-up polish pass, not a blocker.
Positive signals
- Eight independent file-coupling review passes, each reading every assigned file completely (plus full before/after content for the two files too large for GitHub's diff API to render —
app/mod.rsandui/mod.rs, fetched directly), found the move byte-for-byte behavior-preserving across roughly 8,000 added/removed lines — including the highest-risk clusters (app/overlays.rs's mutual-exclusion gating, the quit-confirm/tool-approval state machines, andui/mod.rs's render-order invariants for the modal/manager-overlay/approval/quit-confirm stack). - Both the
cfg(unix)andcfg(windows)TerminationSignalsarms in the newapp/terminal.rswere diffed directly against the pre-move code and are symmetric — no platform-specific cleanup step, await, or error path was dropped on either side, which is exactly the failure mode this kind of split risks most. - The declared exception (re-exporting the four popup helpers from
ui/modal.rsfor backward path compatibility, against the ticket's own "no re-export" instruction) is implemented correctly —pub use crate::ui::panel::{centered_rect, centered_width, draw_popup_frame, grey_overlay};is genuinelypuband complete — and is backed by a real regression test (dash_characterization.rs'spopup_helpers_stay_reachable_through_ui_modal) that calls all four functions through theui::modalpath, asserts concrete recomputed geometry, and would fail to compile if the re-export were ever removed — not just a compile-only smoke check. - The PR follows ROCMAI-758's module-naming guardrails precisely, including two explicit "do not name it X" traps the ticket calls out (not naming a module
approval.rs, which would collide withui::approval; not naming onescrollbar.rs, which would trip theactions_does_not_import_from_scrollbarguard test) — both avoided correctly (tool_approval.rs,chat_state.rs/overlays.rs/scroll.rs). - Visibility changes throughout (the
pub→pub(super)narrowing ondraw_detail, theMENU_ITEMS/PALETTE_DESTS/OPTIONS_TABSpath narrowing, everypub(super)/pub(in crate::ui)widening) were each individually traced to a real caller and found neither over- nor under-scoped.
Deployment notes
- CI was still finishing at review time: all 22 other checks are green (
build-and-test,clippy,Coverage (rocm-dash crates, ratcheted),E2E tests/E2E consolidated report,Commit signatures + sign-off, etc.);windows-build-and-testwas stillin_progressat both the first and the final pre-publish check. Given this PR's own stated highest-risk item is the Windowscfgarm ofTerminationSignals, that job finishing green is worth confirming before merge rather than assuming. - The PR description flags that six other open/draft PRs touching these same files (#715, #631, #563, #721, #812, #801) will need rebasing onto the new module layout once this merges — a coordination note for whoever merges this, not a defect here.
mergeable_stateis currentlyblocked(pending review from the requested reviewer and/or the still-running Windows check) — a process gate, not a code issue.
What this covered
Every one of the 42 changed files, read completely (not just diff hunks), fanned out across 8 independent sub-workers grouped by coupling: (1) app/mod.rs+types.rs+actions.rs+scrollbar.rs+summary.rs+test_support.rs; (2) app/event_loop.rs+terminal.rs+focus.rs+update_check.rs; (3) app/quit_confirm.rs+tool_approval.rs; (4) app/chat_state.rs+overlays.rs+scroll.rs+chat.rs+slash.rs; (5) ui/mod.rs+ui/quit_confirm.rs+header.rs+footer.rs; (6) ui/instance_detail.rs+ui/tabs/instances.rs; (7) ui/panel.rs+ui/modal.rs+8 tiny import-path files; (8) docs/architecture.md+AGENTS.md+e2e comment pins+dash_characterization.rs. Base/head diffed directly for every file; app/mod.rs and ui/mod.rs (too large for GitHub's diff API) were fetched in full at both commits and diffed manually.
AGENTS.md was read in full and its relevant rules (re-export discipline, subprocess/FFI checklist, platform-gate test rule, doc-link citation style) were applied as review criteria; no violations introduced by this change were found.
CI was read via get_check_runs (not re-derived from workflow definitions), checked once mid-review and once immediately before publishing.
Ticket ROCMAI-758 was retrieved in full via the Jira MCP tool and used only after findings were settled, per the review method.
Did not run as separate passes: a dedicated git log/git blame history pass, and a prior-review-comments pass (this repository's existing review threads on this PR were deliberately never read, by the independence requirement below).
One candidate finding from an independent sub-worker (a test apparently missing from app/tool_approval.rs) was traced and confirmed to have moved intact into app/slash.rs instead, by reading the actual diff text — not a defect. This is the only consolidation needed across all 8 passes' results.
Not reconciled against this PR's existing review discussion: by design. The only review-list content read was state/commit_id/overview-bot-summary metadata, required by this automation's own mandatory pre-review duplicate-check step; no inline comment thread was read before or during the findings-forming passes above.
🤖 Generated with Claude Code
Resolve conflicts from the dash-tui module split (#827): re-apply this PR's in-process collector header/instances-empty text, HostGpuSnapshot handling and has_received_gpu_snapshot flag in the new file layout. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com> Signed-off-by: juhovainio <juho.vainio@amd.com>
Why
The dash TUI's
app/mod.rs,app/event_loop.rs,ui/mod.rsandui/tabs/instances.rseach hold several unrelated clusters, andapp/mod.rskept about 120 tests in one block, which makes every change to the dashboard collide with every other. This change splits them along the seams the code already has, as pure moves, so each concern (quit confirmation, tool approval, chat state, scrolling, update checks, terminal restore, the header and footer, the instance detail modal) lives in a file of its own with its tests beside it.app/mod.rsis now about 885 lines with about 28 tests. Completes ROCMAI-758; splitting the remaining modal renderers inui/modal.rsis tracked separately as ROCMAI-817.What changes
Nine commits, one per step, so the branch can be reviewed commit by commit:
ui/panel.rsand the menu data intoapp/types.rs, which reverses the dependency direction ofui/modal.rsso the lateruimoves can import geometry without a cycle.app/quit_confirm.rsandapp/tool_approval.rs.app/chat_state.rs,app/overlays.rsandapp/scroll.rs.event_loop.rsintoapp/focus.rsandapp/update_check.rs.cfg(unix)andcfg(windows)signal types, move intoapp/terminal.rs; nocfggates remain inevent_loop.rs.ui/mod.rsintoui/quit_confirm.rs,ui/header.rsandui/footer.rs.ui/instance_detail.rs.ui/modal.rsre-exports the four popup helpers that step 2 moved (centered_rect,centered_width,draw_popup_frame,grey_overlay), so theirrocm_dash_tui::ui::modalpaths still resolve, and an integration test names them through that path from outside the crate. This is a deliberate exception to the ticket's "no re-export frommodal.rs" instruction, made because those helpers were public at that path before the split; the in-crate callers still import them fromui::panel, so the dependency direction that step 2 reversed is unchanged.ui/modal.rsstays one file of renderers. The crate's public surface is unchanged except for two deliberate non-move changes:draw_detail, which waspubinui::tabs::instances, is nowpub(super)in the new module (nothing in the workspace outside the crate used it), and three menu constants (MENU_ITEMS,PALETTE_DESTS,OPTIONS_TABS) that were reachable asrocm_dash_tui::ui::modal::*now live inapp::typesand are re-exported fromapp, so their oldui::modalpaths no longer resolve (nothing in the workspace used them there, and the ticket does not ask for them to be kept);apps/rocmcompiles unchanged.Besides the relocations, the commits make these edits: some items are widened to
pub(super)orpub(in crate::ui)where a new module or a moved test needs them (ininstances.rsthe test module and six shared fixtures);config_with_chat_sets_local_endpoint_and_clears_authmoves to theapp/chat.rstests because it exerciseschat::config_with_chat; one import that only acfg(unix)test needs is itself gated; the module doc that calledapply_actionan impl inactions.rsnow says it is a free function; comment pins inapps/rocm, the e2e features and steps, and severaluimodules name the new files; andAGENTS.md'spanel.rsbullet anddocs/architecture.md(an entry per new file) are updated.Verification
cargo fmt --checkandcargo clippy -p rocm-dash-tui --all-targets -- -D warningsare clean, andcargo check -p rocmpasses after the rebase onto currentmain.cargo xtask manifest --checkandcargo xtask check-crate-edgespass.tests/e2e-cucumbersuite, the coverage floors and the Windows lane. Thecfg(windows)signal code that moved toapp/terminal.rswas type-checked forx86_64-pc-windows-msvcin a scratch crate, including its tests, with no warnings, but nothing runs the Windows tests locally.Scope
This is an internal refactor with no user-observable change, so no end-to-end scenario is added; the unmodified
tests/e2e-cucumbersuite covers the unchanged behaviour. Splitting theModal::*renderers inui/modal.rsinto several files is deliberately left out and tracked under its own ticket, ROCMAI-817. Open PRs that edit the same files (#715, #631, #563, #721 and the drafts #812 and #801) will need rebasing onto the new layout.Template checklist: not a bug fix, so there are no stale xfail rows to narrow; no new subcommand or subsystem; no user-facing message is added or changed.