# Review Report: Draw Call Batching + Merge Dual STEP Parse
Date: 2026-03-13

## Result: ⚠️ Minor issues

## Problems Found

### [backend/app/services/step_processor.py:405] `dataclass` import at module middle
**Severity**: Low
**Recommendation**: Move `from dataclasses import dataclass, field` to the top of the file with other stdlib imports. Mid-file imports work but violate PEP 8.

### [frontend/src/components/cad/useGeometryMerge.ts:6] Unused import `forEachMeshMaterial`
**Severity**: Low
**Recommendation**: Remove `forEachMeshMaterial` from the import — it's imported but never called.

### [frontend/src/components/cad/useGeometryMerge.ts:148] `partMaterials` and `pbrMap` in useEffect deps
**Severity**: Medium
**Recommendation**: `partMaterials` and `pbrMap` are objects that get new references on re-render. The `stateRef.current` early return (line 55) mitigates unnecessary re-merge, BUT the cleanup function (line 141) runs on every deps change, setting `stateRef.current = null` and triggering a full re-merge next render. Consider comparing by stable serialization or using refs. **Not blocking** because `partMaterials` only changes on user save, and the re-merge is fast for typical assemblies.

### [backend/app/services/step_processor.py:487] `ReadFile` status unchecked
**Severity**: Low
**Recommendation**: `status` from `reader.ReadFile()` is captured but never validated. Not a regression — same pattern exists at lines 270 and 650 in existing code.

## Checklist Results

### Backend / Python
- [x] No new endpoints — no role check needed
- [x] No SQL injections
- [x] Async consistency: `extract_step_metadata` is sync (called from sync Celery tasks)
- [x] Uses `logger` (not `print()`)
- [x] No hardcoded paths
- [x] Fallback preserves existing behavior
- [N/A] No new models or migrations

### Frontend / TypeScript
- [x] `tsc --noEmit` passes with 0 errors
- [x] No new API interfaces needed (pure client-side optimization)
- [x] No Tailwind opacity syntax violations
- [x] Icons from `lucide-react` only

### Security
- [x] No credentials in code
- [x] No hardcoded tokens or secrets
- [x] English variable names and comments

## Positives

1. **Excellent fallback strategy**: Both callers try unified read first, fall back to separate reads on failure. Zero-risk deployment.
2. **Geometry attribute compatibility check** (useGeometryMerge.ts:75-81): Correctly compares attribute sets before merging — prevents crash on mismatched attributes.
3. **Clean restore logic**: `_restore()` properly disposes merged geometry and materials, restores visibility and raycast. No memory leaks on toggle-off.
4. **OCC.Core vs OCP dual-import handling**: `_using_ocp` flag with lambda wrappers for `_s` suffix dispatch matches existing codebase pattern.
5. **World transform baking** (useGeometryMerge.ts:71-73): Correctly applies `matrixWorld` to cloned geometry before merging.
6. **Cloned geometry disposal** (line 131): Disposes intermediate buffers after merge.

## Recommendation

Approved with minor cleanup. The unused import and mid-file import are trivial fixes. The useEffect deps issue is mitigated by existing guards and only affects rapid material reassignment during perfMode — acceptable for now.

Review complete. Result: ⚠️