docs(phase-5): code review complete

.planning/phases/5-core-v2-features/REVIEW.md

@@ -0,0 +1,191 @@
+---
+phase: 5
+name: Core V2 Features
+status: issues
+severity_summary: 0 critical, 2 high, 4 medium, 5 low
+timestamp: 2026-04-22
+---
+
+# Phase 5 Code Review
+
+## Summary
+Phase 5 introduces three major features: quick quantity adjustment, item search, and export functionality. The code is well-structured with strong test coverage, but contains several type safety and error handling issues that require attention before merge. Backend tests are comprehensive and well-designed. Frontend components show good UX patterns but have some compliance gaps with project standards.
+
+Key strengths: Proper error handling patterns, good test coverage, debouncing implementation. Key concerns: Missing auth headers in some API calls, TypeScript strict mode violations, unused assertions in tests.
+
+## Critical Issues (0)
+None identified.
+
+## High Priority (2)
+
+### 1. Missing Authorization Headers in Frontend API Calls
+**File:** `frontend/hooks/useExport.ts`, `frontend/components/inventory/SearchModal.tsx`
+**Lines:** useExport.ts (52-58, 86-92), SearchModal.tsx (52-57)
+**Issue:** Export and search API endpoints likely require authentication, but axios/fetch calls don't include Authorization headers. The backend test fixtures assume `admin_user.token` and `user_token` headers, but frontend doesn't provide these.
+**Impact:** Export endpoints will fail with 403 Unauthorized in production. Search may fail depending on backend auth enforcement.
+**Recommendation:** Add `Authorization: Bearer {token}` header to all API calls. Extract token from localStorage or AuthContext consistently.
+
+```typescript
+// In useExport.ts
+const token = localStorage.getItem('auth_token');
+const response = await axios.post(
+  `/api/admin/exports/inventory-snapshot?format=${format}`,
+  {},
+  {
+    responseType: "blob",
+    headers: { 'Authorization': `Bearer ${token}` }
+  }
+);
+```
+
+### 2. Unvalidated User Input in Quantity Adjustment
+**File:** `frontend/app/inventory/page.tsx`
+**Lines:** 171-173
+**Issue:** Part number is converted to uppercase without validation. No check for empty, null, or special characters. Backend may have stricter validation, but client should validate first.
+**Impact:** Malformed data could be sent to backend, or silent failures if validation is server-only.
+**Recommendation:** Add client-side validation before updating item. Validate part_number is non-empty and matches expected format.
+
+```typescript
+if (updated.part_number && updated.part_number.trim().length === 0) {
+  throw new Error("Part number cannot be empty");
+}
+```
+
+## Medium Priority (4)
+
+### 1. Loading State Not Properly Managed in Concurrent Exports
+**File:** `frontend/hooks/useExport.ts`
+**Lines:** 47-76, 81-110
+**Issue:** `isLoading` state is shared across both `exportSnapshot` and `exportAuditTrail` functions. If user clicks export snapshot while audit trail is exporting, the first export's loading state will be overwritten.
+**Impact:** UI shows incorrect loading state; user may trigger duplicate requests thinking first one didn't start.
+**Recommendation:** Use separate loading states for each export function, or implement a request queue.
+
+### 2. Regex Pattern in Content-Disposition Parser Could Fail
+**File:** `frontend/hooks/useExport.ts`
+**Lines:** 25-28
+**Issue:** Regex `filename="?([^"]+)"?` assumes filename is wrapped in quotes. RFC 2183 specifies multiple format options. Unquoted filenames with special chars will fail to parse.
+**Impact:** Fallback filename used instead of actual filename in some cases.
+**Recommendation:** Use RFC 2183 compliant parser or handle both quoted and unquoted cases explicitly.
+
+### 3. SearchModal Doesn't Cleanup Event Listeners on Unmount
+**File:** `frontend/components/inventory/SearchModal.tsx`
+**Lines:** 100-104
+**Issue:** Escape key listener is attached but cleanup depends on `isOpen` being false. If component unmounts while `isOpen=true`, listener persists.
+**Impact:** Memory leak; Escape key could trigger handlers from unmounted modal.
+**Recommendation:** Ensure listener cleanup is unconditional, or use a ref to track mounted state.
+
+```typescript
+useEffect(() => {
+  if (!isOpen) return;
+  const handleKeyDown = (e: KeyboardEvent) => { ... };
+  document.addEventListener('keydown', handleKeyDown);
+  return () => document.removeEventListener('keydown', handleKeyDown); // Always cleanup
+}, [isOpen, onClose]);
+```
+
+### 4. Unnecessary Assertion in Frontend Test
+**File:** `frontend/tests/admin/exports.test.ts`
+**Lines:** 201-202
+**Issue:** Test creates a link element but doesn't actually use it to verify filename was set. The assertion `expect(link.download).toBe('')` is always false and doesn't test the actual download behavior.
+**Impact:** Test doesn't validate filename extraction works correctly.
+**Recommendation:** Either remove test or properly mock document.createElement to verify link.download is set correctly.
+
+## Low Priority & Notes (5)
+
+### 1. Cache Not Limited in useItemSearch
+**File:** `frontend/hooks/useItemSearch.ts`
+**Lines:** 27, 64
+**Issue:** Search cache grows unbounded. Repeated searches with different queries will accumulate all results in `cacheRef.current` Map without eviction.
+**Impact:** Memory leak on long-lived search sessions (unlikely but possible).
+**Recommendation:** Implement LRU cache with max size (e.g., 50 entries).
+
+```typescript
+const MAX_CACHE_SIZE = 50;
+if (cacheRef.current.size >= MAX_CACHE_SIZE) {
+  const firstKey = cacheRef.current.keys().next().value;
+  cacheRef.current.delete(firstKey);
+}
+```
+
+### 2. Inconsistent Error Handling Pattern
+**File:** `frontend/components/inventory/QuantityAdjustmentModal.tsx`
+**Lines:** 57-61
+**Issue:** Error is logged but thrown anyway; caller must handle. Compare to SearchModal which sets error state. Inconsistent patterns across codebase.
+**Impact:** Different error handling behavior in different components; harder to maintain.
+**Recommendation:** Choose one pattern (either set state + toast, or throw). Use consistently throughout.
+
+### 3. TypeScript `any` Type Used in Multiple Locations
+**File:** `frontend/app/inventory/page.tsx`
+**Lines:** 44, 45, 73, 157
+**Issue:** `stats: any`, `currentUser: any | null`, `editingCategory: any | null`, error payloads. Violates TypeScript strict mode.
+**Impact:** Type safety gaps; refactoring risk.
+**Recommendation:** Define proper interfaces for stats, user, category objects.
+
+```typescript
+interface Stats {
+  total_categories: number;
+  total_items: number;
+}
+interface User { id: number; username: string; ... }
+```
+
+### 4. Subtitle Text Uses Forbidden "tracking-widest"
+**File:** `frontend/app/inventory/page.tsx`
+**Lines:** 280
+**Issue:** Subtitle has `tracking-widest` class which violates AI_RULES.md § Typography ("NO tracking-widest").
+**Impact:** Non-compliance with project premium aesthetics standards.
+**Recommendation:** Remove `tracking-widest` from line 280. Standard tracking is fine.
+
+```typescript
+// Line 280 change:
+<p className="text-xs md:text-sm text-secondary font-normal mt-1">Enterprise Stock Overview</p>
+```
+
+### 5. Weak Test Assertions in search.test.ts
+**File:** `frontend/tests/inventory/search.test.ts`
+**Lines:** 133-154
+**Issue:** Several "placeholder" tests that just `expect(true).toBe(true)` don't validate anything. Component tests skipped without implementation.
+**Impact:** False sense of coverage; actual SearchModal component not tested.
+**Recommendation:** Either implement full component tests with React Testing Library, or remove placeholder tests and add note about manual testing.
+
+## Test Coverage
+
+**Backend Tests (test_exports.py, test_items.py):**
+- Excellent coverage: 15+ test cases across export service, item CRUD, search, and validation
+- Strong fixtures with sample data and mock objects
+- Edge cases covered: empty lists, max results, case-insensitive search, uniqueness constraints
+- Auth enforcement tested: admin-only operations, unauthorized access
+- All assertions are meaningful and test actual behavior
+
+**Frontend Tests (exports.test.ts, search.test.ts):**
+- Export hook well-tested: CSV/Excel formats, error handling, filename extraction
+- Search hook: debounce, caching, error scenarios
+- Missing: Component rendering tests (SearchModal, QuantityAdjustmentModal)
+- Missing: Integration tests for the full search + adjustment workflow
+- Note: 4 placeholder tests in search.test.ts should be implemented or removed
+
+## Recommendations
+
+### Before Merge (Critical Path)
+1. **Add auth headers to all frontend API calls** - Export and search endpoints require auth
+2. **Validate quantity/part_number input** - Prevent empty/invalid data submission
+3. **Fix concurrent export state** - Separate loading states for snapshot vs audit trail
+4. **Remove "tracking-widest"** - Comply with UI standards
+
+### Post-Merge (Technical Debt)
+1. Implement proper component tests for SearchModal and QuantityAdjustmentModal
+2. Add LRU cache to useItemSearch to prevent unbounded growth
+3. Refactor `any` types to proper TypeScript interfaces
+4. Standardize error handling pattern across hooks (state vs throw)
+5. Implement RFC 2183 compliant Content-Disposition parser
+6. Document expected auth token location (localStorage key name)
+
+## Sign-off
+
+**Status:** Issues identified - Requires fixes before merge
+
+The code demonstrates good software engineering practices with strong test coverage and proper async/error handling patterns. However, **authentication header omission is a blocking issue** that will cause feature failures in production. The medium priority issues (concurrent state, regex parsing, memory cleanup) should be addressed before merge to ensure stability.
+
+Once the two high-priority auth/validation issues are fixed and the low-priority style issue (tracking-widest) is corrected, this phase is ready for QA testing.
+
+**Estimated effort to resolve:** 2-3 hours for high/critical issues, 1-2 hours for medium issues.