Implement an Icon service, will be used later to switch icons sets - #2
Conversation
WalkthroughAdds a new themeable icon service (IconService, IconTheme, IconSet) and integrates it into the app state. Updates UI components to use theme-driven icons and titles instead of hard-coded glyphs. Exposes the icons module at crate and main roots. Implements tree-depth-aware project rendering and dynamic label/project icons. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
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: 0
🔭 Outside diff range comments (2)
src/ui/components/projects_list.rs (2)
45-55: Fix Unicode slicing panic when truncating label names (use char iteration, not byte slicing)The current slice &label.name[..N] can panic at runtime if N splits a multi-byte character. Truncate on char boundaries instead.
Apply this diff to make truncation safe:
- // 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 (avoid slicing at non-char boundaries) + let max_chars = max_name_width.saturating_sub(1) as usize; + let display_name = if label.name.chars().count() > max_chars { + let truncated: String = label.name.chars().take(max_chars).collect(); + format!("{}…", truncated) + } else { + label.name.clone() + };Optional (for better visual alignment): account for the icon’s display width when deciding available space, using unicode-width. I can provide a follow-up patch if you want to adopt that.
139-145: Fix Unicode slicing panic when truncating project names; optionally account for icon widthSame slicing risk as labels: &project.name[..N] can split a multi-byte char and panic. Use char-based truncation.
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 max_chars = available_width.saturating_sub(1) as usize; // keep room for ellipsis + let display_name = if project.name.chars().count() > max_chars { + let truncated: String = project.name.chars().take(max_chars).collect(); + format!("{}…", truncated) + } else { + project.name.clone() + };Optional (for precise layout): subtract the icon’s display width and the trailing space from available_width using unicode_width::UnicodeWidthStr. I can draft that if desired.
🧹 Nitpick comments (8)
src/icons.rs (3)
103-111: Avoid reconstructing IconSet on every getter callEach convenience getter calls icons(), which rebuilds the entire IconSet. It's cheap (stack-only, &'static str fields), but still repeated work in hot UI paths (e.g., per-list-item render). Consider adding a zero-cost accessor that returns a shared static:
Apply this diff to introduce a borrowed accessor while keeping the current API:
pub fn icons(&self) -> IconSet { - match self.current_theme { - IconTheme::Emoji => Self::emoji_icons(), - IconTheme::Unicode => Self::unicode_icons(), - IconTheme::Ascii => Self::ascii_icons(), - } + match self.current_theme { + IconTheme::Emoji => Self::emoji_icons(), + IconTheme::Unicode => Self::unicode_icons(), + IconTheme::Ascii => Self::ascii_icons(), + } } + + /// Borrowed access to the current theme's icon set (no reconstruction) + pub fn icons_ref(&self) -> &'static IconSet { + match self.current_theme { + IconTheme::Emoji => &EMOJI_ICONS, + IconTheme::Unicode => &UNICODE_ICONS, + IconTheme::Ascii => &ASCII_ICONS, + } + }And add the static sets (outside this block) so icons_ref() is O(1):
// Consider placing near the top of the file const EMOJI_ICONS: IconSet = IconSet { task_status: TaskStatusIcons { pending: "🔳", completed: "✅", deleted: "❌" }, ui: UiIcons { tasks_title: "📝", projects_title: "📁", error: "❌", info: "💡", warning: "⚠️", success: "✅" }, priority: PriorityIcons { urgent: "🔴", high: "🟡", medium: "🟢", low: "🔵" }, status: StatusIcons { recurring: "🔄", due_date: "📅", duration: "⏱️", sync_in_progress: "🔄", sync_success: "✅", sync_error: "❌", }, }; const UNICODE_ICONS: IconSet = IconSet { task_status: TaskStatusIcons { pending: "□", completed: "✓", deleted: "✗" }, ui: UiIcons { tasks_title: "▶", projects_title: "◆", error: "✗", info: "ⓘ", warning: "⚠", success: "✓" }, priority: PriorityIcons { urgent: "●", high: "◉", medium: "○", low: "◯" }, status: StatusIcons { recurring: "↻", due_date: "◷", duration: "⧖", sync_in_progress: "⟳", sync_success: "✓", sync_error: "✗", }, }; const ASCII_ICONS: IconSet = IconSet { task_status: TaskStatusIcons { pending: "[ ]", completed: "[X]", deleted: "[D]" }, ui: UiIcons { tasks_title: ">", projects_title: "#", error: "X", info: "i", warning: "!", success: "+" }, priority: PriorityIcons { urgent: "!!", high: "!", medium: "+", low: "-" }, status: StatusIcons { recurring: "~", due_date: "@", duration: "T", sync_in_progress: "...", sync_success: "+", sync_error: "X", }, };
248-271: Unify project/label icons within IconSet to avoid duplicate theme matchingproject_regular, project_favorite, and label perform separate matches over current_theme instead of reusing the themed sets. Consider adding a ProjectIcons and LabelIcons group to IconSet and exposing getters via icons_ref(), removing this duplication.
Happy to draft the structs and migrate these into the IconSet for you if you want to pursue this now.
274-316: Broaden test coverage to UI/status/priority and project/label iconsCurrent tests cover only task status icons. Add assertions for:
- ui: tasks_title, projects_title, error/info/warning/success
- priority: urgent/high/medium/low
- status: recurring/due_date/duration/sync_*
- project_regular/project_favorite/label
This guards against future regressions when tweaking glyphs per theme.
I can add a table-driven test to validate all categories across themes. Want me to push it?
src/main.rs (1)
2-2: Avoid re-declaring modules in both bin and lib targetsYou already expose icons in lib.rs. Declaring the same module again in main.rs duplicates compilation (including unit tests within the module) and is unnecessary. Prefer importing from the library crate in main.
Apply this diff:
-pub mod icons;src/ui/app.rs (1)
45-46: Good: centralize theming via App.icons with a sensible defaultStoring IconService in App and initializing with IconService::default() aligns the UI to a single authoritative theme source.
Consider a follow-up: persist theme choice and add a keybinding to cycle IconTheme (you already have set_theme).
Happy to wire a small toggle handler and persistence stub when ready.
Also applies to: 90-92
src/ui/components/tasks_list.rs (1)
47-52: Micro-optimization: avoid repeated icon lookups inside the render loopEach of task_deleted/task_completed/task_pending reconstructs the icon set today. Cache them once per render and reuse in the iterator.
For example:
// before iter().enumerate() let icon_deleted = app.icons.task_deleted(); let icon_completed = app.icons.task_completed(); let icon_pending = app.icons.task_pending(); let items: Vec<ListItem> = app.tasks.iter().enumerate().map(|(index, task)| { let status_icon = if task.is_deleted { icon_deleted } else if task.is_completed { icon_completed } else { icon_pending }; // ... }).collect();src/ui/components/projects_list.rs (2)
83-107: Reduce comparator allocations and improve sorting efficiencyYou clone Option for every comparison and repeatedly traverse the project list to find roots. Two small improvements:
- Avoid cloning for immediate parent: compare Option<&str> via as_deref().
- Consider precomputing a parent map/root id map if project lists grow to reduce repeated O(n) lookups.
Minimal no-allocation change for immediate parent:
- fn get_immediate_parent_id(project: &ProjectDisplay) -> Option<String> { - project.parent_id.clone() - } + fn get_immediate_parent_id(project: &ProjectDisplay) -> Option<&str> { + project.parent_id.as_deref() + }If you want, I can provide a follow-up refactor that precomputes id -> parent/root/depth maps to make the sort comparator O(1).
157-170: Cycle safety in parent traversalBoth calculate_tree_depth and get_root_project_id assume acyclic parent relationships. If upstream data ever contains a cycle, these loops won’t terminate. Defensive guards (e.g., visited set with a hop limit) would make this robust.
Would you like a small guard (visited HashSet + max_depth) added to both helpers?
📜 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 (6)
src/icons.rs(1 hunks)src/lib.rs(1 hunks)src/main.rs(1 hunks)src/ui/app.rs(3 hunks)src/ui/components/projects_list.rs(5 hunks)src/ui/components/tasks_list.rs(3 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (6)
src/lib.rs (1)
src/icons.rs (1)
icons(104-110)
src/main.rs (1)
src/icons.rs (1)
icons(104-110)
src/ui/components/tasks_list.rs (1)
src/icons.rs (2)
icons(104-110)tasks_title(224-226)
src/ui/app.rs (1)
src/icons.rs (3)
icons(104-110)default(20-22)default(80-82)
src/icons.rs (1)
src/ui/app.rs (2)
default(49-51)new(57-93)
src/ui/components/projects_list.rs (1)
src/icons.rs (3)
icons(104-110)label(265-271)projects_title(228-230)
🔇 Additional comments (10)
src/icons.rs (3)
73-77: Solid, cohesive IconService abstractionClean separation of theme enum, grouped icon sets, and a thin service that exposes convenience getters. Defaulting to Unicode is a sensible choice for terminal compatibility.
6-6: Serde derive feature confirmedCargo.toml already includes:
- Cargo.toml:16 –
serde = { version = "1.0", features = ["derive"] }The
Serialize/Deserializederives on IconTheme are supported. No further changes required.
112-144: Be mindful of variation-selector emoji width/alignment in the TUI
- Detected FE0F variation selectors in:
- “
⚠️ ” (U+26A0 U+FE0F)- “⏱️” (U+23F1 U+FE0F)
- These may render as width=2 or inconsistently across terminals/locales, potentially breaking column alignments
- Recommendations:
- Test layouts in your target TUIs (e.g. Alacritty, iTerm2, Windows Terminal)
- If you see misalignment, swap to single-scalar variants (“⚠” U+26A0, “⏱” U+23F1) or fallback ASCII
- Restrict variable-width emoji to non-tabular contexts (titles, headers)
Applies to
emoji_icons()(lines 112–144) and the equivalent mappings in lines 145–176, 178–210.src/lib.rs (1)
2-2: Expose icons from the crate root – LGTMMaking icons a library module is the right place. Consumers (including the bin) can import from the lib without re-declaring.
src/ui/app.rs (1)
3-3: Correct import – integrates the service into App state cleanlyuse crate::icons::IconService; is the right coupling point for UI.
src/ui/components/tasks_list.rs (1)
31-33: Title now theme-aware – nice touchSwitching to format!("{} Tasks", app.icons.tasks_title()) keeps UX consistent with the selected theme.
Also applies to: 122-124
src/ui/components/projects_list.rs (4)
28-32: Nice: headers now use themed iconsUsing app.icons.label() for the Labels header makes the UI theme-aware and consistent with the icon service.
110-114: Good: projects section header is theme-drivenformat!("{} Projects", app.icons.projects_title()) keeps titles consistent across icon themes.
175-177: Nice: themed, centered titleformat!("{} Projects & Labels", app.icons.projects_title()) aligns the title with the active icon theme.
120-124: ✔ IconService methods verifiedBoth
project_favorite()andproject_regular()are publicly defined insrc/icons.rs(lines 249 & 257) and are correctly invoked insrc/ui/components/projects_list.rs(lines 121–123).
1 participant
Summary by CodeRabbit
New Features
Style