rocm-dash-tui: untangle the scrollbar.rs/actions.rs cycle

[jussielo-amd]

Summary

• actions.rs's KeyAction dispatch imported PaneFocus/ScrollTarget from scrollbar.rs (KeyAction::ScrollGrab(ScrollTarget, ...) needs the type), while scrollbar.rs's mouse hit-testing imported KeyAction/handle_mouse/tab_bar_hit/apply_action back from actions.rs — a true cycle despite "hit-testing geometry" vs. "key dispatch" reading like a clean layer split.
• PaneFocus and ScrollTarget move to app/types.rs, already the shared home for every other pure type/enum this app/ split uses (Focus, ActiveTab, Modal, etc.).
• actions.rs and scrollbar.rs now both depend on types.rs for these two types instead of on each other. scrollbar.rs's existing one-directional use of actions.rs (KeyAction, handle_mouse, tab_bar_hit) is untouched and is now the only direct use-level edge between the two modules — actions.rs still reaches ScrollDrag indirectly, through the scroll_drag field it types on AppState (defined in mod.rs; actions.rs never touches the ScrollbarHandle/FooterChip-typed fields), which this move doesn't touch.

This is one of two separate instances of the same architectural pattern tracked in ROCMAI-483 (the other is engines/vllm's process.rs/state.rs cycle, in a separate PR, #593).

Behavior preservation

Pure move — same variants, same derives, same call sites, same field types throughout AppState/KeyAction. No logic changed.

Test plan

• cargo test -p rocm-dash-tui --all-targets — all passing (includes a guard test, actions_does_not_import_from_scrollbar, asserting actions.rs never reintroduces a use-level dependency on scrollbar.rs — the compiler accepts a module-level cycle here, so nothing else would catch it coming back. The test finds every use item in actions.rs by keyword — use has no other syntactic role in Rust — and reads forward, brace-depth aware, to its own terminating ;, so it catches a use naming the scrollbar module or the three names app/mod.rs re-exports from it (ScrollbarHandle, ScrollDrag, FooterChip) in any position or form — top-level or function-local, attribute-gated (e.g. #[cfg(test)]), pub/pub(crate), brace-grouped, or rustfmt-wrapped across multiple lines — without false-positiving on an unrelated identifier like vertical_scrollbar. A positive control in the test asserts the detector actually flags a known-bad case. It does not catch a fully-qualified path referenced inline with no use statement at all, or a use named only inside a /* */ block comment; both are known, accepted scope limits of a source scan rather than a parser)
• cargo clippy -p rocm-dash-tui --all-targets -- -D warnings — clean
• cargo fmt -p rocm-dash-tui --check — clean
• cargo build --workspace — clean, no downstream breakage
• No scenario-level regression test needed: this is a non-behavior-changing type relocation, not a bug fix.

[rominf]

🔴 Automated review · pr-review-watcher · b379786

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.
This PR covers Instance 2 of ROCMAI-483 (dash-tui scrollbar.rs ↔ actions.rs). Instance 1 (vllm process.rs ↔ state.rs) is left to #593, as the description says. The PR does not claim to close the ticket.

Blocking

None

Non-blocking

• The new PaneFocus doc comment gives a reason that is false — crates/rocm-dash-tui/src/app/types.rs:440-442 (new)
  The comment says the type is "mouse-set there too" (in scrollbar.rs). It also says it lives in types.rs so that "scrollbar.rs's hit-testing can … depend on this type". But scrollbar.rs does not mention PaneFocus anywhere, at either the base or the head (grep: 0 hits other than the removed definition). The mouse wheel path in scrollbar.rs only returns KeyAction::Move. pane_focus is written only in actions.rs (68–119) and event_loop.rs:1143. The code is right and the comment is wrong. The real reason is that PaneFocus was defined in scrollbar.rs without being used there, so actions.rs had to import it from scrollbar.rs. The ScrollTarget comment at types.rs:454-457 is accurate, because scrollbar.rs really does use ScrollTarget.
  Confidence 85 · mechanical · new · Fix: say that PaneFocus was moved out of scrollbar.rs because that file defined it but never used it, and the move removes actions.rs's only import from scrollbar.rs. Drop "mouse-set there too".
• The PR description and the commit message get the cycle's direction backwards — PR body, first bullet; commit b379786, first paragraph
  Both say "scrollbar.rs's mouse hit-testing imported PaneFocus/ScrollTarget from actions.rs". The base tree shows the reverse. On the base, scrollbar.rs:228 defines PaneFocus (and ScrollTarget), and actions.rs:14 is use super::scrollbar::{PaneFocus, ScrollTarget};. scrollbar.rs imports only KeyAction, handle_mouse, tab_bar_hit and apply_action from actions.rs (lines 16–17). The base code governs here, and ROCMAI-483 describes the edges the same way the code does. The description of what the change does (move two types to types.rs, leave scrollbar.rs→actions.rs as the only edge) is accurate, so this is a wrong account of the starting state, not a wrong scope. It still matters: the repo squash-merges, so this text is likely to end up in the permanent history.
  Confidence 85 · mechanical · new · Fix: swap the two directions in the first sentence of both texts.
• Two lists of what types.rs holds now leave out the two moved types — crates/rocm-dash-tui/src/app/types.rs:5-8, docs/architecture.md:42
  Both list what types.rs contains ("Focus, ResolvedArgs, connection/tab/chat/replay state, Modal, UpdateStatus, and the slash/plan/approval payload types"). Neither includes the pane-focus and scroll-target types this PR moves in. AGENTS.md §5 asks docs to be updated in the same change.
  Confidence 70 · mechanical · new · Fix: add the two types to both lists, or reword the lists so they don't try to name every type.

Decisions for the author

None

Positive signals

• The crate::app::{PaneFocus, ScrollTarget} re-exports are kept in app/mod.rs. As a result, ui/mod.rs, ui/job_console.rs and ui/tabs/instances.rs compile unchanged, and nothing outside app/ sees the move.
• Afterwards actions.rs does not refer to scrollbar.rs at all (grep confirms). apply_scroll_grab lives in mod.rs, not scrollbar.rs, so there is no hidden dependency through a method either. The cycle is actually gone at the module level, not just moved.

Deployment notes

None

What this covered

Every file in the change was read completely: actions.rs, event_loop.rs, mod.rs, scrollbar.rs and types.rs under crates/rocm-dash-tui/src/app/. Also checked: every reference to PaneFocus, ScrollTarget and scrollbar:: across crates/ and apps/, the base versions of scrollbar.rs and actions.rs, and docs/architecture.md. Commit range: 2023d76…b3797863 (one commit).

Verification was static only, because of disk constraints on the reviewing host. No cargo build, check, clippy or tests were run. Compile and test correctness rests on CI, which is green on b379786: build-and-test, windows-build-and-test, clippy, Test (affected crates), Coverage (rocm-dash crates), E2E tests and Commit signatures all passed. The self-hosted GPU E2E lanes, skillscope and Sphinx were skipped.

The diff adds and changes no tests, so there were no per-test verdicts to give. This is a pure type move with no change in behaviour, so no scenario is needed (AGENTS.md §3 exempts internal refactors).

What did not run:

• The diff is 44 lines, so the code review was a single pass rather than a fan-out.
• The agent-instruction, history and code-comment passes and the design questions ran in the same context, so they were not independent.
• The prior-changes pass did not run: no review comments from earlier PRs on these files were read.

The PR's discussion was not reconciled here.

[jussielo-amd]

Addressed all three non-blocking findings in aec0d01:

• PaneFocus's doc comment on types.rs:440-442: fixed to state the real reason for the move (scrollbar.rs defined PaneFocus but never used it; the false "mouse-set there too" / hit-testing-depends-on-it claims are gone).
• The two "what types.rs holds" lists (its own module doc comment, docs/architecture.md): both now include PaneFocus/ScrollTarget.
• PR description's first bullet had the import direction backwards (said scrollbar.rs imported from actions.rs, when the base tree has it the other way): corrected in the description.

Left the already-pushed commit message's wording as-is rather than amending/force-pushing an open PR; the corrected PR description is what the squash-merge record should pick up.

[rominf]

🔴 Automated review · pr-review-watcher · aec0d01

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.

Watcher note: no change request is filed on this round. The one finding below is about doc-comment wording. Merging it would leave an inaccurate rationale in a comment, and it would change no behaviour. The previous round rated the same class of finding non-blocking, so this round publishes as a comment. The finding is new this round. It is in the text the fix commit added.

Review — needs work

Full review of the whole change.
Covers instance 2 of ROCMAI-483 (the scrollbar.rs/actions.rs cycle in rocm-dash-tui). It does not cover instance 1 (the vllm process.rs/state.rs cycle); the description says that one is in #593.

Blocking

• (new) The new doc comments on PaneFocus and ScrollTarget misdescribe the dependency structure this PR exists to fix — crates/rocm-dash-tui/src/app/types.rs:441-443, crates/rocm-dash-tui/src/app/types.rs:456-459
  • ScrollTarget says it lives in types.rs so both modules can see it "without either depending on the other". That is false: scrollbar.rs still depends on actions.rs (scrollbar.rs:17 use super::actions::{KeyAction, handle_mouse, tab_bar_hit};, plus apply_action under cfg(test) at :16). The PR description says the same thing: that edge "is now the only edge between the two modules".
  • ScrollTarget also says it lives here "for the same reason as [PaneFocus]". The reason given for PaneFocus is that scrollbar.rs "defined this type but never used it". scrollbar.rs uses ScrollTarget throughout (:18, :23-38, the ScrollDrag/ScrollbarHandle fields), so that reason does not carry over.
  • PaneFocus says moving it "removes actions.rs's only import from scrollbar.rs". At base, that import was use super::scrollbar::{PaneFocus, ScrollTarget};, so moving PaneFocus alone leaves it in place. It only disappears because both types moved.
  • The code governs here, since the use lines are what the compiler acts on. These comments are the deliverable of the "fix stale docs" commit (aec0d01). They are also the only in-code statement of the new module boundary, so the next reader of a cycle-untangling change gets the wrong direction from them.
  • Confidence 85 · mechanical · Fix: say the shared home lets actions.rs stop depending on scrollbar.rs, and that scrollbar.rs → actions.rs remains the single edge. Give ScrollTarget its own reason (KeyAction::ScrollGrab carries it). For PaneFocus, drop the "only import" clause or say "together with ScrollTarget".

Non-blocking

None

Decisions for the author

• ScrollTarget now lives apart from ScrollDrag and ScrollbarHandle, which embed it — tradeoff
  • For: types.rs is already the home for shared pure enums, and putting ScrollTarget there is the smallest move that breaks the cycle.
  • Against: docs/architecture.md describes app/ as following full domain extraction. The scrollbar-domain type now sits away from the module that defines the scrollbar state carrying it.
  • Either is defensible. The ticket calls this "a real ownership/design decision", so confirm it was deliberate.
• Nothing prevents the cycle from coming back — non-blocking-improvement
  • The ticket's failure scenario is "no compiler-enforced boundary". cargo xtask check-crate-edges checks only edges between crates, and the compiler accepts cycles between modules, so the next use super::scrollbar::… in actions.rs would go through silently.
  • Options: a short guard test that greps actions.rs for super::scrollbar, or an explicit "must not import scrollbar.rs" line in actions.rs's module doc, would make the boundary stick.

Positive signals

• Both types stay pub and still resolve through the crate::app:: re-export block (mod.rs:51-55). Every outside consumer (ui/*, tests/dash_characterization.rs) compiles unchanged, and the re-export rule from #476 holds.
• types.rs's module doc and docs/architecture.md were updated in the same change, as AGENTS.md §5 asks.

Deployment notes

None

What this covered

• What was read:
  • All 6 changed files in full (app/{actions,event_loop,mod,scrollbar,types}.rs, docs/architecture.md), at a5e507a (merge-base with prw-base)…aec0d01c.
  • A repo-wide grep for every reference to PaneFocus, ScrollTarget and scrollbar.
  • Base scrollbar.rs, to check the "never used PaneFocus" claim (true).
  • History and earlier-merged-PR passes (#476, #62, #92), run by an independent read-only worker.
• What was run locally:
  • cargo clippy -p rocm-dash-tui --all-targets -D warnings is clean, and cargo fmt --check is clean.
  • cargo test -p rocm-dash-tui passes in full under HOME=/tmp/fakehome. Without that, 3 agent::clients tests fail on a root-owned $HOME in this container, a known environment problem unrelated to this change.
• CI: every check on aec0d01 is green. GPU E2E lanes and the skillscope/Sphinx checks were skipped by path filtering.
• What did not run:
  • The code pass and the design questions (Step 7) ran in the driver context, not as independent workers. The change is a 6-file type move, so this was a single pass.
  • Agent-instruction adherence was checked by the driver against AGENTS.md: no Gherkin scenario is needed for a pure refactor, and the description says why.
  • No added-test verdicts, because the change adds no tests.
  • The commit messages were seen during scope resolution, before Step 9.
• Not reconciled against any PR discussion here.

[jussielo-amd]

Addressed the blocking finding from the 10:38:56Z review, in 03b7698:

• ScrollTarget's doc comment claimed neither module depends on the other after the move — false, scrollbar.rs still imports KeyAction/handle_mouse/tab_bar_hit from actions.rs by design, and that's the one edge this PR's description says stays. Reworded to say actions.rs stops depending on scrollbar.rs, and named the remaining reverse edge explicitly instead of implying it's gone.
• ScrollTarget's "same reason as PaneFocus" was also wrong, since scrollbar.rs does use ScrollTarget (unlike PaneFocus, which it defined but never used). Gave it its own reason: KeyAction::ScrollGrab carries one, so it has to be visible to actions.rs.
• PaneFocus's doc overstated itself as removing actions.rs's import from scrollbar.rs alone — at base that was a single use super::scrollbar::{PaneFocus, ScrollTarget}; line, so only moving both together removes it. Reworded to say so.

On the two "Decisions for the author" items from that same review:

• ScrollTarget living apart from ScrollbarState/ScrollbarHandle: confirmed deliberate. types.rs is already the shared home for every other pure type/enum in this split, and this is the smallest move that breaks the cycle.
• Nothing stopped the cycle from re-forming: added a guard test (actions_does_not_import_from_scrollbar in app/mod.rs) asserting actions.rs's source never contains an import from scrollbar.rs. The compiler accepts a module-level cycle here, so this is the cheapest thing that would actually catch a future use super::scrollbar::... landing in actions.rs.

cargo test -p rocm-dash-tui --all-targets, cargo clippy -p rocm-dash-tui --all-targets -- -D warnings, and cargo fmt -p rocm-dash-tui --check all pass locally on 03b7698.

[rominf]

🔴 Automated review · pr-review-watcher · 03b7698

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.

Watcher note: no change request is filed on this round. The one blocking-rated finding below is new: the guard test added in 03b7698 checks less than its name, assertion message and the PR description say. Merging leaves a guard that catches the super::scrollbar spelling and misses others. Base has no guard at all, and no runtime behaviour changes. So this round publishes it as a comment. The objection the previous round raised about the PaneFocus/ScrollTarget doc comments was standing. This round's blind review did not re-find it, and it was then put to the reviewer, which found that all three parts are fixed at 03b7698. It is dropped. Separately, GitHub currently reports this PR as conflicting with main (mergeable_state: dirty). It needs a rebase before it can merge.

Review — needs work

Full review of the whole change.
This PR does instance 2 of ROCMAI-483 (the scrollbar.rs/actions.rs cycle). Instance 1 (vllm process.rs/state.rs) is not in it; the description points to #593 for that. The PR does not claim to close the ticket.

Blocking

• (new) The new guard test is weaker than its name, comment and the PR description say — crates/rocm-dash-tui/src/app/mod.rs:1246-1258
  • The test is named actions_does_not_import_from_scrollbar and fails with "actions.rs must not import from scrollbar.rs". The PR description says it asserts that actions.rs "never reintroduces an import from scrollbar.rs".
  • What it actually checks is narrower: whether actions.rs contains the substring super::scrollbar.
  • I added use crate::app::scrollbar::ScrollDrag; to actions.rs and ran cargo test -p rocm-dash-tui --lib actions_does_not_import_from_scrollbar. It passed (1 passed, exit 0).
  • A worker also confirmed that use super::{scrollbar::FooterChip}; and use super::ScrollbarHandle; (through the mod.rs re-export) both pass. All three bring the dependency on scrollbar.rs back.
  • The test comment says "nothing else catches it", so readers will take the dependency direction as protected when the common spellings are not covered.
  • The needle can also false-positive: a later comment in actions.rs that mentions super::scrollbar would fail the test. I found this by reading the needle, not by running it.
  • Confidence 100 · logic · Fix: either harden the check or narrow the claims. Hardening means a line-based scan of non-comment use lines for the scrollbar path segment, plus the names ScrollbarHandle, ScrollDrag, FooterChip and resolve_mouse. The repo already does a line-based scan like this in no_checker_hand_sets_auto_applicable (crates/rocm-core/src/diagnose.rs:2866). Narrowing means changing the test name, comment and description to say exactly what is checked.

Non-blocking

• (new) The first commit message describes the dependency backwards — commit b379786
  • It says "scrollbar.rs's mouse hit-testing imported PaneFocus/ScrollTarget from actions.rs". On prw-base both types were defined in scrollbar.rs, and scrollbar.rs never used PaneFocus.
  • It also says each module now depends on types.rs "instead of on each other for anything", then a few lines later says the scrollbar.rs → actions.rs edge remains.
  • The two later commits mostly exist to correct the first one's docs.
  • The repo squash-merges, so the fix is to write the squash message fresh rather than concatenating these three.
  • Confidence 85 · mechanical · Fix: write an accurate squash commit message.

Decisions for the author

• Where the guard lives — tradeoff
  • It sits in app/mod.rs's tests, but the invariant belongs to actions.rs.
  • Putting it in actions.rs's own test module, as diagnose.rs does for its self-scan, keeps it next to the code it protects.
  • Keeping it in mod.rs keeps the layering checks for the app/ split in one place.
• What "cycle removed" means here — non-blocking-improvement
  • The direct use edge is gone. actions.rs still reaches scrollbar.rs types indirectly through AppState, whose fields are ScrollbarHandle, ScrollDrag and FooterChip (app/mod.rs:126,129,199). That is unavoidable while AppState lives in mod.rs.
  • Calling it a "one-way import rule" rather than "no cycle" would describe the guarantee more exactly.

Positive signals

• The crate::app::* re-exports are kept: PaneFocus and ScrollTarget are re-exported from types in app/mod.rs. So every external user resolves unchanged: ui/dock.rs, ui/tabs/chat.rs, ui/tabs/instances.rs and tests/dash_characterization.rs.
• docs/architecture.md and the types.rs module doc were updated in the same change.

Deployment notes

None

What this covered

• Read: every file in the change, completely: app/{actions,event_loop,mod,scrollbar,types}.rs and docs/architecture.md. Also every repo-wide reference to PaneFocus and ScrollTarget, and the identifiers defined in scrollbar.rs checked against actions.rs. Range: prw-base…03b76983.
• Ran:
  • cargo check -p rocm-dash-tui --all-targets (exit 0).
  • The guard test against three mutations of actions.rs. I re-ran one myself.
  • The leak scan over the diff (clean).
  • A check of DCO sign-off and signatures on all three commits (present and good).
• CI on the head commit: every check that ran passed. The GPU E2E lanes, Sphinx and skillscope were skipped.
• Not run:
  • A full workspace build or test. Only the rocm-dash-tui crate was compiled here; CI build/test lanes passed.
  • The mutation for the comment false-positive. It timed out in the worker, so that point rests on reading the needle.
• Process: fanned out as a code pass, a source-reading pass (instructions, history, prior changes, comments) and a design pass. All three ran independently.
• Discussion: not reconciled against the PR's discussion here.

[jussielo-amd]

Rebased onto origin/main (78da228) to pick up #582's rewrite of docs/architecture.md into markdown-linked citations. One conflict, in the `rocm-dash-tui`/`rocm-dash-core`/... module-map entry: main's rewrite converted every file citation in that paragraph to a link, while this branch's `aec0d01c` had added `PaneFocus`/`ScrollTarget` to the `app/types.rs` type list in the pre-link prose. Resolved by keeping main's link style and folding the two type names into it.

`cargo test -p rocm-dash-tui --all-targets`, `cargo clippy -p rocm-dash-tui --all-targets -- -D warnings`, and `cargo fmt -p rocm-dash-tui --check` all pass clean on the rebased head.

[rominf]

🔴 Automated review · pr-review-watcher · 78da228

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.

Watcher note: no change request is filed on this round. The one blocking-rated finding below is standing. The previous round (at 03b7698) raised the same guard-test gap and published it as a comment, not a gate. This round re-found it blind, at the same location and with the same mutation result. Merging still leaves the same thing: the production change is correct, and the guard catches the direct super::scrollbar spelling and misses re-exported or brace-grouped spellings. No runtime behaviour is at stake, so it stays non-gating. The commit-message finding is also standing. The other two findings and the decision item are new. The conflict with main reported last round is gone.

Review — needs work

Full review of the whole change.
The description matches the diff. ROCMAI-483 is named as a bare ID and was not retrieved: no tracker tool was available here. The description says this PR is one of two halves of ROCMAI-483; the other half is #593.

Blocking

• (standing) The new guard test does not cover what its comment and the description promise — crates/rocm-dash-tui/src/app/mod.rs:1246-1258
  The test comment and the PR's test plan both say the test asserts that actions.rs "never reintroduces an import from scrollbar.rs". It does not. The test is a raw substring search for super::scrollbar, so any other spelling of the same dependency passes it.

  • Confirmed by running it: I added use super::ScrollbarHandle; to actions.rs and ran cargo test -p rocm-dash-tui --lib actions_does_not_import_from_scrollbar. It reported 1 passed, exit 0. ScrollbarHandle, ScrollDrag and FooterChip are all defined in scrollbar.rs and re-exported from app/mod.rs:51.
  • Also confirmed by a worker, with the same result: use crate::app::scrollbar::resolve_mouse, use super::{scrollbar::ScrollDrag} and use super::{scrollbar, AppState} all compile and all pass the test.
  • The re-exported path is the likeliest way the cycle comes back. Whoever next needs a scroll type in actions.rs will naturally write super::ScrollbarHandle.
  • The search also scans comments and actions.rs's own test module, so a harmless comment can fail it.
  • The test is still worth having: reverting the production change puts back use super::scrollbar::{PaneFocus, ScrollTarget}, which the test does catch. The problem is that the comment claims coverage of the whole class of imports, which means the next reviewer will stop looking.

  Confidence 100 · logic · Fix: either reject every route — super::scrollbar, app::scrollbar, {scrollbar, and the names mod.rs re-exports from scrollbar — and match only on use lines, or narrow the comment and the PR text to "guards the direct super::scrollbar spelling".

Non-blocking

• (new) The test comment points at a file and a split that don't exist — crates/rocm-dash-tui/src/app/mod.rs:1249-1250
  The comment refers to "the module cycle this file's split from scrollbar.rs exists to remove". The comment sits in mod.rs, which was never split from scrollbar.rs. Both actions.rs and scrollbar.rs were split out of app/mod.rs in b1cbc19 (#476), and that split is what created the cycle. What removes the cycle is this PR moving the two types into types.rs. The wording looks carried over from a version of the test that lived in actions.rs.
  Confidence 85 · mechanical · Fix: say the cycle is removed by moving PaneFocus/ScrollTarget into types.rs.

• (new) The concat() obfuscation does nothing — crates/rocm-dash-tui/src/app/mod.rs:1253
["super", "::", "scrollbar"].concat() only matters when a test scans its own file. This test lives in mod.rs and scans actions.rs. The trick doesn't keep the string out of greps either, because the comment at mod.rs:1249 contains use super::scrollbar::... verbatim. The repo's one precedent is crates/rocm-core/src/diagnose.rs no_checker_hand_sets_auto_applicable, and that test does scan its own file.
  Confidence 85 · mechanical · Fix: use a plain literal, or move the test into actions.rs's test module, where the trick is actually needed.

• (standing) The first commit's message is false, and the next two commits correct it — commits d321d02, 5f43d40, 78da228

  • d321d02 says scrollbar.rs "imported PaneFocus/ScrollTarget from actions.rs". At prw-base, both enums are defined in scrollbar.rs, and scrollbar.rs never imports them.
  • The same commit says both modules "now depend on types.rs for these two types". scrollbar.rs imports only ScrollTarget.
  • 5f43d40 and 78da228 mostly fix their predecessor's wording.

  The repo squash-merges. If the squash body is built from the commit list, it will contradict itself.
  Confidence 85 · mechanical · Fix: use the PR description, which is accurate, as the squash message, or squash and reword before merge.

Decisions for the author

• (new) The new types.rs rustdoc explains where the types used to live — non-blocking-improvement
  The PaneFocus and ScrollTarget docs at types.rs:441-443 and types.rs:456-460 explain why each type "lives here, not in scrollbar.rs". That explains the dependency rule, which is useful. It also narrates a past move, which goes stale once nobody remembers the old location. One option is to keep the dependency rule ("visible to actions.rs without depending on scrollbar.rs") and drop the history.

Positive signals

• Moving the two types into types.rs, which already holds every other shared app enum, breaks the cycle without adding a new module. crate::app::{PaneFocus, ScrollTarget} still resolves through the types re-export, so none of the consumers in ui/ or tests/dash_characterization.rs had to change.

Deployment notes

None

What this covered

• Read: all six changed files in full. Also every user of PaneFocus/ScrollTarget across the repo, the app/ re-export block, and xtask/src/crate_edges.rs, which only checks crate-level edges and so has no module-level check this test duplicates. Diff range: db11e47…78da2287.
• Runs: the new test, plus a re-export-spelling mutation. A worker also ran clippy on rocm-dash-tui (--all-targets -D warnings): clean. CI for 78da228 is all success; the GPU and E2E self-hosted lanes, skillscope and Sphinx were skipped.
• Passes: coupled code group, agent-instruction adherence, history, code comments, and design review. The design review ran inline rather than through separate workers because the change is so small. The prior-changes pass did not run, because reading earlier review comments is out of scope for this watcher.
• Ticket: ROCMAI-483 was not retrieved.
• Discussion: not reconciled against any discussion here.

[jussielo-amd]

Reply to the review at 78da2287 (5466660511), addressed in 4c3bf93b:

Blocking — guard test weaker than its name/comment/description claim. Confirmed: the old test only matched the literal substring super::scrollbar, so use super::ScrollbarHandle;, use crate::app::scrollbar::resolve_mouse;, and use super::{scrollbar::ScrollDrag}; all compiled and passed it. Hardened the test to scan actions.rs's non-comment use lines for the scrollbar module name and the three names app/mod.rs re-exports from it (ScrollbarHandle, ScrollDrag, FooterChip) — the same line-scanning approach no_checker_hand_sets_auto_applicable (rocm-core/src/diagnose.rs) uses for a different unenforced invariant. Verified locally against all three of your mutations plus the original violation (use super::scrollbar::{PaneFocus, ScrollTarget};) — all four now fail the test. Only matching use lines (not all lines) also means a stray comment mentioning scrollbar can no longer false-positive it.

Non-blocking (new) — test comment pointed at a nonexistent file/split. Fixed: it no longer says "this file's split from scrollbar.rs" (mod.rs was never split from scrollbar.rs); it now attributes the cycle's removal correctly to moving PaneFocus/ScrollTarget into types.rs.

Non-blocking (new) — concat() obfuscation does nothing. Agreed, removed. This test scans actions.rs, not its own source, so there was never anything for the obfuscation to hide from a self-scan.

Non-blocking (standing) — first commit message describes the dependency backwards. Already addressed in the reply on the 11:01:29Z round: leaving the already-pushed commit wording as-is rather than rewriting history on an open PR, since the repo squash-merges and the accurate PR description is what should seed that squash message.

Decision for the author — rustdoc narrates history alongside the current invariant. Keeping the current form: the "used to live in scrollbar.rs" context is what makes the dependency rule's why legible to a future reader, and it's a doc comment on a small, stable type rather than documentation that has to track the codebase's evolving layout — a low cost if it ever goes stale, and easy to trim then.

cargo test -p rocm-dash-tui --all-targets, cargo clippy -p rocm-dash-tui --all-targets -- -D warnings, and cargo fmt -p rocm-dash-tui --check all pass locally on 4c3bf93b.

[rominf]

🔴 Automated review · pr-review-watcher · 4c3bf93

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.

Watcher note: no change request is filed on this round. The one blocking-rated finding below is standing in narrowed form. The four spellings the 78da228 round cited now fail the test, and the comment false-positive is gone. That earlier wording was settled by putting it, with the author's reply, to the reviewer after its blind report was frozen, and the reviewer re-ran all four mutations. That is anchored evidence, not a blind re-find. The remaining gap was re-found blind: a rustfmt-wrapped multi-line use group, pub(crate) use, and inline paths all still pass the guard. Merging leaves the same thing as last round: correct production code, and a guard that claims more than it checks. No runtime behaviour is at stake, so the finding stays non-gating, as it was rated before. The commit-message finding and the types.rs history-in-docs point are standing. The author's reply on both was read; the blind review reached them again independently, and they remain non-blocking. Everything else is new.

Review — needs work

Full review of the whole change.
Implements the rocm-dash-tui half of ROCMAI-483; the description says the engines/vllm half is in #593. The ticket was named but could not be retrieved from here.

Blocking

• (standing, narrowed) The guard test's comment and the PR description say it catches more than it does — crates/rocm-dash-tui/src/app/mod.rs:1246-1272
  The test only fails when a single line whose trimmed text starts with use contains one of the four names. Its comment says it catches any use that names the scrollbar module "directly, through crate::app::scrollbar, or by one of the names app/mod.rs re-exports", and it concludes "nothing else catches it". The description says the test asserts that actions.rs "never reintroduces a dependency on scrollbar.rs".
  I mutated actions.rs and ran cargo test -p rocm-dash-tui --lib actions_does_not_import_from_scrollbar after each change. The test ran every time (1 passed, 821 filtered out) and stayed green for all of these:

  • A multi-line group: use super::{\n AppState, ScrollDrag,\n};. This is the one most likely to happen, because rustfmt wraps any long use super::{…} group this way, and that is the import style actions.rs already uses.
  • A re-export: pub(crate) use super::scrollbar::ScrollDrag;.
  • Inline paths with no use line: super::scrollbar::resolve_mouse(me, s), or &crate::app::ScrollbarHandle in a signature.

  It also gives a false positive: use crate::ui::panel::vertical_scrollbar; fails the test, because it contains the bare substring scrollbar.

  Reverting the change does make the test fail, so it does test the change. The problem is that its comment claims cases it cannot fail on.
  Confidence 100 · logic · Fix: either harden the test or narrow the claim.

  • Harden: join each use statement up to its ; before matching, accept pub and pub(crate) use, and match on the path (scrollbar::) or on word boundaries rather than a bare substring. Scan every non-comment line, not only use lines.
  • Narrow: change the comment and the description to say exactly what the test covers.

Non-blocking

• (new) The docs say the cycle is gone, but the change only removes the direct import — crates/rocm-dash-tui/src/app/types.rs:456-460, crates/rocm-dash-tui/src/app/mod.rs:1250-1253, PR description
  actions.rs still works on AppState, which holds three types defined in scrollbar.rs:

  • scroll_drag: Option<ScrollDrag> (mod.rs:129)
  • scrollbars: RefCell<Vec<ScrollbarHandle>> (mod.rs:126)
  • last_footer_chips: Vec<FooterChip> (mod.rs:199)

  actions.rs:273 writes state.scroll_drag = None, and a mutation that read all three fields inside actions.rs passed the guard.

  The code governs here. This PR removes actions.rs's use of scrollbar.rs, not every dependency, so "the one edge this module boundary keeps" and "now the only edge between the two modules" overstate it. Breaking the cycle completely would be real work, because FooterChip carries a KeyAction.
  Confidence 70 · architectural · Fix: say the PR "removes actions.rs's direct import of scrollbar.rs", or also move ScrollDrag into types.rs.

• (standing, extended) Some commit bodies are false, and with a squash-merge they would land on main — commits d321d02, 4c3bf93

  • d321d02 says "scrollbar.rs's mouse hit-testing imported PaneFocus/ScrollTarget from actions.rs". At prw-base, both types were defined in scrollbar.rs, and actions.rs imported them from there.
  • (new) 4c3bf93 cites "The guard test added in 03b7698", but git cat-file reports no such object. The test was added in 78da228. The same commit body also reads as a reply to a review round.

  Three of the four commits fix up the PR's own docs and test.
  Confidence 85 · mechanical · Fix: when squashing, reduce the squash body to one accurate message instead of concatenating all four.

• (standing) The new type docs explain the move, not the type — crates/rocm-dash-tui/src/app/types.rs:441-443, 456-460
  Text like "scrollbar.rs defined this type but never used it. Moving it out…" will read as stale history once this lands. That belongs in the commit message.
  Confidence 70 · mechanical · Fix: keep only what the type is and why it lives in types.rs.

Decisions for the author

• (new) Guarding a module boundary by scanning source text — tradeoff
  Inside one crate, the compiler cannot forbid a sibling module's use, so a source scan is the only cheap guard available.

  • For it: it costs nothing to run and documents the intent.
  • Against it: no line-based scan can be complete (see the blocking finding), and a guard believed to be complete stops people looking.

  The options are a scan that understands whole statements, a scan documented as best-effort, or dropping the guard and relying on the module docs plus review.

Positive signals

• The re-exports in app/mod.rs moved from scrollbar to types, so every crate::app::PaneFocus and crate::app::ScrollTarget path in ui/ and tests/dash_characterization.rs still resolves unchanged.

Deployment notes

None

What this covered

• Read: every changed file completely (app/{actions,event_loop,mod,scrollbar,types}.rs and docs/architecture.md), plus every consumer of PaneFocus, ScrollTarget, ScrollbarHandle, ScrollDrag and FooterChip across rocm-dash-tui (ui/, tests/). Range: efcd4800…4c3bf93b.
• Runs:
  • Tests: the rocm-dash-tui lib tests had 815 passed and 3 failed. The 3 failures are agent::clients tests that hit a sandbox token-cache-dir permission error, unrelated to this change. The integration test targets passed.
  • Clippy: clippy -D warnings is clean.
  • Mutations: the guard-test mutations described above.
  • CI at 4c3bf93b: all green, but the GPU and self-hosted E2E lanes, skillscope and Sphinx were skipped.
• Not run:
  • Ticket ROCMAI-483 could not be retrieved, because no tracker tool is available here.
  • The prior-changes pass ran without reading this PR's own review threads, by design.
• Discussion: the review itself is not reconciled against the PR's discussion. The standing-objection check described in the watcher note is the only part that read it.

[jussielo-amd]

Reply to the review at 78da2287 (5468364459), fixed across 1894050f, cc7f763f, and 2fbe9be2:

Blocking — guard test weaker than its comment/description claim. Confirmed and fixed across three rounds, each closing a gap the previous one missed:

• 1894050f hardened the original line-based substring match to join actions.rs's source into whole ;-delimited statements and match whole identifier tokens (catching brace-grouped and rustfmt-wrapped multi-line use groups, pub/pub(crate) use, while no longer false-positiving on vertical_scrollbar).
• cc7f763f added attribute-stripping, since an attribute-prefixed use (e.g. #[cfg(test)] use super::scrollbar::... — the exact pattern scrollbar.rs itself already uses for apply_action) still slipped past the ;-split segment boundary.
• 2fbe9be2 replaced the ;-split approach entirely with a direct keyword scan: use has no other syntactic role in Rust, so the test now finds every whole-word use and reads forward, brace-depth aware, to its own terminating ;. This closes the remaining gap a fresh review found — a use that isn't itself preceded by a semicolon (e.g. the first statement in a function body) used to get silently merged into unrelated code and missed. The test also now has a positive control: an assertion that the detector actually flags a known-bad case, so a future regression in the detector itself won't stay invisible the way this one did.

The test's comment is honest about what's left uncovered: a fully-qualified path referenced inline with no use statement at all, and a use named only inside a /* */ block comment (not stripped, unlike //) — both accepted limits of a source scan rather than a parser. A fresh independent review confirmed these are currently inert for this file (no block comments, no inline scrollbar references outside use and doc comments) and that the detector's one bias — false-positive over false-negative — is the correct one for a regression guard.

Non-blocking (new) — test comment pointed at a nonexistent file/split. Fixed in 1894050f: no longer claims "this file's split from scrollbar.rs" (mod.rs was never split from scrollbar.rs); attributes the cycle's removal correctly to moving PaneFocus/ScrollTarget into types.rs.

Non-blocking (new) — concat() obfuscation does nothing. Removed in 1894050f — the test scans actions.rs, not its own source, so there was nothing for it to hide from.

Non-blocking (new) — doc comment overclaimed the module boundary. types.rs's ScrollTarget doc (and the matching PR description bullet) said the scrollbar.rs→actions.rs edge was "the one edge this module boundary keeps." Narrowed across two rounds: first to "the one direct use-level edge" (naming apply_action as the fourth, test-only import scrollbar.rs draws from actions.rs), then further to say precisely what actions.rs reaches indirectly — only ScrollDrag (via the scroll_drag field on AppState), not ScrollbarHandle or FooterChip, which it never touches. A fresh review verified every remaining factual claim in these doc comments against the code.

Standing — commit-message findings (first commit backwards; 4c3bf93b citing 03b76983). Acknowledged per this PR's established precedent (replies on 2026-10-08) of not rewriting already-pushed commit wording on an open PR. 03b76983 was confirmed an orphaned pre-rebase SHA of the same content now reachable at 78da2287 — not retrievable in an ordinary clone, matching the reviewer's own git cat-file result. The PR description remains the accurate source for the eventual squash-merge message.

Standing — rustdoc narrates history alongside the current invariant. Kept as-is: the "used to live in scrollbar.rs" context is what makes the dependency rule's why legible to a future reader, and it's a doc comment on a small, stable type rather than documentation that must track the codebase's evolving layout.

Decision for the author — guarding via source-text scanning. Committed to hardening the scan properly rather than replacing it with a structural/AST-based check or dropping it: the detector now covers every use-based import form in any position (top-level or function-local, attribute-gated, pub/pub(crate), brace-grouped, multi-line), carries a positive control proving it fires, and its comment honestly scopes the two things it still can't catch.

cargo test -p rocm-dash-tui --all-targets, cargo clippy -p rocm-dash-tui --all-targets -- -D warnings, and cargo fmt -p rocm-dash-tui --check all pass locally on 2fbe9be2, and CI is green (all 30 checks).

[jussielo-amd]

review-coverage · 2fbe9be

Full-scope review of the whole PR diff (merge-base efcd4800…2fbe9be2) by an independent Opus 4 driver fanned out to 2 general-purpose workers, including live mutation testing of the guard test's detector in a pinned worktree against all known use-form variants plus a false-negative probe. No blocking or non-blocking defects found; three already-disclosed author decisions and a pre-merge rebase note only.

[rominf]

🔴 Automated review · pr-review-watcher · 2fbe9be

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.

Watcher note: no change request is filed on this round. This was a full blind review of the whole change. The one blocking-rated finding below is the same class as the standing one from the 4c3bf93 round: the guard test claims more coverage than it has. The spellings that round cited are now handled or listed as known gaps, and this round found a new gap blind (glob imports). Merging still leaves correct production code and a guard that claims more than it checks. No runtime behaviour is at stake, so it stays non-gating, as rated before. The commit-series and the history-in-docs points are standing, and so is the "cycle is gone" overclaim (now at mod.rs). Everything else is new.

Review — needs work

Full review of the whole change.
Implements instance 2 of ROCMAI-483 (the dash-tui scrollbar.rs↔actions.rs cycle), not instance 1 (the vllm process.rs↔state.rs cycle, which the description says is in #593). The PR does not claim to close the ticket.

Blocking

• (standing class, new instance) The guard test says it catches a use "in any position or form", but a glob import gets past it — crates/rocm-dash-tui/src/app/mod.rs:1250-1280 (test comment, and the known-gaps list that leaves globs out), mod.rs:1330 (offenders_in filter), the mod.rs assertion message ("directly or through app/mod.rs's re-exports"), and the PR description's Test plan.
  • What I ran: I added use super::*; plus fn _glob_probe(h: ScrollbarHandle) -> ScrollbarHandle { h } to actions.rs. It compiles, and actions_does_not_import_from_scrollbar still passes: 1 passed, 821 filtered out, EXIT=0.
  • A worker got the same result with use crate::app::*;, and with use super::{self as app}; followed by app::scrollbar::….
  • The claimed coverage is what governs here, because it is what the comment, the assertion message and the description all promise readers. The scanner only matches tokens written out literally, and a glob names none of them.
  • Why it blocks: a check that claims coverage it lacks stops the next reviewer from looking.
  • Confidence 100 · logic · Fix: also flag any use in actions.rs that globs the parent module (super::*, crate::app::*), or switch to the token scan described under Decisions. In either case, list every remaining gap in the comment and the description, or narrow both claims to what the scanner actually catches.

Non-blocking

• (new) The positive control does not cover the re-export names — mod.rs:1279-1282, mod.rs:1336-1345.
  • I replaced "ScrollbarHandle", "ScrollDrag", "FooterChip" in SCROLLBAR_NAMES with dummy names and the test stayed green (EXIT=0, 1 passed).
  • The list is also a hand-copy of pub use scrollbar::{FooterChip, ScrollDrag, ScrollbarHandle} at mod.rs:51. A future re-export from scrollbar.rs would not be guarded.
  • Confidence 100 · logic · Fix: add a control case that uses a re-exported name, and consider deriving the list from include_str!("mod.rs").
• (new) Brace-depth tracking adds nothing for real use items, and it silently drops any statement whose ; it never finds — mod.rs:1305-1324.
  • A use tree cannot contain ;. A worker removed the depth tracking and every case stayed green, including the multi-line and known-bad controls.
  • Its only effect is the if let Some(end) = end branch, which discards an item without failing. use super::scrollbar::{resolve_mouse /* { */}; compiles and passes the guard (worker run, EXIT=0). That input is contrived.
  • The comment's justification ("so a brace-grouped use super::{ ... } is captured whole") is wrong.
  • Confidence 85 · logic · Fix: drop the depth tracking, or fail when no terminator is found.
• (new) "use is a reserved keyword with no other syntactic role in Rust" is false — mod.rs:1257-1259 and the PR description.
  • On edition 2024 (Cargo.toml:24), precise-capturing bounds (impl Iterator<Item = &'a u8> + use<'a>) compile. A worker added one to actions.rs: it compiled, and the detector took it for a use item.
  • The error can only cause false positives, never false negatives.
  • Confidence 100 · mechanical · Fix: correct the sentence.
• (standing, extended) Comments describe this PR's own history, which will be stale once it merges — mod.rs:1263-1268 and 1337-1340 ("an earlier version of this test", in both places), types.rs:441-443, types.rs:456-467.
  • The "earlier version" exists only in this branch's intermediate commits, and rocm-cli squash-merges, so it will never exist on main.
  • The ScrollTarget rustdoc spends most of its length on ScrollDrag/ScrollbarHandle/FooterChip coupling that is unrelated to the type.
  • Its phrase "the scroll_drag field it types on AppState" reads as if actions.rs declares the field type. The field is declared at mod.rs:129.
  • Confidence 85 · mechanical · Fix: say what the code does now, and move the rationale to the test's doc comment.
• (standing) The test comment says the move removes "the module cycle", but a cycle through mod.rs remains — mod.rs:1253-1255.
  • The remaining cycle: actions.rs → AppState::apply_scroll_grab and the scroll_drag field (mod.rs:526, actions.rs:271-273) → scrollbar.rs types on AppState (mod.rs:126,129,199) → scrollbar.rs imports actions.rs.
  • The code governs here, and the ScrollTarget rustdoc already words this correctly ("direct use-level edge").
  • Confidence 70 · mechanical · Fix: say "direct use edge" in the test comment as well.
• (standing) The commit series is a trail of review-round fix-ups, and their messages cite SHAs that will not exist after merge — five of the seven commits ("harden", "close the remaining gaps", "catch an attribute-prefixed use", …).
  • The squash merge flattens them, but the default squash body would carry those messages.
  • Confidence 70 · mechanical · Fix: write the squash message by hand when merging.

Decisions for the author

• Scan comment-stripped actions.rs for whole tokens instead of parsing use items — non-blocking-improvement.
  • A whole-token scan of actions.rs at HEAD, with comments stripped, for scrollbar|ScrollbarHandle|ScrollDrag|FooterChip finds 0 hits today (I ran it).
  • It would also catch the inline fully-qualified path that the comment calls an accepted limit "that would need real parsing". An earlier commit in this series (78da228) checked a broader substring before the check was narrowed to use items.
  • It would remove about 40 lines of keyword and brace scanning. It still needs the glob check from the blocking finding.
• Where the guard test lives — tradeoff.
  • It sits in app/mod.rs tests but reads actions.rs. The repo's one comparable test, no_checker_hand_sets_auto_applicable in crates/rocm-core/src/diagnose.rs, scans its own file, and #476 placed tests by what they exercise.
  • Keeping it in mod.rs puts it next to the module tree it describes. Placing it in actions.rs follows the precedent. Either is defensible.
• Ticket framing — tradeoff.
  • ROCMAI-483 says untangling the cycle is "a real ownership/design decision, not a mechanical move". This PR resolves it by moving the two shared types to types.rs.
  • That is a reasonable ownership call, since types.rs already holds every other pure type in the split. Confirm it is the decision the ticket wanted, rather than a split of responsibilities between actions.rs and scrollbar.rs.

Positive signals

• The re-exports at mod.rs:51-55 keep every crate::app::PaneFocus/ScrollTarget path working. Clippy --all-targets passes over every in-crate caller, including tests/dash_characterization.rs and ui/tabs/pane.rs.
• docs/architecture.md and the types.rs module doc were updated in the same change. A repo-wide grep finds no remaining stale "lives in scrollbar.rs" claim.
• The guard test includes a positive control on the detector itself, which is the right instinct for a source-scan test.

Deployment notes

None

What this covered

• What was read:
  • All six changed files, against prw-base c1c2aeb…2fbe9be2. That is the full diff plus the complete actions.rs, scrollbar.rs and types.rs, and mod.rs lines 1–1360.
  • mod.rs 1360–3428 (unchanged existing tests) and event_loop.rs beyond its one changed import were checked by grep and compilation, not read line by line.
  • The PR description, and ROCMAI-483 retrieved by its identifier (title, body and status only).
• What was run:
  • cargo test -p rocm-dash-tui --lib app::: 228 passed. cargo clippy -p rocm-dash-tui --all-targets -- -D warnings: clean.
  • 9 mutations of actions.rs and 5 of the detector. The glob and re-export-name mutations were re-run by the driver.
  • The leak scan of the diff and commit messages was clean.
  • CI at 2fbe9be: all checks succeeded. The GPU E2E lanes and skillscope were skipped.
• Verdicts on the added test:
  • (1) With the production change reverted, the test fails: re-adding use super::scrollbar::… to actions.rs gives EXIT=101.
  • (2) The production change has no branches. Inside the detector, the word-boundary, depth-tracking, comment-stripping and re-export-name branches can each be broken alone with the test still green.
• Passes:
  • The code, agent-instruction, code-comment, history and prior-changes passes ran as three independent workers.
  • Prior changes means review comments on #476 and #382 (source of the comparable diagnose.rs test) and the source of #337's xtask scan.
  • The Step 7 design questions (guard mechanism, remaining cycle, test placement) were answered by the driver from the workers' evidence rather than by separate workers.
  • No pass was skipped.
• Not done: the full workspace test and clippy gate, and scripts/smoke_local.py.
• The PR's discussion was not reconciled here.