Skip to content

fix(tui): route warning logs to status bar instead of stderr#2

Closed
r3v5 wants to merge 3 commits into
mainfrom
fix-tui-warning-log
Closed

fix(tui): route warning logs to status bar instead of stderr#2
r3v5 wants to merge 3 commits into
mainfrom
fix-tui-warning-log

Conversation

@r3v5

@r3v5 r3v5 commented Jul 7, 2026

Copy link
Copy Markdown
Owner

Summary

  • Replace all 25 tracing::warn!/info!/debug!() calls in openshell-tui with app.status_text assignments to prevent stderr writes from corrupting ratatui's alternate-screen layout
  • Add ForwardWarnings event variant to decouple port-forward warning delivery from sandbox name in CreateResult, preventing downstream gRPC lookup failures in handle_exec_command
  • Remove tracing dependency from the TUI crate as a compile-time guard against reintroduction (14 other crates retain it)

Related Issue

Closes NVIDIA#2120

Changes

crates/openshell-tui/src/lib.rs

  • 18 direct-access tracing::*!() calls → app.status_text assignments
  • 7 spawned-task calls in start_port_forwards()Vec<String> accumulation returned to caller
  • 2 tracing::debug!() calls in refresh_draft_chunks() silently dropped (not user-actionable)
  • start_port_forwards() return type changed from () to Vec<String>
  • Forward warnings sent via Event::ForwardWarnings, not embedded in sandbox name

crates/openshell-tui/src/event.rs

  • Added ForwardWarnings(Vec<String>) variant to Event enum

crates/openshell-tui/Cargo.toml

  • Removed tracing = { workspace = true }

Before / After

Before (stderr spam corrupting TUI layout)

Screenshot 2026-07-06 at 13 30 29

After — error-state sandbox with gateway down (errors in title bar, clean layout)

Screenshot 2026-07-07 at 12 23 23 Screenshot 2026-07-07 at 12 24 02

After — reproduced issue NVIDIA#2120 scenario (error-phase sandbox, gateway stopped mid-session)

Screenshot 2026-07-07 at 12 37 15

Manual Verification

Prerequisites

  • A running OpenShell gateway (brew services start openshell or equivalent)
  • At least one error-phase sandbox (openshell sandbox create --name test-error will create one that enters Error phase)

Test 1: Error-state sandbox (issue NVIDIA#2120 repro)

openshell term
# Navigate to the error-phase sandbox, press Enter
# Wait 10+ seconds (5 ticks)
# Expected: clean TUI layout, no stderr spam
# Expected: title bar may show error text in parentheses if gRPC calls fail

Test 2: Gateway down mid-session

# Tab 1:
openshell term
# Navigate to any sandbox

# Tab 2:
brew services stop openshell

# Tab 1 (wait 4-6 seconds):
# Expected: title bar shows "(failed to refresh sandbox policy: tcp connect error)" or similar
# Expected: TUI layout remains intact, no stderr corruption
# Expected: errors cycle in title bar each tick (policy, sandboxes, providers)

# Cleanup:
brew services start openshell

Test 3: Compile-time guard

# Verify tracing is fully removed from TUI crate
grep -rn 'tracing::' crates/openshell-tui/src/
# Expected: zero results (exit code 1)

cargo clippy -p openshell-tui
# Expected: no warnings

Testing

  • cargo build -p openshell-tui — compiles clean
  • cargo clippy -p openshell-tui — no warnings
  • grep -rn 'tracing::' crates/openshell-tui/src/ — zero results
  • mise run pre-commit — passes
  • Manual: openshell term → error sandbox → clean layout, no stderr corruption
  • Manual: gateway stopped mid-session → errors display in title bar, TUI intact
  • Reproduced exact scenario from issue [term] Warning Log Messed Up the TUI NVIDIA/OpenShell#2120 (error-phase sandbox detail view)

🤖 Generated with Claude Code

r3v5 added 3 commits July 7, 2026 12:58
tracing::warn/info/debug calls in the TUI crate write to stderr via
the global tracing subscriber. In ratatui's alternate-screen/raw-mode,
stderr writes corrupt the terminal layout. Error-state sandboxes
amplify this as background gRPC polls fail every 2s tick.

Replace all 25 tracing calls with app.status_text assignments for
direct-access sites, and Vec<String> accumulation for the spawned
start_port_forwards task. Add ForwardWarnings event variant to
decouple forward warning delivery from the sandbox name in
CreateResult, preventing downstream gRPC lookup failures.

Closes NVIDIA#2120

Signed-off-by: Ian Miller <milleryan2003@gmail.com>
New event type for non-fatal port-forward warnings during sandbox
creation. Keeps warning delivery separate from the sandbox name
in CreateResult to avoid corrupting downstream gRPC lookups.

Signed-off-by: Ian Miller <milleryan2003@gmail.com>
Compile-time guard against reintroducing stderr-writing tracing
calls in the TUI crate. 14 other crates retain the dependency.

Signed-off-by: Ian Miller <milleryan2003@gmail.com>
@r3v5 r3v5 changed the title fix(tui): route warning logs to status bar instead of stderr (#2120) fix(tui): route warning logs to status bar instead of stderr Jul 7, 2026
@r3v5

r3v5 commented Jul 7, 2026

Copy link
Copy Markdown
Owner Author

🤖 claude-code-review

PR Review: fix(tui): route warning logs to status bar instead of stderr

Overview

Fixes TUI layout corruption (NVIDIA#2120) caused by tracing::warn!() writing to stderr inside ratatui's alternate-screen mode. Replaces all 25 tracing calls with app.status_text assignments and a new ForwardWarnings event, then removes the tracing dependency as a compile-time guard.

Key Design Decisions

  • app.status_text over tracing redirect (lib.rs:784): Used existing status bar mechanism (50+ usages already) rather than adding a TUI-compatible tracing layer. Simpler, no new dependencies.
  • ForwardWarnings event variant (event.rs:54): start_port_forwards runs inside tokio::spawn — no app access. New event type keeps warning delivery separate from sandbox name in CreateResult, preventing handle_exec_command from sending a corrupted name to GetSandboxRequest.
  • if let Ok(Ok(resp)) for draft chunks (lib.rs:2422): Debug-level errors silently dropped — not user-actionable. Clippy-suggested simplification.
  • Dependency removal (Cargo.toml:28): Compile-time guard. Future tracing::*!() in TUI = instant compiler error. 14 other crates unaffected.

Notable Code

crates/openshell-tui/src/lib.rs — forward warnings decoupled from sandbox name:

let forward_warnings = start_port_forwards(...).await;
if !forward_warnings.is_empty() {
    let _ = tx.send(Event::ForwardWarnings(forward_warnings));
}
// CreateResult always gets clean name
let _ = tx.send(Event::CreateResult(Ok(sandbox_name)));

crates/openshell-tui/src/lib.rs — handler joins warnings for display:

Some(Event::ForwardWarnings(warnings)) => {
    app.status_text = format!("port forward issues: {}", warnings.join("; "));
}

Potential Concerns

  • Status text overwrite on every tick: When gateway is down, multiple refresh functions set status_text each tick — last one wins. Not a regression (tracing also fired all of them), but now user-visible. Could add dedup guard in a follow-up if noisy.

Verdict

Ship-ready. All original bugs addressed, build clean, manually verified against issue NVIDIA#2120 repro scenario.

@Rushit

Rushit commented Jul 8, 2026

Copy link
Copy Markdown

fyi, I have vouch request open and I took slightly different route here -> https://github.com/NVIDIA/OpenShell/compare/main...Rushit:OpenShell:fix/2120-tui-log-to-file?expand=1. Apparently, The term TUI takes over the terminal with an alternate screen. Writing tracing output to stderr while that screen is active corrupts the display, so routed them files to match current behaviour and just let it display error message for any other use-case too.

@r3v5 r3v5 closed this Jul 10, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[term] Warning Log Messed Up the TUI

2 participants