From 4f47e18ad96a9d1daf0c89f237d8ac81679ff27a Mon Sep 17 00:00:00 2001 From: lila Date: Thu, 30 Apr 2026 01:20:12 +0200 Subject: [PATCH] formatting --- apps/api/src/app.ts | 2 +- .../src/controllers/gameController.test.ts | 2 +- apps/api/src/errors/AppError.ts | 2 +- apps/api/src/routes/gameRouter.ts | 2 +- documentation/roasts/gameService.md | 218 ++++++++---------- documentation/tickets/t00006.md | 13 +- documentation/tickets/t00008.md | 4 +- 7 files changed, 119 insertions(+), 124 deletions(-) diff --git a/apps/api/src/app.ts b/apps/api/src/app.ts index 47c51e6..ad9f509 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 168d2d1..0351c2a 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 7805f3e..bc5a92a 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/routes/gameRouter.ts b/apps/api/src/routes/gameRouter.ts index 9e29a5d..a40622d 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/documentation/roasts/gameService.md b/documentation/roasts/gameService.md index 73283de..ac3d2ed 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,64 +46,55 @@ 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); @@ -111,10 +102,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: @@ -127,15 +118,14 @@ 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)]; @@ -157,30 +147,29 @@ 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: @@ -199,65 +188,64 @@ 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? @@ -267,16 +255,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. @@ -291,37 +279,35 @@ 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 @@ -333,16 +319,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. \ No newline at end of file +Action: Add a ticket to documentation/tickets/t00009.md now. diff --git a/documentation/tickets/t00006.md b/documentation/tickets/t00006.md index 8e777f0..edc5e25 100644 --- a/documentation/tickets/t00006.md +++ b/documentation/tickets/t00006.md @@ -84,7 +84,10 @@ 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: @@ -100,7 +103,11 @@ 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: @@ -114,7 +121,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 b171abc..670f610 100644 --- a/documentation/tickets/t00008.md +++ b/documentation/tickets/t00008.md @@ -27,7 +27,9 @@ 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)]; ```