TUI mode

[domenkozar]

Fixes #2042
Fixes #1848

• duplicated Evaluating
• fix terminal resizing
• support tasks
• fix graceful shutdown

[claude]

Claude finished @domenkozar's task —— View job

---

Reviewing TUI Mode PR

I've completed a comprehensive review of this large PR that adds TUI functionality to devenv. Here are my findings:

Review Todo List

• Analyze new devenv-tui crate architecture and design
• Review TUI integration points in existing crates
• Check error handling and graceful shutdown implementation
• Assess performance considerations for real-time UI updates
• Review security implications of new dependencies and functionality
• Analyze test coverage for new TUI components
• Check for potential memory leaks or resource management issues
• Examine code quality, style, and best practices
• Review documentation and usability aspects

---

🔴 Critical Issues Found:

1. Unsafe Mutex Usage Pattern (app.rs:79-87)

if let Ok(model_guard) = model_clone.lock() {
    // render logic
} else {
    element!(View(width: terminal_width))
}

Problem: Lock poisoning causes silent fallback to empty view, making failures invisible
Impact: Complete UI loss on lock poisoning with no error indication
Fix: Implement proper error recovery or use RwLock for read operations

2. Resource Management Concerns (model.rs:6-9)

const MAX_LOG_MESSAGES: usize = 1000;
const MAX_LOG_LINES_PER_BUILD: usize = 1000;

Problem: No bounds on activity count growth; could exhaust memory during long operations
Impact: OOM crashes on systems with many concurrent builds
Fix: Add activity count limits and implement LRU eviction

3. Race Condition in Spinner Updates (app.rs:48-74)

tokio::time::sleep(Duration::from_millis(50)).await;
if let Ok(mut model_guard) = model.lock() {
    model_guard.ui.spinner_frame = (model_guard.ui.spinner_frame + 1) % 10;
    // triggers re-render
}

Problem: Aggressive polling with shared state mutation creates race conditions
Impact: UI corruption, missed updates, excessive CPU usage
Fix: Use event-driven updates instead of continuous polling

---

🟡 Medium Priority Issues:

4. Error Recovery Gaps (tracing_layer.rs:33-36)

fn update_model<F>(&self, f: F) {
    if let Ok(mut model) = self.model.lock() {
        f(&mut model);
    }
    // Silent failure - lost events
}

Problem: Silent failures could hide critical errors and lose logging events
Fix: Add error logging and retry mechanisms

5. Performance Bottlenecks

• Display calculation in render path: model.rs:406-481 performs complex hierarchical traversal on every render
• Continuous rendering: 50ms loop regardless of activity level wastes resources
• Deep mutex chains: Multiple locks across nix_log_bridge.rs increase deadlock risk

Recommendations:

• Cache display calculations and invalidate on state changes
• Use event-driven rendering triggered by actual data changes
• Consider lock-free data structures for frequently accessed data

6. Integration Complexity

The NixLogBridge (nix_log_bridge.rs) manages complex state synchronization between Nix logs and TUI. While functional, it introduces multiple points of failure.

---

🟢 Positive Aspects:

Good Design Decisions:

1. Clean separation of concerns with separate model, view, and component modules
2. Proper shutdown handling using tokio_shutdown crate
3. No unsafe code - maintains Rust safety guarantees
4. Structured tracing integration with consistent event emission
5. Good error types using thiserror and miette

Code Quality:

• Consistent naming conventions and Rust idioms
• No TODO/FIXME comments left in production code
• Good use of type system for state management
• Clear component hierarchy with proper context passing

---

🛡️ Security Assessment:

Dependencies:

• All new dependencies are from reputable sources
• No direct dependency vulnerabilities found
• iocraft is a newer TUI library - monitor for security updates

Input Handling:

• Keyboard input properly sanitized
• No direct user input to shell commands
• Activity IDs properly typed to prevent injection

---

📊 Test Coverage Issues:

Missing Tests:

• No unit tests for TUI components (devenv-tui/src/)
• No integration tests for TUI-backend interaction
• No error handling tests for race conditions or lock failures
• No performance tests for memory usage or rendering speed

Recommendation: Add comprehensive test suite before merging, especially for:

• Model state transitions
• Error recovery scenarios
• Resource cleanup on shutdown
• Performance under load

---

🚀 Performance Considerations:

Memory Usage:

• Bounded log retention is good, but activity growth is unlimited
• Complex data structures could benefit from memory profiling
• String allocations in render loop could be optimized

CPU Usage:

• 50ms polling is wasteful - move to event-driven architecture
• Consider render scheduling to avoid unnecessary updates

---

📝 Final Recommendations:

Before Merging:

1. Fix critical mutex handling in app.rs:79-87
2. Add activity count limits to prevent memory exhaustion
3. Replace polling with event-driven updates for spinner
4. Add comprehensive test coverage, especially error scenarios
5. Add memory/performance benchmarks for long-running operations

Future Improvements:

1. Implement proper error recovery with user notifications
2. Add configuration options for log retention limits
3. Consider async rendering pipeline for better performance
4. Add accessibility features for screen readers

Architecture Concerns:

The TUI integration is well-architected but adds significant complexity to the codebase. Consider if the improved UX justifies the maintenance overhead.

---

Overall Assessment: This is a substantial feature addition with good architectural design, but contains several critical issues that should be addressed before merging. The TUI functionality works well in happy-path scenarios but lacks robustness for production use.

[github-actions]

🔍 Suggested Reviewers

Based on git blame analysis of the changed lines, the following contributors have significant experience with the modified code:

• @sandydoo - 100.0% of changed lines (128 lines)

Please consider reviewing this PR as you have authored significant portions of the code being modified. Your expertise would be valuable! 🙏

This comment was automatically generated by git-blame-auto-reviewer

Last updated: 2025-10-24T15:59:34.546Z

[cloudflare-workers-and-pages]

Deploying devenv with    Cloudflare Pages

Latest commit:	9304e35
Status:	✅  Deploy successful!
Preview URL:	https://f35aec58.devenv.pages.dev
Branch Preview URL:	https://tui.devenv.pages.dev

View logs

[domenkozar]

Looks like we might use https://flatpak.github.io/xdg-desktop-portal/docs/doc-org.freedesktop.portal.Settings.html to get system colors on Linux at least

[wokalski]

this would be nice! I just wanted to write that the output is not friendly towards beginners, it gives the impression of being "stuck".