sync: ml-ui-connection delta specs to main specs

CODE_REVIEW.md

@@ -1,6 +1,6 @@
 # Code Review: Candle Annotator
 
-**Date**: 2026-02-17
+**Date**: 2026-02-18
 **Reviewer**: Claude Code (Automated Audit)
 **Scope**: Full codebase - frontend (Next.js), backend (FastAPI), configuration, deployment
 **Verdict**: **Request Changes** - Critical and major issues must be fixed before production deployment.
@@ -9,7 +9,7 @@
 
 ## Executive Summary
 
-85 issues identified across the entire codebase: **15 Critical**, **30 Major**, **40 Minor**.
+93 issues identified across the entire codebase: **13 Critical**, **35 Major**, **45 Minor**.
 
 The application has **zero SQL injection vulnerabilities** (Drizzle ORM and SQLAlchemy parameterize all queries), good error handling structure (every route has try/catch), and well-implemented timeouts on proxy routes.
 
@@ -99,7 +99,13 @@ However, the codebase has **no authentication anywhere**, hardcoded credentials
 - **Impact**: Async click handler captures `drawingState`, `selectedLineId`, `dragState`, `annotations` from closure. 12+ item dependency array causes excessive re-subscriptions and stale values between re-subscriptions.
 - **Fix**: Use refs for frequently-changing values that the click handler reads, reducing re-subscription frequency.
 
-### C-12. Pervasive `any` Types Disable Type Safety
+### C-12. Stale Closure in SpanAnnotationManager Keyboard Handler
+
+- **File**: `src/components/SpanAnnotationManager.tsx:461-535`
+- **Impact**: Keyboard handler references `handleDeleteSpan` (depends on `selectedSpanId`) but it's not in the dependency array and not wrapped in `useCallback`. Pressing Delete uses stale `selectedSpanId`, potentially deleting wrong span.
+- **Fix**: Wrap `handleDeleteSpan` in `useCallback` with proper dependencies, or use a ref for `selectedSpanId`.
+
+### C-13. Pervasive `any` Types Disable Type Safety
 
 - **Files**: `page.tsx:134,147,196-198`, `CandleChart.tsx:62,389`, `SpanAnnotationManager.tsx:25`, `Toolbox.tsx:23`, `SpanAnnotationList.tsx:15`, `TalibPatternPanel.tsx:19`, `FileUpload.tsx:45`, `rectangle-drawing.ts:60,63`
 - **Impact**: Type errors at runtime go undetected at compile time. Especially dangerous for `geometry`, `sub_spans`, and prediction cache values.
@@ -277,6 +283,37 @@ However, the codebase has **no authentication anywhere**, hardcoded credentials
 - **File**: `src/app/page.tsx:412-425`
 - **Fix**: Add confirmation dialog before destructive action.
 
+### M-31. No React Error Boundaries
+
+- **File**: `src/app/layout.tsx`
+- **Impact**: No Error Boundary anywhere in the component tree. If CandleChart (canvas APIs) or any child throws during render, the entire app crashes to a white screen.
+- **Fix**: Add Error Boundary wrapping `{children}` in `layout.tsx` or wrapping CandleChart and sidebar sections individually.
+
+### M-32. Hardcoded 1-Minute Candle Interval Assumption
+
+- **File**: `src/components/CandleChart.tsx:452`
+- **Code**: `for (let t = span.start_time; t <= span.end_time; t += 60)`
+- **Impact**: For 5m/15m/1h/daily timeframes, this creates orders of magnitude more iterations than necessary, potentially freezing the browser.
+- **Fix**: Determine actual candle interval from data or iterate over actual candle times.
+
+### M-33. Excessive Prop Drilling (~25+ State Items)
+
+- **File**: `src/app/page.tsx`
+- **Impact**: Home component manages ~25+ pieces of state and passes them through layers. CandleChart receives 20+ props, Toolbox receives 15+.
+- **Fix**: Consider React Context or Zustand for shared state (prediction, span annotations, chart selection).
+
+### M-34. SpanAnnotationManager Recreates ALL Primitives on Every Change
+
+- **File**: `src/components/SpanAnnotationManager.tsx:104-161`
+- **Impact**: O(n) detach + O(n) attach on every selection change or annotation modification.
+- **Fix**: Only update selection state of existing primitives instead of full recreation.
+
+### M-35. Preview Primitive Cleanup Missing on Unmount
+
+- **File**: `src/components/SpanAnnotationManager.tsx:265-324`
+- **Impact**: Mouse move handler creates `SpanRectanglePrimitive` on every move. If component unmounts mid-draw, last preview is never detached.
+- **Fix**: Add cleanup in useEffect return that detaches `previewPrimitive`.
+
 ---
 
 ## 3. Minor Issues
@@ -340,6 +377,11 @@ However, the codebase has **no authentication anywhere**, hardcoded credentials
 | m-38 | `TalibPatternPanel.tsx:39` | Re-fetches when API returns empty array | Use `hasLoaded` flag |
 | m-39 | `CandleChart.tsx:202-221` | `useImperativeHandle` missing dependency array | Add `[candles]` dep |
 | m-40 | `CandleChart.tsx:1059-1110` | Missing `activeChartId` in primitive cleanup effects | Add to dependency arrays |
+| m-41 | `ChartSelector.tsx:48` | Dropdown does not close on outside click | Add click-outside handler |
+| m-42 | `annotation-types/page.tsx`, `span-label-types/page.tsx` | Dark theme not applied - hardcoded light colors | Use theme-aware CSS variables |
+| m-43 | `PredictionPanel.tsx:148-155` | No debounce on confidence slider - re-renders chart every pixel | Debounce or use `onPointerUp` |
+| m-44 | `globals.css:1` | External Google Font `@import` blocks rendering | Use `next/font` for optimization |
+| m-45 | `CandleChart.tsx` | ~1100 line component - too large | Extract into custom hooks (`useChartInit`, `useChartAnnotations`) |
 
 ---
 
@@ -396,6 +438,9 @@ However, the codebase has **no authentication anywhere**, hardcoded credentials
 | Centralize duplicate type definitions | `src/types/` (new files), 6 component files | Medium |
 | Remove `fitContent()` from reconciliation effect | `src/components/SpanAnnotationManager.tsx` | Low |
 | Use refs for click handler closure values | `src/components/CandleChart.tsx` | Medium |
+| Add React Error Boundary | `src/app/layout.tsx` or new component | Low |
+| Fix hardcoded 1-min candle interval | `src/components/CandleChart.tsx` | Low |
+| Fix SpanAnnotationManager full primitive recreation | `src/components/SpanAnnotationManager.tsx` | Medium |
 | Move dev deps to `devDependencies` | `package.json` | Low |
 | Remove dead code (`migrate.ts`, `get_db_session`, dead filter) | Multiple files | Low |
 
@@ -411,6 +456,12 @@ However, the codebase has **no authentication anywhere**, hardcoded credentials
 | Implement proper health checks (ML service) | `services/ml/app/main.py` | Low |
 | Replace deprecated Python APIs | `services/ml/app/main.py`, `db.py` | Low |
 | Extract magic numbers to constants | `CandleChart.tsx`, `page.tsx` | Low |
+| Fix dark theme on settings pages | `annotation-types/page.tsx`, `span-label-types/page.tsx` | Low |
+| Add debounce to confidence slider | `PredictionPanel.tsx` | Low |
+| Use `next/font` instead of CSS `@import` | `globals.css`, `layout.tsx` | Low |
+| Break up CandleChart into custom hooks | `CandleChart.tsx` | High |
+| Add click-outside handler to chart dropdown | `ChartSelector.tsx` | Low |
+| Consider React Context for shared state | `page.tsx`, multiple components | Medium |
 | Write tests | New test files | High |
 
 ---
@@ -430,9 +481,18 @@ However, the codebase has **no authentication anywhere**, hardcoded credentials
 | Category | Critical | Major | Minor | Total |
 |----------|----------|-------|-------|-------|
 | API Routes (TS) | 3 | 7 | 5 | 15 |
-| React Components | 4 | 9 | 18 | 31 |
+| React Components | 5 | 14 | 23 | 42 |
 | Python ML Service | 2 | 7 | 10 | 19 |
 | Config & Deployment | 3 | 7 | 7 | 17 |
-| **Total** | **12** | **30** | **40** | **82** |
+| **Total** | **13** | **35** | **45** | **93** |
 
 > Note: Some issues overlap across categories (e.g., auth affects both frontend and backend).
+
+---
+
+## No XSS or Client-Side Secret Issues Found
+
+- No usage of `dangerouslySetInnerHTML` anywhere in the codebase.
+- All user content rendered through React JSX (auto-escaped).
+- No API keys, tokens, or sensitive data stored in client-side state.
+- All API calls use relative URLs to same origin.