Implement basic creation/edition/deletion for labels - #7
Conversation
WalkthroughAdds label CRUD support across sync, app state, event handling, and rendering. Introduces three SyncService methods, expands App with label state and operations, adds three dialog components (create/edit/delete confirm), routes events for label modes, and integrates dialogs into the render flow with conditional display. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45–75 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: 1
♻️ Duplicate comments (1)
src/ui/components/dialogs/label_creation_dialog.rs (1)
24-39: Same scaling logic duplication as edit dialogConsolidate the height scaling logic into a shared helper to reduce maintenance surface.
🧹 Nitpick comments (12)
src/ui/renderer.rs (1)
152-155: Prevent multiple label dialogs from stacking; gate them with an else-if chainToday all three label dialogs can render in the same frame if states overlap, causing layering/overdraw. Make them mutually exclusive like a single “label modal” state.
Apply this diff:
- if app.creating_label { - LabelCreationDialog::render(f, app); - } + if app.creating_label { + LabelCreationDialog::render(f, app); + } else if app.editing_label { + LabelEditDialog::render(f, app); + } else if app.delete_label_confirmation.is_some() { + LabelDeleteConfirmationDialog::render(f, app); + } - - if app.editing_label { - LabelEditDialog::render(f, app); - } - - if app.delete_label_confirmation.is_some() { - LabelDeleteConfirmationDialog::render(f, app); - }Also applies to: 168-171, 176-179
src/ui/components/dialogs/label_delete_confirmation_dialog.rs (3)
3-8: Enable wrapping to avoid overflow on small terminalsOther confirmation dialogs use wrapping; add Wrap to keep long content/titles readable.
use ratatui::{ layout::Alignment, style::{Color, Style}, - widgets::{Block, Borders, Clear, Paragraph}, + widgets::{Block, Borders, Clear, Paragraph, Wrap}, Frame, };
19-33: Remove redundant Option check and simplify label name lookupYou already guard on presence of the ID; reuse the bound value and drop the unreachable else.
- if app.delete_label_confirmation.is_some() { + if let Some(label_id) = &app.delete_label_confirmation { let dialog_area = LayoutManager::centered_rect(50, 15, f.area()); f.render_widget(Clear, dialog_area); - // Find the label name for confirmation - let label_name = if let Some(label_id) = &app.delete_label_confirmation { - app.labels - .iter() - .find(|l| l.id == *label_id) - .map(|l| l.name.as_str()) - .unwrap_or("Unknown") - } else { - "Unknown" - }; + // Find the label name for confirmation + let label_name = app + .labels + .iter() + .find(|l| l.id == *label_id) + .map(|l| l.name.as_str()) + .unwrap_or("Unknown");
34-44: Align copy/title with existing confirmation dialogs and wrap the textMatch the tone/UX of task/project delete dialogs (“
⚠️ Confirm … Delete”, explicit irreversible warning, 'n'/Esc mention) and add wrapping.- let confirmation_text = - format!("Delete label '{label_name}'?\n\nPress 'y' to confirm, any other key to cancel"); + let confirmation_text = format!( + "Delete label?\n\n\"{}\"\n\nThis action cannot be undone!\n\nPress 'y' to confirm or 'n'/Esc to cancel", + label_name + ); let paragraph = Paragraph::new(confirmation_text) .block( Block::default() .borders(Borders::ALL) - .title("Confirm Label Deletion") + .title("⚠️ Confirm Label Delete") .title_alignment(Alignment::Center), ) .style(Style::default().fg(Color::Red)) - .alignment(Alignment::Center); + .alignment(Alignment::Center) + .wrap(Wrap { trim: true });src/ui/components/dialogs/label_edit_dialog.rs (1)
24-39: DRY up the scaling math shared with label creation dialogThe content/instructions scaling logic is duplicated across label edit and creation dialogs. Consider extracting a small helper to centralize this.
Example helper (can live near LayoutManager or as a small local fn):
fn scaled_heights(total_content: u16, total_instructions: u16, outer_h: u16, border_pad: u16) -> (u16, u16) { let total = total_content + total_instructions + 2; // spacing let available = outer_h.saturating_sub(border_pad); let scale = if available < total { available as f32 / total as f32 } else { 1.0 }; let content_h = (total_content as f32 * scale).max(3.0) as u16; let instr_h = (total_instructions as f32 * scale).max(2.0) as u16; (content_h, instr_h) }Then:
let (scaled_content_height, scaled_instructions_height) = scaled_heights(6, 3, dialog_area.height, 4);src/sync.rs (3)
144-157: Label creation flow looks solid; minor consistency nit with API types.The API call and arg construction are correct. One nit: this file mixes crate-local re-exported types (CreateProjectArgs) with direct todoist_api types (CreateLabelArgs, CreateTaskArgs). Consider standardizing on one approach for consistency/readability.
159-173: Update-by-name only is fine; consider exposing color/favorite updates when needed.The selective update via UpdateLabelArgs is correct and safe. When you add color/favorite editing in the UI, you can extend this method to accept optional fields, mirroring how project updates are handled.
175-182: Delete semantics rely on follow-up sync; optionally mirror project deletion for snappier UX.This is functionally correct (UI immediately calls a force sync). For parity with delete_project, you could also remove the label from local storage right away (if/when storage exposes delete_label), making the UI reactive even if the sync is delayed.
src/ui/events.rs (1)
415-441: Creation handler matches task/project input patterns; optional i18n note.Using is_ascii_graphic() || c == ' ' mirrors task/project editing. If you plan to support non-ASCII names, consider relaxing this predicate in a follow-up.
src/ui/app.rs (3)
867-904: Creation flow is good; optional: clear success message after a short delay for consistency.The pattern mirrors task/project creation, except it doesn’t auto-clear the success message. If desired, apply the same 2s clear used elsewhere:
Apply this diff inside the Ok(()) arm, after setting the info/error message following sync:
@@ match sync_service.force_sync().await { Ok(_) => { // Sync succeeded, reload local data self.load_local_data(sync_service).await; self.info_message = Some("Label created successfully!".to_string()); } Err(e) => { // Sync failed, but try to reload local data anyway eprintln!("Warning: Sync failed after label creation: {e}"); self.load_local_data(sync_service).await; self.error_message = Some("Label created but sync failed - data may be stale".to_string()); } } + // Clear the message after a short delay, matching project/task flows + tokio::time::sleep(tokio::time::Duration::from_secs(2)).await; + self.error_message = None; + self.info_message = None;
906-945: Avoid unnecessary clone of edit_label_id; borrow as &str instead.Cloning the Option just to borrow it is wasteful. Borrow directly via as_deref() to get Option<&str>.
Apply this diff:
- if let Some(label_id) = &self.edit_label_id.clone() { + if let Some(label_id) = self.edit_label_id.as_deref() { @@ - match sync_service - .update_label_content(label_id, self.edit_label_name.trim()) + match sync_service + .update_label_content(label_id, self.edit_label_name.trim()) .await
959-978: Delete label operation is correct; syncing strategy matches project deletion.Using force_clear_and_sync after API delete ensures local views are fresh. The success message makes sense here.
As duplication grows across create/edit/delete flows for tasks/projects/labels, consider extracting a small helper to reduce repetition for the “call API → force sync → load_local_data → set message” pattern.
📜 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/sync.rs(1 hunks)src/ui/app.rs(3 hunks)src/ui/components/dialogs/label_creation_dialog.rs(1 hunks)src/ui/components/dialogs/label_delete_confirmation_dialog.rs(1 hunks)src/ui/components/dialogs/label_edit_dialog.rs(1 hunks)src/ui/components/dialogs/mod.rs(2 hunks)src/ui/events.rs(3 hunks)src/ui/renderer.rs(3 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (7)
src/ui/components/dialogs/label_edit_dialog.rs (2)
src/ui/components/dialogs/label_creation_dialog.rs (1)
render(19-81)src/ui/app.rs (2)
new(74-125)default(66-68)
src/ui/components/dialogs/label_creation_dialog.rs (2)
src/ui/components/dialogs/label_edit_dialog.rs (1)
render(19-81)src/ui/app.rs (2)
new(74-125)default(66-68)
src/sync.rs (3)
src/ui/app.rs (2)
create_label(868-904)delete_label(960-978)src/storage.rs (3)
update_task_labels(402-427)store_labels(325-353)LocalLabel(43-50)src/todoist.rs (2)
LabelDisplay(15-19)name(64-68)
src/ui/components/dialogs/label_delete_confirmation_dialog.rs (3)
src/ui/app.rs (2)
new(74-125)default(66-68)src/ui/components/dialogs/delete_confirmation_dialog.rs (3)
DeleteConfirmationDialog(16-48)render(18-47)DeleteConfirmationDialog(14-14)src/ui/components/dialogs/project_delete_confirmation_dialog.rs (3)
ProjectDeleteConfirmationDialog(16-43)ProjectDeleteConfirmationDialog(14-14)render(18-42)
src/ui/components/dialogs/mod.rs (2)
src/ui/components/mod.rs (1)
dialogs(3-3)src/ui/components/dialogs/project_creation_dialog.rs (1)
ProjectCreationDialog(15-15)
src/ui/app.rs (2)
src/sync.rs (3)
new(27-37)create_label(145-157)delete_label(176-182)src/todoist.rs (1)
LabelDisplay(15-19)
src/ui/renderer.rs (4)
src/ui/components/dialogs/label_creation_dialog.rs (1)
render(19-81)src/ui/components/dialogs/label_edit_dialog.rs (1)
render(19-81)src/ui/components/projects_list.rs (1)
render(19-182)src/ui/components/dialogs/label_delete_confirmation_dialog.rs (1)
render(18-48)
⏰ 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 (13)
src/ui/components/dialogs/mod.rs (2)
6-8: Label dialog modules wired into the module tree — looks goodConsistent with existing dialog submodules. No concerns.
19-21: Re-exports are correct and enable clean access for rendererPublic API surface mirrors projects/tasks dialogs. LGTM.
src/ui/renderer.rs (1)
15-17: Dialog imports updated appropriatelyAll newly added dialogs are imported via the aggregated dialogs module. Looks consistent with existing patterns.
src/ui/components/dialogs/label_edit_dialog.rs (1)
17-81: Solid, bounded dialog renderingLayout, scaling, and conditional instructions are consistent with the rest of the UI. No functional issues spotted.
src/ui/components/dialogs/label_creation_dialog.rs (1)
17-81: Creation dialog rendering is consistent and correctBehavior and styling match other dialogs; instructions and bounds checks look good.
src/ui/events.rs (3)
16-20: Good routing: label delete/create/edit handlers are gated early.Placing these checks alongside the existing project/task handlers keeps the modal flows mutually exclusive and predictable.
Also applies to: 26-30, 46-50
382-386: Key bindings for labels integrate cleanly with existing conventions.
- L starts label creation (parallel to A for projects).
- E handles edit for either project or label based on selection.
- D routes delete for either project or label.
Nice, cohesive ergonomics.
Also applies to: 388-396, 400-408
443-469: Editing handler is consistent and minimal.Input and control keys mirror other edit flows. No issues spotted.
src/ui/app.rs (5)
47-53: Label state added to App is clear and complete.Fields and defaults cover creation, editing, and delete confirmation. Naming aligns with project/task counterparts.
Also applies to: 108-115
819-824: Create/cancel label entry points are straightforward and mirror project/task flows.Good reuse of the same string buffer for creation and clearing on cancel.
Also applies to: 825-830
831-841: Edit label flow correctly seeds state and cleans up on cancel.No functional issues spotted.
Also applies to: 842-848
849-856: Shared input helpers keep label name editing simple.Consistent with project/task helpers; good reuse.
Also applies to: 858-865
947-953: Delete confirmation state management for labels is correct.Entry/exit of the confirmation state mirrors projects. No issues.
Also applies to: 954-958
| /// Handle events when label delete confirmation is open | ||
| async fn handle_label_delete_confirmation( | ||
| key: crossterm::event::KeyEvent, | ||
| app: &mut App, | ||
| sync_service: &SyncService, | ||
| ) -> Result<bool, anyhow::Error> { | ||
| match key.code { | ||
| KeyCode::Char('y') | KeyCode::Char('Y') => { | ||
| // User confirmed deletion | ||
| app.delete_label(sync_service).await; | ||
| Ok(true) | ||
| } | ||
| _ => { | ||
| // Any other key cancels the deletion | ||
| app.cancel_delete_label(); | ||
| Ok(true) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Inconsistent confirmation semantics: any key cancels label delete.
Unlike project delete confirmation (which only cancels on n/N/Esc and ignores other keys), this handler cancels on any non-Y key. That’s surprising and easy to dismiss unintentionally. Align with the project flow:
Apply this diff within the function to only cancel on n/N/Esc and ignore other keys:
@@
- match key.code {
- KeyCode::Char('y') | KeyCode::Char('Y') => {
- // User confirmed deletion
- app.delete_label(sync_service).await;
- Ok(true)
- }
- _ => {
- // Any other key cancels the deletion
- app.cancel_delete_label();
- Ok(true)
- }
- }
+ match key.code {
+ KeyCode::Char('y') | KeyCode::Char('Y') => {
+ // User confirmed deletion
+ app.delete_label(sync_service).await;
+ Ok(true)
+ }
+ KeyCode::Char('n') | KeyCode::Char('N') | KeyCode::Esc => {
+ // Cancel deletion
+ app.cancel_delete_label();
+ Ok(true)
+ }
+ _ => Ok(false), // Ignore other keys during confirmation
+ }Also, ensure the same pattern is used everywhere. The project/task delete handlers currently use the pattern KeyCode::Char('y' | 'Y'), which won’t compile. Consider updating those handlers to KeyCode::Char('y') | KeyCode::Char('Y') for correctness and consistency.
Run this to locate all occurrences that need updating:
🏁 Script executed:
#!/bin/bash
# Find invalid or inconsistent yes/no patterns
rg -n -C2 -e "KeyCode::Char\('y' \| 'Y'\)" -e "KeyCode::Char\('n' \| 'N'\)"Length of output: 1182
Align label delete confirmation semantics and normalize key‐match patterns
The label delete handler currently treats any non-Y key as a cancel, while project/task delete only cancel on n/N/Esc and ignore all other keys. Let’s make them consistent—and unify the OR‐pattern style across all three handlers.
• In src/ui/events.rs around lines 471–489, update handle_label_delete_confirmation to:
async fn handle_label_delete_confirmation(
key: crossterm::event::KeyEvent,
app: &mut App,
sync_service: &SyncService,
) -> Result<bool, anyhow::Error> {
- match key.code {
- KeyCode::Char('y') | KeyCode::Char('Y') => {
- // User confirmed deletion
- app.delete_label(sync_service).await;
- Ok(true)
- }
- _ => {
- // Any other key cancels the deletion
- app.cancel_delete_label();
- Ok(true)
- }
- }
+ match key.code {
+ KeyCode::Char('y') | KeyCode::Char('Y') => {
+ // Confirm delete
+ app.delete_label(sync_service).await;
+ Ok(true)
+ }
+ KeyCode::Char('n') | KeyCode::Char('N') | KeyCode::Esc => {
+ // Cancel delete
+ app.cancel_delete_label();
+ Ok(true)
+ }
+ _ => Ok(false), // ignore all other keys
+ }
}• Normalize the OR‐pattern style in the project/task delete handlers so they match the above form (not KeyCode::Char('y' | 'Y')):
- src/ui/events.rs:80 (
handle_selected_task_delete_confirmation) - src/ui/events.rs:101 (
handle_project_delete_confirmation)
Change
KeyCode::Char('y' | 'Y') => …
KeyCode::Char('n' | 'N') | KeyCode::Esc => …to
KeyCode::Char('y') | KeyCode::Char('Y') => …
KeyCode::Char('n') | KeyCode::Char('N') | KeyCode::Esc => …
_ => Ok(false),This ensures identical confirmation behavior and a consistent, idiomatic pattern style across all delete dialogs.
🤖 Prompt for AI Agents
In src/ui/events.rs around lines 471–489, change
handle_label_delete_confirmation so it only treats explicit cancel keys
(KeyCode::Char('n') | KeyCode::Char('N') | KeyCode::Esc) as cancellation and
returns Ok(false for all other keys, and update the affirmative pattern to the
normalized form KeyCode::Char('y') | KeyCode::Char('Y'). Also normalize the
OR-pattern style in the other two handlers at src/ui/events.rs line ~80
(handle_selected_task_delete_confirmation) and line ~101
(handle_project_delete_confirmation) so they use KeyCode::Char('y') |
KeyCode::Char('Y') for confirm, KeyCode::Char('n') | KeyCode::Char('N') |
KeyCode::Esc for cancel, and add a default _ => Ok(false) branch to ignore
unrelated keys.
1 participant
Summary by CodeRabbit