194 lines
15 KiB
Markdown
194 lines
15 KiB
Markdown
# 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.
|