WIP! A BB-style forum, on the ATmosphere! We're still working... we'll be back soon when we have something to show off!
node typescript hono htmx atproto

docs: add critical patterns learned from ATB-12 PR review cycles

Elevates lessons from ATB-12 (two review cycles) to shared team knowledge.

Added to "TypeScript / Hono Gotchas":
- Type guards for API endpoint parameters (prevent runtime crashes)
- JSON parsing safety pattern (malformed JSON → 400, not 500)

Added to "Testing Standards":
- Pre-review checklist (run before requesting review)
- Dependencies verification (runtime imports must be in dependencies)
- Error test coverage requirements (write during implementation, not after review)

Added to "Error Handling Standards":
- Error classification testing patterns with examples
- Test cases for 400/404/503/500 status codes
- Emphasis on testing user-facing error messages, not just catching

Why these matter:
- Type guards prevent TypeError crashes from missing/wrong-type fields
- Pre-review checklist prevents two-cycle review pattern (implement → review → fix → review)
- Error classification tests verify user experience (actionable feedback), not just error handling

These patterns caught 5 critical issues in ATB-12 review that would have caused:
- Production crashes (TypeError from missing type guards)
- Deployment failures (drizzle-orm in devDependencies)
- Poor UX (network errors returning 500 instead of 503)

+96
+96
CLAUDE.md
··· 163 163 - Write tests after requesting review - tests inform implementation 164 164 - Rely on manual testing alone - automated tests catch regressions 165 165 166 + ### Before Requesting Code Review 167 + 168 + **CRITICAL: Run this checklist before requesting review to catch issues early:** 169 + 170 + ```sh 171 + # 1. Verify all tests pass 172 + pnpm test 173 + 174 + # 2. Check runtime dependencies are correctly placed 175 + # (Runtime imports must be in dependencies, not devDependencies) 176 + grep -r "from 'drizzle-orm'" apps/*/src # If found, verify in dependencies 177 + grep -r "from 'postgres'" apps/*/src # If found, verify in dependencies 178 + 179 + # 3. Verify error test coverage is comprehensive 180 + # For API endpoints, ensure you have tests for: 181 + # - Input validation (missing fields, wrong types, malformed JSON) 182 + # - Error classification (network→503, server→500) 183 + # - Error message clarity (user-friendly, no stack traces) 184 + ``` 185 + 186 + **Common mistake:** Adding error tests AFTER review feedback instead of DURING implementation. Write error tests immediately after implementing the happy path — they often reveal bugs in error classification and input validation that are better caught before review. 187 + 166 188 ## Lexicon Conventions 167 189 168 190 - **Source of truth is YAML** in `packages/lexicon/lexicons/`. Never edit generated JSON or TypeScript. ··· 188 210 - **HTMX attributes in JSX:** The `typed-htmx` package provides types for `hx-*` attributes. See `apps/web/src/global.d.ts` for the augmentation. 189 211 - **Glob expansion in npm scripts:** `@atproto/lex-cli` needs file paths, not globs. Use `bash -c 'shopt -s globstar && ...'` to expand `**/*.json` in npm scripts. 190 212 - **`.env` loading:** Dev and spike scripts use Node's `--env-file=../../.env` flag to load the root `.env` file. No `dotenv` dependency needed. 213 + - **API endpoint parameter type guards:** Never trust TypeScript types for user input. Change handler parameter types from `string` to `unknown` and add explicit `typeof` checks. TypeScript types are erased at runtime — a request missing the `text` field will pass type checking but crash with `TypeError: text.trim is not a function`. 214 + ```typescript 215 + // ❌ BAD: Assumes text is always a string at runtime 216 + export function validatePostText(text: string): { valid: boolean } { 217 + const trimmed = text.trim(); // Crashes if text is undefined! 218 + // ... 219 + } 220 + 221 + // ✅ GOOD: Type guard protects against runtime type mismatches 222 + export function validatePostText(text: unknown): { valid: boolean } { 223 + if (typeof text !== "string") { 224 + return { valid: false, error: "Text is required and must be a string" }; 225 + } 226 + const trimmed = text.trim(); // Safe - text is proven to be a string 227 + // ... 228 + } 229 + ``` 230 + - **Hono JSON parsing safety:** `await c.req.json()` throws `SyntaxError` for malformed JSON. Always wrap in try-catch and return 400 for client errors: 231 + ```typescript 232 + let body: any; 233 + try { 234 + body = await c.req.json(); 235 + } catch { 236 + return c.json({ error: "Invalid JSON in request body" }, 400); 237 + } 238 + ``` 191 239 192 240 ## Error Handling Standards 193 241 ··· 315 363 ); 316 364 }); 317 365 ``` 366 + 367 + ### Testing Error Handling 368 + 369 + **Test error classification, not just error catching.** Users need actionable feedback: "retry later" (503) vs "report this bug" (500). 370 + 371 + ```typescript 372 + // ✅ Test network errors return 503 (retry later) 373 + it("returns 503 when PDS connection fails", async () => { 374 + mockPutRecord.mockRejectedValueOnce(new Error("fetch failed")); 375 + const res = await app.request("/api/topics", { 376 + method: "POST", 377 + body: JSON.stringify({ text: "Test" }) 378 + }); 379 + expect(res.status).toBe(503); // Not 500! 380 + const data = await res.json(); 381 + expect(data.error).toContain("Unable to reach your PDS"); 382 + }); 383 + 384 + // ✅ Test server errors return 500 (bug report) 385 + it("returns 500 for unexpected database errors", async () => { 386 + mockPutRecord.mockRejectedValueOnce(new Error("Database connection lost")); 387 + const res = await app.request("/api/topics", { 388 + method: "POST", 389 + body: JSON.stringify({ text: "Test" }) 390 + }); 391 + expect(res.status).toBe(500); // Not 503! 392 + const data = await res.json(); 393 + expect(data.error).not.toContain("PDS"); // Generic message for server errors 394 + }); 395 + 396 + // ✅ Test input validation returns 400 397 + it("returns 400 for malformed JSON", async () => { 398 + const res = await app.request("/api/topics", { 399 + method: "POST", 400 + headers: { "Content-Type": "application/json" }, 401 + body: "{ invalid json }" 402 + }); 403 + expect(res.status).toBe(400); 404 + const data = await res.json(); 405 + expect(data.error).toContain("Invalid JSON"); 406 + }); 407 + ``` 408 + 409 + **Error classification patterns to test:** 410 + - **400 (Bad Request):** Invalid input, missing required fields, malformed JSON 411 + - **404 (Not Found):** Resource doesn't exist (forum, post, user) 412 + - **503 (Service Unavailable):** Network errors, PDS connection failures, timeouts — user should retry 413 + - **500 (Internal Server Error):** Unexpected errors, database errors — needs bug investigation 318 414 319 415 ## Documentation & Project Tracking 320 416