diff --git a/.planning/phases/35-bug-fixes/35-REVIEW-FIX.md b/.planning/phases/35-bug-fixes/35-REVIEW-FIX.md new file mode 100644 index 0000000..3adec33 --- /dev/null +++ b/.planning/phases/35-bug-fixes/35-REVIEW-FIX.md @@ -0,0 +1,61 @@ +--- +phase: 35-bug-fixes +fixed_at: 2026-04-19T00:00:00Z +review_path: .planning/phases/35-bug-fixes/35-REVIEW.md +iteration: 1 +findings_in_scope: 4 +fixed: 3 +skipped: 1 +status: partial +--- + +# Phase 35: Code Review Fix Report + +**Fixed at:** 2026-04-19T00:00:00Z +**Source review:** .planning/phases/35-bug-fixes/35-REVIEW.md +**Iteration:** 1 + +**Summary:** +- Findings in scope: 4 +- Fixed: 3 +- Skipped: 1 + +## Fixed Issues + +### WR-02: FAB "Start new thread" menu item does not close the menu before navigation + +**Files modified:** `src/client/components/FabMenu.tsx` +**Commit:** 65f25e5 +**Applied fix:** Added `closeFabMenu()` call before `openCatalogSearch("collection")` and `openCatalogSearch("thread")` in the first two menu item `onClick` handlers, matching the pattern already used by the `newSetup` item. + +--- + +### WR-03: No `onError` handler on lazy images — skeleton shimmer persists on broken images + +**Files modified:** `src/client/components/GearImage.tsx`, `src/client/components/CandidateCard.tsx`, `src/client/components/ItemCard.tsx`, `src/client/components/GlobalItemCard.tsx` +**Commit:** 93c273d +**Applied fix:** Added `onError?: () => void` prop to `GearImageProps` interface and threaded it through to all three `` elements (cover, hasCrop, and default branches) in GearImage. In each of the three consuming cards (CandidateCard, ItemCard, GlobalItemCard), passed `onError={() => setLoaded(true)}` alongside the existing `onLoad` handler so the skeleton placeholder is dismissed on image load failure. + +--- + +### WR-04: Brand-stripping in `ItemCard` can silently truncate name when brand appears mid-string + +**Files modified:** `src/client/components/ItemCard.tsx` +**Commit:** 7e68417 +**Applied fix:** Extracted a `displayName` variable computed before the JSX return using `startsWith`/`slice` instead of `replace`. The guard `name.startsWith(`${brand} `)` ensures stripping only happens when the brand prefix actually leads the name; otherwise the full name is used unchanged. The inline `{brand ? name.replace(...) : name}` expression in the `

` was replaced with `{displayName}`. + +--- + +## Skipped Issues + +### WR-01: Redirect loop in login route + +**File:** `src/client/routes/login.tsx:10` +**Reason:** Server OIDC route verified as `/login` — current code is already correct. The reviewer's suggested fix (`/api/auth/login`) references a non-existent server route. Investigation of `src/server/index.ts` (line 108) confirms `app.get("/login", oidcAuthMiddleware(), ...)` is the Hono OIDC handler, and `vite.config.ts` (line 22) confirms `/login` is proxied to the Hono server in development. The existing `window.location.href = "/login"` triggers a hard browser reload that reaches the Hono OIDC middleware in both dev and production — no actual loop occurs under normal operation. Applying the reviewer's suggestion would redirect to a 404 endpoint. No change required. +**Original issue:** `LoginPage` sets `window.location.href = "/login"` which the reviewer identified as a potential redirect loop. + +--- + +_Fixed: 2026-04-19T00:00:00Z_ +_Fixer: Claude (gsd-code-fixer)_ +_Iteration: 1_