diff --git a/.planning/phases/16-tablo-detail/16-REVIEW.md b/.planning/phases/16-tablo-detail/16-REVIEW.md new file mode 100644 index 0000000..82ed8da --- /dev/null +++ b/.planning/phases/16-tablo-detail/16-REVIEW.md @@ -0,0 +1,259 @@ +--- +phase: 16-tablo-detail +reviewed: 2026-05-16T00:00:00Z +depth: standard +files_reviewed: 5 +files_reviewed_list: + - internal/web/handlers/tasks.go + - internal/web/ui/app.css + - internal/web/ui/icon_button.templ + - internal/web/views/tablos.templ + - internal/web/views/tasks.templ +findings: + critical: 3 + warning: 4 + info: 3 + total: 10 +status: issues_found +--- + +# Phase 16: Code Review Report + +**Reviewed:** 2026-05-16 +**Depth:** standard +**Files Reviewed:** 5 +**Status:** issues_found + +## Summary + +Reviewed the phase 16 restyling of the tablo detail view — the tasks handler, CSS, icon button component, tablos list template, and tasks kanban template. The core styling work is solid and uses CSS variables consistently. However, three blocking issues were found: unsafe bare type assertions that will panic if the repository interface is not satisfied, a `renderTaskEditForm` function producing a form with no `action`/HTMX attributes (submit does nothing), and a HTMX swap target mismatch in the task edit modal. Four warnings cover hardcoded hex colors in the `tabloStatusPresentation` function, a hardcoded personal name in production logic, HTMX delete actions missing a confirm dialog, and overdue tasks being silently bucketed to the first date bucket rather than "sans-date" in the roadmap view. + +--- + +## Critical Issues + +### CR-01: Bare type assertion panics in `PostTasks`, `GetEditTaskModal`, and `PatchTask` + +**File:** `internal/web/handlers/tasks.go:113` +**Issue:** After checking `h.repo.(taskMutationRepository)` with an `ok` guard on lines 101, 150, and 202, the same handlers turn around and call `h.repo.(taskPageRepository)` without an `ok` guard on lines 113, 171, and 224. If the concrete repository type satisfies `taskMutationRepository` but not `taskPageRepository` (possible in any future refactor or test double scenario), the assertion panics and kills the server process with no 500 response. + +```go +// lines 101-113 in PostTasks — repo is checked, but taskPageRepository is not: +repo, ok := h.repo.(taskMutationRepository) +if !ok { + http.Error(w, "tasks repository not configured", http.StatusInternalServerError) + return +} +// ... +taskRecords, err := h.repo.(taskPageRepository).ListTasksByOwner(r.Context(), user.ID) // PANICS if not satisfied +``` + +**Fix:** Apply the same two-value assertion pattern used for `taskMutationRepository`: + +```go +taskRepo, ok := h.repo.(taskPageRepository) +if !ok { + http.Error(w, "tasks repository not configured", http.StatusInternalServerError) + return +} +taskRecords, err := taskRepo.ListTasksByOwner(r.Context(), user.ID) +``` + +Apply this fix in `PostTasks` (line 113), `GetEditTaskModal` (line 171), and `PatchTask` (line 224). + +--- + +### CR-02: `renderTaskEditForm` produces a form with no submit action + +**File:** `internal/web/handlers/tasks.go:503` +**Issue:** The raw-HTML builder `renderTaskEditForm` constructs a `
` with no `action`, no `method`, and no HTMX attributes (`hx-patch`, `hx-target`, `hx-swap`). The form is swapped into `#app-main-content` with `hx-swap="beforeend"` (line 337 of `tasks.templ`), so it is injected into the DOM, but submitting it triggers a native GET to the current URL — not a PATCH to `/tasks/{id}`. Task edits are silently discarded. + +```go +// line 505 — no action, no method, no HTMX +builder.WriteString(``) +``` + +Also add a submit button inside the form body, and consider adding a close/cancel mechanism since `beforeend` injects content outside the main region boundary. + +--- + +### CR-03: HTMX `hx-swap="beforeend"` on `#app-main-content` for task edit is semantically broken + +**File:** `internal/web/views/tasks.templ:337` +**Issue:** The task edit "pencil" button uses `hx-target="#app-main-content"` with `hx-swap="beforeend"`. This appends the edit form as the last child of the entire main content area rather than replacing the card or rendering in a modal. Combined with CR-02 (no form action), clicking edit on multiple tasks accumulates orphaned forms in the DOM without any way to close them. This is a functional breakage of the edit flow. + +```go +Attrs: templ.Attributes{ + "hx-get": taskEditHref(task, state), + "hx-target": "#app-main-content", + "hx-swap": "beforeend", // appends to entire page, never cleaned up +}, +``` + +**Fix:** Either render the edit form as a modal (preferred — use `ui.Modal`) with `hx-swap="beforeend"` on `body`, or use `hx-target` pointing to a dedicated `#task-edit-modal` slot element and `hx-swap="innerHTML"`. If a modal approach is chosen, the response component must include a close/cancel path. + +--- + +## Warnings + +### WR-01: Hardcoded hex colors in `tabloStatusPresentation` bypass the design token system + +**File:** `internal/web/handlers/tablos.go:509` +**Issue:** The `tabloStatusPresentation` function returns an inline Tailwind class string containing raw hex color codes `bg-[#FFF4E2] text-[#DB9729] border border-[#DB9729]` for the "En cours" status. All other color usage in the app goes through CSS custom properties (`var(--color-status-warning-*)` etc.), set in `app.css`. This hard-codes a specific amber tone that will not respond to theming or design token changes, creating a visual inconsistency with `tone-warning` defined in `app.css` line 926. + +```go +// handlers/tablos.go:509 +case TabloStatusInProgress: + return "En cours", "bg-[#FFF4E2] text-[#DB9729] border border-[#DB9729]", 50, "warning" +``` + +**Fix:** Align with the design token pattern used by the badge system. Replace the arbitrary Tailwind string with a CSS class that references the existing token variables: + +```go +case TabloStatusInProgress: + return "En cours", "tone-warning", 50, "warning" +``` + +Then ensure `.tone-warning` is applied to the status pill (it is already defined in `app.css:926`). + +--- + +### WR-02: Hardcoded personal name `"margot"` in `etapeIconName` production logic + +**File:** `internal/web/views/tasks_view.go:542` +**Issue:** The `etapeIconName` function (and its companion `etapeBadgeClass`) performs string matching against the value `"margot"` to select a different icon variant. This is a hardcoded personal name that will silently affect all étapes whose title contains "margot" regardless of context, and couples the display logic to a specific user's data. + +```go +if strings.Contains(strings.ToLower(task.EtapeName), "margot") { + return "flame" +} +``` + +**Fix:** Remove the name-based branch entirely. The icon/color should be determined by the étape's position index, creation order, or an explicit étape "category" field — not by a free-text name match. If this logic was debugging scaffolding, it must be removed before the phase lands. + +--- + +### WR-03: Task delete button in kanban/roadmap has no `hx-confirm` attribute + +**File:** `internal/web/views/tasks.templ:341-351` +**Issue:** The `TaskCard` delete button calls `hx-delete` directly without `hx-confirm`. The tablo delete buttons in `tablos.templ:148-155` include `hx-confirm: "Supprimer ce projet ?"`. Omitting the confirmation on task deletion means a mis-click permanently soft-deletes a task with no undo path. This is inconsistent with the pattern established for tablos. + +```go +Attrs: templ.Attributes{ + "hx-delete": taskDeleteHref(task, state), + "hx-target": "#app-main-content", + "hx-swap": "outerHTML", + // missing: "hx-confirm": "Supprimer cette tâche ?" +}, +``` + +**Fix:** +```go +Attrs: templ.Attributes{ + "hx-delete": taskDeleteHref(task, state), + "hx-target": "#app-main-content", + "hx-swap": "outerHTML", + "hx-confirm": "Supprimer cette tâche ?", +}, +``` + +--- + +### WR-04: Overdue tasks silently bucketed to first date column in roadmap view + +**File:** `internal/web/views/tasks_view.go:639` +**Issue:** In `roadmapBucketIndex`, when `days <= 0` (task is overdue or due today) for week mode, the function returns `0` — placing the task in the first upcoming date bucket. The same issue applies to the month calculation. Overdue tasks appear mixed with current-period tasks rather than being distinguishable. The "sans-date" bucket (`bucketCount` index) is only used for `nil` due dates, not for past due dates. + +```go +days := int(dueDate.Sub(start).Hours() / 24) +if days <= 0 { + return 0 // silently puts overdue tasks in the first bucket +} +``` + +**Fix:** Return `bucketCount` (the "sans-date" bucket index) for past-due tasks, or add a dedicated "En retard" bucket at index 0. Either choice is acceptable, but the current behavior is misleading — a task due 6 months ago appears in the same slot as one due this week. + +```go +days := int(dueDate.Sub(start).Hours() / 24) +if days < 0 { + return bucketCount // or a dedicated overdue bucket +} +``` + +--- + +## Info + +### IN-01: `TasksRoadmapView.Mode` field is only populated for `TaskRoadmapModeMonth` + +**File:** `internal/web/views/tasks_view.go:328` +**Issue:** `buildRoadmapView` populates `RoadmapView.Mode` via a map literal that only has one entry — `TaskRoadmapModeMonth`. For the default week mode, `Mode` will be an empty string. The field is currently unused in the template, but if it is ever rendered, week mode will display as blank. + +```go +return TasksRoadmapView{ + Mode: map[taskmodel.TaskRoadmapMode]string{ + taskmodel.TaskRoadmapModeMonth: "Mois", + // TaskRoadmapModeWeek is missing + }[mode], +``` + +**Fix:** Add the week mapping: +```go +Mode: map[taskmodel.TaskRoadmapMode]string{ + taskmodel.TaskRoadmapModeWeek: "Semaine", + taskmodel.TaskRoadmapModeMonth: "Mois", +}[mode], +``` + +--- + +### IN-02: Hardcoded French placeholder text in roadmap lane description + +**File:** `internal/web/views/tasks.templ:294` +**Issue:** The roadmap lane section contains an inline developer note as visible text: `"Étape comme lane horizontale, avec bucketisation par date d'échéance."` This is implementation scaffolding exposed as UI copy and will be visible to end users. + +```go +

Étape comme lane horizontale, avec bucketisation par date d'échéance.

+``` + +**Fix:** Remove the `

` element or replace with meaningful user-facing text. + +--- + +### IN-03: `sortTablosByCreatedAtDesc` uses a bubble sort O(n²) — readability issue only + +**File:** `internal/web/handlers/tablos.go:445` +**Issue:** `sortTablosByCreatedAtDesc` implements a manual bubble sort rather than using `slices.SortFunc` as done everywhere else in the codebase (e.g., `tasks_view.go:159`, `tasks_view.go:195`). This is inconsistent and harder to read, though performance is not a concern for the expected data sizes. + +```go +func sortTablosByCreatedAtDesc(tablos []TabloRecord) { + for i := 0; i < len(tablos); i++ { + for j := i + 1; j < len(tablos); j++ { +``` + +**Fix:** Replace with `slices.SortFunc` to match the project pattern: +```go +func sortTablosByCreatedAtDesc(tablos []TabloRecord) { + slices.SortFunc(tablos, func(a, b TabloRecord) int { + return b.CreatedAt.Compare(a.CreatedAt) + }) +} +``` + +--- + +_Reviewed: 2026-05-16_ +_Reviewer: Claude (gsd-code-reviewer)_ +_Depth: standard_