Code Review Summary -- PR #140

Findings Addressed (commit d928467)

1. [MEDIUM] Subtraction overflow panic in render_disconnection_notification

• File: src/view/frame_renderer.rs (lines 356-411)
• Issue: Bare arithmetic box_width - 4 - text.len() would panic on unsigned underflow when a hostname is long enough (56+ chars) to exceed the box inner width. Similarly, width - 4 panics if terminal width is less than 4.
• Fix: Replaced all bare subtraction with saturating_sub(). Added text truncation via Cow when text exceeds inner width. Added early-return guard for degenerate box_width < 6.

2. [MEDIUM] Redundant as_app_state() allocation in GPU render path

• File: src/view/frame_renderer.rs (line 188, now removed)
• Issue: render_gpu_section called snapshot.as_app_state() to create a second full AppState clone, when render_main had already created one. Each call allocates all Vecs, HashMaps, and VecDeques. For a performance-focused PR, this defeats part of the purpose.
• Fix: Pass &AppState reference from render_main into render_gpu_section, eliminating one full allocation per frame.

3. [LOW] Redundant crossterm::terminal::size() syscall in render_local_devices

• File: src/view/frame_renderer.rs (line 460, now removed)
• Issue: Called crossterm::terminal::size() again inside the render path, even though cols/rows are already available from the caller. This added a syscall and introduced a subtle inconsistency (terminal size could change between the two calls within the same frame).
• Fix: Changed render_local_devices signature to accept cols: u16, rows: u16 from the caller instead.

Findings Not Requiring Code Changes

4. [LOW] update_scroll_offsets method signature improvement (already correct)

• The change from &self method to associated function Self:: is a good cleanup.

Architecture Review -- No Issues Found

• Snapshot completeness: The RenderSnapshot::capture() correctly clones all fields read during rendering. The known_hosts field is intentionally omitted because it is only used by the data collection layer, never during rendering.
• as_app_state() roundtrip: The conversion is complete and correct. Fields like nvml_notification_shown and tenstorrent_notification_shown are not included in the snapshot because they are only read/written by the data collectors, not the rendering path. The AppState::new() defaults are safe for these.
• Critical section correctness: The lock is held only for bookkeeping updates + snapshot capture, then released. No rendering code touches shared state afterward. No deadlock risk since there is only one mutex and it is never held across an await point after the snapshot.
• VecDeque-to-Vec conversion: Using .iter().copied().collect() is correct and avoids cloning the ring buffer internals. The rendering path only iterates forward, so Vec is the right choice.
• No race conditions: The snapshot is immutable after capture. Background collectors can freely update AppState while rendering proceeds on the snapshot.

Verification

• cargo clippy --features cli -- -D warnings passes
• cargo fmt -- --check passes
• All tests pass (unit + doc tests)

PR Finalization Complete

Summary

• Tests: Added 12 new unit tests for FrameRenderer in src/view/frame_renderer.rs:
  • Smoke tests for render_loading (local, remote, with startup status lines)
  • Smoke tests for render_main (empty state, header timestamp, version string)
  • Box geometry tests for render_disconnection_notification (narrow width, normal width, max-width cap, long hostname truncation)
  • Smoke test for render_help
  • Zero-sized struct assertion for FrameRenderer
  • render_snapshot.rs already had 8 unit tests (all passing)
• Documentation: No documentation changes required (no public API surface changes)
• Lint/Format: cargo fmt -- --check and cargo clippy --features cli -- -D warnings both pass with no issues

Total test count: 216 unit tests (up from 204), all passing.