diff --git a/.forgejo/workflows/deploy.yml b/.forgejo/workflows/deploy.yml index dc34ae3..9295dae 100644 --- a/.forgejo/workflows/deploy.yml +++ b/.forgejo/workflows/deploy.yml @@ -29,7 +29,6 @@ jobs: build-and-deploy: runs-on: docker - needs: quality steps: - name: Install tools run: apt-get update && apt-get install -y docker.io openssh-client diff --git a/.prettierignore b/.prettierignore index 773c5d1..9699559 100644 --- a/.prettierignore +++ b/.prettierignore @@ -18,5 +18,3 @@ coverage/ pnpm-lock.yaml routeTree.gen.ts - -.pnpm-store/ diff --git a/apps/api/package.json b/apps/api/package.json index 1a7cdc3..2e2944f 100644 --- a/apps/api/package.json +++ b/apps/api/package.json @@ -7,8 +7,7 @@ "dev": "pnpm --filter shared build && pnpm --filter db build && tsx watch src/server.ts", "build": "tsc", "start": "node dist/src/server.js", - "test": "vitest", - "typecheck": "tsc --noEmit" + "test": "vitest" }, "dependencies": { "@lila/db": "workspace:*", diff --git a/apps/api/src/app.ts b/apps/api/src/app.ts index ad9f509..47c51e6 100644 --- a/apps/api/src/app.ts +++ b/apps/api/src/app.ts @@ -27,7 +27,7 @@ export function createApp() { const store = new InMemoryGameSessionStore(); app.use("/api/v1", createApiRouter(store)); - + app.use(errorHandler); return app; diff --git a/apps/api/src/controllers/gameController.test.ts b/apps/api/src/controllers/gameController.test.ts index 0351c2a..168d2d1 100644 --- a/apps/api/src/controllers/gameController.test.ts +++ b/apps/api/src/controllers/gameController.test.ts @@ -197,7 +197,7 @@ describe("POST /api/v1/game/answer", () => { expect(body.success).toBe(false); expect(body.error).toContain("Question already answered"); }); - + it("returns 400 when a field has an invalid value", async () => { const res = await request(app) .post("/api/v1/game/start") diff --git a/apps/api/src/errors/AppError.ts b/apps/api/src/errors/AppError.ts index bc5a92a..7805f3e 100644 --- a/apps/api/src/errors/AppError.ts +++ b/apps/api/src/errors/AppError.ts @@ -30,4 +30,4 @@ export class UnprocessableEntityError extends AppError { constructor(message: string) { super(message, 422); } -} +} \ No newline at end of file diff --git a/apps/api/src/lib/auth.ts b/apps/api/src/lib/auth.ts index fe41e35..601708e 100644 --- a/apps/api/src/lib/auth.ts +++ b/apps/api/src/lib/auth.ts @@ -4,6 +4,7 @@ import { Resend } from "resend"; import { db } from "@lila/db"; import * as schema from "@lila/db/schema"; +const resend = new Resend(process.env["RESEND_API_KEY"]); const emailFrom = process.env["EMAIL_FROM"] ?? "noreply@lilastudy.com"; export const auth = betterAuth({ @@ -29,7 +30,6 @@ export const auth = betterAuth({ user: { email: string }; url: string; }) => { - const resend = new Resend(process.env["RESEND_API_KEY"]); await resend.emails.send({ from: emailFrom, to: user.email, @@ -48,7 +48,6 @@ export const auth = betterAuth({ user: { email: string }; url: string; }) => { - const resend = new Resend(process.env["RESEND_API_KEY"]); await resend.emails.send({ from: emailFrom, to: user.email, diff --git a/apps/api/src/routes/gameRouter.ts b/apps/api/src/routes/gameRouter.ts index a40622d..9e29a5d 100644 --- a/apps/api/src/routes/gameRouter.ts +++ b/apps/api/src/routes/gameRouter.ts @@ -14,6 +14,6 @@ export const createGameRouter = (store: GameSessionStore): Router => { router.post("/start", controller.createGame as express.RequestHandler); router.post("/answer", controller.submitAnswer as express.RequestHandler); - + return router; }; diff --git a/apps/web/package.json b/apps/web/package.json index 8068c8f..b458eb3 100644 --- a/apps/web/package.json +++ b/apps/web/package.json @@ -6,8 +6,7 @@ "scripts": { "dev": "vite", "build": "tsc -b && vite build", - "preview": "vite preview", - "typecheck": "tsc --noEmit" + "preview": "vite preview" }, "dependencies": { "@lila/shared": "workspace:*", diff --git a/documentation/backlog.md b/documentation/backlog.md index 35a5f05..6a91e2d 100644 --- a/documentation/backlog.md +++ b/documentation/backlog.md @@ -90,6 +90,9 @@ Directionally right, timing is unclear. Revisit when the next/now work is done. - **Resolve eslint peer dependency warning** `[debt]` `eslint-plugin-react-hooks 7.0.1` expects `eslint ^3.0.0โ€“^9.0.0` but found `10.0.3`. Low impact but worth cleaning up when nearby. +- **husky + lint-staged** `[debt]` + Set up husky and lint-staged to run linting and formatting checks before every commit. Prevents CI failures from formatting or lint issues that slipped through locally. + - **OpenAPI documentation for REST endpoints** `[feature]` Document the API surface using OpenAPI/Swagger. Covers all REST endpoints with request/response shapes. Useful groundwork for the admin dashboard and any future contributors. @@ -102,7 +105,6 @@ Directionally right, timing is unclear. Revisit when the next/now work is done. Shipped milestones, newest first. -- **04 - 2026 - husky + lint-staged + CI quality gate** - Pre-commit formatting, pre-push tests, and CI lint/typecheck/test gate before every deploy. - **04 - 2026 - t00001 - Docker credential helper** - **04 - 2026 - Pin dependencies in package.json** - Unpinned deps in a CI/CD pipeline are a real risk. - **04 - 2026 - React error boundaries** - Catch and display runtime errors gracefully instead of crashing the entire app. diff --git a/documentation/roasts/gameService.md b/documentation/roasts/gameService.md index ac3d2ed..73283de 100644 --- a/documentation/roasts/gameService.md +++ b/documentation/roasts/gameService.md @@ -1,11 +1,11 @@ # ๐Ÿ”ฅ GameService Roast: `apps/api/src/services/gameService.ts` -> _"It works on my machine" is not a scalability strategy._ +> *"It works on my machine" is not a scalability strategy.* **Project:** lila โ€” Vocabulary Trainer **File Roasted:** `gameService.ts` **Date:** $(date) -**Roaster:** Qwen3.6 +**Roaster:** Qwen3.6 --- @@ -28,8 +28,8 @@ **Location:** `gameService.ts:45-58` + `InMemoryGameSessionStore.ts:update()` // Current flow (VULNERABLE): -const session = await store.get(submission.sessionId); // READ -const updatedAnswers = new Map(session.answers); // MODIFY (local copy) +const session = await store.get(submission.sessionId); // READ +const updatedAnswers = new Map(session.answers); // MODIFY (local copy) updatedAnswers.delete(submission.questionId); await store.update(submission.sessionId, { answers: updatedAnswers }); // WRITE @@ -46,55 +46,64 @@ Fix Options: // Option A: Add atomic operation to store interface interface GameSessionStore { -deleteAnswer(sessionId: string, questionId: string): Promise; + deleteAnswer(sessionId: string, questionId: string): Promise; } // Option B: Use Valkey Lua script for atomic read-modify-write // Option C: Optimistic locking with version numbers -Priority: ๐Ÿ”ด CRITICAL โ€” Data integrity issue 2. N+1 Query: Database Performance Bomb +Priority: ๐Ÿ”ด CRITICAL โ€” Data integrity issue +2. N+1 Query: Database Performance Bomb Location: gameService.ts:24-26 + termModel.ts:getDistractors() // For each of N terms, we call getDistractors(): const questions: GameQuestion[] = await Promise.all( -terms.map(async (term) => { -const distractorTexts = await getDistractors(term.termId, ...); // ๐Ÿšฉ N queries! -}) + terms.map(async (term) => { + const distractorTexts = await getDistractors(term.termId, ...); // ๐Ÿšฉ N queries! + }) ); Impact Analysis: Rounds + DB Queries + At 50 concurrent users 3 + 1 + 3 = 4 + 200 queries/min 10 + 1 + 10 = 11 + 550 queries/min 20 + 1 + 20 = 21 + 1,050 queries/min Each getDistractors() runs: -SELECT text FROM terms -JOIN translations ON ... -WHERE pos = $1 AND difficulty = $2 AND term_id != $3 AND text != $4 +SELECT text FROM terms +JOIN translations ON ... +WHERE pos = $1 AND difficulty = $2 AND term_id != $3 AND text != $4 ORDER BY RANDOM() LIMIT 6 Fix: Batch Fetch Distractors // Fetch all distractors in ONE query const allDistractors = await db -.select({ termId: terms.id, text: translations.text }) -.from(terms) -.innerJoin(translations, /_ ... _/) -.where(and( -eq(terms.pos, pos), -eq(translations.difficulty, difficulty), -inArray(terms.id, termIds), // Batch! -)) -.limit(DISTRACTOR_FETCH_COUNT \* termIds.length); + .select({ termId: terms.id, text: translations.text }) + .from(terms) + .innerJoin(translations, /* ... */) + .where(and( + eq(terms.pos, pos), + eq(translations.difficulty, difficulty), + inArray(terms.id, termIds), // Batch! + )) + .limit(DISTRACTOR_FETCH_COUNT * termIds.length); // Group by termId in JS, then slice to 3 unique distractors per term const distractorsByTerm = groupByTermId(allDistractors); @@ -102,10 +111,10 @@ const distractorsByTerm = groupByTermId(allDistractors); Priority: ๐Ÿ”ด CRITICAL โ€” Performance/scalability issue 3. Error Handling Inconsistency - Location: gameService.ts:33-36 +Location: gameService.ts:33-36 if (uniqueDistractors.length < 3) { -throw new Error(`Not enough unique distractors for term: ${term.targetText}`); // ๐Ÿšฉ + throw new Error(`Not enough unique distractors for term: ${term.targetText}`); // ๐Ÿšฉ } Problem: Raw Error bypasses your errorHandler middleware: @@ -118,14 +127,15 @@ Fix: import { UnprocessableEntityError } from "../errors/AppError.js"; if (uniqueDistractors.length < 3) { -logger.warn({ termId: term.termId, uniqueCount: uniqueDistractors.length }, -"insufficient_distractors"); -throw new UnprocessableEntityError( -`Not enough unique distractors for term: ${term.targetText}` -); + logger.warn({ termId: term.termId, uniqueCount: uniqueDistractors.length }, + "insufficient_distractors"); + throw new UnprocessableEntityError( + `Not enough unique distractors for term: ${term.targetText}` + ); } Priority: ๐ŸŸก HIGH โ€” Observability & UX issue -โš ๏ธ High-Severity Smells 4. Code Duplication: Singleplayer vs Multiplayer +โš ๏ธ High-Severity Smells +4. Code Duplication: Singleplayer vs Multiplayer Compare: gameService.ts vs multiplayerGameService.ts // gameService.ts const optionTexts = [term.targetText, ...uniqueDistractors.slice(0, 3)]; @@ -147,29 +157,30 @@ Fix: Extract pure function to @lila/shared or new @lila/game-logic: // packages/shared/src/game-logic.ts export const buildQuestionOptions = ( -correctAnswer: string, -distractors: string[], -optionCount: number = 4 + correctAnswer: string, + distractors: string[], + optionCount: number = 4 ): { options: AnswerOption[]; correctOptionId: number } => { -const uniqueDistractors = [...new Set(distractors.filter(d => d !== correctAnswer))]; -const optionTexts = [correctAnswer, ...uniqueDistractors.slice(0, optionCount - 1)]; -const shuffled = shuffleSecure(optionTexts); -const correctOptionId = shuffled.indexOf(correctAnswer); - -return { -options: shuffled.map((text, idx) => ({ optionId: idx, text })), -correctOptionId -}; + const uniqueDistractors = [...new Set(distractors.filter(d => d !== correctAnswer))]; + const optionTexts = [correctAnswer, ...uniqueDistractors.slice(0, optionCount - 1)]; + const shuffled = shuffleSecure(optionTexts); + const correctOptionId = shuffled.indexOf(correctAnswer); + + return { + options: shuffled.map((text, idx) => ({ optionId: idx, text })), + correctOptionId + }; }; -Priority: ๐ŸŸก HIGH โ€” Maintainability issue 5. Shuffle Bias: Math.random() Trap +Priority: ๐ŸŸก HIGH โ€” Maintainability issue +5. Shuffle Bias: Math.random() Trap Location: utils.ts:shuffleArray() + multiplayerGameService.ts:shuffle() export const shuffleArray = (array: T[]): T[] => { -for (let i = result.length - 1; i > 0; i--) { -const j = Math.floor(Math.random() \* (i + 1)); // ๐Ÿšฉ Modulo bias + non-crypto RNG -// ... -} + for (let i = result.length - 1; i > 0; i--) { + const j = Math.floor(Math.random() * (i + 1)); // ๐Ÿšฉ Modulo bias + non-crypto RNG + // ... + } }; The Math: @@ -188,64 +199,65 @@ Fix (if needed): import { randomBytes } from "crypto"; const shuffleSecure = (array: T[]): T[] => { -const result = [...array]; -for (let i = result.length - 1; i > 0; i--) { -// Use crypto.getRandomValues for better randomness -const rand = randomBytes(4).readUInt32LE(0); -const j = rand % (i + 1); -[result[i], result[j]] = [result[j], result[i]]; -} -return result; + const result = [...array]; + for (let i = result.length - 1; i > 0; i--) { + // Use crypto.getRandomValues for better randomness + const rand = randomBytes(4).readUInt32LE(0); + const j = rand % (i + 1); + [result[i], result[j]] = [result[j], result[i]]; + } + return result; }; Priority: ๐ŸŸข LOW โ€” Document tradeoff and move on for now -6. Test Coverage Gaps - File: gameService.test.ts - โœ… Well Tested: +6. Test Coverage Gaps +File: gameService.test.ts +โœ… Well Tested: - Happy path: session creation, answer evaluation - Edge cases: duplicate distractors, empty results, invalid inputs - Error propagation from DB layer + Happy path: session creation, answer evaluation + Edge cases: duplicate distractors, empty results, invalid inputs + Error propagation from DB layer โŒ Missing Tests: // 1. Concurrency test (race condition) it("rejects duplicate answers for same question under concurrent load", async () => { -const session = await createGameSession(validRequest, store, "user-1"); -const question = session.questions[0]!; - -// Submit two answers simultaneously -const [result1, result2] = await Promise.allSettled([ -evaluateAnswer({ sessionId, questionId, selectedOptionId: 0 }, store, "user-1"), -evaluateAnswer({ sessionId, questionId, selectedOptionId: 1 }, store, "user-1"), -]); - -// Exactly one should succeed, one should throw ConflictError -expect([result1, result2].filter(r => r.status === "fulfilled")).toHaveLength(1); + const session = await createGameSession(validRequest, store, "user-1"); + const question = session.questions[0]!; + + // Submit two answers simultaneously + const [result1, result2] = await Promise.allSettled([ + evaluateAnswer({ sessionId, questionId, selectedOptionId: 0 }, store, "user-1"), + evaluateAnswer({ sessionId, questionId, selectedOptionId: 1 }, store, "user-1"), + ]); + + // Exactly one should succeed, one should throw ConflictError + expect([result1, result2].filter(r => r.status === "fulfilled")).toHaveLength(1); }); // 2. TTL expiration test it("deletes session after TTL expires", async () => { -vi.useFakeTimers(); -const session = await createGameSession(validRequest, store, "user-1"); - -vi.advanceTimersByTime(31 _ 60 _ 1000); // 31 minutes - -await expect(store.get(session.sessionId)).resolves.toBeNull(); + vi.useFakeTimers(); + const session = await createGameSession(validRequest, store, "user-1"); + + vi.advanceTimersByTime(31 * 60 * 1000); // 31 minutes + + await expect(store.get(session.sessionId)).resolves.toBeNull(); }); // 3. Distractor fallback strategy test it("uses fallback when <3 unique distractors available", async () => { -mockGetDistractors.mockResolvedValue(["same", "same", "same", "same"]); -// Should either: (a) fetch from broader pool, or (b) reduce rounds gracefully + mockGetDistractors.mockResolvedValue(["same", "same", "same", "same"]); + // Should either: (a) fetch from broader pool, or (b) reduce rounds gracefully }); Priority: ๐ŸŸก HIGH โ€” Prevents regression on critical fixes -๐Ÿงผ Code Quality Nitpicks 7. Magic Numbers +๐Ÿงผ Code Quality Nitpicks +7. Magic Numbers // gameService.ts:52 -await store.create(sessionId, {...}, 30 _ 60 _ 1000); // What is this? +await store.create(sessionId, {...}, 30 * 60 * 1000); // What is this? // termModel.ts:65 .limit(count); // count=6, but why? @@ -255,16 +267,16 @@ optionId: z.number().int().min(0).max(3), // Why 4 options? Fix: Centralize in @lila/shared/constants.ts: -export const GAME*SESSION_TTL_MS = 30 * 60 \_ 1000; +export const GAME_SESSION_TTL_MS = 30 * 60 * 1000; export const DISTRACTOR_FETCH_COUNT = 6; export const GAME_OPTION_COUNT = 4; export const MIN_UNIQUE_DISTRACTORS = 3; 8. Mutable Reference Leakage - Location: InMemoryGameSessionStore.ts:get() +Location: InMemoryGameSessionStore.ts:get() get(sessionId: string): Promise { -return Promise.resolve(entry.data); // ๐Ÿšฉ Returns mutable reference to internal state + return Promise.resolve(entry.data); // ๐Ÿšฉ Returns mutable reference to internal state } Risk: Any code that does session.answers.delete(...) mutates the store's internal Map directly. @@ -279,35 +291,37 @@ return Promise.resolve(entry.data as Readonly); // Option C: Use immutable data structures (overkill for now) 9. Zero Observability - Problem: No logging, no metrics. You're flying blind in production. - Minimal Fix (5 minutes): +Problem: No logging, no metrics. You're flying blind in production. +Minimal Fix (5 minutes): + + // apps/api/src/lib/logger.ts import pino from "pino"; -export const logger = pino({ -level: process.env.LOG_LEVEL || "info", -transport: process.env.NODE_ENV === "production" -? { target: "pino-pretty" } -: undefined +export const logger = pino({ + level: process.env.LOG_LEVEL || "info", + transport: process.env.NODE_ENV === "production" + ? { target: "pino-pretty" } + : undefined }); // In gameService.ts: import { logger } from "../lib/logger.js"; logger.info( -{ userId, sourceLang, targetLang, termCount: terms.length }, -"game_session_created" + { userId, sourceLang, targetLang, termCount: terms.length }, + "game_session_created" ); logger.debug( -{ sessionId, questionId, isCorrect, responseTimeMs }, -"answer_evaluated" + { sessionId, questionId, isCorrect, responseTimeMs }, + "answer_evaluated" ); Bonus: Export a Prometheus histogram for game_service_duration_seconds. 10. ORDER BY RANDOM() Time Bomb - Location: termModel.ts:getGameTerms() + getDistractors() +Location: termModel.ts:getGameTerms() + getDistractors() .orderBy(sql`RANDOM()`) // ๐Ÿšฉ Fine for 10k rows, slow for 1M @@ -319,16 +333,16 @@ Reality Check: "Post-MVP" never comes without a ticket. Fix Options: -- Option A: Pre-computed random_seed column (updated nightly) -WHERE ... AND random_seed >= random() -ORDER BY random_seed +WHERE ... AND random_seed >= random() +ORDER BY random_seed LIMIT $1 -- Option B: TABLESAMPLE for approximate sampling (Postgres 9.5+) -FROM terms TABLESAMPLE SYSTEM(10) -WHERE ... +FROM terms TABLESAMPLE SYSTEM(10) +WHERE ... LIMIT $1 -- Option C: Random offset (simple, but still scans) -OFFSET floor(random() _ (SELECT count(_) FROM terms WHERE ...)) +OFFSET floor(random() * (SELECT count(*) FROM terms WHERE ...)) -Action: Add a ticket to documentation/tickets/t00009.md now. +Action: Add a ticket to documentation/tickets/t00009.md now. \ No newline at end of file diff --git a/documentation/tickets/t00006.md b/documentation/tickets/t00006.md index edc5e25..8e777f0 100644 --- a/documentation/tickets/t00006.md +++ b/documentation/tickets/t00006.md @@ -84,10 +84,7 @@ Rejected because: for user-owned resources identified by opaque IDs, confirming 2. `GameSessionStore.ts` โ€” add `userId` to `GameSessionData`: ```ts - export type GameSessionData = { - answers: Map; - userId: string; - }; + export type GameSessionData = { answers: Map; userId: string }; ``` 3. `gameService.ts` โ€” add `userId` to both function signatures: @@ -103,11 +100,7 @@ Rejected because: for user-owned resources identified by opaque IDs, confirming Store it on create: ```ts - await store.create( - sessionId, - { answers: answerKey, userId }, - 30 * 60 * 1000, - ); + await store.create(sessionId, { answers: answerKey, userId }, 30 * 60 * 1000); ``` Assert on evaluate: @@ -121,7 +114,7 @@ Rejected because: for user-owned resources identified by opaque IDs, confirming 4. `gameController.ts` โ€” extract from authenticated request: ```ts - req.session.user.id; + req.session.user.id ``` 5. `gameRouter.ts` โ€” cast at registration: diff --git a/documentation/tickets/t00008.md b/documentation/tickets/t00008.md index 670f610..b171abc 100644 --- a/documentation/tickets/t00008.md +++ b/documentation/tickets/t00008.md @@ -27,9 +27,7 @@ Not chosen for this ticket โ€” the database query is in `@lila/db` and is a sepa - Filter distractors against the correct answer before building options: ```ts - const uniqueDistractors = distractorTexts.filter( - (t) => t !== term.targetText, - ); + const uniqueDistractors = distractorTexts.filter((t) => t !== term.targetText); const optionTexts = [term.targetText, ...uniqueDistractors.slice(0, 3)]; ``` diff --git a/package.json b/package.json index 8e9a1f6..6a30e1d 100644 --- a/package.json +++ b/package.json @@ -6,13 +6,13 @@ "scripts": { "build": "pnpm --filter @lila/shared build && pnpm --filter @lila/db build && pnpm --filter @lila/api build", "dev": "concurrently --names \"api,web\" -c \"magenta.bold,green.bold\" \"pnpm --filter @lila/api dev\" \"pnpm --filter @lila/web dev\"", - "prepare": "husky || true", + "prepare": "husky", "test": "vitest", "test:run": "vitest run", "lint": "eslint .", "format": "prettier --write .", "format:check": "prettier --check .", - "typecheck": "pnpm -r typecheck" + "typecheck": "tsc --build --noEmit" }, "lint-staged": { "**/*.{ts,tsx}": [ diff --git a/packages/db/package.json b/packages/db/package.json index ee17556..914e989 100644 --- a/packages/db/package.json +++ b/packages/db/package.json @@ -6,8 +6,7 @@ "scripts": { "build": "rm -rf dist && tsc", "generate": "drizzle-kit generate", - "migrate": "drizzle-kit migrate", - "typecheck": "tsc --noEmit" + "migrate": "drizzle-kit migrate" }, "dependencies": { "@lila/shared": "workspace:*", diff --git a/packages/shared/package.json b/packages/shared/package.json index c46ab0a..bf6aa8a 100644 --- a/packages/shared/package.json +++ b/packages/shared/package.json @@ -4,8 +4,7 @@ "private": true, "type": "module", "scripts": { - "build": "tsc", - "typecheck": "tsc --noEmit" + "build": "tsc" }, "exports": { ".": "./dist/src/index.js"