Hartmut
cc3071297b
- export_step_to_usd.py: accept --material_map CLI arg, write
schaeffler:canonicalMaterialName as customData on each Mesh prim,
fix geometry transform (strip shape Location before face exploration,
apply both face_loc and shape_loc sequentially)
- import_usd.py: after Blender USD import, use pxr to read customData
directly from the USD file — builds {part_key: material_name} lookup
(Blender ignores STRING primvars and customData, but pxr reads both)
- _blender_materials.py: add apply_material_library_direct() for exact
dict-based material assignment without name-matching heuristics
- _blender_scene_setup.py: prefer direct USD lookup, fall back to
name-matching for legacy USD files without material metadata
- export_glb.py (generate_usd_master_task): resolve material_map via
material_service.resolve_material_map() and pass to subprocess;
include material hash in cache key for invalidation
- ROADMAP.md: update P5 status, add M5-M7 milestones
Tested: 3/3 parts matched (ans_lfs120), 172/175 parts matched
(F-802007.TR4-D1-H122AG). Previous: 0/25 matched.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
107 lines
10 KiB
Markdown
107 lines
10 KiB
Markdown
# 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: ⚠️
|