# `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 (...) => { ... }; ``` --- **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.