From ea3cc3d0d270bae3e2b7f09132ffac14f242f984 Mon Sep 17 00:00:00 2001 From: Hunter Bown Date: Thu, 12 Mar 2026 21:29:14 -0500 Subject: [PATCH] fix: address PR #4 follow-ups (#5) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix: address PR #4 follow-ups Honor low_motion in the default tool transcript path and align composer cursor padding with the rendered placeholder. Add focused regression tests for both behaviors. * lint: remove redundant imports in empty_state test, reuse create_test_app The test had inner `use` statements for Config, App, TuiOptions, and PathBuf that duplicated the module-level test imports. It also manually constructed App instead of calling the existing create_test_app() helper. * fix: replace useless format!("{text}") with text.to_string() in details_affordance_line * test: pin composer_density in cursor test to avoid sensitivity to loaded settings Settings::load() may return a non-default composer_density on some CI environments. Explicitly set ComposerDensity::Comfortable so the expected cursor position is deterministic across all platforms. * fix: make tool low_motion test robust against coarse Windows timers Use a 2× cycle offset so the animated frame index is 2 (maximally distant from 0), giving 1800 ms of headroom before the animation could wrap back to index 0. The previous 1× offset left only ~15 ms of margin, causing flaky failures on Windows where Instant resolution is approximately 15.6 ms. * fix: correct headroom comment in tool animation test (3600ms, not 1800ms) * fix: resolve lint, parity, and Windows test failures - Fix rustfmt line-length issue in history.rs tool animation test - Settings::path() now respects DEEPSEEK_CONFIG_PATH for Windows test compat - doctor_check_mcp_server recognizes Unix-style absolute paths on Windows - Use checked_sub for Instant arithmetic in web_run tests to prevent underflow on freshly-booted Windows CI runners * fix: expand ~ in DEEPSEEK_CONFIG_PATH when resolving settings path --------- --- crates/tui/src/config.rs | 2 +- crates/tui/src/main.rs | 4 +- crates/tui/src/settings.rs | 15 ++- crates/tui/src/tools/web_run.rs | 18 +++- crates/tui/src/tui/history.rs | 49 +++++++++- crates/tui/src/tui/widgets/mod.rs | 154 ++++++++++++++++++++++-------- 6 files changed, 195 insertions(+), 47 deletions(-) diff --git a/crates/tui/src/config.rs b/crates/tui/src/config.rs index ef14da88..1ca4fa35 100644 --- a/crates/tui/src/config.rs +++ b/crates/tui/src/config.rs @@ -525,7 +525,7 @@ fn default_requirements_path() -> Option { } } -fn expand_path(path: &str) -> PathBuf { +pub(crate) fn expand_path(path: &str) -> PathBuf { if let Some(stripped) = path.strip_prefix('~') && (stripped.is_empty() || stripped.starts_with('/') || stripped.starts_with('\\')) && let Some(mut home) = effective_home_dir() diff --git a/crates/tui/src/main.rs b/crates/tui/src/main.rs index 3b6ea3b0..404eabca 100644 --- a/crates/tui/src/main.rs +++ b/crates/tui/src/main.rs @@ -1918,7 +1918,9 @@ fn doctor_check_mcp_server(server: &McpServerConfig) -> McpServerDoctorStatus { } let cmd_path = Path::new(cmd); - let is_absolute = cmd_path.is_absolute(); + // Also accept Unix-style `/` prefix on Windows, where Path::is_absolute() + // requires a drive letter. + let is_absolute = cmd_path.is_absolute() || cmd.starts_with('/'); if is_absolute && !cmd_path.exists() { return McpServerDoctorStatus::Error(format!("command not found: {cmd}")); diff --git a/crates/tui/src/settings.rs b/crates/tui/src/settings.rs index cdfb10b5..ab7e0013 100644 --- a/crates/tui/src/settings.rs +++ b/crates/tui/src/settings.rs @@ -7,7 +7,7 @@ use std::path::PathBuf; use anyhow::{Context, Result}; use serde::{Deserialize, Serialize}; -use crate::config::normalize_model_name; +use crate::config::{expand_path, normalize_model_name}; /// User settings with defaults #[derive(Debug, Clone, Serialize, Deserialize)] @@ -64,6 +64,19 @@ impl Default for Settings { impl Settings { /// Get the settings file path pub fn path() -> Result { + // Allow tests to override the settings directory via the same env var + // used for config (DEEPSEEK_CONFIG_PATH points at config.toml; the + // settings file lives as a sibling in the same directory). + if let Ok(config_path) = std::env::var("DEEPSEEK_CONFIG_PATH") { + let config_path = config_path.trim(); + if !config_path.is_empty() { + let p = expand_path(config_path); + if let Some(parent) = p.parent() { + return Ok(parent.join("settings.toml")); + } + } + } + let config_dir = dirs::config_dir() .context("Failed to resolve config directory: not found.")? .join("deepseek"); diff --git a/crates/tui/src/tools/web_run.rs b/crates/tui/src/tools/web_run.rs index 46c3a365..67eb401c 100644 --- a/crates/tui/src/tools/web_run.rs +++ b/crates/tui/src/tools/web_run.rs @@ -1555,13 +1555,27 @@ mod tests { let ref_id = format!("{}turn0search1", scoped_ref_prefix(namespace)); store_page(namespace, &ref_id, sample_page("https://example.com/alpha")); - with_state(|state| { + // On Windows, Instant's epoch is system boot. If the CI runner has + // been up for less than WEB_RUN_SESSION_TTL the subtraction would + // underflow, so we skip the test in that case. + let stale = WEB_RUN_SESSION_TTL + Duration::from_secs(1); + let can_test = with_state(|state| { let session = state .sessions .get_mut(namespace) .expect("session should exist"); - session.last_access = Instant::now() - WEB_RUN_SESSION_TTL - Duration::from_secs(1); + match Instant::now().checked_sub(stale) { + Some(past) => { + session.last_access = past; + true + } + None => false, + } }); + if !can_test { + // System uptime shorter than session TTL; can't test eviction. + return; + } let _ = next_turn_for_namespace("session-beta"); diff --git a/crates/tui/src/tui/history.rs b/crates/tui/src/tui/history.rs index 5952ed1a..fd8ff54f 100644 --- a/crates/tui/src/tui/history.rs +++ b/crates/tui/src/tui/history.rs @@ -153,7 +153,10 @@ impl HistoryCell { } lines } - _ => self.lines(width), + HistoryCell::Tool(cell) => cell.lines_with_motion(width, options.low_motion), + HistoryCell::User { .. } + | HistoryCell::Assistant { .. } + | HistoryCell::System { .. } => self.lines(width), } } @@ -1444,7 +1447,7 @@ fn status_symbol(started_at: Option, status: ToolStatus, low_motion: bo fn details_affordance_line(text: &str, style: Style) -> Line<'static> { Line::from(vec![ Span::styled("▏ ", Style::default().fg(palette::TEXT_DIM)), - Span::styled(format!("{text}"), style), + Span::styled(text.to_string(), style), ]) } @@ -1628,7 +1631,11 @@ fn thinking_state_accent(state: ThinkingVisualState) -> Color { #[cfg(test)] mod tests { - use super::{extract_reasoning_summary, render_thinking}; + use super::{ + ExecCell, ExecSource, HistoryCell, TOOL_RUNNING_SYMBOLS, TOOL_STATUS_SYMBOL_MS, ToolCell, + ToolStatus, TranscriptRenderOptions, extract_reasoning_summary, render_thinking, + }; + use std::time::{Duration, Instant}; #[test] fn extract_reasoning_summary_prefers_summary_block() { @@ -1661,4 +1668,40 @@ mod tests { assert!(text.contains("summary only; press v for details")); assert!(text.contains("thinking")); } + + #[test] + fn tool_lines_with_options_respects_low_motion_in_default_path() { + // Use a 2× cycle offset so the animated frame lands on index 2, + // which is maximally far from index 0. This avoids flaky failures on + // platforms with coarse timer resolution (Windows ≈ 15.6 ms) and + // gives 3600 ms of headroom before the index could wrap back to 0 + // (indices 2 → 3 → 0 requires two more full cycles). + let started_at = Some(Instant::now() - Duration::from_millis(TOOL_STATUS_SYMBOL_MS * 2)); + let cell = HistoryCell::Tool(ToolCell::Exec(ExecCell { + command: "echo hi".to_string(), + status: ToolStatus::Running, + output: None, + started_at, + duration_ms: None, + source: ExecSource::Assistant, + interaction: None, + })); + + let animated = cell.lines_with_options(80, TranscriptRenderOptions::default()); + let low_motion = cell.lines_with_options( + 80, + TranscriptRenderOptions { + low_motion: true, + ..TranscriptRenderOptions::default() + }, + ); + + let animated_symbol = animated[0].spans[0].content.trim(); + let low_motion_symbol = low_motion[0].spans[0].content.trim(); + + // low_motion always pins to the first (static) frame. + assert_eq!(low_motion_symbol, TOOL_RUNNING_SYMBOLS[0]); + // The animated path should be on a different frame (index 2). + assert_ne!(animated_symbol, TOOL_RUNNING_SYMBOLS[0]); + } } diff --git a/crates/tui/src/tui/widgets/mod.rs b/crates/tui/src/tui/widgets/mod.rs index 7509b7a8..ec94a08c 100644 --- a/crates/tui/src/tui/widgets/mod.rs +++ b/crates/tui/src/tui/widgets/mod.rs @@ -213,7 +213,7 @@ impl Renderable for ComposerWidget<'_> { let mut input_lines = Vec::new(); if self.app.input.is_empty() { input_lines.push(Line::from(Span::styled( - "Write a task or use /.", + COMPOSER_PLACEHOLDER, Style::default().fg(palette::TEXT_MUTED).italic(), ))); } else { @@ -225,7 +225,16 @@ impl Renderable for ComposerWidget<'_> { } } - let top_padding = composer_vertical_padding(input_lines.len(), input_rows_budget); + // For non-empty input, input_lines.len() already reflects wrapping via + // layout_input. For the empty-input placeholder, Paragraph::wrap will + // wrap the single Line at render time, so we must estimate the wrapped + // row count ourselves to keep padding accurate on narrow widths. + let visual_rows = if self.app.input.is_empty() { + placeholder_visual_lines(content_width) + } else { + input_lines.len() + }; + let top_padding = composer_top_padding(visual_rows, input_rows_budget); let mut lines = Vec::new(); for _ in 0..top_padding { lines.push(Line::from("")); @@ -278,25 +287,18 @@ impl Renderable for ComposerWidget<'_> { let input_rows_budget = composer_input_rows_budget(inner_area.height, self.slash_menu_entries.len()); - let (_visible_lines, cursor_row, cursor_col) = layout_input( + let (visible_lines, cursor_row, cursor_col) = layout_input( &self.app.input, self.app.cursor_position, content_width, input_rows_budget, ); - let top_padding = if self.app.input.is_empty() { - let empty_lines = if self.app.input_history.is_empty() && input_rows_budget > 1 { - 2 - } else { - 1 - }; - composer_vertical_padding(empty_lines, input_rows_budget) + let visual_rows = if self.app.input.is_empty() { + placeholder_visual_lines(content_width) } else { - let visible_count = wrap_input_lines(&self.app.input, content_width) - .len() - .max(1); - composer_vertical_padding(visible_count.min(input_rows_budget), input_rows_budget) + visible_lines.len() }; + let top_padding = composer_top_padding(visual_rows, input_rows_budget); let cursor_x = area .x @@ -805,8 +807,16 @@ fn composer_input_rows_budget(inner_height: u16, extra_lines: usize) -> usize { usize::from(inner_height).saturating_sub(extra_lines).max(1) } -fn composer_vertical_padding(content_lines: usize, rows_budget: usize) -> usize { - rows_budget.saturating_sub(content_lines) +fn composer_top_padding(content_lines: usize, rows_budget: usize) -> usize { + rows_budget.saturating_sub(content_lines.clamp(1, rows_budget)) +} + +/// Placeholder text shown when the composer input is empty. +const COMPOSER_PLACEHOLDER: &str = "Write a task or use /."; + +/// How many visual rows the empty-input placeholder occupies after wrapping. +fn placeholder_visual_lines(content_width: usize) -> usize { + wrap_text(COMPOSER_PLACEHOLDER, content_width).len().max(1) } fn composer_min_input_rows(density: ComposerDensity) -> usize { @@ -1021,18 +1031,42 @@ fn wrap_text(text: &str, width: usize) -> Vec { #[cfg(test)] mod tests { use super::{ - COMPOSER_PANEL_HEIGHT, apply_selection_to_line, composer_height, composer_max_height, - composer_min_input_rows, cursor_row_col, layout_input, pad_lines_to_bottom, + COMPOSER_PANEL_HEIGHT, ComposerWidget, Renderable, apply_selection_to_line, + composer_height, composer_max_height, composer_min_input_rows, composer_top_padding, + cursor_row_col, layout_input, pad_lines_to_bottom, placeholder_visual_lines, should_render_empty_state, slash_completion_hints, wrap_input_lines, wrap_text, }; + use crate::config::Config; use crate::palette; - use crate::tui::app::ComposerDensity; + use crate::tui::app::{App, ComposerDensity, TuiOptions}; use ratatui::{ + layout::Rect, style::Style, text::{Line, Span}, }; + use std::path::PathBuf; use unicode_width::UnicodeWidthStr; + fn create_test_app() -> App { + let options = TuiOptions { + model: "deepseek-chat".to_string(), + workspace: PathBuf::from("."), + allow_shell: false, + use_alt_screen: true, + max_subagents: 1, + skills_dir: PathBuf::from("."), + memory_path: PathBuf::from("memory.md"), + notes_path: PathBuf::from("notes.txt"), + mcp_config_path: PathBuf::from("mcp.json"), + use_memory: false, + start_in_agent_mode: true, + skip_onboarding: true, + yolo: false, + resume_session_id: None, + }; + App::new(options, &Config::default()) + } + #[test] fn pad_lines_to_bottom_noop_when_already_filled() { let mut lines = vec![Line::from("one"), Line::from("two")]; @@ -1274,28 +1308,70 @@ mod tests { } #[test] - fn empty_state_renders_only_without_transcript_activity() { - use crate::config::Config; - use crate::tui::app::{App, TuiOptions}; - use std::path::PathBuf; + fn empty_composer_cursor_matches_placeholder_padding() { + let mut app = create_test_app(); + // Pin density so the test is independent of any loaded user settings. + app.composer_density = ComposerDensity::Comfortable; + let slash_menu_entries = Vec::::new(); + let widget = ComposerWidget::new(&app, 5, &slash_menu_entries); - let options = TuiOptions { - model: "deepseek-chat".to_string(), - workspace: PathBuf::from("."), - allow_shell: false, - use_alt_screen: true, - max_subagents: 1, - skills_dir: PathBuf::from("."), - memory_path: PathBuf::from("memory.md"), - notes_path: PathBuf::from("notes.txt"), - mcp_config_path: PathBuf::from("mcp.json"), - use_memory: false, - start_in_agent_mode: true, - skip_onboarding: true, - yolo: false, - resume_session_id: None, + // Use a wide area so the placeholder fits on one line (no wrapping). + let area = Rect { + x: 0, + y: 0, + width: 40, + height: 5, }; - let mut app = App::new(options, &Config::default()); + + // inner_area: {x:1, y:1, w:38, h:3} (borders shrink by 1 each side) + // input_rows_budget = 3 + // placeholder_visual_lines(38) = 1 (placeholder is 22 chars, fits in 38) + // top_padding = 3 - clamp(1, 1, 3) = 2 + // cursor_x = 0 + (1-0) + 0 = 1 + // cursor_y = 0 + (1-0) + (2+0) = 3 + assert_eq!(widget.cursor_pos(area), Some((1, 3))); + } + + #[test] + fn empty_composer_cursor_accounts_for_placeholder_wrapping() { + let mut app = create_test_app(); + app.composer_density = ComposerDensity::Comfortable; + let slash_menu_entries = Vec::::new(); + let widget = ComposerWidget::new(&app, 5, &slash_menu_entries); + + // Narrow area forces the placeholder to wrap. + let area = Rect { + x: 0, + y: 0, + width: 14, + height: 5, + }; + + // inner_area: {x:1, y:1, w:12, h:3} + // input_rows_budget = 3 + // placeholder_visual_lines(12) = 2 ("Write a task" / " or use /.") + // top_padding = 3 - clamp(2, 1, 3) = 1 + // cursor_x = 0 + (1-0) + 0 = 1 + // cursor_y = 0 + (1-0) + (1+0) = 2 + assert_eq!(placeholder_visual_lines(12), 2); + assert_eq!(widget.cursor_pos(area), Some((1, 2))); + } + + #[test] + fn composer_top_padding_uses_clamp() { + // content_lines=0 is clamped to 1 + assert_eq!(composer_top_padding(0, 3), 2); + // content_lines=1 + assert_eq!(composer_top_padding(1, 3), 2); + // content_lines=3 fills the budget + assert_eq!(composer_top_padding(3, 3), 0); + // content_lines > budget is clamped + assert_eq!(composer_top_padding(5, 3), 0); + } + + #[test] + fn empty_state_renders_only_without_transcript_activity() { + let mut app = create_test_app(); assert!(should_render_empty_state(&app)); app.add_message(crate::tui::history::HistoryCell::User { content: "hello".to_string(),