Introduce labels, rework sidebar to accomodate those - #1
Conversation
WalkthroughAdds local-label accessors to SyncService and refactors the UI to a combined Sidebar (labels + projects) selection model. App state and navigation were updated for sidebar selection; sidebar component renamed and reworked to render labels and projects. Events and renderer updated; tasks list empty-state messaging adjusted. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ 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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🔭 Outside diff range comments (3)
src/ui/components/projects_list.rs (2)
61-105: Replace ad-hoc sorting with App::get_sorted_projects() and fix tree ordering duplication.You’re re-implementing project sorting here. This duplicates App::get_sorted_projects(), and the current approach (root/parent grouping + sibling sort) does not guarantee parents appear immediately before their children. Use the centralized sorter and make it produce a proper pre-order traversal (see suggested refactor in src/ui/app.rs).
Apply this diff to de-duplicate:
- // Sort projects: favorites first within their own hierarchical level - let mut sorted_projects: Vec<_> = app.projects.iter().enumerate().collect(); - - // Helper function to get the root project ID (top-level parent) - fn get_root_project_id(project: &ProjectDisplay, projects: &[ProjectDisplay]) -> String { - let mut current = project; - while let Some(parent_id) = ¤t.parent_id { - if let Some(parent) = projects.iter().find(|p| p.id == *parent_id) { - current = parent; - } else { - break; - } - } - current.id.clone() - } - - // Helper function to get the immediate parent ID - fn get_immediate_parent_id(project: &ProjectDisplay) -> Option<String> { - project.parent_id.clone() - } - - sorted_projects.sort_by(|(_a_idx, a_project), (_b_idx, b_project)| { - // First, sort by root project to keep tree structures together - let a_root = get_root_project_id(a_project, &app.projects); - let b_root = get_root_project_id(b_project, &app.projects); - let root_cmp = a_root.cmp(&b_root); - if root_cmp != std::cmp::Ordering::Equal { - return root_cmp; - } - - // Same root, now sort by immediate parent to keep siblings together - let a_parent = get_immediate_parent_id(a_project); - let b_parent = get_immediate_parent_id(b_project); - let parent_cmp = a_parent.cmp(&b_parent); - if parent_cmp != std::cmp::Ordering::Equal { - return parent_cmp; - } - - // Same immediate parent (siblings), sort favorites first, then by name - match (a_project.is_favorite, b_project.is_favorite) { - (true, false) => std::cmp::Ordering::Less, // a (favorite) comes before b (non-favorite) - (false, true) => std::cmp::Ordering::Greater, // a (non-favorite) comes after b (favorite) - _ => a_project.name.cmp(&b_project.name), // Same favorite status, sort by name - } - }); + // Use App's centralized, hierarchical pre-order sort + let sorted_projects = app.get_sorted_projects();Companion change: update App::get_sorted_projects() to return a true parent-then-children sequence (see app.rs comment).
135-140: Apply Unicode-safe truncation for project names and account for indentation.Same issue as labels: byte-based slicing can panic and miscount width. Use the same helper and pass in the already width-adjusted available_width.
Apply this diff:
- let display_name = if project.name.len() > available_width as usize { - format!("{}…", &project.name[..available_width.saturating_sub(1) as usize]) - } else { - project.name.clone() - }; + let display_name = truncate_str(&project.name, available_width);src/ui/app.rs (1)
190-238: Produce a true hierarchical (pre-order) project list: parent immediately followed by its children.Sorting by root and immediate parent does not guarantee that children appear directly under their parent (and deeper nesting breaks ordering). Implement a pre-order traversal grouped by parent_id, with favorites-first/name-next sibling ordering.
Apply this diff:
- pub fn get_sorted_projects(&self) -> Vec<(usize, &ProjectDisplay)> { - let mut sorted_projects: Vec<_> = self.projects.iter().enumerate().collect(); - - // Helper function to get the root project ID (top-level parent) - fn get_root_project_id(project: &ProjectDisplay, projects: &[ProjectDisplay]) -> String { - let mut current = project; - while let Some(parent_id) = ¤t.parent_id { - if let Some(parent) = projects.iter().find(|p| p.id == *parent_id) { - current = parent; - } else { - break; - } - } - current.id.clone() - } - - // Helper function to get the immediate parent ID - fn get_immediate_parent_id(project: &ProjectDisplay) -> Option<String> { - project.parent_id.clone() - } - - sorted_projects.sort_by(|(_a_idx, a_project), (_b_idx, b_project)| { - // First, sort by root project to keep tree structures together - let a_root = get_root_project_id(a_project, &self.projects); - let b_root = get_root_project_id(b_project, &self.projects); - let root_cmp = a_root.cmp(&b_root); - if root_cmp != std::cmp::Ordering::Equal { - return root_cmp; - } - - // Same root, now sort by immediate parent to keep siblings together - let a_parent = get_immediate_parent_id(a_project); - let b_parent = get_immediate_parent_id(b_project); - let parent_cmp = a_parent.cmp(&b_parent); - if parent_cmp != std::cmp::Ordering::Equal { - return parent_cmp; - } - - // Same immediate parent (siblings), sort favorites first, then by name - match (a_project.is_favorite, b_project.is_favorite) { - (true, false) => std::cmp::Ordering::Less, // a (favorite) comes before b (non-favorite) - (false, true) => std::cmp::Ordering::Greater, // a (non-favorite) comes after b (favorite) - _ => a_project.name.cmp(&b_project.name), // Same favorite status, sort by name - } - }); - sorted_projects - } + pub fn get_sorted_projects(&self) -> Vec<(usize, &ProjectDisplay)> { + use std::collections::{HashMap, HashSet}; + + // Build parent -> children index map + let mut by_parent: HashMap<Option<String>, Vec<usize>> = HashMap::new(); + for (idx, p) in self.projects.iter().enumerate() { + by_parent.entry(p.parent_id.clone()).or_default().push(idx); + } + + // Sibling comparator: favorites first, then by name + let cmp_idx = |a: &usize, b: &usize| { + let pa = &self.projects[*a]; + let pb = &self.projects[*b]; + match (pa.is_favorite, pb.is_favorite) { + (true, false) => std::cmp::Ordering::Less, + (false, true) => std::cmp::Ordering::Greater, + _ => pa.name.cmp(&pb.name), + } + }; + + // Sort all children lists deterministically + for v in by_parent.values_mut() { + v.sort_by(cmp_idx); + } + + // DFS from roots (parent_id == None) + fn dfs( + app: &App, + idx: usize, + by_parent: &HashMap<Option<String>, Vec<usize>>, + visited: &mut HashSet<usize>, + out: &mut Vec<(usize, &ProjectDisplay)>, + ) { + if !visited.insert(idx) { + return; // break potential cycles defensively + } + out.push((idx, &app.projects[idx])); + if let Some(children) = by_parent.get(&Some(app.projects[idx].id.clone())) { + for child_idx in children { + dfs(app, *child_idx, by_parent, visited, out); + } + } + } + + let mut result = Vec::new(); + let mut visited = HashSet::new(); + let mut roots = by_parent.get(&None).cloned().unwrap_or_default(); + roots.sort_by(cmp_idx); + for idx in roots { + dfs(self, idx, &by_parent, &mut visited, &mut result); + } + // Include any orphans that reference a missing parent + for (idx, _) in self.projects.iter().enumerate() { + if !visited.contains(&idx) { + result.push((idx, &self.projects[idx])); + } + } + result + }
🧹 Nitpick comments (9)
src/sync.rs (2)
51-64: Minimize mutex hold time when mapping labelsYou’re holding the storage mutex across both the async call and the subsequent CPU-bound mapping. Scope the guard to release it before mapping to improve concurrency.
Apply this diff:
- pub async fn get_labels(&self) -> Result<Vec<LabelDisplay>> { - let storage = self.storage.lock().await; - let local_labels = storage.get_all_labels().await?; - - // Convert LocalLabel to LabelDisplay - let labels = local_labels.into_iter().map(|local| LabelDisplay { - id: local.id, - name: local.name, - color: local.color, - }).collect(); - - Ok(labels) - } + pub async fn get_labels(&self) -> Result<Vec<LabelDisplay>> { + // Hold the lock only for the async DB call + let local_labels = { + let storage = self.storage.lock().await; + storage.get_all_labels().await? + }; + + // Convert LocalLabel to LabelDisplay outside the lock + let labels = local_labels + .into_iter() + .map(|local| LabelDisplay { + id: local.id, + name: local.name, + color: local.color, + }) + .collect(); + + Ok(labels) + }
66-77: Filtering by label name risks collisions; prefer id-based filtering and drop the lock sooner
- Name-based filtering can break if names collide/rename. If feasible, pass/use the label id for filtering.
- Similarly to get_labels, release the storage mutex before CPU-bound filtering.
Minimal improvement (reduce lock scope):
- pub async fn get_tasks_with_label(&self, label_name: &str) -> Result<Vec<TaskDisplay>> { - let storage = self.storage.lock().await; - let all_tasks = storage.get_all_tasks().await?; - - // Filter tasks that have the specified label - let filtered_tasks = all_tasks.into_iter() - .filter(|task| task.labels.iter().any(|label| label.name == label_name)) - .collect(); - - Ok(filtered_tasks) - } + pub async fn get_tasks_with_label(&self, label_name: &str) -> Result<Vec<TaskDisplay>> { + // Hold the lock only for the async read + let all_tasks = { + let storage = self.storage.lock().await; + storage.get_all_tasks().await? + }; + + // Filter tasks that have the specified label + let filtered_tasks = all_tasks + .into_iter() + .filter(|task| task.labels.iter().any(|label| label.name == label_name)) + .collect(); + + Ok(filtered_tasks) + }If you want, I can:
- add an id-based variant
get_tasks_with_label_id(&self, label_id: &str)and wire the UI to pass ids; or- resolve
label_name -> idvia storage before filtering (handling missing labels gracefully).src/ui/components/tasks_list.rs (2)
20-27: Empty-state message should accommodate label selectionThe message says “No tasks in this project,” but the active selection can be a label. Consider a neutral copy (“selection”) or branching on the selection type for better UX.
Minimal neutral copy:
- "No tasks in this project. Press 'a' to create a task." + "No tasks in this selection. Press 'a' to create a task."Alternatively, branch on app.sidebar_selection if accessible in this scope to show a label-specific message.
28-37: Empty-state rendering via List is fine; mind the cloned ListStateRendering a single-item List for the empty state is consistent. Note that passing a cloned ListState means any internal widget state mutations won’t persist (usually fine for List). If you ever rely on widget-updated state, this would need revisiting.
Please confirm that no widget-driven state changes are expected during render; otherwise, we’ll want a render signature that can borrow App mutably or a separate state for the empty view.
src/ui/components/projects_list.rs (2)
175-178: Sidebar won’t scroll without ListState; selected item can move off-screen.Rendering without a stateful List forfeits built-in scrolling. With many labels/projects, manual highlighting alone won’t keep the selection visible.
Consider re-introducing a dedicated ListState for the sidebar (e.g., app.sidebar_list_state) and render_stateful_widget with a no-op highlight symbol/style, while still applying your custom selected row styling. Alternatively, keep a manual scroll offset and slice all_items to the visible window.
49-53: Minor: label width should subtract the added two-space indent.You prepend " 🏷️ " to label rows but only subtracted icon/space in the layout helper. The truncation should account for the extra two-space indent to avoid overflow. The Unicode-safe fix above does this with max_name_width.saturating_sub(2).
src/ui/app.rs (3)
110-147: Navigation re-sorts on every keypress; consider caching sorted projects.next_sidebar_item/previous_sidebar_item compute get_sorted_projects() on each call. For large lists, this is O(n log n) per keypress.
Cache the hierarchical order (Vec<(usize, &ProjectDisplay)>) alongside a revision counter and recompute only when self.projects changes (e.g., after sync). Alternatively, store a stable “order index” per project once and reuse for navigation.
Also applies to: 150-186
295-297: Dead comment: “Update the list state to match the selection”.This TODO is unimplemented and stale. Either wire a sidebar ListState (for scrolling) or remove the comment to avoid confusion.
579-585: Clarify task creation UX under label selection.Current behavior prevents creating a task when a label is selected (error message is good). Consider pre-selecting the last-used project as a fallback, or prompting the user to pick a project inline. This reduces friction when browsing by label.
Happy to sketch a small project picker flow if you want to keep it entirely keyboard-driven.
Also applies to: 618-651
📜 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 selected for processing (7)
src/sync.rs(2 hunks)src/ui/app.rs(12 hunks)src/ui/components/mod.rs(1 hunks)src/ui/components/projects_list.rs(5 hunks)src/ui/components/tasks_list.rs(2 hunks)src/ui/events.rs(1 hunks)src/ui/renderer.rs(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (4)
src/ui/renderer.rs (1)
src/ui/components/projects_list.rs (1)
render(19-178)
src/ui/components/tasks_list.rs (1)
src/ui/app.rs (2)
new(56-90)default(48-50)
src/ui/components/projects_list.rs (4)
src/ui/components/tasks_list.rs (1)
render(19-135)src/ui/layout.rs (1)
sidebar_constraints(97-100)src/ui/app.rs (2)
new(56-90)default(48-50)src/todoist.rs (2)
from(40-48)from(52-86)
src/ui/app.rs (2)
src/sync.rs (1)
new(27-37)src/todoist.rs (2)
from(40-48)from(52-86)
⏰ 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
🔇 Additional comments (6)
src/sync.rs (1)
7-7: Importing LabelDisplay into SyncService is appropriateThis aligns the service API with the UI’s new label-aware flow. No issues spotted.
src/ui/components/mod.rs (1)
13-13: Re-export rename to Sidebar is consistent with the new architecturePublic surface now clearly reflects the unified left-nav. Looks good.
src/ui/renderer.rs (2)
15-16: Imports updated to Sidebar: consistent and correctThe import list matches the re-export and the Sidebar rename.
91-93: Left pane switched to Sidebar::render — correct integrationThis matches the new combined labels/projects sidebar. No further changes required here.
src/ui/events.rs (1)
224-236: Switched navigation to previous/next_sidebar_item with on-demand task loadGood alignment with the new sidebar model; errors are surfaced to the UI via app.error_message.
Quick check: ensure App::previous_sidebar_item/next_sidebar_item skip non-selectable rows (headers/separators) and clamp at bounds to avoid spurious task reloads.
src/ui/components/projects_list.rs (1)
20-21: sidebar_constraints path is correct – no changes requiredVerified that
sidebar_constraintsis defined as an associated function withinimpl LayoutManagerinsrc/ui/layout.rs. Calling it viaLayoutManager::sidebar_constraints(area.width)is accurate. No import or call-site adjustments needed.Likely an incorrect or invalid review comment.
| fn sort_tasks(&self, mut tasks: Vec<TaskDisplay>) -> Vec<TaskDisplay> { | ||
| tasks.sort_by(|a, b| { | ||
| // Create priority scores: pending=0, completed=1, deleted=2 | ||
| let a_score = if a.is_deleted { 2 } else { i32::from(a.is_completed) }; | ||
| let b_score = if b.is_deleted { 2 } else { i32::from(b.is_completed) }; | ||
|
|
||
| pub fn previous_project(&mut self) { | ||
| if !self.projects.is_empty() { | ||
| let sorted_projects = self.get_sorted_projects(); | ||
| let current_display_index = self.get_selected_project_display_index().unwrap_or(0); | ||
| let prev_display_index = if current_display_index == 0 { | ||
| sorted_projects.len() - 1 | ||
| } else { | ||
| current_display_index - 1 | ||
| }; | ||
| self.select_project_by_display_index(prev_display_index); | ||
| } | ||
| // Sort by score (lower score = higher priority) | ||
| a_score.cmp(&b_score) | ||
| }); |
There was a problem hiding this comment.
Compilation bug: i32::from(bool) is not implemented.
Rust implements From for u8, not i32. This will not compile.
Apply this diff:
- tasks.sort_by(|a, b| {
- // Create priority scores: pending=0, completed=1, deleted=2
- let a_score = if a.is_deleted { 2 } else { i32::from(a.is_completed) };
- let b_score = if b.is_deleted { 2 } else { i32::from(b.is_completed) };
-
- // Sort by score (lower score = higher priority)
- a_score.cmp(&b_score)
- });
+ tasks.sort_by(|a, b| {
+ // Create priority scores: pending=0, completed=1, deleted=2
+ let a_score: u8 = if a.is_deleted { 2 } else if a.is_completed { 1 } else { 0 };
+ let b_score: u8 = if b.is_deleted { 2 } else if b.is_completed { 1 } else { 0 };
+ a_score.cmp(&b_score)
+ });📝 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.
| fn sort_tasks(&self, mut tasks: Vec<TaskDisplay>) -> Vec<TaskDisplay> { | |
| tasks.sort_by(|a, b| { | |
| // Create priority scores: pending=0, completed=1, deleted=2 | |
| let a_score = if a.is_deleted { 2 } else { i32::from(a.is_completed) }; | |
| let b_score = if b.is_deleted { 2 } else { i32::from(b.is_completed) }; | |
| pub fn previous_project(&mut self) { | |
| if !self.projects.is_empty() { | |
| let sorted_projects = self.get_sorted_projects(); | |
| let current_display_index = self.get_selected_project_display_index().unwrap_or(0); | |
| let prev_display_index = if current_display_index == 0 { | |
| sorted_projects.len() - 1 | |
| } else { | |
| current_display_index - 1 | |
| }; | |
| self.select_project_by_display_index(prev_display_index); | |
| } | |
| // Sort by score (lower score = higher priority) | |
| a_score.cmp(&b_score) | |
| }); | |
| fn sort_tasks(&self, mut tasks: Vec<TaskDisplay>) -> Vec<TaskDisplay> { | |
| tasks.sort_by(|a, b| { | |
| // Create priority scores: pending=0, completed=1, deleted=2 | |
| let a_score: u8 = if a.is_deleted { 2 } else if a.is_completed { 1 } else { 0 }; | |
| let b_score: u8 = if b.is_deleted { 2 } else if b.is_completed { 1 } else { 0 }; | |
| a_score.cmp(&b_score) | |
| }); |
🤖 Prompt for AI Agents
In src/ui/app.rs around lines 349 to 357, the code uses i32::from(bool) which
does not exist (From<bool> is implemented for u8, not i32); replace those
expressions by explicit integer values: set a_score and b_score to 2 if
is_deleted, else 1 if is_completed, else 0 (e.g. a_score = if a.is_deleted { 2 }
else if a.is_completed { 1 } else { 0 }), and do the same for b_score so the
sort_by compiles and preserves the intended priority ordering.
| let display_name = if label.name.len() > max_name_width as usize { | ||
| format!("{}…", &label.name[..max_name_width.saturating_sub(1) as usize]) | ||
| } else { | ||
| label.name.clone() | ||
| }; |
There was a problem hiding this comment.
Prevent Unicode slicing panics; use a Unicode-aware truncation helper.
Slicing UTF-8 strings by byte length can panic at runtime (non–char boundary) and mis-measure display width for wide glyphs (e.g., emojis, CJK). Replace byte-based slicing with a Unicode-aware truncation that respects terminal column width.
Apply this diff:
- let display_name = if label.name.len() > max_name_width as usize {
- format!("{}…", &label.name[..max_name_width.saturating_sub(1) as usize])
- } else {
- label.name.clone()
- };
+ // Account for the two-space indent before the icon
+ let display_name = truncate_str(&label.name, max_name_width.saturating_sub(2));Add this helper (and dependency) outside the selected range:
// Add to Cargo.toml:
// unicode-width = "0.1"
use unicode_width::{UnicodeWidthChar, UnicodeWidthStr};
fn truncate_str(text: &str, max_width: u16) -> String {
if UnicodeWidthStr::width(text) as u16 <= max_width {
return text.to_string();
}
let mut out = String::new();
let mut used = 0u16;
for ch in text.chars() {
let w = UnicodeWidthChar::width(ch).unwrap_or(0) as u16;
if used + w >= max_width.saturating_sub(1) {
break;
}
out.push(ch);
used += w;
}
out.push('…');
out
}🤖 Prompt for AI Agents
In src/ui/components/projects_list.rs around lines 43 to 47, the current code
slices label.name by byte index which can panic on UTF-8 boundaries and
mis-measure terminal width; add the unicode-width crate to Cargo.toml
(unicode-width = "0.1"), add a Unicode-aware helper fn truncate_str(text: &str,
max_width: u16) that iterates chars, sums display widths using
UnicodeWidthChar::width and UnicodeWidthStr::width, appends a trailing ellipsis
when truncating, and replace the byte-slice branch with display_name =
truncate_str(&label.name, max_name_width).
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
src/ui/components/projects_list.rs (1)
135-141: Prevent UTF-8 slicing panics for project names; use Unicode-aware truncationSame byte-slicing issue as labels; this can panic and mis-count width with wide glyphs. Use the same truncate_str helper.
Apply this diff:
- // Truncate project name to fit sidebar (accounting for indentation) - let available_width = max_name_width.saturating_sub(indent.len() as u16); - let display_name = if project.name.len() > available_width as usize { - format!("{}…", &project.name[..available_width.saturating_sub(1) as usize]) - } else { - project.name.clone() - }; + // Truncate project name to fit sidebar (accounting for indentation) + let available_width = max_name_width.saturating_sub(indent.len() as u16); + let display_name = truncate_str(&project.name, available_width);
♻️ Duplicate comments (3)
src/ui/components/projects_list.rs (1)
45-51: Fix UTF-8 slicing panic and width miscalculation for label names
&label.name[..N]slices by bytes and can panic on non–char boundaries; it also ignores terminal column widths (emoji/CJK), and the two-space indent before the icon isn't subtracted from available width.Apply this diff:
- // Truncate label name to fit sidebar - let display_name = if label.name.len() > max_name_width as usize { - format!("{}…", &label.name[..max_name_width.saturating_sub(1) as usize]) - } else { - label.name.clone() - }; + // Truncate label name to fit sidebar (account for 2-space indent before the icon) + let display_name = truncate_str(&label.name, max_name_width.saturating_sub(2));Add this helper (and dependency) outside the selected range:
// Cargo.toml: // unicode-width = "0.1" use unicode_width::{UnicodeWidthChar, UnicodeWidthStr}; fn truncate_str(text: &str, max_width: u16) -> String { if UnicodeWidthStr::width(text) as u16 <= max_width { return text.to_string(); } let mut out = String::new(); let mut used = 0u16; for ch in text.chars() { let w = UnicodeWidthChar::width(ch).unwrap_or(0) as u16; if used + w >= max_width.saturating_sub(1) { break; } out.push(ch); used += w; } out.push('…'); out }src/ui/app.rs (2)
244-255: Restore sidebar selection by stable IDs (robust to reordering and data refresh)Persisting selection by indices will select the wrong entity after any reorder/insert/delete. Store the selected item’s stable ID and re-map post-load.
Apply these diffs:
- // Remember the current selection - let current_selection = self.sidebar_selection.clone(); + // Remember the current selection by stable ID (label/project) + let current_selection_id: Option<(bool, String)> = match &self.sidebar_selection { + SidebarSelection::Label(i) => self.labels.get(*i).map(|l| (true, l.id.clone())), + SidebarSelection::Project(i) => self.projects.get(*i).map(|p| (false, p.id.clone())), + };- // Try to restore the previous selection or set a sensible default - let mut selection_restored = false; - - match current_selection { - SidebarSelection::Label(index) => { - if index < self.labels.len() { - self.sidebar_selection = SidebarSelection::Label(index); - selection_restored = true; - } - } - SidebarSelection::Project(index) => { - if index < self.projects.len() { - self.sidebar_selection = SidebarSelection::Project(index); - selection_restored = true; - } - } - } + // Try to restore the previous selection by ID or set a sensible default + let mut selection_restored = false; + if let Some((is_label, id)) = current_selection_id { + if is_label { + if let Some(i) = self.labels.iter().position(|l| l.id == id) { + self.sidebar_selection = SidebarSelection::Label(i); + selection_restored = true; + } + } else if let Some(i) = self.projects.iter().position(|p| p.id == id) { + self.sidebar_selection = SidebarSelection::Project(i); + selection_restored = true; + } + }Also applies to: 262-293
345-354: Compilation bug: i32::from(bool) is not implemented; use explicit scoresThis won’t compile in Rust; From exists for u8, not i32. Also, keep both sides the same type.
Apply this diff:
- tasks.sort_by(|a, b| { - // Create priority scores: pending=0, completed=1, deleted=2 - let a_score = if a.is_deleted { 2 } else { i32::from(a.is_completed) }; - let b_score = if b.is_deleted { 2 } else { i32::from(b.is_completed) }; - - // Sort by score (lower score = higher priority) - a_score.cmp(&b_score) - }); + tasks.sort_by(|a, b| { + // Create priority scores: pending=0, completed=1, deleted=2 + let a_score: u8 = if a.is_deleted { 2 } else if a.is_completed { 1 } else { 0 }; + let b_score: u8 = if b.is_deleted { 2 } else if b.is_completed { 1 } else { 0 }; + // Sort by score (lower score = higher priority) + a_score.cmp(&b_score) + });
🧹 Nitpick comments (2)
src/ui/components/projects_list.rs (1)
62-107: DRY the project sorting; reuse App::get_sorted_projects to keep selection/navigation consistentThe sort logic and helpers duplicate App::get_sorted_projects and risk divergence. Use the shared method here and drop the local helpers. Keep calculate_tree_depth if you want to retain local indentation logic.
Apply this diff:
- // Sort projects: favorites first within their own hierarchical level - let mut sorted_projects: Vec<_> = app.projects.iter().enumerate().collect(); - - // Helper function to get the root project ID (top-level parent) - fn get_root_project_id(project: &ProjectDisplay, projects: &[ProjectDisplay]) -> String { - let mut current = project; - while let Some(parent_id) = ¤t.parent_id { - if let Some(parent) = projects.iter().find(|p| p.id == *parent_id) { - current = parent; - } else { - break; - } - } - current.id.clone() - } - - // Helper function to get the immediate parent ID - fn get_immediate_parent_id(project: &ProjectDisplay) -> Option<String> { - project.parent_id.clone() - } - - sorted_projects.sort_by(|(_a_idx, a_project), (_b_idx, b_project)| { - // First, sort by root project to keep tree structures together - let a_root = get_root_project_id(a_project, &app.projects); - let b_root = get_root_project_id(b_project, &app.projects); - let root_cmp = a_root.cmp(&b_root); - if root_cmp != std::cmp::Ordering::Equal { - return root_cmp; - } - - // Same root, now sort by immediate parent to keep siblings together - let a_parent = get_immediate_parent_id(a_project); - let b_parent = get_immediate_parent_id(b_project); - let parent_cmp = a_parent.cmp(&b_parent); - if parent_cmp != std::cmp::Ordering::Equal { - return parent_cmp; - } - - // Same immediate parent (siblings), sort favorites first, then by name - match (a_project.is_favorite, b_project.is_favorite) { - (true, false) => std::cmp::Ordering::Less, // a (favorite) comes before b (non-favorite) - (false, true) => std::cmp::Ordering::Greater, // a (non-favorite) comes after b (favorite) - _ => a_project.name.cmp(&b_project.name), // Same favorite status, sort by name - } - }); + // Use shared sort to ensure consistency with navigation logic + let sorted_projects = app.get_sorted_projects();Also applies to: 153-166
src/ui/app.rs (1)
293-299: Remove stale TODO/commentThis comment no longer applies after moving to selection-driven loading.
Apply this diff:
- // Update the list state to match the selection - - // Load tasks for the selected item + // Load tasks for the selected item
📜 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 selected for processing (5)
src/sync.rs(2 hunks)src/ui/app.rs(12 hunks)src/ui/components/projects_list.rs(5 hunks)src/ui/components/tasks_list.rs(2 hunks)src/ui/renderer.rs(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- src/ui/components/tasks_list.rs
- src/sync.rs
- src/ui/renderer.rs
🧰 Additional context used
🧬 Code Graph Analysis (2)
src/ui/app.rs (3)
src/sync.rs (1)
new(27-37)src/storage.rs (6)
new(162-170)from(53-61)from(65-94)from(98-109)from(113-138)from(142-151)src/todoist.rs (2)
from(40-48)from(52-86)
src/ui/components/projects_list.rs (4)
src/ui/components/tasks_list.rs (1)
render(19-134)src/ui/layout.rs (1)
sidebar_constraints(97-100)src/ui/app.rs (2)
new(54-88)default(46-48)src/todoist.rs (2)
from(40-48)from(52-86)
⏰ 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). (2)
- GitHub Check: Test (beta)
- GitHub Check: Security Audit
🔇 Additional comments (2)
src/ui/components/projects_list.rs (1)
108-115: Nice UX touches on headers and framingThe section headers and combined title read cleanly; color choices and bold help scanning.
Also applies to: 168-173
src/ui/app.rs (1)
309-343: Task loader flow looks solidClear separation for label vs project, resets selection and updates state appropriately; error propagation is handled cleanly.
1 participant
this is really a WIP, there are a few glitches here and there
Summary by CodeRabbit
New Features
Enhancements
Refactor