Major update for v1 and tests

docs/SUGGESTIONS.md

@@ -0,0 +1,193 @@
+# Apparel Designer — Suggestions
+
+This is the parallel document to `BUGS.md`. Where that file says "this is broken," this one says "this works, but it could be better." Suggestions are grouped by what kind of work they are: things the user feels, things the team feels, and things the server feels.
+
+Items that have been implemented since the original audit have been removed from this file. See `CHANGES.md` for the historical record.
+
+---
+
+## What the user feels
+
+### S1 — Confirm before destructive template switch
+Switching templates wipes the canvas (correct behavior — it would be worse to pile elements from each template on top of the previous). It's the right outcome, but it's a cliff. A small confirm modal ("Switching to '<template>' will replace your current design — keep going?") avoids accidental data loss without adding much friction. Could be a one-time dismissable warning.
+
+### S2 — Export resolution presets
+4500×4500 PNG is the right number for print, but it's overkill for sharing a design preview. Add a small dropdown on the export button: "Print (4500×4500)", "Web (1500×1500)", "Thumbnail (500×500)". The server already has the scale factor parameterized; the only real work is plumbing it through.
+
+### S3 — Undo/redo buttons that show what they'll undo
+Hovering "Undo" could show "Undo: Move text" or similar. Requires labeling history entries when they're created. Low-priority polish, but it's a small thing that makes the editor feel responsive.
+
+### S4 — Visible loading indicator while the background-removal model downloads
+86MB on first use is a lot. The current spinner shows progress, but it's small and inside the button. A more prominent "Downloading background removal model — this happens once" toast would be reassuring.
+
+---
+
+## What the team feels
+
+### S5 — Tests
+Zero tests in the repo. A Vitest setup with the following targets pays for itself quickly:
+- `useDesignEditor` history mechanics (the slice/push/shift/index-shifting logic in particular is subtle enough to deserve regression tests, plus the M7 dedupe, the new restoration-aware `initializeHistory` flow, and the multi-select / drag-reorder paths added in S3)
+- `useTemplate.calculateAutoCrop` and `assignImageToSlot`
+- `server.js` `/api/export` rendering against a small set of golden-image fixtures, including filter pipeline (S21) and stroke text (S8)
+- The hot-path bound functions in `DesignCanvas.jsx` (`constrainTransform`, `canvasDragBound`) — assertions that they return the same `newBox` reference when in-bounds, plus targeted tests on the rotation slide-or-revert and scale-clamp branches
+- The Zod export-request schema (S19) — fuzz invalid bodies and assert 400s, then verify the renderer accepts the validated shape
+- The newly-extracted shared utilities — `loadImageDimensions`, `unflipReportedXY`, `getNodePageRect`, `useTimer`, `safeLocalStorage` (readJson/writeJson/removeKey), `makeElementId`. Most are pure functions and trivial to cover.
+
+### S6 — Type checking
+The project is plain JS with `@types/react` installed but no `tsconfig.json`. A non-blocking step toward TypeScript would be enabling `// @ts-check` at the top of the heavier files (`useDesignEditor`, `server.js`, `useTemplate`, `DesignCanvas`) — the JSDoc annotations are already pretty good. The newly-extracted utility modules in `src/utils/` and `src/constants/` are good first targets — small, mostly pure, well-documented function signatures.
+
+### S7 — Storybook or a component playground
+Filerobot is a heavyweight modal, the layers panel has several states (selected / multi-selected / dragging), the slot placeholder has empty / loading / filled looks, and `PreviewModal` has loading/error/success states. A Storybook would speed up iteration on each in isolation.
+
+### S8 — Continue the CSS consolidation pass
+S13 set up the foundation: `.visually-hidden`, `.sr-only`, `.btn-reset`, and the shared `fade-up-in` keyframe live in `index.css`, and `App.css`'s old `toast-in` was migrated to use it. The remaining ~13 component stylesheets weren't fully swept — running through them and replacing redundant button-resets and one-off animations with the shared utilities is straightforward follow-on work, just mechanical.
+
+### S9 — Stream the export file instead of buffering
+`canvas.toBuffer('image/png')` then `writeFileSync` then `res.json` with a URL the client immediately fetches is three hops. Streaming the PNG directly through `canvas.createPNGStream().pipe(res)` cuts memory and latency for large exports — though it does mean dropping the "saved file URL" model in favor of "Save As" downloads.
+
+---
+
+## Cross-cutting
+
+### S10 — Continue the i18n sweep
+S23 set up the scaffold: `src/i18n/messages.js` (flat `en` catalog, ~50 keys covering header, toasts, cart, sidebar tabs, text tab, toolbar, layers, bounds, and preview modal), `src/i18n/t.js` (10-line helper with `{name}` interpolation and missing-key surfacing), and the `App.jsx` toasts + `Header.jsx` button labels are migrated. `Sidebar.jsx`, `TextTab.jsx`, `ElementToolbar.jsx`, `LayersPanel.jsx`, and `PreviewModal.jsx` still have hardcoded strings that should be migrated to the catalog. The pattern is established and the keys are already in the catalog — `t('text.field.font')`, `t('toolbar.delete')`, etc. — so the sweep is mechanical.
+
+Adjacent: B12 (in `BUGS.md`) flags `handleShare`'s `navigator.share` strings, App-level FAB aria-labels, and the export-toast verbiage as the missed cohort that doesn't show up in DOM-text greps.
+
+### S11 — Design token doc
+`index.css` defines a sensible token system (`--accent`, `--radius-md`, etc), and the Pawfectly Yours redesign expanded it with brand colors. It's not documented anywhere, so new contributors reach for hex codes instead. Even a one-page `docs/design-tokens.md` would help.
+
+---
+
+## Audit findings (May 2026 sweep)
+
+The following came out of a code-quality pass that explicitly didn't touch user-facing behavior. They're internal-hygiene items: dead code, redundant aliases, magic-number duplication, file size. Each one is mechanical when implemented.
+
+### S12 — `TemplatesTab.jsx` is dead code
+**File:** `src/components/sidebar/TemplatesTab.jsx`, `src/components/sidebar/index.js`
+
+The Templates tab was removed from the editor when URL-driven template loading shipped (`?template=X` deep links from the marketing site replaced the picker). The component file is still on disk and the barrel `src/components/sidebar/index.js` still re-exports it:
+
+```js
+export { TemplatesTab } from './TemplatesTab';
+```
+
+No remaining import statement references it. Bundlers will tree-shake it out of production builds, but it confuses code search and ESLint can't help — it's "exported", just unused. Delete the file and the barrel line. (Verifiable mechanically: `rg -F "TemplatesTab" src/` should return zero hits after removal.)
+
+### S13 — `isPhoto = isImage` is a redundant alias
+**File:** `src/components/canvas/ElementToolbar.jsx`
+
+```js
+const isImage = element.type === 'image';
+const isSticker = element.type === 'sticker';
+const isPhoto = isImage; // Type-based gating replaces the previous `!element.emoji` heuristic.
+```
+
+The S10 work split `'image'` and `'sticker'` into separate types, so `isPhoto` and `isImage` mean exactly the same thing now. Pick one (the codebase elsewhere prefers `isImage`) and remove the alias. The comment explaining the history can move to `CHANGES.md` if it's worth keeping.
+
+### S14 — Stale `Fix for #N` comment references
+**Files:** widespread (App.jsx, DesignCanvas.jsx, TextElement.jsx, ImageElement.jsx, ElementToolbar.jsx, MobileBottomSheet.jsx, etc.)
+
+Many comments refer to issue numbers that lived in the old BUGS.md cycle: `#11`, `#12`, `#15`, `#19`, `#20`, `#21`, `#24`, `#25`, `#27`, `#28-30`, `#31`, `#32`, `#33`, `#35`. Those entries have been removed from `BUGS.md`; the references no longer resolve to anything. The comments themselves still carry value — they explain WHY a piece of code looks the way it does — but the `(Fix for #N)` suffix is now noise.
+
+Two paths:
+- Rip the `(Fix for #N)` suffix from comments and let the explanation stand alone.
+- Cross-reference `CHANGES.md` instead — `(see CHANGES.md → recent-colors)` or similar.
+
+The first is faster; the second is more discoverable. Either is fine. Avoid leaving the references as-is — they're a small but real "what's this number?" cost on every code review.
+
+Also stale: the App.jsx keyboard-shortcut comment that says "Cmd/Ctrl+A 'select all' is intentionally NOT bound here … makes sense once multi-select lands (S3)". S3 has landed; the comment misrepresents current state.
+
+### S15 — Extract duplicated undo/redo selection-preservation logic
+**File:** `src/hooks/useDesignEditor.js`
+
+`undo` and `redo` contain identical "filter the current selection set against the restored elements' ids" blocks. Extracting to a small helper is a readability win and prevents the two paths from drifting:
+
+```js
+function preserveSelection(prev, nextElements) {
+  const liveIds = new Set(nextElements.map((e) => e.id));
+  const filtered = new Set();
+  for (const id of prev) if (liveIds.has(id)) filtered.add(id);
+  return filtered.size === prev.size ? prev : filtered;
+}
+```
+
+### S16 — Split `App.jsx` into focused hooks
+**File:** `src/App.jsx`
+
+The file is now 1100+ lines. Three logically separable concerns each have meaningful state + a useEffect cluster:
+
+- **Keyboard shortcuts** — the keydown effect with all the shortcut branches (undo, redo, duplicate, delete, esc, [/], arrow nudge). Could become `useEditorKeyboardShortcuts({ selectedId, selectedIds, ...callbacks })`.
+- **Persistence** — the restore-on-mount effect, the auto-save effect, and `isFirstSaveRef`. Could become `usePersistence({ elements, shirtColorId, ... })`.
+- **Toast surface** — `toast` state, `showToast`, `dismissToast`, with a separate `<ToastSurface toast={toast} onDismiss={dismissToast} />` component for the JSX. (The timer mechanics are already factored out via `useTimer`.)
+
+None of these is urgent, but the file's size means anyone adding a new effect has to scan past unrelated state. Splitting would also make it easier to write focused tests (S5) for each concern.
+
+### S17 — Remove `commitHistory` alias
+**File:** `src/hooks/useDesignEditor.js`
+
+```js
+const commitHistory = useCallback(() => flushPendingChanges(), [flushPendingChanges]);
+```
+
+It does literally nothing except provide a second name. Pick one (`commitHistory` reads better at call sites, `flushPendingChanges` reads better in the hook's internals — but having both is unnecessary). Lots of components call `onCommit={commitHistory}`, so changing the public name is the bigger sweep; either decision is fine.
+
+### S18 — `SlotPlaceholder` corner brackets repetition
+**File:** `src/components/canvas/SlotPlaceholder.jsx`
+
+Eight `<Line>` elements drawing the four corner brackets, each spelled out with explicit coordinates. A small `corners.map(({x, y, dx, dy}) => <Line ... />)` would cut ~16 lines of repetition without changing the rendered output. Minor readability win.
+
+### S19 — `useBackgroundRemoval.hasModel` is unused
+**File:** `src/hooks/useBackgroundRemoval.js`, `src/components/canvas/BackgroundRemovalButton.jsx`
+
+The hook returns `hasModel` (set to true after `loadModel` succeeds). The only consumer, `BackgroundRemovalButton`, destructures it but never reads it. Either:
+- Wire it: hide the button until `hasModel` is true, OR show a different label ("Remove Background (model loading...)") when false.
+- Drop it from the hook's return.
+
+The first is a feature; the second is dead-code removal.
+
+### S20 — `useTemplate.templateRef` is dead state
+**File:** `src/hooks/useTemplate.js`
+
+`templateRef.current` is assigned in `loadTemplate` and `clearTemplate`, but nothing reads it. Looks like a leftover from an earlier iteration where consumers needed an imperative handle. Remove the ref and the assignments.
+
+### S21 — `useTemplate.getSlots` is only used internally
+**File:** `src/hooks/useTemplate.js`
+
+`getSlots` is defined as a `useCallback` and exported in the return value, but the only call site is `assignImageToSlot` inside the same hook. No external consumer destructures `getSlots`. Inline `const slots = currentTemplate?.slots || [];` in `assignImageToSlot` and remove from the public API.
+
+### S22 — `MobileBottomSheet.sheetRef` is declared but never read
+**File:** `src/components/MobileBottomSheet.jsx`
+
+```js
+const sheetRef = useRef(null);
+// ...
+<div ref={sheetRef} className="mbs__sheet" ... >
+```
+
+The ref is attached to the sheet element but never used (no animation reads from it, no scroll-locking targets it specifically). Probably leftover from an earlier interaction model. Remove the ref or document why it's there.
+
+### S23 — Client-side `console.error` calls have no observable destination in production
+**Files:** `src/App.jsx` (`handleSlotImageUpload`), `src/hooks/useExport.js`, `src/hooks/useBackgroundRemoval.js`, `src/components/sidebar/UploadTab.jsx`, `src/components/PWAInstall.jsx`
+
+The server got structured logging in S22 (pino + pino-http). The client still uses `console.error` for failures, which is fine in development but invisible in production unless the user opens devtools. Two reasonable directions:
+
+- **Thin client logger** — a small `src/utils/logger.js` exporting `log.error(...)` etc, that today just calls `console.error` but can later post to an error-reporting service (Sentry, Bugsnag).
+- **Status quo with explicit acceptance** — document in CHANGES.md that "client errors are devtools-only by design; production users only see toast-level error feedback".
+
+Either is fine; mixing both (some files use `console.error`, others wrap) is the worst outcome.
+
+### S24 — Generalize `withTextCenterPreservation` to a shape-agnostic helper
+**Files:** `src/utils/textGeometry.js`, `src/App.jsx` (`handlePhotoEditComplete`)
+
+Both paths preserve the visual center of an element across a dimension change, but the math is duplicated:
+
+- `withTextCenterPreservation` derives height as `fontSize × LINE_HEIGHT_RATIO`, width via `measureTextWidth`, and returns a center-shifted x/y.
+- `handlePhotoEditComplete` does the same logic inline using raw width/height: `oldCenterX = (x ?? 0) + oldWidth / 2; newX = oldCenterX - newWidth / 2`.
+
+A shape-agnostic helper `preserveCenter(oldX, oldY, oldW, oldH, newW, newH) → { x, y }` could underlie both. The text path would compose: text-specific bbox computation → preserveCenter. The photo path calls preserveCenter directly. Worth doing alongside a test for both — the math looks easy but the rounding behavior on photos differs from text (photos round at the call site, text returns floats).
+
+### S25 — Generalize blob-URL revocation in element-replacement paths
+**File:** `src/App.jsx` (`handlePhotoEditComplete`)
+
+`handlePhotoEditComplete` revokes the previous blob URL after the new image loads. Other element-replacement paths in the future (drag-drop replacement on a slot, "swap photo" feature) would want the same cleanup. The existing `revokeBlobUrl` helper handles the URL primitive; what's missing is a higher-level "replace this element's src and revoke the old one" helper. Could live in `useDesignEditor` as `replaceElementSrc(id, newSrc)`. Not a refactor target until there's a second caller.