xtablo-source/.planning/phases/04-tasks-kanban/04-REVIEW.md

phase	reviewed	depth	files_reviewed	files_reviewed_list	findings	status
04-tasks-kanban	2026-05-15T00:00:00Z	standard	11	backend/cmd/web/main.go backend/internal/db/queries/tasks.sql backend/internal/web/handlers_tasks.go backend/internal/web/handlers_tasks_test.go backend/internal/web/router.go backend/internal/web/ui/button.css backend/migrations/0004_tasks.sql backend/templates/layout.templ backend/templates/tablos.templ backend/templates/tasks.templ backend/templates/tasks_forms.go	critical warning info total 2 4 2 8	fixed

Phase 04: Code Review Report

Reviewed: 2026-05-15T00:00:00Z Depth: standard Files Reviewed: 11 Status: issues_found

Summary

Phase 04 adds the Tasks (Kanban) surface on top of the existing Tablos workflow. The core CRUD and ownership model is sound: loadOwnedTabloForTask correctly gates all task mutations behind tablo ownership and the tablo_id WHERE clause in UpdateTask / DeleteTask queries prevents cross-tablo task access. CSRF is applied uniformly, input lengths are validated, and all status values are validated against a whitelist before touching the DB.

Two blocking issues were found: a missing ParseForm call in TaskCreateHandler that silently swallows the request body on most Go HTTP clients, and an integer overflow path in TaskCreateHandler that can corrupt positions when a column holds 21 million+ tasks. Four additional warnings cover a silent-skip UpdateTask error in the reorder loop, an inconsistency between the TaskCreateFormFragment swap target and the HX-Retarget header, a security-relevant XSS vector in the raw fmt.Fprintf delete response, and a description field that is left trimmed to empty string but still passed as Valid: true in TaskUpdateHandler.

---

Critical Issues

CR-01: TaskCreateHandler never calls r.ParseForm() — form body not parsed

File: backend/internal/web/handlers_tasks.go:132

Issue: TaskCreateHandler calls r.PostFormValue("title") and r.PostFormValue("status") without first calling r.ParseForm(). Go's http.Request.PostFormValue only parses the form once implicitly; however the gorilla/csrf middleware reads the request body to validate the _csrf token and may consume the body before this handler runs, depending on whether the CSRF middleware buffers the body. In practice, if the middleware does not restore the body after reading it, all subsequent r.PostFormValue calls return empty strings. The HTMX path will always render a "Title is required" 422 error instead of creating the task. TaskUpdateHandler has the same missing call at line 321.

By contrast, TaskReorderHandler (line 401) correctly calls r.ParseForm() first. The two create/update handlers omit this entirely.

Fix:

func TaskCreateHandler(deps TasksDeps) http.HandlerFunc {
    return func(w http.ResponseWriter, r *http.Request) {
        tablo, _, ok := loadOwnedTablo(w, r, TablosDeps{Queries: deps.Queries})
        if !ok {
            return
        }
        ctx := r.Context()

        if err := r.ParseForm(); err != nil {
            http.Error(w, "bad request", http.StatusBadRequest)
            return
        }

        title := strings.TrimSpace(r.PostFormValue("title"))
        statusStr := r.PostFormValue("status")
        // ... rest unchanged
    }
}

Apply the same fix to TaskUpdateHandler at line 321.

---

CR-02: fmt.Fprintf in TaskDeleteHandler emits unescaped HTML — potential XSS

File: backend/internal/web/handlers_tasks.go:274

Issue: After a successful delete, the HTMX response is built with a bare fmt.Fprintf:

fmt.Fprintf(w, `<div id="task-%s" class="task-card-zone"></div>`, task.ID.String())

task.ID is a uuid.UUID whose String() output is always a lowercase hexadecimal UUID (safe). However the pattern is dangerous: it bypasses the templ auto-escaping pipeline used everywhere else in this codebase. If this pattern is copied for a field that is not a UUID (e.g., a task title in a future notification snippet), it becomes an XSS vector immediately. Additionally, the id attribute constructed here must exactly match id="task-{task.ID}" used by TaskCard; the manual string construction is fragile and inconsistent with every other handler.

Fix: Use a proper templ component instead of raw fmt.Fprintf:

// In tasks.templ — add:
templ TaskCardGone(taskID uuid.UUID) {
    <div id={ "task-" + taskID.String() } class="task-card-zone"></div>
}

// In handlers_tasks.go replace fmt.Fprintf with:
_ = templates.TaskCardGone(task.ID).Render(r.Context(), w)

---

Warnings

WR-01: UpdateTask errors silently discarded in TaskReorderHandler loop

File: backend/internal/web/handlers_tasks.go:479

Issue: Inside the main reorder loop and also in the single-task path (line 441), UpdateTask errors are explicitly discarded with _, _ =. If the DB is temporarily unavailable or a constraint fails mid-loop, the handler returns 200 with a stale board render. The user sees an apparent success while some (or all) position updates were never committed. The same silent discard appears at line 441 in the single-task branch.

Fix: Log errors and either abort with a 500 or collect failures and surface them:

if _, err := deps.Queries.UpdateTask(ctx, sqlc.UpdateTaskParams{...}); err != nil {
    slog.Default().Error("tasks reorder: UpdateTask failed",
        "task_id", existing.ID, "err", err)
    // option A: abort entire reorder
    http.Error(w, "internal server error", http.StatusInternalServerError)
    return
}

---

WR-02: TaskCreateHandler swap target mismatch — TaskCardOOB body lands in #column-{status} but card and OOB fragment need different targets

File: backend/internal/web/handlers_tasks.go:213-215

Issue: On success, the handler sets:

w.Header().Set("HX-Reswap", "beforeend")
w.Header().Set("HX-Retarget", "#column-"+string(status))

Then renders TaskCardOOB, which emits two top-level elements: a TaskCard div AND an OOB swap div. The HX-Retarget header redirects the primary swap to #column-{status} with beforeend. The OOB div (hx-swap-oob="innerHTML:#add-task-slot-{status}") is processed separately by HTMX. This appears correct at first glance.

However, TaskCreateFormFragment (in tasks.templ:240) declares hx-target="#column-{status}" and hx-swap="beforeend" on the form itself. When HTMX processes the response, it uses the server-sent HX-Retarget / HX-Reswap headers to override the form's own targets. The form is currently inside #add-task-slot-{status} (added there by AddTaskTrigger), not inside #column-{status}. If HX-Retarget successfully redirects the primary swap target to #column-{status}, the TaskCard will appear in the column — but the original form element inside #add-task-slot-{status} will remain because no swap targets it directly. The OOB resets #add-task-slot-{status}, which should clean it up.

The real issue: on a validation error path (422), the handler does NOT set HX-Retarget. The form's own hx-target="#column-{status}" and hx-swap="beforeend" then replace the entire column's content with the error form, not the add-task slot, destroying all task cards in that column for the duration of the error display. This is a functional bug that corrupts the user's view.

Fix: On the 422 validation-error path, either set HX-Retarget/HX-Reswap headers to point back at #add-task-slot-{status} with innerHTML, or change TaskCreateFormFragment's hx-target and hx-swap to target the slot (and rely on server headers only for the success path).

---

WR-03: Description whitespace-only string sets Valid: true in TaskUpdateHandler

File: backend/internal/web/handlers_tasks.go:352

Issue: The description field is read without trimming:

description := r.PostFormValue("description")

Then stored as:

Description: pgtype.Text{String: description, Valid: description != ""},

A user who submits a description of " " (spaces only) will store a whitespace-only string as a valid, non-null description in the DB. This is inconsistent with how other nullable text fields are handled (e.g., tablo description and color fields are typically empty-string checked). Querying for tasks with non-empty descriptions, or displaying them, will produce unexpected results.

Fix:

description := strings.TrimSpace(r.PostFormValue("description"))
// Then the existing Valid check `description != ""` correctly excludes whitespace-only.

---

WR-04: Integer overflow in TaskCreateHandler position arithmetic

File: backend/internal/web/handlers_tasks.go:189

Issue: maxPos is int32 (returned by MaxPositionByTabloAndStatus). maxPos + 100 is also int32. If maxPos is math.MaxInt32 - 99 (2,147,483,548) or higher, the addition overflows silently to a negative value, and the InsertTask call will store a negative position. Postgres stores it without complaint because the column is integer NOT NULL with no positive constraint.

Subsequent MaxPositionByTabloAndStatus calls will return the negative position, causing the next task to be inserted with an even-more-negative position. The board will silently render cards in an incorrect order.

While reaching 2 billion tasks per column requires pathological abuse, there is no server-side guard preventing a user from creating that many tasks, and the condition is undetectable from the user side.

Fix: Add an explicit overflow check:

const maxAllowedPosition = int32(2_000_000_000)
if maxPos > maxAllowedPosition-100 {
    errs.General = "Column has too many tasks to add more."
    // render error fragment
    return
}

---

Info

IN-01: UpdateTask query lacks tablo_id in WHERE clause — task can be updated across tablos via race condition

File: backend/internal/db/queries/tasks.sql:17-21

Issue: The UpdateTask query filters only on id:

UPDATE tasks SET ... WHERE id = $1

There is no AND tablo_id = $2 guard. The handler correctly fetches the task via GetTaskByID (which includes AND tablo_id = $2) before calling UpdateTask, providing application-layer isolation. However this means ownership enforcement is entirely in Go rather than in the SQL layer. A future handler that calls UpdateTask directly with a task ID obtained from an untrusted source would silently bypass the tablo-ownership check. The DeleteTask query correctly uses AND tablo_id = $2 — UpdateTask should too.

Fix:

-- name: UpdateTask :one
UPDATE tasks
SET title = $3, description = $4, status = $5, position = $6, updated_at = now()
WHERE id = $1 AND tablo_id = $2
RETURNING id, tablo_id, title, description, status, position, created_at, updated_at;

Update UpdateTaskParams and all callers accordingly.

---

IN-02: Dead code — redundant taskIDs re-assignment in TaskReorderHandler

File: backend/internal/web/handlers_tasks.go:415

Issue: Inside the len(taskCols) == 0 branch, lines 413-415 contain:

if len(taskIDs) == 0 {
    taskIDs = r.Form["task_id"]  // identical to line 406 — already assigned
}

taskIDs was assigned r.Form["task_id"] at line 406 unconditionally. Re-reading the same form field inside the branch produces the same value. The inner if block is unreachable dead code.

Fix: Remove lines 413-416:

// Delete this block entirely:
if len(taskIDs) == 0 {
    taskIDs = r.Form["task_id"]
}

---

Reviewed: 2026-05-15T00:00:00Z Reviewer: Claude (gsd-code-reviewer) Depth: standard