docs(20): add code review report
This commit is contained in:
Arthur Belleville
parent
77b9f8473b
commit
efc8dc4c01
1 changed files with 232 additions and 0 deletions
232
.planning/phases/20-tablo-detail-kanban-restyle/20-REVIEW.md
Normal file
232
.planning/phases/20-tablo-detail-kanban-restyle/20-REVIEW.md
Normal file
|
|
@ -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 `<form>` 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 `<input>` 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 `<a href="/tablos/{tabloID}/tasks/create?status={col.ID}">` 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 `<span class="tablo-kanban-task-count">` 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_
|
||||
Loading…
Reference in a new issue