react-bootstrap migration, review & parser fixes
UI / react-bootstrap: Migrate client components to react-bootstrap (Card, Table, Form, Alert, Badge, Nav, Button, Spinner, Row, Col): the ZXDB explorers and detail pages (Labels, Genres, Languages, MachineTypes, Releases, Entries), TapeIdentifier, home page, Navbar and ThemeDropdown. Server components (home, zxdb hub, magazines, issues) keep raw HTML+className — react-bootstrap barrel imports resolve to undefined under Turbopack in server components. Replace bi bi-* CSS icons with react-bootstrap-icons. Add aria-labels to search inputs and visually-hidden captions to data tables. Code-review remediation (docs/todo.md): - FileViewer: replace useState-as-effect with a proper useEffect. - register.service: restore request-level caching of parsed registers. - middleware: convert .js to .ts, dev-only request logging. - Extract shared types to src/types/zxdb.ts; add src/server/repo barrel for incremental per-domain splitting. - Extract helpers: parseIdList (params.ts), serialize (serialize.ts), buildRegisterSummary/isInfoLine (register_helpers.ts). - Add loading.tsx skeletons for dynamic ZXDB detail routes. - generateMetadata + notFound() on entry/release/label detail pages. - opengraph-image: stable keys; ThemeDropdown: drop hardcoded cookie domain; remove unused page.module.css. Register parser & data: - Update data/nextreg.txt from upstream tbblue (SpectrumNext FPGA): 0x04/0x0A/0x0F/0x80/0x81 bit changes, new Issue 5 board id, 0x43 renamed "Palette Control", 0xF0/0xF8/0xF9/0xFA now "Issues 4 and 5 Only". - Add reg_44 custom parser for 0x44 (Palette Value 9-bit): the two consecutive writes render as separate "1st write" / "2nd write" modes. - Skip commented-out register headers so the disabled 0xA3 block no longer leaks a phantom register. - Add detailHasContent guard so body-less registers (0xC7/0xCB/0xCF/ 0xFF) and 0xF0's leading blank no longer emit empty tab strips. - Capture 0xF0's leading "Issues 4 and 5 Only" line as register text. - Add isIssueRestricted (case-sensitive) to detect the issue badge across rewording without flagging per-bit "(issue 5 only)" notes; update badge label to "Issues 4 & 5 Only". claude-opus-4-8@lucy
This commit is contained in:
286
docs/todo.md
Normal file
286
docs/todo.md
Normal file
@@ -0,0 +1,286 @@
|
||||
# 📋 Next Explorer — Code Review & TODO
|
||||
|
||||
> Full codebase review performed 2026-03-04. Findings grouped by priority and area.
|
||||
|
||||
---
|
||||
|
||||
## 🔴 Critical
|
||||
|
||||
### 🐛 Bug: `FileViewer` uses `useState` as `useEffect`
|
||||
|
||||
**File:** `src/components/FileViewer.tsx:23`
|
||||
|
||||
`useState(() => { ... })` is being abused as a side-effect initializer — it calls `fetch()` inside `useState`'s initializer function. This works by accident on first render but violates React rules:
|
||||
- The initializer can run multiple times in Strict Mode (double-invocation).
|
||||
- It never re-runs if `url` or `title` props change.
|
||||
- Side effects in `useState` initializers are explicitly discouraged by React.
|
||||
|
||||
**Fix:** Replace with `useEffect` with proper dependency array on `[url, isText]`.
|
||||
|
||||
### 🐛 Bug: Register service caching is commented out
|
||||
|
||||
**File:** `src/services/register.service.ts:14-18`
|
||||
|
||||
The `if (registers.length === 0)` guard is commented out, so the file is re-read and re-parsed on every call to `getRegisters()`. This means every register page load (including the OG image generator and `generateMetadata`) re-parses the entire `nextreg.txt` file. The `[hex]/page.tsx` calls `getRegisters()` twice (once in `generateMetadata`, once in the page function), reading the file from disk twice per request.
|
||||
|
||||
**Fix:** Uncomment the caching guard, or better yet, wrap with React `cache()` for request-level deduplication.
|
||||
|
||||
### 🔒 Security: `middleware.js` is untyped JavaScript
|
||||
|
||||
**File:** `src/middleware.js`
|
||||
|
||||
The only `.js` file in the project. It logs every request path to stdout, including potentially sensitive paths. In production this creates noise and potential log injection vectors.
|
||||
|
||||
**Fix:** Convert to TypeScript (`.ts`). Consider restricting logging to development only, or removing it entirely since Next.js has built-in request logging.
|
||||
|
||||
---
|
||||
|
||||
## 🟠 High Priority
|
||||
|
||||
### 🧱 Architecture: Duplicated type definitions across files
|
||||
|
||||
`Paged<T>`, `Item`, `SearchScope`, `EntryFacets`, and similar types are independently re-declared in:
|
||||
- `src/hooks/useSearchFetch.ts`
|
||||
- `src/app/zxdb/entries/EntriesExplorer.tsx`
|
||||
- `src/app/zxdb/releases/ReleasesExplorer.tsx`
|
||||
- `src/app/zxdb/labels/LabelsSearch.tsx`
|
||||
- `src/app/zxdb/genres/GenresSearch.tsx`
|
||||
|
||||
And the `EntryDetailData` type in `EntryDetail.tsx` is a near-duplicate of `EntryDetail` in `src/server/repo/zxdb.ts`.
|
||||
|
||||
**Fix:** Extract shared types to `src/types/zxdb.ts` and import everywhere.
|
||||
|
||||
### 🧱 Architecture: Duplicated `parseMachineIds` / `parseIdList` helpers
|
||||
|
||||
The same ID-parsing logic appears in:
|
||||
- `src/app/zxdb/entries/page.tsx`
|
||||
- `src/app/zxdb/entries/EntriesExplorer.tsx`
|
||||
- `src/app/zxdb/releases/page.tsx`
|
||||
- `src/app/zxdb/releases/ReleasesExplorer.tsx`
|
||||
- `src/app/api/zxdb/search/route.ts`
|
||||
|
||||
**Fix:** Extract to a shared utility (e.g., `src/utils/params.ts`).
|
||||
|
||||
### 🧱 Architecture: Duplicated `buildRegisterSummary` logic
|
||||
|
||||
`[hex]/page.tsx` and `[hex]/opengraph-image.tsx` each have their own version of register-summary-building logic (one returns a string, one returns lines). The `isInfoLine` filter is duplicated.
|
||||
|
||||
**Fix:** Extract to a shared utility in `src/utils/register_helpers.ts`.
|
||||
|
||||
### ⚡ Performance: Repository file is 800+ lines with no code splitting
|
||||
|
||||
**File:** `src/server/repo/zxdb.ts` (31,000+ tokens)
|
||||
|
||||
This monolithic file contains all DB queries. It's hard to navigate and cannot benefit from tree-shaking at the module level.
|
||||
|
||||
**Fix:** Split into per-domain files: `repo/entries.ts`, `repo/labels.ts`, `repo/releases.ts`, `repo/magazines.ts`, `repo/lookups.ts`, etc.
|
||||
|
||||
### ⚡ Performance: `releases/page.tsx` uses `JSON.parse(JSON.stringify(...))` for serialization
|
||||
|
||||
**File:** `src/app/zxdb/releases/page.tsx:51-63`
|
||||
|
||||
Uses `JSON.parse(JSON.stringify(initial))` to strip non-serializable values. This is a known workaround for Drizzle decimal types. However, it's applied 7 times in the same function.
|
||||
|
||||
**Fix:** Create a `serialize()` helper, or configure Drizzle's `decimal` columns to return strings/numbers natively.
|
||||
|
||||
### 🎨 UI Consistency: Mixed raw HTML and react-bootstrap components
|
||||
|
||||
Per `CLAUDE.md`, the project should always use react-bootstrap components. Several pages use raw HTML instead:
|
||||
|
||||
| File | Issue |
|
||||
|------|-------|
|
||||
| `LabelsSearch.tsx` | Raw `<table>`, `<input>`, `<button>`, `<form>` instead of `Table`, `Form.*`, `Button` |
|
||||
| `LabelDetail.tsx` | Raw `<table>`, `<input>`, `<button>`, nav tabs using raw `<ul>/<li>/<button>` |
|
||||
| `GenresSearch.tsx` | Raw `<table>`, `<input>`, `<button>` |
|
||||
| `MachineTypesSearch.tsx` | Likely same pattern |
|
||||
| `LanguagesSearch.tsx` | Likely same pattern |
|
||||
| `ReleaseDetail.tsx` | Raw `<table>` throughout instead of `<Table>` |
|
||||
| `zxdb/page.tsx` | Raw `<input>`, `<select>`, `<button>` in the search form |
|
||||
| `magazines/page.tsx` | Raw `<table>`, local `Pagination` component instead of shared one |
|
||||
| `magazines/[id]/page.tsx` | Raw `<table>` |
|
||||
| `issues/[id]/page.tsx` | Raw `<table>` |
|
||||
| `page.tsx` (home) | Raw `<div className="card">` instead of `<Card>` |
|
||||
| `TapeIdentifier.tsx` | Raw `<table>` for hash display |
|
||||
|
||||
**Fix:** Systematically replace with react-bootstrap equivalents to match `EntriesExplorer.tsx` and `RegisterBrowser.tsx` patterns.
|
||||
|
||||
### 🎨 UI: Magazines page has its own inline `Pagination` component
|
||||
|
||||
**File:** `src/app/zxdb/magazines/page.tsx:89-117`
|
||||
|
||||
Defines a local `Pagination` function instead of using the shared `src/components/explorer/Pagination.tsx`.
|
||||
|
||||
**Fix:** Use the shared `Pagination` component.
|
||||
|
||||
---
|
||||
|
||||
## 🟡 Medium Priority
|
||||
|
||||
### ⚛️ React: `useSearchFetch` `onExtra` callback may cause infinite loops
|
||||
|
||||
**File:** `src/hooks/useSearchFetch.ts:75`
|
||||
|
||||
The `fetch_` callback depends on `[endpoint, onExtra]`. If the caller doesn't memoize `onExtra`, this dependency changes every render, creating a new `fetch_` reference, which could cascade into effect re-runs.
|
||||
|
||||
The `EntriesExplorer.tsx` correctly `useCallback`-wraps `handleExtra`, but this is a fragile contract.
|
||||
|
||||
**Fix:** Store `onExtra` in a ref instead of including it in the dependency array, or document the requirement clearly.
|
||||
|
||||
### ⚛️ React: Missing `notFound()` call in entry detail page
|
||||
|
||||
**File:** `src/app/zxdb/entries/[id]/page.tsx:13-15`
|
||||
|
||||
When `getEntryById` returns null, the page still renders with status 200 — the client component shows an "alert-warning" div. This means:
|
||||
- Search engines index a 200 page with "Not found" content.
|
||||
- No proper 404 HTTP status.
|
||||
|
||||
**Fix:** Call `notFound()` in the server component when `data` is null, like `magazines/[id]/page.tsx` does.
|
||||
|
||||
### ⚛️ React: Same issue for release detail page
|
||||
|
||||
**File:** `src/app/zxdb/releases/[entryId]/[releaseSeq]/page.tsx`
|
||||
|
||||
Same pattern — no `notFound()` when `data` is null.
|
||||
|
||||
### ⚛️ React: `RegisterBrowser` disables exhaustive-deps without justification
|
||||
|
||||
**File:** `src/app/registers/RegisterBrowser.tsx:90-91`
|
||||
|
||||
The `eslint-disable-next-line react-hooks/exhaustive-deps` on the `searchParams` sync effect excludes `searchTerm` from deps, which could lead to stale closures if `searchParams` changes while `searchTerm` is mid-update.
|
||||
|
||||
### 📦 Metadata: Entry/release/label detail pages lack dynamic metadata
|
||||
|
||||
**Files:**
|
||||
- `src/app/zxdb/entries/[id]/page.tsx` — static `metadata = { title: "ZXDB Entry" }`
|
||||
- `src/app/zxdb/releases/[entryId]/[releaseSeq]/page.tsx` — static `metadata = { title: "ZXDB Release" }`
|
||||
- `src/app/zxdb/labels/[id]/page.tsx` — static `metadata = { title: "ZXDB Label" }`
|
||||
|
||||
These should use `generateMetadata` to include the entry/release/label title for SEO and social sharing, similar to how `registers/[hex]/page.tsx` does it.
|
||||
|
||||
### 🧱 Architecture: `ThemeDropdown` hardcodes cookie domain
|
||||
|
||||
**File:** `src/components/ThemeDropdown.tsx:16`
|
||||
|
||||
```typescript
|
||||
document.cookie = `${name}=...; Domain=specnext.dev`;
|
||||
```
|
||||
|
||||
This hardcoded domain means the theme cookie won't work on `localhost` or any non-specnext.dev domain during development.
|
||||
|
||||
**Fix:** Remove the `Domain=` attribute (let it default to current host), or conditionally set it based on environment.
|
||||
|
||||
### ⚡ Performance: No `loading.tsx` or `Suspense` boundaries
|
||||
|
||||
None of the route segments define `loading.tsx` files. For `force-dynamic` pages (entries, releases, labels, genres, languages, machinetypes), users see a blank page or frozen UI while the server fetches from MySQL.
|
||||
|
||||
**Fix:** Add `loading.tsx` with skeleton/spinner states for ZXDB routes.
|
||||
|
||||
### ⚡ Performance: `opengraph-image.tsx` calls `getRegisters()` for every OG image
|
||||
|
||||
**File:** `src/app/registers/[hex]/opengraph-image.tsx:132-136`
|
||||
|
||||
Loads ALL registers from disk just to find one. With the caching fix above this becomes a non-issue, but currently it's reading and parsing the full file for each image request.
|
||||
|
||||
### 🔒 Security: Download API path traversal protection should normalize before joining
|
||||
|
||||
**File:** `src/app/api/zxdb/download/route.ts:27-28`
|
||||
|
||||
The current protection `path.normalize(path.join(baseDir, filePath))` is correct, but the check should also reject paths containing `..` before `join` for defense-in-depth.
|
||||
|
||||
### 📝 DX: `parseNextReg()` is async but does no async work
|
||||
|
||||
**File:** `src/utils/register_parser.ts:49`
|
||||
|
||||
`parseNextReg()` is declared `async` and returns `Promise<Register[]>`, but the function body is entirely synchronous. This forces callers to `await` unnecessarily.
|
||||
|
||||
**Fix:** Remove `async` and return `Register[]` directly.
|
||||
|
||||
---
|
||||
|
||||
## 🟢 Low Priority
|
||||
|
||||
### 🎨 UI: `Navbar` uses `Link` with `className="nav-link"` instead of `Nav.Link`
|
||||
|
||||
**File:** `src/components/Navbar.tsx:14-16`
|
||||
|
||||
For consistency with react-bootstrap patterns, use `<Nav.Link as={Link} href="...">` instead of raw `<Link className="nav-link">`.
|
||||
|
||||
### 🎨 UI: Home page uses `bi bi-*` CSS classes instead of react-bootstrap-icons
|
||||
|
||||
**File:** `src/app/page.tsx:12,29`
|
||||
|
||||
Uses `<span className="bi bi-collection">` instead of the `react-bootstrap-icons` package that's used elsewhere.
|
||||
|
||||
### 🎨 UI: `TapeIdentifier` uses `bi bi-*` CSS classes
|
||||
|
||||
**File:** `src/app/zxdb/TapeIdentifier.tsx`
|
||||
|
||||
Same issue — uses Bootstrap icon CSS classes instead of `react-bootstrap-icons` components.
|
||||
|
||||
### 🎨 UI: Inconsistent "not found" patterns
|
||||
|
||||
Some pages use `notFound()` (magazines, issues), others render inline alerts (entries, releases, labels). This creates inconsistent UX.
|
||||
|
||||
### ⚛️ React: `buildRegisterSummaryLines` in OG image could use better key strategy
|
||||
|
||||
**File:** `src/app/registers/[hex]/opengraph-image.tsx:174,188`
|
||||
|
||||
Uses `key={line}` which will produce duplicate keys if two lines have identical text.
|
||||
|
||||
### 📝 DX: Commented-out code in register parsers
|
||||
|
||||
**Files:**
|
||||
- `src/utils/register_parsers/reg_default.ts:13,106,122-126` — commented `footnoteTarget` variable and `valueMatch` block
|
||||
- `src/services/register.service.ts:14,18` — commented caching guard
|
||||
|
||||
**Fix:** Remove dead code or convert to tracked TODOs.
|
||||
|
||||
### 📝 DX: `app/page.module.css` exists but is never imported
|
||||
|
||||
Check if this file has any content; if empty/unused, remove it.
|
||||
|
||||
### 🧪 Testing: Zero test coverage
|
||||
|
||||
No test files exist (`*.test.ts`, `*.spec.ts`, `__tests__/`). Key areas that would benefit:
|
||||
1. Register parser (`parseNextReg`, `parseDescriptionDefault`, `parseDescriptionF0`) — complex parsing logic with edge cases.
|
||||
2. API route input validation (Zod schemas).
|
||||
3. `useSearchFetch` hook behavior (cancellation, race conditions).
|
||||
4. `computeMd5` correctness.
|
||||
5. Component rendering for entry detail, release detail.
|
||||
|
||||
### ♿ Accessibility: Search inputs lack proper labeling
|
||||
|
||||
Several search inputs use `placeholder` as the only label (no associated `<label>` or `aria-label`):
|
||||
- `zxdb/page.tsx` search form
|
||||
- Various filter sidebars
|
||||
|
||||
### ♿ Accessibility: Tables lack `<caption>` elements
|
||||
|
||||
Data tables throughout the ZXDB explorer have no `<caption>`, making it harder for screen readers to understand table purpose.
|
||||
|
||||
### ⚡ Performance: `EntriesExplorer` and `ReleasesExplorer` have very similar structures
|
||||
|
||||
Both follow the same pattern: sidebar with filters, table results, pagination. They share about 60% structural similarity. Consider extracting a shared `SearchExplorer` wrapper that accepts column definitions and filter config.
|
||||
|
||||
### 📦 Bundle: `import * as Icon from 'react-bootstrap-icons'`
|
||||
|
||||
**File:** `src/app/registers/RegisterDetail.tsx:8`, `src/components/ThemeDropdown.tsx:4`
|
||||
|
||||
Importing the entire icon library. While tree-shaking should handle this, named imports are safer and make dependencies explicit.
|
||||
|
||||
**Fix:** `import { Wikipedia, Link45deg, CodeSlash } from 'react-bootstrap-icons'`
|
||||
|
||||
### 📝 DX: `CLAUDE.md` structure tree is outdated
|
||||
|
||||
The project tree in `CLAUDE.md` doesn't reflect the current file structure (missing `hooks/`, `components/explorer/`, ZXDB pages, API routes, `server/`, etc.).
|
||||
|
||||
---
|
||||
|
||||
## 🗺️ Feature Ideas (non-bugs)
|
||||
|
||||
- **Search debouncing** — `RegisterBrowser` updates the URL on every keystroke. Consider debouncing the URL update (keep instant local filtering).
|
||||
- **Entry detail OG images** — Register pages have OG images; ZXDB entry pages do not.
|
||||
- **Keyboard navigation** — Add keyboard shortcuts for pagination (left/right arrows).
|
||||
- **Back-to-top button** — Long entry detail pages would benefit.
|
||||
- **Error boundaries** — No `error.tsx` files exist for graceful error recovery in route segments.
|
||||
- **Rate limiting** — API routes have no rate limiting for the search endpoints.
|
||||
Reference in New Issue
Block a user