From bb2001f3ce28591ea46c4079d9b44051f10d86f5 Mon Sep 17 00:00:00 2001 From: Arthur Belleville Date: Sun, 17 May 2026 10:41:32 +0200 Subject: [PATCH] docs(17): add code review report --- .../phases/17-chat-planning/17-REVIEW.md | 191 ++++++++++++++++++ 1 file changed, 191 insertions(+) create mode 100644 .planning/phases/17-chat-planning/17-REVIEW.md diff --git a/.planning/phases/17-chat-planning/17-REVIEW.md b/.planning/phases/17-chat-planning/17-REVIEW.md new file mode 100644 index 0000000..d82c603 --- /dev/null +++ b/.planning/phases/17-chat-planning/17-REVIEW.md @@ -0,0 +1,191 @@ +--- +phase: 17-chat-planning +reviewed: 2026-05-17T00:00:00Z +depth: standard +files_reviewed: 6 +files_reviewed_list: + - backend/templates/discussion.templ + - backend/templates/discussion_forms.go + - backend/templates/planning.templ + - backend/internal/web/handlers_discussion.go + - backend/internal/web/handlers_tablos.go + - backend/internal/web/ui/app.css +findings: + critical: 1 + warning: 4 + info: 2 + total: 7 +status: issues_found +--- + +# Phase 17: Code Review Report + +**Reviewed:** 2026-05-17 +**Depth:** standard +**Files Reviewed:** 6 +**Status:** issues_found + +## Summary + +Phase 17 adds server-side `IsOwn` branching for chat bubbles, a Planning page restyle, and fixes the HTMX full-page-in-tab regression in `TabloDetailHandler`. The logic for `IsOwn` itself is sound. The primary correctness bug is an HTMX DOM-structure mismatch that silently breaks message append on a thread with existing messages. Beyond that, four warnings cover the SSE realtime path broadcasting the wrong HTML to other subscribers, a hardcoded color value that diverges from the design-token contract, a stale hardcoded string in the participant count, and missing HTMX error-swap headers that prevent validation errors from being visible. Two info items cover minor quality concerns. + +--- + +## Critical Issues + +### CR-01: HTMX `beforeend` appends into `.ui-card` wrapper, not the inner `.divide-y` list + +**File:** `backend/templates/discussion.templ:74-75` + +**Issue:** The composer form targets `#discussion-messages` (the `.ui-card` div) with `hx-swap="beforeend"`. When messages already exist the actual list of `
` elements lives inside a nested `
` child. Appending the new `
` directly to the `.ui-card` sibling of that `
` means the new message sits outside the styled list container, missing `divide-y` separators and breaking the visual layout. On an empty thread the `DiscussionEmptyState` is also a direct child, so the first message lands correctly — but any subsequent post into a populated thread is misplaced. + +The JS deduplication logic in `discussion-sse.js::ensureMessageList` deliberately targets `.divide-y` inside `#discussion-messages`, showing that the intended target for direct-append is the inner div, not the card wrapper. The HTMX path and the SSE path therefore write to different DOM locations. + +**Fix:** Either change `hx-target` to point at the inner list wrapper (requires a stable `id` on that `
`) or change the swap strategy so the server returns the full updated message list. The simplest fix: + +```templ + +
+ ... +
+ + +hx-target="#discussion-message-list" +hx-swap="beforeend" +``` + +And in `discussion-sse.js::ensureMessageList`, replace the `.divide-y` selector with `#discussion-message-list`. + +--- + +## Warnings + +### WR-01: SSE broadcast sends the sender's `IsOwn=true` HTML to all other subscribers + +**File:** `backend/internal/web/handlers_discussion.go:125-135` + +**Issue:** `renderDiscussionMessageHTML` renders the message using the posting user's `message` struct, which has `IsOwn: true`. That HTML string is then broadcast to every subscriber of the tablo via `deps.Realtime.Publish`. Every other connected user receives a bubble styled as `message-own` (right-aligned, brand-colored) even though the message was written by someone else. The visual distinction between own and other-party messages is thus broken for real-time receives. + +```go +// handlers_discussion.go line 125: message.IsOwn is always true here +html, err := renderDiscussionMessageHTML(r, message) +// ... then published to ALL subscribers +deps.Realtime.Publish(DiscussionEvent{..., MessageHTML: html}) +``` + +**Fix:** The broker would need to render two HTML variants (own / other) and deliver the correct one per subscriber, or omit `MessageHTML` from the SSE payload and instead have the client re-fetch via HTMX trigger. The simplest approach until multi-user is fully supported: render and broadcast the `IsOwn=false` variant (the "other" bubble) for all subscribers since the author already sees their own message via the HTMX response, and only the author will incorrectly see a `message-other` bubble for their own post in a second tab — an acceptable degradation for v1. + +```go +// Render with IsOwn=false for broadcast to other subscribers +broadcastMessage := message +broadcastMessage.IsOwn = false +html, err := renderDiscussionMessageHTML(r, broadcastMessage) +``` + +--- + +### WR-02: Validation errors on HTMX message post are invisible — missing `HX-Reswap`/`HX-Retarget` + +**File:** `backend/internal/web/handlers_discussion.go:93-97` + +**Issue:** When body validation fails (empty or too long), the handler writes a 422 and renders `DiscussionComposer` — which is the `
` element. The HTMX request has `hx-target="#discussion-messages"` and `hx-swap="beforeend"`. By default HTMX does not swap on non-2xx responses unless `htmx.config.responseHandling` is configured to do so. The form fragment is therefore silently discarded; the user gets no error feedback. The error message never reaches the DOM. + +**Fix:** Either set response-handling config to treat 422 as swappable, or (better) add `HX-Reswap` and `HX-Retarget` response headers so HTMX replaces the form in-place: + +```go +// handlers_discussion.go — in the validation error branch +w.Header().Set("HX-Retarget", "form[action$='/discussion/messages']") +w.Header().Set("HX-Reswap", "outerHTML") +w.Header().Set("Content-Type", "text/html; charset=utf-8") +w.WriteHeader(http.StatusUnprocessableEntity) +_ = templates.DiscussionComposer(tablo, form, errs, csrf.Token(r)).Render(r.Context(), w) +``` + +--- + +### WR-03: Hardcoded `rgba` color in `.message-own .message-bubble` violates design-token contract + +**File:** `backend/internal/web/ui/app.css:752` + +**Issue:** The file header explicitly states "All color values use `var(--...)` design tokens — no hard-coded hex values." Line 752 uses `rgba(128, 78, 236, 0.10)` for the own-message bubble background. This value is invisible to dark-mode or theme overrides and will not respond to future token changes. The rest of the bubble rules (lines 758-759) already use `var(--color-surface-default, #ffffff)` with fallbacks, which is the correct pattern. + +**Fix:** +```css +/* Define a token (or reuse an existing brand alpha token) */ +.message-row.message-own .message-bubble { + background-color: var(--color-brand-bubble-own, rgba(128, 78, 236, 0.10)); + border-radius: 0.75rem 0.75rem 0.25rem 0.75rem; + color: var(--color-text-primary); +} +``` + +--- + +### WR-04: Hardcoded "1 participant" string in `DiscussionTabFragment` + +**File:** `backend/templates/discussion.templ:15` + +**Issue:** The participant count is hardcoded as the string `"1 participant"`. This will be factually wrong the moment a second user accesses the same tablo discussion, and it gives screen readers misleading information. Even if true for v1 solo use, this is a data integrity bug: the string is shown on every tablo regardless of actual state, and `DiscussionTabData` carries no participant count field to derive it from. + +**Fix:** Either remove the paragraph entirely (simplest, no incorrect information) or thread a real participant count through `DiscussionTabData`: + +```go +// discussion_forms.go +type DiscussionTabData struct { + Messages []DiscussionMessageView + ParticipantCount int +} +``` + +```templ +// discussion.templ +if data.ParticipantCount > 0 { +

{ strconv.Itoa(data.ParticipantCount) } participant

+} +``` + +--- + +## Info + +### IN-01: Double `form.reset()` on successful post — HTMX `hx-on` and JS `afterRequest` handler both fire + +**File:** `backend/templates/discussion.templ:76` and `backend/static/discussion-sse.js:85-92` + +**Issue:** The composer has `hx-on::after-request="if (event.detail.xhr.status >= 200 && ...) this.reset()"` at the template level (line 76). `discussion-sse.js` also resets the form on `htmx:afterRequest` at lines 85-92 including an explicit `textarea.value = ""`. Both run on every successful post. This is harmless today but creates a maintenance trap: the two reset paths are easy to get out of sync if one is changed (e.g. adding field-specific state to the form). One reset mechanism should own this responsibility. + +**Fix:** Remove the inline `hx-on::after-request` attribute from the `` in `discussion.templ` and rely solely on the JS handler in `discussion-sse.js`, which is more maintainable and already handles the textarea explicitly. + +--- + +### IN-02: `DiscussionMessageFromRow` `isOwn` parameter is always `row.AuthorUserID == user.ID` at every call site + +**File:** `backend/templates/discussion_forms.go:70` and `backend/internal/web/handlers_discussion.go:121` + +**Issue:** `DiscussionMessageFromRow` accepts an `isOwn bool` parameter, but every call site computes it as `row.AuthorUserID == user.ID` inline. The function signature implies the caller is responsible for the comparison, yet the information needed (the two UUIDs) is available inside the function via `row.AuthorUserID`. This is a leaky abstraction: it matches `DiscussionMessagesFromRows` which takes `currentUserID uuid.UUID` and does the comparison internally, creating an inconsistent API surface. + +**Fix:** Change `DiscussionMessageFromRow` to accept `currentUserID uuid.UUID` and compute `IsOwn` internally, matching the `DiscussionMessagesFromRows` pattern: + +```go +func DiscussionMessageFromRow(row sqlc.GetDiscussionMessageWithAuthorRow, currentUserID uuid.UUID) DiscussionMessageView { + return DiscussionMessageView{ + ID: row.ID, + AuthorEmail: row.AuthorEmail, + Body: row.Body, + CreatedAt: row.CreatedAt.Time, + IsOwn: row.AuthorUserID == currentUserID, + } +} +``` + +Update the call site: +```go +// handlers_discussion.go line 121 +message := templates.DiscussionMessageFromRow(row, user.ID) +``` + +--- + +_Reviewed: 2026-05-17_ +_Reviewer: Claude (gsd-code-reviewer)_ +_Depth: standard_