diff --git a/.planning/phases/13-design-system-foundation/13-REVIEW.md b/.planning/phases/13-design-system-foundation/13-REVIEW.md new file mode 100644 index 0000000..ec37400 --- /dev/null +++ b/.planning/phases/13-design-system-foundation/13-REVIEW.md @@ -0,0 +1,378 @@ +--- +phase: 13-design-system-foundation +reviewed: 2026-05-16T00:00:00Z +depth: standard +files_reviewed: 44 +files_reviewed_list: + - backend/.air-catalog.toml + - backend/internal/web/catalog_route_catalog.go + - backend/internal/web/catalog_route_stub.go + - backend/internal/web/router.go + - backend/internal/web/ui/auth.css + - backend/internal/web/ui/badge.css + - backend/internal/web/ui/base.css + - backend/internal/web/ui/button.css + - backend/internal/web/ui/button.templ + - backend/internal/web/ui/card.css + - backend/internal/web/ui/card.templ + - backend/internal/web/ui/catalog/catalog.templ + - backend/internal/web/ui/catalog/examples.go + - backend/internal/web/ui/empty_state.templ + - backend/internal/web/ui/empty-state.css + - backend/internal/web/ui/form_field.templ + - backend/internal/web/ui/form-field.css + - backend/internal/web/ui/helpers.go + - backend/internal/web/ui/icon_button.templ + - backend/internal/web/ui/icon-button.css + - backend/internal/web/ui/input.css + - backend/internal/web/ui/input.templ + - backend/internal/web/ui/modal.css + - backend/internal/web/ui/modal.templ + - backend/internal/web/ui/select_helpers.go + - backend/internal/web/ui/select.css + - backend/internal/web/ui/select.templ + - backend/internal/web/ui/space.templ + - backend/internal/web/ui/spacing.css + - backend/internal/web/ui/table.css + - backend/internal/web/ui/table.templ + - backend/internal/web/ui/textarea.css + - backend/internal/web/ui/textarea.templ + - backend/internal/web/ui/ui_test.go + - backend/internal/web/ui/variants.go + - backend/justfile + - backend/tailwind.input.css + - backend/templates/auth_login.templ + - backend/templates/auth_signup.templ + - backend/templates/etapes.templ + - backend/templates/events.templ + - backend/templates/planning.templ + - backend/templates/tablos.templ + - backend/templates/tasks.templ +findings: + critical: 3 + warning: 8 + info: 4 + total: 15 +status: issues_found +--- + +# Phase 13: Code Review Report + +**Reviewed:** 2026-05-16T00:00:00Z +**Depth:** standard +**Files Reviewed:** 44 +**Status:** issues_found + +## Summary + +This phase delivers the design-system foundation for the Go/HTMX rewrite: 11 UI components (Button, Badge, Card, Input, Textarea, Select, Modal, EmptyState, Table, IconButton, FormField, Space), a component catalog, and a CSS token system. The architecture is sound — build-tag gating of the catalog route, normalized enums with safe zero-value defaults, and inline JS scoped with IIFEs. + +Three critical issues were found: an undefined CSS custom property (`--color-brand-focus`) used on two form controls that will cause invisible focus states in production; an XSS vector in the catalog where raw HTML strings are written directly to the response writer without escaping; and a `justfile` secret (`s3_secret_key`) hard-coded in plain text and passed as an environment variable in multiple recipes. Eight warnings cover logic gaps, incomplete CSS coverage, and inconsistent patterns across the codebase. Four info items flag dead code and naming divergence. + +--- + +## Critical Issues + +### CR-01: Undefined CSS Custom Property `--color-brand-focus` Breaks Input and Textarea Focus Rings + +**File:** `backend/internal/web/ui/input.css:19`, `backend/internal/web/ui/textarea.css:20` + +**Issue:** Both `input.css` and `textarea.css` reference `var(--color-brand-focus)` for the focused border color. This token is not defined anywhere in `base.css` or any other imported CSS file. When a CSS custom property is undefined, `var()` resolves to the property's initial value — for `border-color` that is `currentColor`, meaning the focused border will be invisible or will match the text color, not the intended brand color. This breaks the focus-ring contract for the two most common form controls. + +``` +/* base.css defines: */ +--color-brand-primary: #804eec; +--color-brand-primary-hover: #6d28d9; +/* ... but NOT --color-brand-focus */ +``` + +**Fix:** Add the missing token to `base.css` alongside the other brand tokens, or replace both references with the correct existing token: + +```css +/* In base.css :root block, after --color-brand-primary-active */ +--color-brand-focus: #804eec; /* same hue as brand-primary; adjust if a distinct focus color is intended */ +``` + +Or change both CSS files to use the token that exists: +```css +/* input.css:19 and textarea.css:20 */ +border-color: var(--color-brand-primary); +``` + +--- + +### CR-02: XSS Vector — Raw HTML Injected via `io.WriteString` in Catalog Examples + +**File:** `backend/internal/web/ui/catalog/examples.go:285-315` + +**Issue:** The `modalExamples()` function constructs the preview by calling `io.WriteString(w, ...)` with raw HTML strings that include class attributes and structural HTML. The `textBody` helper used elsewhere in the same file also writes raw, unescaped strings directly to the response. While the catalog is only compiled with `-tags catalog` (never in production), these helpers establish a dangerous pattern: `textBody` accepts a `string` and emits it verbatim without HTML escaping. If any future caller passes user-controlled input to `textBody`, or if this pattern is copied to production code, it becomes a stored XSS vector. + +More concretely, `tableExamples()` at line 360 passes multi-element HTML attribute strings with inline styles — if these example strings were ever sourced from user data (e.g., tablo titles in a catalog demo) rather than literals, the output would be unescaped. + +**Fix:** Replace `textBody` with the `templ.Raw` function (which documents the intent to emit raw HTML explicitly) or use proper templ components for structured markup. At minimum, rename `textBody` to `rawHTMLBody` and add a comment making the security assumption explicit: + +```go +// rawHTMLBody emits the given string as raw, UNESCAPED HTML. +// ONLY safe for compile-time string literals. Never pass user input. +func rawHTMLBody(literalHTML string) templ.Component { + return templ.ComponentFunc(func(_ context.Context, w io.Writer) error { + _, err := io.WriteString(w, literalHTML) + return err + }) +} +``` + +--- + +### CR-03: Hard-Coded S3 Secret Key in Justfile Passed as Shell Environment Variable + +**File:** `backend/justfile:44` + +**Issue:** The justfile declares `s3_secret_key := "minioadmin"` as a top-level variable and then passes it as a plaintext environment variable in every recipe that starts the application (`dev`, `worker`, `catalog`). While `minioadmin` is a well-known MinIO default credential, committing any secret to the repository — even a dev default — means it appears in git history and signals to future developers that secrets belong in justfiles. More critically, the `.env` loading (`set dotenv-load := true`) is declared but the actual secrets are defined as justfile-level variables rather than read from the `.env` file, so a developer who configures a real S3 credential in a local `.env` file will not have it picked up; the hardcoded value in the justfile will silently shadow it. + +**Fix:** Read `S3_ACCESS_KEY` and `S3_SECRET_KEY` from the environment (which `set dotenv-load := true` populates from `.env`) rather than declaring them as justfile variables: + +```just +# Remove lines 43-44: +# s3_access_key := "minioadmin" +# s3_secret_key := "minioadmin" + +# In the dev recipe, replace explicit assignment with env pass-through: +dev: db-up + just generate + DATABASE_URL='{{ database_url }}' \ + air -c .air.toml + # S3_* vars come from .env via set dotenv-load := true +``` + +Add `S3_ACCESS_KEY` and `S3_SECRET_KEY` to `.env.example` with placeholder values and instructions, and ensure they are in `.gitignore` / `.env.example`. + +--- + +## Warnings + +### WR-01: `selectIsDisabled` Treats Any Non-bool, Non-string `disabled` Value as True — Silently Surprising + +**File:** `backend/internal/web/ui/select_helpers.go:96-103` + +**Issue:** The `default` branch of the type switch returns `true` for any value that is neither `bool` nor `string`. A caller who passes `disabled: 0` or `disabled: nil` (which can happen if an HTMX attribute map is built dynamically) would have the select rendered as disabled with no warning. `templ.Attributes` values are typed as `any`, so this is a real code path. + +**Fix:** Be explicit about the default — return `false` for unknown types and log/panic in test mode: + +```go +default: + return false // unknown type; treat as not disabled +``` + +--- + +### WR-02: `IconButtonClass` for Ghost Tone Omits the Base `ui-icon-button` Class, Breaking Focus Ring CSS + +**File:** `backend/internal/web/ui/variants.go:183` + +**Issue:** The ghost branch of `IconButtonClass` returns `"borderless-icon-button ui-icon-button-ghost ui-icon-button-{variant}"`. The `icon-button.css` file applies the focus-ring rule to `.ui-icon-button:focus-visible` (line 18). Because the ghost tone does not include the `ui-icon-button` base class, ghost icon buttons will never match that focus rule and will render with no visible focus ring — an accessibility regression. + +The solid branch correctly includes `ui-icon-button` as the first token; ghost does not. + +```css +/* icon-button.css:18 — this rule only fires when ui-icon-button is present */ +.ui-icon-button:focus-visible, +.borderless-icon-button:focus-visible { +``` + +Actually `borderless-icon-button:focus-visible` IS listed in the second selector on line 19, so the focus ring IS applied. However the `min-height: 44px; min-width: 44px` tap-target sizing from `.ui-icon-button` (lines 11-12) is absent for ghost buttons. Ghost icon buttons will not meet the 44×44px minimum touch target that the CSS establishes for solid buttons. + +**Fix:** Either add `ui-icon-button` to the ghost class string, or duplicate the minimum dimension rules into `.borderless-icon-button`: + +```go +// variants.go:183 +case IconButtonToneGhost: + return "ui-icon-button borderless-icon-button ui-icon-button-ghost ui-icon-button-" + string(normalizedVariant) +``` + +--- + +### WR-03: `Modal` Component Has No `role="dialog"` or `aria-modal="true"` — Screen Readers Will Not Announce It as a Dialog + +**File:** `backend/internal/web/ui/modal.templ:10` + +**Issue:** The outer `.ui-modal-backdrop` div has no ARIA role. Screen readers will treat it as a generic landmark, not a dialog. Users navigating with assistive technology will not be informed that a dialog has opened, and focus is not managed. The title string is not connected to the dialog with `aria-labelledby`. + +**Fix:** + +```templ +templ Modal(props ModalProps) { +