Feature 50/move tasks across project #88

noe.trevino · 2026-02-15T04:36:41-06:00

noe.trevino commented

2026-02-15 04:36:41 -06:00

Checklist

Read CONTRIBUTING.md
Ran tests (go test ./... -race) locally
Formatted and linted with gofmt -w . and golangci-lint run ./... respectively
Added tests if necessary (significant changes/complex logic)
Updated documentation if applicable
Updated README.md if applicable

Summary

Closes #50
Closes #79
Closes #78

## Checklist - [x] Read `CONTRIBUTING.md` - [x] Ran tests (`go test ./... -race`) locally - [x] Formatted and linted with `gofmt -w .` and `golangci-lint run ./...` respectively - [x] Added tests if necessary (significant changes/complex logic) - [x] Updated documentation if applicable - [x] Updated `README.md` if applicable ## Summary  Closes #50 Closes #79 Closes #78

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

2026-02-15 04:37:48 -06:00

internal/cli/task/switch_logic.go

					
				@ -0,0 +24,4 @@

						return nil, fmt.Errorf("task ID and project ID are required")

					}

					if len(args) > 2 {

The len(args) > 2 check is redundant because the cobra command already enforces ExactArgs(2) (see switch.go:36). This validation will never be reached.

The `len(args) > 2` check is redundant because the cobra command already enforces `ExactArgs(2)` (see switch.go:36). This validation will never be reached.

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

2026-02-15 04:38:12 -06:00

internal/tui/update_pickers.go

					
				@ -754,0 +801,4 @@

							m.Forms.Form.EditingTaskID = 0

						}

						m.Pickers.Project.Reset()

						return m, nil

Missing m.Pickers.Project.Reset() call before returning. This can leave stale state in the picker after closing. Compare with the esc case at line 768 which correctly calls Reset().

Missing `m.Pickers.Project.Reset()` call before returning. This can leave stale state in the picker after closing. Compare with the esc case at line 768 which correctly calls Reset().

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

2026-02-15 04:38:37 -06:00

internal/config/keymaps.go

					
				@ -48,9 +48,8 @@ type KeyMappings struct {

					Quit          string `yaml:"quit"`

Dead code: SortList field is no longer used after removing the sort functionality. Should be removed along with its default value and validation logic (lines 105, 218-219).

Dead code: `SortList` field is no longer used after removing the sort functionality. Should be removed along with its default value and validation logic (lines 105, 218-219).

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

2026-02-15 04:38:50 -06:00

internal/services/task/service.go

When sql.ErrNoRows is returned, the error should be wrapped with a descriptive message instead of returning it directly. This makes it inconsistent with the handling of other database operations in this function (lines 1239-1240, 1248-1249). Should return something like fmt.Errorf("task %d not found", taskID) instead.

When `sql.ErrNoRows` is returned, the error should be wrapped with a descriptive message instead of returning it directly. This makes it inconsistent with the handling of other database operations in this function (lines 1239-1240, 1248-1249). Should return something like `fmt.Errorf("task %d not found", taskID)` instead.

claude[bot] commented

2026-02-15 04:39:44 -06:00

(Migrated from github.com)

Review Summary

Found 4 substantive issues:

Dead code in keymaps (internal/config/keymaps.go:53): The SortList field is no longer used after removing the sort functionality. Should be removed along with its default value and validation logic.
Missing Reset() call (internal/tui/update_pickers.go:799-803): The project picker's enter case doesn't call m.Pickers.Project.Reset() before returning, which can leave stale state. Compare with the esc case at line 768 which correctly calls Reset().
Redundant validation (internal/cli/task/switch_logic.go:27): The len(args) > 2 check is redundant because cobra already enforces ExactArgs(2). This validation will never be reached.
Inconsistent error handling (internal/services/task/service.go:1226-1228): When sql.ErrNoRows is returned, the error should be wrapped with a descriptive message instead of returned directly. This is inconsistent with other database operations in the same function (lines 1239-1240, 1248-1249).

Score: 7/10

The implementation is functionally correct with good test coverage, but has cleanup issues (dead code) and minor bugs (missing Reset call, inconsistent error handling) that should be addressed.

## Review Summary Found 4 substantive issues: 1. **Dead code in keymaps** (internal/config/keymaps.go:53): The `SortList` field is no longer used after removing the sort functionality. Should be removed along with its default value and validation logic. 2. **Missing Reset() call** (internal/tui/update_pickers.go:799-803): The project picker's enter case doesn't call `m.Pickers.Project.Reset()` before returning, which can leave stale state. Compare with the esc case at line 768 which correctly calls Reset(). 3. **Redundant validation** (internal/cli/task/switch_logic.go:27): The `len(args) > 2` check is redundant because cobra already enforces `ExactArgs(2)`. This validation will never be reached. 4. **Inconsistent error handling** (internal/services/task/service.go:1226-1228): When `sql.ErrNoRows` is returned, the error should be wrapped with a descriptive message instead of returned directly. This is inconsistent with other database operations in the same function (lines 1239-1240, 1248-1249). **Score: 7/10** The implementation is functionally correct with good test coverage, but has cleanup issues (dead code) and minor bugs (missing Reset call, inconsistent error handling) that should be addressed.

Sign in to join this conversation.

No reviewers