Feat/config file - #67
Conversation
WalkthroughAdds a TOML-based configuration system with XDG paths and generation, integrates Config across startup, logging, sync, and UI (including display options and mouse capture), updates render loop and component APIs to accept Config, adds file logging, and updates docs and tests. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests
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 |
3579a3c to
795fdda
Compare
- Added a new `config` module to handle loading, parsing, and validation of configuration files. - Introduced a default configuration structure with options for UI, sync, display, and logging settings. - Implemented functionality to generate a default configuration file and load existing configurations. - Updated the main application to support configuration generation and loading. - Enhanced the logger to support file logging based on configuration settings. - Modified UI components to utilize configuration settings for display options and sidebar behavior.
795fdda to
16f5066
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/ui/app_component.rs (2)
81-88: UI logger overrides config‑driven file logger from SyncService.
SyncService::new(.., &config)already picks file logging when enabled. Creating a freshLogger::new()here and callingset_loggerdiscards that, so file logging never happens.Apply this diff to mirror config and preserve file logging:
- let (task_manager, background_action_rx) = TaskManager::new(); - let logger = Logger::new(); - sync_service.set_logger(logger.clone()); + let (task_manager, background_action_rx) = TaskManager::new(); + let logger = if config.logging.enabled { + Logger::new_with_file_logging().unwrap_or_else(|_| Logger::new()) + } else { + Logger::new() + }; + sync_service.set_logger(logger.clone());Alternatively (preferred): expose
fn logger(&self) -> Option<&Logger>onSyncServiceand reuse its logger instead of creating a new one.
653-665: Apply initial selection after first data load (once).Set the configured selection after
update_dataso IDs can resolve. Gate it so subsequent syncs don’t override the user.Apply this diff (adds a simple one‑shot flag):
@@ - // Update app state with loaded data - self.state.update_data(projects, labels, sections, tasks); + // Update app state with loaded data + self.state.update_data(projects, labels, sections, tasks); + // One-shot apply of initial sidebar selection now that data is present + static ONCE: std::sync::Once = std::sync::Once::new(); + ONCE.call_once(|| { + // Safe to ignore logs here; just align to config at startup + self.set_initial_sidebar_selection(); + });If you prefer not to use a static, add a
did_apply_initial_sidebar: boolfield onAppComponentand flip it after applying.src/ui/components/task_list_item_component.rs (1)
197-229: Potential panic: slicing UTF‑8 with byte indices; width math is incorrectUsing description_line[..available_width-3] can panic on non‑ASCII. Also len() counts bytes, not display cells, so truncation is visually wrong with emojis/wide chars.
Fix slice and improve width estimation:
- // Calculate used width so far (approximation using string length) - let mut used_width = 0; - for span in &line_spans { - used_width += span.content.len(); - } + // Use ratatui width calculation for existing spans + let used_width = Line::from(line_spans.clone()).width(); // Reserve some space for padding and ensure we don't overflow if used_width < max_width.saturating_sub(10) { let available_width = max_width - used_width - 3; // Reserve space for " - " prefix // Get first line of description and truncate if needed let description_line = self.task.description.lines().next().unwrap_or(""); - let description_text = if description_line.len() > available_width { - if available_width > 3 { - format!("{}...", &description_line[..available_width.saturating_sub(3)]) - } else { - "...".to_string() - } - } else { - description_line.to_string() - }; + let description_text = if description_line.chars().count() > available_width { + if available_width > 3 { + let head: String = description_line.chars().take(available_width.saturating_sub(3)).collect(); + format!("{}...", head) + } else { + "...".to_string() + } + } else { + description_line.to_string() + };Optional: for accurate display width, switch to unicode-width and truncate by columns; I can wire this if you prefer.
🧹 Nitpick comments (24)
Cargo.toml (1)
38-39: Preferdirectoriesover deprecateddirs.The
dirscrate is deprecated/unmaintained. Usedirectories(ordirs-next) for robust, cross‑platform config paths.Apply this diff:
toml = "0.8" -dirs = "5.0" +directories = "5"I’ve proposed corresponding code changes in src/config.rs to switch to
ProjectDirs.src/config.rs (3)
5-8: Switch todirectories::ProjectDirsfor XDG paths.This aligns with the dependency change and yields consistent paths on all OSes.
use anyhow::{Context, Result}; use serde::{Deserialize, Serialize}; use std::path::{Path, PathBuf}; +use directories::ProjectDirs; @@ - // 2. Check XDG config directory - if let Some(config_dir) = dirs::config_dir() { - let xdg_config = config_dir.join("terminalist").join("config.toml"); + // 2. Check XDG/OS config directory (ProjectDirs) + if let Some(proj_dirs) = ProjectDirs::from("net", "doxin", "terminalist") { + let xdg_config = proj_dirs.config_dir().join("config.toml"); if xdg_config.exists() { return Ok(Some(xdg_config)); } } @@ - pub fn get_xdg_config_dir() -> Result<PathBuf> { - dirs::config_dir() - .ok_or_else(|| anyhow::anyhow!("Could not determine config directory")) - .map(|dir| dir.join("terminalist")) - } + pub fn get_xdg_config_dir() -> Result<PathBuf> { + let proj_dirs = ProjectDirs::from("net", "doxin", "terminalist") + .ok_or_else(|| anyhow::anyhow!("Could not determine config directory"))?; + Ok(proj_dirs.config_dir().to_path_buf()) + }Also applies to: 141-147, 203-208
133-150: Consider supporting./config.tomlalongside./terminalist.toml.Small DX improvement: check both names in CWD for symmetry with XDG
config.toml.// 1. Check current directory let current_dir_config = PathBuf::from("terminalist.toml"); if current_dir_config.exists() { return Ok(Some(current_dir_config)); } + let current_dir_config_alt = PathBuf::from("config.toml"); + if current_dir_config_alt.exists() { + return Ok(Some(current_dir_config_alt)); + }
199-200: Avoid printing from library code.Consider returning the path and letting the CLI decide what to print, or gate behind a verbosity flag.
src/ui/components/dialog_component.rs (1)
66-68: Take&DisplayConfigto avoid unnecessary moves (tiny nit).Pass by reference and clone only if needed.
- pub fn update_display_config(&mut self, display_config: DisplayConfig) { - self.display_config = display_config; + pub fn update_display_config(&mut self, display_config: &DisplayConfig) { + self.display_config = display_config.clone(); }src/logger.rs (3)
41-47: Use XDG config dir for log path (Windows/macOS portability).Prefer
dirs::config_dir()overhome_dir()/.configto honor platform conventions and avoid hardcoding “.config”.Apply this diff:
- fn get_log_file_path() -> io::Result<PathBuf> { - let home_dir = - dirs::home_dir().ok_or_else(|| io::Error::new(io::ErrorKind::NotFound, "Home directory not found"))?; - - Ok(home_dir.join(".config").join("terminalist").join("terminalist.log")) - } + fn get_log_file_path() -> io::Result<PathBuf> { + let config_dir = dirs::config_dir() + .ok_or_else(|| io::Error::new(io::ErrorKind::NotFound, "Config directory not found"))?; + Ok(config_dir.join("terminalist").join("terminalist.log")) + }
59-67: Avoid reopening the file on every log call.Opening the file for each write is unnecessary I/O and can interleave writes under concurrency. Keep a
std::fs::Filebehind aMutexand write to it.Follow‑up: add
file: Arc<Mutex<Option<std::fs::File>>>toLogger, open once innew_with_file_logging(), and reuse inlog(); lazily (re)open on error if needed.
49-58: Unbounded in‑memory log growth.
logs: Vec<String>grows forever. Consider a ring buffer (e.g., cap to N entries) to avoid unbounded memory use in long sessions.src/ui/app_component.rs (3)
1045-1047: Width math can underflow/overflow on small terminals; clamp percent.
rect.width - 20can underflow for widths < 20 (u16). Also,width * percentmay overflow in release builds. Clamp the percent and usesaturating_sub.Apply this diff:
- let sidebar_width = (rect.width * self.config.ui.sidebar_width / 100).min(rect.width - 20); + let pct = self.config.ui.sidebar_width.clamp(10, 40) as u16; + let max_main = rect.width.saturating_sub(20); + let calc = ((rect.width as u32) * (pct as u32) / 100) as u16; + let sidebar_width = calc.min(max_main);
137-176: Initial sidebar selection runs before data loads; likely falls back to Today.When
projects/labelsare empty at startup, “inbox” or specific project IDs won’t resolve. Defer until data is loaded, then apply once.Apply this diff to guard early, and then call it after data load (see next comment):
fn set_initial_sidebar_selection(&mut self) { + // Defer until we have data to resolve inbox/project IDs + if self.state.projects.is_empty() && !matches!(self.config.ui.default_project.as_str(), "today" | "tomorrow" | "upcoming") { + return; + }
968-971: Mouse hit testing ignores configurable sidebar width.The hardcoded
30columns contradicts the configurable sidebar width. Compute a threshold from the same percentage used inrender(e.g., via terminal width or cached lastRect).src/ui/components/task_list_component.rs (1)
311-319: Avoid cloning heavy data per item.
self.icons.clone()andself.projects.clone()are cloned for every task. With large project lists, this is costly.Prefer sharing: store
Arc<IconService>andArc<Vec<ProjectDisplay>>inTaskItem(or pass references if lifetimes allow) to avoid per‑item clones.src/main.rs (3)
45-58: Config generation path mismatches docs.Docs say
--generate-configcreates./terminalist.toml, but code writes to the XDG path (~/.config/terminalist/config.toml). Align behavior or update docs.Two options:
- Keep XDG: update docs to say it writes to the default XDG path.
- Prefer project-local: write to
./terminalist.tomlif not present; otherwise to XDG.
55-57: Remove stale TODO.You already return immediately after generation.
Apply this diff:
- // TODO: make sure we return after this, the app should not run.
15-21: Consider using clap for CLI parsing.Manual arg scanning is brittle as flags grow.
clap(orargh) improves UX, validation, and help/version handling.docs/CONFIG_FILE_FEATURE.md (5)
45-50: Doc/code mismatch: generation target path.Text says the generator creates
terminalist.tomlin the current directory, butmain.rsgenerates in the XDG config dir. Please align and clarify precedence.Suggested wording: “Generates a default config at the XDG path (~/.config/terminalist/config.toml). If you prefer a project‑local config, copy it next to your project as ./terminalist.toml.”
30-34: Fix MD026: remove trailing punctuation in heading.Drop the colon.
Apply this diff:
-### Default Locations (in order of precedence): +### Default locations (in order of precedence)
136-139: Fix MD001: heading level jump.Use h2 to keep levels incremental.
Apply this diff:
-### 4. Display Configuration +## 4. Display Configuration
161-167: Fix MD001: heading level jump.Use h2 for this section as well.
Apply this diff:
-### 5. Logging Configuration +## 5. Logging Configuration
232-235: Minor: XDG phrasing and path clarity.Consider explicitly noting Windows/macOS equivalents for XDG (e.g., AppData/Roaming on Windows) to avoid confusion.
src/ui/renderer.rs (2)
33-33: Avoid cloning Config across the treeIf Config grows, cloning here propagates copies to many components. Consider passing Arc (or &Config where ownership isn’t needed) to reduce copies.
- let mut app = AppComponent::new(sync_service, config.clone()); + let config = std::sync::Arc::new(config); + let mut app = AppComponent::new(sync_service, config.clone());
69-71: Intervals are unused in the loop_cleanup_interval and _render_interval are never ticked, so they don’t drive any behavior. Either wire them via select! or remove to avoid confusion.
Apply this minimal wiring:
-async fn run_app_loop<B: Backend>( +async fn run_app_loop<B: Backend>( terminal: &mut Terminal<B>, app: &mut AppComponent, event_handler: &mut EventHandler, - _cleanup_interval: &mut tokio::time::Interval, - _render_interval: &mut tokio::time::Interval, + cleanup_interval: &mut tokio::time::Interval, + render_interval: &mut tokio::time::Interval, ) -> anyhow::Result<()> { let mut needs_render = true; loop { - // Render when needed + // Render when needed if needs_render { terminal.draw(|f| app.render(f, f.area()))?; needs_render = false; } - // Simplified event loop to avoid deadlocks - let event_result = event_handler.next_event().await?; - - match event_result { + // Drive events and periodic tasks + let event_result = tokio::select! { + _ = render_interval.tick() => crate::ui::core::EventType::Render, + _ = cleanup_interval.tick() => crate::ui::core::EventType::Tick, + ev = event_handler.next_event() => ev?, + }; + + match event_result { EventType::Key(_) | EventType::Mouse(_) | EventType::Resize(_, _) => { app.handle_event(event_result).await?; needs_render = true; } EventType::Tick => { // Process background actions on tick (less frequent) let background_actions = app.process_background_actions();src/ui/components/task_list_item_component.rs (2)
145-155: Project color handling is stubbedBoth branches use Color::Cyan; when show_project_colors is true, map the Todoist project color to a ratatui Color.
- let project_style = if display_config.show_project_colors { - // Use project color if available, otherwise cyan - Style::default().fg(Color::Cyan) - } else { - Style::default().fg(Color::Cyan) - }; + let project_style = { + let fg = if display_config.show_project_colors { + todoist_color_to_ratatui(&project.color).unwrap_or(Color::Cyan) + } else { + Color::Cyan + }; + Style::default().fg(fg) + };Add once in this module:
fn todoist_color_to_ratatui(name: &str) -> Option<Color> { match name.to_lowercase().as_str() { "red" => Some(Color::Red), "blue" => Some(Color::Blue), "green" => Some(Color::Green), "yellow" => Some(Color::Yellow), "orange" => Some(Color::Rgb(255,165,0)), "purple" => Some(Color::Magenta), "cyan" => Some(Color::Cyan), "gray" | "grey" => Some(Color::Gray), _ => None, } }
286-295: Remove unused variable_line_width is computed and never used.
- let _line_width = max_width.saturating_sub(self.indent * 4);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (12)
Cargo.toml(1 hunks)docs/CONFIG_FILE_FEATURE.md(1 hunks)src/config.rs(1 hunks)src/lib.rs(1 hunks)src/logger.rs(1 hunks)src/main.rs(5 hunks)src/sync.rs(2 hunks)src/ui/app_component.rs(6 hunks)src/ui/components/dialog_component.rs(4 hunks)src/ui/components/task_list_component.rs(4 hunks)src/ui/components/task_list_item_component.rs(8 hunks)src/ui/renderer.rs(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (8)
src/sync.rs (2)
src/logger.rs (2)
new(33-39)new_with_file_logging(17-30)src/ui/app_component.rs (1)
new(82-107)
src/ui/components/task_list_component.rs (2)
src/config.rs (5)
default(62-69)default(73-79)default(83-87)default(91-100)default(104-106)src/ui/components/dialog_component.rs (2)
default(39-41)update_display_config(66-68)
src/ui/components/task_list_item_component.rs (3)
src/ui/components/task_list_component.rs (2)
render(493-511)default(33-35)src/config.rs (5)
default(62-69)default(73-79)default(83-87)default(91-100)default(104-106)src/ui/components/badge.rs (1)
create_task_badges(30-51)
src/ui/app_component.rs (4)
src/ui/components/dialog_component.rs (1)
new(45-64)src/sync.rs (1)
new(29-53)src/ui/components/task_list_component.rs (1)
new(39-52)src/ui/components/sidebar_component.rs (1)
new(29-39)
src/logger.rs (3)
src/sync.rs (3)
sync(434-454)new(29-53)log(61-65)src/ui/app_component.rs (1)
new(82-107)src/ui/core/context.rs (1)
new(18-27)
src/ui/components/dialog_component.rs (2)
src/config.rs (5)
default(62-69)default(73-79)default(83-87)default(91-100)default(104-106)src/ui/components/task_list_item_component.rs (5)
render(15-15)render(33-39)render(97-231)render(256-262)render(286-295)
src/main.rs (4)
src/config.rs (3)
get_default_config_path(211-213)generate_default_config(184-201)load(111-119)src/sync.rs (2)
sync(434-454)new(29-53)src/ui/app_component.rs (1)
new(82-107)src/ui/renderer.rs (1)
run_app(18-63)
src/ui/renderer.rs (1)
src/ui/app_component.rs (1)
new(82-107)
🪛 markdownlint-cli2 (0.17.2)
docs/CONFIG_FILE_FEATURE.md
30-30: Trailing punctuation in heading
Punctuation: ':'
(MD026, no-trailing-punctuation)
136-136: Heading levels should only increment by one level at a time
Expected: h2; Actual: h3
(MD001, heading-increment)
161-161: Heading levels should only increment by one level at a time
Expected: h2; Actual: h3
(MD001, heading-increment)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Test (windows-latest, beta)
- GitHub Check: Test (windows-latest, stable)
- GitHub Check: Security Audit
🔇 Additional comments (11)
src/lib.rs (1)
1-1: Publicconfigmodule exposure looks good.src/ui/components/dialog_component.rs (1)
35-35: DisplayConfig threading into dialogs/rendering looks solid.Also applies to: 62-63, 443-444
src/sync.rs (1)
29-29: Breaking signature — update all SyncService::new call sites to include Config
Automated verification failed due to script errors; confirm there are no remaining 2‑argument calls to SyncService::new after the signature change in src/sync.rs:29 and update any occurrences to pass the new Config argument.src/ui/components/task_list_component.rs (3)
29-31: LGTM: DisplayConfig is threaded into the list component.Good defaulting and public exposure for runtime updates.
54-56: LGTM: Simple runtime update API for DisplayConfig.Straightforward setter; no side effects.
385-396: LGTM: Rendering honors DisplayConfig.Passing
&self.display_configthroughrenderis consistent with the new contract.src/ui/renderer.rs (2)
22-28: Mouse capture toggled by config: looks goodConditional Enable/DisableMouseCapture mirrors config.ui.mouse_enabled; teardown is symmetric and after disable_raw_mode, which matches common crossterm patterns.
Also applies to: 53-58
18-18: Resolved — run_app signature change verifiedDefinition at src/ui/renderer.rs:18; only call found at src/main.rs:84 (ui::run_app(sync_service, config).await?). No remaining callers of the old signature.
src/ui/components/task_list_item_component.rs (3)
33-38: Enum delegations correctly forward display_configVariant dispatch is clean and consistent.
256-262: HeaderItem render change: OKSignature updated and usage is straightforward.
15-15: Trait signature widened with DisplayConfig — verifiedAll ListItem implementors were updated and the list call site was adjusted. Trait + impls: src/ui/components/task_list_item_component.rs; list call site: src/ui/components/task_list_component.rs:393 (item.render(max_width, false, &self.display_config)). No other ListItem impls or render call sites found.
There was a problem hiding this comment.
Actionable comments posted: 12
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/PRD.md (1)
426-436: Sync docs/PRD.md dependency versions with Cargo.toml
- todoist-api mismatch — docs/PRD.md lists todoist-api = "0.2.0" but root Cargo.toml (line 37) has todoist-api = "0.3.0".
- Missing in root Cargo.toml — docs list tokio = "1.0" and sqlx = "0.8" but neither appears in the root Cargo.toml; confirm if those live in workspace members and update docs accordingly.
- Dev-deps mismatch — docs list anyhow = "1.0", serde = "1.0", chrono = "0.4" but these are not present in the root Cargo.toml; confirm their actual locations or update the PRD.
Locations: docs/PRD.md (lines 426-436); Cargo.toml (root, lines ~28-39).
🧹 Nitpick comments (16)
docs/CONFIG_FILE_FEATURE.md (5)
30-33: Precedence should include env override at highest priority.Document
TERMINALIST_CONFIG_PATHas the top‑priority source to match the Implementation Plan and typical UX.Apply this diff to update the list:
-### Default Locations (in order of precedence): -1. `./terminalist.toml` (project-specific config) -2. `~/.config/terminalist/config.toml` (user config) -3. Built-in defaults +### Default Locations (in order of precedence) +0. `TERMINALIST_CONFIG_PATH` (environment override) +1. `./terminalist.toml` (project-specific config) +2. `~/.config/terminalist/config.toml` (user config) +3. Built-in defaults
30-30: Fix markdownlint: remove trailing punctuation in heading.Drop the trailing colon to satisfy MD026.
-### Default Locations (in order of precedence): +### Default Locations (in order of precedence)
136-137: Heading level jumps flagged by markdownlint.Adjust these to increment by one level as suggested by MD001.
-### 4. Display Configuration +## 4. Display Configuration ... -### 5. Logging Configuration +## 5. Logging ConfigurationAlso applies to: 161-167
232-235: Clarify OS-specific paths for XDG.On Windows and macOS,
dirs::config_dir()resolves to platform-appropriate locations (e.g.,%APPDATA%on Windows). Note this explicitly to avoid confusion with~/.config/....
91-94: Logging section: add rotation and PII guidance.Briefly state max file size/rotation policy (even if “not yet”) and that API tokens or sensitive content must never be logged.
Also applies to: 164-167, 195-197
src/logger.rs (2)
11-13: Redundantenabledflag; rely onlog_file.is_some().
enabledduplicatesOption<PathBuf>. Simplify and reduce state drift risk.pub struct Logger { logs: Arc<Mutex<Vec<String>>>, - log_file: Option<PathBuf>, - enabled: bool, + log_file: Option<PathBuf>, } @@ - pub fn new() -> Self { + pub fn new() -> Self { Self { logs: Arc::new(Mutex::new(Vec::new())), - log_file: None, - enabled: false, + log_file: None, } }Also applies to: 32-39
49-58: Unbounded in‑memory growth.
logsgrows indefinitely. Consider a ring buffer (e.g., fixed cap like 2_000 entries).src/config.rs (1)
216-254: Add tests for partial configs and env override.Cover: missing sections, subset of fields within sections, and
TERMINALIST_CONFIG_PATH.I can add unit tests exercising partial TOML and env override behavior once serde defaults are in place.
src/sync.rs (1)
16-17: Consider makingloggernon-optional.It’s always set to
Some(...). Simplify the type and callers by storingLoggerdirectly.- logger: Option<Logger>, + logger: Logger,Follow-up: adjust
new,set_logger,logaccordingly.src/ui/components/dialog_component.rs (1)
62-63: Defaulting toDisplayConfig::default()is fine; ensure it’s overridden from Config on startup.Wire this via
AppComponentafter loadingConfigso runtime settings take effect.// in src/ui/app_component.rs, after constructing dialog & reading Config - let task_list = TaskListComponent::new(); - let dialog = DialogComponent::new(); + let mut task_list = TaskListComponent::new(); + task_list.update_display_config(config.display.clone()); + let mut dialog = DialogComponent::new(); + dialog.update_display_config(config.display.clone());README.md (1)
223-260: Add language to fenced code block to satisfy markdownlint (MD040).Annotate the project tree block as
text.-``` +```text src/ ├── main.rs # Main application entry point ...</blockquote></details> <details> <summary>src/ui/components/task_list_component.rs (1)</summary><blockquote> `50-51`: **Defaulting is fine; ensure runtime override from `Config`.** Same wiring note as DialogComponent: call `update_display_config` from `AppComponent` on startup/config change. </blockquote></details> <details> <summary>src/main.rs (1)</summary><blockquote> `45-58`: **Early return after `--generate-config`; remove stale TODO.** You already return; drop the comment to avoid confusion. ```diff - // TODO: make sure we return after this, the app should not run. return Ok(());Cargo.toml (1)
38-39: Replacedirswith a maintained "-next" crate (dirs-next or directories-next)Cargo.toml (lines 38–39): replace
dirs = "5.0"withdirs-next = "2.0"(low-level, drop-in) ordirectories-next = "2.0"(mid-level, project-aware); both are actively maintained as of 2025-09-16 (v2.0.0).src/ui/app_component.rs (1)
1046-1047: Consider adding bounds checking for sidebar width calculation.The sidebar width calculation
(rect.width * self.config.ui.sidebar_width / 100)could potentially overflow for large terminal widths or produce unexpected results ifsidebar_widthis misconfigured (e.g., > 100 or negative).Consider adding validation:
-let sidebar_width = (rect.width * self.config.ui.sidebar_width / 100).min(rect.width - 20); +let sidebar_width = (rect.width * self.config.ui.sidebar_width.clamp(10, 90) / 100).min(rect.width - 20);src/ui/components/task_list_item_component.rs (1)
148-155: Simplify redundant project color logic.The conditional for
show_project_colorscurrently has identical outcomes in both branches - both setColor::Cyan. This appears to be a placeholder for future color support.Simplify until actual color support is added:
-let project_style = if display_config.show_project_colors { - // Use project color if available, otherwise cyan - Style::default().fg(Color::Cyan) -} else { - Style::default().fg(Color::Cyan) -}; +// TODO: Use actual project colors when show_project_colors is true +let project_style = Style::default().fg(Color::Cyan);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (14)
Cargo.toml(1 hunks)README.md(7 hunks)docs/CONFIG_FILE_FEATURE.md(1 hunks)docs/PRD.md(2 hunks)src/config.rs(1 hunks)src/lib.rs(1 hunks)src/logger.rs(1 hunks)src/main.rs(5 hunks)src/sync.rs(2 hunks)src/ui/app_component.rs(6 hunks)src/ui/components/dialog_component.rs(4 hunks)src/ui/components/task_list_component.rs(4 hunks)src/ui/components/task_list_item_component.rs(8 hunks)src/ui/renderer.rs(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (8)
src/ui/components/task_list_component.rs (2)
src/config.rs (5)
default(62-69)default(73-79)default(83-87)default(91-100)default(104-106)src/ui/components/dialog_component.rs (2)
default(39-41)update_display_config(66-68)
src/sync.rs (3)
src/logger.rs (2)
new(33-39)new_with_file_logging(17-30)src/ui/app_component.rs (1)
new(82-107)src/ui/core/context.rs (1)
new(18-27)
src/ui/app_component.rs (4)
src/ui/components/dialog_component.rs (1)
new(45-64)src/ui/components/task_list_component.rs (1)
new(39-52)src/ui/core/context.rs (1)
new(18-27)src/ui/components/sidebar_component.rs (1)
new(29-39)
src/logger.rs (3)
src/sync.rs (3)
sync(434-454)new(29-53)log(61-65)src/ui/app_component.rs (1)
new(82-107)src/ui/core/context.rs (1)
new(18-27)
src/ui/components/dialog_component.rs (3)
src/config.rs (5)
default(62-69)default(73-79)default(83-87)default(91-100)default(104-106)src/ui/components/task_list_component.rs (3)
default(33-35)update_display_config(54-56)render(493-511)src/ui/components/task_list_item_component.rs (5)
render(15-15)render(33-39)render(97-231)render(256-262)render(286-295)
src/ui/components/task_list_item_component.rs (3)
src/ui/components/task_list_component.rs (2)
render(493-511)default(33-35)src/config.rs (5)
default(62-69)default(73-79)default(83-87)default(91-100)default(104-106)src/ui/components/badge.rs (1)
create_task_badges(30-51)
src/ui/renderer.rs (1)
src/ui/app_component.rs (1)
new(82-107)
src/main.rs (4)
src/config.rs (3)
get_default_config_path(211-213)generate_default_config(184-201)load(111-119)src/sync.rs (2)
sync(434-454)new(29-53)src/ui/app_component.rs (1)
new(82-107)src/ui/renderer.rs (1)
run_app(18-63)
🪛 markdownlint-cli2 (0.17.2)
docs/CONFIG_FILE_FEATURE.md
30-30: Trailing punctuation in heading
Punctuation: ':'
(MD026, no-trailing-punctuation)
136-136: Heading levels should only increment by one level at a time
Expected: h2; Actual: h3
(MD001, heading-increment)
161-161: Heading levels should only increment by one level at a time
Expected: h2; Actual: h3
(MD001, heading-increment)
README.md
225-225: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🪛 GitHub Actions: CI
src/config.rs
[error] 61-64: Clippy (derivable_impls): Default impl for Config can be derived. Replace manual implementation with #[derive(Default)].
[error] 103-107: Clippy (derivable_impls): Default impl for LoggingConfig can be derived. Replace manual implementation with #[derive(Default)].
🔇 Additional comments (25)
docs/PRD.md (1)
79-79: Nice PRD alignment with implemented config.The check‑marked statements look good and consistent with the docs and code.
Also applies to: 282-283
src/lib.rs (1)
1-1: Public config module exposure looks good.src/ui/components/dialog_component.rs (2)
35-36: Good addition: per-dialog display configuration.Storing
DisplayConfigin the dialog makes search results consistent with the task list.
443-444: Correct: passDisplayConfiginto item renderer.This keeps rendering consistent with main list.
README.md (4)
25-25: Nice: feature list now mentions configuration.
65-73: Helpful: documented config generation flow matches CLI.
81-116: Clear configuration precedence and example.Looks consistent with the new
Configmodule and defaults.
220-222: Dependency doc sync looks good.src/ui/components/task_list_component.rs (3)
29-31: Good:DisplayConfigstored on the component.
54-57: Setter is straightforward.
393-394: Correct: passDisplayConfigto rendering.src/main.rs (3)
34-38: Help text updated comprehensively.
60-62: Config loading before token checks is sensible.Keeps CLI options and UI behavior configurable even when token is missing.
79-85: Passconfigto UI: good; ensure UI components actually consume it.See notes to propagate
displayand logging configs insideAppComponent.src/ui/app_component.rs (3)
1-1: LGTM!The import of the
Configtype is appropriate for enabling configuration-driven behavior throughout the application component.
73-75: Good configuration integration!The addition of the
config: Configfield and its propagation through the constructor properly establishes the configuration system foundation. The signature change tonew(mut sync_service: SyncService, config: Config)correctly reflects the new requirement.Also applies to: 82-83, 103-103
185-185: LGTM! Clean configuration propagation to UI components.The
update_display_configcalls properly propagate the display configuration to bothTaskListComponentandDialogComponent, enabling consistent config-driven rendering.Also applies to: 195-195
src/ui/renderer.rs (4)
1-1: Good API evolution with Config parameter!The addition of
Configimport and the updatedrun_appsignature to acceptconfig: Configproperly establishes the configuration-driven architecture.Also applies to: 18-18, 33-33
22-28: Excellent conditional mouse capture handling!The implementation properly gates mouse capture based on
config.ui.mouse_enabledboth during initialization and cleanup, preventing unnecessary terminal state changes when mouse support is disabled.Also applies to: 53-59
36-37: Well-structured initial sync trigger!Triggering the initial sync immediately after component initialization ensures data is loaded early in the application lifecycle.
65-124: Clean separation with run_app_loop!The extraction of the main event loop into
run_app_loopimproves code organization and testability. The event handling logic is well-structured with proper render flag management.src/ui/components/task_list_item_component.rs (4)
1-1: LGTM! Clean DisplayConfig integration.The import and signature updates properly establish DisplayConfig-driven rendering throughout the list item hierarchy.
Also applies to: 15-15, 33-38
175-195: Well-implemented conditional metadata rendering!The metadata badges (durations, labels) are properly gated by their respective display configuration flags, allowing users to customize the information density.
198-228: Good description excerpt implementation!The conditional description display with width calculation and truncation is well-implemented, preventing overflow while maintaining readability.
102-114: Nice hierarchical indentation for subtasks!The tree-style indentation with
└─connectors provides clear visual hierarchy for nested tasks.
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/sync.rs (1)
68-75: Don't hold the storage MutexGuard across async DB calls — it serializes DB I/OLocalStorage methods (projects/sections/labels/tasks/db) use sqlx and .await; sync.rs locks Arc<Mutex> then calls those async methods, which keeps the MutexGuard held across DB awaits and will serialize unrelated callers.
Actionable fixes:
- Preferred: remove the outer Mutex — make SyncService.storage an Arc and ensure LocalStorage only contains cloneable, Send+Sync fields (use SqlitePool; move/remove the non-clone _anchor SqliteConnection or wrap it internally).
- Or: move the Mutex into LocalStorage and protect only small mutable state so DB calls run without holding the external lock.
- Short-term alternative: use tokio::sync::RwLock for concurrent reads (temporary mitigation).
Relevant locations: src/sync.rs:72-75; storage impls: src/storage/projects.rs (get_projects), src/storage/sections.rs (get_sections), src/storage/labels.rs (get_all_labels), src/storage/tasks.rs (various), src/storage/db.rs (has_data/clear_all_data).
♻️ Duplicate comments (1)
src/sync.rs (1)
29-29: Public API change: ensure versioning and call‑site updates.Constructor now requires
config: &Config. Confirm all call sites/docs are updated and bump crate minor version if this is public.
🧹 Nitpick comments (7)
src/sync.rs (1)
142-158: UTC date for “today/tomorrow” may mismatch user’s local day.Using
chrono::Utc::now()can show/route tasks to the wrong bucket for users not in UTC. Prefer local time or a config toggle.Apply this diff to switch to local date strings:
- let today = chrono::Utc::now().format("%Y-%m-%d").to_string(); + let today = chrono::Local::now().format("%Y-%m-%d").to_string(); @@ - let tomorrow = (chrono::Utc::now() + chrono::Duration::days(1)).format("%Y-%m-%d").to_string(); + let tomorrow = (chrono::Local::now() + chrono::Duration::days(1)).format("%Y-%m-%d").to_string(); @@ - let today = chrono::Utc::now().format("%Y-%m-%d").to_string(); - let three_months_later = (chrono::Utc::now() + chrono::Duration::days(90)).format("%Y-%m-%d").to_string(); + let today = chrono::Local::now().format("%Y-%m-%d").to_string(); + let three_months_later = (chrono::Local::now() + chrono::Duration::days(90)).format("%Y-%m-%d").to_string();Also applies to: 165-166
RATATUI_ARCHITECTURE_GUIDELINES.md (1)
1227-1229: Doc snippet won’t compile:?onAppComponent::new.
AppComponent::new(sync_service, config)returnsSelf, notResult. Remove?.Apply this diff:
- let mut app = AppComponent::new(sync_service, config)?; + let mut app = AppComponent::new(sync_service, config);src/logger.rs (5)
55-62: Allow overriding log path for tests/envs.Let tests redirect logs to a temp dir and avoid touching real user config.
Apply this diff:
pub fn get_log_file_path() -> io::Result<PathBuf> { - let config_dir = - dirs::config_dir().ok_or_else(|| io::Error::new(io::ErrorKind::NotFound, "Config directory not found"))?; - - Ok(config_dir.join("terminalist").join("terminalist.log")) + if let Ok(dir) = std::env::var("TERMINALIST_LOG_DIR") { + return Ok(PathBuf::from(dir).join("terminalist.log")); + } + let config_dir = dirs::config_dir() + .ok_or_else(|| io::Error::new(io::ErrorKind::NotFound, "Config directory not found"))?; + Ok(config_dir.join("terminalist").join("terminalist.log")) }Add import:
use std::env; // at top of file
74-82: Minor:enabledduplicatesfile_writer.is_some().You can drop
enabledand useif let Some(writer) = ...as the single source of truth.
130-160: Tests write to real config dir; isolate via temp path.Redirect logs to a temp directory and clean up afterwards.
Apply this diff:
#[test] fn test_config_based_logging_enabled() { - // Test with logging enabled + // Test with logging enabled (redirect to temp dir) + let tmpdir = std::env::temp_dir().join("terminalist_test_logs"); + let _ = fs::create_dir_all(&tmpdir); + std::env::set_var("TERMINALIST_LOG_DIR", &tmpdir); let logger = Logger::from_config(true).unwrap(); @@ - if log_path.exists() { + if log_path.exists() { let file_content = fs::read_to_string(&log_path).unwrap_or_default(); assert!(file_content.contains("Test message with file")); // Clean up test file - let _ = fs::remove_file(&log_path); + let _ = fs::remove_file(&log_path); + let _ = fs::remove_dir_all(&tmpdir); + std::env::remove_var("TERMINALIST_LOG_DIR"); }
65-67: Optional: include date in timestamps.Only time is logged; date helps across long sessions.
Apply this diff:
- let timestamp = Utc::now().format("%H:%M:%S%.3f").to_string(); + let timestamp = Utc::now().format("%Y-%m-%d %H:%M:%S%.3f").to_string();
63-83: API ergonomics nit: acceptimpl AsRef<str>orimpl Into<String>.
log(&self, message: String)forces allocations at call sites.Suggestion:
pub fn log(&self, message: impl AsRef<str>) { let msg = message.as_ref(); // format and write using msg }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
RATATUI_ARCHITECTURE_GUIDELINES.md(2 hunks)src/config.rs(1 hunks)src/logger.rs(2 hunks)src/sync.rs(3 hunks)src/ui/app_component.rs(6 hunks)tests/ui_renderer_test.rs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/config.rs
- src/ui/app_component.rs
🧰 Additional context used
🧬 Code graph analysis (2)
src/logger.rs (4)
src/sync.rs (4)
sync(435-455)new(29-49)log(62-66)logger(52-54)src/ui/app_component.rs (1)
new(82-106)src/ui/components/dialog_component.rs (1)
new(45-64)src/ui/core/context.rs (1)
new(18-27)
src/sync.rs (3)
src/logger.rs (3)
new(47-53)from_config(17-23)default(106-108)src/ui/app_component.rs (1)
new(82-106)src/ui/core/context.rs (1)
new(18-27)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Test (windows-latest, beta)
- GitHub Check: Test (windows-latest, stable)
- GitHub Check: Security Audit
🔇 Additional comments (3)
src/sync.rs (2)
34-41: Config‑driven logger init with graceful fallback — LGTM.Good: uses
Logger::from_config, warns on failure, falls back to in‑memory.
51-55: Logger accessor — LGTM.Safe clone via
Option<Logger>matches the current design.src/logger.rs (1)
25-44: Buffered, single‑open file logging — LGTM.Opens once, wraps in
BufWriter, and shares viaArc<Mutex<_>>. Good for perf and correctness.
| // This would require a mock sync service and config | ||
| // let config = Config::default(); | ||
| // let sync_service = SyncService::new("dummy_token".to_string(), false, &config).await.unwrap(); | ||
| // let app = AppComponent::new(sync_service, config); |
There was a problem hiding this comment.
Commented‑out test code: make it executable or remove.
This test currently does nothing; it won’t catch regressions in the new Config/SyncService/AppComponent wiring. Either implement a mock SyncService and construct AppComponent with a real Config, or mark the test ignored until a proper mock exists.
Apply this diff to clearly mark it pending and avoid misleading green tests:
- // This would require a mock sync service and config
- // let config = Config::default();
- // let sync_service = SyncService::new("dummy_token".to_string(), false, &config).await.unwrap();
- // let app = AppComponent::new(sync_service, config);
+ // TODO: add a proper mock SyncService and real Config, then remove this ignore.
+ // let config = Config::default();
+ // let sync_service = MockSyncService::new(&config);
+ // let app = AppComponent::new(sync_service, config);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // This would require a mock sync service and config | |
| // let config = Config::default(); | |
| // let sync_service = SyncService::new("dummy_token".to_string(), false, &config).await.unwrap(); | |
| // let app = AppComponent::new(sync_service, config); | |
| // TODO: add a proper mock SyncService and real Config, then remove this ignore. | |
| // let config = Config::default(); | |
| // let sync_service = MockSyncService::new(&config); | |
| // let app = AppComponent::new(sync_service, config); |
🤖 Prompt for AI Agents
In tests/ui_renderer_test.rs around lines 5–8, the test contains commented-out
setup for Config, SyncService, and AppComponent so it does nothing; either make
it an actual test or mark it explicit pending. To fix: either (A) implement a
simple MockSyncService that satisfies the same trait/API used by AppComponent,
un-comment the setup, construct a real Config::default(), instantiate
AppComponent with the mock sync service, and add at least one assertion
exercising wiring; or (B) if a mock is nontrivial, leave the commented code
removed and mark the test function with #[test] #[ignore] plus a short TODO
comment referencing the required mock, so CI doesn’t report a false green test.
Ensure the chosen change is committed and the test file compiles.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
src/logger.rs (4)
10-13: Use VecDeque for O(1) ring buffer; avoid O(n) drains and extra copies.Current drain(0..drop_n) shifts the whole Vec. VecDeque keeps constant memory and fast pop_front().
Apply this diff:
@@ -use std::fs::{File, OpenOptions}; +use std::fs::{File, OpenOptions}; use std::io::{self, BufWriter, Write}; use std::path::PathBuf; +use std::collections::VecDeque; @@ - logs: Arc<Mutex<Vec<String>>>, + logs: Arc<Mutex<VecDeque<String>>>, @@ - Ok(Self { - logs: Arc::new(Mutex::new(Vec::new())), + Ok(Self { + logs: Arc::new(Mutex::new(VecDeque::new())), file_writer, enabled: true, }) @@ pub fn new() -> Self { Self { - logs: Arc::new(Mutex::new(Vec::new())), + logs: Arc::new(Mutex::new(VecDeque::new())), file_writer: None, enabled: false, } } @@ - if let Ok(mut logs) = self.logs.lock() { - const MAX_LOGS: usize = 5000; - logs.push(formatted_message.clone()); - if logs.len() > MAX_LOGS { - let drop_n = logs.len() - MAX_LOGS; - logs.drain(0..drop_n); - } - } + if let Ok(mut logs) = self.logs.lock() { + const MAX_LOGS: usize = 5000; + if logs.len() == MAX_LOGS { + let _ = logs.pop_front(); + } + logs.push_back(formatted_message.clone()); + } @@ - if let Ok(logs) = self.logs.lock() { - let mut sorted_logs = logs.clone(); - // Reverse to show newest logs first (descending order by timestamp) - sorted_logs.reverse(); - sorted_logs + if let Ok(logs) = self.logs.lock() { + // Collect to Vec and reverse to show newest first + let mut v: Vec<String> = logs.iter().cloned().collect(); + v.reverse(); + v } else { Vec::new() }Also applies to: 39-44, 49-53, 68-76, 90-100
78-87: Gate file writes on file_writer presence; ‘enabled’ is redundant.Single source of truth prevents drift; you can keep
enabledfor future toggles but don’t rely on it here.- // Write to file if file logging is enabled - if self.enabled { - if let Some(ref file_writer) = self.file_writer { - if let Ok(mut writer) = file_writer.lock() { - // Write to buffered writer, ignore errors to avoid logging recursion - let _ = writeln!(writer, "{}", formatted_message); - // Let BufWriter handle flushing automatically, don't flush on every entry - } - } - } + // Write to file if a writer is configured + if let Some(ref file_writer) = self.file_writer { + if let Ok(mut writer) = file_writer.lock() { + let _ = writeln!(writer, "{}", formatted_message); + } + }Optional: add
debug_assert_eq!(self.enabled, self.file_writer.is_some());in constructors to keep them in sync.
65-65: Include date in timestamp to disambiguate multi‑day sessions.- let timestamp = Utc::now().format("%H:%M:%S%.3f").to_string(); + let timestamp = Utc::now().format("%Y-%m-%d %H:%M:%S%.3f").to_string();
90-95: Docstring nit: it’s reverse insertion order, not “sorted by date.”- /// Get all logs sorted by date (newest first) + /// Get all logs in reverse insertion order (newest first)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/logger.rs(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/logger.rs (3)
src/sync.rs (4)
sync(435-455)new(29-49)log(62-66)logger(52-54)src/ui/app_component.rs (1)
new(82-106)src/ui/core/context.rs (1)
new(18-27)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Test (windows-latest, beta)
- GitHub Check: Test (windows-latest, stable)
- GitHub Check: Security Audit
🔇 Additional comments (3)
src/logger.rs (3)
55-62: LGTM: XDG‑aware default path.Good move to dirs::config_dir(); matches prior feedback.
25-44: LGTM: single open + buffered writer under a mutex.This fixes per‑call reopen/flush and interleaving writes.
116-165: Tests cover both modes; nice.Assertions validate in‑memory and file paths; flushing via the mutex is correct.
| /// Create a new logger with file logging enabled | ||
| pub fn new_with_file_logging() -> io::Result<Self> { | ||
| let log_file_path = Self::get_log_file_path()?; | ||
|
|
||
| // Ensure the config directory exists | ||
| if let Some(parent) = log_file_path.parent() { | ||
| std::fs::create_dir_all(parent)?; | ||
| } | ||
|
|
||
| // Open the file once and create a buffered writer | ||
| let file = OpenOptions::new().create(true).append(true).open(&log_file_path)?; | ||
| let buf_writer = BufWriter::new(file); | ||
| let file_writer = Some(Arc::new(Mutex::new(buf_writer))); | ||
|
|
||
| Ok(Self { | ||
| logs: Arc::new(Mutex::new(Vec::new())), | ||
| file_writer, | ||
| enabled: true, | ||
| }) | ||
| } |
There was a problem hiding this comment.
Plan for log file growth (rotation/retention).
With file logging enabled, the on‑disk log grows unbounded. Add simple size‑based rotation (e.g., 5 MB x 3 files) or gate retention limits via config.
If you want a quick in‑house solution, I can sketch a size‑check in log() that rotates the file under the same mutex; otherwise consider tracing + tracing_appender::rolling for a robust setup.
Also applies to: 78-87
🤖 Prompt for AI Agents
In src/logger.rs around lines 25-44 (also apply same change to 78-87), the file
logger currently appends unbounded logs to a single file; implement simple
size-based rotation and retention: before writing in log() (under the existing
file_writer mutex) check the current file size, and if it exceeds the configured
threshold (e.g., 5 MB) perform a rotation by closing the current writer,
renaming existing files (log -> log.1, log.1 -> log.2) up to N=3, deleting the
oldest, then reopen a new log file and replace file_writer with the new
BufWriter wrapped in Arc<Mutex<...>>; ensure all file ops propagate IO errors
and keep the directory creation code, or alternatively wire in
tracing_appender::rolling and swap the file_writer creation to use that rolling
appender if you prefer the external solution.
| fn test_config_based_logging_enabled() { | ||
| // Test with logging enabled | ||
| let logger = Logger::from_config(true).unwrap(); | ||
| assert!(logger.enabled); | ||
| assert!(logger.file_writer.is_some()); | ||
|
|
||
| logger.log("Test message with file".to_string()); | ||
|
|
||
| // Check in-memory logs (for UI display with "G" key) | ||
| let logs = logger.get_logs(); | ||
| assert_eq!(logs.len(), 1); | ||
| assert!(logs[0].contains("Test message with file")); | ||
|
|
||
| // Test that the file writer exists and works by forcing a flush and checking the file | ||
| if let Some(ref writer_arc) = logger.file_writer { | ||
| // Force flush the buffered writer | ||
| if let Ok(mut writer) = writer_arc.lock() { | ||
| let _ = writer.flush(); | ||
| } | ||
|
|
||
| // Check if log file was created at the expected path | ||
| let log_path = Logger::get_log_file_path().unwrap(); | ||
| if log_path.exists() { | ||
| let file_content = fs::read_to_string(&log_path).unwrap_or_default(); | ||
| assert!(file_content.contains("Test message with file")); | ||
| // Clean up test file | ||
| let _ = fs::remove_file(&log_path); | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
🧩 Analysis chain
Tests write to the real XDG config dir; prefer temp file path injection.
This avoids polluting user config and makes tests hermetic.
- Add a constructor for tests:
// Add inside impl Logger (production-safe too)
pub fn new_with_path<P: AsRef<std::path::Path>>(path: P) -> io::Result<Self> {
if let Some(parent) = path.as_ref().parent() {
std::fs::create_dir_all(parent)?;
}
let file = OpenOptions::new().create(true).append(true).open(path)?;
Ok(Self {
logs: Arc::new(Mutex::new(std::collections::VecDeque::new())),
file_writer: Some(Arc::new(Mutex::new(BufWriter::new(file)))),
enabled: true,
})
}- Update the test to use a temp dir:
use tempfile::tempdir;
#[test]
fn test_config_based_logging_enabled() {
let dir = tempdir().unwrap();
let log_path = dir.path().join("terminalist.log");
let logger = Logger::new_with_path(&log_path).unwrap();
logger.log("Test message with file".to_string());
if let Some(ref writer_arc) = logger.file_writer {
writer_arc.lock().ok().map(|mut w| w.flush().ok());
}
let file_content = std::fs::read_to_string(&log_path).unwrap_or_default();
assert!(file_content.contains("Test message with file"));
}Add dev-dep: tempfile = "3".
Run to verify no other tests rely on the real path:
🏁 Script executed:
#!/bin/bash
rg -n -C2 'get_log_file_path\(\)|from_config\(\s*true' --type=rustLength of output: 1283
Don't write tests to the user's XDG config dir — inject a temp log path for tests
Logger::from_config(true) and Logger::get_log_file_path() use dirs::config_dir(), so the test at src/logger.rs:135-165 writes to the real XDG config dir. Add a constructor that accepts a path and update the test to use tempfile::tempdir() to make tests hermetic.
Locations: src/logger.rs (new_with_file_logging -> uses get_log_file_path at ~25-29; get_log_file_path at ~56-58; test at ~135-165).
Suggested changes:
// inside impl Logger
pub fn new_with_path<P: AsRef<std::path::Path>>(path: P) -> std::io::Result<Self> {
if let Some(parent) = path.as_ref().parent() {
std::fs::create_dir_all(parent)?;
}
let file = std::fs::OpenOptions::new().create(true).append(true).open(path)?;
Ok(Self {
logs: std::sync::Arc::new(std::sync::Mutex::new(std::collections::VecDeque::new())),
file_writer: Some(std::sync::Arc::new(std::sync::Mutex::new(std::io::BufWriter::new(file)))),
enabled: true,
})
}Test update:
use tempfile::tempdir;
#[test]
fn test_config_based_logging_enabled() {
let dir = tempdir().unwrap();
let log_path = dir.path().join("terminalist.log");
let logger = Logger::new_with_path(&log_path).unwrap();
logger.log("Test message with file".to_string());
if let Some(ref writer_arc) = logger.file_writer {
writer_arc.lock().ok().map(|mut w| w.flush().ok());
}
let file_content = std::fs::read_to_string(&log_path).unwrap_or_default();
assert!(file_content.contains("Test message with file"));
}Add dev-dep: tempfile = "3"
Optional quick check (already run): rg -n -C2 'get_log_file_path()|from_config(\s*true' --type=rust
🤖 Prompt for AI Agents
In src/logger.rs around lines 25-29, 56-58 and the test at 135-165, the unit
test currently writes to the user's XDG config dir because
Logger::from_config(true) and Logger::get_log_file_path() call
dirs::config_dir(); add a new constructor Logger::new_with_path<P:
AsRef<std::path::Path>> that ensures parent dirs exist, opens the given path
with OpenOptions (create + append) wrapped in a BufWriter inside the existing
Arc<Mutex<...>> file_writer and sets enabled=true, then update the test at
135-165 to create a tempfile::tempdir(), build a log_path inside it, instantiate
the logger via Logger::new_with_path(&log_path).unwrap(), call logger.log(...),
flush the writer (lock and flush) and assert the temp file contains the message;
finally add tempfile = "3" as a dev-dependency in Cargo.toml for tests.
1 participant
Summary by CodeRabbit
New Features
Documentation
Chores