docs(03): add code review report
This commit is contained in:
Arthur Belleville
parent
a420a9c45c
commit
7b945652d3
1 changed files with 255 additions and 0 deletions
255
.planning/phases/03-tablos-crud/03-REVIEW.md
Normal file
255
.planning/phases/03-tablos-crud/03-REVIEW.md
Normal file
|
|
@ -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 `<form>` 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 `<form>` 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" `<form>` 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 `<input name="description" value="{tablo.Description.String}">` 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_
|
||||
Loading…
Reference in a new issue