diff --git a/documentation/roasts/gameService.md b/documentation/roasts/gameService.md new file mode 100644 index 0000000..871ff7a --- /dev/null +++ b/documentation/roasts/gameService.md @@ -0,0 +1,397 @@ +# `gameService.ts` — Code Review & Fixes + +--- + +## 1. Hardcoded singleton kills the abstraction + +**Problem** + +A `GameSessionStore` interface exists, an `InMemoryGameSessionStore` implements it, and then the concrete class is immediately hardcoded as a module-level singleton. The interface is decorative — nothing can inject an alternative implementation without editing this file. + +```ts +// ❌ current — store is unreachable from outside +const gameSessionStore = new InMemoryGameSessionStore(); + +export const createGameSession = async (request: GameRequest) => { ... }; +export const evaluateAnswer = async (submission: AnswerSubmission) => { ... }; +``` + +**Fix — inject the store** + +Accept the store as a parameter (or use a factory). The simplest approach that requires no framework: + +```ts +// ✅ inject the store +export const createGameSession = async ( + request: GameRequest, + store: GameSessionStore, +): Promise => { ... }; + +export const evaluateAnswer = async ( + submission: AnswerSubmission, + store: GameSessionStore, +): Promise => { ... }; +``` + +The call site (controller) owns the store instance and passes it in. Tests can pass a fresh `InMemoryGameSessionStore` per test — no mocking required, no shared state. + +```ts +// gameController.ts +const store = new InMemoryGameSessionStore(); + +// later, swap for ValKeyGameSessionStore with one line change +``` + +--- + +## 2. Sessions are never deleted — memory leak + +**Problem** + +`GameSessionStore.delete()` is defined and implemented but never called. Every session ever created stays in the Map until the process restarts. Under real traffic this is a slow memory leak; under a spike it's a fast one. + +**Fix — delete after answer, or add a TTL** + +The simplest fix: delete the session once the last question is answered. If partial completion is needed, add a TTL on creation instead. + +```ts +// ✅ option A — delete on answer +export const evaluateAnswer = async ( + submission: AnswerSubmission, + store: GameSessionStore, +): Promise => { + const session = await store.get(submission.sessionId); + if (!session) throw new NotFoundError(`Game session not found: ${submission.sessionId}`); + + const correctOptionId = session.answers.get(submission.questionId); + if (correctOptionId === undefined) throw new NotFoundError(`Question not found: ${submission.questionId}`); + + // delete answered question; delete session when all questions are answered + session.answers.delete(submission.questionId); + if (session.answers.size === 0) { + await store.delete(submission.sessionId); + } + + return { + questionId: submission.questionId, + isCorrect: submission.selectedOptionId === correctOptionId, + correctOptionId, + selectedOptionId: submission.selectedOptionId, + }; +}; +``` + +```ts +// ✅ option B — TTL in InMemoryGameSessionStore +export class InMemoryGameSessionStore implements GameSessionStore { + private sessions = new Map(); + private readonly ttlMs: number; + + constructor(ttlMs = 30 * 60 * 1000) { // 30 minutes default + this.ttlMs = ttlMs; + } + + create(sessionId: string, data: GameSessionData): Promise { + this.sessions.set(sessionId, { data, expiresAt: Date.now() + this.ttlMs }); + return Promise.resolve(); + } + + get(sessionId: string): Promise { + const entry = this.sessions.get(sessionId); + if (!entry) return Promise.resolve(null); + if (Date.now() > entry.expiresAt) { + this.sessions.delete(sessionId); + return Promise.resolve(null); + } + return Promise.resolve(entry.data); + } + + delete(sessionId: string): Promise { + this.sessions.delete(sessionId); + return Promise.resolve(); + } +} +``` + +--- + +## 3. `shuffle` is defined after it's used + +**Problem** + +`shuffle` is called inside `createGameSession` but defined below it. It works at runtime (module evaluation order), but reads as if the file was written top-to-bottom without a plan. + +```ts +// ❌ shuffle appears after the function that calls it +export const createGameSession = async (...) => { + const shuffledTexts = shuffle(optionTexts); // used here +}; + +const shuffle = (array: T[]): T[] => { ... }; // defined down here +``` + +**Fix — move helpers to the top, exports to the bottom** + +```ts +// ✅ utilities first, then exported functions +const shuffle = (array: T[]): T[] => { + const result = [...array]; + for (let i = result.length - 1; i > 0; i--) { + const j = Math.floor(Math.random() * (i + 1)); + const temp = result[i]!; + result[i] = result[j]!; + result[j] = temp; + } + return result; +}; + +export const createGameSession = async (...) => { ... }; +export const evaluateAnswer = async (...) => { ... }; +``` + +--- + +## 4. `rounds` is typed as a string + +**Problem** + +`GameRequest.rounds` is typed as `string` in `@lila/shared`, forcing the service to cast it every time: + +```ts +// ❌ why is a round count a string? +Number(request.rounds) +``` + +**Fix — fix the schema in `@lila/shared`** + +```ts +// ✅ in packages/shared +export const GameRequestSchema = z.object({ + source_language: z.string(), + target_language: z.string(), + pos: z.string(), + difficulty: z.string(), + rounds: z.coerce.number().int().min(1).max(50), // coerce handles form inputs, validates range +}); + +export type GameRequest = z.infer; +``` + +The `z.coerce.number()` handles the case where the value arrives as a string from a query param or form — Zod does the conversion at the boundary so the rest of the system never sees a string. + +--- + +## 5. `correctAnswers` is a misleading variable name + +**Problem** + +The variable holds `terms` — word pairs fetched from the database. Calling them `correctAnswers` jumps ahead semantically; they only become "correct answers" once options are constructed around them. + +```ts +// ❌ these are terms, not answers yet +const correctAnswers = await getGameTerms(...); +``` + +**Fix** + +```ts +// ✅ +const terms = await getGameTerms(...); + +// and inside the map: +terms.map(async (term) => { + const distractorTexts = await getDistractors( + term.termId, + term.targetText, + ... + ); + ... + const correctOptionId = shuffledTexts.indexOf(term.targetText); + ... +}); +``` + +--- + +## 6. Tautological test: `"distractors are never the correct answer"` + +**Problem** + +The test filters the correct answer out of the options array, then asserts the remaining items are not the correct answer. It is testing that `Array.filter` works. + +```ts +// ❌ this cannot fail +it("distractors are never the correct answer", async () => { + const distractorTexts = question.options + .map((o) => o.text) + .filter((t) => t !== correctText); // removes correct answer... + + for (const text of distractorTexts) { + expect(text).not.toBe(correctText); // ...then checks they're not the correct answer + } +}); +``` + +**What to actually test** + +The real concern is that `getDistractors` doesn't return the target word. Test that the service handles it correctly if it does: + +```ts +// ✅ test that the correct answer appears exactly once even if a distractor collides +it("correct answer appears exactly once in options even if distractor matches", async () => { + // simulate getDistractors returning the correct answer as one of the distractors + mockGetDistractors.mockResolvedValueOnce(["cane", "wrong2", "wrong3"]); + + const session = await createGameSession(validRequest, new InMemoryGameSessionStore()); + const question = session.questions[0]!; + const optionTexts = question.options.map((o) => o.text); + + // "cane" should only appear once regardless of the duplicate from getDistractors + expect(optionTexts.filter((t) => t === "cane")).toHaveLength(1); + expect(question.options).toHaveLength(4); +}); +``` + +> **Note:** the current implementation doesn't actually handle this case — a duplicate distractor would produce a 4-option list where the correct answer appears twice and one distractor slot is wasted. Worth fixing in `createGameSession` alongside the test. + +--- + +## 7. Store not reset between tests + +**Problem** + +`beforeEach` calls `vi.clearAllMocks()` which resets mock functions, but the `gameSessionStore` module-level singleton is never cleared. Ghost sessions from earlier tests persist for the entire test run. + +It doesn't bite today because each session gets a unique UUID and tests don't share IDs — but it's one non-UUID lookup away from a very confusing afternoon. + +**Fix — a consequence of fixing issue #1** + +Once the store is injected rather than module-level, each test creates its own instance: + +```ts +// ✅ no shared state, no ghost sessions +describe("evaluateAnswer", () => { + it("returns isCorrect: true for correct option", async () => { + const store = new InMemoryGameSessionStore(); + const session = await createGameSession(validRequest, store); + ... + const result = await evaluateAnswer({ ... }, store); + ... + }); +}); +``` + +No `beforeEach` cleanup needed — the store simply doesn't outlive the test that created it. + +--- + +## 8. No answer replay protection + +**Problem** + +`evaluateAnswer` can be called multiple times with the same `questionId`. The +service will evaluate it every time. In multiplayer this could be abused to +farm points or desync state. + +**Fix — delete the question from the answer key after first evaluation** + +```ts +// ✅ inside evaluateAnswer, after retrieving correctOptionId +session.answers.delete(submission.questionId); + +if (submission.selectedOptionId !== correctOptionId) { + // already removed — can't retry +} +``` + +Once the question key is deleted, a second submission hits the +`correctOptionId === undefined` branch and throws `NotFoundError`. One shot +per question. + +--- + +## 9. No ownership check in `evaluateAnswer` + +**Problem** + +The service accepts any `sessionId` without verifying it belongs to the +requesting user. If auth middleware doesn't tie sessions to users at a higher +layer, Alice can submit answers for Bob's session by guessing or intercepting +his `sessionId`. + +**Fix — store `userId` alongside the session and assert it on retrieval** + +```ts +// GameSessionStore.ts +export type GameSessionData = { + answers: Map; + userId: string; +}; + +// evaluateAnswer +const session = await store.get(submission.sessionId); + +if (!session) throw new NotFoundError(`Game session not found`); +if (session.userId !== requestingUserId) throw new NotFoundError(`Game session not found`); +// ^^^ same error — don't confirm the session exists to the wrong user +``` + +Pass `requestingUserId` in from the controller, where it's already available +via auth middleware. + +--- + +## 10. No test for empty `getGameTerms` result + +**Problem** + +If the database returns zero terms (no words match the difficulty/language/pos +filter), `createGameSession` happily returns a session with an empty +`questions` array. The frontend receives it, tries to render question 1, and +crashes. The user sees nothing useful. + +**Fix — guard in the service and add a test** + +```ts +// ✅ inside createGameSession, after fetching terms +if (terms.length === 0) { + throw new AppError("No terms found for the given filters", 404); +} +``` + +```ts +// ✅ test +it("throws when getGameTerms returns no terms", async () => { + mockGetGameTerms.mockResolvedValue([]); + + await expect(createGameSession(validRequest, new InMemoryGameSessionStore())) + .rejects.toThrow("No terms found"); +}); +``` + +--- + +## 11. No test for `getDistractors` rejection + +**Problem** + +`createGameSession` uses `Promise.all` over the terms array. If +`getDistractors` rejects for any single term, the entire `Promise.all` rejects +— no session is created, no partial recovery, the user gets a 500 with +"connection refused" leaking through. + +**Fix — test the failure path and consider a fallback** + +```ts +// ✅ test +it("propagates getDistractors failure", async () => { + mockGetDistractors.mockRejectedValue(new Error("db timeout")); + + await expect(createGameSession(validRequest, new InMemoryGameSessionStore())) + .rejects.toThrow("db timeout"); +}); +``` + +For resilience, consider catching per-term distractor failures and falling back +to random terms from the already-fetched set rather than collapsing the whole +session.