Feature/time estimate #65

noe.trevino · 2026-02-12T00:19:59-06:00

noe.trevino commented

2026-02-12 00:19:59 -06:00

No description provided.

claude[bot] (Migrated from github.com) reviewed

2026-02-12 00:21:15 -06:00

internal/services/task/validation.go

DRY violation: This validation logic duplicates ParseEstimate in estimate.go. Both functions:

Parse the same format (digit+unit sequences)
Track duplicate units with a map
Validate unit characters

Extract common parsing logic to eliminate duplication. Consider making ValidateEstimate call ParseEstimate and discard the result, or create a shared helper function.

**DRY violation**: This validation logic duplicates `ParseEstimate` in `estimate.go`. Both functions: - Parse the same format (digit+unit sequences) - Track duplicate units with a map - Validate unit characters Extract common parsing logic to eliminate duplication. Consider making `ValidateEstimate` call `ParseEstimate` and discard the result, or create a shared helper function.

claude[bot] (Migrated from github.com) reviewed

2026-02-12 00:21:18 -06:00

internal/cli/task/estimate.go

					
				@ -0,0 +96,4 @@

					return updateEstimate(cmd, cliInstance, taskID, &estimate, formatter, jsonOutput, quietMode)

				}

				func updateEstimate(cmd *cobra.Command, cliInstance *cli.CLI, taskID int, estimate *string, formatter *cli.OutputFormatter, jsonOutput, quietMode bool) error {

Unnecessary parameter duplication: The function signature takes 7 parameters where most are just passed through from the caller. This violates the "preserve whole object" refactoring pattern.

Consider extracting these into a struct or context object to reduce parameter count and improve maintainability.

**Unnecessary parameter duplication**: The function signature takes 7 parameters where most are just passed through from the caller. This violates the "preserve whole object" refactoring pattern. Consider extracting these into a struct or context object to reduce parameter count and improve maintainability.

github-actions[bot] commented

2026-02-12 00:21:31 -06:00

(Migrated from github.com)

PR Summary (auto-generated)

Added time estimate feature to tasks, stored as nullable string field in database (e.g., "2h", "1w2d3h", "30m") across SQLite and PostgreSQL with migration 00009_add_task_estimate.sql
Implemented new CLI command paso task estimate <task_id> [estimate] with --clear flag to remove estimates, --json and --quiet output modes, and validation for format (combinations of numbers + units: h/d/w/m)
Added TUI support with quick-edit keybinding for setting estimates via overlay modal (layer-based input form) and display in detail panel showing formatted time estimates
Updated all task queries (GetTask, GetTaskSummaries*, GetTaskDetail) to include estimate field, propagated through database adapters, converters, service layer, and models (TaskSummary, TaskDetail)
Added comprehensive test coverage including integration tests for CLI commands, service-level tests for estimate validation/parsing, and error handling for invalid formats (duplicate units, invalid units, missing values)
Estimate validation enforces compact format with no spaces, positive quantities, unique units per estimate, and rejects invalid/unsupported time units beyond h/d/w/m

**PR Summary** _(auto-generated)_ - Added time estimate feature to tasks, stored as nullable string field in database (e.g., "2h", "1w2d3h", "30m") across SQLite and PostgreSQL with migration 00009_add_task_estimate.sql - Implemented new CLI command `paso task estimate <task_id> [estimate]` with --clear flag to remove estimates, --json and --quiet output modes, and validation for format (combinations of numbers + units: h/d/w/m) - Added TUI support with quick-edit keybinding for setting estimates via overlay modal (layer-based input form) and display in detail panel showing formatted time estimates - Updated all task queries (GetTask, GetTaskSummaries*, GetTaskDetail) to include estimate field, propagated through database adapters, converters, service layer, and models (TaskSummary, TaskDetail) - Added comprehensive test coverage including integration tests for CLI commands, service-level tests for estimate validation/parsing, and error handling for invalid formats (duplicate units, invalid units, missing values) - Estimate validation enforces compact format with no spaces, positive quantities, unique units per estimate, and rejects invalid/unsupported time units beyond h/d/w/m

claude[bot] (Migrated from github.com) reviewed

2026-02-12 00:21:58 -06:00

internal/tui/update_pickers.go

					
				@ -834,0 +884,4 @@

							var estimatePtr *string

							if buffer != "" {

								estimatePtr = &buffer

DRY violation: Lines 878-907 duplicate the same "update if editing, otherwise just set form state" pattern found in updateAssigneePicker (lines 780-828), updatePriorityPicker (lines 631-671), and updateTypePicker (lines 711-745).

This is the 4th repetition of this conditional update pattern. Extract to a helper function that takes the update operation as a callback.

**DRY violation**: Lines 878-907 duplicate the same "update if editing, otherwise just set form state" pattern found in `updateAssigneePicker` (lines 780-828), `updatePriorityPicker` (lines 631-671), and `updateTypePicker` (lines 711-745). This is the 4th repetition of this conditional update pattern. Extract to a helper function that takes the update operation as a callback.

claude[bot] (Migrated from github.com) reviewed

2026-02-12 00:22:05 -06:00

internal/tui/update_pickers.go

					
				@ -834,0 +862,4 @@

							}

						}

						return m, nil

Code duplication: Lines 856-860 and 919-923 repeat the same validation logic. The error message string "Invalid format (use e.g. 2h, 1d, 1w2d)" appears 3 times in this function.

Extract to a constant or helper function.

**Code duplication**: Lines 856-860 and 919-923 repeat the same validation logic. The error message string `"Invalid format (use e.g. 2h, 1d, 1w2d)"` appears 3 times in this function. Extract to a constant or helper function.

claude[bot] (Migrated from github.com) reviewed

2026-02-12 00:22:21 -06:00

internal/services/task/estimate.go

					
				@ -0,0 +43,4 @@

						return nil, fmt.Errorf("%w: no valid components found", ErrInvalidEstimateFormat)

					}

					// Verify that the entire string was matched (no extra characters)

Inefficient string reconstruction: Building reconstructed by concatenating all matches is wasteful. You're creating a new string just to compare it with the original.

Instead, track cumulative byte positions or use FindAllStringIndex() to verify the entire string was consumed without allocating a new string.

**Inefficient string reconstruction**: Building `reconstructed` by concatenating all matches is wasteful. You're creating a new string just to compare it with the original. Instead, track cumulative byte positions or use `FindAllStringIndex()` to verify the entire string was consumed without allocating a new string.

claude[bot] (Migrated from github.com) reviewed

2026-02-12 00:22:27 -06:00

internal/services/task/service.go

Validation inconsistency: The estimate is set here without validation. While ValidateEstimate is called in the CLI layer (estimate.go:92), this service method should also validate to protect against invalid data from other callers.

Add validation here or document that callers must validate.

**Validation inconsistency**: The estimate is set here without validation. While `ValidateEstimate` is called in the CLI layer (estimate.go:92), this service method should also validate to protect against invalid data from other callers. Add validation here or document that callers must validate.

claude[bot] (Migrated from github.com) reviewed

2026-02-12 00:22:51 -06:00

internal/services/task/validation.go

					
				@ -151,0 +176,4 @@

					// Delegate to ParseEstimate for validation

					_, err := ParseEstimate(est)

					return err

				}

Critical DRY violation: The validation logic in ValidateEstimate (lines 158-224) is nearly identical to ParseEstimate in estimate.go (lines 33-84). Both:

Use the same state machine approach (tracking position i)
Check for digits followed by units
Track seen units to prevent duplicates
Validate the same unit characters (h, d, w, m)

This is a textbook violation of DRY. Refactor ValidateEstimate to call ParseEstimate and discard the result:

func ValidateEstimate(estimate *string) error {
    if estimate == nil || *estimate == "" {
        return nil
    }
    _, err := ParseEstimate(strings.TrimSpace(*estimate))
    return err
}

**Critical DRY violation**: The validation logic in `ValidateEstimate` (lines 158-224) is nearly identical to `ParseEstimate` in `estimate.go` (lines 33-84). Both: - Use the same state machine approach (tracking position `i`) - Check for digits followed by units - Track seen units to prevent duplicates - Validate the same unit characters (h, d, w, m) This is a textbook violation of DRY. Refactor `ValidateEstimate` to call `ParseEstimate` and discard the result: ```go func ValidateEstimate(estimate *string) error { if estimate == nil || *estimate == "" { return nil } _, err := ParseEstimate(strings.TrimSpace(*estimate)) return err } ```

claude[bot] commented

2026-02-12 00:23:15 -06:00

(Migrated from github.com)

Code Review Summary

Identified several code quality issues requiring attention:

Critical Issues

DRY Violations (High Priority)

ValidateEstimate and ParseEstimate duplicate ~60 lines of identical parsing logic
updateEstimateInput, updateAssigneePicker, updatePriorityPicker, and updateTypePicker all repeat the same "update if editing, else set form state" pattern
Parent/child picker enter handlers duplicate 98% of their logic across functions

Refactoring Opportunities

Guard Pattern Flattening

5 validation functions (validateTaskID, validateColumnID, etc.) follow identical if-id-less-than-zero pattern and can be consolidated into a single helper

Performance

Inefficient string reconstruction in ParseEstimate (line 46-54) - builds a string just to compare equality

Design

Missing validation in service layer (CreateTask sets estimate without validation)
runEstimate violates Single Responsibility Principle by mixing parsing, validation, and business logic
Parameter duplication in updateEstimate (7 parameters, most just passed through)

Minor Issues

Magic string for error message repeated 3 times - extract to constant
Ignored errors from GetBool() flag parsing

See inline comments for specific locations and suggested fixes.

## Code Review Summary Identified several code quality issues requiring attention: ### Critical Issues **DRY Violations (High Priority)** - ValidateEstimate and ParseEstimate duplicate ~60 lines of identical parsing logic - updateEstimateInput, updateAssigneePicker, updatePriorityPicker, and updateTypePicker all repeat the same "update if editing, else set form state" pattern - Parent/child picker enter handlers duplicate 98% of their logic across functions ### Refactoring Opportunities **Guard Pattern Flattening** - 5 validation functions (validateTaskID, validateColumnID, etc.) follow identical if-id-less-than-zero pattern and can be consolidated into a single helper **Performance** - Inefficient string reconstruction in ParseEstimate (line 46-54) - builds a string just to compare equality **Design** - Missing validation in service layer (CreateTask sets estimate without validation) - runEstimate violates Single Responsibility Principle by mixing parsing, validation, and business logic - Parameter duplication in updateEstimate (7 parameters, most just passed through) ### Minor Issues - Magic string for error message repeated 3 times - extract to constant - Ignored errors from GetBool() flag parsing See inline comments for specific locations and suggested fixes.

Sign in to join this conversation.

No reviewers