docs(15): add code review report
This commit is contained in:
Arthur Belleville
parent
716678cddd
commit
d5ce65a003
1 changed files with 241 additions and 0 deletions
241
.planning/phases/15-dashboard-tablos/15-REVIEW.md
Normal file
241
.planning/phases/15-dashboard-tablos/15-REVIEW.md
Normal file
|
|
@ -0,0 +1,241 @@
|
|||
---
|
||||
phase: 15-dashboard-tablos
|
||||
reviewed: 2026-05-16T00:00:00Z
|
||||
depth: standard
|
||||
files_reviewed: 14
|
||||
files_reviewed_list:
|
||||
- backend/internal/web/handlers_account.go
|
||||
- backend/internal/web/handlers_discussion.go
|
||||
- backend/internal/web/handlers_events.go
|
||||
- backend/internal/web/handlers_files.go
|
||||
- backend/internal/web/handlers_planning.go
|
||||
- backend/internal/web/handlers_tablos.go
|
||||
- backend/internal/web/handlers_tablos_test.go
|
||||
- backend/internal/web/ui/app.css
|
||||
- backend/tailwind.input.css
|
||||
- backend/templates/account_providers.templ
|
||||
- backend/templates/app_layout.templ
|
||||
- backend/templates/app_layout_helpers.go
|
||||
- backend/templates/planning.templ
|
||||
- backend/templates/tablos.templ
|
||||
findings:
|
||||
critical: 3
|
||||
warning: 5
|
||||
info: 3
|
||||
total: 11
|
||||
status: issues_found
|
||||
---
|
||||
|
||||
# Phase 15: Code Review Report
|
||||
|
||||
**Reviewed:** 2026-05-16T00:00:00Z
|
||||
**Depth:** standard
|
||||
**Files Reviewed:** 14
|
||||
**Status:** issues_found
|
||||
|
||||
## Summary
|
||||
|
||||
Phase 15 delivers the dashboard + sidebar redesign (AppLayout), project-card grid on `/`, and the tablo detail page with inline-edit for title/description. The Go handler and template code is generally well-structured, with clear ownership checks (user_id in the DB query), CSRF wiring, and explicit HTMX response paths. The test suite is comprehensive.
|
||||
|
||||
Three blockers were found: a runtime panic risk when the authenticated user has an empty email, a filename truncation that slices on bytes instead of runes (corrupting multi-byte filenames), and an event time validation that silently accepts a midnight end-time equal to a start-time of 00:00 (off-by-one). Five warnings cover unsafe color injection, `context.WithTimeout` cancel leak, `time.Local` in a server process, missing nil guard, and an unignored render error. Three informational items cover dead template code, a hardcoded inline style, and a redundant nil guard pattern repeated across every handler.
|
||||
|
||||
---
|
||||
|
||||
## Critical Issues
|
||||
|
||||
### CR-01: Runtime panic — empty email sliced without length guard
|
||||
|
||||
**File:** `backend/templates/app_layout.templ:105`
|
||||
**Issue:** `string([]rune(user.Email)[:1])` panics with a slice-bounds-out-of-range error when `user.Email` is an empty string. Although the current signup flow always stores an email, the `auth.User` struct does not enforce non-empty at the type level, so any user record with a blank email (e.g. OAuth path bug, manual DB edit, future feature) causes every authenticated page render to panic and return a 500 with no fragment rendered. Because `AppLayout` is called from every protected handler, the blast radius is the entire authenticated surface.
|
||||
|
||||
**Fix:**
|
||||
```go
|
||||
// app_layout.templ — SidebarOrganizationFooter
|
||||
templ SidebarOrganizationFooter(user *auth.User, csrfToken string) {
|
||||
<div class="sidebar-organization">
|
||||
<div class="organization-button">
|
||||
if len(user.Email) > 0 {
|
||||
<span class="organization-avatar">{ string([]rune(user.Email)[:1]) }</span>
|
||||
} else {
|
||||
<span class="organization-avatar">?</span>
|
||||
}
|
||||
<span class="organization-copy">
|
||||
<span class="organization-name">{ user.Email }</span>
|
||||
</span>
|
||||
</div>
|
||||
...
|
||||
</div>
|
||||
}
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
### CR-02: Filename truncation slices bytes, not runes — corrupts multi-byte filenames / may panic
|
||||
|
||||
**File:** `backend/internal/web/handlers_files.go:211`
|
||||
**Issue:** `filename = filename[:255]` operates on the raw byte slice of the Go string. When `filename` contains multi-byte UTF-8 characters (e.g. Japanese, emoji), the slice boundary can fall inside a character, producing an invalid UTF-8 string stored in the database and potentially returned to browsers. Additionally, if the `filepath.Base` result is shorter than 255 bytes but the caller path reaches this line due to a logic error, a panic is possible. The subsequent `filename == "." || filename == ""` check that follows the truncation does not protect against this.
|
||||
|
||||
**Fix:**
|
||||
```go
|
||||
// Truncate by rune count, not bytes, to avoid splitting multi-byte characters.
|
||||
runes := []rune(filename)
|
||||
if len(runes) > 255 {
|
||||
filename = string(runes[:255])
|
||||
}
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
### CR-03: Event end-time validation accepts 00:00 when start is also 00:00 (off-by-one boundary)
|
||||
|
||||
**File:** `backend/internal/web/handlers_events.go:344`
|
||||
**Issue:** The validation condition is `endValue.Microseconds <= startValue.Microseconds`. When a user submits `start_time = "00:00"` and `end_time = "00:00"`, both produce `Microseconds = 0`; the condition is true (`0 <= 0`) so the error fires correctly. However, the intent described in the error message ("End time must be after the start time") means **strictly greater than**, which `<=` does express — but the boundary case at midnight is ambiguous: a user wishing to model a range ending at "00:00 the next day" will be rejected. More critically, the midnight boundary (00:00) means `Microseconds = 0`; because `pgtype.Time` has no date component, start=23:00 and end=00:00 (next morning) produce `endValue.Microseconds (0) <= startValue.Microseconds (82800000000)` → true → error fires, rejecting a legitimate overnight range. This is a logic error for multi-day events.
|
||||
|
||||
**Fix:** Add an explicit guard: if `end_time` represents 00:00 it should be treated as 24:00 (i.e., `24 * 3600 * 1e6`) when the intent is end-of-day. Alternatively, enforce that the same-day restriction is documented and validated consistently:
|
||||
```go
|
||||
// Reject 00:00 as an end time explicitly (it cannot mean "next midnight"
|
||||
// without a date component). Provide a clear validation message.
|
||||
if endValue.Microseconds == 0 {
|
||||
errs.EndTime = "End time cannot be midnight (00:00). Use a time within the same day."
|
||||
} else if startValue.Valid && endValue.Microseconds <= startValue.Microseconds {
|
||||
errs.EndTime = "End time must be after the start time."
|
||||
}
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
## Warnings
|
||||
|
||||
### WR-01: Unvalidated tablo color written directly into HTML `style` attribute — XSS risk
|
||||
|
||||
**File:** `backend/templates/tablos.templ:105` and `backend/templates/app_layout.templ:88`
|
||||
**Issue:** The tablo color value from the database is interpolated directly into a `style` attribute: `style={ "background-color: " + card.Tablo.Color.String }`. templ auto-escapes attribute *values* in most contexts but `style` content is not HTML-escaped the same way as text content — CSS injection via `expression(...)` (IE), `url('javascript:...')`, or closing the style attribute with `"` are not blocked by templ's default escaping. The validation in `TablosCreateHandler` (hexColorRE) gates the create path, but **there is no validation in `TabloUpdateHandler`** — the update handler reads and passes `tablo.Color` unchanged from the DB. Any color value already stored (or injected via a direct DB write) propagates to the UI without re-validation. The `SidebarProjectsSection` in `app_layout.templ:88` has the same pattern.
|
||||
|
||||
**Fix:** Re-validate the color string before rendering, or strip the CSS injection risk entirely by mapping known-good colors through a whitelist server-side. At minimum, re-apply `isValidCSSColor` in the template helper before emitting the style attribute. Since templ does not apply CSS-specific escaping, a CSS-safe function is required:
|
||||
```go
|
||||
// In a helper file:
|
||||
func safeCSSColor(s string) string {
|
||||
if isValidCSSColor(s) {
|
||||
return s
|
||||
}
|
||||
return ""
|
||||
}
|
||||
```
|
||||
Then in the template: `if safeCSSColor(card.Tablo.Color.String) != "" { ... }`.
|
||||
|
||||
---
|
||||
|
||||
### WR-02: `context.WithTimeout` cancel called after use — context leak in error path
|
||||
|
||||
**File:** `backend/internal/web/handlers_files.go:238-243`
|
||||
**Issue:** The cleanup context is created with `context.WithTimeout`, then `deps.Files.Delete` is called, and then `cancel()` is called immediately after. While `cancel()` is called, it is called *after* the blocking `Delete` call, which means the cancel function is not deferred. If `Delete` panics (unlikely but possible), `cancel()` is never called, leaking the goroutine tracking the timeout. The comment says "call immediately after Delete, not via defer" which misunderstands the purpose of `defer cancel()` — `defer` is idiomatic and correct here since `cleanupCtx` is scoped to this block and `Delete` has already returned by the time `cancel()` is reached.
|
||||
|
||||
More importantly, the cancel is called *after* `Delete` has already completed. This means the WithTimeout has no practical effect on the `Delete` call (it cancels a context that was already used). The real guard against a slow `Delete` is the context itself, which is passed in. If `Delete` takes longer than 10 seconds, the context expires and `Delete` should respect it — but `cancel()` must be deferred to ensure cleanup even on early returns.
|
||||
|
||||
**Fix:**
|
||||
```go
|
||||
cleanupCtx, cancel := context.WithTimeout(context.Background(), 10*time.Second)
|
||||
defer cancel() // defer — always runs, prevents goroutine leak on any exit path
|
||||
if delErr := deps.Files.Delete(cleanupCtx, s3Key); delErr != nil {
|
||||
slog.Default().Error("files upload: S3 cleanup after DB failure", "s3_key", s3Key, "err", delErr)
|
||||
}
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
### WR-03: `time.Local` in server handlers — environment-sensitive behavior
|
||||
|
||||
**File:** `backend/internal/web/handlers_events.go:28,31,35,60,100` and `backend/internal/web/handlers_planning.go:22,31`
|
||||
**Issue:** All date/time calculations use `time.Local`. In a containerized server deployment (Cloud Run, Docker), the container's local timezone is typically UTC, which may differ from the user's timezone. Event dates stored using `time.Local` on the server will be correct only if the server TZ matches what the user expects. If the server is in UTC and a user in UTC-5 submits a date of "2026-01-01", the server interprets it as UTC midnight, which is correct for storage. But the boundary: if a user's browser submits "2026-01-31" as the last day of the month, and the server is in a timezone where the local midnight differs, the computed `monthEnd = monthStart.AddDate(0, 1, -1)` may include or exclude an extra day. This is not a crash but can cause events to be invisible in the calendar view (off by one day at month boundaries).
|
||||
|
||||
**Fix:** Use `time.UTC` consistently throughout date-only calculations, or pass an explicit timezone via configuration. This eliminates environment-sensitive behavior in the deployed binary.
|
||||
|
||||
---
|
||||
|
||||
### WR-04: Template render errors silently ignored throughout
|
||||
|
||||
**File:** Multiple handlers — e.g., `backend/internal/web/handlers_tablos.go:60`, `backend/internal/web/handlers_discussion.go:64`, `backend/internal/web/handlers_events.go:151`, `backend/internal/web/handlers_files.go:95`
|
||||
**Issue:** Every `templates.*.Render(...)` call uses the blank identifier: `_ = templates.Foo(...).Render(r.Context(), w)`. If the template render fails (e.g., due to a context cancellation mid-stream, writer error, or broken pipe), the failure is swallowed entirely with no log. For HTMX partial swaps this results in a silent empty response that the browser may interpret as a successful swap, leaving the UI in an inconsistent state. The pattern is consistent across all handlers.
|
||||
|
||||
**Fix:** At minimum, log render errors at warn level:
|
||||
```go
|
||||
if err := templates.Foo(...).Render(r.Context(), w); err != nil {
|
||||
slog.Default().Warn("render failed", "handler", "TablosListHandler", "err", err)
|
||||
}
|
||||
```
|
||||
Note: Because headers may already be sent by the time Render fails mid-stream, the only recoverable action is logging — not writing a new HTTP error response. The log is sufficient.
|
||||
|
||||
---
|
||||
|
||||
### WR-05: `PlanningPageHandler` ignores the `ok` return from `auth.Authed`
|
||||
|
||||
**File:** `backend/internal/web/handlers_planning.go:36`
|
||||
**Issue:** `_, user, _ := auth.Authed(r.Context())` discards the `ok` boolean. The route is protected by `auth.RequireAuth` middleware (confirmed in router.go), so `ok` is always true in practice. However, the pattern is inconsistent with `AccountProvidersHandler` (handlers_account.go:16) which correctly checks `ok` and redirects on failure. If `RequireAuth` middleware ever fails to set the user in context (bug, future refactor, middleware reordering), `PlanningPageHandler` will dereference `user.ID` on a nil or zero-value User, producing a runtime panic or silent wrong-user query. `TablosListHandler` and `TablosCreateHandler` have the same pattern.
|
||||
|
||||
**Fix:** Follow the `AccountProvidersHandler` pattern:
|
||||
```go
|
||||
_, user, ok := auth.Authed(r.Context())
|
||||
if !ok {
|
||||
http.Redirect(w, r, "/login", http.StatusSeeOther)
|
||||
return
|
||||
}
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
## Info
|
||||
|
||||
### IN-01: Dead template — `TabloCard` and `tabloCardBody` are unreferenced
|
||||
|
||||
**File:** `backend/templates/tablos.templ:122-153`
|
||||
**Issue:** `TabloCard` and its helper `tabloCardBody` implement the old card layout using `ui.Card`. The dashboard now uses `TabloProjectCard` exclusively (confirmed by `TablosDashboard` and `TabloCardWithOOBFormClear` which call `TabloProjectCard` and `TabloCard` respectively — wait: `TabloCardWithOOBFormClear` at line 231 calls `TabloCard`, so it is still referenced). On closer inspection: `TabloCardWithOOBFormClear` (line 231) calls `TabloCard(TabloCardFromTablo(tablo), csrfToken)` which keeps the old card alive in the OOB-form-clear path. However `TabloCard` renders the *old* card layout (using `ui.Card`), while the dashboard uses `TabloProjectCard` (new project-card layout). This means the card inserted by a create action will look visually different from all other cards on the page — the `TabloCardWithOOBFormClear` response appends an old-style card to a grid of new project-cards. This is a UI consistency defect.
|
||||
|
||||
**Fix:** Update `TabloCardWithOOBFormClear` to call `TabloProjectCard` instead of `TabloCard`:
|
||||
```go
|
||||
templ TabloCardWithOOBFormClear(tablo sqlc.Tablo, csrfToken string) {
|
||||
@TabloProjectCard(TabloCardFromTablo(tablo), csrfToken)
|
||||
<div id="create-form-slot" hx-swap-oob="true"></div>
|
||||
}
|
||||
```
|
||||
Then remove `TabloCard` and `tabloCardBody` if they have no other callers.
|
||||
|
||||
---
|
||||
|
||||
### IN-02: Hardcoded inline `style="width: 0%;"` for progress bar — always renders 0%
|
||||
|
||||
**File:** `backend/templates/tablos.templ:285-288`
|
||||
**Issue:** The progress bar in the tablo detail page header is hardcoded to `style="width: 0%;"` with label `0%`. There is no backend data driving this value. This is dead UI — a progress bar that is always empty. If this is intentional placeholder for a future feature, the element should be hidden or commented; as-is it presents misleading information to the user.
|
||||
|
||||
**Fix:** Either compute actual progress from task completion data and pass it to the template, or remove the progress bar element until the feature is implemented:
|
||||
```html
|
||||
<!-- Remove until task-completion progress is implemented -->
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
### IN-03: Redundant nil-then-empty-slice guard pattern repeated in every handler
|
||||
|
||||
**File:** All handler files — e.g., `backend/internal/web/handlers_tablos.go:49-51`, `handlers_files.go:88-91`, `handlers_discussion.go:71-74`
|
||||
**Issue:** Every handler that calls a `List*` query follows this pattern:
|
||||
```go
|
||||
results, err := deps.Queries.ListFoo(...)
|
||||
if err != nil { ... results = []T{} }
|
||||
if results == nil { results = []T{} }
|
||||
```
|
||||
The `if results == nil` guard is redundant when `err != nil` already assigns `[]T{}` — and when `err == nil`, `pgx`/`sqlc` returns an empty non-nil slice, not nil. The double guard adds noise and misleads reviewers into thinking nil returns are possible from sqlc.
|
||||
|
||||
**Fix:** Consolidate into a helper or remove the nil guard:
|
||||
```go
|
||||
if err != nil {
|
||||
slog.Default().Error(...)
|
||||
results = []T{}
|
||||
}
|
||||
// No need for: if results == nil { results = []T{} }
|
||||
```
|
||||
Or use a small helper: `func nonNil[T any](s []T) []T { if s == nil { return []T{} }; return s }`.
|
||||
|
||||
---
|
||||
|
||||
_Reviewed: 2026-05-16T00:00:00Z_
|
||||
_Reviewer: Claude (gsd-code-reviewer)_
|
||||
_Depth: standard_
|
||||
Loading…
Reference in a new issue