feat(tui): add render diff debug log#2332
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a render debugging feature to the TUI color compatibility backend, allowing frame rendering metrics and cell diff samples to be logged when the CODEWHALE_TUI_DEBUG environment variable is enabled. The feedback recommends addressing a directory resolution issue to ensure new installations default to the .codewhale directory instead of the legacy .deepseek directory. Additionally, the reviewer suggests optimizing the debug line formatting to avoid intermediate allocations and using an RAII guard in tests to guarantee environment variables are restored even in the event of a panic.
| pub(crate) fn log_directory() -> Option<PathBuf> { | ||
| let resolve = |base: PathBuf| -> Option<PathBuf> { | ||
| let primary = base.join(".codewhale").join("logs"); | ||
| if primary.exists() { |
There was a problem hiding this comment.
Branding & Directory Creation Issue
Currently, log_directory() resolves to .deepseek/logs if .codewhale/logs does not exist, even if .deepseek/logs also does not exist. For a new user/installation, this will result in the creation of the legacy ~/.deepseek directory instead of the new ~/.codewhale directory.
To fix this and ensure that new installations correctly use the .codewhale directory while maintaining backward compatibility for existing users, the resolution logic should check if .deepseek/logs exists, and if not, default to .codewhale/logs.
Here is how the resolve closure can be updated:
let resolve = |base: PathBuf| -> Option<PathBuf> {
let primary = base.join(".codewhale").join("logs");
if primary.exists() {
return Some(primary);
}
let legacy = base.join(".deepseek").join("logs");
if legacy.exists() {
return Some(legacy);
}
Some(primary)
};Note: You will also need to update the corresponding tests (such as log_directory_prefers_home in runtime_log.rs and backend_writes_render_debug_log_when_enabled in color_compat.rs) to expect .codewhale instead of .deepseek for clean environments.
| fn render_debug_line( | ||
| frame: u64, | ||
| viewport: Option<Size>, | ||
| diff_cells: usize, | ||
| sample: &[(u16, u16)], | ||
| ) -> String { | ||
| let size = viewport | ||
| .map(|size| format!("{}x{}", size.width, size.height)) | ||
| .unwrap_or_else(|| "unknown".to_string()); | ||
| let sample = sample | ||
| .iter() | ||
| .map(|(x, y)| format!("{x}:{y}")) | ||
| .collect::<Vec<_>>() | ||
| .join(","); | ||
| format!("frame={frame} size={size} diff_cells={diff_cells} sample={sample}\n") | ||
| } |
There was a problem hiding this comment.
Performance Optimization: Avoid Intermediate Allocations
Since render_debug_line is called on every frame when debug logging is enabled, we should avoid allocating intermediate Strings and Vecs for the viewport size and coordinate samples.
Formatting directly into a single pre-allocated String buffer eliminates these allocations entirely, improving rendering performance and reducing GC/allocator pressure.
fn render_debug_line(
frame: u64,
viewport: Option<Size>,
diff_cells: usize,
sample: &[(u16, u16)],
) -> String {
use std::fmt::Write;
let mut line = String::new();
write!(line, "frame={frame} size=").unwrap();
if let Some(size) = viewport {
write!(line, "{}x{}", size.width, size.height).unwrap();
} else {
line.push_str("unknown");
}
write!(line, " diff_cells={diff_cells} sample=").unwrap();
for (i, (x, y)) in sample.iter().enumerate() {
if i > 0 {
line.push(',');
}
write!(line, "{x}:{y}").unwrap();
}
line.push('\n');
line
}| fn backend_writes_render_debug_log_when_enabled() { | ||
| let _lock = lock_test_env(); | ||
| let tmp = tempfile::tempdir().expect("tempdir"); | ||
| let previous_home = env::var_os("HOME"); | ||
| let previous_userprofile = env::var_os("USERPROFILE"); | ||
| let previous_debug = env::var_os(RENDER_DEBUG_ENV); | ||
|
|
||
| // SAFETY: environment mutation is serialized by lock_test_env. | ||
| unsafe { | ||
| env::set_var("HOME", tmp.path()); | ||
| env::set_var("USERPROFILE", ""); | ||
| env::set_var(RENDER_DEBUG_ENV, "1"); | ||
| } | ||
|
|
||
| let writer = SharedWriter::default(); | ||
| let mut backend = ColorCompatBackend::new(writer, ColorDepth::TrueColor, PaletteMode::Dark); | ||
| let mut cell = Cell::default(); | ||
| cell.set_symbol("x"); | ||
| backend.draw(std::iter::once((3, 4, &cell))).unwrap(); | ||
|
|
||
| let log_path = tmp | ||
| .path() | ||
| .join(".deepseek") | ||
| .join("logs") | ||
| .join("tui-render.log"); | ||
| let body = fs::read_to_string(log_path).expect("render debug log"); | ||
| assert!(body.contains("frame=1"), "{body}"); | ||
| assert!(body.contains("diff_cells=1"), "{body}"); | ||
| assert!(body.contains("sample=3:4"), "{body}"); | ||
|
|
||
| // SAFETY: environment mutation is serialized by lock_test_env. | ||
| unsafe { | ||
| match previous_home { | ||
| Some(value) => env::set_var("HOME", value), | ||
| None => env::remove_var("HOME"), | ||
| } | ||
| match previous_userprofile { | ||
| Some(value) => env::set_var("USERPROFILE", value), | ||
| None => env::remove_var("USERPROFILE"), | ||
| } | ||
| match previous_debug { | ||
| Some(value) => env::set_var(RENDER_DEBUG_ENV, value), | ||
| None => env::remove_var(RENDER_DEBUG_ENV), | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Robustness: Use RAII Guard for Environment Variable Cleanup
If this test panics or fails during execution (e.g., at the fs::read_to_string or assert! statements), the environment variables HOME, USERPROFILE, and CODEWHALE_TUI_DEBUG will not be restored because the cleanup block at the end of the function is skipped during stack unwinding. This can pollute the environment for other tests running in the same process, leading to flaky test suites.
Using an RAII guard (Drop implementation) guarantees that the environment variables are always restored, even if the test panics.
fn backend_writes_render_debug_log_when_enabled() {
let _lock = lock_test_env();
let tmp = tempfile::tempdir().expect("tempdir");
struct EnvGuard {
previous_home: Option<std::ffi::OsString>,
previous_userprofile: Option<std::ffi::OsString>,
previous_debug: Option<std::ffi::OsString>,
}
impl Drop for EnvGuard {
fn drop(&mut self) {
// SAFETY: environment mutation is serialized by lock_test_env.
unsafe {
match &self.previous_home {
Some(value) => env::set_var("HOME", value),
None => env::remove_var("HOME"),
}
match &self.previous_userprofile {
Some(value) => env::set_var("USERPROFILE", value),
None => env::remove_var("USERPROFILE"),
}
match &self.previous_debug {
Some(value) => env::set_var(RENDER_DEBUG_ENV, value),
None => env::remove_var(RENDER_DEBUG_ENV),
}
}
}
}
let _env_guard = EnvGuard {
previous_home: env::var_os("HOME"),
previous_userprofile: env::var_os("USERPROFILE"),
previous_debug: env::var_os(RENDER_DEBUG_ENV),
};
// SAFETY: environment mutation is serialized by lock_test_env.
unsafe {
env::set_var("HOME", tmp.path());
env::set_var("USERPROFILE", "");
env::set_var(RENDER_DEBUG_ENV, "1");
}
let writer = SharedWriter::default();
let mut backend = ColorCompatBackend::new(writer, ColorDepth::TrueColor, PaletteMode::Dark);
let mut cell = Cell::default();
cell.set_symbol("x");
backend.draw(std::iter::once((3, 4, &cell))).unwrap();
let log_path = tmp
.path()
.join(".deepseek")
.join("logs")
.join("tui-render.log");
let body = fs::read_to_string(log_path).expect("render debug log");
assert!(body.contains("frame=1"), "{body}");
assert!(body.contains("diff_cells=1"), "{body}");
assert!(body.contains("sample=3:4"), "{body}");
}![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| tracing::debug!(?err, "failed to create TUI render debug log directory"); | ||
| return None; | ||
| } | ||
| let path = log_dir.join("tui-render.log"); |
There was a problem hiding this comment.
tui-render.log falls under the 7-day pruning filter
is_tui_log_file_name in runtime_log.rs matches any file whose name starts with "tui-" and ends with ".log". tui-render.log satisfies both conditions, so prune_old_logs — called every time runtime_log::init() runs — will silently delete this file once its modification time is older than the configured retention window (default 7 days). Because RenderDebugLog opens the file in append mode, the expected use is accumulating data across many sessions; pruning defeats that intent and destroys the debugging history without any user-visible warning. The per-session rotating files (tui-YYYY-MM-DD-PID.log) follow a different access pattern and the pruning rule was written for them, not for this fixed-name accumulating file.
| let file = OpenOptions::new() | ||
| .create(true) | ||
| .append(true) | ||
| .open(&path) | ||
| .map_err(|err| { | ||
| tracing::debug!(?err, path = %path.display(), "failed to open TUI render debug log"); | ||
| err | ||
| }) | ||
| .ok()?; |
There was a problem hiding this comment.
tui-render.log is opened in append mode with no size limit or rotation. At typical ratatui render rates (up to 60 FPS), each frame line can be ~100–300 bytes, meaning several hundred MB per hour under heavy use. Since this is an opt-in debug flag, the risk is lower, but users who leave CODEWHALE_TUI_DEBUG=1 set for extended sessions could accumulate surprisingly large files with no warning.
2 participants
Summary
Notes
CODEWHALE_TUI_DEBUG=1#2085 is already present on main. This PR covers the missing debug-log portion.Testing
Part of #2085.
Greptile Summary
This PR adds an opt-in render-diff debug log (
CODEWHALE_TUI_DEBUG=1) to the TUI backend and migrates thelog_directory()fallback from.deepseek/logsto.codewhale/logswhen neither directory exists yet.runtime_log.rs:log_directory()is promoted topub(crate), the default-fallback path is now.codewhale/logs, and a new test covers the legacy.deepseek/logsdetection branch.color_compat.rs: A newRenderDebugLogstruct is wired intoColorCompatBackend::draw(). When the env var is truthy, each frame appends a structured line (frame number, viewport size, diff cell count, up to 24 sampled coordinates) totui-render.login the existing TUI log directory. Cell text/content is deliberately omitted.Confidence Score: 5/5
Safe to merge; the feature is fully opt-in behind an env var and the log-directory migration is backwards-compatible with existing
.deepseek/logsinstalls.The render-diff logging is gated behind
CODEWHALE_TUI_DEBUG=1, making it invisible to normal users. Thelog_directory()migration is backward-compatible — existing.deepseek/logsdirectories are detected and preserved. All error paths fail gracefully. The sole concern is log usability across sessions, not correctness.No files require special attention beyond the session-boundary suggestion on
color_compat.rs.Important Files Changed
log_directory()pub(crate), changed fallback path from.deepseek/logsto.codewhale/logswhen neither directory exists, added legacy-directory test, and cleaned up doc comments.RenderDebugLogthat appends per-frame size, diff cell count, and sampled coordinates totui-render.log; no session boundary markers written between sessions, making multi-session log analysis difficult.Sequence Diagram
sequenceDiagram participant Env as Environment participant Backend as ColorCompatBackend participant RDL as RenderDebugLog participant LogDir as log_directory() participant File as tui-render.log Backend->>Env: var("CODEWHALE_TUI_DEBUG") Env-->>Backend: "1" (enabled) Backend->>LogDir: log_directory() LogDir-->>Backend: ~/.codewhale/logs Backend->>File: OpenOptions::append(true).open() File-->>Backend: "RenderDebugLog { file, frame: 0 }" loop Each draw() call Backend->>Backend: adapt cell colors Backend->>Backend: "self.size() -> viewport" Backend->>RDL: "record(viewport, &adapted_cells)" RDL->>RDL: "frame += 1, sample first 24 (x,y)" RDL->>File: "write frame=N size=WxH diff_cells=M sample=..." Backend->>Backend: inner.draw(adapted_cells) endReviews (2): Last reviewed commit: "fix(tui): keep render debug logs in code..." | Re-trigger Greptile