# Review Report: P2 USD Foundation Date: 2026-03-12 ## Result: ⚠️ Minor issues --- ## Problems Found ### [backend/app/api/routers/cad.py:537] `save_manual_material_overrides` does not set `updated_at` **Severity**: Low **Recommendation**: Add `cad.updated_at = datetime.utcnow()` before `db.commit()`, consistent with `save_part_materials` at line 430 which does set it. The ORM `onupdate` on `CadFile.updated_at` only fires when SQLAlchemy detects a dirty column via ORM tracking. JSONB mutations are not always detected reliably. --- ### [backend/app/api/routers/cad.py:101,336] Hardcoded `"admin"` role string **Severity**: Low **Recommendation**: Lines 101 (`user.role.value != "admin"`) and 336 (`user.role.value != "admin"`) predate the multi-role system. These are not new in P2 but they remain in changed files. Should be migrated to `is_privileged(user)` / `require_admin()` on the next pass. Not a P2 blocker. --- ### [frontend/src/pages/Admin.tsx:42,50] `as any[]` for admin/users and templates queries **Severity**: Low **Recommendation**: These two `as any[]` casts exist in pre-P2 code but are present in the changed file. Acceptable technical debt already present before P2; not introduced by P2 itself. Document for a future typing cleanup pass. --- ### [render-worker/scripts/export_step_to_usd.py:560] `schaeffler:partKey` stored as custom data, not a USD attribute **Severity**: Low (design note, not a bug) **Recommendation**: The implementation uses `prim.SetCustomDataByKey("schaeffler:partKey", ...)` which stores the value in the prim's `customData` dictionary rather than as a typed USD attribute. Both are valid USD patterns; `customData` is non-animatable metadata and is fine for a stable identifier used only by Python tooling. If downstream tools (e.g. Houdini LOPs) need to query it via `UsdAttributeQuery`, it would need `prim.CreateAttribute("schaeffler:partKey", Sdf.ValueTypeNames.String).Set(part_key)`. For the current use case the choice is acceptable. --- ### [render-worker/scripts/export_step_to_usd.py] No `UsdUtils.FlattenLayerStack()` call **Severity**: Low (checklist item — not applicable to this milestone) **Recommendation**: This script writes a single `.usd` layer (`Usd.Stage.CreateNew` + `stage.Save()`), so there are no sublayers to flatten. `FlattenLayerStack()` is only relevant when compositing multiple USDA/USDC layers into a delivery package. Not applicable at M1/M2 scope. --- ### [backend/app/core/tenant_context.py:70,100] New sync engine created per call **Severity**: Low (performance, not correctness) **Recommendation**: `resolve_tenant_id_for_cad()` and `resolve_tenant_id_for_order_line()` create a new `create_engine()` on every call and dispose it immediately, opening a fresh Postgres connection each time. At current task volume this is fine, but a module-level lazy singleton engine would be more efficient. Not a blocking issue for P2. --- ### [backend/app/domains/pipeline/tasks/export_glb.py:89-95] Confusing `eng.dispose()` placement in cache-hit early-return path **Severity**: Medium (code clarity; not a runtime bug) **Recommendation**: In `generate_gltf_geometry_task`, the cache-hit early-return path calls `eng.dispose()` and `return` from inside the `with Session(eng) as session:` block (lines 89-95). The Python context manager correctly closes the session before `dispose()` is reached, so this is safe. But the style is confusing — the normal path disposes the engine at line 96 after the `with` block, while the cache-hit path calls it from inside. If code is ever added after line 95, double-dispose risk increases. Recommend factoring the early-return check out of the `with` block. --- ### [frontend/src/components/cad/ThreeDViewer.tsx:536] `as any` cast for Three.js scene userData **Severity**: Low **Recommendation**: `(sceneRef.current as any).userData` is necessary due to the `@react-three/fiber` ref typing. A narrower cast `(sceneRef.current as THREE.Object3D).userData` would be more type-safe without losing any functionality. --- ## Positives - **All three migrations (060-062)** are correct, additive, and include proper `downgrade()` implementations. Migration 062 uses `UPDATE ... WHERE key = ...` safely — no unexpected DROP statements. - **USD exporter (`export_step_to_usd.py`)** is a comprehensive 631-line implementation: full XCAF traversal, correct OCC→USD Y-up coordinate swap (`(x, -z, y)`), index-space sharp-edge primvars stored against local mesh vertex indices (not world coordinates), `metersPerUnit=0.001` set correctly, and `MANIFEST_JSON:` stdout IPC protocol for the Celery task to parse without re-opening the USD file. - **`pxr` import is from `usd-core`** — correctly installed via `pip3 install "usd-core>=24.11"` in the Dockerfile. - **Celery task queue placement** is correct: both `generate_gltf_geometry_task` and `generate_usd_master_task` are on `thumbnail_rendering` (concurrency=1), ensuring no parallel Blender/OCC conflicts. - **`PipelineLogger` used** at the top of both new tasks with `self.request.id`. No `render_job_doc.celery_task_id` pattern needed here — that is specific to the `WorkflowRun` system; consistent with existing pipeline task patterns. - **Auth on all new endpoints**: `scene-manifest` (GET) requires `get_current_user`; `generate-usd-master` (POST) and `manual-material-overrides` (PUT) require `is_privileged(user)`. GET `manual-material-overrides` requires auth but not privilege — correct for read access. - **No SQL injections**: all DB access uses SQLAlchemy ORM or parameterized `text()` queries. - **Pydantic validation**: `ManualMaterialOverridesIn`, `PartEntry`, `SceneManifest` models validate all POST/PUT inputs. - **No hardcoded credentials or secrets** anywhere in the changed files. - **All English identifiers and comments** throughout. - **No `print()` in backend Python** — render-worker scripts use stdout as the IPC protocol with the Celery task, which is the correct approach. - **`storage_key` values are relative**: the `_prefix` strip logic in `export_glb.py` correctly strips `upload_dir` before storing. - **`_inject_glb_extras()` called after `RWGltf_CafWriter` export** — satisfies the render pipeline checklist item. - **Frontend types**: `SceneManifest`, `PartEntry`, `ManualMaterialOverridesResponse` all have proper TypeScript interfaces in `sceneManifest.ts` and `cad.ts`. No `as any` in the new API surface code. - **Loading states** with `isPending` and error feedback with `toast.error()` present in all new UI mutations. - **`effectiveMaterials` merge** in ThreeDViewer correctly combines legacy `partMaterials` (mesh-name keyed) and new `manualOverrides` (partKey keyed), providing backward compatibility for pre-P2 GLBs. - **`build_scene_manifest()` four-layer priority** (manual → auto → source → default) correctly implemented per spec. - **`generate_part_key()` algorithm duplicated consistently** between `export_step_to_usd.py` and `part_key_service.py` — same `_AF_RE` strip, same deduplication loop. Keys generated by the USD exporter match keys computed by the API service. - **New admin bulk-action endpoints** use `require_admin`, batch-fetch existing asset IDs efficiently (single query), and queue only missing items. - **`step_tasks.py` shim** correctly re-exports `generate_usd_master_task` for Celery task name resolution. - **New `tenant_context.py`** provides clean, documented sync helpers for RLS in Celery tasks, using parameterized `SET LOCAL` queries to prevent injection. - **`vite-env.d.ts`** fix (`/// `) correctly resolves the `ImportMeta.env` TypeScript error without over-typing. - **`GPUProbeResult` interface extended** (`timestamp`, `devices`, `render_time_s`) without breaking existing callers — optional fields only. --- ## Summary of Issues | # | File | Line | Severity | Description | |---|------|------|----------|-------------| | 1 | `cad.py` | 537 | Low | `updated_at` not set on `save_manual_material_overrides` | | 2 | `cad.py` | 101, 336 | Low | Pre-existing hardcoded `"admin"` role strings (not introduced by P2) | | 3 | `Admin.tsx` | 42, 50 | Low | Pre-existing `as any[]` casts (not introduced by P2) | | 4 | `export_step_to_usd.py` | 560 | Low | `schaeffler:partKey` in `customData` vs typed attribute (design choice, acceptable) | | 5 | `export_step_to_usd.py` | — | Low | No `FlattenLayerStack()` — not applicable at current scope | | 6 | `tenant_context.py` | 70, 100 | Low | New sync engine per call (performance concern only) | | 7 | `export_glb.py` | 89-95 | Medium | Confusing `eng.dispose()` in cache-hit early-return inside `with` block | | 8 | `ThreeDViewer.tsx` | 536 | Low | `as any` for scene userData (could use `THREE.Object3D`) | No critical or blocking issues. All new endpoints are properly secured, migrations are clean, the USD exporter correctly authors index-space primvars and sets `schaeffler:partKey` metadata, and the frontend type coverage of the new API surface is complete. --- ## Recommendation Approved. Fix issue #1 (`updated_at` missing in `save_manual_material_overrides`) and issue #7 (refactor `eng.dispose()` in cache-hit path for clarity) before next production deployment. All remaining items are Low severity and can be addressed in a follow-up cleanup commit.