ROCMAI-84: Modularize crates/rocm-dash-tui/src/app/mod.rs

[jussielo-amd]

Summary

• Splits app/mod.rs (8,967 lines) into an app/ directory following the modularization effort's full-domain-extraction convention:
  • types.rs — shared type/enum defs (Focus, ResolvedArgs, connection/tab/chat/replay state, Modal, UpdateStatus, slash/plan/approval payload types). No AppState access.
  • event_loop.rs — terminal lifecycle, signal handling, and the tick loop (run, event_loop, the termination-signal watcher, startup-focus/Updates-tile tick helpers).
  • scrollbar.rs — mouse/scroll hit-testing: resolving a raw MouseEvent against recorded scrollbar tracks, the tab bar, and footer-legend chips into a KeyAction.
  • actions.rs — KeyAction dispatch: handle_key/handle_mouse/tab_bar_hit, apply_action/run_approved reducer glue.
  • mod.rs keeps only AppState and its apply_event-adjacent reducer impl — "the reducer's reason to exist," per the ticket.
• Pure code motion — no behavior change. crate::app::* paths for the module's public surface are preserved via re-exports from mod.rs, so ui/, apps/rocm, and the dash e2e/example targets that reference them are unaffected.
• Updates docs/architecture.md's module map in this same PR (not deferred), per the modularization plan's documentation rule.
• Why: part of the rocm-cli modularization plan (epic ROCMAI-27, Phase 4) to make large, monolithic source files easier to navigate and review.

Design decisions

• mod.rs's re-exports preserve crate::app::* for the module's public surface — the scope the ticket's acceptance criteria and "no behavior change" actually cover (AppState's public surface used by ui/, plus anything externally reachable). They do not preserve incidental reachability of pub(crate) items that have no caller through crate::app:: — a pub(crate) symbol with zero current callers on that path can't silently break anyone if the path disappears (any real in-crate need would be a compile error, not a runtime regression), so there's nothing for parity to protect there. handle_mouse/tab_bar_hit are the converse case: real pub surface a downstream consumer of the crate could depend on, which is why those got re-added per Copilot's review.

Notes

• Before starting, re-verified the cluster's actual line boundaries against current main by grep/structure rather than trusting the ticket's (stale, pre-drift) line numbers — the file had grown from 6.7k to 8,967 lines since epic-authoring time.
• The original's 208 #[test] + 1 #[tokio::test] functions (209 total) were partitioned across the 5 files by what they actually exercise (verified against call-sites), not by position. Test count is unchanged.
• Sequencing: the epic sequences this (Phase 4) after Phase 3 (agent.rs, ROCMAI-51) to avoid two large concurrent diffs on this crate. ROCMAI-51 is still in progress, so this PR is landing slightly out of the stated order — flagging for awareness; there's no code dependency between the two, just review/merge-overlap risk.

Test plan

• cargo check --workspace --all-targets — clean.
• cargo clippy --workspace --all-targets -- -D warnings — clean.
• cargo test --lib (rocm-dash-tui) — 804 passed, 0 failed, 4 ignored; app-module test count (209) matches pre-refactor exactly.
• cargo xtask manifest --check — passed.
• prek hooks (fmt, license headers, etc.) — all passed.
• Confirmed AppState's public surface used by ui/ views is unaffected (workspace build covers all consumers, including apps/rocm, the dash e2e tests, and the gen_cast/gen_screenshots examples).

[Copilot]

Copilot review overview

🟡 Changes recommended

The extraction unintentionally removes two public API paths and introduces an unresolved documentation link.

Review effort: Balanced
Findings: 1 · 1

Open (2)

• Re-export handle_mouse and tab_bar_hit to preserve public API · New
• Qualify ClientMsg intra-doc link after module extraction · New

What changed in this PR

Modularizes the dashboard’s monolithic application module while preserving its reducer architecture and intended public API.

Changes:

• Extracts shared types, event-loop logic, input actions, and scrollbar handling.
• Retains AppState and reducer logic in mod.rs.
• Updates the architecture documentation.

File	Description
docs/​architecture.md	Documents the new module structure.
app/​mod.rs	Retains reducer state and re-exports extracted APIs.
app/​types.rs	Contains shared application types.
app/​event_loop.rs	Contains terminal lifecycle and event processing.
app/​scrollbar.rs	Contains mouse and scrollbar hit-testing.
app/​actions.rs	Contains input translation and action dispatch.

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[rominf]

🔴 Automated review · pr-review-watcher · 04289b0

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.

Summary

Since 94e251d, this PR has fixed three comments that still pointed at the old app/mod.rs location after the split (ROCMAI-84). Each now names the right file. Elsewhere in app/mod.rs one dead re-export remains, and a few stale claims about where things live survive nearby. Outcome: Needs work.

Reviewed: commit range 94e251d..04289b0 (apps/rocm/src/main.rs, crates/rocm-dash-tui/src/agent/mod.rs, crates/rocm-dash-tui/tests/dash_journeys.rs), plus all of crates/rocm-dash-tui/src/app/mod.rs, compared against prw-base. Also read: the headers and imports of app/{types,event_loop,scrollbar,actions}.rs, ui/launcher.rs, ui/onboarding.rs, apps/rocm/src/dash_seam.rs, docs/architecture.md, xtask/src/architecture_doc.rs, the comfyui/dash .feature files and comfyui_steps.rs. Earlier rounds' non-blocking findings outside these files were not re-examined.

Previous round's blockers: the stale-path blocker (main.rs, dash_journeys.rs, agent/mod.rs) was re-reviewed in this round and not re-found — those files were in scope. The NO_CHAT_BACKEND_MSG re-export blocker was re-derived independently below.

Verified: in rocm-dash-tui, all integration tests pass (20, 5 and 5) and the lib tests pass 803 of 806. The 3 failures are in agent/clients.rs, which this PR does not touch, and all fail with "cannot create token cache dir: Permission denied" from this sandbox. cargo clippy -p rocm-dash-tui -p rocm --all-targets -D warnings (run after touching the sources) and cargo xtask check-architecture-doc both pass. The app test count is unchanged across the split (211 before and after). Each new path checked out: startup_focus_gate_only_opens_onboarding_for_explicit_setup_focus is at event_loop.rs:2118; the tests that call handle_key are only in actions.rs; app/mod.rs does re-export its submodules' items. The other test-name references in dash_seam.rs, comfyui.feature, dash.feature, comfyui_steps.rs and onboarding.rs still resolve to app/mod.rs. The split itself is a faithful move: the AppState struct and impl are byte-identical, and every relocated test is unchanged.

Blocking: 1 · Non-blocking: 6.

🚫 Blocking (must fix before merge)

crates/rocm-dash-tui/src/app/mod.rs:50-54 — The re-export pub(crate) use types::NO_CHAT_BACKEND_MSG; is dead code, and #[allow(unused_imports)] hides that. Its own comment says nothing calls it through crate::app::: the callers (event_loop.rs:42, and the test at mod.rs:1243) import it from super::types. The "parity with crate::app::*" reason does not hold for a pub(crate) item. Nothing outside the crate can reach it, and inside the crate the compiler already proves every caller resolves, so there is nothing for the re-export to protect. The fix is to delete the comment, the #[allow] and the use line.

Non-blocking

• crates/rocm-dash-tui/src/app/mod.rs:5-6 — The module doc says apply_action is an entry point in this file. It is actually defined in actions.rs:61 and is not re-exported from mod.rs; only apply_event (mod.rs:1220) lives here.
• docs/architecture.md:42 — Same wrong claim: "app/mod.rs keeps only AppState and its apply_event/apply_action-adjacent reducer impl". check-architecture-doc only checks file paths, not which item lives where, so CI will not catch it.
• crates/rocm-dash-tui/tests/dash_journeys.rs:16 — This commit edited the line but left "module-private". handle_key and apply_action are now pub(crate) in actions.rs (they were private before the split), so "crate-private" is the accurate word.
• crates/rocm-dash-tui/tests/dash_journeys.rs:17 — The comment points only to actions.rs. That is right for handle_key, but apply_action is also tested in scrollbar.rs (11 mouse-driven call sites), and the comment's "handle_key/apply_action" pairing reads as if both live only in actions.rs.
• crates/rocm-dash-tui/src/app/mod.rs:43-44 — restore_after_session is re-exported only for a test (launcher.rs:526, inside #[cfg(test)]). Gating it with #[cfg(test)] would say exactly that; #[allow(unused_imports)] would also hide a future loss of the caller.
• crates/rocm-dash-tui/src/app/mod.rs:23 — The comment says the split keeps "the core reducer + event loop focused", but the event loop now lives in event_loop.rs. The same stale phrase appears in the module headers of actions.rs, scrollbar.rs and types.rs.

[rominf]

🔴 Automated review · pr-review-watcher · 94e251d

Requesting changes for the two blocking findings in this round's review comment:

1. Comments in other files still point at the old location. apps/rocm/src/main.rs:29733 names app/mod.rs as the home of startup_focus_gate_only_opens_onboarding_for_explicit_setup_focus, but that test is now in app/event_loop.rs. crates/rocm-dash-tui/tests/dash_journeys.rs:16-17 names src/app/mod.rs for the keystroke tests, which are now in src/app/actions.rs. crates/rocm-dash-tui/src/agent/mod.rs:21-26 describes app as not re-exporting its submodules, which it now does. To resolve: correct the two paths and reword the agent/mod.rs sentence.
2. crates/rocm-dash-tui/src/app/mod.rs:50-54 adds the pub(crate) use types::NO_CHAT_BACKEND_MSG re-export. Nothing reaches it through that path, and it only compiles cleanly because of #[allow(unused_imports)]. To resolve: delete the re-export and its allow.

The automation withdraws this request itself once a later round confirms both points are fixed.

[jussielo-amd]

Addressing the two blocking findings from the automated review round (commit 94e251d):

1. Fixed in adea8ed — corrected the three stale path references: apps/rocm/src/main.rs and tests/dash_journeys.rs now point at app/event_loop.rs and app/actions.rs respectively (where those tests actually live post-split), and reworded agent/mod.rs's module doc, which previously contrasted itself with app/mod.rs's submodules being "private" — app/mod.rs now re-exports its submodules' public items too, so that contrast was no longer accurate.

2. Kept intentionally, not fixed — pub(crate) use types::NO_CHAT_BACKEND_MSG (and its #[allow(unused_imports)]) stays. It's unreachable through crate::app:: today because callers use super::types:: directly, but it was reachable through that path pre-split, and the split's explicit goal is that every crate::app::* path that resolved before keeps resolving after — the same reasoning that restored handle_mouse/tab_bar_hit per Copilot's earlier review. Removing it the moment it goes locally-unused would make the re-export list's completeness depend on caller behavior rather than on the pre-split surface. Documented this in the PR description's new "Design decisions" section.

[rominf]

Re the reply above, on the two blocking findings from the 94e251d round:

Finding 1 (stale path references) — confirmed fixed at 04289b0. apps/rocm/src/main.rs now names app/event_loop.rs (the test is at event_loop.rs:2118), tests/dash_journeys.rs names src/app/actions.rs, and the agent/mod.rs doc no longer contrasts itself with app.

Finding 2 (pub(crate) use types::NO_CHAT_BACKEND_MSG under #[allow(unused_imports)]) — your facts check out, and the finding still stands.

• At the base, NO_CHAT_BACKEND_MSG was pub(crate) in app/mod.rs (line 397), so crate::app::NO_CHAT_BACKEND_MSG did resolve before the split — although nothing in the crate used that path then either.
• Parity is complete: every module-level pub/pub(crate) item from the old app/mod.rs still resolves at crate::app::<name>, and the app/mod.rs header and the PR description both state that rule.
• The difference from handle_mouse/tab_bar_hit: those are plain pub use in a pub mod, so they are public surface that code outside this crate could depend on, and parity protects something there. NO_CHAT_BACKEND_MSG is crate-private. Any in-crate caller is compiled together with this module, so a path that disappears can't break anyone silently, and the re-export protects no caller. It needs the lint turned off to compile without a warning.

This round's review re-derived the same finding from the code, so it stays blocking and the change request stays in place for this point only. If you and the maintainers decide parity should also cover crate-private paths, a human reviewer can make that call. Otherwise, deleting the comment, the #[allow] and the use line clears it.

Withdrawing: objection 1 (stale path comments) is fixed at 04289b0. Objection 2 (the documented crate-private NO_CHAT_BACKEND_MSG re-export under allow(unused_imports)) is a style preference with no consequence after merge, so it stays a non-blocking comment rather than a gate.

[jussielo-amd]

Fair — the public/crate-private distinction is correct and I was conflating two different kinds of parity. Fixed in a647de4: removed the pub(crate) use types::NO_CHAT_BACKEND_MSG re-export, its comment, and the #[allow(unused_imports)].

Also narrowed the architecture doc and PR description's "Design decisions" section so this doesn't recur: the parity guarantee is scoped to the module's public surface (what the ticket's acceptance criteria actually requires), not incidental pub(crate) reachability with zero callers on that path. cargo check/cargo clippy --all-targets -- -D warnings clean on rocm-dash-tui after the removal.