Feature 124/archive tasks #129

noe.trevino · 2026-02-22T23:55:30-06:00

noe.trevino commented

2026-02-22 23:55:30 -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: #124

## 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: #124

github-actions[bot] commented

2026-02-22 23:55:40 -06:00

(Migrated from github.com)

Informational Warning:

The snapshot tests have been changed.
Please add a screenshot showing what has been changed, and a description of why it was changed.

If you do not provide an explanation and screenshot in 3 days, this PR will be closed.

@TheNoeTrevino

Changed snapshot files (10 files)

internal/testing/snapshots/cli/testdata/help/paso-task-archive.golden
internal/testing/snapshots/cli/testdata/help/paso-task.golden
internal/testing/snapshots/tui/testdata/board_with_labels.golden
internal/testing/snapshots/tui/testdata/board_with_multiple_tasks.golden
internal/testing/snapshots/tui/testdata/board_with_tasks.golden
internal/testing/snapshots/tui/testdata/connection_disconnected.golden
internal/testing/snapshots/tui/testdata/connection_reconnecting.golden
internal/testing/snapshots/tui/testdata/empty_project_board.golden
internal/testing/snapshots/tui/testdata/notification_error.golden
internal/testing/snapshots/tui/testdata/notification_warning.golden

**Informational Warning:** The snapshot tests have been changed. Please add a screenshot showing what has been changed, and a description of why it was changed. If you do not provide an explanation and screenshot in 3 days, this PR will be closed. @TheNoeTrevino <details> <summary>Changed snapshot files (10 files)</summary> - `internal/testing/snapshots/cli/testdata/help/paso-task-archive.golden` - `internal/testing/snapshots/cli/testdata/help/paso-task.golden` - `internal/testing/snapshots/tui/testdata/board_with_labels.golden` - `internal/testing/snapshots/tui/testdata/board_with_multiple_tasks.golden` - `internal/testing/snapshots/tui/testdata/board_with_tasks.golden` - `internal/testing/snapshots/tui/testdata/connection_disconnected.golden` - `internal/testing/snapshots/tui/testdata/connection_reconnecting.golden` - `internal/testing/snapshots/tui/testdata/empty_project_board.golden` - `internal/testing/snapshots/tui/testdata/notification_error.golden` - `internal/testing/snapshots/tui/testdata/notification_warning.golden` </details>

noe.trevino commented

2026-02-22 23:56:30 -06:00

all changes are due to the overlap with the new filterbar padding on each chip. See screenshot

<img width="2011" height="82" alt="image" src="https://github.com/user-attachments/assets/01ca9f79-7dec-4932-bffd-ef20e975ab36" /> all changes are due to the overlap with the new filterbar padding on each chip. See screenshot

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

2026-02-22 23:57:43 -06:00

internal/services/task/service.go

					
				@ -582,0 +607,4 @@

					}

					s.publishTaskEventForProject(int(projectID))

Race condition: non-atomic read-modify-write

The toggle is split across two separate DB calls: GetTaskDetail reads the current Archived value, then UpdateTaskArchived writes its inverse. Two concurrent callers (e.g. CLI + TUI) can both read Archived: false, both compute !false = true, and both write true — leaving the task archived instead of returning to active.

The fix is an atomic toggle in the SQL:

-- Postgres
UPDATE tasks SET archived = NOT archived, updated_at = current_timestamp WHERE id = $1;
-- SQLite
UPDATE tasks SET archived = 1 - archived, updated_at = current_timestamp WHERE id = ?;

Then remove the GetTaskDetail call entirely from this function and return the new state from the query if the caller needs it.

**Race condition: non-atomic read-modify-write** The toggle is split across two separate DB calls: `GetTaskDetail` reads the current `Archived` value, then `UpdateTaskArchived` writes its inverse. Two concurrent callers (e.g. CLI + TUI) can both read `Archived: false`, both compute `!false = true`, and both write `true` — leaving the task archived instead of returning to active. The fix is an atomic toggle in the SQL: ```sql -- Postgres UPDATE tasks SET archived = NOT archived, updated_at = current_timestamp WHERE id = $1; -- SQLite UPDATE tasks SET archived = 1 - archived, updated_at = current_timestamp WHERE id = ?; ``` Then remove the `GetTaskDetail` call entirely from this function and return the new state from the query if the caller needs it.

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

2026-02-22 23:57:50 -06:00

internal/cli/task/archive.go

					
				@ -0,0 +83,4 @@

					}

					result := &ArchiveResult{

						TaskID:      input.TaskID,

Unnecessary DB round-trip

ArchiveTask internally calls GetTaskDetail and then UpdateTaskArchived. Then runArchive calls GetTaskDetail a second time just to populate result.WasArchived. Since the operation is a toggle, the new state is deterministically !previousState, so the post-call fetch is redundant.

Either:

Have ArchiveTask return (bool, error) — the new archived state — so the handler doesn't need to re-fetch, or
Compute result.WasArchived from the pre-call knowledge (the command was issued, so the task is now the opposite of what it was).

As written, a successful archive of a non-archived task causes three complex join queries for a single user action.

**Unnecessary DB round-trip** `ArchiveTask` internally calls `GetTaskDetail` and then `UpdateTaskArchived`. Then `runArchive` calls `GetTaskDetail` a second time just to populate `result.WasArchived`. Since the operation is a toggle, the new state is deterministically `!previousState`, so the post-call fetch is redundant. Either: - Have `ArchiveTask` return `(bool, error)` — the new archived state — so the handler doesn't need to re-fetch, or - Compute `result.WasArchived` from the *pre-call* knowledge (the command was issued, so the task is now the opposite of what it was). As written, a successful archive of a non-archived task causes three complex join queries for a single user action.

Sign in to join this conversation.

No reviewers