Skip to content

CODEBASE_REVIEW.md

Latest commit

 

History

History
310 lines (217 loc) · 9.61 KB

CODEBASE_REVIEW.md

File metadata and controls

310 lines (217 loc) · 9.61 KB
Raw

TaskFlow Code Improvements

Status: Progress tracked - Last updated Dec 2024

Summary

Category Total Fixed Partial Remaining
Performance 8 6 0 2
Code Quality 5 4 0 1
Testing Gaps 6 6 0 0
Documentation 6 6 0 0

Performance Issues

1. O(n²) Hierarchy Traversal - ✅ FIXED

File: src/app/model/hierarchy.rs

Solution: Now uses TaskCache.children HashMap for O(1) lookups.

// Current: Uses pre-built cache
if let Some(children) = self.task_cache.children.get(&current_id) { ... }

2. O(n²) Duplicate Detection - ✅ FIXED

File: src/domain/duplicate_detector.rs

Solution: Pre-groups tasks by project_id and caches lowercase titles.

  • Lines 86-98: Pre-computes lowercase titles once
  • Lines 101-114: Only compares within same project groups
  • Complexity reduced from O(n²) to O(Σ(p²)) where p is tasks per project
  • Added tests for Unicode/emoji, long titles, special characters

3. Nested Tag Filtering Loop - ✅ FIXED

File: src/app/model/filtering/matching.rs

Solution: Uses HashSet for O(1) tag lookup.

// Current: HashSet-based lookup
let task_tags_lower: HashSet<String> = task.tags.iter().map(|t| t.to_lowercase()).collect();
filter_tags.iter().any(|ft| task_tags_lower.contains(ft))

4. Linear Scan for Task ID Lookup - ✅ FIXED

File: src/storage/backends/in_memory/task_repo.rs

Solution: Changed ExportData.tasks from Vec<Task> to HashMap<TaskId, Task>.

// Current: O(1) HashMap operations
fn create_task(&mut self, task: &Task) -> StorageResult<()> {
    if self.data().tasks.contains_key(&task.id) {
        return Err(StorageError::already_exists("Task", task.id.to_string()));
    }
    self.data_mut().tasks.insert(task.id, task.clone());
    Ok(())
}

fn get_task(&self, id: &TaskId) -> StorageResult<Option<Task>> {
    Ok(self.data().tasks.get(id).cloned())
}

Updated files:

  • src/storage/repository.rs - ExportData struct definition
  • src/storage/backends/in_memory/task_repo.rs - All CRUD operations
  • src/storage/backends/markdown/storage.rs - Import/export
  • src/storage/backends/sqlite/storage.rs - Import/export
  • src/bin/taskflow/commands/pipe/handlers/ - CLI handlers

5. Excessive Task Cloning for Undo - ✅ FIXED

File: src/app/model/storage.rs

Solution: Created modify_task_with_undo() helper (line 481).

pub fn modify_task_with_undo<F>(&mut self, task_id: &TaskId, modifier: F) -> bool
where F: FnOnce(&mut Task),
{
    let before = task.clone();
    modifier(task);
    let after = task.clone();
    self.undo_stack.push(UndoAction::TaskModified { before, after });
    true
}

Used throughout src/app/update/task.rs for SetStatus, SetPriority, CyclePriority, MoveToProject.

6. String Allocations in SQLite Queries - ✅ ACCEPTABLE

File: src/storage/backends/sqlite/task_repo.rs

Status: Some allocations remain but are unavoidable with dynamic SQL. The params! macro requires owned String values for dynamic queries.

7. Secondary Indexes - ✅ FIXED

File: src/app/model/cache.rs

All indexes now present:

  • ✅ tasks: HashMap<TaskId, Task> - Primary
  • ✅ task_cache.children: HashMap<TaskId, Vec<TaskId>> - Hierarchy
  • ✅ task_cache.time_entries_by_task - Time tracking
  • ✅ task_cache.work_logs_by_task - Work logs
  • ✅ task_cache.tasks_by_project: HashMap<Option<ProjectId>, Vec<TaskId>> - Project lookup
  • ✅ task_cache.tasks_by_due_date: HashMap<Option<NaiveDate>, Vec<TaskId>> - Due date lookup
  • ✅ task_cache.contexts: HashSet<String> - Pre-computed @-prefixed tags

Solution: Added rebuild_secondary_indexes() method called from rebuild_caches(). Query methods all_contexts() and tasks_for_day() now use cached indexes for O(1) lookups.

8. Frequent refresh_visible_tasks Calls - ✅ ACCEPTABLE

Files: Multiple in src/app/update/

Status: Calls are strategic and justified by data modifications. Uses pre-computed FilterCache for fast execution.

Code Quality Issues

1. Unsafe unwrap() in Lexer - ✅ FIXED

File: src/domain/filter_dsl/lexer.rs

Solution: Uses pattern matching and if-let guards instead of unwrap().

// Current: Safe pattern matching
match remaining.chars().next() {
    Some(ch) => Err(ParseError::unexpected_char(start, ch)),
    None => Err(ParseError::unexpected_eof("token")),
}

2. Long Function: handle_navigation() - ✅ FIXED

File: src/app/update/navigation/ (now a module directory)

Solution: Refactored 756-line monolithic function into 10 view-specific modules:

src/app/update/navigation/
├── mod.rs           # Main dispatcher (~120 lines)
├── calendar.rs      # Calendar navigation + helpers
├── sidebar.rs       # Sidebar navigation + helpers
├── kanban.rs        # Kanban board navigation
├── eisenhower.rs    # Eisenhower matrix navigation
├── weekly_planner.rs # Weekly planner navigation
├── timeline.rs      # Timeline view navigation
├── reports.rs       # Reports panel navigation
├── network.rs       # Network view navigation
├── task_list.rs     # Basic Up/Down/First/Last/PageUp/PageDown
└── view.rs          # GoToView handling

Benefits:

  • Each module is focused and under 100 lines
  • Removed #[allow(clippy::too_many_lines)]
  • Easier to navigate and maintain view-specific logic

3. Complex Sort Logic - ✅ FIXED

File: src/domain/filter.rs

Solution: Extracted sort logic into impl SortField methods.

impl SortField {
    pub fn compare(&self, a: &Task, b: &Task) -> Ordering {
        match self {
            SortField::CreatedAt => a.created_at.cmp(&b.created_at),
            SortField::UpdatedAt => a.updated_at.cmp(&b.updated_at),
            SortField::DueDate => Self::compare_due_date(a, b),
            SortField::Priority => Self::compare_priority(a, b),
            SortField::Title => a.title.to_lowercase().cmp(&b.title.to_lowercase()),
            SortField::Status => Self::compare_status(a, b),
        }
    }
    // Private helper methods for each field type...
}

pub fn compare_sort_order(a: &Task, b: &Task) -> Ordering { ... }

Updated: src/app/model/filtering/visibility.rs now uses sort_field.compare(a, b) - reduced 50-line closure to ~15 lines.

4. Repeated Clone Pattern - ✅ FIXED

File: src/app/update/task.rs

Solution: Uses modify_task_with_undo() helper. See Performance Issue #5.

5. Dead Code Annotations - ✅ JUSTIFIED

Files:

  • src/app/model/filtering/matching.rs:81 - Alternative API method
  • src/bin/taskflow/commands/pipe/types.rs - Public API for external tools
  • src/ui/components/evening_review/queries.rs - Query builder utilities

Status: Annotations are intentional - code serves as API contract or test utilities.

Testing Gaps

All testing gaps have been addressed:

1. Filter DSL Parser - ✅ FIXED

Added property-based tests in tests/proptest_tests.rs:

  • ✅ Operator precedence edge cases (AND binds tighter than OR)
  • ✅ Date range boundary validation
  • ✅ Malformed input handling (fuzz testing)

2. Goal-KeyResult Integration - ✅ FIXED

Added tests in src/app/update/tests/goal.rs:

  • ✅ Tests linking KeyResults to Goals
  • ✅ Progress calculation with 0 target (division edge case)
  • ✅ Multiple KeyResults aggregation

3. Storage Stress Tests - ✅ FIXED

Added tests in tests/stress.rs:

  • ✅ Deep hierarchy tests (depth 10, linear chains 1000+)
  • ✅ JSON/SQLite backend stress tests (1000+ tasks)
  • ✅ Filtered query performance tests

4. Export Round-trip Tests - ✅ FIXED

Added tests in src/storage/export/*.rs:

  • ✅ CSV special character handling (commas, quotes, newlines, unicode)
  • ✅ ICS escaping and structure validation
  • ✅ DOT/Mermaid structure verification (all statuses, edges)

5. Habit Recurrence Edge Cases - ✅ FIXED

Added tests in src/domain/habit/tests.rs and src/app/update/task.rs:

  • ✅ Year boundary crossing (weekly habits)
  • ✅ Leap year handling (EveryNDays frequency)
  • ✅ Monthly tasks on 31st in short months

6. Duplicate Detector Edge Cases - ✅ FIXED

  • ✅ Unicode/emoji in titles
  • ✅ Very long titles
  • ✅ Special characters
  • ✅ Stress tests with large datasets

Documentation Improvements

All documentation gaps have been addressed:

  1. ✅ Analytics Module (src/domain/analytics.rs) - Added type overview table, ReportConfig examples, cross-references
  2. ✅ Work Log Module (src/domain/work_log.rs) - Added usage pattern workflow, TimeEntry comparison
  3. ✅ Calendar Event Module (src/domain/calendar_event.rs) - Added ICS import mapping, enum status docs, query examples
  4. ✅ Pomodoro Module (src/domain/pomodoro.rs) - Added session lifecycle, phase transitions, pause/resume docs
  5. ✅ Goal/KeyResult Modules (src/domain/goal.rs, key_result.rs) - Added OKR workflow, linked tasks example
  6. ✅ Tag Module (src/domain/tag.rs) - Added naming conventions, filter DSL integration examples

Quick Wins (All Completed)

All identified quick wins have been addressed:

  • ✅ Changed ExportData to HashMap (Dec 2024)
  • ✅ Added secondary indexes to TaskCache (Dec 2024)
  • ✅ Extracted sort logic into SortField methods (Dec 2024)
  • ✅ Split handle_navigation() into view-specific modules (Dec 2024)

Verification

cargo test
cargo clippy --all-targets
cargo bench  # if benchmarks added