fix: UI/UX polish — 42 issues across 3 waves

[PohTeyToe]

Summary

Comprehensive UI/UX polish addressing 42 actionable issues found during a code-level audit of the React dashboard. Changes organized into 3 waves:

Wave 1: Critical Bugs + Rule Violations

• Remove duplicate 401 interceptor that raced with AuthContext token refresh
• Convert ConfirmDialog to native <dialog> with role="alertdialog" and focus trap
• Add htmlFor/id label association to FormField component
• Short-circuit DetailDrawer render when closed (prevents unnecessary data fetching)
• Replace raw fetch in AuditLog with TanStack Query hook + fix Fragment key warning
• Replace hardcoded hex colors with CSS variables (Settings, Sidebar, ConfirmDialog)
• Add --accent-teal-dark CSS variable, remove non-functional tenant switcher hover
• Redirect authenticated users away from /login and /register

Wave 2: UX Improvements + Consistency

• Reports: replace inline table styles with CSS Module classes, chart colors from CSS vars
• Suppliers: extract performance modal inline styles to dedicated CSS Module
• CsvImport: replace inline result card styles, add toast for non-CSV file drops
• Login/Register: add password visibility toggle (Eye/EyeOff), consolidate shared CSS
• Register: inline password mismatch validation on blur, success toast on registration
• Inventory: add sortable column headers (SKU, Name, Qty, Unit Price) + shared Pagination
• PurchaseOrders: keep pipeline visible when filter active, highlight active stage
• Categories & Warehouses: add ExportDropdown with CSV + PDF export
• StockMovements: fix Record Movement button size
• Sidebar: add ConfirmDialog before logout

Wave 3: Accessibility + Error States + Polish

• Create shared <ErrorState> component with AlertTriangle icon and retry button
• Add ErrorState to all 6 CRUD pages for API failure recovery
• Disable save buttons during mutation with "Saving..." feedback
• Add aria-label to all icon-only action buttons and bulk select checkboxes
• ExportDropdown: keyboard navigation (Arrow keys, Enter, Escape) + ARIA attributes
• Header: only render search input when onSearchClick is provided
• Dashboard.module.css: remove 5 unused CSS classes

Test plan

• npm run build passes with no errors
• npm test — all 69 tests pass
• CI pipeline (lint, typecheck, test, build) should pass
• Manual: visit /login while authenticated → redirect to /
• Manual: ConfirmDialog opens as <dialog>, Tab trapped, Escape closes
• Manual: password toggle works on Login/Register
• Manual: ExportDropdown navigable via keyboard
• Manual: error state appears when API is down, retry works

🤖 Generated with Claude Code

[github-actions]

Preview Deploy

Status: ✅ Deployed successfully
Preview: https://logistics-dashboard-73k9qhdht-abdallah-safis-projects-bae5f9c3.vercel.app
Commit: b23f8ef feat: add error states, loading feedback, and accessibility
Updated: Mar 14, 12:27 PM UTC

Deployment History

Commit	Preview	Updated (UTC)
b23f8ef feat: add error states, loading feedback, and accessibility	Open	Mar 14, 12:27 PM

Build Logs (last 50 lines)

``` dist/assets/suppliers-CRoaLqw8.js 0.46 kB │ gzip: 0.23 kB dist/assets/FormField-Czi0nmxX.js 0.53 kB │ gzip: 0.34 kB dist/assets/shared.module-BcxFr9pL.js 0.57 kB │ gzip: 0.28 kB dist/assets/usePagination-B9fiFiWr.js 0.68 kB │ gzip: 0.43 kB dist/assets/exportCsv-BU8gXNVC.js 0.74 kB │ gzip: 0.50 kB dist/assets/DetailDrawer-Ct8bM7UZ.js 1.08 kB │ gzip: 0.57 kB dist/assets/Pagination-zNaoGULI.js 1.28 kB │ gzip: 0.70 kB dist/assets/useBulkSelect-DCzvbVYa.js 1.43 kB │ gzip: 0.74 kB dist/assets/CrudPage.module-DckKj5DD.js 1.79 kB │ gzip: 0.75 kB dist/assets/useTableSort-LoniQF40.js 1.95 kB │ gzip: 0.94 kB dist/assets/CsvImport-BJ_ySymu.js 3.40 kB │ gzip: 1.33 kB dist/assets/AuditLog-BxFaE0Nx.js 5.32 kB │ gzip: 2.06 kB dist/assets/Reports-BbJ3kGWM.js 5.44 kB │ gzip: 2.01 kB dist/assets/Categories-DzVOyvWb.js 6.04 kB │ gzip: 2.26 kB dist/assets/Warehouses-Bz2AuWq8.js 7.04 kB │ gzip: 2.54 kB dist/assets/StockMovements-DSV5x0b-.js 7.04 kB │ gzip: 2.67 kB dist/assets/Suppliers-BEl4blXs.js 9.23 kB │ gzip: 3.13 kB dist/assets/Settings-Cl8ucCdw.js 10.93 kB │ gzip: 3.39 kB dist/assets/Analytics-DsCqdOYe.js 11.06 kB │ gzip: 3.86 kB dist/assets/PurchaseOrders-Lb5F2B4k.js 13.81 kB │ gzip: 4.55 kB dist/assets/Inventory-CeyPjMYz.js 16.28 kB │ gzip: 4.73 kB dist/assets/purify.es-CovBOfck.js 22.58 kB │ gzip: 8.67 kB dist/assets/vendor-react-B9CkSc1y.js 48.73 kB │ gzip: 17.17 kB dist/assets/index.es-Bqw8QK-r.js 158.75 kB │ gzip: 53.02 kB dist/assets/html2canvas.esm-DXEQVQnt.js 201.04 kB │ gzip: 47.43 kB dist/assets/vendor-recharts-C9M_SBP_.js 411.05 kB │ gzip: 120.07 kB dist/assets/exportPdf--w_gQJe0.js 419.18 kB │ gzip: 136.62 kB dist/assets/index-C1J_kQfD.js 473.72 kB │ gzip: 146.32 kB ✓ built in 6.88s Build Completed in .vercel/output [23s] Retrieving project… Deploying abdallah-safis-projects-bae5f9c3/logistics-dashboard Uploading [--------------------] (0.0B/1.2MB) Uploading [=====---------------] (301.5KB/1.2MB) Uploading [==========----------] (607.7KB/1.2MB) Uploading [===============-----] (895.7KB/1.2MB) Uploading [====================] (1.2MB/1.2MB) Inspect: https://vercel.com/abdallah-safis-projects-bae5f9c3/logistics-dashboard/CWnigXaMsGRNKNeK18yMSKhrqQyk [3s] Preview: https://logistics-dashboard-73k9qhdht-abdallah-safis-projects-bae5f9c3.vercel.app [3s] https://logistics-dashboard-73k9qhdht-abdallah-safis-projects-bae5f9c3.vercel.appBuilding... Building... Building: Running build in Portland, USA (West) – pdx1 Building: Build machine configuration: 2 cores, 8 GB Building: Retrieving list of deployment files... Building: Previous build caches not available. Building: Downloading 49 deployment files... Preview: https://logistics-dashboard-73k9qhdht-abdallah-safis-projects-bae5f9c3.vercel.app [7s] Completing... To deploy to production (logistics-dashboard-pi.vercel.app), run `vercel --prod`

</details>
📦 [Full build logs](https://github.com/PohTeyToe/LogisticsInventorySystem/actions/runs/23087926922) · [Download artifact](https://github.com/PohTeyToe/LogisticsInventorySystem/actions/runs/23087926922#artifacts)
Deployed via Vercel CLI — preview environment only (production is on Azure).
<!-- DEPLOY_HISTORY
b23f8ef|https://logistics-dashboard-73k9qhdht-abdallah-safis-projects-bae5f9c3.vercel.app|2026-03-14T12:27:18.728Z|feat: add error states, loading feedback, and accessibility 
-->

[claude]

Code Review: PR #27 — UI/UX Polish (42 issues across 3 waves)

Executive Summary

• Solid quality overall. 34 files, +594/−292. All changes are frontend-only (React dashboard). The scope is well-organized into 3 logical waves.
• Good wins: native <dialog> migration, removal of duplicate 401 interceptor race, inline-styles → CSS Modules, TanStack Query migration for AuditLog, consistent ErrorState/retry across all CRUD pages.
• One correctness bug: ConfirmDialog's early return null unmounts the <dialog> before useEffect can call .close(), causing stale DOM state on re-renders.
• One UX bug: Register page navigates immediately after addToast(), so the success toast is never visible.
• Minor theme concern: color: var(--bg-surface) in Settings is semantically wrong and will break if a light theme is introduced.
• Sidebar duplicates 9 lines of ConfirmDialog JSX across mobile/desktop branches — easily extractable.

Top Risks

1. ConfirmDialog: early return null races with useEffect calling .close()

Severity: High

When open transitions from true → false, the component returns null (line 42) before the useEffect (line 30-35) fires, so dialog.close() is never called on the DOM element. The dialog element is unmounted while still in the browser's modal state. If the browser retains the top layer entry, subsequent showModal() calls may throw.

File: src/logistics-dashboard/src/components/shared/ConfirmDialog.tsx:30-42

// Current: returns null before useEffect can call .close()
if (!open) return null;

Fix: Remove the early return. Instead, always render the <dialog> and let useEffect manage open/close state:

// Remove: if (!open) return null;
// Add opacity/display control via CSS when !open, or simply let
// dialog.close() handle it and conditionally render children:
return (
  <dialog ref={dialogRef} className={styles.overlay} ...>
    {open && (
      <div className={styles.card}>...</div>
    )}
  </dialog>
);

---

2. Register success toast is invisible — navigation fires immediately

Severity: Medium

addToast('Account created successfully!', 'success') is followed immediately by navigate('/', { replace: true }), which unmounts the Register page and its ToastContainer.

File: src/logistics-dashboard/src/pages/Register.tsx:33-34

addToast('Account created successfully!', 'success');
navigate('/', { replace: true });

Fix: Either (a) remove the toast and rely on the redirect, (b) move the toast to a global/layout-level ToastContainer, or (c) delay navigation with setTimeout(() => navigate(...), 1500) (least ideal).

---

3. Settings: semantic CSS variable misuse

Severity: Medium

color: var(--bg-surface) uses a background-intended variable for foreground text. This happens to work in the current dark theme (--bg-surface: #0D1117 → dark text on teal button) but will break if a light theme is added where --bg-surface is white.

File: src/logistics-dashboard/src/pages/Settings.module.css:61

color: var(--bg-surface) !important;

Fix: Use a dedicated text-on-accent variable or a specific dark color variable. The original #0D1117 was at least explicit; replacing it with a variable whose semantics don't match is arguably worse.

---

4. getChartColors() reads computed styles on every render

Severity: Medium

getChartColors() calls getComputedStyle(document.documentElement) and reads 8 CSS properties. It's called unconditionally in the component body, so it runs on every render of Reports.

File: src/logistics-dashboard/src/pages/Reports.tsx:17-20

function getChartColors(): string[] {
  const root = getComputedStyle(document.documentElement);
  return Array.from({ length: 8 }, (_, i) => root.getPropertyValue(`--chart-color-${i + 1}`).trim() || '#8B949E');
}
// Called on every render:
const chartColors = getChartColors();

Fix: Wrap in useMemo or useRef with lazy init:

const chartColors = useMemo(() => getChartColors(), []);

---

5. Sidebar: duplicated ConfirmDialog JSX

Severity: Low

The same 8-line ConfirmDialog block is copy-pasted in both the mobile and desktop return paths (lines 152-160 and 169-177).

File: src/logistics-dashboard/src/components/layout/Sidebar.tsx:135-179

Fix: Render the ConfirmDialog once outside the conditional:

return (
  <>
    {mobileOpen ? (
      <>
        <div className={styles.backdrop} onClick={onClose} role="presentation" />
        {sidebarContent}
      </>
    ) : (
      sidebarContent
    )}
    <ConfirmDialog ... />
  </>
);

---

6. FormField: generated id may collide across multiple forms

Severity: Low

label.toLowerCase().replace(/\s+/g, '-') generates IDs like "name", "email" etc. If two forms render simultaneously on the same page (e.g. a modal over a page), IDs will collide, breaking label association.

File: src/logistics-dashboard/src/components/shared/FormField.tsx:10

Fix: Use React's useId() as the fallback instead of deriving from label text:

const autoId = useId();
const fieldId = id || autoId;

---

7. ExportDropdown: focus not returned to trigger on close

Severity: Low

After selecting an item or pressing Escape, focus is not explicitly returned to the trigger button. Keyboard-only users lose their place.

File: src/logistics-dashboard/src/components/shared/ExportDropdown.tsx:42-43

Fix: Add a ref to the trigger and call triggerRef.current?.focus() after setOpen(false).

---

8. Reports: one remaining inline style

Severity: Low

style={{ color: 'var(--status-danger)' }} remains in the low-stock table for currentQuantity.

File: src/logistics-dashboard/src/pages/Reports.tsx:133 (approx)

This is inconsistent with the otherwise complete migration to CSS Module classes in this same PR.

---

Quick Wins

1. ConfirmDialog test polyfill: The else branch in test/setup.ts:12-13 creates an HTMLDialogElement class that doesn't extend HTMLElement from jsdom's prototype chain and lacks showModal/close. Consider using a proper polyfill package or at minimum adding the methods.
2. Remove unused useToast import in AuditLog.tsx:48 — addToast is destructured but no longer used (the TanStack Query migration removed the only addToast call).
3. CsvImport.tsx:87 — remaining style={{ marginTop: 16 }} inline style on the result cards container.

Non-Blocking Suggestions

1. Global ToastContainer: Several pages independently render <ToastContainer>. A single layout-level toast provider would simplify and fix the Register toast issue.
2. ErrorState + loading skeleton composition: Consider a <QueryState> wrapper that renders skeleton/error/children based on query status, reducing the repeated {isError && !loading && <ErrorState ... />} pattern across 6 pages.
3. PurchaseOrders pipeline toggle uses dashStyles[stage${status}] — these dynamic class lookups bypass CSS Module dead-code analysis. Consider a lookup map for better tree-shaking and type safety.
4. Consistent aria-label naming: Some buttons use "Edit item" (generic) while others use "Edit ${item.name}" (specific). Pick one pattern.

---

Overall: This is a well-structured, high-value polish PR. The ConfirmDialog race (Risk #1) is the only item I'd flag as must-fix before merge. The rest can be addressed here or in a follow-up.

Rating: COMMENT — no blocking issues that prevent merge, but Risk #1 should ideally be addressed.

[claude]

Inline comments on specific code locations — see summary comment for full report.

[claude]

High: The early return null executes before the useEffect (lines 30-35) can call dialog.close(). When open goes true→false, the <dialog> is unmounted from DOM while still in the browser's top-layer/modal state. This can cause showModal() to throw on subsequent opens.

Fix: always render the <dialog> element, conditionally render only the card contents:

return (
  <dialog ref={dialogRef} className={styles.overlay} ...>
    {open && <div className={styles.card}>...</div>}
  </dialog>
);

[claude]

Medium: addToast() fires but navigate() immediately unmounts this page and its <ToastContainer>, so the user never sees the success toast. Consider a global toast provider, removing the toast, or delaying navigation.

[claude]

Medium: getChartColors() calls getComputedStyle() and reads 8 CSS properties on every render. Wrap in useMemo:

const chartColors = useMemo(() => getChartColors(), []);

[claude]

Low: This ID generation collides if two FormFields with the same label exist simultaneously (e.g., modal form + page form both with "Name"). Use useId() as fallback:

const autoId = useId();
const fieldId = id || autoId;

[claude]

Low: The <ConfirmDialog> block is duplicated in both mobile (line 152) and desktop (line 169) return paths. Extract it outside the conditional to avoid the duplication.

[claude]

Code Review: fix: UI/UX polish — 42 issues across 3 waves

Reviewer verdict: REQUEST_CHANGES — 2 bugs to fix before merge, several medium items to address.

---

Executive Summary

• Solid quality-of-life PR — migrating to native <dialog>, adding TanStack Query for AuditLog, extracting reusable ErrorState/ExportDropdown/Pagination, and cleaning up dead CSS
• ConfirmDialog has a race condition — if (!open) return null on line 42 conflicts with the showModal() effect, meaning modal behavior (backdrop, focus trap, top-layer) may not activate reliably
• 401 interceptor removal creates a gap — requests firing before AuthProvider mounts will silently swallow 401s instead of redirecting to login
• CSS variable violations — several hardcoded rgba() values in new CSS classes, violating the project's CSS-variables-only constraint
• Good accessibility progress but several a11y patterns are incomplete (missing role="alert", focus return on Escape, aria-describedby for errors)
• CI is green — lint, typecheck, tests, build, CodeQL all pass

---

Top Risks

1. ConfirmDialog: showModal() never called due to early return guard

Severity: Critical

When open is false, line 42 returns null, unmounting the <dialog>. When open flips to true, React renders the <dialog> for the first time and runs the effect — this works on the initial open. But the real issue is that the <dialog> is conditionally mounted/unmounted rather than persistently in the DOM, which means:

• Close animations are impossible (element is removed instantly)
• The dialog.close() branch in the effect fires against a stale ref
• If React batches state updates, the effect may miss transitions

File: src/logistics-dashboard/src/components/shared/ConfirmDialog.tsx:30-42

Fix: Remove the early return. Always render the <dialog> and let showModal()/close() control visibility. Also merge the two effects into one:

useEffect(() => {
  const dialog = dialogRef.current;
  if (!dialog) return;
  if (open && !dialog.open) {
    dialog.showModal();
    cancelRef.current?.focus();
  } else if (!open && dialog.open) {
    dialog.close();
  }
}, [open]);

// Remove: if (!open) return null;

---

2. 401 handler removed from Axios client with no fallback

Severity: High

The old 401 handler in client.ts was present from module load time. The replacement in AuthContext.tsx is registered inside a useEffect, creating a window where any API request (e.g., from an eagerly-executing TanStack Query) gets a 401 with no handler — silent failure, no redirect.

File: src/logistics-dashboard/src/api/client.ts (removed lines 37-46)

Fix: Keep a baseline 401 handler in client.ts at module scope:

client.interceptors.response.use(undefined, (error) => {
  if (error.response?.status === 401) {
    localStorage.removeItem('token');
    window.location.href = '/login';
  }
  return Promise.reject(error);
});

AuthContext can then eject this and register a smarter refresh-token interceptor.

---

3. Hardcoded rgba() colors violate CSS-variables-only constraint

Severity: Medium

Multiple new CSS classes use raw rgba() values instead of CSS variables.

Files:

• src/logistics-dashboard/src/pages/CrudPage.module.css — .resultCardSuccess, .resultCardError
• src/logistics-dashboard/src/components/shared/ConfirmDialog.module.css — lines 59, 112-113

Fix: Add dim variants to theme.css:

--status-success-dim: rgba(63, 185, 80, 0.08);
--status-success-border: rgba(63, 185, 80, 0.2);
--status-danger-dim: rgba(248, 81, 73, 0.08);
--status-danger-border: rgba(248, 81, 73, 0.2);

---

4. Sidebar: duplicated ConfirmDialog JSX

Severity: Medium

The <ConfirmDialog> block is copy-pasted identically in both the mobileOpen and desktop branches of Sidebar.tsx. Any prop change must be made in two places.

File: src/logistics-dashboard/src/components/layout/Sidebar.tsx:135-166

Fix: Render <ConfirmDialog> once, outside the conditional branches.

---

5. AuditLog: params object recreated every render

Severity: Medium

The params object is constructed as a plain literal every render. While TanStack Query v5 does deep comparison, new Date(dateFrom).toISOString() can produce microsecond-different strings across renders, causing cache misses.

File: src/logistics-dashboard/src/pages/AuditLog.tsx:50-55

Fix: Wrap in useMemo keyed on [page, pageSize, entityType, dateFrom, dateTo].

---

6. ExportDropdown: keyboard a11y gaps

Severity: Medium

• Escape closes menu but doesn't return focus to trigger button (WAI-ARIA menu button requirement)
• Tab key not handled — menu stays visually open when user tabs away
• Missing aria-controls linking trigger to menu

File: src/logistics-dashboard/src/components/shared/ExportDropdown.tsx

Fix: Add triggerRef, focus it on Escape/select. Handle Tab to close menu. Add aria-controls + id on menu.

---

7. useAuditLogQueries: type safety bypassed

Severity: Medium

Hook accepts Record<string, string | number> instead of AuditLogParams, allowing arbitrary keys to pass type checking.

File: src/logistics-dashboard/src/hooks/queries/useAuditLogQueries.ts:9

Fix: export function useAuditLogs(params: AuditLogParams = {})

---

8. Pagination imports page-specific CSS Module

Severity: Medium

Pagination.tsx (in shared/) imports ../../pages/CrudPage.module.css — tight coupling to a page module. If CrudPage styles change, Pagination breaks.

File: src/logistics-dashboard/src/components/shared/Pagination.tsx:2

Fix: Extract pagination-specific classes into Pagination.module.css.

---

9. FormField: ID collision from label text

Severity: Low

ID is derived from label.toLowerCase().replace(/\s+/g, '-'). Two fields with the same label (e.g., "Name" in different modals) produce duplicate IDs.

File: src/logistics-dashboard/src/components/shared/FormField.tsx:10

Fix: Use React's useId() hook as fallback instead of label derivation.

---

10. DetailDrawer: missing modal ARIA semantics

Severity: Low

The drawer lacks role="dialog", aria-modal="true", and has no focus trap. Users can Tab into obscured content behind the overlay.

File: src/logistics-dashboard/src/components/shared/DetailDrawer.tsx

---

Quick Wins

1. Add role="alert" to ErrorState.tsx outer div
2. Use item-specific aria-labels in Inventory.tsx (match the Categories.tsx pattern: `Edit ${item.name}`)
3. Add aria-describedby linking ConfirmDialog message to the dialog element
4. AuditLog page: add <ErrorState> for isError (all other CRUD pages got it, this one was missed)
5. Add aria-hidden="true" to DetailDrawer overlay div

Non-Blocking Suggestions

• Consider --text-on-danger: #fff variable instead of var(--text-primary) for danger button hover text — future-proof for light theme
• ConfirmDialog tests should verify showModal() is called (jsdom doesn't implement it natively — may need a polyfill or mock)
• ExportDropdown: clear stale refs in menuItemRefs.current when menu closes
• AuditLog useAuditLogs hook: consider enabled guard if the component is ever rendered conditionally

---

Reviewed by Claude Code — requested changes on 2 items (ConfirmDialog race condition, 401 handler gap), 6 medium-severity items to address.

🤖 Generated with Claude Code

[claude]

REQUEST_CHANGES — 2 blocking issues, 6 medium items.

Blocking:

1. ConfirmDialog race condition — if (!open) return null on line 42 conflicts with showModal() effect. The dialog may render without modal behavior (no backdrop, no focus trap, no top-layer). Remove the early return and always render the <dialog> element.

2. 401 interceptor gap — removing the 401 handler from client.ts creates a window where early API requests (before AuthProvider mounts) silently swallow 401 errors. Keep a baseline handler at module scope.

Medium (should fix):
3. Hardcoded rgba() in CrudPage.module.css and ConfirmDialog.module.css — add CSS variables
4. Sidebar has duplicated ConfirmDialog JSX — render once outside the conditional
5. AuditLog params object needs useMemo to prevent cache misses
6. ExportDropdown: focus return on Escape, Tab handling, aria-controls
7. useAuditLogQueries should use AuditLogParams type, not Record<string, string | number>
8. Pagination.tsx imports CrudPage.module.css — extract its own CSS Module

See full review comment for details, quick wins, and non-blocking suggestions.