Improve task navigation and bulk workflows - #186
Conversation
|
Warning Review limit reached
Next review available in: 16 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (22)
WalkthroughChangesUI task workflow and backend synchronization
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
7532f08 to
c03065c
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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/ui/components/dialog_component.rs (2)
137-142: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winReset selection when accepting results for a new query.
Clamping preserves the old numeric index even though the result identities changed. A subsequent
tcan therefore mutate an unrelated task at that index. Reset to0, or preserve selection by UUID.Proposed fix
if query == self.input_buffer { self.search_results = results; - self.search_selected_index = - self.search_selected_index.min(self.search_results.len().saturating_sub(1)); + self.search_selected_index = 0; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/ui/components/dialog_component.rs` around lines 137 - 142, Update update_search_results so that when query matches self.input_buffer and new results are accepted, reset search_selected_index to 0 instead of clamping the previous numeric index; keep the existing stale-query guard and result assignment unchanged.
466-495: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winScroll the result list with its selected item.
The index can advance beyond the visible popup, but this stateless
Listalways renders from offset zero. Use a persistentListState, selectsearch_selected_index, and callrender_stateful_widget.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/ui/components/dialog_component.rs` around lines 466 - 495, Update the search-results rendering block in the dialog component to use a persistent ListState, selecting search_selected_index before rendering. Replace the stateless results_list_widget rendering with render_stateful_widget and pass the state so the list scrolls to keep the selected result visible.src/ui/app_component.rs (1)
401-440: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winRemove task-specific global shortcuts to enforce bulk logic.
These
t,T,w, andWshortcuts are still defined as global keys that operate only on a single selected task. If triggered while the Navigation pane is focused, they bypass the new bulk operation logic (SetTasksDueDate) and ignore any marked tasks.By contrast, the new
u(unschedule) andx(mark) shortcuts are appropriately omitted from this global handler. Removing these legacy mappings ensures all task-related shortcuts properly delegate totask_list_componentand behave consistently.♻️ Proposed fix
Remove these match arms:
- KeyCode::Char('t') => { - // Set task due date to today - if let Some(task) = self.task_list.get_selected_task() { - info!("Global key: 't' - setting task '{}' due today", task.content); - Action::SetTaskDueToday(task.uuid) - } else { - info!("Global key: 't' - no task selected"); - Action::ShowDialog(DialogType::Info(UI_NO_TASK_SELECTED_DUE_DATE.to_string())) - } - } - KeyCode::Char('T') => { - // Set task due date to tomorrow - if let Some(task) = self.task_list.get_selected_task() { - info!("Global key: 'T' - setting task '{}' due tomorrow", task.content); - Action::SetTaskDueTomorrow(task.uuid) - } else { - info!("Global key: 'T' - no task selected"); - Action::ShowDialog(DialogType::Info(UI_NO_TASK_SELECTED_DUE_DATE.to_string())) - } - } - KeyCode::Char('w') => { - // Set task due date to next week (Monday) - if let Some(task) = self.task_list.get_selected_task() { - info!("Global key: 'w' - setting task '{}' due next week", task.content); - Action::SetTaskDueNextWeek(task.uuid) - } else { - info!("Global key: 'w' - no task selected"); - Action::ShowDialog(DialogType::Info(UI_NO_TASK_SELECTED_DUE_DATE.to_string())) - } - } - KeyCode::Char('W') => { - // Set task due date to weekend (Saturday) - if let Some(task) = self.task_list.get_selected_task() { - info!("Global key: 'W' - setting task '{}' due weekend", task.content); - Action::SetTaskDueWeekEnd(task.uuid) - } else { - info!("Global key: 'W' - no task selected"); - Action::ShowDialog(DialogType::Info(UI_NO_TASK_SELECTED_DUE_DATE.to_string())) - } - }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/ui/app_component.rs` around lines 401 - 440, Remove the KeyCode::Char('t'), 'T', 'w', and 'W' match arms from the global key handler so task due-date shortcuts are delegated to task_list_component and bulk marked-task handling via SetTasksDueDate is preserved.
🧹 Nitpick comments (2)
src/ui/core/task_manager.rs (1)
261-270: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueConsider optimizing label task counts.
This loop executes a sequential database query for each label (
N+1queries). While local SQLite queries are generally fast and this runs in a background task, it could eventually become a performance bottleneck for users with a large number of labels. Consider adding a bulk query toSyncService(e.g., using aGROUP BYclause) to fetch all label counts at once in a future optimization.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/ui/core/task_manager.rs` around lines 261 - 270, Optimize the label-counting flow around the labels loop by adding a SyncService bulk query that returns active, non-deleted task counts grouped by label UUID, then populate navigation_counts.labels from that result instead of calling get_tasks_with_label separately for each label. Preserve zero counts for labels absent from the grouped results.src/ui/app_component.rs (1)
576-593: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider accumulating errors for bulk operations.
Using the
?operator inside the iteration causes the loop to exit on the very first failure (e.g. a momentary network hiccup). This leaves the remaining marked tasks completely unprocessed, which can be confusing for a bulk action.Consider capturing the errors and continuing the loop so that as many tasks as possible are processed, similar to the pattern you might want for
Action::SetTasksDueDatebelow.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/ui/app_component.rs` around lines 576 - 593, The Action::ToggleTasks handler currently stops at the first restore_task or complete_task error; accumulate each operation’s errors while continuing through all tasks, then return a result that reports the collected failures after processing every task, following the existing bulk-operation pattern used by Action::SetTasksDueDate.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/ui/app_component.rs`:
- Around line 1229-1236: Update the sidebar drag handling in the
resizing_sidebar branch so the upper bound passed to mouse.column.clamp is never
below SIDEBAR_MIN_WIDTH. Ensure the screen_width subtraction result is raised to
at least SIDEBAR_MIN_WIDTH before calling clamp, preserving the existing
minimum-width behavior.
In `@src/ui/components/dialog_component.rs`:
- Around line 607-623: Update the TaskSearch handling in
src/ui/components/dialog_component.rs lines 607-623 to stop consuming printable
j, k, and t characters as result commands; use arrow keys, modifiers, or an
explicit navigation mode, while preserving result navigation and due-today
actions through non-conflicting bindings. In
tests/ui/components/dialog_component.rs lines 39-54, verify arrow-key navigation
without requiring j/k behavior; in lines 56-67, update the due-today shortcut
test to the new binding and verify lowercase t remains searchable.
---
Outside diff comments:
In `@src/ui/app_component.rs`:
- Around line 401-440: Remove the KeyCode::Char('t'), 'T', 'w', and 'W' match
arms from the global key handler so task due-date shortcuts are delegated to
task_list_component and bulk marked-task handling via SetTasksDueDate is
preserved.
In `@src/ui/components/dialog_component.rs`:
- Around line 137-142: Update update_search_results so that when query matches
self.input_buffer and new results are accepted, reset search_selected_index to 0
instead of clamping the previous numeric index; keep the existing stale-query
guard and result assignment unchanged.
- Around line 466-495: Update the search-results rendering block in the dialog
component to use a persistent ListState, selecting search_selected_index before
rendering. Replace the stateless results_list_widget rendering with
render_stateful_widget and pass the state so the list scrolls to keep the
selected result visible.
---
Nitpick comments:
In `@src/ui/app_component.rs`:
- Around line 576-593: The Action::ToggleTasks handler currently stops at the
first restore_task or complete_task error; accumulate each operation’s errors
while continuing through all tasks, then return a result that reports the
collected failures after processing every task, following the existing
bulk-operation pattern used by Action::SetTasksDueDate.
In `@src/ui/core/task_manager.rs`:
- Around line 261-270: Optimize the label-counting flow around the labels loop
by adding a SyncService bulk query that returns active, non-deleted task counts
grouped by label UUID, then populate navigation_counts.labels from that result
instead of calling get_tasks_with_label separately for each label. Preserve zero
counts for labels absent from the grouped results.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 07031024-f083-4822-9b5e-e1f2541e7311
⛔ Files ignored due to path filters (2)
Cargo.lockis excluded by!**/*.lockdocs/assets/pr-186/navigation-after-dummy.pngis excluded by!**/*.png
📒 Files selected for processing (25)
Cargo.tomlREADME.mddocs/ARCHITECTURE_REFACTOR_PLAN.mddocs/CONFIGURATION.mddocs/CURRENT_UI_WORK_BASELINE.mddocs/KEYBOARD_SHORTCUTS.mddocs/README.mdsrc/backend/mod.rssrc/backend/todoist.rssrc/config.rssrc/sync/tasks.rssrc/ui/app_component.rssrc/ui/components/dialog_component.rssrc/ui/components/dialogs/system_dialogs.rssrc/ui/components/sidebar_component.rssrc/ui/components/task_list_component.rssrc/ui/components/task_list_item_component.rssrc/ui/core/actions.rssrc/ui/core/task_manager.rssrc/utils/datetime.rstests/config.rstests/ui/components/dialog_component.rstests/ui/components/task_list_component.rstests/ui/core/task_manager.rstests/utils/datetime.rs
| if self.resizing_sidebar { | ||
| if matches!(mouse.kind, crossterm::event::MouseEventKind::Drag(_)) { | ||
| self.sidebar_width_override = Some( | ||
| mouse | ||
| .column | ||
| .clamp(SIDEBAR_MIN_WIDTH, self.screen_width.saturating_sub(MAIN_AREA_MIN_WIDTH)), | ||
| ); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win
Prevent panic during mouse drag on small terminal sizes.
If the terminal is resized to be very small, self.screen_width.saturating_sub(MAIN_AREA_MIN_WIDTH) can evaluate to a value smaller than SIDEBAR_MIN_WIDTH. When this happens, u16::clamp will panic because its max argument is less than its min argument, instantly crashing the app.
Ensure the calculated maximum is at least SIDEBAR_MIN_WIDTH.
🐛 Proposed fix
if self.resizing_sidebar {
if matches!(mouse.kind, crossterm::event::MouseEventKind::Drag(_)) {
+ let max_width = self.screen_width.saturating_sub(MAIN_AREA_MIN_WIDTH).max(SIDEBAR_MIN_WIDTH);
self.sidebar_width_override = Some(
mouse
.column
- .clamp(SIDEBAR_MIN_WIDTH, self.screen_width.saturating_sub(MAIN_AREA_MIN_WIDTH)),
+ .clamp(SIDEBAR_MIN_WIDTH, max_width),
);
}📝 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.
| if self.resizing_sidebar { | |
| if matches!(mouse.kind, crossterm::event::MouseEventKind::Drag(_)) { | |
| self.sidebar_width_override = Some( | |
| mouse | |
| .column | |
| .clamp(SIDEBAR_MIN_WIDTH, self.screen_width.saturating_sub(MAIN_AREA_MIN_WIDTH)), | |
| ); | |
| } | |
| if self.resizing_sidebar { | |
| if matches!(mouse.kind, crossterm::event::MouseEventKind::Drag(_)) { | |
| let max_width = self | |
| .screen_width | |
| .saturating_sub(MAIN_AREA_MIN_WIDTH) | |
| .max(SIDEBAR_MIN_WIDTH); | |
| self.sidebar_width_override = Some( | |
| mouse | |
| .column | |
| .clamp(SIDEBAR_MIN_WIDTH, max_width), | |
| ); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/ui/app_component.rs` around lines 1229 - 1236, Update the sidebar drag
handling in the resizing_sidebar branch so the upper bound passed to
mouse.column.clamp is never below SIDEBAR_MIN_WIDTH. Ensure the screen_width
subtraction result is raised to at least SIDEBAR_MIN_WIDTH before calling clamp,
preserving the existing minimum-width behavior.
[Consolidated] Integrate cache, typed operations, snapshots, and search focus
1 participant
Screenshot
Sanitized example using fictional tasks and projects:
What
Why
The navigation pane used excessive space, count semantics did not consistently match visible task rows, and longer-running commands remained interactive without feedback. Several related behaviors also needed a stable baseline before deeper state-management and transport refactors.
Impact
This changes navigation layout and keyboard behavior, adds bulk workflows, makes filtered subtasks visible, and temporarily blocks input during foreground mutations. Search and background sync remain non-blocking.
Due-date clearing currently uses a dedicated reqwest path because the existing Todoist wrapper cannot express the required clear operation. Consolidating that path is tracked in the architecture refactor plan.
Checks
cargo fmt --checkcargo clippy --all-targets -- -D warningscargo testcargo buildAll pass. The storage integration test requires normal host filesystem access and passes with that expected access.
Follow-up
docs/ARCHITECTURE_REFACTOR_PLAN.mdseparates typed operations, versioned snapshots, cache preservation, grouped counts, component-update optimization, and Todoist transport consolidation into later focused branches and pull requests.Summary by CodeRabbit