359 lines
11 KiB
Markdown
359 lines
11 KiB
Markdown
# `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<GameSession> => { ... };
|
|
|
|
export const evaluateAnswer = async (
|
|
submission: AnswerSubmission,
|
|
store: GameSessionStore,
|
|
): Promise<AnswerResult> => { ... };
|
|
```
|
|
|
|
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<AnswerResult> => {
|
|
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<string, { data: GameSessionData; expiresAt: number }>();
|
|
private readonly ttlMs: number;
|
|
|
|
constructor(ttlMs = 30 * 60 * 1000) { // 30 minutes default
|
|
this.ttlMs = ttlMs;
|
|
}
|
|
|
|
create(sessionId: string, data: GameSessionData): Promise<void> {
|
|
this.sessions.set(sessionId, { data, expiresAt: Date.now() + this.ttlMs });
|
|
return Promise.resolve();
|
|
}
|
|
|
|
get(sessionId: string): Promise<GameSessionData | null> {
|
|
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<void> {
|
|
this.sessions.delete(sessionId);
|
|
return Promise.resolve();
|
|
}
|
|
}
|
|
```
|
|
|
|
---
|
|
|
|
|
|
|
|
**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<typeof GameRequestSchema>;
|
|
```
|
|
|
|
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.
|
|
|
|
---
|
|
|
|
**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<string, number>;
|
|
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.
|