Refactor/global architecture - #22
Conversation
WalkthroughVersion bump plus a major UI rewrite: legacy TUI replaced by a component-based UI (AppComponent, core actions/traits, TaskManager, dialog/sidebar/task-list components, new_renderer). Storage and sync gained single-record write/update APIs and local persistence; main now requires TODOIST_API_TOKEN and starts the new renderer. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
✨ 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 |
f794581 to
526bcdb
Compare
526bcdb to
b8b3f3e
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: 12
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (16)
src/ui/components/badge.rs (2)
99-105: Fix unreadable “silver” labels and spacing regression.
- White fg on white/super-light bg makes label text invisible (e.g., "silver" -> White).
- Removing padding around label text breaks visual separation with adjacent badges.
Apply contrast-aware fg and restore padding:
- let style = Style::default() - .bg(bg_color) - .fg(Color::White) - .add_modifier(Modifier::BOLD); - - Span::styled(name.to_string(), style) + // Choose contrasting fg, keep padded text + let mut style = Style::default().bg(bg_color).add_modifier(Modifier::BOLD); + style = match bg_color { + // Light backgrounds → black text + Color::White + | Color::Yellow + | Color::LightYellow + | Color::LightCyan + | Color::Cyan => style.fg(Color::Black), + // Heuristic for custom RGBs (orange, pink, brown, etc.) + Color::Rgb(r, g, b) + if (r as u32 * 299 + g as u32 * 587 + b as u32 * 114) >= 128_000 => + style.fg(Color::Black), + _ => style.fg(Color::White), + }; + Span::styled(format!(" {name} "), style)Note: This also aligns spacing with other badges (create_badge adds padding), avoiding run-together spans like “(30m)label”.
90-95: Map “silver” to gray, not white, to preserve contrast.Current mapping yields white bg; with prior fg=White it was invisible. Even with contrast fix, pure white is harsh; use a light gray.
- "silver" => Color::White, // Changed from LightGray which doesn't exist + // Silver → light gray for better readability across terminals + "silver" => Color::Rgb(192, 192, 192),README.md (2)
41-67: Implement or remove ‘i’ icon-theme shortcutREADME lists
ifor cycling icon themes, but noKeyCode::Char('i')handler exists insrc/ui—add the handler or remove the shortcut from the docs.
120-132: Synchronize README dependency versions with Cargo.toml
- ratatui: bump from 0.24 → 0.29
- crossterm: bump from 0.27 → 0.29
- sqlx: bump from 0.7 → 0.8
- serde: add version 1.0
- todoist-api: replace
0.2.0with the Git dependency spec in Cargo.tomlsrc/storage.rs (10)
551-577: Fix label resolution: query by label IDs and update both name and colorCurrently you pass label names to
get_labels_by_ids(which filters onid) and only patch colors. This leaves colors at default and names incorrect when tasks store label IDs (Todoist’s default). Resolve by collecting IDs fromtask_display.labels, fetching labels by ID, and updating both name and color.pub async fn update_task_labels(&self, task_display: &mut TaskDisplay) -> Result<()> { if task_display.labels.is_empty() { return Ok(()); } - // Extract label names from the task - let label_names: Vec<String> = task_display.labels.iter().map(|l| l.name.clone()).collect(); - - // Get the actual label objects from storage - let stored_labels = self.get_labels_by_ids(&label_names).await?; - - // Create a map of label names to colors - let mut label_color_map = std::collections::HashMap::new(); - for label in stored_labels { - label_color_map.insert(label.name, label.color); - } - - // Update the task labels with proper colors - for label_display in &mut task_display.labels { - if let Some(color) = label_color_map.get(&label_display.name) { - label_display.color = color.clone(); - } - } + // Collect label IDs from the task + let label_ids: Vec<String> = task_display.labels.iter().map(|l| l.id.clone()).collect(); + let stored_labels = self.get_labels_by_ids(&label_ids).await?; + + // Map ID -> (name, color) + let mut labels_by_id = std::collections::HashMap::new(); + for label in stored_labels { + labels_by_id.insert(label.id, (label.name, label.color)); + } + + // Update both name and color using ID match + for label_display in &mut task_display.labels { + if let Some((name, color)) = labels_by_id.get(&label_display.id) { + label_display.name = name.clone(); + label_display.color = color.clone(); + } + } Ok(()) }
676-685: Exclude deleted tasks in project queryDeleted tasks appear in project views. Add a filter to hide them.
- WHERE project_id = ? + WHERE project_id = ? + AND is_deleted = false
704-719: NULL-safe description to prevent runtime decode errors
descriptionis nullable in the schema. Reading it asStringcan panic. Read asOption<String>and default.- description: row.get("description"), + description: row + .get::<Option<String>, _>("description") + .unwrap_or_default(),
733-741: Exclude deleted tasks in “all tasks”Align with other views and hide deleted tasks.
- FROM tasks - ORDER BY is_completed ASC, priority DESC, order_index ASC + FROM tasks + WHERE is_deleted = false + ORDER BY is_completed ASC, priority DESC, order_index ASC
760-775: NULL-safe description in “all tasks”Same decoding issue as above.
- description: row.get("description"), + description: row + .get::<Option<String>, _>("description") + .unwrap_or_default(),
833-848: NULL-safe description in “today/overdue”Prevent decode errors on null descriptions.
- description: row.get("description"), + description: row + .get::<Option<String>, _>("description") + .unwrap_or_default(),
901-916: NULL-safe description in “tomorrow”Prevent decode errors on null descriptions.
- description: row.get("description"), + description: row + .get::<Option<String>, _>("description") + .unwrap_or_default(),
579-596: Delete sections when deleting a projectOrphans remain in
sections. Delete them (or add FK + cascade).pub async fn delete_project(&self, project_id: &str) -> Result<()> { let mut tx = self.pool.begin().await?; // Delete tasks first, then the project sqlx::query("DELETE FROM tasks WHERE project_id = ?") .bind(project_id) .execute(&mut *tx) .await?; + // Delete sections for this project + sqlx::query("DELETE FROM sections WHERE project_id = ?") + .bind(project_id) + .execute(&mut *tx) + .await?; + sqlx::query("DELETE FROM projects WHERE id = ?") .bind(project_id) .execute(&mut *tx) .await?;
1014-1028: Clear all data should include sections and labelsThe current reset skips
sectionsandlabels.pub async fn clear_all_data(&self) -> Result<()> { let mut tx = self.pool.begin().await?; sqlx::query("DELETE FROM tasks").execute(&mut *tx).await?; + sqlx::query("DELETE FROM sections").execute(&mut *tx).await?; sqlx::query("DELETE FROM projects") .execute(&mut *tx) .await?; + sqlx::query("DELETE FROM labels") + .execute(&mut *tx) + .await?; sqlx::query("DELETE FROM sync_metadata") .execute(&mut *tx) .await?;
1030-1046: Add migration foris_inbox_projectOlder DBs may lack this column. Add it for forward compatibility.
async fn run_migrations(&self) -> Result<()> { // Check if parent_id column exists in projects table let has_parent_id = sqlx::query_scalar::<_, Option<String>>( "SELECT name FROM pragma_table_info('projects') WHERE name = 'parent_id'", ) .fetch_optional(&self.pool) .await? .is_some(); if !has_parent_id { sqlx::query("ALTER TABLE projects ADD COLUMN parent_id TEXT") .execute(&self.pool) .await?; } + + // Add is_inbox_project if missing + let has_inbox = sqlx::query_scalar::<_, Option<String>>( + "SELECT name FROM pragma_table_info('projects') WHERE name = 'is_inbox_project'", + ) + .fetch_optional(&self.pool) + .await? + .is_some(); + if !has_inbox { + sqlx::query("ALTER TABLE projects ADD COLUMN is_inbox_project BOOLEAN NOT NULL DEFAULT 0") + .execute(&self.pool) + .await?; + } Ok(()) }docs/PRD.md (2)
685-691: Docs mismatch with code: priority naming (P0–P3 vs [P1]–[P4]).Help dialog lists [P1]..[P4] with P1 highest; PRD states P0–P3. Align terminology.
I can send a patch to standardize on one scheme across code and docs.
221-227: Module names reflect legacy architecture.Sections still reference
app.rs,renderer.rs, etc., while the code now usesapp_component.rs,new_renderer.rs, andui/core/*. Update to the new structure to reduce confusion.I can draft updated sections mirroring the current module tree.
Also applies to: 425-451
🧹 Nitpick comments (41)
src/ui/core/event_handler.rs (1)
1-2: Import KeyEventKind; drop dead_code if intervals are now used.-use crossterm::event::{poll, Event, KeyEvent}; +use crossterm::event::{poll, Event, KeyEvent, KeyEventKind};If you adopt the change above, these allows can be removed:
- #[allow(dead_code)] tick_interval: tokio::time::Interval, - #[allow(dead_code)] render_interval: tokio::time::Interval,src/ui/mod.rs (3)
13-13: Provide a stable alias for the entrypoint; avoid thenew_prefixKeep
run_new_appexported, but also alias it torun_appso downstream callers can migrate without churn.-pub use new_renderer::run_new_app; +pub use new_renderer::run_new_app; +// Prefer `run_app` going forward; keep `run_new_app` for transition. +pub use new_renderer::run_new_app as run_app;
11-11: Offer a deprecated compatibility alias forAppIf you previously exported
App, consider a deprecated alias to ease migration.pub use app_component::AppComponent; +#[deprecated(note = "Renamed to AppComponent")] +pub use app_component::AppComponent as App;
5-9: Tighten the public surface of submodulesIf external consumers don’t need to reach into these modules directly, make the modules private and re-export only the intended types from here to reduce API surface area.
README.md (1)
147-156: Add missingcontext.rsto the project structure tree
src/ui/core/context.rsexists but isn’t listed under Core in the README’s tree.├── core/ # Core architecture components │ ├── actions.rs # Action system for component communication │ ├── component.rs # Component trait and lifecycle + │ ├── context.rs # Shared app context │ ├── event_handler.rs # Event processing system │ └── task_manager.rs # Background async task managementsrc/ui/core/mod.rs (2)
1-5: Limit module visibility; re-export symbols insteadUse private modules to avoid leaking internals; keep the public API via re-exports already provided.
-pub mod actions; -pub mod component; -pub mod context; -pub mod event_handler; -pub mod task_manager; +mod actions; +mod component; +mod context; +mod event_handler; +mod task_manager;
7-12: Add a brief module doc and a prelude for ergonomicsInline docs help discovery, and a
preludeeases imports for downstream code.+//! Core UI primitives: actions, components, events, context, and background tasks. // Re-exports for easier access pub use actions::{Action, DialogType, SidebarSelection}; pub use component::Component; pub use context::AppContext; pub use event_handler::{EventHandler, EventType}; pub use task_manager::{TaskId, TaskManager, TaskResult}; + +/// Commonly used core items. +pub mod prelude { + pub use super::actions::{Action, DialogType, SidebarSelection}; + pub use super::component::Component; + pub use super::context::AppContext; + pub use super::event_handler::{EventHandler, EventType}; + pub use super::task_manager::{TaskId, TaskManager, TaskResult}; +}src/main.rs (3)
38-47: Exit with a non-zero code when the API token is missingReturning Ok makes CI/shells treat this as success; exit with a failure code instead.
- eprintln!("\n💡 Use --help for more options"); - return Ok(()); + eprintln!("\n💡 Use --help for more options"); + std::process::exit(2);
52-62: Flatten timeout/result handling for clarity and add contextMore idiomatic error propagation and a clearer timeout error.
- match tokio::time::timeout(tokio::time::Duration::from_secs(10), sync::SyncService::new(api_token)).await { - Ok(Ok(sync_service)) => { - ui::run_new_app(sync_service).await?; - } - Ok(Err(e)) => { - return Err(e); - } - Err(_) => { - return Err(anyhow::anyhow!("Sync service creation timed out")); - } - } + let sync_service = tokio::time::timeout( + tokio::time::Duration::from_secs(10), + sync::SyncService::new(api_token), + ) + .await + .map_err(|_| anyhow::anyhow!("Sync service creation timed out"))??; + + ui::run_new_app(sync_service).await?;
14-17: Consider a CLI parser (clap) for options/helpHand-rolled parsing is fine now, but
clapwill scale better as flags grow.src/ui/core/context.rs (2)
8-15: Restrict field visibility to maintain invariantsExpose getters/setters as needed; this prevents accidental external mutation of core state.
-pub struct AppContext { - pub sync_service: SyncService, - pub projects: Vec<ProjectDisplay>, - pub labels: Vec<LabelDisplay>, - pub sections: Vec<SectionDisplay>, - pub icons: IconService, - pub debug_logger: DebugLogger, -} +#[derive(Debug)] +pub struct AppContext { + pub(crate) sync_service: SyncService, + pub(crate) projects: Vec<ProjectDisplay>, + pub(crate) labels: Vec<LabelDisplay>, + pub(crate) sections: Vec<SectionDisplay>, + pub(crate) icons: IconService, + pub(crate) debug_logger: DebugLogger, +}
17-27: Add minimal docs to clarify responsibilitiesDoc comments make it easier for contributors to use/extend the context.
impl AppContext { - pub fn new(sync_service: SyncService) -> Self { + /// Create an application context holding shared services and cached display data. + pub fn new(sync_service: SyncService) -> Self { Self { sync_service, projects: Vec::new(), labels: Vec::new(), sections: Vec::new(), icons: IconService::default(), debug_logger: DebugLogger::new(), } } }src/sync.rs (2)
274-303: Due-date update: good logging + local write; prefer stronger typing for datesConsider taking a typed date (e.g., chrono::NaiveDate) or a domain Date newtype and only stringify at API boundaries. This avoids invalid strings leaking into storage and simplifies validation.
317-350: Harden toggle_task for races, optimize lookup, and handle missing tasks
- Race: local state may change after releasing the lock and before the API call; make the operation idempotent (e.g. attempt complete and fallback to reopen based on API response) or re-fetch the task state after the call to confirm.
- Lookup:
storage.get_all_tasks()is an O(n) scan; add aget_task_by_id(&self, task_id: &str)helper (or index) to avoid full-list iteration.- Not-found branch: currently assumes “complete” when the task isn’t found; distinguish between 404/deleted vs. genuinely incomplete for clearer UX.
src/ui/new_renderer.rs (5)
4-8: Terminal setup/restore: pair mouse capture enable/disable; consider panic-safe cleanupEnable mouse capture on entry (you already disable it on exit). Optionally add a guard to restore terminal state on panic.
- use crossterm::{ - event::DisableMouseCapture, + use crossterm::{ + event::{EnableMouseCapture, DisableMouseCapture}, execute, terminal::{disable_raw_mode, enable_raw_mode, EnterAlternateScreen, LeaveAlternateScreen}, }; @@ - execute!(stdout, EnterAlternateScreen)?; + execute!(stdout, EnterAlternateScreen, EnableMouseCapture)?; @@ - disable_raw_mode()?; - execute!(terminal.backend_mut(), LeaveAlternateScreen, DisableMouseCapture)?; + disable_raw_mode()?; + execute!(terminal.backend_mut(), LeaveAlternateScreen, DisableMouseCapture)?;If desired, wrap setup in a small RAII guard (e.g., scopeguard) to guarantee restore on panic.
Also applies to: 21-21, 44-48
32-42: Remove or wire unused cleanup/render intervalsThey’re created and passed but unused. Either drive logic with them or drop for simplicity.
- use tokio::time::{interval, Duration}; + // use tokio::time::Duration; @@ - // Create intervals for periodic tasks - let mut cleanup_interval = interval(Duration::from_secs(5)); // Clean up finished tasks every 5 seconds - let mut render_interval = interval(Duration::from_millis(16)); // ~60 FPS rendering - let result = run_app_loop( - &mut terminal, - &mut app, - &mut event_handler, - &mut cleanup_interval, - &mut render_interval, - ) + let result = run_app_loop( + &mut terminal, + &mut app, + &mut event_handler, + ) .await; @@ -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, -) -> anyhow::Result<()> { +async fn run_app_loop<B: Backend>( + terminal: &mut Terminal<B>, + app: &mut AppComponent, + event_handler: &mut EventHandler, +) -> anyhow::Result<()> {Also applies to: 52-58, 14-15
113-121: AppStatus shape: align time type with SyncService/chrono or reuse SyncStatsConsider chrono::DateTime for last_sync (or expose SyncStats) to avoid mixing SystemTime with Chrono elsewhere.
136-140: request_render is a no-opAdd a dirty flag in AppComponent that the loop checks, or post an internal Action::RequestRender to wake the loop.
146-166: Tests: good event timing check; add a quit-path smoke testA minimal test where EventHandler yields a Quit action would exercise run_app_loop’s shutdown path. I can draft a mock EventHandler if helpful.
src/ui/core/component.rs (2)
10-16: Unify event type across UIComponents using crossterm::Event while the app routes EventType increases coupling to the backend. Consider handling your internal EventType here to keep crossterm localized.
- use crossterm::event::{Event, KeyEvent}; + use crossterm::event::KeyEvent; + use super::event_handler::EventType; @@ - fn handle_events(&mut self, event: Option<Event>) -> Action { - if let Some(Event::Key(key)) = event { + fn handle_events(&mut self, event: Option<EventType>) -> Action { + if let Some(EventType::Key(key)) = event { self.handle_key_events(key) } else { Action::None } }
25-26: Render signature relies on ratatui’s Frame aliasIf you plan to support multiple backends or future ratatui changes, consider a generic method signature (e.g., render<B: ratatui::backend::Backend>(&mut self, f: &mut ratatui::Frame, rect: Rect)).
src/storage.rs (4)
345-367: Consider updating sync metadata on single-project upsertKeep
last_sync_projectsconsistent after immediate upserts.pub async fn store_single_project(&self, project: Project) -> Result<()> { let local_project: LocalProject = project.into(); sqlx::query( @@ .execute(&self.pool) .await?; - Ok(()) + self.update_sync_timestamp("projects").await }
442-472: Consider updating sync metadata on single-task upsertMirror batch behavior for
tasks.pub async fn store_single_task(&self, task: Task) -> Result<()> { let local_task: LocalTask = task.into(); @@ .execute(&self.pool) .await?; - Ok(()) + self.update_sync_timestamp("tasks").await }
217-307: Add indexes for hot pathsTo keep UI snappy, index frequent predicates:
(project_id),(is_deleted, is_completed),(due_date), and label lookups.Example:
CREATE INDEX IF NOT EXISTS idx_tasks_project_id ON tasks(project_id); CREATE INDEX IF NOT EXISTS idx_tasks_deleted_completed ON tasks(is_deleted, is_completed); CREATE INDEX IF NOT EXISTS idx_tasks_due_date ON tasks(due_date); CREATE INDEX IF NOT EXISTS idx_labels_order ON labels(order_index);
520-547: Prefer compile-time checked queries
sqlx::query_as!(orquery!) would catch column/type mismatches (e.g., nullabledescription) at build time.src/ui/components/sidebar_component.rs (4)
201-209: Compute real tree depth for indentationIf deeper nesting appears, show correct indentation.
- /// Since Todoist only has parent/child (no deeper nesting), depth is either 0 or 1 - fn calculate_tree_depth(&self, project: &ProjectDisplay) -> usize { - if project.parent_id.is_some() { - 1 - } else { - 0 - } - } + fn calculate_tree_depth(&self, project: &ProjectDisplay) -> usize { + let mut depth = 0usize; + let mut current = project; + while let Some(parent_id) = ¤t.parent_id { + if let Some(parent) = self.projects.iter().find(|p| p.id == *parent_id) { + depth += 1; + current = parent; + } else { + break; + } + } + depth + }
215-223: Support lowercase vim keys ‘j’/‘k’Common TUI convention.
- KeyCode::Char('J') | KeyCode::Down => { + KeyCode::Char('j') | KeyCode::Char('J') | KeyCode::Down => { @@ - KeyCode::Char('K') | KeyCode::Up => { + KeyCode::Char('k') | KeyCode::Char('K') | KeyCode::Up => {
283-287: Unicode-safe truncationSlicing strings by byte index can split UTF‑8 graphemes and panic. Truncate by graphemes or display width.
Example helper (outside this file):
fn truncate_graphemes(s: &str, max: usize) -> String { use unicode_segmentation::UnicodeSegmentation; let g = s.graphemes(true).collect::<Vec<_>>(); if g.len() > max { format!("{}...", g[..max.saturating_sub(3)].concat()) } else { s.to_string() } }Then replace current slicing with the helper for labels and projects.
Also applies to: 318-324
14-19: Consider making fields privateExpose setters/APIs instead of public fields to keep invariants on
selection/projects/labels/icons.src/ui/components/dialog_component.rs (5)
67-113: Reject blank/whitespace-only submissions.Use
.trim().is_empty()across submit branches.Apply this diff:
- if !self.input_buffer.is_empty() { + if !self.input_buffer.trim().is_empty() { @@ - if !self.input_buffer.is_empty() { + if !self.input_buffer.trim().is_empty() { @@ - if !self.input_buffer.is_empty() { + if !self.input_buffer.trim().is_empty() { @@ - if !self.input_buffer.is_empty() { + if !self.input_buffer.trim().is_empty() {Also applies to: 114-125, 126-136, 137-148
1059-1091: Avoid cloning DialogType in render (unnecessary allocs).Borrow instead of cloning and pass string refs.
Apply this diff:
- fn render(&mut self, f: &mut Frame, rect: Rect) { - if let Some(dialog_type) = self.dialog_type.clone() { - match dialog_type { + fn render(&mut self, f: &mut Frame, rect: Rect) { + if let Some(dialog_type) = self.dialog_type.as_ref() { + match dialog_type { DialogType::TaskCreation { .. } => self.render_task_creation_dialog(f, rect), DialogType::TaskEdit { .. } => self.render_task_edit_dialog(f, rect), DialogType::ProjectCreation => { self.render_project_creation_dialog(f, rect); } DialogType::ProjectEdit { .. } => { self.render_project_edit_dialog(f, rect); } DialogType::LabelCreation => { self.render_label_creation_dialog(f, rect); } DialogType::LabelEdit { .. } => { self.render_label_edit_dialog(f, rect); } - DialogType::DeleteConfirmation { item_type, .. } => { - self.render_delete_confirmation_dialog(f, rect, &item_type); + DialogType::DeleteConfirmation { item_type, .. } => { + self.render_delete_confirmation_dialog(f, rect, item_type); } - DialogType::Info(message) => { - self.render_info_dialog(f, rect, &message); + DialogType::Info(message) => { + self.render_info_dialog(f, rect, message); } - DialogType::Error(message) => { - self.render_error_dialog(f, rect, &message); + DialogType::Error(message) => { + self.render_error_dialog(f, rect, message); } DialogType::Help => { self.render_help_dialog(f, rect); } DialogType::Logs => { self.render_logs_dialog(f, rect); } } } }
206-209: Avoid setting scroll_offset to usize::MAX.This sentinel is brittle and leaks into state updates. Track a boolean “jump_to_bottom” or clamp using known content length at render time without mutating
scrollbar_statehere.Happy to sketch a minimal change that defers clamping entirely to render paths.
746-753: Use IconService for emojis to respect ASCII/unicode themes.Hardcoded emojis in titles/legends can degrade in non-Unicode terminals.
Example:
- .title("📖 Help - Press 'Esc', '?' or 'h' to close") + .title(format!("{} Help - Press 'Esc', '?' or 'h' to close", self.icons.info())) @@ - .title("🔍 Debug Logs - Press 'Esc', 'G' or 'q' to close") + .title(format!("{} Debug Logs - Press 'Esc', 'G' or 'q' to close", self.icons.info())) @@ -🔳 Pending task -✅ Completed task -❌ Deleted task +{} Pending task +{} Completed task +{} Deleted taskAnd expand IconService to expose suitable symbols.
Also applies to: 831-835, 686-691
995-1001: Minor UX: keep project selection valid when cycling with Tab after data refresh.Combined with clamping in
update_data, Tab cycling will remain safe even if the projects list changes.No changes needed beyond the
update_dataclamp above.Also applies to: 54-57
src/ui/core/task_manager.rs (2)
168-173: Avoid stringly-typed is_syncing().Checking
description.contains("sync")can false-match. Track a task kind flag or a dedicatedis_syncboolean onBackgroundTask.I can propose a minimal
enum Kind { Sync, Op, DataLoad }field and update filters.
197-265: Data load swallows errors with unwrap_or_default; consider surfacing partial failures.On failure to get tasks, you drop the error and continue silently. Emit an error action or log via DebugLogger to aid diagnostics.
I can wire an
Action::ShowDialog(Error(...))for the specific failing branch.src/ui/core/actions.rs (2)
5-11: Consider using more specific index types for better type safety.Using
usizeindices into vectors can lead to runtime panics if the indices are out of bounds. Consider using newtype wrappers or IDs that directly reference the entities.pub enum SidebarSelection { #[default] Today, // Today view (special view) Tomorrow, // Tomorrow view (special view) - Label(usize), // Index into labels vector - Project(usize), // Index into projects vector + Label(String), // Label ID + Project(String), // Project ID }This would eliminate the need for bounds checking and make the code more robust against data changes between selections.
88-89: Consider movingdefault_project_idto be a field at theDialogTypelevel.The
default_project_idfield appears only inTaskCreation, but based on the UI flow, other creation dialogs might benefit from having default context. Consider a more uniform approach.src/ui/components/task_list_component.rs (1)
741-742: Remove unused import.The
use std::collections::HashMap;import on line 741 appears to be redundant since HashMap is already imported at line 17.- use std::collections::HashMap; -src/ui/app_component.rs (2)
31-32: Remove obsolete comment about scrolling.The comment "didnt we just got rid of custom scrolling ?" appears to be a leftover from development and should be removed or clarified.
pub show_help: bool, - /// didnt we just got rid of custom scrolling ? pub help_scroll_offset: usize,
610-612: Error handling could be more robust.The toggle task operation returns a Result but only formats success/error messages. Consider propagating more detailed error information.
Consider using anyhow's context to add more information:
"Toggle task" => match sync_service.toggle_task(&task_info).await { Ok(()) => Ok(format!("✅ Task toggled: {}", task_info)), - Err(e) => Err(format!("❌ Failed to toggle task: {}", e)), + Err(e) => Err(format!("❌ Failed to toggle task {}: {}", task_info, e)), },
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (44)
Cargo.toml(1 hunks)README.md(1 hunks)docs/PRD.md(1 hunks)src/main.rs(1 hunks)src/storage.rs(6 hunks)src/sync.rs(5 hunks)src/todoist.rs(2 hunks)src/ui/app.rs(0 hunks)src/ui/app_component.rs(1 hunks)src/ui/components/badge.rs(2 hunks)src/ui/components/dialog_component.rs(1 hunks)src/ui/components/dialogs/debug_dialog.rs(0 hunks)src/ui/components/dialogs/delete_confirmation_dialog.rs(0 hunks)src/ui/components/dialogs/error_dialog.rs(0 hunks)src/ui/components/dialogs/info_dialog.rs(0 hunks)src/ui/components/dialogs/label_creation_dialog.rs(0 hunks)src/ui/components/dialogs/label_delete_confirmation_dialog.rs(0 hunks)src/ui/components/dialogs/label_edit_dialog.rs(0 hunks)src/ui/components/dialogs/mod.rs(0 hunks)src/ui/components/dialogs/project_creation_dialog.rs(0 hunks)src/ui/components/dialogs/project_delete_confirmation_dialog.rs(0 hunks)src/ui/components/dialogs/project_edit_dialog.rs(0 hunks)src/ui/components/dialogs/syncing_dialog.rs(0 hunks)src/ui/components/dialogs/task_creation_dialog.rs(0 hunks)src/ui/components/dialogs/task_edit_dialog.rs(0 hunks)src/ui/components/help_panel.rs(0 hunks)src/ui/components/mod.rs(1 hunks)src/ui/components/projects_list.rs(0 hunks)src/ui/components/sidebar_component.rs(1 hunks)src/ui/components/status_bar.rs(0 hunks)src/ui/components/task_list_component.rs(1 hunks)src/ui/components/tasks_list.rs(0 hunks)src/ui/core/actions.rs(1 hunks)src/ui/core/component.rs(1 hunks)src/ui/core/context.rs(1 hunks)src/ui/core/event_handler.rs(1 hunks)src/ui/core/mod.rs(1 hunks)src/ui/core/task_manager.rs(1 hunks)src/ui/events.rs(0 hunks)src/ui/mod.rs(1 hunks)src/ui/new_renderer.rs(1 hunks)src/ui/renderer.rs(0 hunks)src/utils/date.rs(1 hunks)src/utils/mod.rs(1 hunks)
💤 Files with no reviewable changes (21)
- src/ui/components/status_bar.rs
- src/ui/components/dialogs/delete_confirmation_dialog.rs
- src/ui/components/projects_list.rs
- src/ui/components/dialogs/task_edit_dialog.rs
- src/ui/components/dialogs/debug_dialog.rs
- src/ui/renderer.rs
- src/ui/components/dialogs/task_creation_dialog.rs
- src/ui/components/dialogs/label_delete_confirmation_dialog.rs
- src/ui/components/dialogs/label_creation_dialog.rs
- src/ui/components/dialogs/project_creation_dialog.rs
- src/ui/events.rs
- src/ui/components/dialogs/label_edit_dialog.rs
- src/ui/components/dialogs/project_delete_confirmation_dialog.rs
- src/ui/components/dialogs/syncing_dialog.rs
- src/ui/components/dialogs/error_dialog.rs
- src/ui/components/dialogs/info_dialog.rs
- src/ui/components/help_panel.rs
- src/ui/components/dialogs/mod.rs
- src/ui/app.rs
- src/ui/components/tasks_list.rs
- src/ui/components/dialogs/project_edit_dialog.rs
🧰 Additional context used
🧬 Code graph analysis (19)
src/ui/core/mod.rs (1)
src/lib.rs (1)
ui(6-6)
src/ui/components/badge.rs (1)
src/ui/components/task_list_component.rs (1)
create_badge(471-473)
src/ui/components/mod.rs (1)
src/ui/components/dialogs/mod.rs (5)
task_edit_dialog(15-15)debug_dialog(3-3)task_creation_dialog(14-14)syncing_dialog(13-13)project_edit_dialog(12-12)
src/ui/core/context.rs (7)
src/icons.rs (1)
icons(114-120)src/sync.rs (2)
sync(353-373)new(29-40)src/ui/app_component.rs (1)
new(78-101)src/ui/components/dialog_component.rs (2)
new(39-52)default(33-35)src/ui/components/sidebar_component.rs (2)
new(28-35)default(22-24)src/ui/components/task_list_component.rs (2)
new(81-92)default(75-77)src/ui/app.rs (3)
App(20-71)App(79-1233)new(82-138)
src/ui/components/dialog_component.rs (6)
src/icons.rs (3)
icons(114-120)info(253-255)warning(258-260)src/ui/components/task_list_component.rs (5)
default(75-77)new(81-92)handle_key_events(525-605)update(607-633)render(635-678)src/sync.rs (2)
new(29-40)set_debug_logger(43-45)src/ui/app_component.rs (4)
new(78-101)handle_key_events(845-848)update(850-857)render(859-882)src/ui/layout.rs (1)
centered_rect(67-85)src/ui/core/component.rs (3)
handle_key_events(18-18)update(20-23)render(25-25)
src/ui/components/sidebar_component.rs (3)
src/icons.rs (3)
icons(114-120)today(296-302)label(287-293)src/ui/components/projects_list.rs (2)
Sidebar(15-15)Sidebar(17-209)src/ui/app.rs (1)
SidebarSelection(12-17)
src/ui/core/actions.rs (2)
src/sync.rs (1)
sync(353-373)src/ui/app.rs (3)
App(20-71)App(79-1233)SidebarSelection(12-17)
src/sync.rs (1)
src/ui/app.rs (3)
load_local_data(469-556)toggle_selected_task(668-694)create_task(941-989)
src/main.rs (4)
src/sync.rs (2)
sync(353-373)new(29-40)src/ui/app_component.rs (1)
new(78-101)src/ui/new_renderer.rs (1)
run_new_app(17-50)src/ui/renderer.rs (1)
run_app(26-67)
src/ui/new_renderer.rs (3)
src/sync.rs (2)
sync(353-373)new(29-40)src/ui/app_component.rs (1)
new(78-101)src/ui/core/event_handler.rs (2)
new(13-19)should_render(44-46)
src/ui/core/component.rs (4)
src/ui/app_component.rs (3)
handle_key_events(845-848)update(850-857)render(859-882)src/ui/components/dialog_component.rs (3)
handle_key_events(857-1007)update(1009-1057)render(1059-1093)src/ui/components/sidebar_component.rs (3)
handle_key_events(213-225)update(227-236)render(238-343)src/ui/components/task_list_component.rs (3)
handle_key_events(525-605)update(607-633)render(635-678)
README.md (1)
src/ui/app.rs (2)
App(20-71)App(79-1233)
src/ui/components/task_list_component.rs (5)
src/icons.rs (3)
icons(114-120)label(287-293)tasks_title(238-240)src/ui/components/badge.rs (7)
to_style(23-57)create_priority_badge(139-147)create_task_badges(109-135)create_badge(62-64)create_bracket_badge(68-70)create_paren_badge(74-76)create_label_badge(80-105)src/ui/components/dialog_component.rs (6)
default(33-35)new(39-52)update_data(54-57)handle_key_events(857-1007)update(1009-1057)render(1059-1093)src/ui/app_component.rs (5)
new(78-101)update_data(37-48)handle_key_events(845-848)update(850-857)render(859-882)src/ui/components/tasks_list.rs (3)
create_task_item(250-341)create_task_list_items(64-147)create_today_task_items(150-217)
src/ui/core/event_handler.rs (2)
src/ui/renderer.rs (1)
run_ui(70-123)src/ui/events.rs (2)
handle_task_creation_mode(198-223)handle_debug_modal(518-552)
src/ui/core/task_manager.rs (2)
src/sync.rs (3)
sync(353-373)new(29-40)is_syncing(123-125)src/ui/app_component.rs (3)
new(78-101)spawn_task_operation(600-742)is_syncing(113-115)
docs/PRD.md (4)
src/ui/app.rs (2)
App(20-71)App(79-1233)src/ui/components/dialogs/mod.rs (1)
syncing_dialog(13-13)src/lib.rs (1)
ui(6-6)src/ui/components/projects_list.rs (1)
Sidebar(15-15)
src/storage.rs (1)
src/sync.rs (1)
update_task_due_date(273-303)
src/ui/mod.rs (4)
src/ui/new_renderer.rs (1)
run_new_app(17-50)src/ui/renderer.rs (1)
render_ui(126-192)src/ui/app.rs (1)
App(20-71)src/ui/layout.rs (1)
LayoutManager(6-6)
src/ui/app_component.rs (6)
src/sync.rs (3)
sync(353-373)new(29-40)is_syncing(123-125)src/ui/components/dialog_component.rs (6)
update_data(54-57)new(39-52)default(33-35)handle_key_events(857-1007)update(1009-1057)render(1059-1093)src/ui/components/sidebar_component.rs (6)
update_data(37-40)new(28-35)default(22-24)handle_key_events(213-225)update(227-236)render(238-343)src/ui/components/task_list_component.rs (6)
update_data(94-108)new(81-92)default(75-77)handle_key_events(525-605)update(607-633)render(635-678)src/ui/core/task_manager.rs (4)
new(38-49)default(280-283)is_syncing(169-173)spawn_task_operation(90-138)src/utils/date.rs (2)
next_weekday(11-19)format_ymd(6-8)
⏰ 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: Security Audit
- GitHub Check: Test (beta)
- GitHub Check: Test (stable)
🔇 Additional comments (21)
src/ui/components/badge.rs (1)
141-146: Confirm change: wildcard now returns P4 badge instead of skippingThis makes
create_priority_badge(…)always returnSome, so unknown priorities will render a white “P4” badge rather than nothing.
- If this is desired, change the signature to
-> Span<'static>and drop theOption(remove wrappingSome/None).- If not, revert
_ => Some(…)back to_ => Noneto restore “no badge” for unknown values.src/utils/date.rs (1)
47-47: LGTM: trailing newline only; no functional changes.src/utils/mod.rs (1)
1-1: LGTM: module export intact.Cargo.toml (2)
3-3: Version bump to 0.2.0: looks good.
3-3: Pin git dependency and verify ratatui/crossterm compatibility
- Use a fixed commit for
todoist-api:todoist-api = { git = "https://github.com/romaintb/todoist-api", rev = "<PINNED_COMMIT_SHA>" }- Manually confirm you have a single
crosstermversion by running:cargo tree -p crosstermsrc/ui/core/event_handler.rs (1)
21-36: Render and tick intervals wired; fixed sleep removed; KeyEventKind::Press filtering added.src/sync.rs (2)
149-154: Immediate local persistence after project creation — LGTMStoring the created project locally right after the API call will make the UI feel snappier.
180-185: Immediate local persistence after task creation — LGTMSame positive effect for tasks; good call.
src/ui/new_renderer.rs (1)
123-133: get_status last_sync is always NonePlumb the last sync into AppComponent state or query a cached value so this field is meaningful.
src/storage.rs (1)
74-75: ProjectDisplay mapping: inbox flag propagation looks goodThe new
is_inbox_projectis correctly forwarded.src/ui/components/mod.rs (1)
5-13: LGTM: module layout and re-exports match the new component architectureThe module surface is clean and minimal.
src/ui/components/dialog_component.rs (1)
8-8: Ignore incorrect import suggestion
centered_rectis defined as an associated function onLayoutManager;use crate::ui::layout::LayoutManagerand calls toLayoutManager::centered_rect(...)are valid.Likely an incorrect or invalid review comment.
src/ui/core/actions.rs (1)
61-66: DataLoaded action carries display types directly.The
DataLoadedaction variant properly uses display types from the todoist module, which is good for separation of concerns.src/ui/components/task_list_component.rs (3)
115-117: Potential index out of bounds when adjusting selected_index.The code uses
saturating_subwhich is good, but the logic could be clearer by usingmindirectly.if self.selected_index >= self.tasks.len() { - self.selected_index = self.tasks.len().saturating_sub(1); + self.selected_index = self.tasks.len().saturating_sub(1).min(self.tasks.len() - 1); }Actually, the existing code is correct. The saturating_sub already handles the edge case properly.
559-574: Good project context handling for task creation.The logic properly determines the default project based on the current sidebar selection, defaulting to inbox for non-project views.
194-195: No changes needed for date parsingTodoist’s
due.datefield is returned inYYYY-MM-DDformat, which aligns withchrono::NaiveDate::parse_from_str(due_date_str, "%Y-%m-%d").src/ui/app_component.rs (5)
620-632: Good date string extraction pattern.The code properly extracts task_id from the compound string format and handles the split correctly.
653-666: Date utility functions are properly used.The code correctly uses the date utility functions for calculating next week and weekend dates.
738-739: Good error conversion pattern.Converting string errors to anyhow errors with
map_erris the right approach.
789-841: Well-structured event handling hierarchy.The event handling properly prioritizes dialog > sidebar > task list > global keys, which is the correct order for a modal UI.
114-115: Consider using the sync service's is_syncing method.Instead of tracking
active_sync_tasklocally, you could directly use the sync service'sis_syncing()method to reduce state duplication.pub fn is_syncing(&self) -> bool { - self.active_sync_task.is_some() + // Use tokio::task::block_in_place or make this async + // For now, keeping local state is actually better for performance + self.active_sync_task.is_some() }Actually, keeping the local state is better to avoid blocking async calls in a sync context.
| pub is_inbox_project: bool, | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Good addition: propagate inbox flag; add a test and scan for struct literals.
- Field is correctly mapped from API to display.
- Please assert it in tests and ensure no direct
ProjectDisplay { ... }literals are missing the new field.
Add to test_project_conversion:
assert_eq!(display.is_inbox_project, false);Scan for potential breakages:
Also applies to: 57-58
🏁 Script executed:
#!/bin/bash
# Find all struct literal inits; confirm they set `is_inbox_project`
rg -nP --type=rust 'ProjectDisplay\s*\{' -C2Length of output: 1403
Include is_inbox_project in all ProjectDisplay literals
- In
impl From<Project> for ProjectDisplay(src/todoist.rs), add
is_inbox_project: project.is_inbox_project(or the correct flag) in theSelf { … }block. - In
impl From<LocalProject> for ProjectDisplay(src/storage.rs), add
is_inbox_project: falsein itsSelf { … }. - In the row-mapping in src/storage.rs (
.map(|row| ProjectDisplay { … })), add
is_inbox_project: row.get("is_inbox_project"). - Update
test_project_conversionto include
assert_eq!(display.is_inbox_project, false);.
🤖 Prompt for AI Agents
In src/todoist.rs around lines 12-13 (the ProjectDisplay struct and its
From<Project> impl), src/storage.rs where impl From<LocalProject> for
ProjectDisplay is defined and where rows are mapped (.map(|row| ProjectDisplay {
… })), and the test file with test_project_conversion, add the missing
is_inbox_project field to every ProjectDisplay construction: in From<Project>
set is_inbox_project: project.is_inbox_project (or the correct source flag), in
From<LocalProject> set is_inbox_project: false, and in the row-mapping set
is_inbox_project: row.get("is_inbox_project"); then update the
test_project_conversion to assert_eq!(display.is_inbox_project, false); so all
literals and mappings include the new boolean field consistently.
| pub fn update_data(&mut self, projects: Vec<ProjectDisplay>, labels: Vec<LabelDisplay>) { | ||
| self.projects = projects; | ||
| self.labels = labels; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Clamp selected_project_index when refreshing data to avoid OOB panics.
If projects shrink while a dialog is open, self.projects[self.selected_project_index] can panic.
Apply this diff:
pub fn update_data(&mut self, projects: Vec<ProjectDisplay>, labels: Vec<LabelDisplay>) {
self.projects = projects;
self.labels = labels;
+ // Keep selection in-bounds
+ if !self.projects.is_empty() {
+ self.selected_project_index = self.selected_project_index.min(self.projects.len() - 1);
+ } else {
+ self.selected_project_index = 0;
+ }
}📝 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.
| pub fn update_data(&mut self, projects: Vec<ProjectDisplay>, labels: Vec<LabelDisplay>) { | |
| self.projects = projects; | |
| self.labels = labels; | |
| } | |
| pub fn update_data(&mut self, projects: Vec<ProjectDisplay>, labels: Vec<LabelDisplay>) { | |
| self.projects = projects; | |
| self.labels = labels; | |
| // Keep selection in-bounds | |
| if !self.projects.is_empty() { | |
| self.selected_project_index = self.selected_project_index.min(self.projects.len() - 1); | |
| } else { | |
| self.selected_project_index = 0; | |
| } | |
| } |
🤖 Prompt for AI Agents
In src/ui/components/dialog_component.rs around lines 54 to 57, when updating
self.projects the code doesn't clamp self.selected_project_index, which can lead
to an out-of-bounds panic if the projects list has shrunk; update the method to
reset or clamp selected_project_index after assigning self.projects — e.g., if
projects.is_empty() set selected_project_index to 0, otherwise set it to
min(self.selected_project_index, projects.len() - 1) (use saturating_sub(1) or
checked arithmetic to avoid underflow).
| KeyCode::Esc => Action::HideDialog, | ||
| KeyCode::Enter => self.handle_submit(), | ||
| KeyCode::Char(c) => { | ||
| self.input_buffer.insert(self.cursor_position, c); | ||
| self.cursor_position += 1; | ||
| Action::None | ||
| } | ||
| KeyCode::Backspace => { | ||
| if self.cursor_position > 0 { | ||
| self.input_buffer.remove(self.cursor_position - 1); | ||
| self.cursor_position -= 1; | ||
| } | ||
| Action::None | ||
| } | ||
| KeyCode::Delete => { | ||
| if self.cursor_position < self.input_buffer.len() { | ||
| self.input_buffer.remove(self.cursor_position); | ||
| } | ||
| Action::None | ||
| } | ||
| KeyCode::Left => { | ||
| if self.cursor_position > 0 { | ||
| self.cursor_position -= 1; | ||
| } | ||
| Action::None | ||
| } | ||
| KeyCode::Right => { | ||
| if self.cursor_position < self.input_buffer.len() { | ||
| self.cursor_position += 1; | ||
| } | ||
| Action::None | ||
| } |
There was a problem hiding this comment.
Fix Unicode cursor/editing: using byte indices as “char” count can panic/corrupt text.
String::len/insert/remove operate on bytes; the current per-1 increments break for multi-byte chars and can panic on removal. Track cursor as a byte offset and move/delete on char boundaries.
Apply this diff:
- KeyCode::Char(c) => {
- self.input_buffer.insert(self.cursor_position, c);
- self.cursor_position += 1;
- Action::None
- }
+ KeyCode::Char(c) => {
+ self.input_buffer.insert(self.cursor_position, c);
+ self.cursor_position += c.len_utf8();
+ Action::None
+ }
@@
- KeyCode::Backspace => {
- if self.cursor_position > 0 {
- self.input_buffer.remove(self.cursor_position - 1);
- self.cursor_position -= 1;
- }
- Action::None
- }
+ KeyCode::Backspace => {
+ if self.cursor_position > 0 {
+ if let Some(prev) = self.input_buffer[..self.cursor_position].chars().next_back() {
+ let new_pos = self.cursor_position - prev.len_utf8();
+ self.input_buffer.drain(new_pos..self.cursor_position);
+ self.cursor_position = new_pos;
+ } else {
+ self.cursor_position = 0;
+ }
+ }
+ Action::None
+ }
@@
- KeyCode::Delete => {
- if self.cursor_position < self.input_buffer.len() {
- self.input_buffer.remove(self.cursor_position);
- }
- Action::None
- }
+ KeyCode::Delete => {
+ if self.cursor_position < self.input_buffer.len() {
+ if let Some(next) = self.input_buffer[self.cursor_position..].chars().next() {
+ let end = self.cursor_position + next.len_utf8();
+ self.input_buffer.drain(self.cursor_position..end);
+ }
+ }
+ Action::None
+ }
@@
- KeyCode::Left => {
- if self.cursor_position > 0 {
- self.cursor_position -= 1;
- }
- Action::None
- }
+ KeyCode::Left => {
+ if self.cursor_position > 0 {
+ if let Some(prev) = self.input_buffer[..self.cursor_position].chars().next_back() {
+ self.cursor_position -= prev.len_utf8();
+ } else {
+ self.cursor_position = 0;
+ }
+ }
+ Action::None
+ }
@@
- KeyCode::Right => {
- if self.cursor_position < self.input_buffer.len() {
- self.cursor_position += 1;
- }
- Action::None
- }
+ KeyCode::Right => {
+ if self.cursor_position < self.input_buffer.len() {
+ if let Some(next) = self.input_buffer[self.cursor_position..].chars().next() {
+ self.cursor_position += next.len_utf8();
+ } else {
+ self.cursor_position = self.input_buffer.len();
+ }
+ }
+ Action::None
+ }Also applies to: 970-981, 977-981, 983-988, 989-994
| /// Get the root project ID (top-level parent) | ||
| /// Since Todoist only has parent/child, root is either the project itself or its parent | ||
| fn get_root_project_id(&self, project: &ProjectDisplay) -> String { | ||
| project | ||
| .parent_id | ||
| .clone() | ||
| .unwrap_or_else(|| project.id.clone()) | ||
| } | ||
|
|
||
| /// Get the root project (top-level parent) - always returns from self.projects | ||
| fn get_root_project(&self, project: &ProjectDisplay) -> &ProjectDisplay { | ||
| let root_id = self.get_root_project_id(project); | ||
| self.projects | ||
| .iter() | ||
| .find(|p| p.id == root_id) | ||
| .expect("Root project should exist in projects list") | ||
| } | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid panic when resolving root project; support deeper hierarchies
expect will panic if parent not present; also get_root_project_id assumes max depth=1. Traverse parents and fall back gracefully.
- /// Get the root project ID (top-level parent)
- /// Since Todoist only has parent/child, root is either the project itself or its parent
- fn get_root_project_id(&self, project: &ProjectDisplay) -> String {
- project
- .parent_id
- .clone()
- .unwrap_or_else(|| project.id.clone())
- }
+ /// Get the top-level ancestor ID by walking parent links
+ fn get_root_project_id(&self, project: &ProjectDisplay) -> String {
+ let mut current = project;
+ while let Some(parent_id) = ¤t.parent_id {
+ if let Some(parent) = self.projects.iter().find(|p| p.id == *parent_id) {
+ current = parent;
+ } else {
+ break;
+ }
+ }
+ current.id.clone()
+ }
@@
- fn get_root_project(&self, project: &ProjectDisplay) -> &ProjectDisplay {
- let root_id = self.get_root_project_id(project);
- self.projects
- .iter()
- .find(|p| p.id == root_id)
- .expect("Root project should exist in projects list")
- }
+ fn get_root_project(&self, project: &ProjectDisplay) -> &ProjectDisplay {
+ let root_id = self.get_root_project_id(project);
+ self.projects
+ .iter()
+ .find(|p| p.id == root_id)
+ .unwrap_or(project)
+ }📝 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.
| /// Get the root project ID (top-level parent) | |
| /// Since Todoist only has parent/child, root is either the project itself or its parent | |
| fn get_root_project_id(&self, project: &ProjectDisplay) -> String { | |
| project | |
| .parent_id | |
| .clone() | |
| .unwrap_or_else(|| project.id.clone()) | |
| } | |
| /// Get the root project (top-level parent) - always returns from self.projects | |
| fn get_root_project(&self, project: &ProjectDisplay) -> &ProjectDisplay { | |
| let root_id = self.get_root_project_id(project); | |
| self.projects | |
| .iter() | |
| .find(|p| p.id == root_id) | |
| .expect("Root project should exist in projects list") | |
| } | |
| /// Get the top-level ancestor ID by walking parent links | |
| fn get_root_project_id(&self, project: &ProjectDisplay) -> String { | |
| let mut current = project; | |
| while let Some(parent_id) = ¤t.parent_id { | |
| if let Some(parent) = self.projects.iter().find(|p| p.id == *parent_id) { | |
| current = parent; | |
| } else { | |
| break; | |
| } | |
| } | |
| current.id.clone() | |
| } | |
| /// Get the root project (top-level parent) - always returns from self.projects | |
| fn get_root_project(&self, project: &ProjectDisplay) -> &ProjectDisplay { | |
| let root_id = self.get_root_project_id(project); | |
| self.projects | |
| .iter() | |
| .find(|p| p.id == root_id) | |
| .unwrap_or(project) | |
| } |
🤖 Prompt for AI Agents
In src/ui/components/sidebar_component.rs around lines 183-200, the current code
assumes only one parent level and uses expect which can panic; update
get_root_project_id to iteratively walk the parent chain using self.projects to
look up each parent_id until you reach a project with no parent (or no matching
parent found) and return that root id (fallback to the original project.id if
lookup fails), and change get_root_project to avoid expect by returning either
an Option<&ProjectDisplay> or returning the input project reference when the
resolved root id isn't found in self.projects so the code no longer panics.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/ui/core/task_manager.rs (1)
109-113: Project deletion navigation fix looks good.Using starts_with("Delete project") resolves the prior dead branch.
🧹 Nitpick comments (10)
src/ui/core/task_manager.rs (8)
59-76: Don’t emit StartSync when a sync is already running.Avoid flashing a “start” signal if SyncService reports in-progress; short-circuit and surface InProgress directly.
let handle = tokio::spawn(async move { - // Send sync started notification - let _ = action_sender.send(Action::StartSync); + // If a sync is already running, don't emit StartSync again + if sync_service.is_syncing().await { + let status = SyncStatus::InProgress; + let _ = action_sender.send(Action::SyncCompleted(status.clone())); + return Ok(TaskResult::SyncCompleted(status)); + } + // Send sync started notification + let _ = action_sender.send(Action::StartSync); match sync_service.force_sync().await {
98-101: Trim an extra clone in spawn_task_operation.Only one clone is needed; move the original into the task and keep one clone for struct storage.
- let action_sender = self.action_sender.clone(); - let desc_clone = description.clone(); - let desc_for_task = description.clone(); + let action_sender = self.action_sender.clone(); + let desc_for_task = description.clone(); + let desc_clone = description; // moved into the task
140-161: Return real task results from finished handles (make async).cleanup_finished_tasks discards actual outcomes. If callers use the returned Vec, await finished handles to propagate results.
- pub fn cleanup_finished_tasks(&mut self) -> Vec<(TaskId, anyhow::Result<TaskResult>)> { + pub async fn cleanup_finished_tasks(&mut self) -> Vec<(TaskId, anyhow::Result<TaskResult>)> { let mut completed = Vec::new(); let mut to_remove = Vec::new(); @@ - for task_id in to_remove { - if let Some(_task) = self.tasks.remove(&task_id) { - // Since the task is finished, we'll just mark it as completed - // The actual result was already sent via the action channel - let result = Ok(TaskResult::Other("Task completed".to_string())); - completed.push((task_id, result)); - } - } + for task_id in to_remove { + if let Some(task) = self.tasks.remove(&task_id) { + // Join is non-blocking here because is_finished() was true + let result = match task.handle.await { + Ok(inner) => inner, + Err(e) => Err(anyhow::anyhow!(format!("task join error: {}", e))), + }; + completed.push((task_id, result)); + } + }Note: update call sites to await this method.
168-173: Avoid string-matching to detect sync tasks; track kind explicitly.String contains("sync") is brittle. Add a TaskKind and set it at spawn sites.
pub type TaskId = u64; +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum TaskKind { + Sync, + Operation, + DataLoad, +} + #[derive(Debug)] pub struct BackgroundTask { pub id: TaskId, - pub handle: JoinHandle<anyhow::Result<TaskResult>>, + pub handle: JoinHandle<anyhow::Result<TaskResult>>, pub description: String, pub started_at: std::time::Instant, + pub kind: TaskKind, } @@ let task = BackgroundTask { id: task_id, handle, description, started_at: std::time::Instant::now(), + kind: TaskKind::Sync, }; @@ let task = BackgroundTask { id: task_id, handle, description: desc_for_task, started_at: std::time::Instant::now(), + kind: TaskKind::Operation, }; @@ let task = BackgroundTask { id: task_id, handle, description, started_at: std::time::Instant::now(), + kind: TaskKind::DataLoad, }; @@ pub fn is_syncing(&self) -> bool { - self.tasks - .values() - .any(|task| task.description.contains("sync")) + self.tasks.values().any(|task| matches!(task.kind, TaskKind::Sync)) }Also applies to: 79-84, 129-136, 267-274, 7-15
205-265: Load projects/labels/sections concurrently.These awaits are sequential; use try_join! to cut wait time.
- let handle = tokio::spawn(async move { - match ( - sync_service.get_projects().await, - sync_service.get_labels().await, - sync_service.get_sections().await, - ) { - (Ok(projects), Ok(labels), Ok(sections)) => { + let handle = tokio::spawn(async move { + let (projects, labels, sections) = match tokio::try_join!( + sync_service.get_projects(), + sync_service.get_labels(), + sync_service.get_sections(), + ) { + Ok(vals) => vals, + Err(e) => { + let error_msg = format!("Failed to load data: {}", e); + let _ = action_sender.send(Action::ShowDialog( + crate::ui::core::actions::DialogType::Error(error_msg.clone()), + )); + return Ok(TaskResult::Other(error_msg)); + } + }; - // Get tasks based on sidebar selection + // Get tasks based on sidebar selection let tasks = match sidebar_selection {
248-253: Minimize clones when emitting DataLoaded and returning TaskResult.If TaskResult isn’t consumed, these clones are wasted. Either (a) drop TaskResult for fire-and-forget tasks, or (b) keep TaskResult but retrieve it via cleanup_finished_tasks and move values accordingly. Otherwise, consider wrapping in Arc to share.
Also applies to: 241-246
31-35: Public handle leaks cancellation outside the manager.Expose id/description/timestamps, but keep handle private to avoid external aborts; manage cancellation via TaskManager API.
pub struct TaskManager { @@ } @@ #[derive(Debug)] pub struct BackgroundTask { pub id: TaskId, - pub handle: JoinHandle<anyhow::Result<TaskResult>>, + handle: JoinHandle<anyhow::Result<TaskResult>>, pub description: String, pub started_at: std::time::Instant, }If external UI needs read-only stats, add getters instead of exposing the handle.
Also applies to: 39-49
34-35: Unbounded action channel can grow without limit.If the UI lags, actions may accumulate. Consider a small bounded channel with backpressure and .send().await to keep memory in check.
Also applies to: 39-49
src/icons.rs (2)
357-371: Tests cover all themes; minor DRY improvement.Factor repeated assertions via a small helper to reduce duplication and ease future changes:
fn assert_day_icons(theme: IconTheme, today: &str, tomorrow: &str) { let s = IconService::new(theme); assert_eq!(s.today(), today); assert_eq!(s.tomorrow(), tomorrow); }
304-311: Unify calendar icons via IconService and remove hard-coded glyphs
- Change
today()to delegate toself.icons().status.due_date(and add a matching field or helper for tomorrow inIconSet/IconService) to centralize theme mappings.- Replace literal
"📅 Today"and"📅 Tomorrow"insrc/ui/components/task_list_component.rs(lines 179 & 208) with calls toapp.icons.today()/app.icons.tomorrow()so the UI always uses the current theme.- Audit other hard-coded “Today”/“Tomorrow” icons across UI components and swap in the service methods for consistency.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/icons.rs(2 hunks)src/ui/components/sidebar_component.rs(1 hunks)src/ui/core/task_manager.rs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/ui/components/sidebar_component.rs
🧰 Additional context used
🧬 Code graph analysis (2)
src/icons.rs (3)
src/ui/components/sidebar_component.rs (1)
new(28-35)src/ui/components/tasks_list.rs (2)
create_tomorrow_task_items(220-237)create_today_task_items(150-217)src/ui/app.rs (1)
next_sidebar_item(308-364)
src/ui/core/task_manager.rs (3)
src/sync.rs (4)
sync(353-373)new(29-40)is_syncing(123-125)SyncService(27-493)src/ui/app_component.rs (3)
new(78-101)spawn_task_operation(600-742)is_syncing(113-115)src/todoist.rs (1)
TaskDisplay(30-45)
⏰ 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). (1)
- GitHub Check: Security Audit
| /// Spawn a background data loading operation | ||
| pub fn spawn_data_load(&mut self, sync_service: SyncService, sidebar_selection: SidebarSelection) -> TaskId { | ||
| let task_id = self.next_task_id; | ||
| self.next_task_id += 1; | ||
|
|
||
| let action_sender = self.action_sender.clone(); | ||
| let description = "Loading data from storage".to_string(); | ||
|
|
||
| let handle = tokio::spawn(async move { | ||
| match ( |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Guard against stale DataLoaded from out-of-order completions.
Multiple overlapping data loads can race; an older task finishing last may overwrite newer state. Consider canceling prior data-load tasks before spawning, or tagging loads with a generation and ignoring stale ones in the reducer.
Run to inspect DataLoaded handling and any generation checks:
Expected: a single in-flight data-load policy or a generation check at the action consumer.
🏁 Script executed:
#!/bin/bash
set -e
rg -nP -C3 'Action::DataLoaded\s*\{' -g '*.rs' src
rg -n 'spawn_data_load\(' -C2 -g '*.rs' srcLength of output: 1698
Prevent stale DataLoaded actions from overwriting newer data.
- Overlapping calls to
spawn_data_load(src/ui/core/task_manager.rs:198–206) can complete out of order and send an oldAction::DataLoaded(sent at line 248), overwriting more recent updates. - Either cancel any in-flight load before spawning a new one, or attach a monotonically increasing generation/tag to each load and have the reducer (in src/ui/app_component.rs:489) discard actions whose tag doesn’t match the latest.
🤖 Prompt for AI Agents
In src/ui/core/task_manager.rs around lines 197–206, overlapping spawn_data_load
calls can complete out-of-order and send stale Action::DataLoaded that
overwrites newer state; fix by adding a monotonically increasing generation/tag:
add a generation counter field to the TaskManager, increment it when spawning,
capture the current generation in the async task and include it in the
DataLoaded action payload, then update the reducer in src/ui/app_component.rs to
ignore DataLoaded actions whose tag doesn't match the TaskManager's latest
generation. Ensure the generation is updated atomically with next_task_id so
each load has a unique tag and no extra blocking/cancellation logic is required.
I know this is not the rust way to do things, but I'm just trying to get comfy with rust for now.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
src/todoist.rs (1)
12-13: Good addition: surface inbox flag in the display model.Ensure every ProjectDisplay literal across the codebase (e.g., storage mappings, UI components) sets this new field to avoid defaulting to false implicitly.
Run:
#!/bin/bash # Verify all ProjectDisplay literals include is_inbox_project rg -nP --type=rust -C2 'ProjectDisplay\s*\{'tests/todoist_test.rs (1)
20-25: Add an assertion for the inbox flag.Covers the newly added field on ProjectDisplay as previously requested.
let display: ProjectDisplay = project.into(); assert_eq!(display.id, "123"); assert_eq!(display.name, "Test Project"); assert_eq!(display.color, "blue"); assert!(display.is_favorite); + assert!(!display.is_inbox_project);
🧹 Nitpick comments (17)
tests/date_utils_test.rs (2)
1-3: Use explicit imports instead of a glob.Keeps the test namespace clean and catches accidental re-exports.
-use terminalist::utils::date::*; +use terminalist::utils::date::{format_ymd, next_weekday};
5-8: Good baseline; consider a leap-day case.Add a Feb 29 case to harden formatting coverage.
@@ #[test] fn test_format_ymd() { let date = NaiveDate::from_ymd_opt(2023, 12, 25).unwrap(); assert_eq!(format_ymd(date), "2023-12-25"); } + +#[test] +fn test_format_ymd_leap_day() { + let date = NaiveDate::from_ymd_opt(2024, 2, 29).unwrap(); + assert_eq!(format_ymd(date), "2024-02-29"); +}tests/icons_test.rs (2)
18-24: Deduplicate per-theme status icon testsThe three blocks are near-identical; consider a small table-driven helper to cut repetition and make future theme additions cheaper.
Example helper-based approach:
fn assert_status_icons(theme: IconTheme, pending: &str, completed: &str, deleted: &str) { let svc = IconService::new(theme); assert_eq!(svc.task_pending(), pending); assert_eq!(svc.task_completed(), completed); assert_eq!(svc.task_deleted(), deleted); }And call it for each theme in one test.
Also applies to: 26-32, 34-40
42-55: Also assert “today” stays in sync with StatusIcons.due_dateThis guards against drift between today() and the theme’s due_date glyph.
Apply this diff to extend the assertions:
@@ let emoji_service = IconService::new(IconTheme::Emoji); assert_eq!(emoji_service.today(), "📅"); assert_eq!(emoji_service.tomorrow(), "🗓️"); + assert_eq!(emoji_service.today(), emoji_service.icons().status.due_date); @@ let unicode_service = IconService::new(IconTheme::Unicode); assert_eq!(unicode_service.today(), "◷"); assert_eq!(unicode_service.tomorrow(), "◶"); + assert_eq!(unicode_service.today(), unicode_service.icons().status.due_date); @@ let ascii_service = IconService::new(IconTheme::Ascii); assert_eq!(ascii_service.today(), "@"); assert_eq!(ascii_service.tomorrow(), "+"); + assert_eq!(ascii_service.today(), ascii_service.icons().status.due_date);src/icons.rs (1)
304-310: Add a brief doc comment for the new tomorrow() APIPublic API looks good and matches tests. Add a one-liner doc to keep consistency with other convenience methods.
- #[must_use] - pub fn tomorrow(&self) -> &'static str { + /// Icon for "tomorrow" (next day) in the current theme. + #[must_use] + pub fn tomorrow(&self) -> &'static str { match self.current_theme { IconTheme::Emoji => "🗓️", IconTheme::Unicode => "◶", IconTheme::Ascii => "+", } }src/todoist.rs (2)
73-110: Trim clones and remove magic string in Task → TaskDisplay conversion.
- Destructure task to move fields and avoid cloning due/deadline strings.
- Replace hardcoded "blue" with a module-level constant.
Example sketch:
const DEFAULT_LABEL_COLOR: &str = "blue"; impl From<Task> for TaskDisplay { fn from(task: Task) -> Self { let Task { id, content, project_id, section_id, is_completed, priority, due, deadline, duration, labels, description, .. } = task; let duration = duration.map(|d| match d.unit.as_str() { "minute" => format!("{}m", d.amount), "hour" => format!("{}h", d.amount), "day" => format!("{}d", d.amount), _ => format!("{} {}", d.amount, d.unit), }); let (due, due_datetime, is_recurring) = match due { Some(d) => (Some(d.date), d.datetime, d.is_recurring), None => (None, None, false), }; let labels = labels.into_iter().map(|name| { let id = name.clone(); LabelDisplay { id, name, color: DEFAULT_LABEL_COLOR.to_string() } }).collect(); Self { id, content, project_id, section_id, is_completed, is_deleted: false, priority, due, due_datetime, is_recurring, deadline, duration, labels, description } } }
1-2: Consider narrowing the public re-export.Re-exporting todoist_api::* leaks the entire external API to your consumers. Prefer a curated prelude or explicit re-exports of only used types to keep your public surface stable.
tests/todoist_test.rs (3)
58-67: Strengthen Task conversion assertions.Validate all mapped fields and the defaulted flags to prevent regressions.
let display: TaskDisplay = task.into(); assert_eq!(display.id, "456"); assert_eq!(display.content, "Test Task"); assert_eq!(display.project_id, "123"); assert_eq!(display.priority, 3); assert!(!display.is_completed); assert!(display.is_recurring); assert_eq!(display.labels.len(), 2); assert_eq!(display.duration, Some("30m".to_string())); + assert_eq!(display.due, Some("2023-01-02".to_string())); + assert_eq!(display.due_datetime, None); + assert_eq!(display.deadline, None); + assert_eq!(display.section_id, None); + assert_eq!(display.description, "Test Description"); + assert!(!display.is_deleted); + assert!(display.labels.iter().all(|l| l.color == "blue"));
1-1: Reduce coupling to the crate’s re-export.Importing from terminalist::todoist::* ties tests to the module’s public surface. Import only the display types from terminalist and construct domain types directly from todoist_api if feasible.
27-67: Add a small Section conversion test.Covers Section → SectionDisplay mapping and ordering.
I can push a follow-up test like:
#[test] fn test_section_conversion() { let section = Section { id: "s1".into(), name: "Inbox".into(), project_id: "123".into(), order: 7 }; let display: SectionDisplay = section.into(); assert_eq!(display.id, "s1"); assert_eq!(display.name, "Inbox"); assert_eq!(display.project_id, "123"); assert_eq!(display.order, 7); }Do you want me to open a PR task for this?
tests/ui_renderer_test.rs (3)
3-9: Ignore or implement the placeholder testThis test does nothing; keep the intent visible without running it in CI.
-#[tokio::test] -async fn test_app_creation() { +#[tokio::test] +#[ignore = "Needs mock SyncService/AppComponent; implement after wiring settles"] +async fn test_app_creation() { // This would require a mock sync service // let sync_service = SyncService::new("dummy_token".to_string()).await.unwrap(); // let app = AppComponent::new(sync_service); // assert!(!app.should_quit()); }
11-22: Make the render-gate timing test deterministic (or add margin)Sleeping 17 ms is brittle under scheduler jitter. Prefer virtual time; otherwise, add headroom.
Option A (preferred; works if EventHandler uses tokio::time::Instant):
-#[tokio::test] +#[tokio::test(start_paused = true)] async fn test_event_handling() { // Test that the event handler can be created let event_handler = EventHandler::new(); // Initially should not need to render (just created) assert!(!event_handler.should_render()); - // After waiting, should be ready to render - tokio::time::sleep(tokio::time::Duration::from_millis(17)).await; - assert!(event_handler.should_render()); + // Advance virtual time to exercise the 16 ms gate deterministically + tokio::time::advance(tokio::time::Duration::from_millis(15)).await; + assert!(!event_handler.should_render()); + tokio::time::advance(tokio::time::Duration::from_millis(1)).await; + assert!(event_handler.should_render()); }Option B (fallback if EventHandler uses std::time::Instant):
- tokio::time::sleep(tokio::time::Duration::from_millis(17)).await; + tokio::time::sleep(tokio::time::Duration::from_millis(30)).await; // add jitter headroomTo confirm which clock is used, check EventHandler’s last-render timestamp type (tokio::time::Instant vs std::time::Instant).
11-12: Rename test to reflect intentClearer, behavior-focused name.
-async fn test_event_handling() { +async fn test_event_handler_should_render_gate() {src/ui/new_renderer.rs (4)
32-41: Remove unused render/cleanup intervals and simplify the loop signature.The explicit
cleanup_intervalandrender_intervalare created but never used (and are passed into_-prefixed params). SinceEventHandleralready owns timing, drop these to avoid confusion and needless timers.Apply:
- use tokio::time::{interval, Duration}; + // Timing is managed by EventHandler; no external intervals needed. @@ - // Create intervals for periodic tasks - let mut cleanup_interval = interval(Duration::from_secs(5)); // Clean up finished tasks every 5 seconds - let mut render_interval = interval(Duration::from_millis(16)); // ~60 FPS rendering - let result = run_app_loop( - &mut terminal, - &mut app, - &mut event_handler, - &mut cleanup_interval, - &mut render_interval, - ) - .await; + // Drive the main loop (EventHandler manages ticks/renders) + let result = run_app_loop(&mut terminal, &mut app, &mut event_handler).await; @@ -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, -) -> anyhow::Result<()> { +async fn run_app_loop<B: Backend>( + terminal: &mut Terminal<B>, + app: &mut AppComponent, + event_handler: &mut EventHandler, +) -> anyhow::Result<()> {Also applies to: 52-58
19-24: Attach error context to terminal lifecycle ops for easier debugging.Adding
anyhow::Contextto the raw-mode, alt-screen, and terminal ops yields actionable error messages (what failed and where).Apply:
+use anyhow::Context; @@ - enable_raw_mode()?; + enable_raw_mode().context("enabling raw mode")?; @@ - execute!(stdout, EnterAlternateScreen, EnableMouseCapture)?; + execute!(stdout, EnterAlternateScreen, EnableMouseCapture).context("entering alternate screen")?; @@ - let mut terminal = Terminal::new(backend)?; + let mut terminal = Terminal::new(backend).context("creating ratatui terminal")?; @@ - disable_raw_mode()?; - execute!(terminal.backend_mut(), LeaveAlternateScreen, DisableMouseCapture)?; + disable_raw_mode().context("disabling raw mode")?; + execute!(terminal.backend_mut(), LeaveAlternateScreen, DisableMouseCapture).context("leaving alternate screen")?;Also applies to: 45-47
135-139: Implementrequest_renderinstead of a no-op.Callers expecting this to force a redraw will be surprised. Minimal approach: track a flag in
AppComponentand let the loop consume it.Example (requires small additions in
AppComponent):// in AppComponent pub(crate) fn request_render(&mut self) { self.state.render_requested = true; } pub(crate) fn take_render_request(&mut self) -> bool { let was = self.state.render_requested; self.state.render_requested = false; was }Then in the loop, after handling an event:
if app.take_render_request() { needs_render = true; }
123-133: Surface a reallast_syncinAppStatus.
last_sync: Nonelimits usefulness. After a successful sync, storeSystemTime::now()inAppComponentand return it here.Example:
pub fn get_status(&self) -> AppStatus { AppStatus { active_tasks: self.active_task_count(), is_syncing: self.is_syncing(), - last_sync: None, // TODO: Track last sync time + last_sync: self.last_sync_time(), // e.g., Option<SystemTime> maintained by AppComponent total_tasks: self.total_tasks(), total_projects: self.total_projects(), } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
src/icons.rs(1 hunks)src/todoist.rs(2 hunks)src/ui/components/task_list_component.rs(1 hunks)src/ui/core/task_manager.rs(1 hunks)src/ui/new_renderer.rs(1 hunks)src/utils/date.rs(0 hunks)tests/date_utils_test.rs(1 hunks)tests/icons_test.rs(1 hunks)tests/todoist_test.rs(1 hunks)tests/ui_renderer_test.rs(1 hunks)
💤 Files with no reviewable changes (1)
- src/utils/date.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- src/ui/core/task_manager.rs
- src/ui/components/task_list_component.rs
🧰 Additional context used
🧬 Code graph analysis (4)
tests/icons_test.rs (2)
src/icons.rs (10)
icons(114-120)default(20-22)default(80-82)theme(94-96)new(88-90)task_pending(223-225)task_completed(228-230)task_deleted(233-235)today(296-302)tomorrow(305-311)src/ui/components/task_list_component.rs (2)
default(32-34)new(38-49)
tests/ui_renderer_test.rs (1)
src/ui/core/event_handler.rs (1)
should_render(44-46)
tests/date_utils_test.rs (1)
src/utils/date.rs (2)
format_ymd(6-8)next_weekday(11-19)
src/ui/new_renderer.rs (5)
src/sync.rs (2)
sync(353-373)new(29-40)src/ui/app_component.rs (1)
new(78-101)src/ui/core/event_handler.rs (1)
new(13-19)src/ui/renderer.rs (3)
run_ui(70-123)run_app(26-67)render_ui(126-192)src/ui/app.rs (2)
App(79-1233)App(20-71)
🔇 Additional comments (8)
tests/date_utils_test.rs (2)
11-16: LGTM: correct Friday → Monday advance.Matches strict “next” semantics of next_weekday.
18-24: LGTM: same-day goes to next week.Asserts the intended delta=7 behavior when weekdays match.
tests/icons_test.rs (3)
3-7: Solid default-theme assertionMatches IconTheme::default() implementation. No changes needed.
10-16: Theme switching coveredHappy path for set_theme is exercised and aligned with cycle order.
57-70: Cycle order verifiedCovers the full wrap-around Ascii → Unicode → Emoji → Ascii.
src/todoist.rs (1)
57-58: LGTM: correct propagation from Project → ProjectDisplay.tests/ui_renderer_test.rs (1)
16-18: Initial assertion looks goodThe “fresh instance ⇒ no render” check matches the 16 ms threshold logic.
src/ui/new_renderer.rs (1)
61-67: LGTM: render only when needed.The
needs_rendergate avoids redundant draws and pairs well with theEventType::Renderand background-action-driven renders.
| use crossterm::{ | ||
| event::DisableMouseCapture, | ||
| execute, | ||
| terminal::{disable_raw_mode, enable_raw_mode, EnterAlternateScreen, LeaveAlternateScreen}, | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Make terminal setup/teardown symmetric (enable mouse capture, hide cursor, clear screen).
We disable mouse capture and show the cursor on exit but never enable/hide them on entry. This causes inconsistent UX and can leave the cursor visible during rendering.
Apply:
use crossterm::{
- event::DisableMouseCapture,
+ event::{DisableMouseCapture, EnableMouseCapture},
execute,
terminal::{disable_raw_mode, enable_raw_mode, EnterAlternateScreen, LeaveAlternateScreen},
};
@@
- execute!(stdout, EnterAlternateScreen)?;
+ execute!(stdout, EnterAlternateScreen, EnableMouseCapture)?;
let backend = CrosstermBackend::new(stdout);
let mut terminal = Terminal::new(backend)?;
+ terminal.hide_cursor()?;
+ terminal.clear()?;
@@
- execute!(terminal.backend_mut(), LeaveAlternateScreen, DisableMouseCapture)?;
+ execute!(terminal.backend_mut(), LeaveAlternateScreen, DisableMouseCapture)?;
terminal.show_cursor()?;Also applies to: 21-24, 45-47
🤖 Prompt for AI Agents
In src/ui/new_renderer.rs around lines 4-8 (and similarly adjust lines 21-24 and
45-47), the terminal setup/teardown is asymmetric: exit path disables mouse
capture and shows the cursor but the entry path does not enable mouse capture,
hide the cursor, or clear the screen. Update the entry/initialization sequence
to enable mouse capture, hide the cursor, and clear/enter the alternate screen
(alongside existing enable_raw_mode and EnterAlternateScreen), and ensure the
teardown still symmetrically disables mouse capture, shows the cursor, and
leaves/clears the alternate screen so setup and teardown are consistent.
| /// Enhanced async event loop with proper background task support | ||
| pub async fn run_new_app(sync_service: SyncService) -> anyhow::Result<()> { | ||
| // Setup terminal | ||
| enable_raw_mode()?; | ||
| let mut stdout = io::stdout(); | ||
| execute!(stdout, EnterAlternateScreen)?; | ||
| let backend = CrosstermBackend::new(stdout); | ||
| let mut terminal = Terminal::new(backend)?; | ||
|
|
||
| // Initialize application components | ||
| let mut app = AppComponent::new(sync_service); | ||
| let mut event_handler = EventHandler::new(); | ||
|
|
||
| // Start initial sync automatically | ||
| app.trigger_initial_sync(); | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Guard against early errors leaving the terminal in a bad state.
If a failure occurs after raw mode is enabled but before the explicit teardown, the terminal can be left in raw mode. Consider a small Drop guard that always restores state.
Add this helper (can live in this module):
struct TuiGuard;
impl Drop for TuiGuard {
fn drop(&mut self) {
let _ = crossterm::terminal::disable_raw_mode();
let _ = crossterm::execute!(std::io::stdout(), crossterm::terminal::LeaveAlternateScreen, crossterm::event::DisableMouseCapture);
}
}Then at the top of run_new_app after enabling raw mode and entering the alt screen:
let _tui_guard = TuiGuard; // ensures cleanup on early-return/panicYou can keep the explicit teardown; the guard makes cleanup robust on early errors.
🤖 Prompt for AI Agents
In src/ui/new_renderer.rs around lines 16 to 31, enabling raw mode and entering
the alternate screen can leave the terminal in a broken state if an error occurs
before the explicit teardown; add a small Drop guard type in this module that
disables raw mode, leaves the alternate screen and disables mouse capture in its
Drop implementation, then instantiate it (e.g., let _tui_guard = TuiGuard;)
immediately after enable_raw_mode() and EnterAlternateScreen so the terminal is
always restored on early returns or panics while keeping the existing explicit
teardown at the end.
1 participant
Trying to use the recommended architecture for ratatui apps
Summary by CodeRabbit
New Features
Improvements
Documentation
Chores