HartOMat/review-report.md

Review Report: Material Alias Completeness with Blocking Dialog

Date: 2026-03-14

Result: ✅ Approved

Checklist Results

Backend / Python

• New endpoints have role checks: check_materials uses get_current_user, batch_create_aliases uses require_admin_or_pm
• No SQL injections — all ORM usage
• Pydantic input validation: BatchAliasCreate with BatchAliasMapping for POST body
• Invalid IDs return 404: order not found → 404, material not found → 404
• No new routers — endpoints added to existing orders and materials routers
• No new models — uses existing Material, MaterialAlias
• Async consistency: all handlers are async def
• No print() in production code
• No hardcoded paths
• storage_key not touched

Database

• No migration needed — uses existing tables

Frontend / TypeScript

• New API interfaces in frontend/src/api/materials.ts: MaterialSuggestion, UnmappedMaterial, UnmappedMaterialCheck
• No as any for API responses — correct types throughout
• CSS vars use inline style where needed (style={{ backgroundColor: 'var(--color-bg-surface)' }})
• Loading states: checkingMaterials for button, saving in dialog, quickMapMut.isPending
• Error feedback: error state in dialog, toast on quick-map success/failure
• No new role-dependent UI elements (dispatch button already gated by canDispatch)
• TypeScript compiles clean (tsc --noEmit passes)

Render Pipeline

• Material alias lookup order preserved: aliases FIRST, then exact name
• find_unmapped_materials() follows same resolution logic as resolve_material_map()

Security

• No credentials in code
• No hardcoded tokens
• 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.

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

Recommendation

Approved — ready to commit.