[Consolidated] Integrate navigation, cache, typed operations, snapshots, and search - #188
[Consolidated] Integrate navigation, cache, typed operations, snapshots, and search#188rsheyd wants to merge 26 commits into
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThe PR adds Todoist completion and due-date transport support, transactional cache persistence, typed and generation-aware UI loading, UUID-based navigation, bulk task controls, task-search focus handling, shortcut-bar configuration, version metadata, and related documentation and tests. ChangesApplication refactor and UI behavior
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/ui/components/dialog_component.rs (1)
476-509: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winKeep the selected search result visible while navigating. The popup renders results statically, so once the list is taller than the view the focused row can disappear while
tstill acts onsearch_selected_index. Use a statefulListStateor an explicit viewport/window around the selection.🤖 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 476 - 509, Update the search-results rendering around TaskListItem::render and results_list_widget to use a stateful ListState or equivalent viewport logic, keeping search_selected_index within the visible portion of the popup while navigating. Preserve the existing focused-row styling and selection behavior, and pass the selection state when rendering the list.
🤖 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/backend/todoist.rs`:
- Around line 264-289: The clear_due_date branch in the task update flow must
not early-return while discarding other requested changes. Update the logic
around the visible clear_due_date handling and UpdateTaskArgs construction to
send one request that clears the due date while preserving content, priority,
labels, duration, and other fields; alternatively reject combined arguments
explicitly. Add a regression test covering a clear_due_date plus content update.
In `@src/sync/storage.rs`:
- Around line 14-41: Update store_snapshot to reconcile the complete remote
snapshot by removing active cached projects, labels, sections, and tasks absent
from the supplied collections within the existing transaction, while preserving
intentional local tombstones. Ensure section reconciliation runs only when the
section fetch succeeded; distinguish fetch failure from an authoritative empty
sections result so empty successful snapshots remove stale active sections.
In `@src/ui/app_component.rs`:
- Around line 583-595: The bulk task mutation closures around
`spawn_task_operation` at src/ui/app_component.rs:583-595 and
src/ui/app_component.rs:703-727 must return per-task completion outcomes,
including successful mutations before an error, so the generic task runner can
refresh data after partial failure. Update both restore/complete and due-date
mutation flows consistently, preserving error propagation while reporting
partial progress and triggering the existing refresh behavior.
- Around line 1012-1016: Update the sidebar width calculation in the drag
handling around self.sidebar_width_override so the clamp bounds remain valid
when the terminal is narrower than MAIN_AREA_MIN_WIDTH plus SIDEBAR_MIN_WIDTH.
Ensure the upper bound is never below SIDEBAR_MIN_WIDTH before calling clamp,
while preserving the existing width constraints for normal terminal sizes.
- Around line 1067-1069: Update the key-routing match in the active-pane event
handling so ActivePane::Navigation delegates to sidebar.handle_key_events(key)
only when the sidebar is visible; otherwise route the key to
task_list.handle_key_events(key) and preserve the existing Tasks behavior.
In `@src/ui/core/operations.rs`:
- Around line 77-147: The execute method currently adds only the generic
description to service failures, causing TaskManager’s UI rendering to omit the
underlying cause. Update the error propagation in execute so each operation
preserves and displays the full anyhow error chain, using the existing
description context while ensuring TaskManager’s {} rendering includes the
backend/storage failure details.
In `@src/ui/core/task_manager.rs`:
- Around line 222-257: Update the task snapshot loading flow around the sidebar
task query, get_all_tasks, and label reads to propagate any storage error
instead of using unwrap_or_default. Collect these reads into one fallible load,
and emit DataLoadFailed when any query fails so the last-good view is preserved
rather than replacing it with partial or empty data.
- Around line 167-169: Update TaskManager::has_blocking_work so tasks whose
description starts with “Background sync” are treated like “Searching tasks” and
do not block UI navigation or controls; keep data-loading and mutation tasks
blocking.
---
Outside diff comments:
In `@src/ui/components/dialog_component.rs`:
- Around line 476-509: Update the search-results rendering around
TaskListItem::render and results_list_widget to use a stateful ListState or
equivalent viewport logic, keeping search_selected_index within the visible
portion of the popup while navigating. Preserve the existing focused-row styling
and selection behavior, and pass the selection state when rendering the list.
🪄 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: 793eec3c-bb7e-4c30-812e-e571d43e6d72
⛔ Files ignored due to path filters (3)
Cargo.lockis excluded by!**/*.lockdocs/assets/pr-186/navigation-after-dummy.pngis excluded by!**/*.pngdocs/assets/pr-188/search-focus-dummy.pngis excluded by!**/*.png
📒 Files selected for processing (35)
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/main.rssrc/storage.rssrc/sync/mod.rssrc/sync/storage.rssrc/sync/tasks.rssrc/ui/app_component.rssrc/ui/components/dialog_component.rssrc/ui/components/dialogs/mod.rssrc/ui/components/dialogs/system_dialogs.rssrc/ui/components/sidebar_component.rssrc/ui/components/sidebar_item_component.rssrc/ui/components/task_list_component.rssrc/ui/components/task_list_item_component.rssrc/ui/core/actions.rssrc/ui/core/mod.rssrc/ui/core/operations.rssrc/ui/core/task_manager.rssrc/ui/core/view_snapshot.rssrc/utils/datetime.rstests/config.rstests/storage/db.rstests/ui/components/dialog_component.rstests/ui/components/task_list_component.rstests/ui/core/task_manager.rstests/utils/datetime.rs
| if args.clear_due_date { | ||
| let response = self | ||
| .client | ||
| .post(format!("https://api.todoist.com/api/v1/tasks/{remote_id}")) | ||
| .bearer_auth(&self.api_token) | ||
| .json(&serde_json::json!({ "due_string": "no date" })) | ||
| .send() | ||
| .await | ||
| .map_err(|error| BackendError::Network(error.to_string()))?; | ||
| if !response.status().is_success() { | ||
| let status = response.status(); | ||
| let body = response.text().await.unwrap_or_default(); | ||
| return Err(BackendError::Other(format!("Todoist returned {status}: {body}"))); | ||
| } | ||
| let task = response | ||
| .json::<crate::todoist::Task>() | ||
| .await | ||
| .map_err(|error| BackendError::InvalidData(error.to_string()))?; | ||
| return Ok(Self::task_to_backend(&task)); | ||
| } | ||
|
|
||
| let todoist_args = crate::todoist::UpdateTaskArgs { | ||
| content: args.content, | ||
| description: args.description, | ||
| priority: args.priority, | ||
| due_string: None, |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Do not discard other fields when clearing the due date.
When clear_due_date is true, the early return silently ignores requested content, priority, labels, duration, and other updates. Build one update request containing the clear operation and remaining fields, or reject combined arguments explicitly; add a regression test for a clear-plus-content update.
🤖 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/backend/todoist.rs` around lines 264 - 289, The clear_due_date branch in
the task update flow must not early-return while discarding other requested
changes. Update the logic around the visible clear_due_date handling and
UpdateTaskArgs construction to send one request that clears the due date while
preserving content, priority, labels, duration, and other fields; alternatively
reject combined arguments explicitly. Add a regression test covering a
clear_due_date plus content update.
| 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 clamp from panicking on narrow terminals.
When screen_width - MAIN_AREA_MIN_WIDTH is below SIDEBAR_MIN_WIDTH, u16::clamp receives min > max and panics during a drag.
Proposed fix
- self.sidebar_width_override = Some(
- mouse
- .column
- .clamp(SIDEBAR_MIN_WIDTH, self.screen_width.saturating_sub(MAIN_AREA_MIN_WIDTH)),
- );
+ let max_width = self.screen_width.saturating_sub(MAIN_AREA_MIN_WIDTH);
+ let min_width = SIDEBAR_MIN_WIDTH.min(max_width);
+ self.sidebar_width_override =
+ Some(mouse.column.clamp(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.
| self.sidebar_width_override = Some( | |
| mouse | |
| .column | |
| .clamp(SIDEBAR_MIN_WIDTH, self.screen_width.saturating_sub(MAIN_AREA_MIN_WIDTH)), | |
| ); | |
| let max_width = self.screen_width.saturating_sub(MAIN_AREA_MIN_WIDTH); | |
| let min_width = SIDEBAR_MIN_WIDTH.min(max_width); | |
| self.sidebar_width_override = | |
| Some(mouse.column.clamp(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 1012 - 1016, Update the sidebar width
calculation in the drag handling around self.sidebar_width_override so the clamp
bounds remain valid when the terminal is narrower than MAIN_AREA_MIN_WIDTH plus
SIDEBAR_MIN_WIDTH. Ensure the upper bound is never below SIDEBAR_MIN_WIDTH
before calling clamp, while preserving the existing width constraints for normal
terminal sizes.
| let tasks = match &sidebar_selection { | ||
| SidebarSelection::Today => sync_service.get_tasks_for_today().await.unwrap_or_default(), | ||
| SidebarSelection::Tomorrow => sync_service.get_tasks_for_tomorrow().await.unwrap_or_default(), | ||
| SidebarSelection::Upcoming => sync_service.get_tasks_for_upcoming().await.unwrap_or_default(), | ||
| SidebarSelection::Project(index) => { | ||
| if let Some(project) = projects.get(index) { | ||
| sync_service.get_tasks_for_project(&project.uuid).await.unwrap_or_default() | ||
| } else { | ||
| Vec::new() | ||
| } | ||
| SidebarSelection::Project(project_uuid) => { | ||
| sync_service.get_tasks_for_project(project_uuid).await.unwrap_or_default() | ||
| } | ||
| SidebarSelection::Label(index) => { | ||
| if let Some(label) = labels.get(index) { | ||
| sync_service.get_tasks_with_label(label.uuid).await.unwrap_or_default() | ||
| } else { | ||
| Vec::new() | ||
| } | ||
| SidebarSelection::Label(label_uuid) => { | ||
| sync_service.get_tasks_with_label(*label_uuid).await.unwrap_or_default() | ||
| } | ||
| }; | ||
| let all_tasks = sync_service.get_all_tasks().await.unwrap_or_default(); | ||
| let today = chrono::Local::now().date_naive(); | ||
| let tomorrow = today + chrono::Duration::days(1); | ||
| let upcoming_end = today + chrono::Duration::days(90); | ||
| let mut navigation_counts = NavigationCounts::default(); | ||
| for task in all_tasks.iter().filter(|task| !task.is_completed && !task.is_deleted) { | ||
| if let Some(due) = &task.due_date { | ||
| if let Ok(due) = crate::utils::datetime::parse_date(due) { | ||
| navigation_counts.today += usize::from(due <= today); | ||
| navigation_counts.tomorrow += usize::from(due == tomorrow); | ||
| navigation_counts.upcoming += usize::from(due <= upcoming_end); | ||
| } | ||
| } | ||
| *navigation_counts.projects.entry(task.project_uuid).or_default() += 1; | ||
| } | ||
| for label in &labels { | ||
| let count = sync_service | ||
| .get_tasks_with_label(label.uuid) | ||
| .await | ||
| .unwrap_or_default() | ||
| .iter() | ||
| .filter(|task| !task.is_completed && !task.is_deleted) | ||
| .count(); | ||
| navigation_counts.labels.insert(label.uuid, count); | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Reject the snapshot when any task query fails.
These unwrap_or_default calls convert storage failures into a successful partial snapshot. That snapshot can replace the last-good view with empty tasks or incorrect navigation counts. Collect all reads into one fallible load and emit DataLoadFailed for any error.
🤖 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 222 - 257, Update the task snapshot
loading flow around the sidebar task query, get_all_tasks, and label reads to
propagate any storage error instead of using unwrap_or_default. Collect these
reads into one fallible load, and emit DataLoadFailed when any query fails so
the last-good view is preserved rather than replacing it with partial or empty
data.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/sync/tasks.rs (1)
367-400: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winComplete_task can report failure and leave local state stale despite a successful remote completion.
backend.complete_task(&remote_id)succeeds, thenfetch_completed_tasksis required to find the same task with acompleted_atvalue or the whole call errors viaok_or_else. If Todoist's completed-task feed lags behind the completion write (or the second call simply fails/times out), the task is completed remotely but this function returnsErrbefore ever updating local storage — the UI shows an error and the task stays uncrossed-out until the next full sync, even though the action actually succeeded.🛡️ Suggested fallback instead of hard failure
- let (completed_since, completed_until) = datetime::today_completion_range(); - let completed_at = backend - .fetch_completed_tasks(&completed_since, &completed_until) - .await - .map_err(|e| anyhow::anyhow!("Task completed remotely, but completion refresh failed: {}", e))? - .into_iter() - .find(|task| task.remote_id == remote_id) - .and_then(|task| task.completed_at) - .ok_or_else(|| { - anyhow::anyhow!("Task completed remotely, but Todoist did not return its completion timestamp") - })?; + let (completed_since, completed_until) = datetime::today_completion_range(); + let completed_at = backend + .fetch_completed_tasks(&completed_since, &completed_until) + .await + .ok() + .and_then(|tasks| tasks.into_iter().find(|task| task.remote_id == remote_id)) + .and_then(|task| task.completed_at) + .unwrap_or_else(|| chrono::Utc::now().to_rfc3339());This keeps the remote completion (already committed) reflected locally, using a local fallback timestamp only when the authoritative one can't be fetched immediately.
🤖 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/sync/tasks.rs` around lines 367 - 400, Update complete_task so failures to fetch Todoist’s completion feed or locate completed_at do not return an error after backend.complete_task succeeds. Use the authoritative timestamp when available, otherwise fall back to a local current timestamp, then continue updating the task via TaskRepository::update so local state reflects the successful remote completion.
♻️ Duplicate comments (1)
src/backend/todoist.rs (1)
307-332: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winClear-due-date branch still discards other requested fields.
When
args.clear_due_dateis true, this returns early after only sendingdue_string: "no date", silently droppingcontent,priority,labels, anddurationfrom the same update call. This was already flagged in a prior review and remains unresolved.♻️ Suggested approach
- if args.clear_due_date { - let response = self - .client - .post(format!("https://api.todoist.com/api/v1/tasks/{remote_id}")) - .bearer_auth(&self.api_token) - .json(&serde_json::json!({ "due_string": "no date" })) - .send() - .await - .map_err(|error| BackendError::Network(error.to_string()))?; - if !response.status().is_success() { - let status = response.status(); - let body = response.text().await.unwrap_or_default(); - return Err(BackendError::Other(format!("Todoist returned {status}: {body}"))); - } - let task = response - .json::<crate::todoist::Task>() - .await - .map_err(|error| BackendError::InvalidData(error.to_string()))?; - return Ok(Self::task_to_backend(&task)); - } - - let todoist_args = crate::todoist::UpdateTaskArgs { - content: args.content, - description: args.description, - priority: args.priority, - due_string: None, - due_date: args.due_date, - due_datetime: args.due_datetime, - labels: args.labels, - duration: args.duration.as_ref().and_then(...), - ..Default::default() - }; + let todoist_args = crate::todoist::UpdateTaskArgs { + content: args.content, + description: args.description, + priority: args.priority, + due_string: args.clear_due_date.then(|| "no date".to_string()), + due_date: (!args.clear_due_date).then(|| args.due_date).flatten(), + due_datetime: (!args.clear_due_date).then(|| args.due_datetime).flatten(), + labels: args.labels, + duration: args.duration.as_ref().and_then(...), + ..Default::default() + };Combining into one request (if the Todoist API accepts
due_stringalongside other fields in a single PATCH) avoids losing the rest of the update.🤖 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/backend/todoist.rs` around lines 307 - 332, Update the clear_due_date branch in the task update method so it includes all requested fields—content, priority, labels, and duration—alongside due_string: "no date" in the same update request. Remove the early return and reuse the existing UpdateTaskArgs/update flow where possible, while preserving the current response validation and task conversion behavior.
🧹 Nitpick comments (1)
src/sync/mod.rs (1)
237-259: 🗄️ Data Integrity & Integration | 🔵 TrivialOnly today's completions are retained in the local cache — confirm this is the intended data-retention window.
completed_tasksare fetched only for the current local day and merged into the snapshot beforestore_snapshotpersists it. Since the snapshot-removal step (remove_tasks_absent_from_snapshotinsrc/sync/storage.rs) hard-deletes any local task not present in the merged list, a task completed on a prior day will no longer appear in any subsequent sync's snapshot and gets pruned from the local cache. This is consistent with the "Today view" reconciliation goal, but other read paths (e.g. per-project task listing) query local storage without a completion-date filter, so they'll silently stop returning older completed tasks once a day boundary passes. Worth confirming this is the intended scope rather than an oversight, since users may expect completed tasks to remain visible in project views beyond the day they were completed.Also applies to: 293-299
🤖 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/sync/mod.rs` around lines 237 - 259, The sync flow retains only tasks completed within today_completion_range, causing remove_tasks_absent_from_snapshot to delete older completed tasks from local storage. Confirm the intended retention window; if older completions must remain available to project listing paths, adjust the completed-task fetch or snapshot/removal flow to preserve them while retaining the Today view reconciliation behavior.
🤖 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/utils/datetime.rs`:
- Around line 38-49: Update completion_range_for_local_date to handle
LocalResult::None for both local-midnight conversions without calling expect or
panicking. Define the intended fallback for skipped midnights and preserve the
existing RFC3339 UTC range output for normal and ambiguous local times.
---
Outside diff comments:
In `@src/sync/tasks.rs`:
- Around line 367-400: Update complete_task so failures to fetch Todoist’s
completion feed or locate completed_at do not return an error after
backend.complete_task succeeds. Use the authoritative timestamp when available,
otherwise fall back to a local current timestamp, then continue updating the
task via TaskRepository::update so local state reflects the successful remote
completion.
---
Duplicate comments:
In `@src/backend/todoist.rs`:
- Around line 307-332: Update the clear_due_date branch in the task update
method so it includes all requested fields—content, priority, labels, and
duration—alongside due_string: "no date" in the same update request. Remove the
early return and reuse the existing UpdateTaskArgs/update flow where possible,
while preserving the current response validation and task conversion behavior.
---
Nitpick comments:
In `@src/sync/mod.rs`:
- Around line 237-259: The sync flow retains only tasks completed within
today_completion_range, causing remove_tasks_absent_from_snapshot to delete
older completed tasks from local storage. Confirm the intended retention window;
if older completions must remain available to project listing paths, adjust the
completed-task fetch or snapshot/removal flow to preserve them while retaining
the Today view reconciliation behavior.
🪄 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: ed97e50f-ea87-4cd2-bc28-8c7581432988
📒 Files selected for processing (14)
src/backend/mod.rssrc/backend/todoist.rssrc/entities/task.rssrc/repositories/task.rssrc/storage.rssrc/sync/mod.rssrc/sync/storage.rssrc/sync/tasks.rssrc/ui/app_component.rssrc/ui/components/task_list_component.rssrc/utils/datetime.rstests/storage/db.rstests/ui/components/dialog_component.rstests/ui/components/task_list_component.rs
🚧 Files skipped from review as they are similar to previous changes (5)
- tests/ui/components/task_list_component.rs
- tests/ui/components/dialog_component.rs
- src/ui/app_component.rs
- src/ui/components/task_list_component.rs
- src/sync/storage.rs
| fn completion_range_for_local_date(date: NaiveDate) -> (String, String) { | ||
| let start = Local | ||
| .from_local_datetime(&date.and_hms_opt(0, 0, 0).expect("midnight is valid")) | ||
| .earliest() | ||
| .expect("local date has a midnight"); | ||
| let end_date = date + Duration::days(1); | ||
| let end = Local | ||
| .from_local_datetime(&end_date.and_hms_opt(0, 0, 0).expect("midnight is valid")) | ||
| .earliest() | ||
| .expect("local date has a midnight"); | ||
| (start.to_utc().to_rfc3339(), end.to_utc().to_rfc3339()) | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '--- src/utils/datetime.rs ---\n'
wc -l src/utils/datetime.rs
sed -n '1,220p' src/utils/datetime.rs
printf '\n--- call sites ---\n'
rg -n "completion_range_for_local_date|perform_sync|get_tasks_for_today|complete_task" srcRepository: romaintb/terminalist
Length of output: 249
🌐 Web query:
chrono LocalResult earliest None from_local_datetime DST gap midnight documentation
💡 Result:
In the Chrono library for Rust, LocalResult is an enum used to handle the results of converting a local (naive) date/time to a time-zone-aware DateTime [1][2]. When you perform a conversion that involves a Daylight Saving Time (DST) transition, the result may fall into one of three categories [2][3]: 1. Single(T): The conversion resulted in a single, unique valid time [2][3]. 2. Ambiguous(T, T): The conversion is ambiguous because the clock was turned backwards (a "fold"), creating an overlap [1][2]. The two values represent the earliest and latest possible results [1][3]. 3. None: The conversion failed, most commonly because the requested local time falls into a "gap" created when the clock is turned forwards (e.g., skipping from 01:59 to 03:00) [2][3]. This variant is also returned if there was an error, such as missing time zone data, OS API errors, or overflow [1][2]. The method LocalResult::earliest returns the earliest possible result of the time zone mapping [1][2]. If the result is None (such as during a DST gap) or an error occurred, earliest will return None [1][2]. Similarly, latest returns the latest possible result, also returning None in the case of a gap or error [1][2]. Developers are encouraged to handle these cases explicitly [4]. Because None represents both missing time (a gap) and potential errors, Chrono users generally treat a None result as an indication that the specified local time does not exist in the target time zone [2][4]. When dealing with ambiguity (fold), you can use.earliest or.latest to select a specific occurrence, whereas for a gap, you must determine an appropriate fallback strategy, such as shifting the time forward or rejecting the input [4]. Note: In some environments, such as WASM or certain Windows configurations, the platform-provided time zone functions may always return Single(T), masking the expected Ambiguous or None results that would occur on other platforms [2][5].
Citations:
- 1: https://docs.rs/chrono/latest/chrono/offset/enum.LocalResult.html
- 2: https://docs.rs/chrono/latest/chrono/offset/type.MappedLocalTime.html
- 3: https://github.com/chronotope/chrono/blob/main/src/offset/mod.rs
- 4: https://app.studyraid.com/en/read/15077/521946/addressing-daylight-saving-time-transitions
- 5: How to identify if a generic
DateTime<Tz>is in a DST transition? chronotope/chrono#1242
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- src/utils/datetime.rs ---'
wc -l src/utils/datetime.rs
sed -n '1,220p' src/utils/datetime.rs
echo
echo '--- call sites ---'
rg -n "completion_range_for_local_date|perform_sync|get_tasks_for_today|complete_task" srcRepository: romaintb/terminalist
Length of output: 8353
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- src/sync/tasks.rs (today range usage) ---'
sed -n '1,140p' src/sync/tasks.rs
echo
echo '--- src/sync/mod.rs (perform_sync context) ---'
sed -n '170,240p' src/sync/mod.rs
echo
echo '--- other datetime usages ---'
rg -n "today_completion_range\(|completion_range_for_local_date\(" srcRepository: romaintb/terminalist
Length of output: 9371
Handle midnight gaps in src/utils/datetime.rs
LocalResult::None is returned when local midnight is skipped during a DST spring-forward transition, so earliest().expect(...) can panic here. This path feeds sync, Today view, and task completion flows; handle None explicitly instead of unwrapping.
🤖 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/utils/datetime.rs` around lines 38 - 49, Update
completion_range_for_local_date to handle LocalResult::None for both
local-midnight conversions without calling expect or panicking. Define the
intended fallback for skipped midnights and preserve the existing RFC3339 UTC
range output for normal and ambiguous local times.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/ui/core/task_manager.rs (1)
287-296: 🚀 Performance & Scalability | 🟠 Major | ⚡ Quick winAvoid N+1 queries when computing label counts.
This loop performs a separate await/query for every label sequentially to compute navigation counts. If the user has many labels, this sequential I/O will significantly block and delay the data load.
Consider computing these counts from the already-loaded
all_tasks(if task label associations are present in memory), or adding a bulk count method tosync_servicethat resolves all label counts in a single efficient database query.🤖 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 287 - 296, The label-count loop in the navigation count construction performs one sequential get_tasks_with_label query per label. Replace this N+1 pattern by deriving counts from the already-loaded all_tasks and their label associations when available, or by introducing and using a sync_service bulk-count method that returns all label counts in one query, while preserving the existing completed/deleted filtering and navigation_counts.labels updates.
🧹 Nitpick comments (1)
build.rs (1)
14-29: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider emitting
cargo:rerun-if-changedinstructions or using a library to prevent stale version output.Without explicit
rerun-if-changedinstructions, Cargo re-runs the build script only when package files change, ignoring the.gitdirectory. Runninggit commitdoes not change source file timestamps, so the subsequentcargo buildwill reuse the old build script output. This leaves the binary with a stale commit hash and an outdated "dirty" flag during local development.Manually tracking all the correct Git files (like
.git/HEAD,.git/index, and branch refs) alongside source files can be brittle. Consider using a dedicated build-script library likevergenorbuiltto handle these cache-invalidation rules robustly.If you prefer to keep this dependency-free, you can mitigate the most common issues by explicitly instructing Cargo to watch the Git state and your source directory.
♻️ Proposed mitigation using explicit rerun directives
Add the following at the end of your
mainfunction:// Force Cargo to re-run this script if the current commit, staging area, or source files change. println!("cargo:rerun-if-changed=.git/HEAD"); println!("cargo:rerun-if-changed=.git/index"); println!("cargo:rerun-if-changed=src"); println!("cargo:rerun-if-changed=build.rs");🤖 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 `@build.rs` around lines 14 - 29, Update the build-script main function to emit Cargo rerun-if-changed directives for .git/HEAD, .git/index, the src directory, and build.rs, ensuring commit and working-tree changes refresh TERMINALIST_GIT_REVISION and TERMINALIST_GIT_DIRTY. Keep the existing Git detection and environment-variable emission unchanged.
🤖 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.
Outside diff comments:
In `@src/ui/core/task_manager.rs`:
- Around line 287-296: The label-count loop in the navigation count construction
performs one sequential get_tasks_with_label query per label. Replace this N+1
pattern by deriving counts from the already-loaded all_tasks and their label
associations when available, or by introducing and using a sync_service
bulk-count method that returns all label counts in one query, while preserving
the existing completed/deleted filtering and navigation_counts.labels updates.
---
Nitpick comments:
In `@build.rs`:
- Around line 14-29: Update the build-script main function to emit Cargo
rerun-if-changed directives for .git/HEAD, .git/index, the src directory, and
build.rs, ensuring commit and working-tree changes refresh
TERMINALIST_GIT_REVISION and TERMINALIST_GIT_DIRTY. Keep the existing Git
detection and environment-variable emission unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a1076fa6-bb90-4170-84b7-5448ca661dd9
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (11)
CONTRIBUTING.mdCargo.tomlbuild.rssrc/main.rssrc/ui/app_component.rssrc/ui/components/dialog_component.rssrc/ui/components/task_list_component.rssrc/ui/core/task_manager.rstests/ui/components/dialog_component.rstests/ui/components/task_list_component.rstests/ui/core/task_manager.rs
🚧 Files skipped from review as they are similar to previous changes (5)
- tests/ui/core/task_manager.rs
- src/main.rs
- tests/ui/components/task_list_component.rs
- src/ui/components/dialog_component.rs
- src/ui/components/task_list_component.rs
|
This PR consolidates several related branches and is ready for review. I realize it’s substantial; would you prefer that I split it into smaller stacked PRs? |
|
A note on the failing Security Audit check: this appears to be an upstream workflow/toolchain issue rather than a failure in the PR code. I do not have access to administer or rerun the upstream check, but I am happy to prepare a workflow fix if you would like. |
|
@rsheyd thank you for this ! |
Thanks! Yes, I can rebase it and split it into smaller PRs. I also have some additional changes in my fork that I may propose separately later. I took a look at the current upstream changes, and the theme work overlaps quite a bit with the UI and configuration changes in my branches. Before I start resolving that, are there any other near-term UI, theme, configuration, or backend changes planned that it might make sense to wait for? Mostly trying to avoid doing the same integration work twice, and, save a little Codex quota :) |
|
I’ve decided to close this consolidated PR and propose the changes as a series of smaller, focused PRs instead. I’ll create each PR from the latest upstream/main. I’ve started with #201, which adds the Git revision to terminalist --version: #201 The next likely pieces include the persistent cache lifecycle, typed background operations, versioned view loading, and navigation and bulk-task improvements. I’ve written down the rough longer-term roadmap here: https://github.com/rsheyd/terminalist-edge/blob/main/docs/POST_PR188_CHANGES.md The list is provisional, and I’ll compare each item with the latest upstream code before preparing it. I’m going to close #188 in favor of this approach. |
|
@rsheyd nothing planned for the near future. Go ahead 😉 |
thanks! I submitted 2 PRs which are ready for your review :) |
2 participants
Consolidated integration PR
This PR replaces upstream PRs #186 and #187, plus the former stacked draft PRs #1–#3 on the
rsheyd/terminalistfork, with one integrated and end-to-end validated change set. It combines the navigation and bulk-workflow baseline with cache lifecycle fixes, typed operations, versioned view snapshots, search focus, responsive completion, an Agenda view with due-time scheduling, context-aware task creation, and a recoverable local Trash.Screenshots
Navigation sizing, counts, focus, and task footer
Search input and result focus
Both screenshots contain intentionally fabricated task, project, and label data.
Included work
completed_attimestamp for crossed-out Today rows;terminalist --versionfor local builds;Why consolidate
The user-visible workflows and correctness fixes meet at the same action-routing, cache, and view-loading boundaries. One integrated PR provides a clearer upstream test target while retaining focused commits that can still be reviewed or split independently.
Suggested review order
a067228— navigation, counts, bulk task commands, and processing feedback.e39f64f— persistent SQLite lifecycle, transactional refresh, and offline tests.6fd5b88— typed background operations.e047f2b— UUID selections, versioned snapshots, and stale-result rejection.d155a0e— search input/result focus and keyboard behavior.603885e— correct Inbox task creation.55d7b64andd46109e— shortcut-bar completion and remaining-task counts.a3bbff0— completion-history reconciliation and remote timestamps.74f4f78and9b76b28— completed-task contrast and selection styling.5756df1— local build revision metadata and contributing guidance.b26af89— non-blocking single-task completion, duplicate suppression, and live search refresh.c40259b— context-aware task creation and retained local Trash.bf4dc7c— Trash-specific shortcut-bar actions.951fa0bande0a7f07— Agenda scheduling, due-time editing, compact time rendering, and dialog input visibility.The largest apparent churn is in
AppComponent. Reviewing typed operations, versioned loads, completion responsiveness, and Trash routing in their separate commits avoids treating that work as one undifferentiated rewrite.User and developer impact
|or:are preserved exactly;Trash semantics and limitations
Todoist does not provide an API operation that undeletes a task. Terminalist therefore deletes remotely first and retains a local SQLite tombstone. Restore recreates the task in Todoist and receives a new Todoist task ID; it does not revive the original remote object. Trash is local to this Terminalist database, and Empty Trash or expiry permanently removes the remaining recreation data.
Completion-history root cause
Todoist's active-task endpoint omits completed and deleted tasks, so an active-only snapshot cannot distinguish progress history from stale cache entries. Terminalist now reads the completion-date endpoint for the current local day, caches Todoist's
completed_atvalue, and reconciles the combined snapshot transactionally. Intentional local Trash tombstones are excluded from active views and preserved separately until restoration, explicit emptying, or 30-day expiry.Validation
cargo fmt --all -- --checkcargo clippy --all-targets --all-features -- -D warningscargo testcargo buildFollow-up planning
docs/ARCHITECTURE_REFACTOR_PLAN.mdpredates this consolidated PR and the later completion-history, responsive-operation, context-aware creation, and Trash work. It is retained as historical design context, but its branch sequence and implementation-status sections are no longer authoritative. Remaining architecture work should be re-audited and re-baselined after this PR lands rather than inferred from that document's current checklists.