diff --git a/review-report.md b/review-report.md index fa25ac5..2429445 100644 --- a/review-report.md +++ b/review-report.md @@ -1,61 +1,45 @@ -# Review Report: Material Alias Completeness with Blocking Dialog -Date: 2026-03-14 +# Review Report: Full UI/UX Cleanup & Simplification +Date: 2026-03-15 ## Result: ✅ Approved ## Checklist Results -### Backend / Python -- [x] New endpoints have role checks: `check_materials` uses `get_current_user`, `batch_create_aliases` uses `require_admin_or_pm` -- [x] No SQL injections — all ORM usage -- [x] Pydantic input validation: `BatchAliasCreate` with `BatchAliasMapping` for POST body -- [x] Invalid IDs return 404: order not found → 404, material not found → 404 -- [x] No new routers — endpoints added to existing `orders` and `materials` routers -- [x] No new models — uses existing `Material`, `MaterialAlias` -- [x] Async consistency: all handlers are `async def` -- [x] No `print()` in production code -- [x] No hardcoded paths -- [x] `storage_key` not touched - -### Database -- [x] No migration needed — uses existing tables - ### Frontend / TypeScript -- [x] New API interfaces in `frontend/src/api/materials.ts`: `MaterialSuggestion`, `UnmappedMaterial`, `UnmappedMaterialCheck` -- [x] No `as any` for API responses — correct types throughout -- [x] CSS vars use inline style where needed (`style={{ backgroundColor: 'var(--color-bg-surface)' }}`) -- [x] Loading states: `checkingMaterials` for button, `saving` in dialog, `quickMapMut.isPending` -- [x] Error feedback: error state in dialog, toast on quick-map success/failure -- [x] No new role-dependent UI elements (dispatch button already gated by `canDispatch`) -- [x] TypeScript compiles clean (`tsc --noEmit` passes) +- [x] TypeScript compiles clean (`npx tsc --noEmit` — zero errors) +- [x] No `bg-surface/50` Tailwind opacity syntax with CSS vars +- [x] Loading states preserved (all existing `isPending` usage untouched) +- [x] Error feedback preserved (all existing `toast.error` calls untouched) +- [x] Role-dependent UI elements unchanged +- [x] No new API interfaces needed (frontend-only visual refactor) -### Render Pipeline -- [x] Material alias lookup order preserved: aliases FIRST, then exact name -- [x] `find_unmapped_materials()` follows same resolution logic as `resolve_material_map()` +### Backend / Database / Render Pipeline +- [x] N/A — all changes are frontend-only (6 component/page files + plan.md) ### Security - [x] No credentials in code -- [x] No hardcoded tokens +- [x] No hardcoded tokens or secrets - [x] English variable names and comments ## Minor Notes (non-blocking) -### `UnmappedMaterialsDialog.tsx:56` — `bg-amber-500/10` opacity syntax -Uses Tailwind opacity on `bg-amber-500/10` and `border-amber-500/30`. This is fine because `amber-500` is a static Tailwind color (not a CSS variable), so the `/opacity` syntax works correctly here. No issue. - -### `UnmappedMaterialsDialog.tsx:99` — `.sort()` mutates in render -The `libraryMaterials.sort(...)` inside JSX mutates the filtered array on every render. Functionally harmless since `libraryMaterials` is a fresh array from `.filter()`, but a `[...libraryMaterials].sort(...)` or `useMemo` would be cleaner. Non-blocking. - -### `dispatch_single_line_render` endpoint in orders.py -This endpoint (and its frontend `dispatchLineRender` function + `dispatchLineMut` usage in `OrderLineRow`) appeared in the diff but is not part of the material alias plan. It's a separate per-line dispatch feature. It looks correct: proper auth (`require_admin_or_pm`), 404 checks, status validation, same dispatch pattern as the bulk dispatch endpoint. +### `as any` usage in val()/set() helpers +**Severity**: Low +The RenderTemplateTable uses `(editDraft as any)[field]` and `(form as any)[field]` in the shared `renderEditFormGrid()` helper — same pattern as the reference OutputTypeTable implementation. This is a TypeScript limitation with dynamic field access on union types. Non-blocking; could be improved with generics later but works correctly. ## Positives -- Clean separation: service function (`find_unmapped_materials`) is reusable and follows the same resolution logic as `resolve_material_map()` -- Deduplication in material name collection (`seen` set, case-insensitive) -- Graceful fallback: if `checkOrderMaterials` fails, dispatch proceeds anyway (no blocking on network errors) -- Suggestions via `SequenceMatcher` give useful context without external dependencies -- Quick-map on Materials page provides a second entry point for the same workflow -- Batch alias endpoint reuses existing `MaterialAlias` model — no schema changes needed + +1. **Consistent pattern across all 4 admin tables**: OutputTypeTable, RenderTemplateTable, PricingTierTable, and GlobalRenderPositionsPanel all now use the identical expandable edit row pattern — display row always visible, edit form as full-width colSpan row below with accent border. + +2. **RenderTemplateTable column reduction**: Consolidated 4 boolean columns (Mat Replace, Lighting Only, Shadow Catcher, Camera Orbit) into a single "Flags" column with compact badges — reduces visual width from 11 to 8 columns. + +3. **Shared renderEditFormGrid()**: Both RenderTemplateTable and PricingTierTable use a shared helper for add/edit forms, keeping the pattern DRY. + +4. **OrderDetail material override UX**: The per-line override now shows a compact "+ override" link instead of always-visible dropdown — significantly reduces visual noise while keeping functionality accessible. + +5. **WorkerManagement controls**: Larger touch targets (p-2 rounded-lg) make the scale controls usable on touch devices. + +6. **Billing status indicator**: ChevronDown icon next to the status select makes the interactivity obvious without changing the badge aesthetic. ## Recommendation -Approved — ready to commit. +Approved — ready to merge.