test: add unit tests for rate limiter middleware
This commit is contained in:
@@ -0,0 +1,74 @@
|
||||
# Code Quality Improvements (Round 2) Design
|
||||
|
||||
**Date:** 2026-04-03
|
||||
**Scope:** Combined formatters hook, test helper schema generation, stale todo cleanup
|
||||
|
||||
## 1. useFormatters Combined Hook
|
||||
|
||||
**Problem:** 14 component files import the same 3-4 lines: `useWeightUnit`, `useCurrency`, `formatWeight`, `formatPrice`. This is repetitive boilerplate.
|
||||
|
||||
**Solution:** Create `src/client/hooks/useFormatters.ts` that returns pre-bound formatting functions:
|
||||
|
||||
```ts
|
||||
export function useFormatters() {
|
||||
const unit = useWeightUnit();
|
||||
const currency = useCurrency();
|
||||
return {
|
||||
weight: (grams: number | null) => formatWeight(grams, unit),
|
||||
price: (cents: number | null) => formatPrice(cents, currency),
|
||||
unit,
|
||||
currency,
|
||||
};
|
||||
}
|
||||
```
|
||||
|
||||
**Consumer files to update (14):**
|
||||
- CollectionView.tsx
|
||||
- setups/$setupId.tsx
|
||||
- routes/index.tsx
|
||||
- WeightSummaryCard.tsx
|
||||
- TotalsBar.tsx
|
||||
- settings.tsx
|
||||
- ThreadCard.tsx
|
||||
- SetupCard.tsx
|
||||
- ItemPicker.tsx
|
||||
- ItemCard.tsx
|
||||
- ComparisonTable.tsx
|
||||
- CandidateCard.tsx
|
||||
- CandidateListItem.tsx
|
||||
- CategoryHeader.tsx
|
||||
|
||||
Each file replaces 3-4 imports + 2 hook calls with 1 import + 1 destructured hook call. Components that need raw `unit` or `currency` (e.g., WeightSummaryCard uses `unit` as a type, TotalsBar has a unit toggle) get them from the return object.
|
||||
|
||||
## 2. Test Helper Schema Generation
|
||||
|
||||
**Problem:** `tests/helpers/db.ts` has 120 lines of hand-written CREATE TABLE SQL that must manually mirror `src/db/schema.ts`. Any schema change requires updating both files — a known source of `SqliteError: no such column` failures.
|
||||
|
||||
**Solution:** Replace hand-written SQL with Drizzle's migration runner:
|
||||
|
||||
```ts
|
||||
import { migrate } from "drizzle-orm/bun-sqlite/migrator";
|
||||
|
||||
export function createTestDb() {
|
||||
const sqlite = new Database(":memory:");
|
||||
sqlite.run("PRAGMA foreign_keys = ON");
|
||||
const db = drizzle(sqlite, { schema });
|
||||
migrate(db, { migrationsFolder: "./drizzle" });
|
||||
db.insert(schema.categories).values({ name: "Uncategorized", icon: "package" }).run();
|
||||
return db;
|
||||
}
|
||||
```
|
||||
|
||||
This reduces the file from ~128 lines to ~15 lines and eliminates all future manual sync.
|
||||
|
||||
## 3. Stale Todo Cleanup
|
||||
|
||||
**Problem:** Pending todo "Replace planning category filter select with icon-aware dropdown" from 2026-03-15 is already resolved — `PlanningView.tsx` uses `<CategoryFilterDropdown>` which renders Lucide icons.
|
||||
|
||||
**Solution:** Move the todo file from `pending/` to `done/`.
|
||||
|
||||
## Commit Strategy
|
||||
|
||||
1. **useFormatters hook** — create hook + update all 14 consumer files
|
||||
2. **Test helper migration** — replace hand-written SQL with migrate()
|
||||
3. **Todo cleanup** — move stale todo to done
|
||||
@@ -0,0 +1,152 @@
|
||||
# Codebase Improvements Design
|
||||
|
||||
**Date:** 2026-04--03
|
||||
**Scope:** General code quality, error handling, resilience, and maintainability improvements
|
||||
|
||||
## 1. Server Hardening
|
||||
|
||||
### 1a. Explicit DB Context Middleware
|
||||
|
||||
**File:** `src/server/index.ts`
|
||||
|
||||
Add middleware that explicitly sets `c.set("db", prodDb)` for all API routes. Currently routes call `c.get("db")` but nothing sets it in production — services silently fall back to `prodDb` via default parameters. This makes production behavior match the test pattern.
|
||||
|
||||
```ts
|
||||
import { db as prodDb } from "../db/index.ts";
|
||||
|
||||
app.use("/api/*", async (c, next) => {
|
||||
c.set("db", prodDb);
|
||||
return next();
|
||||
});
|
||||
```
|
||||
|
||||
Place this **before** the auth middleware so `db` is available when auth checks run.
|
||||
|
||||
### 1b. Route Parameter Validation
|
||||
|
||||
**New file:** `src/server/lib/params.ts`
|
||||
|
||||
Create a helper that validates numeric route params:
|
||||
|
||||
```ts
|
||||
export function parseId(raw: string): number | null {
|
||||
const id = Number(raw);
|
||||
if (!Number.isInteger(id) || id <= 0) return null;
|
||||
return id;
|
||||
}
|
||||
```
|
||||
|
||||
Update all route files (`items.ts`, `threads.ts`, `categories.ts`, `setups.ts`) to replace `Number(c.req.param("id"))` with `parseId()`, returning 400 for invalid IDs.
|
||||
|
||||
### 1c. Centralized Error Handling
|
||||
|
||||
**File:** `src/server/index.ts`
|
||||
|
||||
Add Hono's `onError` handler:
|
||||
|
||||
```ts
|
||||
app.onError((err, c) => {
|
||||
console.error(`[${c.req.method}] ${c.req.path}:`, err);
|
||||
const status = err instanceof HTTPException ? err.status : 500;
|
||||
const message = process.env.NODE_ENV === "production"
|
||||
? "Internal server error"
|
||||
: err.message;
|
||||
return c.json({ error: message }, status);
|
||||
});
|
||||
```
|
||||
|
||||
### 1d. Auth Comment Fix
|
||||
|
||||
**File:** `src/server/index.ts`
|
||||
|
||||
Change comment from:
|
||||
```
|
||||
// Auth middleware for write operations (POST/PUT/DELETE) on non-auth routes
|
||||
```
|
||||
To:
|
||||
```
|
||||
// Auth middleware for write operations (POST/PUT/PATCH/DELETE) on non-auth routes
|
||||
```
|
||||
|
||||
### 1e. Rate Limiting on Auth Endpoints
|
||||
|
||||
**New file:** `src/server/middleware/rateLimit.ts`
|
||||
|
||||
In-memory rate limiter using a `Map<string, { count: number; resetAt: number }>`:
|
||||
|
||||
- Tracks by IP (`c.req.header("x-forwarded-for") || "unknown"`)
|
||||
- 5 attempts per 15-minute window
|
||||
- Returns 429 with `{ error: "Too many attempts. Try again later." }` and `Retry-After` header
|
||||
- Stale entries cleaned on each check
|
||||
- Applied to `POST /api/auth/login` and `POST /api/auth/setup`
|
||||
|
||||
## 2. Client Resilience
|
||||
|
||||
### Error Boundary
|
||||
|
||||
**File:** `src/client/routes/__root.tsx`
|
||||
|
||||
Add `errorComponent` to the root route definition:
|
||||
|
||||
```ts
|
||||
export const Route = createRootRoute({
|
||||
component: RootLayout,
|
||||
errorComponent: RootErrorBoundary,
|
||||
});
|
||||
```
|
||||
|
||||
`RootErrorBoundary` renders a centered error message with:
|
||||
- "Something went wrong" heading
|
||||
- Error message in dev mode
|
||||
- "Try again" button that calls `router.invalidate()` + `reset()`
|
||||
|
||||
Uses TanStack Router's `ErrorComponentProps` which provides `error` and `reset`.
|
||||
|
||||
## 3. Client Refactor
|
||||
|
||||
### Split collection/index.tsx
|
||||
|
||||
Extract the three tab-level functions into separate component files:
|
||||
|
||||
| Source function | New file | Approx lines |
|
||||
|----------------|----------|-------------|
|
||||
| `CollectionView()` | `src/client/components/CollectionView.tsx` | ~260 |
|
||||
| `PlanningView()` | `src/client/components/PlanningView.tsx` | ~190 |
|
||||
| `SetupsView()` | `src/client/components/SetupsView.tsx` | ~110 |
|
||||
|
||||
`collection/index.tsx` keeps:
|
||||
- Route definition with `searchSchema` and `validateSearch`
|
||||
- `CollectionPage` function (tab switcher + AnimatePresence)
|
||||
- `TAB_ORDER` and `slideVariants` constants
|
||||
- Imports from the three new component files
|
||||
|
||||
Each extracted component is a named export, self-contained with its own hooks and local state.
|
||||
|
||||
## 4. Docs Cleanup
|
||||
|
||||
### PROJECT.md
|
||||
|
||||
**File:** `.planning/PROJECT.md`
|
||||
|
||||
Update Constraints section line:
|
||||
```
|
||||
- **Scope**: No auth, single user for v1
|
||||
```
|
||||
To:
|
||||
```
|
||||
- **Scope**: Single user with cookie/API key auth
|
||||
```
|
||||
|
||||
## Commit Strategy
|
||||
|
||||
Group into 3-4 commits by area:
|
||||
1. **Server hardening**: DB middleware, param validation, error handler, rate limiter, comment fix
|
||||
2. **Client resilience + refactor**: Error boundary, split collection route
|
||||
3. **Docs cleanup**: PROJECT.md update
|
||||
|
||||
## Testing
|
||||
|
||||
- All 183 existing tests must continue to pass
|
||||
- Rate limiter: manual verification (no automated test needed for in-memory rate limiting in a single-user app)
|
||||
- Error boundary: manual verification by triggering a render error
|
||||
- Param validation: existing route tests cover happy paths; invalid IDs are a new edge case but won't break existing tests
|
||||
128
docs/superpowers/specs/2026-04-03-testing-improvements-design.md
Normal file
128
docs/superpowers/specs/2026-04-03-testing-improvements-design.md
Normal file
@@ -0,0 +1,128 @@
|
||||
# Testing Improvements Design
|
||||
|
||||
**Date:** 2026-04-03
|
||||
**Scope:** Unit tests for new server code + Playwright E2E test setup with seeded database
|
||||
|
||||
## Part 1: Unit/Integration Tests (Bun test runner)
|
||||
|
||||
### tests/lib/params.test.ts
|
||||
|
||||
Tests for `parseId` helper in `src/server/lib/params.ts`:
|
||||
- Valid positive integers (1, 42, 999) return the number
|
||||
- Zero returns null
|
||||
- Negative numbers (-1, -100) return null
|
||||
- Decimals (1.5, 3.14) return null
|
||||
- Non-numeric strings ("abc", "", "hello") return null
|
||||
- NaN-producing values return null
|
||||
|
||||
### tests/middleware/rateLimit.test.ts
|
||||
|
||||
Tests for rate limiter in `src/server/middleware/rateLimit.ts`:
|
||||
- First request passes through (200)
|
||||
- 5 requests succeed, 6th returns 429
|
||||
- 429 response includes `Retry-After` header
|
||||
- Different IPs tracked independently
|
||||
- After window expires, requests succeed again
|
||||
|
||||
Since the rate limiter uses a module-level `Map`, tests need to either:
|
||||
- Reset the store between tests (export a `resetStore` for testing), OR
|
||||
- Use unique paths/IPs per test to avoid interference
|
||||
|
||||
Recommended: export a `_resetForTesting()` function from rateLimit.ts that clears the store. Only used in tests.
|
||||
|
||||
### tests/routes/params.test.ts
|
||||
|
||||
Route-level integration tests verifying 400 responses for invalid IDs:
|
||||
- `GET /api/items/abc` → 400
|
||||
- `GET /api/items/-1` → 400
|
||||
- `GET /api/items/0` → 400
|
||||
- `DELETE /api/categories/notanumber` → 400
|
||||
- `GET /api/threads/abc` → 400
|
||||
- `GET /api/setups/abc` → 400
|
||||
|
||||
Uses existing test app pattern with in-memory DB.
|
||||
|
||||
## Part 2: Playwright E2E Setup
|
||||
|
||||
### Installation
|
||||
|
||||
- `bun add -d @playwright/test`
|
||||
- `bunx playwright install chromium` (only Chromium needed)
|
||||
|
||||
### Configuration: playwright.config.ts
|
||||
|
||||
```ts
|
||||
export default defineConfig({
|
||||
testDir: "./e2e",
|
||||
webServer: {
|
||||
command: "DATABASE_PATH=./e2e/test.db bun run dev:server",
|
||||
port: 3000,
|
||||
reuseExistingServer: !process.env.CI,
|
||||
},
|
||||
use: {
|
||||
baseURL: "http://localhost:3000",
|
||||
},
|
||||
projects: [{ name: "chromium", use: { ...devices["Desktop Chrome"] } }],
|
||||
});
|
||||
```
|
||||
|
||||
### Database Seeding: e2e/seed.ts
|
||||
|
||||
Script that creates `e2e/test.db` with:
|
||||
- Run Drizzle migrations against the file
|
||||
- Seed data:
|
||||
- 1 user (username: "admin", password: "password123")
|
||||
- 3 categories: Shelter, Sleep System, Cook Kit
|
||||
- 6 items across categories with realistic weights/prices
|
||||
- 1 active thread with 3 candidates (with pros/cons, sort_order)
|
||||
- 1 resolved thread
|
||||
- 1 setup with 4 items (mixed classifications)
|
||||
- Settings: weightUnit=g, currency=USD, onboardingComplete=true
|
||||
|
||||
Run before E2E tests via `e2e/global-setup.ts` (Playwright globalSetup).
|
||||
|
||||
### E2E Test Files
|
||||
|
||||
**e2e/dashboard.spec.ts**
|
||||
- Dashboard page loads
|
||||
- Summary cards show item count, weight, cost
|
||||
- Navigation links to collection work
|
||||
|
||||
**e2e/collection.spec.ts**
|
||||
- Gear tab renders items grouped by category
|
||||
- Search input filters items by name
|
||||
- Category filter dropdown works
|
||||
- Tab switching between gear/planning/setups
|
||||
|
||||
**e2e/threads.spec.ts**
|
||||
- Thread detail page loads with candidates
|
||||
- Comparison view toggle works (shows table)
|
||||
- Rank badges visible on candidates
|
||||
|
||||
**e2e/auth.spec.ts**
|
||||
- Login page renders
|
||||
- Login with valid credentials succeeds
|
||||
- Login with wrong password shows error
|
||||
- Rate limiting returns error after 5 attempts
|
||||
|
||||
**e2e/error-boundary.spec.ts**
|
||||
- App doesn't white-screen on unknown routes
|
||||
- Navigating to a non-existent thread/setup shows appropriate error
|
||||
|
||||
### Scripts
|
||||
|
||||
Add to package.json:
|
||||
- `"test:e2e": "bunx playwright test"`
|
||||
- `"test:e2e:ui": "bunx playwright test --ui"` (for debugging)
|
||||
|
||||
### Files to .gitignore
|
||||
|
||||
- `e2e/test.db`
|
||||
- `test-results/`
|
||||
- `playwright-report/`
|
||||
|
||||
## Commit Strategy
|
||||
|
||||
1. Unit tests for parseId, rate limiter, route params
|
||||
2. Playwright setup (install, config, seed, global-setup)
|
||||
3. Playwright E2E test files
|
||||
Reference in New Issue
Block a user