7.8 KiB
phase, reviewed, depth, files_reviewed, files_reviewed_list, findings, status
| phase | reviewed | depth | files_reviewed | files_reviewed_list | findings | status | |||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| 35-bug-fixes | 2026-04-19T00:00:00Z | standard | 9 |
|
|
issues_found |
Phase 35: Code Review Report
Reviewed: 2026-04-19T00:00:00Z Depth: standard Files Reviewed: 9 Status: issues_found
Summary
Reviewed 9 client-side source files spanning components, a hook, a route, and a page. The code is generally clean and follows project patterns. No security vulnerabilities or data-loss risks were found.
Four warnings were identified: a redirect loop in the login route, an icon button that never auto-closes the FAB menu before navigation, a missing onError handler on lazy-loaded images (leaves the skeleton shimmer visible forever if the image 404s), and an off-by-one risk in the ItemCard brand-stripping logic. Four informational items cover dead/unreachable code, a stub action, and minor code-smell patterns.
Warnings
WR-01: Redirect loop in login route
File: src/client/routes/login.tsx:10
Issue: LoginPage immediately sets window.location.href = "/login", which reloads the same route repeatedly. A browser hitting /login will spin forever. The intent appears to be a redirect to the server-side OIDC handler at /api/auth/login (or similar), but the current target is the same client-side route.
Fix:
useEffect(() => {
window.location.href = "/api/auth/login"; // point at the Hono OIDC handler
}, []);
WR-02: FAB "Start new thread" menu item does not close the menu before navigation
File: src/client/components/FabMenu.tsx:36-38
Issue: The openCatalogSearch("thread") action is fired without calling closeFabMenu() first. The backdrop and menu items remain visible behind the catalog search overlay. The "Add to collection" item has the same gap. Compare with the newSetup item at line 47 which explicitly calls closeFabMenu() first.
Fix:
{
label: t("fab.addToCollection"),
icon: <Package className="w-5 h-5 text-gray-600" />,
onClick: () => {
closeFabMenu();
openCatalogSearch("collection");
},
},
{
label: t("fab.startNewThread"),
icon: <Search className="w-5 h-5 text-gray-600" />,
onClick: () => {
closeFabMenu();
openCatalogSearch("thread");
},
},
WR-03: No onError handler on lazy images — skeleton shimmer persists on broken images
File: src/client/components/CandidateCard.tsx:178-187, src/client/components/ItemCard.tsx:203-212, src/client/components/GlobalItemCard.tsx:55-64
Issue: All three cards use a loaded state toggled only by onLoad. If the image URL returns a 4xx/5xx, onLoad never fires, the animate-pulse placeholder skeleton stays permanently visible, and the actual broken-image icon shows through at opacity-0. The same GearImage wrapper is used in all three locations.
Fix: Add an onError prop to GearImage and pass it through to the <img> element. In the consuming cards, set loaded to true (or a separate errored flag) on error so the placeholder is dismissed:
// GearImage.tsx — add to props and each <img>
onError?: () => void;
// ...
<img ... onError={onError} />
// CandidateCard / ItemCard / GlobalItemCard
<GearImage
...
onLoad={() => setLoaded(true)}
onError={() => setLoaded(true)} // dismiss the skeleton on failure
/>
WR-04: Brand-stripping in ItemCard can silently truncate name when brand appears mid-string
File: src/client/components/ItemCard.tsx:232
Issue: name.replace(${brand} , "") uses String.prototype.replace, which replaces only the first occurrence and will also match a brand name that appears in the middle of the product name (e.g. brand "Lezyne", name "Lezyne Lezyne Pro" → "Lezyne Pro" instead of "Lezyne Pro"). More critically, if the item name does not actually start with the brand prefix (server data inconsistency), the displayed name silently drops the brand substring wherever it first appears.
Fix: Strip only a leading occurrence so accidental mid-string matches are avoided, and guard against the name not starting with the brand:
const displayName =
brand && name.startsWith(`${brand} `)
? name.slice(brand.length + 1)
: name;
// then use displayName instead of the inline replace:
<h3 ...>{displayName}</h3>
Info
IN-01: imageFilename prop is accepted but immediately discarded (dead parameter)
File: src/client/components/CandidateCard.tsx:44, src/client/components/ItemCard.tsx:42
Issue: Both components declare imageFilename in their props interface and rename it _imageFilename on destructuring, signalling it is intentionally unused. The prop is still part of the public API, which means callers must provide it unnecessarily and TypeScript will warn if it is ever missing. If the component has permanently switched to imageUrl, the prop should be removed from the interface (and call sites updated), or at minimum documented with a TODO explaining why it is kept.
Fix: Remove imageFilename from both prop interfaces and their call sites, or add a comment explaining why it is retained for forward-compat.
IN-02: Stub "new setup" FAB menu item does nothing
File: src/client/components/FabMenu.tsx:42-51
Issue: The isSetupsPage branch adds a "New Setup" menu item whose onClick handler only closes the menu. There is an inline comment "Stub: setup creation is handled by the setups page itself", but the setups page appears to have its own creation mechanism. The dead menu item is shown to users and can cause confusion.
Fix: Either wire the handler to the real setup-creation action (e.g. openCreateSetupModal()), or remove the conditional item entirely if the setups page already surfaces a dedicated button.
IN-03: useExportItems is a factory function wrapping a direct window.location assignment, bypassing React Query
File: src/client/hooks/useItems.ts:117-121
Issue: useExportItems returns a plain function rather than following the useMutation pattern used by all other mutation hooks in the file. This is a minor inconsistency — it is not harmful, but the pattern makes it easy for future callers to forget to invoke the returned function. A small naming clarification (e.g. returning it directly as a named function, or wrapping in a useCallback) would improve consistency.
Fix (optional): Rename to make the double-call explicit, or return a useMutation wrapping the navigation if loading-state feedback is ever needed.
IN-04: thread.candidates used directly in grid view while displayItems (with drag-reorder) is used in list view
File: src/client/routes/threads/$threadId/index.tsx:278
Issue: The grid view at line 278 maps over thread.candidates directly instead of displayItems. This means drag-reorder state (tempItems) is not reflected in the grid view — if the user switches from list-with-drag to grid, the order resets to server order visually. This is likely intentional (drag only makes sense in list view), but it creates a subtle inconsistency: reordering in list mode and switching to grid shows stale order until the server round-trip completes.
Fix: Use displayItems in the grid view as well:
{displayItems.map((candidate, index) => (
<CandidateCard key={candidate.id} ... />
))}
Reviewed: 2026-04-19T00:00:00Z Reviewer: Claude (gsd-code-reviewer) Depth: standard