662 lines
17 KiB
Markdown
662 lines
17 KiB
Markdown
# Codebase Improvements Implementation Plan
|
|
|
|
> **For agentic workers:** REQUIRED SUB-SKILL: Use superpowers:subagent-driven-development (recommended) or superpowers:executing-plans to implement this plan task-by-task. Steps use checkbox (`- [ ]`) syntax for tracking.
|
|
|
|
**Goal:** Harden the server (explicit DB context, param validation, error handling, rate limiting), add client error boundaries, split the oversized collection route into focused components, and fix stale docs.
|
|
|
|
**Architecture:** Server changes are middleware-level (DB context, error handler, rate limiter) plus a small utility for param parsing. Client changes are a TanStack Router error boundary on the root route and extracting three tab components from the 634-line collection route. Docs change is a one-line fix in PROJECT.md.
|
|
|
|
**Tech Stack:** Hono middleware, TanStack Router errorComponent, React, TypeScript
|
|
|
|
---
|
|
|
|
### Task 1: Explicit DB Context Middleware
|
|
|
|
**Files:**
|
|
- Modify: `src/server/index.ts:1-59`
|
|
- Modify: `src/server/routes/settings.ts:3,12` (remove prodDb fallback)
|
|
|
|
- [ ] **Step 1: Add DB import and middleware to server index**
|
|
|
|
In `src/server/index.ts`, add the import for the production database at the top, alongside existing imports:
|
|
|
|
```ts
|
|
import { db as prodDb } from "../db/index.ts";
|
|
```
|
|
|
|
Then add a middleware **before** the auth middleware (before line 26) that sets the DB on every API request:
|
|
|
|
```ts
|
|
// Inject production database into request context
|
|
app.use("/api/*", async (c, next) => {
|
|
c.set("db", prodDb);
|
|
return next();
|
|
});
|
|
```
|
|
|
|
- [ ] **Step 2: Fix auth middleware comment**
|
|
|
|
In the same file, update the comment on the auth middleware from:
|
|
|
|
```ts
|
|
// Auth middleware for write operations (POST/PUT/DELETE) on non-auth routes
|
|
```
|
|
|
|
to:
|
|
|
|
```ts
|
|
// Auth middleware for write operations (POST/PUT/PATCH/DELETE) on non-auth routes
|
|
```
|
|
|
|
- [ ] **Step 3: Remove prodDb fallback from settings route**
|
|
|
|
In `src/server/routes/settings.ts`, remove the `prodDb` import and fallback. Change:
|
|
|
|
```ts
|
|
import { db as prodDb } from "../../db/index.ts";
|
|
```
|
|
|
|
Remove this import entirely.
|
|
|
|
Change both occurrences of:
|
|
```ts
|
|
const database = c.get("db") ?? prodDb;
|
|
```
|
|
to:
|
|
```ts
|
|
const database = c.get("db");
|
|
```
|
|
|
|
- [ ] **Step 4: Run tests**
|
|
|
|
Run: `bun test`
|
|
Expected: All 183 tests pass. Tests already set `c.set("db", testDb)` so this change doesn't affect them.
|
|
|
|
- [ ] **Step 5: Run lint**
|
|
|
|
Run: `bun run lint`
|
|
Expected: No errors.
|
|
|
|
- [ ] **Step 6: Commit**
|
|
|
|
```bash
|
|
git add src/server/index.ts src/server/routes/settings.ts
|
|
git commit -m "fix: add explicit DB context middleware for all API routes"
|
|
```
|
|
|
|
---
|
|
|
|
### Task 2: Route Parameter Validation
|
|
|
|
**Files:**
|
|
- Create: `src/server/lib/params.ts`
|
|
- Modify: `src/server/routes/items.ts`
|
|
- Modify: `src/server/routes/categories.ts`
|
|
- Modify: `src/server/routes/threads.ts`
|
|
- Modify: `src/server/routes/setups.ts`
|
|
- Modify: `src/server/routes/auth.ts:187-189`
|
|
|
|
- [ ] **Step 1: Create parseId helper**
|
|
|
|
Create `src/server/lib/params.ts`:
|
|
|
|
```ts
|
|
/**
|
|
* Parse a route parameter as a positive integer ID.
|
|
* Returns the number if valid, or null if the string is not a positive integer.
|
|
*/
|
|
export function parseId(raw: string): number | null {
|
|
const id = Number(raw);
|
|
if (!Number.isInteger(id) || id <= 0) return null;
|
|
return id;
|
|
}
|
|
```
|
|
|
|
- [ ] **Step 2: Update items routes**
|
|
|
|
In `src/server/routes/items.ts`, add the import:
|
|
|
|
```ts
|
|
import { parseId } from "../lib/params.ts";
|
|
```
|
|
|
|
Replace all `Number(c.req.param("id"))` patterns. For each route that uses an ID param, add validation. Example for `GET /:id`:
|
|
|
|
```ts
|
|
app.get("/:id", (c) => {
|
|
const db = c.get("db");
|
|
const id = parseId(c.req.param("id"));
|
|
if (!id) return c.json({ error: "Invalid item ID" }, 400);
|
|
const item = getItemById(db, id);
|
|
if (!item) return c.json({ error: "Item not found" }, 404);
|
|
return c.json(item);
|
|
});
|
|
```
|
|
|
|
Apply the same pattern to `PUT /:id` and `DELETE /:id`. In each case, add `const id = parseId(...)` + the null check returning 400 right after.
|
|
|
|
- [ ] **Step 3: Update categories routes**
|
|
|
|
In `src/server/routes/categories.ts`, add the import:
|
|
|
|
```ts
|
|
import { parseId } from "../lib/params.ts";
|
|
```
|
|
|
|
Replace `Number(c.req.param("id"))` with `parseId(c.req.param("id"))` in `PUT /:id` and `DELETE /:id`, adding the null check:
|
|
|
|
```ts
|
|
const id = parseId(c.req.param("id"));
|
|
if (!id) return c.json({ error: "Invalid category ID" }, 400);
|
|
```
|
|
|
|
- [ ] **Step 4: Update threads routes**
|
|
|
|
In `src/server/routes/threads.ts`, add the import:
|
|
|
|
```ts
|
|
import { parseId } from "../lib/params.ts";
|
|
```
|
|
|
|
Replace all `Number(c.req.param(...))` calls. There are 8 occurrences across these handlers:
|
|
- `GET /:id` — `const id = parseId(c.req.param("id"))`
|
|
- `PUT /:id` — same
|
|
- `DELETE /:id` — same
|
|
- `POST /:id/candidates` — `const threadId = parseId(c.req.param("id"))`
|
|
- `PUT /:threadId/candidates/:candidateId` — `const candidateId = parseId(c.req.param("candidateId"))`
|
|
- `DELETE /:threadId/candidates/:candidateId` — same
|
|
- `PATCH /:id/candidates/reorder` — `const threadId = parseId(c.req.param("id"))`
|
|
- `POST /:id/resolve` — `const threadId = parseId(c.req.param("id"))`
|
|
|
|
For each, add the null check returning 400 with a descriptive message like `"Invalid thread ID"` or `"Invalid candidate ID"`.
|
|
|
|
- [ ] **Step 5: Update setups routes**
|
|
|
|
In `src/server/routes/setups.ts`, add the import:
|
|
|
|
```ts
|
|
import { parseId } from "../lib/params.ts";
|
|
```
|
|
|
|
Replace all `Number(c.req.param(...))` calls. There are 6 occurrences:
|
|
- `GET /:id` — `const id = parseId(c.req.param("id"))`
|
|
- `PUT /:id` — same
|
|
- `DELETE /:id` — same
|
|
- `PUT /:id/items` — same
|
|
- `PATCH /:id/items/:itemId/classification` — both `setupId` and `itemId`
|
|
- `DELETE /:id/items/:itemId` — both `setupId` and `itemId`
|
|
|
|
For the classification and item removal routes with two params:
|
|
|
|
```ts
|
|
const setupId = parseId(c.req.param("id"));
|
|
const itemId = parseId(c.req.param("itemId"));
|
|
if (!setupId || !itemId) return c.json({ error: "Invalid ID" }, 400);
|
|
```
|
|
|
|
- [ ] **Step 6: Update auth routes**
|
|
|
|
In `src/server/routes/auth.ts`, add the import:
|
|
|
|
```ts
|
|
import { parseId } from "../lib/params.ts";
|
|
```
|
|
|
|
Update `DELETE /keys/:id` (line 187-189):
|
|
|
|
```ts
|
|
app.delete("/keys/:id", requireAuth, (c) => {
|
|
const db = c.get("db");
|
|
const id = parseId(c.req.param("id"));
|
|
if (!id) return c.json({ error: "Invalid key ID" }, 400);
|
|
deleteApiKey(db, id);
|
|
return c.json({ ok: true });
|
|
});
|
|
```
|
|
|
|
- [ ] **Step 7: Run tests**
|
|
|
|
Run: `bun test`
|
|
Expected: All 183 tests pass. Existing tests use valid integer IDs so no breakage.
|
|
|
|
- [ ] **Step 8: Run lint**
|
|
|
|
Run: `bun run lint`
|
|
Expected: No errors.
|
|
|
|
- [ ] **Step 9: Commit**
|
|
|
|
```bash
|
|
git add src/server/lib/params.ts src/server/routes/items.ts src/server/routes/categories.ts src/server/routes/threads.ts src/server/routes/setups.ts src/server/routes/auth.ts
|
|
git commit -m "fix: validate route ID parameters, return 400 for invalid IDs"
|
|
```
|
|
|
|
---
|
|
|
|
### Task 3: Centralized Error Handler
|
|
|
|
**Files:**
|
|
- Modify: `src/server/index.ts`
|
|
|
|
- [ ] **Step 1: Add onError handler**
|
|
|
|
In `src/server/index.ts`, add the error handler after the app is created (after `const app = new Hono()`) but before any routes:
|
|
|
|
```ts
|
|
// Centralized error handler
|
|
app.onError((err, c) => {
|
|
console.error(`[${c.req.method}] ${c.req.path}:`, err);
|
|
const message =
|
|
process.env.NODE_ENV === "production"
|
|
? "Internal server error"
|
|
: err.message || "Internal server error";
|
|
return c.json({ error: message }, 500);
|
|
});
|
|
```
|
|
|
|
- [ ] **Step 2: Run tests**
|
|
|
|
Run: `bun test`
|
|
Expected: All 183 tests pass.
|
|
|
|
- [ ] **Step 3: Commit**
|
|
|
|
```bash
|
|
git add src/server/index.ts
|
|
git commit -m "fix: add centralized error handler for unhandled exceptions"
|
|
```
|
|
|
|
---
|
|
|
|
### Task 4: Rate Limiting on Auth Endpoints
|
|
|
|
**Files:**
|
|
- Create: `src/server/middleware/rateLimit.ts`
|
|
- Modify: `src/server/routes/auth.ts`
|
|
|
|
- [ ] **Step 1: Create rate limiter middleware**
|
|
|
|
Create `src/server/middleware/rateLimit.ts`:
|
|
|
|
```ts
|
|
import type { Context, Next } from "hono";
|
|
|
|
interface RateLimitEntry {
|
|
count: number;
|
|
resetAt: number;
|
|
}
|
|
|
|
const store = new Map<string, RateLimitEntry>();
|
|
|
|
const MAX_ATTEMPTS = 5;
|
|
const WINDOW_MS = 15 * 60 * 1000; // 15 minutes
|
|
|
|
function getClientIp(c: Context): string {
|
|
return c.req.header("x-forwarded-for")?.split(",")[0]?.trim() || "unknown";
|
|
}
|
|
|
|
function cleanup() {
|
|
const now = Date.now();
|
|
for (const [key, entry] of store) {
|
|
if (now >= entry.resetAt) {
|
|
store.delete(key);
|
|
}
|
|
}
|
|
}
|
|
|
|
export async function rateLimit(c: Context, next: Next) {
|
|
cleanup();
|
|
|
|
const ip = getClientIp(c);
|
|
const key = `${ip}:${c.req.path}`;
|
|
const now = Date.now();
|
|
|
|
const entry = store.get(key);
|
|
|
|
if (!entry || now >= entry.resetAt) {
|
|
store.set(key, { count: 1, resetAt: now + WINDOW_MS });
|
|
return next();
|
|
}
|
|
|
|
if (entry.count >= MAX_ATTEMPTS) {
|
|
const retryAfter = Math.ceil((entry.resetAt - now) / 1000);
|
|
c.header("Retry-After", String(retryAfter));
|
|
return c.json({ error: "Too many attempts. Try again later." }, 429);
|
|
}
|
|
|
|
entry.count++;
|
|
return next();
|
|
}
|
|
```
|
|
|
|
- [ ] **Step 2: Apply rate limiter to auth routes**
|
|
|
|
In `src/server/routes/auth.ts`, add the import:
|
|
|
|
```ts
|
|
import { rateLimit } from "../middleware/rateLimit.ts";
|
|
```
|
|
|
|
Update the `POST /setup` handler to include the rate limiter:
|
|
|
|
```ts
|
|
app.post("/setup", rateLimit, zValidator("json", setupSchema), async (c) => {
|
|
```
|
|
|
|
Update the `POST /login` handler to include the rate limiter:
|
|
|
|
```ts
|
|
app.post("/login", rateLimit, zValidator("json", loginSchema), async (c) => {
|
|
```
|
|
|
|
- [ ] **Step 3: Run tests**
|
|
|
|
Run: `bun test`
|
|
Expected: All 183 tests pass. Auth tests make fewer than 5 requests per endpoint so rate limiting won't trigger.
|
|
|
|
- [ ] **Step 4: Run lint**
|
|
|
|
Run: `bun run lint`
|
|
Expected: No errors.
|
|
|
|
- [ ] **Step 5: Commit**
|
|
|
|
```bash
|
|
git add src/server/middleware/rateLimit.ts src/server/routes/auth.ts
|
|
git commit -m "feat: add rate limiting on login and setup endpoints"
|
|
```
|
|
|
|
---
|
|
|
|
### Task 5: Client Error Boundary
|
|
|
|
**Files:**
|
|
- Modify: `src/client/routes/__root.tsx`
|
|
|
|
- [ ] **Step 1: Add error boundary component and wire it up**
|
|
|
|
In `src/client/routes/__root.tsx`, add the import for `useRouter` at the top (add to existing import from `@tanstack/react-router`):
|
|
|
|
```ts
|
|
import {
|
|
createRootRoute,
|
|
Outlet,
|
|
useMatchRoute,
|
|
useNavigate,
|
|
useRouter,
|
|
type ErrorComponentProps,
|
|
} from "@tanstack/react-router";
|
|
```
|
|
|
|
Add the `errorComponent` to the route definition:
|
|
|
|
```ts
|
|
export const Route = createRootRoute({
|
|
component: RootLayout,
|
|
errorComponent: RootErrorBoundary,
|
|
});
|
|
```
|
|
|
|
Add the `RootErrorBoundary` function before `RootLayout`:
|
|
|
|
```tsx
|
|
function RootErrorBoundary({ error, reset }: ErrorComponentProps) {
|
|
const router = useRouter();
|
|
|
|
return (
|
|
<div className="min-h-screen bg-gray-50 flex items-center justify-center">
|
|
<div className="max-w-md mx-auto text-center px-4">
|
|
<div className="w-12 h-12 bg-red-100 rounded-full flex items-center justify-center mx-auto mb-4">
|
|
<svg
|
|
className="w-6 h-6 text-red-600"
|
|
fill="none"
|
|
stroke="currentColor"
|
|
viewBox="0 0 24 24"
|
|
>
|
|
<path
|
|
strokeLinecap="round"
|
|
strokeLinejoin="round"
|
|
strokeWidth={2}
|
|
d="M12 9v2m0 4h.01m-6.938 4h13.856c1.54 0 2.502-1.667 1.732-2.5L13.732 4c-.77-.833-1.964-.833-2.732 0L4.082 16.5c-.77.833.192 2.5 1.732 2.5z"
|
|
/>
|
|
</svg>
|
|
</div>
|
|
<h1 className="text-xl font-semibold text-gray-900 mb-2">
|
|
Something went wrong
|
|
</h1>
|
|
<p className="text-sm text-gray-500 mb-6">
|
|
{error instanceof Error ? error.message : "An unexpected error occurred"}
|
|
</p>
|
|
<button
|
|
type="button"
|
|
onClick={() => {
|
|
reset();
|
|
router.invalidate();
|
|
}}
|
|
className="px-5 py-2.5 bg-gray-700 hover:bg-gray-800 text-white text-sm font-medium rounded-lg transition-colors"
|
|
>
|
|
Try again
|
|
</button>
|
|
</div>
|
|
</div>
|
|
);
|
|
}
|
|
```
|
|
|
|
- [ ] **Step 2: Run lint**
|
|
|
|
Run: `bun run lint`
|
|
Expected: No errors.
|
|
|
|
- [ ] **Step 3: Run tests**
|
|
|
|
Run: `bun test`
|
|
Expected: All 183 tests pass.
|
|
|
|
- [ ] **Step 4: Commit**
|
|
|
|
```bash
|
|
git add src/client/routes/__root.tsx
|
|
git commit -m "feat: add error boundary to root route for crash resilience"
|
|
```
|
|
|
|
---
|
|
|
|
### Task 6: Split Collection Route into Tab Components
|
|
|
|
**Files:**
|
|
- Create: `src/client/components/CollectionView.tsx`
|
|
- Create: `src/client/components/PlanningView.tsx`
|
|
- Create: `src/client/components/SetupsView.tsx`
|
|
- Modify: `src/client/routes/collection/index.tsx`
|
|
|
|
- [ ] **Step 1: Create CollectionView component**
|
|
|
|
Create `src/client/components/CollectionView.tsx` with the `CollectionView` function extracted from `collection/index.tsx` (lines 72-334). The component needs these imports:
|
|
|
|
```tsx
|
|
import { useMemo, useState } from "react";
|
|
import { CategoryFilterDropdown } from "./CategoryFilterDropdown";
|
|
import { CategoryHeader } from "./CategoryHeader";
|
|
import { ItemCard } from "./ItemCard";
|
|
import { useCategories } from "../hooks/useCategories";
|
|
import { useCurrency } from "../hooks/useCurrency";
|
|
import { useItems } from "../hooks/useItems";
|
|
import { useTotals } from "../hooks/useTotals";
|
|
import { useWeightUnit } from "../hooks/useWeightUnit";
|
|
import { formatPrice, formatWeight } from "../lib/formatters";
|
|
import { LucideIcon } from "../lib/iconData";
|
|
import { useUIStore } from "../stores/uiStore";
|
|
|
|
export function CollectionView() {
|
|
// ... exact same function body as lines 73-334 of collection/index.tsx
|
|
}
|
|
```
|
|
|
|
Copy the entire `CollectionView` function body as-is. No logic changes.
|
|
|
|
- [ ] **Step 2: Create PlanningView component**
|
|
|
|
Create `src/client/components/PlanningView.tsx` with the `PlanningView` function extracted from `collection/index.tsx` (lines 337-523):
|
|
|
|
```tsx
|
|
import { useState } from "react";
|
|
import { CategoryFilterDropdown } from "./CategoryFilterDropdown";
|
|
import { CreateThreadModal } from "./CreateThreadModal";
|
|
import { ThreadCard } from "./ThreadCard";
|
|
import { useCategories } from "../hooks/useCategories";
|
|
import { useThreads } from "../hooks/useThreads";
|
|
import { useUIStore } from "../stores/uiStore";
|
|
|
|
export function PlanningView() {
|
|
// ... exact same function body as lines 338-523 of collection/index.tsx
|
|
}
|
|
```
|
|
|
|
Copy the entire `PlanningView` function body as-is. No logic changes.
|
|
|
|
- [ ] **Step 3: Create SetupsView component**
|
|
|
|
Create `src/client/components/SetupsView.tsx` with the `SetupsView` function extracted from `collection/index.tsx` (lines 526-633):
|
|
|
|
```tsx
|
|
import { useState } from "react";
|
|
import { SetupCard } from "./SetupCard";
|
|
import { useCreateSetup, useSetups } from "../hooks/useSetups";
|
|
|
|
export function SetupsView() {
|
|
// ... exact same function body as lines 527-633 of collection/index.tsx
|
|
}
|
|
```
|
|
|
|
Copy the entire `SetupsView` function body as-is. No logic changes.
|
|
|
|
- [ ] **Step 4: Update collection/index.tsx**
|
|
|
|
Replace the entire file content. Keep only the route definition, tab switching logic, animation constants, and imports from the new components:
|
|
|
|
```tsx
|
|
import { createFileRoute } from "@tanstack/react-router";
|
|
import { AnimatePresence, motion } from "framer-motion";
|
|
import { useRef } from "react";
|
|
import { z } from "zod";
|
|
import { CollectionView } from "../../components/CollectionView";
|
|
import { PlanningView } from "../../components/PlanningView";
|
|
import { SetupsView } from "../../components/SetupsView";
|
|
|
|
const searchSchema = z.object({
|
|
tab: z.enum(["gear", "planning", "setups"]).catch("gear"),
|
|
});
|
|
|
|
export const Route = createFileRoute("/collection/")({
|
|
validateSearch: searchSchema,
|
|
component: CollectionPage,
|
|
});
|
|
|
|
const TAB_ORDER = ["gear", "planning", "setups"] as const;
|
|
|
|
const slideVariants = {
|
|
enter: (dir: number) => ({ x: `${dir * 15}%`, opacity: 0 }),
|
|
center: { x: 0, opacity: 1 },
|
|
exit: (dir: number) => ({ x: `${dir * -15}%`, opacity: 0 }),
|
|
};
|
|
|
|
function CollectionPage() {
|
|
const { tab } = Route.useSearch();
|
|
const prevTab = useRef(tab);
|
|
|
|
const direction =
|
|
TAB_ORDER.indexOf(tab) >= TAB_ORDER.indexOf(prevTab.current) ? 1 : -1;
|
|
prevTab.current = tab;
|
|
|
|
return (
|
|
<div className="max-w-7xl mx-auto px-4 sm:px-6 lg:px-8 py-6 overflow-x-hidden">
|
|
<AnimatePresence mode="wait" initial={false} custom={direction}>
|
|
<motion.div
|
|
key={tab}
|
|
custom={direction}
|
|
variants={slideVariants}
|
|
initial="enter"
|
|
animate="center"
|
|
exit="exit"
|
|
transition={{ duration: 0.12, ease: "easeInOut" }}
|
|
>
|
|
{tab === "gear" ? (
|
|
<CollectionView />
|
|
) : tab === "planning" ? (
|
|
<PlanningView />
|
|
) : (
|
|
<SetupsView />
|
|
)}
|
|
</motion.div>
|
|
</AnimatePresence>
|
|
</div>
|
|
);
|
|
}
|
|
```
|
|
|
|
- [ ] **Step 5: Run lint**
|
|
|
|
Run: `bun run lint`
|
|
Expected: No errors. (Biome may flag import organization — fix if needed.)
|
|
|
|
- [ ] **Step 6: Run tests**
|
|
|
|
Run: `bun test`
|
|
Expected: All 183 tests pass.
|
|
|
|
- [ ] **Step 7: Commit**
|
|
|
|
```bash
|
|
git add src/client/components/CollectionView.tsx src/client/components/PlanningView.tsx src/client/components/SetupsView.tsx src/client/routes/collection/index.tsx
|
|
git commit -m "refactor: extract tab views from collection route into separate components"
|
|
```
|
|
|
|
---
|
|
|
|
### Task 7: Docs Cleanup
|
|
|
|
**Files:**
|
|
- Modify: `.planning/PROJECT.md:84`
|
|
|
|
- [ ] **Step 1: Update stale constraint**
|
|
|
|
In `.planning/PROJECT.md`, change line 84 from:
|
|
|
|
```
|
|
- **Scope**: No auth, single user for v1
|
|
```
|
|
|
|
to:
|
|
|
|
```
|
|
- **Scope**: Single user with cookie/API key auth
|
|
```
|
|
|
|
- [ ] **Step 2: Commit**
|
|
|
|
```bash
|
|
git add .planning/PROJECT.md
|
|
git commit -m "docs: update PROJECT.md constraints to reflect auth implementation"
|
|
```
|
|
|
|
---
|
|
|
|
### Task 8: Final Verification
|
|
|
|
- [ ] **Step 1: Run full test suite**
|
|
|
|
Run: `bun test`
|
|
Expected: All 183 tests pass.
|
|
|
|
- [ ] **Step 2: Run lint**
|
|
|
|
Run: `bun run lint`
|
|
Expected: No errors.
|
|
|
|
- [ ] **Step 3: Verify dev server starts**
|
|
|
|
Run: `bun run dev:server &` then `curl http://localhost:3000/api/health`
|
|
Expected: `{"status":"ok"}`
|
|
Then kill the background server.
|