path: root/REFACTOR_PLAN.md

# Refactoring Plan — Task Dashboard

**Generated:** 2026-03-16
**Baseline commit:** `7d627d1`

---

## 1. Executive Summary

The Task Dashboard codebase is in solid shape overall: it builds cleanly, uses
consistent patterns (CacheFetcher, Atom abstraction, chi router), and has a
reasonable test suite for the core workflows. The most significant concerns are
a **failing test in CI** caused by a research file calling a deprecated API
endpoint, **low test coverage** in auth (20.4%) and API clients (46.2%), and
**scattered raw-HTML string building** in several handlers that bypasses the
template system and makes those paths untestable.

Secondary concerns include a correctness bug in `GetTasksByDateRange` (unused
`start` parameter), a meal-grouping algorithm duplicated verbatim in two
separate handlers, and a `convertSyncItemToTask` wrapper that allocates a
one-element slice just to unwrap it. None of these are regressions waiting to
happen, but they add maintenance surface and mislead future readers.

The codebase does NOT need a large structural overhaul. The recommended approach
is targeted, low-risk fixes ordered by impact.

---

## 2. Baseline Metrics

| Metric | Value |
|--------|-------|
| Build status | ✅ Clean |
| Test suite | ❌ 1 failing (`TestTodoistSyncResearch`) |
| `internal/api` coverage | 46.2% |
| `internal/auth` coverage | 20.4% |
| `internal/handlers` coverage | 53.4% |
| `internal/store` coverage | 65.9% |
| `internal/models` coverage | 73.6% |
| `internal/config` coverage | 87.2% |
| `internal/middleware` coverage | 86.8% |
| Total non-test Go source lines | ~6,200 |
| handlers.go (single file) | 1,240 lines |
| store/sqlite.go (single file) | 1,529 lines |

---

## 3. Refactoring Catalogue

Items are ordered by **impact × effort** (high-impact, low-effort first).

---

### RF-01: Fix failing research test (breaking CI)

**Priority:** Critical
**Effort:** Small
**Risk:** Low

**Current state:**
`internal/api/research_test.go:TestTodoistSyncResearch` calls the real Todoist
Sync v9 API (`https://api.todoist.com/sync/v9/sync`) using a raw HTTP client.
The test has a skip guard for `TODOIST_API_KEY == ""`, but the key is set in
the sandbox environment. Todoist has deprecated this endpoint and now returns
HTTP 410, causing `go test ./...` to exit non-zero.

**Target state:**
Tag the file with a `//go:build integration` constraint so it only runs
explicitly (`go test -tags integration ./...`). This immediately unblocks CI
without deleting useful research.

**Affected files:**
- `internal/api/research_test.go`

**Why it matters:** Every run of `go test ./...` fails, which masks real
regressions and violates the TDD green-bar requirement.

**Before:**
```go
// research_test.go (no build tag)
func TestTodoistSyncResearch(t *testing.T) {
    apiKey := os.Getenv("TODOIST_API_KEY")
    if apiKey == "" {
        t.Skip("TODOIST_API_KEY not set, skipping research test")
    }
    // ... calls real API that returns 410
```

**After:**
```go
//go:build integration

package api
// ... rest of file unchanged
```

---

### RF-02: Fix correctness bug in `GetTasksByDateRange` — unused `start` param

**Priority:** High
**Effort:** Small
**Risk:** Low

**Current state:**
`store/sqlite.go:GetTasksByDateRange(start, end time.Time)` accepts a `start`
parameter but the SQL query ignores it:

```sql
WHERE due_date IS NOT NULL AND due_date <= ?
  AND completed = FALSE
```

This returns **all tasks with due_date ≤ end**, including tasks from years ago.
The function comment says it intentionally returns overdue tasks, but the `start`
parameter is actively misleading — callers believe they are filtering to a range.

**Target state:**
Either (a) remove the `start` parameter to match actual behavior, or (b) add a
lower bound but exclude items before an arbitrary "overdue cutoff" (e.g., 30
days). Option (a) is simpler.

**Affected files:**
- `internal/store/sqlite.go` — remove `start` from signature
- `internal/handlers/timeline_logic.go` — update call site
- `internal/store/sqlite_test.go` — update test helper calls

**Why it matters:** The signature is a lie. A reader writing a new call with
`start=30 days ago` expects filtering that never happens. This is a latent
correctness bug: timeline items from an indefinite past can appear.

**Before:**
```go
func (s *Store) GetTasksByDateRange(start, end time.Time) ([]models.Task, error) {
    rows, err := s.db.Query(`
        SELECT ...
        FROM tasks
        WHERE due_date IS NOT NULL AND due_date <= ?  -- start ignored!
          AND completed = FALSE
    `, end)
```

**After:**
```go
// GetTasksByDateRange retrieves tasks due on or before end, including overdue.
func (s *Store) GetTasksByDateRange(end time.Time) ([]models.Task, error) {
    rows, err := s.db.Query(`
        SELECT ...
        FROM tasks
        WHERE due_date IS NOT NULL AND due_date <= ?
          AND completed = FALSE
    `, end)
```

---

### RF-03: Extract meal-grouping logic to a shared helper

**Priority:** High
**Effort:** Small
**Risk:** Low

**Current state:**
The meal-grouping algorithm (group meals by date+mealType, combine recipe names,
apply time defaults) appears **verbatim in two places**:

1. `handlers.go:HandleTabMeals` (lines ~1149–1185)
2. `timeline_logic.go:BuildTimeline` (lines ~51–106)

Any change to meal display (e.g., adding ingredients, changing sort order) must
be made in both places. Additionally, the `mealTypeOrder()` function in
`handlers.go` and the SQL `CASE` expression in `store.GetMeals()` both encode
the same ordering (`breakfast=0, lunch=1, dinner=2`).

**Target state:**
Extract a `groupAndSortMeals(meals []models.Meal) []CombinedMeal` helper in a
shared location (e.g., `handlers/meals.go`). Both `HandleTabMeals` and
`BuildTimeline` call this helper. Remove the duplicate `CASE` in `GetMeals()` by
relying on the Go-layer sort (or add a constant).

**Affected files:**
- `internal/handlers/handlers.go` — use helper, remove local grouping code
- `internal/handlers/timeline_logic.go` — use same helper
- New: `internal/handlers/meals.go` (or added to `helpers.go`)

**Why it matters:** DRY. Any meal display change now requires one edit, not two.
This is the highest-value duplication removal in the codebase.

**Before (abbreviated):**
```go
// handlers.go HandleTabMeals
type mealKey struct { date string; mealType string }
mealGroups := make(map[mealKey][]models.Meal)
for _, meal := range meals { ... }
var combined []CombinedMeal
for _, group := range mealGroups { ... }
sort.Slice(combined, ...)

// timeline_logic.go BuildTimeline — identical logic
type mealKey struct { date string; mealType string }
mealGroups := make(map[mealKey][]models.Meal)
for _, meal := range meals { ... }
```

**After:**
```go
// handlers/meals.go
func groupMeals(meals []models.Meal) []CombinedMeal { ... }

// handlers.go — uses helper
combined := groupMeals(meals)

// timeline_logic.go — uses same helper
for _, cm := range groupMeals(meals) { ... }
```

---

### RF-04: Replace inline HTML builders with templates

**Priority:** High
**Effort:** Medium
**Risk:** Low

**Current state:**
Three handlers build and return raw HTML strings instead of using the template
system. This bypasses XSS safety, breaks testability, and is inconsistent with
all other handlers:

1. `handlers.go:HandleGetBugs` (lines ~854–866) — inline `fmt.Fprintf` loop
2. `handlers.go:HandleGetTaskDetail` (lines ~922–934) — 8-line HTML literal
3. `handlers.go:HandleGetListsOptions` (lines ~838–844) — `fmt.Fprintf` loop

All other handlers use `HTMLResponse(w, h.renderer, "template-name", data)`.

**Target state:**
Move each of these to named partial templates. The handlers become 5-line
`HTMLResponse(...)` calls.

**Affected files:**
- `internal/handlers/handlers.go`
- `web/templates/partials/bugs-list.html` (new)
- `web/templates/partials/task-detail.html` (new)
- `web/templates/partials/list-options.html` (new)

**Why it matters:** Inline HTML builders cannot be tested via `assertTemplateContains`. They also defeat Go's auto-escaping, requiring manual `template.HTMLEscapeString()` calls that can be forgotten. One missed escape = XSS.

**Before:**
```go
func (h *Handler) HandleGetBugs(w http.ResponseWriter, r *http.Request) {
    // ...
    w.Header().Set("Content-Type", "text/html")
    if len(bugs) == 0 {
        _, _ = fmt.Fprint(w, `<p class="text-gray-500 text-sm">No bugs reported yet.</p>`)
        return
    }
    for _, bug := range bugs {
        _, _ = fmt.Fprintf(w, `<div class="...">%s</div>`,
            template.HTMLEscapeString(bug.Description), ...)
    }
}
```

**After:**
```go
func (h *Handler) HandleGetBugs(w http.ResponseWriter, r *http.Request) {
    bugs, err := h.store.GetBugs()
    if err != nil {
        JSONError(w, http.StatusInternalServerError, "Failed to fetch bugs", err)
        return
    }
    HTMLResponse(w, h.renderer, "bugs-list", bugs)
}
```

---

### RF-05: Add `GetTask(id)` and `GetCard(id)` store methods

**Priority:** Medium
**Effort:** Small
**Risk:** Low

**Current state:**
`handlers.go:getAtomDetails()` (lines 732–768) needs a single task or card by
ID. It does this by fetching **all** tasks / all boards / all cards and
iterating:

```go
case "todoist":
    if tasks, err := h.store.GetTasks(); err == nil {
        for _, t := range tasks {
            if t.ID == id { return t.Content, t.DueDate }
        }
    }
case "trello":
    if boards, err := h.store.GetBoards(); err == nil {
        for _, b := range boards {
            for _, c := range b.Cards {
                if c.ID == id { return c.Name, c.DueDate }
            }
        }
    }
```

`HandleGetTaskDetail` (lines ~888–935) has the same pattern for fetching a
single task.

**Target state:**
Add `GetTask(id string) (*models.Task, error)` and `GetCard(id string)
(*models.Card, error)` to the store. Use indexed SQL lookups instead of O(n)
Go iteration.

**Affected files:**
- `internal/store/sqlite.go` — add two methods
- `internal/handlers/handlers.go` — use new methods in `getAtomDetails` and `HandleGetTaskDetail`
- `internal/store/sqlite_test.go` — add tests for new methods

**Why it matters:** Each task completion currently reads ALL tasks from SQLite.
More importantly, it's semantically wrong — these are O(1) operations disguised
as O(n).

---

### RF-06: Eliminate `convertSyncItemToTask` wrapper

**Priority:** Medium
**Effort:** Small
**Risk:** Low

**Current state:**
`handlers.go:convertSyncItemToTask` (lines 412–430) wraps a single item in a
slice, calls `ConvertSyncItemsToTasks`, and unwraps:

```go
func (h *Handler) convertSyncItemToTask(item api.SyncItemResponse, projectMap map[string]string) models.Task {
    items := api.ConvertSyncItemsToTasks([]api.SyncItemResponse{item}, projectMap)
    if len(items) > 0 {
        return items[0]
    }
    // Fallback: reconstruct manually (same logic duplicated)
    return models.Task{ ... }
}
```

The fallback path also duplicates the field mapping from `ConvertSyncItemsToTasks`.

**Target state:**
Export a `ConvertSyncItemToTask(item SyncItemResponse, projectMap map[string]string) (models.Task, bool)` function in the `api` package, returning `(task, wasActive)`. The handler calls this directly for incremental sync items. The `convertSyncItemToTask` method on Handler is deleted.

**Affected files:**
- `internal/api/todoist.go` — add single-item converter
- `internal/handlers/handlers.go` — remove method, use api package function
- `internal/api/todoist_test.go` — add test for single-item converter

**Why it matters:** Clarity and correctness. The current fallback silently drops due-date parsing for the single-item case (note: `ConvertSyncItemsToTasks` skips completed/deleted items, so the fallback IS reachable for completed items being reported by incremental sync).

---

### RF-07: Remove `GetMealsByDateRange` wrapper

**Priority:** Low
**Effort:** Tiny
**Risk:** None

**Current state:**
```go
// sqlite.go
func (s *Store) GetMealsByDateRange(start, end time.Time) ([]models.Meal, error) {
    return s.GetMeals(start, end)
}
```

This is a one-line wrapper with identical signature and semantics to `GetMeals`.
Both exist in the public API of Store, creating confusion about which to call.

**Target state:**
Delete `GetMealsByDateRange`. All callers use `GetMeals` directly. (Only one
caller exists: `timeline_logic.go`.)

**Affected files:**
- `internal/store/sqlite.go` — remove method
- `internal/handlers/timeline_logic.go` — call `GetMeals` directly

---

### RF-08: Standardize `ParseForm` + field extraction pattern

**Priority:** Low
**Effort:** Medium
**Risk:** Low

**Current state:**
Most handlers ignore the `requireFormValue` helper defined in `helpers.go` and
manually call `r.ParseForm()` + `r.FormValue()` with explicit nil checks.
`parseFormOr400` is used in only 2 of ~20 handler functions. The pattern varies:

```go
// Pattern A (most common):
if err := r.ParseForm(); err != nil {
    JSONError(w, http.StatusBadRequest, "Failed to parse form", err)
    return
}
id := r.FormValue("id")
if id == "" {
    JSONError(w, http.StatusBadRequest, "Missing id", nil)
    return
}

// Pattern B (2 handlers):
if !parseFormOr400(w, r) { return }
id := r.FormValue("id")
if id == "" { ... }

// Pattern C (0 usages):
id, ok := requireFormValue(w, r, "id")
if !ok { return }
```

**Target state:**
Migrate all handlers to Pattern C (`requireFormValue`) where applicable. This
reduces handler boilerplate from 6-8 lines to 2. Add a test for `requireFormValue`.

**Affected files:**
- `internal/handlers/handlers.go`
- `internal/handlers/settings.go`
- `internal/handlers/shopping.go`
- `internal/handlers/helpers.go` — add tests

---

### RF-09: Add `//go:build integration` tag to Trello research test

**Priority:** Low
**Effort:** Tiny
**Risk:** None

**Current state:**
`TestTrelloOptimizationResearch` also makes real network calls (it passes
currently only because the Trello token in the environment is valid). It should
share the `integration` build tag applied in RF-01.

**Affected files:**
- `internal/api/research_test.go`

---

### RF-10: Investigate Todoist Sync v9 endpoint status

**Priority:** Medium
**Effort:** Small
**Risk:** Medium

**Current state:**
`api/todoist.go` uses `todoistSyncBaseURL = "https://api.todoist.com/sync/v9"`
for the incremental sync client. The research test reveals the v9 sync endpoint
returns 410 when called. However, the production sync tests pass because they
use mocked HTTP servers.

**Why it matters:** If `/sync/v9/sync` is truly deprecated, the production sync
path will fail silently (falling back to stale cache). This needs live
verification.

**Target state:**
Add an integration test for `TodoistClient.Sync()` against the real API (behind
the `integration` build tag) to confirm the endpoint works or identify the
correct migration path.

**Affected files:**
- `internal/api/todoist_test.go` — add integration test

---

## 4. Test Coverage Gaps

The following areas have no tests or critically insufficient coverage:

| Area | Coverage | Gap |
|------|----------|-----|
| `internal/api` — Google Calendar client | ~0% | No unit tests for `GetUpcomingEvents`, `parseEventTime`, `deduplicateEvents` |
| `internal/api` — Google Tasks client | ~0% | No unit tests for `GetTasks`, `GetTasksByDateRange`, `CompleteTask` |
| `internal/auth` — middleware | 20.4% | Session load/save, CSRF token injection not directly tested |
| `internal/handlers` — Planning tab | 0% | `HandleTabPlanning` has complex logic (3 sort passes, dual categorization) with no test |
| `internal/handlers` — Meals tab | 0% | `HandleTabMeals` with meal grouping has no test |
| `internal/handlers` — Bug handlers | 0% | `HandleGetBugs`, `HandleReportBug` have no tests |
| `internal/handlers` — Task detail | 0% | `HandleGetTaskDetail`, `HandleUpdateTask` have no tests |
| `internal/handlers` — List options | 0% | `HandleGetListsOptions` has no test |
| `internal/store` — agent session expiry | partial | Expired session handling not tested |
| `internal/store` — `GetCompletedTasks` | partial | Time parsing for SQLite format not regression-tested |

### Priority test additions

1. **Google Calendar unit test** — mock the Google API client, test
   `deduplicateEvents` and `parseEventTime` in isolation (these are pure
   functions with no HTTP dependency).

2. **`HandleTabPlanning` test** — this handler has the most branching logic in
   the codebase (events/tasks/cards across 3 time windows, 2 categorizations).
   One happy-path integration test and one edge case (item right on the `tomorrow` boundary).

3. **`GetCompletedTasks` time parsing test** — the function parses SQLite's
   `"2006-01-02 15:04:05"` format manually. A test should verify this doesn't
   silently zero-out `CompletedAt` on timezone changes.

4. **Auth middleware coverage** — expand to test CSRF token injection and session
   creation edge cases.

5. **Google Tasks unit tests** — `GetTasksByDateRange` applies date filtering in
   Go (not SQL), which makes it particularly important to test boundary conditions.

---

## 5. Known Gaps from SESSION_STATE.md

| Known Gap | Status in This Plan |
|-----------|---------------------|
| Google Tasks API client lacks dedicated unit tests | Addressed in RF-10 + Test Gap §1 |
| WebAuthn requires env vars in production | Not a code issue — deploy concern only |
| Agent write operations (Phase 2, 3) | Out of scope for refactoring |

Additional finding not in SESSION_STATE:

- **`GetTasksByDateRange` silently drops `start` filter** (RF-02) — this was
  not previously identified. The overdue-task comment masks the bug.
- **Research test breaks CI** (RF-01) — not documented anywhere.

---

## 6. Do-Not-Refactor List

| Area | Reason |
|------|--------|
| `CacheFetcher[T]` generic pattern | Well-designed, tested, and stable. |
| `Atom` model + `ComputeUIFields` | Correct and well-tested. |
| `handleAtomToggle` dispatch switch | Clear, tested, and covers all sources. |
| Agent auth flow (`agent.go`) | Comprehensive tests, Phase 2/3 coming soon. |
| WebSocket hub (`websocket.go`) | Works correctly; testing requires goroutine coordination. Leave for now. |
| Migration system (`runMigrations`) | Simple and correct. SQLite doesn't support transactional DDL. |
| `sortTasksByUrgency` / `SortAtomsByUrgency` | Two different sort strategies for two views — not duplication. |
| Auth bcrypt handlers | Correct and the coverage is what it is given bcrypt timing. |

---

## 7. Implementation Roadmap

### Phase 1 — High-impact, zero-regression risk (do first)

1. **RF-01** — Add `//go:build integration` to research_test.go → fixes CI
2. **RF-09** — Tag Trello research test
3. **RF-02** — Remove unused `start` param from `GetTasksByDateRange`
4. **RF-07** — Delete `GetMealsByDateRange` wrapper
5. **Add Google Calendar unit tests** (deduplicateEvents, parseEventTime — pure functions)

### Phase 2 — Medium-impact improvements

6. **RF-05** — Add `GetTask(id)` and `GetCard(id)` store methods with tests
7. **RF-06** — Eliminate `convertSyncItemToTask` wrapper
8. **RF-03** — Extract shared `groupMeals()` helper
9. **RF-10** — Add Todoist Sync v9 integration test to confirm endpoint health
10. **Add `HandleTabPlanning` test** (happy path + boundary)
11. **Add `HandleTabMeals` test**

### Phase 3 — Template migration + broader coverage

12. **RF-04** — Move inline HTML builders to templates; add tests for each
13. **RF-08** — Standardize form parsing with `requireFormValue`
14. **Add Google Tasks unit tests** (date-range filtering, completion)
15. **Expand auth middleware tests**
16. **Add `HandleGetBugs`, `HandleReportBug` tests**

---

## Appendix: Specific Code Locations

| Issue | File | Lines |
|-------|------|-------|
| Failing research test | `internal/api/research_test.go` | 1, 20 |
| `start` param ignored | `internal/store/sqlite.go` | 741–748 |
| Meal grouping duplicate #1 | `internal/handlers/handlers.go` | 1149–1185 |
| Meal grouping duplicate #2 | `internal/handlers/timeline_logic.go` | 51–106 |
| Inline HTML: bugs | `internal/handlers/handlers.go` | 854–866 |
| Inline HTML: task detail | `internal/handlers/handlers.go` | 922–934 |
| Inline HTML: list options | `internal/handlers/handlers.go` | 838–844 |
| `convertSyncItemToTask` | `internal/handlers/handlers.go` | 412–430 |
| `GetAtomDetails` O(n) scan | `internal/handlers/handlers.go` | 732–768 |
| `GetMealsByDateRange` wrapper | `internal/store/sqlite.go` | 757–759 |
| `requireFormValue` unused | `internal/handlers/helpers.go` | 19–26 |