102 lines
4.1 KiB
Markdown
102 lines
4.1 KiB
Markdown
---
|
|
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
|