refactor(maestro): TUI dispatch returns effects #42

2026-08-22T13:08:29+01:00

naps62-yolo commented

2026-08-22 13:08:29 +01:00

dispatch was async fn … -> () with reqwest calls and task spawns inlined, so it could not report intent — 5 of 8 arms were tested and the three with effects were unreachable. It now returns Vec<Effect> and App::run performs them.

Two live bugs the old shape produced, both fixed with regression tests:

The toast could not be dismissed. handle_overlay_input returned Pass for Toast(_) and no path ever cleared it, so spawn failed: … stayed on screen until another overlay replaced it. It now clears the toast and still returns Pass, so the keystroke does its normal job rather than being swallowed.
Closing a tab leaked its emulator. Only submit_kill called SessionEmulators::remove, so a closed tab's WebSocket task and vt100 parser lived on forever. close_tab now reports the session it dropped — and deliberately returns None when another tab still shows the same one.

AppState::overlay is private behind show_overlay / dismiss_overlay / toast. Four sites poked the field directly, which is precisely why the toast bug existed: it was the one piece of state with no method owning its transitions.

Newly testable, and now tested: AttachHighlighted yields Effect::Attach, kill-confirm y yields Effect::Kill and n/Esc yields nothing, spawn-modal submit carries the typed cwd and command. Those needed a live daemon before. The existing dispatch tests dropped #[tokio::test] for plain #[test] — no runtime is involved any more.

I kept state.rs as a module rather than folding it into app.rs. The review called that seam artificial, and it was — but the real complaint was that AppState owned no invariant App couldn't bypass. Making the transitions methods fixes that; moving the file would only have made app.rs 700 lines.

Verification

cargo fmt --all -- --check                   ok
cargo clippy --all-targets -- -D warnings    0
cargo test --all                             301 passed, 0 failed

Third of nine from the architecture review, after #40 and #41.

`dispatch` was `async fn … -> ()` with `reqwest` calls and task spawns inlined, so it could not report intent — 5 of 8 arms were tested and the three with effects were unreachable. It now returns `Vec<Effect>` and `App::run` performs them. **Two live bugs the old shape produced**, both fixed with regression tests: - **The toast could not be dismissed.** `handle_overlay_input` returned `Pass` for `Toast(_)` and no path ever cleared it, so `spawn failed: …` stayed on screen until another overlay replaced it. It now clears the toast and still returns `Pass`, so the keystroke does its normal job rather than being swallowed. - **Closing a tab leaked its emulator.** Only `submit_kill` called `SessionEmulators::remove`, so a closed tab's WebSocket task and vt100 parser lived on forever. `close_tab` now reports the session it dropped — and deliberately returns `None` when another tab still shows the same one. `AppState::overlay` is private behind `show_overlay` / `dismiss_overlay` / `toast`. Four sites poked the field directly, which is precisely why the toast bug existed: it was the one piece of state with no method owning its transitions. Newly testable, and now tested: `AttachHighlighted` yields `Effect::Attach`, kill-confirm `y` yields `Effect::Kill` and `n`/Esc yields nothing, spawn-modal submit carries the typed cwd and command. Those needed a live daemon before. The existing dispatch tests dropped `#[tokio::test]` for plain `#[test]` — no runtime is involved any more. I kept `state.rs` as a module rather than folding it into `app.rs`. The review called that seam artificial, and it was — but the real complaint was that `AppState` owned no invariant `App` couldn't bypass. Making the transitions methods fixes that; moving the file would only have made `app.rs` 700 lines. <details> <summary>Verification</summary> ``` cargo fmt --all -- --check ok cargo clippy --all-targets -- -D warnings 0 cargo test --all 301 passed, 0 failed ``` </details> Third of nine from the architecture review, after #40 and #41.

naps62-yolo added 1 commit 2026-08-22 13:08:30 +01:00

refactor(maestro): make the TUI dispatch a reducer over effects

ci / rust (pull_request) Successful in 1m57s

Details

ci / rust (push) Successful in 1m54s

Details

f01771568b

dispatch was `async fn -> ()` with reqwest calls and task spawns inlined,
so it could not report intent and the spawn, kill and attach arms had no
test. It now returns Vec<Effect> and App::run performs them.

Two bugs the old shape produced, both fixed and covered:

- The toast overlay could not be dismissed. handle_overlay_input returned
  Pass for Toast(_) and nothing cleared it, so "spawn failed: ..." stayed
  until another overlay replaced it. It now clears and still returns Pass,
  so the keystroke does its normal job instead of being eaten.
- Closing a tab leaked its emulator. Only submit_kill released one, so a
  closed tab's WebSocket task and vt100 parser outlived it forever.
  close_tab now reports the session it dropped — and holds onto it when
  another tab still shows the same one.

AppState::overlay is private behind show_overlay/dismiss_overlay/toast.
Four sites poked the field directly; that is why the toast bug existed.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

naps62-yolo reviewed

2026-08-22 13:32:21 +01:00

naps62-yolo left a comment

No findings.

No findings.

naps62-yolo merged commit f01771568b into main

2026-08-22 13:32:47 +01:00

naps62-yolo deleted branch lithuanians

2026-08-22 13:32:47 +01:00

naps62-yolo referenced this pull request

2026-08-22 13:37:32 +01:00

refactor(maestro): collapse the daemon's double event fan-out #43