diff options
Diffstat (limited to 'REFACTOR_PLAN.md')
| -rw-r--r-- | REFACTOR_PLAN.md | 588 |
1 files changed, 588 insertions, 0 deletions
diff --git a/REFACTOR_PLAN.md b/REFACTOR_PLAN.md new file mode 100644 index 0000000..861803f --- /dev/null +++ b/REFACTOR_PLAN.md @@ -0,0 +1,588 @@ +# 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 | + |