# Review Report — P6/P9/P10
Date: 2026-03-12

## Result: ⚠️ Minor issues

---

## Problems Found

### [frontend/src/pages/OrderDetail.tsx:1698,1701] `bg-surface-hover/30` opacity syntax on CSS variable
**Severity**: Medium
**Detail**: `hover:bg-surface-hover/30` and `group-hover:bg-surface-hover/30` are used in two existing table-row class strings (lines 1698 and 1701). `surface-hover` is defined in `tailwind.config.js` as `'var(--color-bg-surface-hover)'` — a CSS variable. Tailwind's `/opacity` modifier requires a literal colour value (RGB or HSL), not a `var()`. The opacity modifier silently produces no effect at these two sites.
**Note**: These two lines are pre-existing and not part of the P10-2 diff, but they are in the file changed by this PR. Flag for a cleanup pass.
**Recommendation**: Replace with `style={{ backgroundColor: 'var(--color-bg-surface-hover)', opacity: 0.3 }}` or define a separate Tailwind colour with explicit RGBA values.

### [backend/app/api/routers/orders.py:1034] Redundant local import of `sql_update`
**Severity**: Low
**Detail**: `from sqlalchemy import update as sql_update` is imported locally inside `reject_order_line` at line 1034. The module already imports `update` at the top level (line 16). The local import is harmless but inconsistent with the rest of the file.
**Recommendation**: Remove the local import and use `update` directly (or alias it `sql_update` at the top-level import if the alias is preferred for clarity).

### [frontend/src/pages/OrderDetail.tsx:reject modal] Modal marks only `latest` notification read in batch
**Severity**: Low
**Detail**: In `NotificationCenter.tsx`, when a batch notification is clicked, only `latest.id` is passed to `markOneMutation.mutate(latest.id)`. The other notifications in the batch remain `unread`. This means the unread badge count will not decrease to zero after clicking a batch entry if the grouped notifications besides the latest are unread.
**Recommendation**: Collect all IDs in the batch (they are consumed into `j - i` slots) and mark them all read, or call a `markAllRead` mutation instead.

### [backend/app/domains/pipeline/tasks/export_glb.py] Cache miss path sets `step_file_hash` but not `render_config`
**Severity**: Low
**Detail**: When `effective_cache_key` is not `None` and there is no stored key match (cache miss), the code updates `cad_file.step_file_hash = _current_hash` and commits. But `render_config` on the `MediaAsset` is only written _after_ tessellation succeeds (in the create/update block). This is correct — `render_config` is written on the asset once it is created. However, the early `cad_file.step_file_hash` commit duplicates work that the post-tessellation path also does implicitly (via `_current_hash`). Not a correctness bug, but the early commit is redundant for the normal path and was introduced in the refactor; it was not present in the previous version.
**Recommendation**: Remove the two early `cad_file.step_file_hash = _current_hash; session.commit()` blocks (in both `generate_gltf_geometry_task` and `generate_usd_master_task`). The hash is already stored after tessellation completes. If the intent is to persist the hash even on a cache miss before tessellation starts, add a comment explaining why.

### [frontend/src/pages/Orders.tsx] P10-2 kanban drag-to-reject not implemented
**Severity**: Low / informational
**Detail**: The plan.md listed P10-2 (kanban drag-to-reject in `Orders.tsx`) as an open task. The diff contains no changes to `Orders.tsx`. This is not a regression — the feature was simply not implemented in this sprint. The per-line reject button in `OrderDetail.tsx` (P10-2 alternative) is a valid substitute for many workflows.
**Recommendation**: Track as carry-over to next sprint; not a merge blocker.

---

## Positives

### P6-2 — Admin.tsx progressive disclosure toggle
- `showAdvancedTess` state hides the four manual deflection inputs behind an "Advanced: manual deflection values" toggle using `ChevronRight`/`ChevronDown` icons from the existing lucide-react import. No new dependencies.
- The Save button is correctly placed **outside** the `{showAdvancedTess && ...}` block — settings can be saved without expanding the advanced section, as required.
- Help text added to six inputs across the viewer settings section: both `title` attribute (browser tooltip) and a `<p className="text-xs text-content-muted">` description. All descriptions are accurate.
- Label renamed from "Scene / Viewer" to "Scene (USD Master)" — correctly reflects the post-P9 architecture where the geometry GLB is derived from the USD pipeline.

### P9-1/P9-2 — Composite cache key in export_glb.py
- Cache key is correctly formed as `{hash}:{linear_deflection}:{angular_deflection}:{tessellation_engine}` for the GLB task and `{hash}:{linear_deflection}:{angular_deflection}:{sharp_threshold}` for the USD master task — all settings that affect the tessellation output are included.
- Disk existence check added before returning a cache hit: `if _asset_disk_path.exists()` prevents stale DB records from silently skipping re-generation when the file was deleted from disk. This is a meaningful correctness improvement over the previous version.
- `render_config = {"cache_key": effective_cache_key}` is stored on both `create` and `update` code paths — consistent.
- `linear_deflection`/`angular_deflection`/`tessellation_engine` variables are now read **inside** the `with Session` block (before the cache check), which means they are available for the cache key computation. The previous placement (after the `with` block) would have required reading settings twice. Clean refactor.
- `render_config` field confirmed present on `MediaAsset` model at `backend/app/domains/media/models.py:51` (`Mapped[dict | None] = mapped_column(JSONB, nullable=True)`).

### P9-3 — `step_hash` exposed in CAD API response
- `"step_hash": cad.step_file_hash` added to the `get_objects` endpoint at `cad.py:232`. Field is nullable; no schema change required. Clean, minimal addition.
- `logger = logging.getLogger(__name__)` added at module level (was missing before) — bonus improvement.

### P10-1 — Notification batching in NotificationCenter.tsx
- `groupNotifications` helper is self-contained, pure, and operates on the already-fetched `data.items` array. No backend round-trips.
- Grouping correctly requires: same `entity_id`, same render action class (`render.completed` or `render.failed`), and timestamps within 5 minutes. The `Math.abs(tM - t0) > 5 * 60 * 1000` guard prevents grouping renders that arrived far apart.
- `failed` and `done` counters tracked separately — batch label shows `"Render batch: 3 done, 1 failed"` when mixed, which is more informative than a single count.
- Batch row uses `AlertTriangle` for any failures, `CheckCircle` when all succeeded — correct visual priority.
- `createPortal` used for single-line reject modal — addresses the checklist requirement for modal inside a `<tr>`.

### P10-2 — Per-line reject button and modal in OrderDetail.tsx
- `createPortal(…, document.body)` correctly escapes the `<tr>` context. Modal has both backdrop-click and X-button close.
- `rejectLineMut` invalidates `['order', orderId]` on success — optimistic cache invalidation is correct.
- `canRejectLine` guard (`isPrivileged && line.item_status !== 'rejected'`) prevents double-reject. Already-rejected lines do not show the button.
- `rejectOrderLine` in `orders.ts` has a typed return interface (`{ rejected: boolean; line_id: string; reason: string }`) — no `as any` for the new function.
- Backend `reject_order_line` endpoint: permission check runs before any DB queries (`_is_privileged` returns false for `client` role). `order_id` used in the `OrderLine.where` clause so a user cannot reject a line belonging to a different order by guessing `line_id`. No SQL injection vectors.
- `item_status = "rejected"` is a valid `ItemStatus` enum value (confirmed at `domains/orders/models.py:53`).
- `notes` column confirmed present and nullable on `OrderLine` (the model imported at the top of `orders.py` from `app.models.order_line`).

### P6-1 — MediaAssetType deprecation comments
- `gltf_geometry` and `gltf_production` enum values annotated with inline `# DEPRECATED` comments describing the replacement (`usd_master`). Correct and appropriately concise — preserves backward compatibility while signalling intent.

### Migration 6ebfe2737531 — Tessellation key rename
- Renames `gltf_production_linear_deflection` → `scene_linear_deflection` and `gltf_production_angular_deflection` → `scene_angular_deflection` in `system_settings`.
- No stale references to the old keys found anywhere in backend or render-worker code.
- `downgrade()` correctly reverses both renames.

### admin.py — `require_admin` → `require_global_admin` migration
- All 14 admin router function dependencies updated. `require_global_admin` is defined in `auth.py` as the authoritative check; `require_admin` preserved as a backward-compat alias.

### render_blender.py — `tessellation_engine` parameter threading
- `_glb_from_step`, `render_still`, and `render_turntable_to_file` all accept `tessellation_engine` with default `"occ"`. Passed through to the subprocess CLI flag `--tessellation_engine`. Consistent with `step_processor._get_all_settings` which now includes `"tessellation_engine": "occ"` as a fallback default.

### export_step_to_usd.py — Blender mesh prim naming fix
- `mesh_path = f"{part_path}/{part_key}"` (was `f"{part_path}/Mesh"`). Blender 5.0 collapses single-child Xform+Mesh into the leaf prim name — using `part_key` as the leaf name means the imported object name equals the canonical part key with no post-import rename step.
- Learning documented in `LEARNINGS.md` with the precise Blender 5.0 USD import behaviour.

### General
- No SQL injection vectors. All DB writes use ORM or parameterized bulk-update.
- No `print()` introduced in backend Python files. Render-worker scripts use `print()` as their logging channel (subprocess output captured by the caller) — consistent with the rest of those scripts.
- No hardcoded file paths or credentials in any changed file.
- All FastAPI route handlers `async def`; all Celery tasks `def`. Async consistency maintained.
- LEARNINGS.md and ROADMAP.md updated in the same change set.

---

## Recommendation

Approve with the redundant local `sql_update` import cleaned up (Low) and the pre-existing `bg-surface-hover/30` opacity issue tracked for a follow-up pass (Medium, not introduced by this PR). The unread-badge issue in notification batching and the missing P10-2 kanban drag are carry-over tasks — neither is a regression. No blockers to merging.

---

Review complete. Result: ⚠️