From 45eaeb0462b5e1eede73de45497fe2c545510745 Mon Sep 17 00:00:00 2001
From: Jean-Luc Makiola <business@jeanlucmakiola.de>
Date: Sun, 19 Apr 2026 21:36:14 +0200
Subject: [PATCH] docs(37): add code review report

---
 .../37-REVIEW.md                              | 101 ++++++++++++++++++
 1 file changed, 101 insertions(+)
 create mode 100644 .planning/phases/37-admin-global-item-management/37-REVIEW.md

diff --git a/.planning/phases/37-admin-global-item-management/37-REVIEW.md b/.planning/phases/37-admin-global-item-management/37-REVIEW.md
new file mode 100644
index 0000000..ef5f91a
--- /dev/null
+++ b/.planning/phases/37-admin-global-item-management/37-REVIEW.md
@@ -0,0 +1,101 @@
+---
+phase: 37
+status: issues_found
+depth: standard
+files_reviewed: 9
+findings:
+  critical: 0
+  warning: 3
+  info: 2
+  total: 5
+reviewed_at: "2026-04-19"
+---
+
+# Code Review: Phase 37 — Admin Global Item Management
+
+## Summary
+
+9 files reviewed at standard depth. No critical issues. 3 warnings (real bugs / UX gaps worth fixing), 2 info notes.
+
+---
+
+## Findings
+
+### WR-01 — Tags not populated from loaded item on edit page
+
+**Severity:** warning
+**File:** `src/client/routes/admin/items.$itemId.tsx` (line 130)
+
+The `useEffect` that populates form state from the loaded item hardcodes `tags: []` instead of pulling tags from `item`. Since `AdminGlobalItemDetail` extends `Omit<AdminGlobalItem, "tags">`, tags are not present on the detail endpoint response. However, this means a user saving the form immediately will send `tags: []` to the PUT endpoint, which calls `syncGlobalItemTags` with an empty array — silently clearing all existing tags.
+
+**Fix options:**
+1. Include tags in the `GET /api/admin/items/:id` response (extend `getGlobalItemWithOwnerCount` to also fetch tags).
+2. Populate tags on the list page item click by passing them through navigation state, or fetch them separately on the edit page.
+
+The safest fix is option 1: add a tag fetch to `getGlobalItemWithOwnerCount` and include them in `AdminGlobalItemDetail`.
+
+---
+
+### WR-02 — Save always sends `tags: form.tags` (empty array clears tags)
+
+**Severity:** warning
+**File:** `src/client/routes/admin/items.$itemId.tsx` (line 166)
+
+Related to WR-01. The `handleSave` function always includes `tags: form.tags` in the PUT payload, even when the user hasn't interacted with the tag field. Since `form.tags` starts as `[]`, saving without touching tags will call `updateGlobalItemById` with `tags: []`, which triggers `syncGlobalItemTags(tx, id, [])` — deleting all existing tags silently.
+
+**Fix:** Only include `tags` in the payload when the user has explicitly modified the tag field. Track a `tagsModified` boolean flag, or use `undefined` as the default form tags value and only set it to `[]` when the user clears the last tag.
+
+---
+
+### WR-03 — `limit` query param negative values not rejected
+
+**Severity:** warning
+**File:** `src/server/routes/admin-items.ts` (line 48)
+
+The limit guard is:
+```ts
+limit: isNaN(limit) || limit > 100 ? 50 : limit,
+```
+
+This passes through negative `limit` values (e.g., `?limit=-1`) to `listGlobalItemsForAdmin`, which passes them directly to Drizzle's `.limit()`. SQLite treats `LIMIT -1` as no limit — returning all rows. This is an admin-only endpoint so exploitation risk is low, but it could cause unintended large payloads.
+
+**Fix:** Add `limit <= 0` to the fallback condition:
+```ts
+limit: isNaN(limit) || limit <= 0 || limit > 100 ? 50 : limit,
+```
+
+---
+
+### INFO-01 — `type Env = { Variables: { db?: any; userId?: number } }` duplicated
+
+**Severity:** info
+**Files:** `src/server/routes/admin-items.ts` (line 12), `src/server/routes/admin.ts` (line 4)
+
+Both new files define `Env` locally using `any` for `db`. This is consistent with the existing pattern in other route files (e.g., `global-items.ts`), but it suppresses type safety on the `db` variable. Not a new issue — pre-existing pattern.
+
+---
+
+### INFO-02 — `AdminGlobalItemDetail` redundantly declares `ownerCount`
+
+**Severity:** info
+**File:** `src/client/hooks/useAdminGlobalItems.ts` (line 40)
+
+```ts
+export interface AdminGlobalItemDetail extends Omit<AdminGlobalItem, "tags"> {
+  ownerCount: number;
+}
+```
+
+`AdminGlobalItem` already includes `ownerCount: number`, so `Omit<AdminGlobalItem, "tags">` already carries `ownerCount`. The explicit re-declaration is harmless but redundant. Minor cleanup opportunity.
+
+---
+
+## Self-Check
+
+- [x] No SQL injection vectors (Drizzle parameterized queries throughout)
+- [x] Auth protection: all admin routes inherit `requireAuth + requireAdmin` from parent router
+- [x] Transaction safety: delete and update operations use `db.transaction`
+- [x] FK integrity: delete nullifies `items.globalItemId` before deleting
+- [x] Input validation: PUT body validated via `zValidator` with Zod schema
+- [x] No secrets or credentials in code
+- [x] Build passes, tests pass