Code Review Summary: PR #139

Overall Assessment

This PR implements a well-designed transition from a fixed 100ms poll loop to an event-driven tokio::select!-based architecture. The core design is sound: the UiEventCoordinator cleanly separates terminal I/O (blocking, in spawn_blocking) from async data notifications (Arc<Notify>) and animation ticks, unifying them through a single next_event() select point.

Architecture Review

What works well:

• The tokio::select! with conditional animation branch (if self.animations_active) is the correct approach for pausing animation ticks without busy-spinning
• Using mpsc::channel(64) as a bounded bridge from the blocking terminal reader is appropriate -- prevents unbounded memory growth while accommodating input bursts
• MissedTickBehavior::Skip is correct for animations -- burst-firing missed ticks would cause visual artifacts
• The Notify::notify_one() semantics are appropriate here: multiple notifications between renders collapse into a single "data changed" signal, which is the desired behavior since the UI reads data_version to detect actual changes
• The render throttle (MIN_RENDER_INTERVAL_MS) correctly gates expensive re-renders even when events arrive faster

No deadlock risk: The Arc<Mutex<AppState>> is only locked in one direction (collector locks briefly to write, UI locks to read/render), and the Notify is fire-and-forget -- there is no lock ordering issue.

Findings Addressed

Fix Applied (commit e09b1d0)

• Added UiEvent::TerminalClosed variant for graceful shutdown on terminal loss
• Changed term_tx from mpsc::Sender<Event> to Option<mpsc::Sender<Event>>, moving the sender into the spawned task via take() instead of cloning
• Changed spawn_terminal_reader(&self) to spawn_terminal_reader(&mut self) to support the take() pattern
• Added TerminalClosed handler in ui_loop.rs that breaks out of the main loop
• When the terminal reader exits (error, terminal gone), the channel closes naturally, recv() returns None, and the UI exits gracefully

Items Reviewed With No Issues Found

• Security: No user input handling, no network-facing code in the changed files. The event coordinator only processes crossterm terminal events and internal notifications. No injection surface.
• Race conditions: Notify::notify_one() stores at most one permit, but this is acceptable -- multiple data updates between renders are coalesced by design (the UI checks data_version).
• Performance: The architecture genuinely achieves the stated goal. Idle CPU usage drops because the thread sleeps in tokio::select! rather than polling every 100ms. The blocking reader uses 50ms poll timeout which is a reasonable tradeoff between shutdown responsiveness and CPU usage.
• Memory: Bounded channel (64 events) prevents unbounded growth. No new allocations in the hot path.
• Correctness of animation gating: set_animations_active() is called after each render based on current state. Minor delay (one event cycle) in re-enabling animations is acceptable for a TUI monitoring tool.

Verification

• cargo clippy --features cli -- -D warnings: zero warnings
• cargo test --features cli: 389 tests pass, 0 failures

PR Finalization Complete

Summary

• Tests: Added 14 unit tests in src/view/ui_events.rs covering all UiEvent variants, UiEventCoordinator construction, set_animations_active toggling, the DataReady notification path (with DataCollector wiring), the TerminalClosed path when the channel closes, event mapping logic for Resize and TerminalInput, and sanity/value assertions for the new ANIMATION_TICK_MS and TERMINAL_READER_POLL_MS AppConfig constants.
• Documentation: No documentation gaps found; the new module has module-level and per-item doc comments already.
• Lint/Format: cargo clippy --features cli -- -D warnings and cargo fmt --check both pass cleanly.

All 193 tests passing (179 lib + 14 new view::ui_events). Ready for merge.