docs(13): add code review report
This commit is contained in:
Arthur Belleville
parent
9d17acbe2b
commit
9dcf628322
1 changed files with 378 additions and 0 deletions
378
.planning/phases/13-design-system-foundation/13-REVIEW.md
Normal file
378
.planning/phases/13-design-system-foundation/13-REVIEW.md
Normal file
|
|
@ -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) {
|
||||
<div class="ui-modal-backdrop" role="dialog" aria-modal="true" aria-labelledby="modal-title">
|
||||
<div class="ui-modal-panel">
|
||||
<div class="ui-modal-header">
|
||||
<h2 id="modal-title">{ props.Title }</h2>
|
||||
</div>
|
||||
...
|
||||
</div>
|
||||
</div>
|
||||
}
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
### WR-04: Select JS Re-initializes on Every `htmx:afterSwap` Event Against the Full Document, Not Just the Swapped Subtree
|
||||
|
||||
**File:** `backend/internal/web/ui/select.templ:242-244`
|
||||
|
||||
**Issue:** The HTMX listener calls `window.__uiSelectInitAll(event.target)` where `event.target` is the swapped element. The `__uiSelectInitAll` implementation then queries `(scope || document).querySelectorAll("[data-ui-select-root]")`. When `event.target` is passed, only selects within the swapped subtree are re-initialized, which is correct. However the `__uiSelectInitAll` function is also called at the bottom of every IIFE (line 247: `window.__uiSelectInitAll(document)`) — every time any `<select>` component renders and the script tag executes (i.e. after every HTMX swap that includes a Select), it re-runs init against the full document. Because `__uiSelectSetOpen` is assigned on `window` (shared global), multiple selects on the same page are each racing to set these functions, and the init-all against `document` will call `__uiSelectSync` on every existing select, potentially resetting the displayed label of a select the user has already interacted with.
|
||||
|
||||
**Fix:** The `window.__uiSelectInitAll(document)` call at line 247 should be scoped to the nearest ancestor `[data-ui-select-root]` rather than the whole document:
|
||||
|
||||
```js
|
||||
// Replace line 247:
|
||||
var thisRoot = document.currentScript
|
||||
? document.currentScript.closest("[data-ui-select-root]")
|
||||
: null;
|
||||
window.__uiSelectInitAll(thisRoot || document);
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
### WR-05: `TaskCreateFormFragment` Does Not Include `etape_id` in `hx-post` Filter Propagation — Filter State Is Silently Dropped on Create
|
||||
|
||||
**File:** `backend/templates/tasks.templ:259-321`
|
||||
|
||||
**Issue:** `TaskCreateFormFragment` posts to `/tablos/{id}/tasks` without appending the etape filter query parameter. The "Discard" button correctly includes `filter.QuerySuffix()` when constructing the cancel URL (line 314), but the form's `hx-post` attribute (line 261) and `action` attribute (line 259) do not. After a successful task create, the HTMX response appends the new card to `#column-{status}` via `beforeend`, but because the etape filter was not in the POST URL, the server may respond with a task that belongs to a different etape than the one currently filtered, leaking the task into the visible column.
|
||||
|
||||
**Fix:** Append `filter.QueryParam()` to the post action:
|
||||
|
||||
```templ
|
||||
hx-post={ "/tablos/" + tabloID.String() + "/tasks" + filter.QueryParam() }
|
||||
```
|
||||
|
||||
(matching the pattern already used in `KanbanBoard` at line 31.)
|
||||
|
||||
---
|
||||
|
||||
### WR-06: Auth Templates Use Tailwind Utility Classes Directly on Form Inputs Instead of `ui.Input` Component
|
||||
|
||||
**File:** `backend/templates/auth_login.templ:47-50`, `backend/templates/auth_signup.templ:47-50`, `backend/templates/etapes.templ:116-123`, `backend/templates/events.templ:117-143`, `backend/templates/tasks.templ:188-196`
|
||||
|
||||
**Issue:** The auth and feature templates use raw `<input>` and `<textarea>` elements with inline Tailwind classes (`"mt-1 block w-full rounded border border-slate-300 px-3 py-2 text-sm..."`) instead of the newly introduced `ui.Input` and `ui.Textarea` components. This creates two separate input styling systems: the design-system token-based one (in `ui.Input`) and a hard-coded Tailwind one in feature templates. When input styles need to change, developers must update both systems. The auth template submit button already uses `ui.Button` correctly (line 67 in `auth_login.templ`), making the inconsistency more visible.
|
||||
|
||||
**Fix:** Replace raw inputs in feature templates with `ui.Input` and `ui.Textarea`:
|
||||
|
||||
```templ
|
||||
// auth_login.templ:42-51 — replace with:
|
||||
@ui.FormField(ui.FormFieldProps{
|
||||
Label: "Email address",
|
||||
For: "email",
|
||||
Field: ui.Input(ui.InputProps{
|
||||
ID: "email",
|
||||
Name: "email",
|
||||
Type: "email",
|
||||
Value: form.Email,
|
||||
Required: true,
|
||||
Placeholder: "you@example.com",
|
||||
}),
|
||||
Error: errs.Email,
|
||||
})
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
### WR-07: `textareaRows` Helper Accepts Negative `rows` and Normalizes to 4 — But Negative Values Are a Caller Bug, Not a Valid Default Case
|
||||
|
||||
**File:** `backend/internal/web/ui/helpers.go:51-55`
|
||||
|
||||
**Issue:** `textareaRows` normalizes `rows <= 0` to 4. A caller passing `rows = -1` gets 4 rows silently. Negative values cannot be the product of a valid prop configuration — they indicate a bug at the call site (e.g. off-by-one in a loop counter assigned to `Rows`). The silent normalization hides the bug rather than surfacing it.
|
||||
|
||||
**Fix:** Keep the zero-value default (zero means "use default"), but add a panic or log for negative values in non-production builds:
|
||||
|
||||
```go
|
||||
func textareaRows(rows int) string {
|
||||
if rows < 0 {
|
||||
panic(fmt.Sprintf("ui.Textarea: Rows must be >= 0, got %d", rows))
|
||||
}
|
||||
if rows == 0 {
|
||||
rows = 4
|
||||
}
|
||||
return strconv.Itoa(rows)
|
||||
}
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
### WR-08: `NormalizedSize` Does Not Handle `SizeMD` Explicitly — Zero-Value Case Falls Through to Default Correctly, but the Switch Is Misleading
|
||||
|
||||
**File:** `backend/internal/web/ui/variants.go:76-83`
|
||||
|
||||
**Issue:** `NormalizedSize` handles `SizeSM` and `SizeLG` in the case list and falls through to `default: return SizeMD`. This means `SizeMD` itself is treated as an unknown value and normalized to `SizeMD` only by coincidence via the default case. The intent is correct but fragile: if someone adds a new `SizeXL` in the future and forgets to update the switch, it will silently normalize to `SizeMD` rather than erroring. More importantly, `SizeMD` is not in the explicit case list, which is inconsistent with how `NormalizedButtonVariant` (line 88) handles all explicit variants.
|
||||
|
||||
```go
|
||||
// Current — SizeMD is handled by default, not explicitly
|
||||
func NormalizedSize(size Size) Size {
|
||||
switch size {
|
||||
case SizeSM, SizeLG:
|
||||
return size
|
||||
default:
|
||||
return SizeMD
|
||||
}
|
||||
}
|
||||
```
|
||||
|
||||
**Fix:** Include `SizeMD` in the case list so the intent is documented:
|
||||
|
||||
```go
|
||||
func NormalizedSize(size Size) Size {
|
||||
switch size {
|
||||
case SizeSM, SizeMD, SizeLG:
|
||||
return size
|
||||
default:
|
||||
return SizeMD
|
||||
}
|
||||
}
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
## Info
|
||||
|
||||
### IN-01: `tokens.go` Declares Constants That Are Unused by Any Component or CSS
|
||||
|
||||
**File:** `backend/internal/web/ui/tokens.go:15-21`
|
||||
|
||||
**Issue:** `TokenPrimary`, `TokenNeutral`, `TokenWarning`, `TokenSuccess`, `TokenDanger` are declared but referenced nowhere in the codebase within the reviewed files. The file comment says CSS rules do not yet dereference these constants, and they are described as forward-compatibility placeholders. Dead exported constants are not caught by Go's unused-variable checker (they are exported) and will accumulate. If these are genuinely deferred, a `//nolint:unused` or a `TODO` comment with a tracking reference would clarify intent; without that, a future developer cannot distinguish "placeholder" from "dead code."
|
||||
|
||||
**Fix:** Either wire them or add explicit `// TODO(phase-N): ...` comments to each constant explaining when they will be used.
|
||||
|
||||
---
|
||||
|
||||
### IN-02: Blank Line in `ui_test.go` Between Two Test Functions Violates `gofmt` Convention
|
||||
|
||||
**File:** `backend/internal/web/ui/ui_test.go:71`
|
||||
|
||||
**Issue:** There are two consecutive blank lines between `TestButton_ExplicitTypeSubmit` (ending line 69) and `TestBadge_InfoVariant` (starting line 72). Go convention (and `gofmt`) uses a single blank line between top-level declarations. `just lint` runs `gofmt -l .` and `grep .` to fail on any diff — this file will fail the lint check.
|
||||
|
||||
**Fix:** Remove one blank line at line 71 to leave a single blank line between the two functions.
|
||||
|
||||
---
|
||||
|
||||
### IN-03: `catalog/examples.go` `anyComponent` Type Alias Is Unused
|
||||
|
||||
**File:** `backend/internal/web/ui/catalog/examples.go:19`
|
||||
|
||||
**Issue:** `type anyComponent = templ.Component` is declared but never referenced in the file. Go's `noUnusedVariables` does not catch type aliases (they are exported by the type system even if unexported, and `go vet` does not flag unused type aliases). The alias adds noise.
|
||||
|
||||
**Fix:** Remove the unused type alias.
|
||||
|
||||
---
|
||||
|
||||
### IN-04: Catalog Page Hard-Codes `tailwind.css` Path in a `<link>` Tag Without Cache-Busting
|
||||
|
||||
**File:** `backend/internal/web/ui/catalog/catalog.templ:14`
|
||||
|
||||
**Issue:** `<link rel="stylesheet" href="/static/tailwind.css"/>` loads the CSS file with no cache-busting query parameter. In development this is fine (browsers re-request on reload), but since the catalog is a dev tool that runs under air with live reload, repeated CSS changes during visual sign-off may not be reflected in the browser until the cache is cleared. This is exclusively a developer experience issue, not a production one.
|
||||
|
||||
**Fix:** Add a build timestamp or random query param when the catalog is served, or instruct developers to hard-reload (Cmd+Shift+R) during sign-off sessions. A code-level fix is low-priority given the dev-only scope.
|
||||
|
||||
---
|
||||
|
||||
_Reviewed: 2026-05-16T00:00:00Z_
|
||||
_Reviewer: Claude (gsd-code-reviewer)_
|
||||
_Depth: standard_
|
||||
Loading…
Reference in a new issue