127 lines
11 KiB
Markdown
127 lines
11 KiB
Markdown
# Apparel Designer — Bug Report
|
||
|
||
This is the active bug tracker. New regressions or freshly discovered defects belong here; once a bug is fixed, it moves to `CHANGES.md` (which is the historical record) and is removed from this file.
|
||
|
||
---
|
||
|
||
## 🔴 Critical
|
||
|
||
_None outstanding._
|
||
|
||
## 🟠 Major
|
||
|
||
### B1 — `Cmd/Ctrl+D` on a multi-selection produces N undo entries
|
||
**File:** `src/App.jsx` (keyboard-shortcut effect, around the Cmd+D branch)
|
||
|
||
The handler iterates the selection set and calls `duplicateElement(id)` once per id. Each `duplicateElement` call commits its own history entry, so duplicating 3 selected elements produces 3 separate undo steps — the user has to press Cmd-Z three times to "undo the duplicate" they just did. The single-select path is correct (one duplicate → one undo); only multi-select is wrong.
|
||
|
||
Fix shape: add `duplicateMany(ids)` to `useDesignEditor` that mirrors `deleteMany`'s "single history entry, set selection to the new ids" pattern, and have the Cmd+D handler call that when `selectedIds.size > 1`.
|
||
|
||
## 🟡 Minor
|
||
|
||
### B2 — `[` / `]` z-order shortcuts are inert in multi-select mode
|
||
**File:** `src/App.jsx`
|
||
|
||
The handler is gated on `selectedId` (the single-select derived value, which is `null` whenever `selectedIds.size > 1`). With multiple elements selected, `[` and `]` do nothing. Arrow-key nudging has the same gap — it operates on `selectedId` only.
|
||
|
||
Reasonable interpretations:
|
||
- Iterate the selection (each element moves one step in z-order) — note that for adjacent selected layers this can produce confusing reorderings.
|
||
- Treat the multi-selection as a contiguous block and shift it as a unit.
|
||
|
||
Either is a real call; the current behavior (silent no-op) is the worst.
|
||
|
||
A related stale-comment issue: the same handler still carries a comment "Cmd/Ctrl+A 'select all' is intentionally NOT bound here … makes sense once multi-select lands (S3)". S3 has now landed, so the comment misrepresents current state.
|
||
|
||
### B3 — `ImageElement.jsx` filter `useEffect` over-invalidates on width/height changes
|
||
**File:** `src/components/canvas/ImageElement.jsx`
|
||
|
||
The effect's dep array is `[filter, src, width, height]`, but the body only references `filter` and `src`. width/height are listed because Konva's filter cache is dimension-sensitive — but Konva ALSO re-caches automatically when the node's size changes, so listing them in the deps causes a redundant `node.cache()` + `clearCache()` cycle on every resize. During a continuous transformer drag this fires once per intermediate `onUpdate`, which on filtered images becomes a noticeable hot path.
|
||
|
||
Fix shape: drop width/height from the deps and rely on Konva's built-in re-cache. Verify with a brief drag of a filtered image that the filter still updates correctly.
|
||
|
||
### B4 — `useBackgroundRemoval.removeBackground` early-return doesn't toggle `loading`
|
||
**File:** `src/hooks/useBackgroundRemoval.js`
|
||
|
||
When `loadModel()` fails (network error fetching the 86MB model), the early-return path returns `null` without calling `setLoading(false)`. `loadModel` itself does call `setLoading(false)` in its catch block, so today the user-visible state is consistent — but the contract between the two functions is fragile: if `loadModel` is ever changed to return false without resetting loading, this caller silently strands the spinner.
|
||
|
||
Fix shape: defensive `setLoading(false)` in the early-return path of `removeBackground`.
|
||
|
||
### B5 — `useBackgroundRemoval` progress is non-monotonic and confusing
|
||
**File:** `src/hooks/useBackgroundRemoval.js`
|
||
|
||
Model-download phase reports 0–50 (scaled from the HF progress callback). Then `removeBackground` jumps straight to `setProgress(50)`, then `70`, `90`, `100` at fixed pipeline checkpoints. The 50→70→90 jumps are visible to the user as a stuttering bar that pauses, leaps, pauses, leaps. It still finishes — just looks broken.
|
||
|
||
Fix shape: either (a) replace the progress bar with an indeterminate spinner once the model is loaded — the per-image inference is fast enough that fake-stepping doesn't help, or (b) genuinely time-budget the post-load steps and report linearly.
|
||
|
||
### B6 — `UploadTab.jsx` uses bare `alert()` for validation errors
|
||
**File:** `src/components/sidebar/UploadTab.jsx`
|
||
|
||
Three `alert()` calls — file-type rejection, file-size rejection, upload-failure — bypass the app-wide toast system that #19 introduced. These are the only `alert()` calls left in the codebase. The toast system has the right "info / error" kinds for these messages.
|
||
|
||
Fix shape: thread `showToast` into UploadTab (via Sidebar prop drilling, or via a context) and replace the `alert` calls. Note: this IS a user-visible change (alert vs toast looks different) — log it here rather than fix immediately, per the "no user-facing changes" instruction.
|
||
|
||
### B7 — `UploadTab.placeImage` hard-codes 300 for canvas centering
|
||
**File:** `src/components/sidebar/UploadTab.jsx`
|
||
|
||
The drop-zone path computes `(300 - width) / 2` and `(300 - height) / 2` to center an uploaded photo. 300 is the current `canvasSize`, but `getActiveProduct().canvasSize` is the source of truth. Drift risk: if the active product is ever swapped to one with a different canvas size, uploaded photos won't center correctly.
|
||
|
||
Fix shape: `import { getActiveProduct } from '../../constants/products'` and read `canvasSize` once at module top (or per-call).
|
||
|
||
### B8 — `OfflineIndicator` initializes state without an SSR guard
|
||
**File:** `src/components/OfflineIndicator.jsx`
|
||
|
||
`useState(!navigator.onLine)` runs at module-execution time inside the function component. There's no guard for a missing `navigator`. The app isn't SSR'd today, but this is the pattern that breaks first if anyone tries — and it's gratuitous; the rest of the codebase consistently guards (`typeof window === 'undefined'`).
|
||
|
||
Fix shape: `useState(() => typeof navigator !== 'undefined' ? !navigator.onLine : false)`.
|
||
|
||
### B9 — `SlotPlaceholder` mouseLeave forces `cursor: 'default'`
|
||
**File:** `src/components/canvas/SlotPlaceholder.jsx`
|
||
|
||
`handleMouseLeave` sets `stage.container().style.cursor = 'default'`. This stomps on whatever cursor was inherited (e.g. another shape that sets `'pointer'` via its own enter handler may not get to restore on the next move event because we just hard-set 'default'). The standard idiom is `'auto'` or `''` to clear the inline style and fall back to the inherited cascade.
|
||
|
||
In practice the bug is hard to trigger today (the only other shape-level cursor handler IS this same one), so it's classified Minor — but it's a latent foot-gun.
|
||
|
||
### B10 — `persistence.js` `stripUnpersistable` filter narrowness
|
||
**File:** `src/utils/persistence.js`
|
||
|
||
The filter only inspects `type === 'image'` elements. Today stickers (`type: 'sticker'`) have data URLs, not blob URLs, so this works. But the contract isn't symmetric with reality — if any future code path produces a sticker with a `blob:` src (e.g. a sticker authored from a blob URL on first paste), it won't be filtered, and reload will produce a broken sticker.
|
||
|
||
Fix shape: filter on src URL prefix, not type — `if (typeof el.src === 'string' && el.src.startsWith('blob:')) drop`. Same intent, narrower-than-needed gate today.
|
||
|
||
### B11 — `useExport` progress simulator is misleading
|
||
**File:** `src/hooks/useExport.js`
|
||
|
||
`progressInterval` advances `progress` 0→90 in 10% steps every 200ms while the export is in flight, regardless of actual upload/render progress. On a fast export, the user sees 30% before the request even arrives at the server. On a slow export, the bar pegs at 90% and waits silently for whatever's actually slow.
|
||
|
||
Two real fixes:
|
||
1. Wire to fetch's upload-progress events (XHR has them; fetch+ReadableStream support is patchy but adequate for our use case). Server-render time is opaque from the client, so progress will still cap at "uploaded" and then sit indeterminate.
|
||
2. Replace the bar with an indeterminate spinner — honest about the fact that we don't know how far along the server is.
|
||
|
||
### B12 — `handleShare` has hardcoded English strings not in the i18n catalog
|
||
**File:** `src/App.jsx` (`handleShare`)
|
||
|
||
`'My Pawfectly Yours design'` (share title) and `'Check out the shirt I made!'` (share text) are passed directly to `navigator.share`. The S23 catalog covers toast messages and button labels but missed these because they don't render in the DOM — they go straight to the OS share sheet. Should be added to the catalog and pulled via `t()`.
|
||
|
||
Same applies to several inline strings in App.jsx render — FAB aria-labels (`'Close customization options'` / `'Open customization options'`), export-toast verbiage (`'Saving design…'`, `'⚠️ Save failed:'`, `'✅ Saved!'`, `'Download'`, `'Dismiss'`). All logged collectively under the S10 i18n-sweep follow-on, but B12 specifically calls out the share strings since they're easy to miss in a DOM-text grep.
|
||
|
||
---
|
||
|
||
## Closed since the May 2026 audit
|
||
|
||
The original May 2026 audit identified twelve critical (C1–C12), seven major (M1–M7), and nine minor (m1–m9) bugs. **All of them have been fixed.** See `CHANGES.md` for the per-bug summary, the files touched, and the verification steps. The performance contract for the drag/rotate hot path (M4) is the most operationally important section — it documents the constraints any future change to `DesignCanvas.jsx`'s bound functions must respect.
|
||
|
||
Subsequent fix rounds (the polish/UX work tracked outside this file under issue numbers like #1–#35) are also captured in `CHANGES.md`. Items addressed there but originally listed in `SUGGESTIONS.md` have been removed from that file too.
|
||
|
||
---
|
||
|
||
## Notes on items deliberately not flagged as bugs
|
||
|
||
These are observations from the audit that were *not* bugs at the time and are kept here as ongoing watch-items so future contributors don't re-audit ground we've already considered.
|
||
|
||
- **Express 4 wildcard routes** (`app.all('/api/*')`, `app.get('*')`) are correct for the pinned `^4.18.2`. These would break under Express 5's path-to-regexp v6 and are worth revisiting if/when the project upgrades, but they aren't bugs today.
|
||
- **`canvas` package version `^2.11.2`** is older than the current 3.x line. 2.x is still maintained and works; mentioning here only for transparency.
|
||
- **CORS in dev allows all origins** — intentional, gated on `IS_PRODUCTION`. Not a bug.
|
||
- **`saveToHistory` JSON-stringify dedupe** — uses string equality on `JSON.stringify(elements)`, which is sensitive to numeric formatting (`0` vs `0.0`, key ordering between `Object.assign` results, etc.). In practice React's setState updaters produce stable object shapes so this is fine, but if a future code path constructs elements differently, identical-content snapshots could end up as separate history entries. Worth keeping aware of, not worth fixing speculatively.
|
||
- **`useBackgroundRemoval.hasModel`** — returned from the hook but ignored by every consumer (only `BackgroundRemovalButton` calls the hook). Not a user-visible bug; logged as S21 in suggestions for cleanup.
|
||
- **`useTemplate.templateRef`** — set in `loadTemplate` and `clearTemplate` but never read anywhere. Dead state; logged as S22 in suggestions.
|