docs(17): add code review report
This commit is contained in:
Arthur Belleville
parent
ab756c64ce
commit
bb2001f3ce
1 changed files with 191 additions and 0 deletions
191
.planning/phases/17-chat-planning/17-REVIEW.md
Normal file
191
.planning/phases/17-chat-planning/17-REVIEW.md
Normal file
|
|
@ -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 `<article>` elements lives inside a nested `<div class="divide-y divide-slate-100">` child. Appending the new `<article>` directly to the `.ui-card` sibling of that `<div>` 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 `<div>`) or change the swap strategy so the server returns the full updated message list. The simplest fix:
|
||||
|
||||
```templ
|
||||
<!-- In discussion.templ, give the inner list a stable id -->
|
||||
<div id="discussion-message-list" class="divide-y divide-slate-100">
|
||||
...
|
||||
</div>
|
||||
|
||||
<!-- Update composer hx-target -->
|
||||
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 `<form>` 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 {
|
||||
<p class="mt-1 text-sm text-slate-600">{ strconv.Itoa(data.ParticipantCount) } participant</p>
|
||||
}
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
## 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 `<form>` 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_
|
||||
Loading…
Reference in a new issue