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

phase, name, status, severity_summary, timestamp

phase	name	status	severity_summary	timestamp
5	Core V2 Features	issues	0 critical, 2 high, 4 medium, 5 low	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.

// 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.

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.

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).

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.

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.

// 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.