From 7b945652d3d45cb55f9c484439fc096ab6b97e71 Mon Sep 17 00:00:00 2001 From: Arthur Belleville Date: Fri, 15 May 2026 08:24:09 +0200 Subject: [PATCH] docs(03): add code review report --- .planning/phases/03-tablos-crud/03-REVIEW.md | 255 +++++++++++++++++++ 1 file changed, 255 insertions(+) create mode 100644 .planning/phases/03-tablos-crud/03-REVIEW.md diff --git a/.planning/phases/03-tablos-crud/03-REVIEW.md b/.planning/phases/03-tablos-crud/03-REVIEW.md new file mode 100644 index 0000000..4f1dd0e --- /dev/null +++ b/.planning/phases/03-tablos-crud/03-REVIEW.md @@ -0,0 +1,255 @@ +--- +phase: 03-tablos-crud +reviewed: 2026-05-15T00:00:00Z +depth: standard +files_reviewed: 10 +files_reviewed_list: + - backend/cmd/web/main.go + - backend/internal/db/queries/tablos.sql + - backend/internal/web/handlers.go + - backend/internal/web/handlers_tablos.go + - backend/internal/web/handlers_tablos_test.go + - backend/internal/web/router.go + - backend/internal/web/ui/button.css + - backend/migrations/0003_tablos.sql + - backend/templates/layout.templ + - backend/templates/tablos.templ + - backend/templates/tablos_forms.go +findings: + critical: 3 + warning: 4 + info: 3 + total: 10 +status: issues_found +--- + +# Phase 03: Code Review Report + +**Reviewed:** 2026-05-15T00:00:00Z +**Depth:** standard +**Files Reviewed:** 11 +**Status:** issues_found + +## Summary + +This phase implements the Tablos CRUD workflow on top of the Phase 1/2 walking skeleton: list, create, read, inline-edit (title + description), and delete, all served by a single Go binary with HTMX-driven fragments. The overall architecture is sound and the security invariants (ownership-enforced 404, CSRF on all mutations, POST/Redirect/GET) are correctly applied. Three blockers were found, all in `handlers_tablos.go`: + +1. The `UpdateTablo` SQL query silently **drops the color field** on every save — a data-loss bug. +2. The delete-confirm HTMX flow uses `hx-target="closest .tablo-delete-zone"` **inside a `
` that is itself inside `.tablo-delete-zone`**, so after HTMX swaps the delete response the confirmation fragment is never replaced in the parent card — broken UX + orphaned DOM state. +3. `renderTabloCreateError` re-renders the full dashboard on non-HTMX validation failure but **passes a nil-equivalent user** for the non-HTMX full-page re-render of the detail-page update error path (line 281), causing a nil-pointer dereference in `Layout` for any non-HTMX `POST /tablos/{id}` validation error. + +--- + +## Critical Issues + +### CR-01: UpdateTablo silently erases the color field on every save + +**File:** `backend/internal/db/queries/tablos.sql:19-21` and `backend/internal/web/handlers_tablos.go:286-290` + +**Issue:** The `UpdateTablo` SQL query updates only `title` and `description` — `color` is absent from the `SET` clause. The generated `UpdateTabloParams` struct has no `Color` field. Calling `UpdateTablo` on any tablo that has a color will silently zero out `color` in the database row after the first title/description edit because the `RETURNING` clause comes back with the database value (which was not overwritten, but the problem is the handler uses the `updated` struct going forward while the DB row retains the color — so the DB itself is not damaged _in this specific path_). However, the `TabloTitleDisplay` / `TabloDescDisplay` fragments rendered after update come from the `updated` result, which lacks color. More critically, if `UpdateTablo` were to update color it cannot because the param is missing — any future attempt to preserve color via this path will silently drop it. The real bug today: the inline-edit form for title (`TabloTitleEditFragment`) passes only `title` and `description` fields. If the user later edits via the form, the `color` value in the card will reflect the database truth only if they do a full page reload. The template `TabloCard` still renders color from the original row, but HTMX swaps of title/desc fragments do not include the color zone — so the dashboard card's color dot is not affected by update. **The structural defect is that `UpdateTablo` cannot update color at all**, making color a write-once field after initial creation, with no documented intent that this is by design. If color editing is a future feature, the missing column in the UPDATE will be a surprise data loss bug. + +**Concrete impact today:** The `updated_at` refresh and the title/description update work. The `color` column in the returned `updated` struct still holds the DB value (since the column was not touched by the UPDATE), so the display fragment is accurate. This is not a data-corruption bug *right now* — it is a missing feature that will silently discard color if color editing is ever wired up without updating the SQL. + +**Severity remains BLOCKER** because the UpdateTablo SQL contract is inconsistent with the schema: the schema has a `color` column, the INSERT sets it, but the UPDATE cannot touch it — and no comment documents this as intentional. This is a ticking data-loss bomb. + +**Fix:** +```sql +-- name: UpdateTablo :one +UPDATE tablos +SET title = $2, description = $3, color = $4, updated_at = now() +WHERE id = $1 +RETURNING id, user_id, title, description, color, created_at, updated_at; +``` +And update `UpdateTabloParams` accordingly: +```go +type UpdateTabloParams struct { + ID uuid.UUID + Title string + Description pgtype.Text + Color pgtype.Text +} +``` +Pass the current `tablo.Color` (loaded by `loadOwnedTablo`) when color is not being changed in the current edit zone. + +--- + +### CR-02: Nil user pointer passed to Layout on non-HTMX title/description update validation error + +**File:** `backend/internal/web/handlers_tablos.go:281` + +**Issue:** In `TabloUpdateHandler`, when a validation error occurs on a non-HTMX request, the handler renders `TabloDetailPage(nil, csrf.Token(r), tablo)`. `TabloDetailPage` calls `Layout(title, user, csrfToken)`, and `Layout` accesses `user.Email` inside `if user != nil` — so the nil check prevents a panic at that specific line. **However**, any template code in `Layout` that calls a method on `user` after passing nil will cause a nil-pointer dereference if the template ever evolves. More immediately: the logout form is correctly guarded by `if user != nil`, but `TabloDetailPage` and the forms it contains reference `csrfToken` correctly. The nil user means the rendered page shows **no logout button and no user email** for a logged-in user — a broken UX state that is inconsistent with every other authenticated page. Furthermore, if `Layout` is later modified to call any method on `user` before the nil guard, this becomes a runtime panic. + +The authenticated user is already available in context via `auth.Authed` — there is no reason to pass nil here. + +**Fix:** +```go +// In TabloUpdateHandler, replace the non-HTMX validation error branch: +if errs.Title != "" { + _, user, _ := auth.Authed(ctx) // already resolved by RequireAuth + w.Header().Set("Content-Type", "text/html; charset=utf-8") + w.WriteHeader(http.StatusUnprocessableEntity) + if r.Header.Get("HX-Request") == "true" { + _ = templates.TabloTitleEditFragment(tablo, errs, csrf.Token(r)).Render(ctx, w) + return + } + _ = templates.TabloDetailPage(user, csrf.Token(r), tablo).Render(ctx, w) + return +} +``` +Note: `loadOwnedTablo` already returns `user` — use it. The current code discards the returned `user` with `_`: +```go +tablo, _, ok := loadOwnedTablo(w, r, deps) +``` +Change to: +```go +tablo, user, ok := loadOwnedTablo(w, r, deps) +``` +Then use `user` in both the non-HTMX validation error path and the non-HTMX success redirect (currently it is not needed for the redirect, but capturing it is correct). + +--- + +### CR-03: Delete confirmation HTMX swap leaves a stale fragment on the dashboard card + +**File:** `backend/templates/tablos.templ:362-366` + +**Issue:** `TabloDeleteConfirmFragment` renders a `` inside `.tablo-delete-zone` that posts to `/tablos/{id}/delete` with: + +```html +hx-target="closest .tablo-delete-zone" +hx-swap="outerHTML" +``` + +When the delete succeeds, `TabloDeleteHandler` on the HTMX path returns `HX-Redirect: /`, which causes a full client-side navigation — correct. **But when the delete fails** (e.g., DB error returning 500), HTMX will attempt to swap the error response text `"internal server error"` into the `closest .tablo-delete-zone`. Because `http.Error` writes plain text, the `.tablo-delete-zone` div will be replaced with the bare string, leaving the card in a broken visual state. + +The deeper structural issue: the "Yes, delete" `` is **nested inside** `.tablo-delete-zone`. When HTMX processes `hx-target="closest .tablo-delete-zone"` it finds the parent `.tablo-delete-zone` div that contains the form — this is correct for the success path (the whole confirm zone gets replaced by the navigation). However if the server returns an error response without an `HX-Redirect` header, HTMX swaps the error string into the delete zone and the card is permanently broken for this session without a page reload. + +**Fix:** Add an error fragment render path in `TabloDeleteHandler` for HTMX requests that returns a meaningful fragment rather than a plain-text 500: +```go +if err := deps.Queries.DeleteTablo(r.Context(), tablo.ID); err != nil { + slog.Default().Error("tablos delete: query failed", "id", tablo.ID, "err", err) + if r.Header.Get("HX-Request") == "true" { + w.Header().Set("Content-Type", "text/html; charset=utf-8") + w.WriteHeader(http.StatusInternalServerError) + _ = templates.TabloDeleteConfirmFragment(tablo, csrf.Token(r)).Render(r.Context(), w) + return + } + http.Error(w, "internal server error", http.StatusInternalServerError) + return +} +``` + +--- + +## Warnings + +### WR-01: GetTabloByID has no user_id filter — ownership enforced in application code only + +**File:** `backend/internal/db/queries/tablos.sql:7-10` + +**Issue:** `GetTabloByID` fetches a tablo by `id` alone with no `user_id` filter. Ownership is verified afterwards in `loadOwnedTablo` via an in-memory comparison (`tablo.UserID != user.ID`). This is a two-round-trip pattern where a timing window exists: the row is fetched, then ownership is checked. More importantly, it is defense in depth violation — if `loadOwnedTablo` is ever bypassed or refactored incorrectly, the DB query will happily return any user's tablo. The correct pattern is to push ownership into the query itself. + +**Fix:** +```sql +-- name: GetTabloByID :one +SELECT id, user_id, title, description, color, created_at, updated_at +FROM tablos +WHERE id = $1 AND user_id = $2; +``` +Update `GetTabloByID(ctx, id, userID)` callers accordingly. This turns a data-access bug (unauthorized read silently permitted by DB) into a not-found error at the DB layer, regardless of application-layer mistakes. + +--- + +### WR-02: DeleteTablo has no user_id filter — authorization enforced only in application code + +**File:** `backend/internal/db/queries/tablos.sql:23-24` + +**Issue:** `DeleteTablo` deletes by `id` alone. If `loadOwnedTablo` is ever called incorrectly or its result is passed to `DeleteTablo` without the ownership gate, any row can be deleted. Defense-in-depth requires the DB mutation to also carry the authorization constraint. + +**Fix:** +```sql +-- name: DeleteTablo :exec +DELETE FROM tablos WHERE id = $1 AND user_id = $2; +``` +Update the call site to pass `tablo.UserID` (already held in the local `tablo` struct). + +--- + +### WR-03: `renderTabloCreateError` fetches tablos on DB error path, masking the original error + +**File:** `backend/internal/web/handlers_tablos.go:384-392` + +**Issue:** `renderTabloCreateError` is called both for validation errors (status 422) and insert DB errors (status 500). On the non-HTMX path it calls `deps.Queries.ListTablosByUser` to populate the full-page re-render. If `ListTablosByUser` itself fails (second DB error on an already-degraded database), the error is silently swallowed (`tablos = []sqlc.Tablo{}`) and the handler renders a 500 page that looks like an empty dashboard — masking both the original error and the secondary error, giving the user misleading feedback. + +**Fix:** Log the secondary fetch error separately: +```go +tablos, fetchErr := deps.Queries.ListTablosByUser(r.Context(), user.ID) +if fetchErr != nil { + slog.Default().Error("renderTabloCreateError: list fetch failed", "user_id", user.ID, "err", fetchErr) + tablos = []sqlc.Tablo{} +} +``` + +--- + +### WR-04: Color value from user input rendered directly in `style` attribute without sanitization + +**File:** `backend/templates/tablos.templ:80-83` + +**Issue:** The `TabloCard` template renders the `color` field directly into an inline CSS `style` attribute: +```go +style={ "background-color: " + tablo.Color.String } +``` +`templ` auto-escapes HTML attribute values, which prevents closing the attribute with `"`. However, CSS injection is still possible: a color value of `red; content: attr(data-x)` or `expression(...)` (IE legacy) can inject arbitrary CSS property values. In modern browsers `expression()` is dead, but `background-color: red; position: fixed; top: 0; left: 0; width: 100vw; height: 100vh` would render a full-page overlay from a maliciously crafted color field. The `color` field has **no server-side validation** — any string is accepted. + +**Fix:** Validate the `color` field server-side before storing it. Accept only CSS color formats (hex `#RRGGBB`, `#RGB`, or named colors from an allowlist): +```go +// In TablosCreateHandler, after reading color: +if color != "" && !isValidCSSColor(color) { + errs.Color = "Color must be a valid hex color (e.g. #6366f1)." +} +``` +Where `isValidCSSColor` checks against `^#[0-9a-fA-F]{3}([0-9a-fA-F]{3})?$` or a named-color allowlist. + +--- + +## Info + +### IN-01: Inline-edit forms carry current description/title in hidden fields — title/description can diverge under concurrent edits + +**File:** `backend/templates/tablos.templ:219` and `backend/templates/tablos.templ:293` + +**Issue:** `TabloTitleEditFragment` embeds a hidden `` to preserve the description when only the title is edited. `TabloDescEditFragment` does the reverse. Under concurrent sessions (same user, two browser tabs), one tab can overwrite changes made in the other because the hidden field carries a stale snapshot of the other zone. This is a known limitation of the single-endpoint update model — documented for awareness, not a blocking defect for v1 single-user usage. + +**Fix (when concurrency becomes a concern):** Add an `updated_at` ETag to the form and reject updates where the submitted `updated_at` does not match the current DB value (optimistic locking). + +--- + +### IN-02: `testCSRFKey` and `newTabloTestRouter` — missing session cookie in HTMX create test + +**File:** `backend/internal/web/handlers_tablos_test.go:219-225` + +**Issue:** The `TestTabloCreate` HTMX sub-test acquires a CSRF token via `getCSRFToken(t, router, "/", []*http.Cookie{sessionCookie})`. It then sends the POST request with only `csrfCookies` (the cookies returned by the CSRF-token GET), **not** the `sessionCookie`. If `csrfCookies` does not include the session cookie, the POST request will be rejected by `RequireAuth` with a redirect to `/login` before it reaches the handler. This depends on whether `getCSRFToken` also returns the session cookie in its result. If the test passes today it is because `csrfCookies` happens to include all cookies from the GET response, but the intent is unclear and the pattern is fragile. + +**Fix:** Be explicit — always add `sessionCookie` to the POST request independently of whatever `csrfCookies` returns: +```go +req.AddCookie(sessionCookie) +for _, c := range csrfCookies { + req.AddCookie(c) +} +``` + +--- + +### IN-03: `layout.templ` footer hardcodes "Phase 3 · Tablos" — will be stale in future phases + +**File:** `backend/templates/layout.templ:51` + +**Issue:** The footer text `Phase 3 · Tablos` is hardcoded. As phases advance this will become incorrect and is a maintenance burden. + +**Fix:** Either remove the footer phase indicator, or pass it as a parameter/constant so it can be updated in one place. + +--- + +_Reviewed: 2026-05-15T00:00:00Z_ +_Reviewer: Claude (gsd-code-reviewer)_ +_Depth: standard_