docs: update review report for UI/UX cleanup
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
This commit is contained in:
+27
-43
@@ -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.
|
||||
|
||||
Reference in New Issue
Block a user