Icons/set ascii as default - #3
Conversation
WalkthroughChanged default icon theme to Ascii and added must_use annotations and multi-theme icon mappings in icons; adjusted projects list UI spacing, indentation and truncation; small refactors and option/map style updates across storage, sync, todoist, UI app, and dialog rendering conversions. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
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: 2
🔭 Outside diff range comments (1)
src/icons.rs (1)
277-280: Updatetest_default_themeto expect the new Ascii defaultThe
IconService::default()implementation now usesIconTheme::Ascii, so the existing test insrc/icons.rswill fail. Please update the assertion accordingly:• File:
src/icons.rs#[test] fn test_default_theme() { let service = IconService::default(); - assert_eq!(service.theme(), IconTheme::Unicode); + assert_eq!(service.theme(), IconTheme::Ascii); }No other tests reference
Unicodeas the default theme.
🧹 Nitpick comments (3)
src/ui/components/projects_list.rs (3)
27-33: Remove commented-out “Labels” header codeLeaving commented-out code adds noise. Prefer deleting it (version control preserves history).
Apply this diff:
- // all_items.push(ListItem::new(Line::from(vec![Span::styled( - // format!("{} Labels", app.icons.label()), - // Style::default() - // .fg(Color::Cyan) - // .add_modifier(Modifier::BOLD), - // )])));
108-115: Remove commented-out “Projects” header codeSame rationale as the labels header: delete rather than comment out.
Apply this diff:
- // all_items.push(ListItem::new(Line::from(vec![Span::styled( - // format!("{} Projects", app.icons.projects_title()), - // Style::default() - // .fg(Color::Cyan) - // .add_modifier(Modifier::BOLD), - // )])));
46-50: Optional: account for icon+space when truncating namesNow that we add a space after the icon, consider reducing the available width by 1 when truncating names so lines don’t slightly overflow on tight layouts. This applies to both labels and projects (projects already subtract indent; subtract one more for the icon’s trailing space).
Also applies to: 140-145
📜 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 (2)
src/icons.rs(2 hunks)src/ui/components/projects_list.rs(4 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/ui/components/projects_list.rs (1)
src/icons.rs (2)
icons(102-108)label(263-269)
🔇 Additional comments (2)
src/icons.rs (2)
21-21: Default IconTheme switched to ASCII — LGTMChange aligns with the PR objective to prefer ASCII for maximum compatibility. No API breakage.
88-88: Constructor formatting simplification — LGTMPurely stylistic; no behavioral change.
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (2)
src/ui/components/projects_list.rs (2)
46-49: UTF-8 unsafe truncation can panic; use char-based truncationUsing String::len() and a byte slice like &s[..n] can split a multibyte character and panic at runtime. Switch to char-based truncation to be safe. (Optional: use display width-aware truncation; see note below.)
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() - }; + let max = max_name_width.saturating_sub(1) as usize; // reserve 1 for ellipsis + let display_name = if label.name.chars().count() > max { + let truncated: String = label.name.chars().take(max).collect(); + format!("{}…", truncated) + } else { + label.name.clone() + };Optional improvement (display width-aware truncation for emoji/unicode): use unicode-width and unicode-segmentation to truncate by terminal cell width instead of char count. I can provide a helper if you want to go this route.
139-145: UTF-8 unsafe truncation (possible panic) and width doesn’t account for icon + space
- Slicing by bytes can panic on multibyte chars.
- available_width ignores the icon and a separating space, so names may overflow.
Make truncation char-safe and reserve room for the icon and a space.
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 + icon + space) + let available_width = max_name_width + .saturating_sub(indent.len() as u16) + .saturating_sub(2); // 1 for icon (ASCII default) + 1 for separating space + let avail = available_width.saturating_sub(1) as usize; // reserve 1 for ellipsis + let display_name = if project.name.chars().count() > avail { + let truncated: String = project.name.chars().take(avail).collect(); + format!("{}…", truncated) + } else { + project.name.clone() + };Optional upgrade: compute the actual icon display width and truncate by unicode display width (unicode-width) to support Emoji/Unicode themes perfectly. Happy to wire that up if you want.
♻️ Duplicate comments (2)
src/ui/components/projects_list.rs (2)
53-55: Icon and label text are concatenated (“@inbox”) — insert a space after the iconThis was previously flagged and is still present. Add a single trailing space to the icon for readability.
Apply this diff:
- Span::styled(app.icons.label().to_string(), style), + Span::styled(format!("{} ", app.icons.label()), style),
149-151: Icon and project name are concatenated (“#Project”) — add a space after the iconEnsure there’s one separator for readability.
Apply this diff:
- Span::styled(icon.to_string(), style), + Span::styled(format!("{} ", icon), style),
🧹 Nitpick comments (8)
src/sync.rs (1)
106-106: Stylistic nit:map(ToString::to_string)is fine; considermap(str::to_owned)ormap(String::from)Functionally equivalent, just a touch more idiomatic/readable in many codebases:
parent_id: parent_id.map(str::to_owned)project_id: project_id.map(str::to_owned)Also applies to: 122-122
src/ui/components/dialogs/project_creation_dialog.rs (1)
33-41: Theme consistency: avoid hardcoded emoji in dialog titleWith ASCII as default, consider replacing the hardcoded "📁 New Project" title with the theme-aware icon service (e.g.,
app.icons.projects_title()), so the dialog title reflects the selected theme.Outside the selected lines, update the title like:
.title(format!("{} New Project", app.icons.projects_title()))src/icons.rs (3)
87-90:#[must_use]on constructors/getters: acceptable; watch for warning noise
- Marking
new,theme,icons, and convenience getters as#[must_use]is defensible to catch ignored-return mistakes.- Minor caution: this can surface warnings if any call sites intentionally ignore a value (rare for pure getters).
Optional micro-optimization: since
IconSetholds only&'static strs, you could cache per-themeIconSetasconst/staticand return a&'static IconSetto avoid reconstructing on each call (not critical given the small size).Also applies to: 93-96, 104-111, 213-256
260-266: New theme-aware project/label icons: add tests to lock behaviorGood addition. Consider adding unit tests that assert
project_regular(),project_favorite(), andlabel()values for Emoji/Unicode/Ascii, similar to the existing task_status tests, to prevent regressions.If you want, I can draft the test cases mirroring the existing style.
Also applies to: 268-275, 277-284
292-295: Updated default theme test: good—consider broader coverageThis assertion matches the new default. You might extend tests to cover:
projects_title()across themesproject_regular(),project_favorite(),label()across themesThis will align tests with the newly added mappings.
src/ui/components/projects_list.rs (3)
27-32: Remove commented-out “Labels” header code to avoid driftStale commented code tends to rot. Either delete it or gate it behind a config/feature flag.
109-114: Remove commented-out “Projects” header codeSame rationale as the Labels header: delete or gate behind a feature/setting.
83-106: Comparator does repeated linear parent lookups; consider precomputing an indexget_root_project_id and the .find() calls inside sort_by make sorting roughly O(n log n) comparisons with O(n) scans each, which can be O(n^2 log n). If project counts grow, precompute a HashMap<id, &ProjectDisplay> (or id -> parent_id) to make lookups O(1). Alternatively, build a parent -> children adjacency map and do a pre-order traversal to guarantee “parent before children” without relying on string id ordering.
📜 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 (8)
src/icons.rs(7 hunks)src/storage.rs(2 hunks)src/sync.rs(2 hunks)src/todoist.rs(1 hunks)src/ui/app.rs(3 hunks)src/ui/components/dialogs/project_creation_dialog.rs(1 hunks)src/ui/components/dialogs/task_creation_dialog.rs(1 hunks)src/ui/components/projects_list.rs(4 hunks)
✅ Files skipped from review due to trivial changes (2)
- src/storage.rs
- src/ui/components/dialogs/task_creation_dialog.rs
🧰 Additional context used
🧬 Code Graph Analysis (2)
src/ui/components/projects_list.rs (1)
src/icons.rs (2)
icons(105-111)label(278-284)
src/icons.rs (1)
src/ui/app.rs (1)
new(57-93)
⏰ 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: Security Audit
- GitHub Check: Test (beta)
🔇 Additional comments (5)
src/todoist.rs (1)
80-80: Confirm Rust toolchain ≥ 1.70I didn’t find any pin on the Rust version in your repo (no
rust-toolchain*file norrust-versioninCargo.toml). SinceOption::is_some_andis stable starting in Rust 1.70, please verify or enforce a minimum toolchain:• In Cargo.toml (root):
Under[package], addrust-version = "1.70"• Or add a
rust-toolchain.tomlat the repo root:[toolchain] channel = "1.70.0"This will prevent CI/build failures on older compilers.
src/ui/components/dialogs/project_creation_dialog.rs (1)
33-41: Safer numeric conversions withf32::from: LGTMUsing
f32::fromavoids accidental narrowing viaascasts and reads better. The min-height guards also prevent collapsing to zero-height.src/icons.rs (1)
21-21: Defaulting to ASCII theme: confirm user-facing docs and UX expectationsSwitching the default from Unicode to ASCII improves compatibility. Double-check any README/CHANGELOG and initial UI hints so users understand the new default, and that the app initialization paths (e.g., IconService::default in UI) expect ASCII-first.
src/ui/app.rs (1)
511-511: Match arms changed toOk(()): LGTMTighter pattern matches; consistent with the
Result<(), _>signatures of the called methods. No functional change, readability improved.Also applies to: 558-558, 622-622
src/ui/components/projects_list.rs (1)
120-124: LGTM: favorite vs regular icon selection is clear and idiomaticChoosing the icon via a simple conditional keeps intent obvious.
1 participant
Summary by CodeRabbit
Style
New Features