fix: TUI rendering for log items - #1340
Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (13)
📝 WalkthroughWalkthroughThe change routes ChangesTerminal dashboard integrity
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant SkippyServer
participant MeshTracingStderrWriter
participant OutputSink
participant Dashboard
SkippyServer->>MeshTracingStderrWriter: emit structured warning
MeshTracingStderrWriter->>OutputSink: route stderr event
OutputSink->>Dashboard: deliver OutputEvent
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
This pull request is currently a draft. Reviews will not take place until the PR is marked as ready for review. |
c611fa2 to
cb2137a
Compare
Review: right diagnosis, wrong remedy on the redraw halfI reproduced this on a fresh clone of
1. The premise is real. I reproduced it.The theory behind this PR — out-of-band writes to stderr desync ratatui's back buffer and corrupt the alternate screen permanently — is correct. I verified it directly rather than inferring it. With the dashboard running at 100x40, I wrote to the process's own stderr from outside, which is exactly what a stray Four seconds and ~120 redraws later: It never healed. Ratatui's diff has no way to know those cells changed. So: real bug, worth fixing. What does and doesn't repair it on
And here's the thing: So the targeted fix is sitting right there: make 2. Nick's instinct about the constant repaint is correct — here are numbersI measured bytes emitted through a
The idle row is the one that matters. Each of those 60 frames begins with This was also a decision someone already made deliberately and this PR reverses by deleting the test that recorded it. Suggested direction instead — invalidate on events, not on frames:
That gets a user-reachable repair for every corruption case I could produce, at zero steady-state cost. Minor, but while you're in there: 3. The
|
Follow-up: visual A/B, measured in displayed screen states rather than bytes@nickdizazzo asked whether the flicker could be shown visually instead of inferred from byte counts. It can, and the answer is sharper than the byte measurement was — including one correction to my own numbers above. MethodByte counts were the wrong unit; so is a screenshot. A screenshot samples one arbitrary instant and cannot resolve a 9 ms blank. What resolves it is the layer underneath the glyphs: the character grid a terminal computes after interpreting the escape sequences. So I ran each binary on a real PTY sized 100x40 and recorded every Both runs: same host (carrack), same PTY geometry, same model (
Measurement window is 15.5 s of idle dashboard: model loaded, no traffic, nothing on screen changing. 1. The flicker is real, and it is a fully blank screen
Every one of the 41 erases leaves a completely blank 100x40 grid — occupancy 0, not "mostly blank". Each blank state is held 1.9–29.2 ms (median 9.1 ms) before the repaint bytes arrive, totalling 344 ms of blank screen inside 15.5 s — 2.15% of wall-clock time, while the dashboard has nothing to redraw. Whether a given blank window is actually painted depends on where the display refresh lands. Integrating the measured gaps against a 60 Hz refresh: ≈1.24 fully-blank painted frames per second, ≈2.14 at 120 Hz. That is not a subtle artifact; that is a dashboard that blinks black about once a second while idle. Blank frame, rendered from the captured grid ( Correction to my earlier comment. I wrote ~490 KB/s, extrapolating 16.7 KB/frame at the 33 ms redraw interval. The idle dashboard does not actually repaint at 30 fps — measured, it is ~2.6 full repaints/s, so the real figure is ~44 KB/s, and the ratio against main is 452x, not 591x. Still ~44 KB/s of terminal traffic over SSH to draw a screen that did not change, and the flicker conclusion holds — it is now measured rather than extrapolated. 2. Credit where it is due: the fix does work for the bug it targetsI reproduced the original corruption the same way as before — an out-of-band write to the running process's stderr, i.e. exactly what a stray
main, 3 s after the injection and after pressing this PR, 1.4 s after the same injection — clean: So the diagnosis is right and the mechanism works. The disagreement is only about the dose: this buys a 170 ms self-heal for a corruption that is rare, and pays for it with a black frame about once a second, forever, on every node. 3.
|
…ing them The MODEL/PROCESSES column was laid out with `Constraint::Fill(1)` while its cell text was truncated to a separately computed width that had a minimum of 8. The two never had to agree, and at narrow panel widths they did not: the text was fitted to 8 characters and then rendered into whatever `Fill(1)` had left over, which at a 100-column terminal was a single character. The table showed a `M` header over an `l` cell. Raising the dashboard's minimum width does not fix this — the column is still one character at 100 columns and still truncated at 120 — it only hides the narrow cases while costing every 80-column user the dashboard entirely. Columns are now solved explicitly and rendered with exact `Length` constraints, so the layout is what the text was fitted to. When the panel cannot afford every column it surrenders them from the right (STATE, then PORT) rather than crushing the column that identifies the row. Truncating a header to `STA` is not an improvement over dropping it. Measured across 80/100/120/160/200 columns: every width now renders whole, legible columns, and the model name stays recognizable at all of them. Refs #1340 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Live PTY testing found two gaps in the capture reader that the unit tests could not: a stray write with no trailing newline was intercepted correctly (the frame stayed clean) but then sat in the reader's buffer forever, and raw escape bytes were forwarded into the dashboard verbatim. Both matter. `print!` without a newline and `\r` progress counters are ordinary output, and holding them until the next newline means the operator never sees them. Rendering an unfiltered escape sequence into a dashboard cell would move the cursor and corrupt the very frame capture exists to protect. The reader now polls with a 150 ms idle timeout and flushes whatever partial line is pending, treats `\r` as a line end so progress counters surface, and strips control characters before the text reaches a cell. Refs #1340 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Live PTY testing found two gaps in the capture reader that the unit tests could not: a stray write with no trailing newline was intercepted correctly (the frame stayed clean) but then sat in the reader's buffer forever, and raw escape bytes were forwarded into the dashboard verbatim. Both matter. `print!` without a newline and `\r` progress counters are ordinary output, and holding them until the next newline means the operator never sees them. Rendering an unfiltered escape sequence into a dashboard cell would move the cursor and corrupt the very frame capture exists to protect. The reader now polls with a 150 ms idle timeout and flushes whatever partial line is pending, treats `\r` as a line end so progress counters surface, and strips control characters before the text reaches a cell. Refs #1340 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Refs #1340 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Superseded by #1382, which takes the diagnosis here and the half I said should ship, and goes after the root cause. Keeping from this PR: the skippy-server Not keeping: the per-draw clear (measured 41 erases and 344 ms of blank screen in 15.5 s), the 60 → 100 min-width bump (does not fix the truncation — still one character wide at 100, still truncated at 120 — and costs every 80-column user the dashboard), and the five widened tests. The root cause neither of us was addressing: the dashboard rendered to Full A/B on a real PTY in #1382. Thanks for the diagnosis — the reproduction and the |
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
Quality / CI contracts and consistency failed on this branch: the console-print ratchet still approved four eprintln! occurrences that this PR converted to tracing — crates/mesh-llm-host-runtime/src/runtime/tracing_writer.rs:321 and crates/skippy-server/src/kv_integration/config.rs:110/120/152 — so the checker reported them as "approved occurrence is missing or was replaced". Regenerated with `cargo run -p xtask -- repo-consistency no-console-print --regen`. The diff is deletions only: 1044 legacy hits across 115 files becomes 1040 across 113. No entry was added and no line number moved, so the ratchet strictly tightened. Refs #1340 Co-authored-by: Nick DiZazzo <nick.dizazzo@gmail.com> Signed-off-by: Nick DiZazzo <nick.dizazzo@gmail.com>
`spawn_reader` discarded the `thread::Builder::spawn` result, so a failed spawn still left fd 1 and fd 2 pointing at a pipe with nothing draining it. The failure mode is the worst kind: everything works until the 64 KiB pipe buffer fills, and then every write to stdout or stderr — in this process and in every child that inherited those descriptors — blocks forever, while the dashboard keeps painting as if nothing is wrong. The spawn result is now propagated and `install` puts the saved descriptors back before returning the error. Capture is optional at the call site (`enter_terminal` treats a failure as "no capture"), so the dashboard still comes up — just without interception. Also stop dropping tabs from captured lines. `char::is_control` counts `\t`, and llama.cpp's loader lines are tab-separated, so stripping it ran two columns together; it degrades to a space instead. Raised by CodeRabbit on #1382. Refs #1340 Co-authored-by: Nick DiZazzo <nick.dizazzo@gmail.com> Signed-off-by: Nick DiZazzo <nick.dizazzo@gmail.com>
Two ways the repair could fail to repair.
The handler matched only `Char('r')`, but the status bar reads
`R Refresh` and Shift+R arrives as `Char('R')` — pressing the advertised
key did nothing. It now accepts either, with the existing guard that
keeps both as filter text while the events filter is being edited.
`render_if_dirty` also cleared `pending_full_repaint` with `mem::take`
before the fallible repair ran. If the erase failed, the request was gone
and the next dirty render was an ordinary diff against a screen ratatui
still believed was intact — so the damage survived a key press the
operator had already made. The flag is now cleared only after
`repair_tui_terminal` succeeds, and the propagated error leaves `dirty`
set so the next render retries.
Raised by CodeRabbit on #1382.
Refs #1340
Co-authored-by: Nick DiZazzo <nick.dizazzo@gmail.com>
Signed-off-by: Nick DiZazzo <nick.dizazzo@gmail.com>
The dashboard rendered to `io::stderr()` — the same descriptor used by `eprintln!`, tracing's default writer, inherited plugin child stderr, and every noisy C library in the process. Sharing that descriptor is why a stray write lands *on top of* the dashboard and never heals: ratatui diffs against its own idea of the screen, so damaged cells already match the buffer it believes is displayed. Converting individual call sites cannot close this. `plugin/runtime.rs` hands spawned plugins `Stdio::inherit()`, the staged llama.cpp runtime is C, and third-party crates print whatever they like. So the fix is at the descriptor layer: - Render to the controlling terminal (`/dev/tty`, `CONOUT$`) instead of fd 2, giving the dashboard a channel nothing else holds. This is why `less`, `fzf`, and `vim` open the tty directly. - With that in place, redirect fd 1 and fd 2 into the dashboard while it owns the screen. A reader thread turns each line into an `OutputEvent`, so stray output becomes a dashboard row instead of screen damage. The original descriptors are restored on exit and on the panic path. - Wire `R` to a one-shot physical clear plus diff invalidation. The status bar has advertised `R Refresh` since the dashboard shipped with nothing behind it; it is the repair for damage capture cannot intercept, such as another process writing straight to the tty. Also converts the three `skippy-server` KV-tier `eprintln!` calls and the embedded-runtime-tracing `eprintln!` to `tracing`, and adds the `skippy_server=warn` directive without which `EnvFilter::from_default_env` (which defaults to ERROR) drops those warnings before the writer sees them — converting them alone would have silenced the diagnostics rather than routed them. `mesh-llm-tui` keeps `#![forbid(unsafe_code)]`: the descriptor plumbing uses `std::io::pipe` and rustix's safe `dup2`/`fcntl` wrappers. Refs #1340 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ing them The MODEL/PROCESSES column was laid out with `Constraint::Fill(1)` while its cell text was truncated to a separately computed width that had a minimum of 8. The two never had to agree, and at narrow panel widths they did not: the text was fitted to 8 characters and then rendered into whatever `Fill(1)` had left over, which at a 100-column terminal was a single character. The table showed a `M` header over an `l` cell. Raising the dashboard's minimum width does not fix this — the column is still one character at 100 columns and still truncated at 120 — it only hides the narrow cases while costing every 80-column user the dashboard entirely. Columns are now solved explicitly and rendered with exact `Length` constraints, so the layout is what the text was fitted to. When the panel cannot afford every column it surrenders them from the right (STATE, then PORT) rather than crushing the column that identifies the row. Truncating a header to `STA` is not an improvement over dropping it. Measured across 80/100/120/160/200 columns: every width now renders whole, legible columns, and the model name stays recognizable at all of them. Refs #1340 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Live PTY testing found two gaps in the capture reader that the unit tests could not: a stray write with no trailing newline was intercepted correctly (the frame stayed clean) but then sat in the reader's buffer forever, and raw escape bytes were forwarded into the dashboard verbatim. Both matter. `print!` without a newline and `\r` progress counters are ordinary output, and holding them until the next newline means the operator never sees them. Rendering an unfiltered escape sequence into a dashboard cell would move the cursor and corrupt the very frame capture exists to protect. The reader now polls with a 150 ms idle timeout and flushes whatever partial line is pending, treats `\r` as a line end so progress counters surface, and strips control characters before the text reaches a cell. Refs #1340 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Refs #1340 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Quality / CI contracts and consistency failed on this branch: the console-print ratchet still approved four eprintln! occurrences that this PR converted to tracing — crates/mesh-llm-host-runtime/src/runtime/tracing_writer.rs:321 and crates/skippy-server/src/kv_integration/config.rs:110/120/152 — so the checker reported them as "approved occurrence is missing or was replaced". Regenerated with `cargo run -p xtask -- repo-consistency no-console-print --regen`. The diff is deletions only: 1044 legacy hits across 115 files becomes 1040 across 113. No entry was added and no line number moved, so the ratchet strictly tightened. Refs #1340 Co-authored-by: Nick DiZazzo <nick.dizazzo@gmail.com> Signed-off-by: Nick DiZazzo <nick.dizazzo@gmail.com>
`spawn_reader` discarded the `thread::Builder::spawn` result, so a failed spawn still left fd 1 and fd 2 pointing at a pipe with nothing draining it. The failure mode is the worst kind: everything works until the 64 KiB pipe buffer fills, and then every write to stdout or stderr — in this process and in every child that inherited those descriptors — blocks forever, while the dashboard keeps painting as if nothing is wrong. The spawn result is now propagated and `install` puts the saved descriptors back before returning the error. Capture is optional at the call site (`enter_terminal` treats a failure as "no capture"), so the dashboard still comes up — just without interception. Also stop dropping tabs from captured lines. `char::is_control` counts `\t`, and llama.cpp's loader lines are tab-separated, so stripping it ran two columns together; it degrades to a space instead. Raised by CodeRabbit on #1382. Refs #1340 Co-authored-by: Nick DiZazzo <nick.dizazzo@gmail.com> Signed-off-by: Nick DiZazzo <nick.dizazzo@gmail.com>
Two ways the repair could fail to repair.
The handler matched only `Char('r')`, but the status bar reads
`R Refresh` and Shift+R arrives as `Char('R')` — pressing the advertised
key did nothing. It now accepts either, with the existing guard that
keeps both as filter text while the events filter is being edited.
`render_if_dirty` also cleared `pending_full_repaint` with `mem::take`
before the fallible repair ran. If the erase failed, the request was gone
and the next dirty render was an ordinary diff against a screen ratatui
still believed was intact — so the damage survived a key press the
operator had already made. The flag is now cleared only after
`repair_tui_terminal` succeeds, and the propagated error leaves `dirty`
set so the next render retries.
Raised by CodeRabbit on #1382.
Refs #1340
Co-authored-by: Nick DiZazzo <nick.dizazzo@gmail.com>
Signed-off-by: Nick DiZazzo <nick.dizazzo@gmail.com>
1 participant
WIP
Summary by CodeRabbit
Bug Fixes
Improvements
Documentation