diff --git a/.planning/phases/20-tablo-detail-kanban-restyle/20-REVIEW.md b/.planning/phases/20-tablo-detail-kanban-restyle/20-REVIEW.md new file mode 100644 index 0000000..3329474 --- /dev/null +++ b/.planning/phases/20-tablo-detail-kanban-restyle/20-REVIEW.md @@ -0,0 +1,232 @@ +--- +phase: 20-tablo-detail-kanban-restyle +reviewed: 2026-05-18T00:00:00Z +depth: standard +files_reviewed: 8 +files_reviewed_list: + - go-backend/internal/web/handlers/tablo_detail.go + - go-backend/internal/web/handlers/tablo_detail_tab.go + - go-backend/internal/web/handlers/tablo_detail_test.go + - go-backend/internal/web/ui/app.css + - go-backend/internal/web/views/tablo_detail.templ + - go-backend/internal/web/views/tablo_detail_view.go + - go-backend/internal/web/views/tablo_detail_view_test.go + - go-backend/router.go +findings: + critical: 3 + warning: 4 + info: 2 + total: 9 +status: issues_found +--- + +# Phase 20: Code Review Report + +**Reviewed:** 2026-05-18 +**Depth:** standard +**Files Reviewed:** 8 +**Status:** issues_found + +## Summary + +The tablo detail kanban restyle introduces a well-structured view model, clean templ components, and solid unit-test coverage for the `TabloDetailViewModel` builder. The Go handler logic is straightforward and handles auth/ownership correctly. + +Three blockers are present. Two of them are dead-on-arrival: kanban drag-to-reorder and the "Add task" links both reference routes that do not exist in the router, so those features produce 404 responses at runtime. The third blocker is a post-delete navigation bug: deleting a task from the kanban board re-renders the `/tasks` page instead of the current tablo, breaking the user's context. + +The warnings cover a non-functional active tab indicator, a stale task count in the column header after reorder, an O(n²) child-count loop in the view model builder, and a shadowed built-in `min` function in the test file that may cause confusion as Go versions evolve. + +--- + +## Critical Issues + +### CR-01: Kanban drag-reorder endpoint not registered in router + +**File:** `go-backend/router.go:45-49` / `go-backend/internal/web/views/tablo_detail.templ:121` + +**Issue:** Every kanban column contains a hidden `
` that POSTs to `/tablos/{tabloID}/tasks/reorder?status={col.ID}` when Sortable.js fires its `onEnd` event. That route is not registered in `router.go`. Every drag-drop action results in a 404 response. The form also contains no `` fields, so even if the route existed the server would receive no task-ordering data — the feature is doubly broken. + +**Fix:** Register the handler and populate the form with the serialized task order before submit: + +```go +// router.go +mux.Post("/tablos/{tabloID}/tasks/reorder", authHandler.PostTabloTasksReorder()) +``` + +In the Sortable `onEnd` callback, serialize task IDs into hidden inputs before calling `requestSubmit()`: + +```js +onEnd: function(evt) { + var col = el; + var form = document.getElementById('reorder-form-' + col.dataset.status); + if (!form) return; + // Clear old inputs + form.querySelectorAll('input[name="task_id"]').forEach(function(i) { i.remove(); }); + // Write current order + col.querySelectorAll('[data-task-id]').forEach(function(card) { + var inp = document.createElement('input'); + inp.type = 'hidden'; + inp.name = 'task_id'; + inp.value = card.dataset.taskId; + form.appendChild(inp); + }); + form.requestSubmit(); +} +``` + +--- + +### CR-02: "Add task" link targets a non-existent route + +**File:** `go-backend/internal/web/views/tablo_detail_view.go:84` / `go-backend/router.go` + +**Issue:** Each column header renders an `` link (via `col.CreateHref`). There is no `GET /tablos/{tabloID}/tasks/create` route registered in the router. Clicking "+ Ajouter" in any column produces a 404. + +**Fix:** + +```go +// router.go +mux.Get("/tablos/{tabloID}/tasks/create", authHandler.GetCreateTaskModal()) +``` + +Implement `GetCreateTaskModal()` to render a task creation form pre-populated with the `status` query param, matching the existing `PostTasks` handler's expected form shape. + +--- + +### CR-03: Task deletion from kanban navigates user to `/tasks` page instead of current tablo + +**File:** `go-backend/internal/web/views/tablo_detail.templ:143-146` + +**Issue:** The delete button on each task card targets `#app-main-content` with `hx-swap="outerHTML"`. The `DeleteTask` handler calls `renderTasksPage()`, which renders the full `/tasks` list page. After deleting a task from the kanban board, the user is silently redirected to the task list — losing their current tablo context. This is particularly disorienting because no redirect header is sent; the page content just changes under the user. + +**Fix:** Either (a) have the `DeleteTask` handler detect its referer and re-render the tablo detail page, or (b) use a narrower HTMX swap target so only the removed card is removed from the DOM without a full re-render: + +```templ +// Option B: remove just the card on success +"hx-target": "closest article", +"hx-swap": "outerHTML", +``` + +The server response for a 200 can then be an empty body (HTMX will replace the element with nothing), and no re-render is needed at all. + +--- + +## Warnings + +### WR-01: Active tab indicator is hardcoded and has no CSS definition + +**File:** `go-backend/internal/web/views/tablo_detail.templ:69` / `go-backend/internal/web/ui/app.css` + +**Issue:** The "Tâches" tab has `class="tab-nav-item tab-nav-item--active"` hardcoded. Two problems: +1. `tab-nav-item--active` has no corresponding CSS rule in `app.css`, so the active state has no visual effect. +2. When the user clicks another tab (which swaps only `#tab-content`), the tab bar is not re-rendered, so the active class stays on "Tâches" regardless of which tab is currently displayed. + +**Fix:** Add a CSS rule for the active state and either (a) include the full tab bar in each tab-swap response with the correct tab marked active, or (b) use JavaScript to toggle the class on `htmx:afterSettle`. + +```css +/* app.css */ +.tab-nav-item--active { + border-bottom: 2px solid var(--color-brand-primary); + color: var(--color-text-primary); + font-weight: 600; +} +``` + +--- + +### WR-02: Column task count becomes stale after HTMX reorder swap + +**File:** `go-backend/internal/web/views/tablo_detail.templ:106` / `go-backend/internal/web/views/tablo_detail.templ:122-124` + +**Issue:** The reorder form's `hx-target` is `#task-list-{colID}` with `hx-swap="innerHTML"`. This replaces only the task card list inside the column body. The `` that shows the task count lives in the column header (outside `#task-list-{colID}`). After a cross-column drag-drop, both the source and destination column counts become incorrect and are never refreshed. + +**Fix:** Expand the swap target to cover the entire column, or include the count within `#task-list-{colID}` so it is part of the replaced fragment. + +--- + +### WR-03: O(n²) child-count loop in `NewTabloDetailViewModel` + +**File:** `go-backend/internal/web/views/tablo_detail_view.go:108-126` + +**Issue:** For each etape task, the code iterates all tasks to count children. With `E` etapes and `T` total tasks, this is O(E × T). For a tablo with many tasks this is wasteful and the pattern will silently scale poorly. + +**Fix:** Build a `parentID → count` map in one pass before the etapes loop: + +```go +childCount := make(map[uuid.UUID]int) +for _, task := range tasks { + if !task.IsEtape && task.ParentTaskID != nil { + childCount[*task.ParentTaskID]++ + } +} + +for _, task := range tasks { + if !task.IsEtape { + continue + } + etapes = append(etapes, TabloDetailEtapeView{ + ID: task.ID.String(), + Name: task.Title, + TaskCount: childCount[task.ID], + }) +} +``` + +--- + +### WR-04: `func min` in test file shadows Go 1.21+ built-in + +**File:** `go-backend/internal/web/handlers/tablo_detail_test.go:182-187` + +**Issue:** `func min(a, b int) int` is declared at package scope. The project uses Go 1.26 (`go.mod` line 3), which provides `min` as a built-in. While this currently compiles (package-level declarations may shadow predeclared identifiers), the `go vet` toolchain already emits a warning for this pattern in some configurations, and it will cause a compile error if the declaration is ever moved to a non-test file that uses the built-in `min` elsewhere. + +**Fix:** Remove the custom `min` function and use the built-in directly: + +```go +// Replace: +body[:min(len(body), 500)] + +// With (Go 1.21+): +body[:min(len(body), 500)] // uses built-in — remove the manual func min declaration +``` + +Delete lines 182-187 of `tablo_detail_test.go`. + +--- + +## Info + +### IN-01: `projectInitialFromName` does not uppercase non-ASCII first letters + +**File:** `go-backend/internal/web/views/tablo_detail_view.go:146-161` + +**Issue:** The manual ASCII-range check (`result >= "a" && result <= "z"`) only uppercases A–Z codepoints. A tablo named "étude" returns "é" as the initial rather than "É". The project targets French users, where accented-letter names are common. + +**Fix:** Replace the manual rune arithmetic with `strings.ToUpper`: + +```go +import "strings" + +func projectInitialFromName(name string) string { + runes := []rune(strings.TrimSpace(name)) + if len(runes) == 0 { + return "P" + } + return strings.ToUpper(string(runes[0])) +} +``` + +--- + +### IN-02: `tabloStatusPresentation` is duplicated between `views` and `handlers` packages + +**File:** `go-backend/internal/web/views/tablo_detail_view.go:163-174` + +**Issue:** The comment on `tabloStatusPresentation` in the views package says it "mirrors the handler-side function to avoid import cycle." Duplicate presentation logic means status labels and color tokens can diverge silently. + +**Fix:** Move `tabloStatusPresentation` (and `tabloColorPattern` / related constants) to a shared internal package (e.g., `internal/tablopresentation`) imported by both `views` and `handlers`, eliminating the duplication without creating a cycle. + +--- + +_Reviewed: 2026-05-18_ +_Reviewer: Claude (gsd-code-reviewer)_ +_Depth: standard_