Refactor game services and API endpoints with security improvements - #18

src/lib/services/scoreService.ts (1)

207-208: The as GameID cast is redundant since GameType is already aliased to GameID.

The type alias export type GameType = GameID in src/lib/server/db/types.ts means these types are identical—no divergence is possible. The cast on line 207 can be removed for clarity.

src/lib/games/word-scramble/init.ts (1)

21-23: Return type Promise<any> weakens type safety.

The function returns a well-defined object shape. Consider defining an explicit interface (consistent with how other games like SnakeGameInstance, Game2048Instance, PathNavigatorGameInstance define their return types).

Suggested approach

+export interface WordScrambleGameInstance {
+    restart: () => void | undefined
+    getState: () => GameState | undefined
+    endGame: () => void | undefined
+    callbacks: GameCallbacks
+    cleanup: () => void
+}
+
 export async function initWordScrambleGame(externalCallbacks?: {
     onGameOver?: (finalScore: number, stats: GameStats) => void
-}): Promise<any> {
+}): Promise<WordScrambleGameInstance | undefined> {

src/lib/games/sudoku/init.ts (1)

29-29: Pre-existing: innerHTML = '' used to clear children.

Lines 29 and 239 use innerHTML = '' to clear container contents. As per coding guidelines, children should be cleared safely without using innerHTML. This is a pre-existing pattern not introduced by this PR, but worth addressing in a follow-up.

Safe alternative

-    container.innerHTML = '' // Clear existing content
+    while (container.firstChild) {
+        container.removeChild(container.firstChild)
+    }

Same for gameElement.innerHTML = '' in renderGame.

As per coding guidelines: "clear children safely without using innerHTML to manipulate parent elements".

Also applies to: 239-239

src/lib/games/quick-math/init.ts (1)

6-7: Module-level mutable state (gameInstance, gameCallbacks) can cause issues with multiple initializations.

If initQuickMathGame is called more than once (e.g., on navigation in an SPA), the previous instance is silently overwritten without cleanup. Consider moving these into the function scope or adding a guard that cleans up any prior instance.

src/lib/games/tetris/init.ts (2)

52-52: stats: any weakens type safety in the game-over callback.

Same concern as the memory-matrix file. Consider typing stats with the actual stats shape to get compile-time checks. As per coding guidelines, TypeScript files should use type annotations for function parameters.

---

30-267: Missing debug object exposure on window — inconsistent with other games.

The memory-matrix init (and per coding guidelines for src/lib/games/**/{game,init}.ts) exposes a debug object like window.memoryMatrixGame. This file doesn't expose window.tetrisGame. Consider adding it for consistency and runtime debugging convenience.

Suggested addition at the end of the file

// Export for debugging
if (typeof window !== 'undefined') {
    ;(window as any).tetrisGame = {
        getState: () => enhancedState,
        cleanup: () => { /* call the cleanup from the returned instance */ },
    }
}

Note: since enhancedState is function-scoped, you'd need to decide whether to hoist it or return the debug exposure from initTetrisGame. An alternative is to attach it inside the function body before the return statement.

As per coding guidelines: "Use game debug objects window.gameNameGame.getGame() for development inspection and debugging."

src/pages/api/user/update-streaks.ts (1)

37-38: Missing error logging in catch block.

Other endpoints in this PR (e.g., login-rewards/index.ts Line 24, login-rewards/claim.ts Line 29) log the error with console.error before returning errorResponse. This catch silently discards the error, which will make debugging streak update failures harder.

Suggested fix

-    } catch (_e) {
-        return errorResponse('Internal server error')
+    } catch (error) {
+        console.error('[POST /api/user/update-streaks] Error:', error)
+        return errorResponse('Internal server error')
     }

src/pages/api/scores/history.ts (1)

31-33: Consider adding error logging for consistency.

Similar to the observation in update-streaks.ts, this catch block silently discards the error. Other endpoints in this PR log errors before returning (e.g., settings/index.ts Line 108). Adding a console.error here would aid in diagnosing production issues.

src/pages/api/leaderboard.ts (1)

65-67: Consider adding error logging for consistency.

Same observation as other endpoints — the catch block silently discards the error. A console.error would help with debugging.

src/lib/server/db/queries.ts (1)

1028-1056: processed count is misleading — it includes users without user_stats rows.

processed returns allUserIds.length (all users), but incremented and reset only include users with existing user_stats rows. This means processed != incremented + reset for users without stats. Consider renaming or adjusting:

Suggested clarification

     return {
-        processed: allUserIds.length,
+        processed: existingStatsUserIds.size,
         incremented: activeUserIds.length,
         reset: inactiveUserIds.length,
     }

src/pages/api/user/profile.test.ts (1)

6-9: Consider mocking validateBody to isolate the endpoint logic from validation internals.

Currently these tests exercise both the profile endpoint and the full Zod schema validation. This is fine for integration-style tests, but any schema change will ripple into these tests. If this is intentional, no action needed — just noting for awareness.

src/pages/api/user/profile.ts (1)

49-51: Likely unreachable: the Zod refine already ensures at least one field is provided.

The profileUpdateSchema refine rejects payloads where all of name, displayName, and username are undefined. After validation succeeds, at least one field will be non-undefined, so Object.keys(updates).length === 0 should never be true. This is harmless as a defensive guard, but the error message 'No valid fields to update' differs from the schema's 'At least one field must be provided', which could confuse debugging.

src/pages/api/scores/best.ts (1)

31-39: Minor: the getGameById check (line 36-38) is redundant after isGameId passes.

If the GameID enum values are guaranteed to map 1:1 with the GAMES array, getGameById will always find a match after isGameId succeeds. This is fine as defense-in-depth, but if you want to simplify:

Optional simplification

         if (!isGameId(gameId)) {
             return badRequestResponse('Invalid game ID')
         }

-        // Verify game exists
-        const game = getGameById(gameId)
-        if (!game) {
-            return badRequestResponse('Invalid game ID')
-        }
-
         const bestScore = await getUserBestScore(session.user.id, gameId)

src/pages/api/scores/best.test.ts (1)

14-27: GameID mock duplicates enum values — consider importing and spreading the real enum.

If the GameID enum changes, this mock will silently drift. However, since this is a unit test with an intentionally controlled mock, this is acceptable.

src/lib/challenges.test.ts (2)

125-125: Prefer using the actual GameID enum instead of as any.

Per the AI summary, GameID is a shared enum used across tests. Using it here would keep the test data type-safe and consistent with the pattern in other test files.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/lib/challenges.test.ts` at line 125, Replace the unsafe cast "gameId:
'tetris' as any" with the proper GameID enum value to keep the test type-safe
and consistent; locate the test object using the gameId field in
src/lib/challenges.test.ts and change it to use GameID.Tetris (or the exact enum
member name for the tetris id) so the test references the shared GameID enum
instead of using as any.

---

73-140: Consider extracting a helper to reduce the repeated pool-mutation boilerplate.

The save → splice → try/finally → restore pattern is duplicated three times. A small helper would reduce duplication and the risk of forgetting cleanup in future tests:

♻️ Suggested helper

function withChallengePool(replacement: typeof CHALLENGE_POOL, fn: () => void) {
    const original = [...CHALLENGE_POOL]
    CHALLENGE_POOL.splice(0, CHALLENGE_POOL.length, ...replacement)
    try {
        fn()
    } finally {
        CHALLENGE_POOL.splice(0, CHALLENGE_POOL.length, ...original)
    }
}

Then each test becomes e.g.:

it('should return empty list when challenge pool is empty', () => {
    withChallengePool([], () => {
        const challenges = generateDailyChallenges(new Date('2025-09-01'))
        expect(challenges).toEqual([])
    })
})

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/lib/challenges.test.ts` around lines 73 - 140, Extract the repeated
save→splice→try/finally→restore pattern into a helper (e.g., withChallengePool)
to reduce duplication around CHALLENGE_POOL manipulation; implement
withChallengePool(replacement, fn) that saves [...CHALLENGE_POOL], replaces the
pool (CHALLENGE_POOL.splice(..., ...replacement)), executes fn(), and always
restores the original in a finally block, then update tests that mutate
CHALLENGE_POOL (the three it blocks using generateDailyChallenges) to call
withChallengePool(...) wrapping their assertions.

src/lib/games.test.ts (1)

73-90: Direct mutation of shared GAMES array is fragile.

Both tests mutate the module-level GAMES export in-place. While the try/finally guards restore state, this pattern couples tests to array ordering and risks inter-test pollution if a test throws before the finally (e.g., during setup). Consider injecting or abstracting the data source instead of mutating the shared constant.

Additionally, the search exclusion test (line 157) searches for 'tetris' and asserts GAMES[0] is excluded — this silently passes (false-positive) if GAMES[0] isn't actually tetris. Use original.name.toLowerCase() or original.id as the search term to make the test self-consistent.

Suggested fix for the search exclusion test

         it('should exclude inactive games from results', () => {
             const original = GAMES[0]
             GAMES[0] = {
                 ...original,
                 isActive: false,
             }
 
             try {
-                const results = searchGames('tetris')
+                const results = searchGames(original.name)
                 expect(results.some(game => game.id === original.id)).toBe(
                     false
                 )
             } finally {
                 GAMES[0] = original
             }
         })

Also applies to: 149-164

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/lib/games.test.ts` around lines 73 - 90, The test mutates the
module-level GAMES array (used by getMultiplayerGames and other tests), which is
fragile; instead, stop in-place mutation by having tests provide an isolated
data source or a cloned array: update tests to call the game-querying helpers
with a test-specific games array (or deep-clone GAMES before modification)
rather than assigning to GAMES[0], and restore nothing after the test;
specifically refactor references in the failing test to avoid mutating GAMES and
use a clonedGameList passed into getMultiplayerGames (or a new helper that
accepts a games param). Also fix the search-exclusion test by using a
deterministic search term from the modified fixture (e.g.,
original.name.toLowerCase() or original.id) so the assertion that the modified
game is excluded can never silently pass.

src/pages/api/user/avatar.test.ts (1)

20-25: File.prototype.arrayBuffer patch is never restored after tests.

The Object.defineProperty override on File.prototype isn't undone by vi.clearAllMocks() (which only resets Vitest spies/mocks). While configurable: true allows re-definition, this can leak to other test files running in the same vitest worker thread. Consider saving and restoring the original descriptor in an afterEach/afterAll.

♻️ Suggested cleanup

+    let originalArrayBuffer: PropertyDescriptor | undefined
+
     beforeEach(() => {
         vi.clearAllMocks()
         vi.stubGlobal('Buffer', Buffer)
+        originalArrayBuffer = Object.getOwnPropertyDescriptor(File.prototype, 'arrayBuffer')
         Object.defineProperty(File.prototype, 'arrayBuffer', {
             configurable: true,
             value: async function () {
                 return new Uint8Array([97, 98, 99]).buffer
             },
         })
         vi.mocked(updateUser).mockResolvedValue(true)
     })
+
+    afterEach(() => {
+        if (originalArrayBuffer) {
+            Object.defineProperty(File.prototype, 'arrayBuffer', originalArrayBuffer)
+        }
+    })

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/pages/api/user/avatar.test.ts` around lines 20 - 25, The test overrides
File.prototype.arrayBuffer via Object.defineProperty but never restores the
original descriptor, leaking into other tests; capture the original descriptor
with Object.getOwnPropertyDescriptor(File.prototype, 'arrayBuffer') before
redefining and restore it in an afterEach or afterAll teardown (instead of
relying on vi.clearAllMocks), using Object.defineProperty(File.prototype,
'arrayBuffer', originalDescriptor) to revert; reference the existing override of
File.prototype.arrayBuffer and use afterEach/afterAll to ensure cleanup across
tests.

src/lib/games/memory-matrix/init.ts (1)

109-117: Captured gameInstance reference remains usable after cleanup().

After cleanup() destroys and nulls the module-level game, the returned gameInstance ref still points to the destroyed object. Methods like restart() would call resetGame() on it, potentially producing unexpected state. Consider guarding the returned methods or documenting that the instance must not be used after cleanup().

One possible guard pattern

+    let cleaned = false
+    const guardedCleanup = () => {
+        cleaned = true
+        cleanup()
+    }
     return {
-        restart: () => gameInstance.resetGame(),
-        getState: () => gameInstance.getGameState(),
-        getStats: () => gameInstance.getGameStats(),
-        endGame: () => gameInstance.endGameEarly(),
-        cleanup,
+        restart: () => { if (!cleaned) gameInstance.resetGame() },
+        getState: () => gameInstance.getGameState(),
+        getStats: () => gameInstance.getGameStats(),
+        endGame: () => { if (!cleaned) gameInstance.endGameEarly() },
+        cleanup: guardedCleanup,
     }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/lib/games/memory-matrix/init.ts` around lines 109 - 117, The returned
object currently closes over gameInstance (captured from module-level game) so
it remains usable after cleanup(); instead, change the returned methods
(restart, getState, getStats, endGame) to resolve the current module-level game
at call time and either delegate to it or throw a clear error if game is null
(i.e., after cleanup), or add a guard helper that checks the module-level game
before calling resetGame/getGameState/getGameStats/endGameEarly; ensure cleanup
still nulls the module-level game and document that callers will get an error if
they call these methods post-cleanup.

src/pages/api/user/profile.ts (1)

40-43: Use errorResponse for consistency with other error paths.

This manually constructs the { error: ... } shape via jsonResponse, but errorResponse already does exactly that. Using it here keeps the error responses uniform with lines 21, 51, and 59.

Proposed fix

-                    return jsonResponse(
-                        { error: 'Username is already taken' },
-                        409
-                    )
+                    return errorResponse('Username is already taken', 409)

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/pages/api/user/profile.ts` around lines 40 - 43, Replace the manual
jsonResponse({ error: 'Username is already taken' }, 409) return with the
existing errorResponse helper to keep error shapes consistent; locate the return
in src/pages/api/user/profile.ts that currently calls jsonResponse and swap it
to call errorResponse with the same payload and status (use errorResponse({
error: 'Username is already taken' }, 409)) so it matches other error paths that
use errorResponse.

src/lib/server/validations.test.ts (1)

14-38: Consider adding a test for the new score upper bound.

The schema now enforces .max(999_999_999), but there's no test verifying that scores exceeding this limit are rejected. This was a key change in this PR.

Suggested addition inside the `scoreSubmissionSchema` describe block

         expect(invalidScore.success).toBe(false)
       })
+
+      it('rejects score exceeding maximum', () => {
+          const result = scoreSubmissionSchema.safeParse({
+              gameId: GameID.TETRIS,
+              score: 1_000_000_000,
+          })
+
+          expect(result.success).toBe(false)
+      })
   })

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/lib/server/validations.test.ts` around lines 14 - 38, Add a unit test
inside the describe('scoreSubmissionSchema') block that verifies the new upper
bound by calling scoreSubmissionSchema.safeParse with a payload using gameId:
GameID.TETRIS and score: 1_000_000_000 (or any value > 999_999_999) and
asserting result.success is false; also consider adding a complementary test
that submits score: 999_999_999 and asserts success is true to verify the
boundary. Ensure you reference scoreSubmissionSchema and GameID.TETRIS when
creating the test cases.

src/lib/server/validations.ts (1)

39-45: Long line — consider breaking the inner schema for readability.

Line 45's inner Zod chain for displayName is a single long line combining .min(), .max(), .nullable(), and .optional(). Consider formatting it across multiple lines for consistency with the username schema just below.

Proposed formatting

-        displayName: z.preprocess(val => {
-            if (typeof val === 'string') {
-                const trimmed = val.trim()
-                return trimmed === '' ? null : trimmed
-            }
-            return val
-        }, z.string().min(1, 'Display name cannot be empty').max(100, 'Display name must be 100 characters or less').nullable().optional()),
+        displayName: z.preprocess(
+            val => {
+                if (typeof val === 'string') {
+                    const trimmed = val.trim()
+                    return trimmed === '' ? null : trimmed
+                }
+                return val
+            },
+            z
+                .string()
+                .min(1, 'Display name cannot be empty')
+                .max(100, 'Display name must be 100 characters or less')
+                .nullable()
+                .optional()
+        ),

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/lib/server/validations.ts` around lines 39 - 45, The displayName
preprocessing chain is a long single line; break the inner Zod schema used in
z.preprocess into a multi-line chain (same style as the username schema) so
.min(...), .max(...), .nullable(), and .optional() each appear on their own
lines for readability; locate the displayName entry and reformat the
z.string().min(...).max(...).nullable().optional() portion across multiple lines
while keeping the existing z.preprocess logic intact.

src/pages/api/user/update-streaks.test.ts (1)

18-20: Move vi.unstubAllEnvs() into afterEach to prevent leaked stubs on test failure.

Tests at lines 107, 129, and 156 call vi.unstubAllEnvs() at the end of the test body. If an assertion fails or an error is thrown before that line, the stub leaks into subsequent tests. Centralizing cleanup in afterEach is safer.

Suggested fix

     afterEach(() => {
         delete process.env.CRON_SECRET
+        vi.unstubAllEnvs()
     })

Then remove the inline vi.unstubAllEnvs() calls at lines 107, 129, and 156.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/pages/api/user/update-streaks.test.ts` around lines 18 - 20, Move the
vi.unstubAllEnvs() call into the existing afterEach cleanup block so environment
stubs are always removed even when a test fails: add vi.unstubAllEnvs() inside
the afterEach callback that currently deletes process.env.CRON_SECRET, and then
remove the inline vi.unstubAllEnvs() calls that currently appear at the ends of
the individual tests (the explicit vi.unstubAllEnvs() invocations in the test
bodies).

src/lib/auth.ts (1)

61-64: trustedOrigins re-evaluates env vars unnecessarily — simplify to [baseURL].

baseURL is already validated and computed above (always a non-empty string by the time betterAuth(...) is reached — the prod/no-URL case throws at line 20). The inline || (!import.meta.env.PROD ? ... : '') expression is redundant, and .filter(Boolean) can never actually filter anything out at this point.

♻️ Proposed simplification

-    trustedOrigins: [
-        import.meta.env.BETTER_AUTH_URL ||
-            (!import.meta.env.PROD ? 'http://localhost:4325' : ''),
-    ].filter(Boolean),
+    trustedOrigins: [baseURL],

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/lib/auth.ts` around lines 61 - 64, The trustedOrigins array in betterAuth
currently re-evaluates env vars and includes a redundant fallback expression;
replace the inline expression used to build trustedOrigins with the
already-validated baseURL value computed earlier so trustedOrigins becomes
[baseURL]; remove the unnecessary import.meta.env.BETTER_AUTH_URL || ... logic
and the .filter(Boolean) call, referencing the trustedOrigins property and the
baseURL variable in the betterAuth configuration.

src/pages/api/user/profile.ts (2)

25-25: Prefer UserUpdate directly over Record<string, unknown> cast.

updates is initialized as Record<string, unknown> and then forcibly cast to UserUpdate on line 46, bypassing TypeScript's nullability checks for displayName and username (which can be null). Since all assigned keys are already members of UserUpdate, using it as the declared type eliminates the unsafe cast entirely.

♻️ Proposed refactor

-        const updates: Record<string, unknown> = {}
+        const updates: UserUpdate = {}
 
         // ...
 
-        const success = await updateUser(user.id, updates as UserUpdate)
+        const success = await updateUser(user.id, updates)

Also applies to: 46-46

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/pages/api/user/profile.ts` at line 25, The variable updates is declared
as Record<string, unknown> and later forcibly cast to UserUpdate (bypassing
nullability for displayName/username); change the declaration of updates to use
the UserUpdate type directly (e.g., const updates: UserUpdate = {}) and remove
the forced cast currently applied where updates is used (the cast at the
assignment site around line 46); ensure the existing conditional assignments to
displayName and username still respect their nullable types so TypeScript
preserves correct nullability checks.

---

55-57: Log the caught error for observability.

_error is silently discarded. Any unexpected exception (e.g., network failure, unhandled rejection) becomes invisible in production. As per coding guidelines, async error handling should surface the failure.

♻️ Proposed fix

-    } catch (_error) {
-        return errorResponse('Internal server error')
+    } catch (error) {
+        console.error('[POST /api/user/profile] Unexpected error:', error)
+        return errorResponse('Internal server error')
     }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/pages/api/user/profile.ts` around lines 55 - 57, The catch block in the
profile API swallows the thrown value (caught as _error); change it to capture
the error (e.g., catch (error)) and emit an observability log before returning
errorResponse. Update the catch in the handler that currently does "catch
(_error) { return errorResponse('Internal server error') }" to log the error
(using the project's logger like processLogger.error(...) or fallback to
console.error) with a short context message, then still return
errorResponse('Internal server error').

src/lib/games/memory-matrix/init.ts (2)

219-225: abortController not nulled after abort() in cleanup().

game and renderer are correctly set to null, but abortController is left as the aborted instance. On the next initMemoryMatrixGame() call, the guard at line 55 evaluates to true and redundantly re-aborts an already-aborted controller. Nulling it keeps the state symmetric.

♻️ Proposed refactor

 function cleanup(): void {
     abortController?.abort()
+    abortController = null
     game?.destroy()
     renderer?.destroy()
     game = null
     renderer = null
 }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/lib/games/memory-matrix/init.ts` around lines 219 - 225, In cleanup(),
after calling abortController?.abort(), also set abortController = null so the
aborted controller is cleared; update the cleanup() function to call
abortController?.abort() followed by abortController = null, leaving game and
renderer nulling as-is, which prevents the guard in initMemoryMatrixGame() from
operating on a stale aborted controller.

---

25-27: onGameComplete return type doesn't reflect that it's await-ed.

Since await callbacks.onGameComplete(...) is used at line 81, the declared return type void silently accepts async callbacks but gives callers no indication their returned Promise will be awaited. Typing it as void | Promise<void> makes the contract explicit.

♻️ Proposed refactor

 export interface MemoryMatrixCallbacks {
-    onGameComplete?: (finalScore: number, stats: GameStats) => void
+    onGameComplete?: (finalScore: number, stats: GameStats) => void | Promise<void>
 }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/lib/games/memory-matrix/init.ts` around lines 25 - 27, The callback type
for MemoryMatrixCallbacks.onGameComplete is declared as returning void but the
implementation awaits callbacks.onGameComplete(...); update the interface to
reflect that async handlers are supported by changing the onGameComplete
signature to return void | Promise<void>, so callers know their Promise will be
awaited; update the MemoryMatrixCallbacks interface declaration and ensure any
related type imports or references use the new union return type (referencing
MemoryMatrixCallbacks and the callbacks.onGameComplete usage).

src/lib/server/db/queries.ts (1)

1057-1094: Transaction reads are performed outside the transaction boundary — potential TOCTOU concern.

The getAllUserIds(), getActiveUserIdsBetween(), and the per-chunk selectFrom('user_stats') queries (lines 1018–1046) all run before the transaction starts on line 1058. If a user is created or removed between the reads and the transactional writes, the streak updates will act on stale data.

For a daily streak job this is very low risk, but worth noting. If you ever move to higher-frequency or concurrent callers, consider pulling the reads into the transaction as well.

Also, new Date().toISOString() is called inside each chunk iteration (lines 1068, 1085), producing slightly different timestamps per chunk. Consider hoisting a single updatedAt before the transaction to keep all rows consistent.

Hoist timestamp for consistency

+    const updatedAt = new Date().toISOString()
+
     const { incremented, reset } = await db.transaction().execute(async trx => {
         let incremented = 0
         if (activeUserIds.length > 0) {
             const activeChunks = chunkArray(activeUserIds, 100)
             for (const chunk of activeChunks) {
                 await trx
                     .updateTable('user_stats')
                     .set(eb => ({
                         streak_days: eb('streak_days', '+', 1),
-                        updated_at: new Date().toISOString(),
+                        updated_at: updatedAt,
                     }))
                     .where('user_id', 'in', chunk)
                     .execute()
             }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/lib/server/db/queries.ts` around lines 1057 - 1094, The reads
(getAllUserIds, getActiveUserIdsBetween and any selectFrom('user_stats') calls)
happen before calling db.transaction().execute which creates a TOCTOU window;
move those read queries into the same transaction callback passed to
db.transaction().execute so the selects and subsequent writes run under the same
trx (use the trx instance for selects as well), and hoist a single updatedAt =
new Date().toISOString() outside/just before the transaction and use that value
inside both chunked update loops to ensure consistent timestamps across all
updated rows (adjust usages in updateTable('user_stats') where streak_days and
updated_at are set and keep chunkArray logic the same).

src/lib/server/db/streaks.test.ts (1)

106-114: Consider adding edge-case tests for the empty-users guard and single-user scenarios.

The current test only covers the happy path (3 users, 2 active, 1 inactive). The production code now has an empty-array guard and chunking logic. Adding tests for allUserIds = [] and for a user count exceeding the chunk size boundary (e.g., >100) would strengthen confidence in those code paths.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/lib/server/db/streaks.test.ts` around lines 106 - 114, Add two new unit
tests for updateAllUserStreaksForUTC: one where the user-fetch returns an empty
array (allUserIds = []) and assert the result is { processed: 0, incremented: 0,
reset: 0 } and that no per-user processing functions are called; and one where
the fetched user list exceeds the chunk size boundary (generate >100 ids or use
the known CHUNK_SIZE constant) to verify chunking logic by asserting all users
are processed and counts match expected (e.g., processed equals generated count)
and that per-chunk processing function (the helper that handles each chunk) is
invoked multiple times. Mock/stub the same DB helpers and use vi.clearAllMocks()
in beforeEach, referencing updateAllUserStreaksForUTC, allUserIds, and the
chunking helper to locate code.

src/lib/server/validations.test.ts (3)

164-166: Update .min() / .max() error messages to Zod 4 error parameter.

Both local test schemas use the Zod 3-style plain-string second argument for .min():

• Line 165: z.string().min(2, 'Too short')
• Line 234: z.number().int().min(1, 'count must be >= 1')

Zod 4 replaces the message param with error; the old message parameter is still supported but deprecated. The same pattern is also present in the production validations.ts schemas (e.g., .min(1, 'Name cannot be empty')). While tests still pass today, updating now avoids silent breakage in a future Zod release.

♻️ Proposed fix (test file)

-    const schema = z.object({
-        name: z.string().min(2, 'Too short'),
-    })
+    const schema = z.object({
+        name: z.string().min(2, { error: 'Too short' }),
+    })

-            .pipe(z.number().int().min(1, 'count must be >= 1')),
+            .pipe(z.number().int().min(1, { error: 'count must be >= 1' })),

The same fix should be applied to the score, name, displayName, username, limit, page, and pageSize fields in src/lib/server/validations.ts.

Also applies to: 232-234

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/lib/server/validations.test.ts` around lines 164 - 166, Update Zod
.min()/.max() calls to use the Zod 4-style error object instead of the
deprecated message string: replace signatures like z.string().min(2, 'Too
short') and z.number().int().min(1, 'count must be >= 1') with z.string().min(2,
{ error: 'Too short' }) and z.number().int().min(1, { error: 'count must be >=
1' }) in the test schema(s) (look for the const schema declarations in
validations.test.ts) and make the same change in src/lib/server/validations.ts
for the fields score, name, displayName, username, limit, page, and pageSize so
all .min/.max calls pass an { error: '...' } object instead of a plain string.

---

210-215: Extract the duplicate schemaWithoutIssues stub into a shared helper.

The same mock object is copy-pasted in both validateBody and validateQuery suites:

// Lines 210-215 and 258-263 are identical
const schemaWithoutIssues = {
    safeParse: () => ({
        success: false as const,
        error: { issues: [] },
    }),
}

♻️ Proposed refactor

+    const emptyIssuesSchema = {
+        safeParse: () => ({ success: false as const, error: { issues: [] } }),
+    }

     describe('validateBody', () => {
         ...
-        const schemaWithoutIssues = {
-            safeParse: () => ({
-                success: false as const,
-                error: { issues: [] },
-            }),
-        }
-        const result = await validateBody(request, schemaWithoutIssues as any)
+        const result = await validateBody(request, emptyIssuesSchema as any)
     ...
     describe('validateQuery', () => {
         ...
-        const schemaWithoutIssues = {
-            safeParse: () => ({
-                success: false as const,
-                error: { issues: [] },
-            }),
-        }
-        const result = validateQuery(url, schemaWithoutIssues as any)
+        const result = validateQuery(url, emptyIssuesSchema as any)

Also applies to: 258-263

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/lib/server/validations.test.ts` around lines 210 - 215, Extract the
duplicated schema stub into a single shared helper to avoid copy-paste between
the validateBody and validateQuery test suites: create a helper (e.g.,
makeSchemaWithoutIssues or schemaWithoutIssuesStub) exported/defined once and
replace the inline objects used in validateBody and validateQuery with
calls/references to that helper; ensure the helper returns the same shape
(safeParse -> { success: false as const, error: { issues: [] } }) so existing
tests referencing schemaWithoutIssues behave identically.

---

40-56: Add a minimum boundary test for score: 0.

The suite covers the upper boundary (999_999_999) and one value below the lower boundary (-1), but not score: 0 itself (the valid minimum). A symmetric boundary test would complete the coverage:

✅ Suggested addition

+        it('accepts score at minimum boundary', () => {
+            const atMin = scoreSubmissionSchema.safeParse({
+                gameId: GameID.TETRIS,
+                score: 0,
+            })
+
+            expect(atMin.success).toBe(true)
+        })

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/lib/server/validations.test.ts` around lines 40 - 56, Add a test to
verify the minimum valid boundary by calling scoreSubmissionSchema.safeParse
with { gameId: GameID.TETRIS, score: 0 } and assert that the result's success is
true; place it alongside the existing boundary tests (the 'rejects score
exceeding maximum allowed value' and 'accepts score at maximum boundary' blocks)
so it reads e.g. "accepts score at minimum boundary" and uses the same pattern
of storing the parse result (e.g. const atMin) and
expect(atMin.success).toBe(true).

src/pages/api/user/avatar.ts (2)

52-56: Redundant alias — avatarEntry is already narrowed to File

After the instanceof File guard, TypeScript narrows avatarEntry to File. The intermediate const file = avatarEntry adds an extra name without benefit.

♻️ Proposed simplification

-        if (!(avatarEntry instanceof File)) {
-            return badRequestResponse('Avatar must be a file')
-        }
-
-        const file = avatarEntry
-
-        // Validate file type using allowlist (reject SVG due to XSS risk)
+        if (!(avatarEntry instanceof File)) {
+            return badRequestResponse('Avatar must be a file')
+        }
+
+        // Validate file type using allowlist (reject SVG due to XSS risk)
+        const file = avatarEntry  // narrowed to File; rename or inline below

Or simply replace all subsequent file references with avatarEntry, removing the alias entirely.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/pages/api/user/avatar.ts` around lines 52 - 56, Remove the redundant
alias "const file = avatarEntry" after the instanceof File check in the avatar
upload handler; TypeScript already narrows avatarEntry to File, so either delete
the alias and use avatarEntry in subsequent code or replace all references to
file with avatarEntry, leaving the instanceof File guard and the
badRequestResponse('Avatar must be a file') check intact.

---

78-92: Storing up to ~6.7 MB of base64 per user row will degrade DB and API performance at scale

A 5 MB file becomes roughly 6.7 MB when base64-encoded and stored in the image column. Every query that selects the user record (login, profile fetch, session hydration, etc.) will pull this payload unless the column is explicitly excluded. There is also no way to serve the avatar via a CDN, and memory pressure grows linearly with active users.

Recommend moving avatars to object storage (S3, Cloudflare R2, Supabase Storage, etc.) and storing only the resulting URL in image. The endpoint would become a pre-signed-URL issuer or a streaming proxy, and the DB row stays small regardless of image size.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/pages/api/user/avatar.ts` around lines 78 - 92, The avatar handler
currently encodes the uploaded file to base64 and stores it in the user's image
column (see file, base64Image, and updateUser), which will bloat the DB; change
this flow to upload the raw file bytes to object storage (S3/R2/Supabase
Storage) and store only the returned object URL or key in updateUser instead of
base64Image; implement either a presigned upload URL endpoint (issue a presigned
put URL to the client and save the URL/key on finalize) or perform a server-side
upload from file.arrayBuffer() to the storage provider and then call
updateUser(user.id, { image: <public_or_presigned_url> }); ensure any consumers
read the URL from the image column and serve via CDN/redirect rather than
embedding base64 in DB responses.

src/lib/games/memory-matrix/init.ts (1)

231-237: Debug window object is exposed in production.

The only guard is typeof window !== 'undefined', so window.memoryMatrixGame.cleanup() is callable by any script in production. As per coding guidelines, this is intended "for development inspection and debugging" only, and should be gated behind a dev-mode check.

♻️ Proposed fix

-// Export for debugging
-if (typeof window !== 'undefined') {
+// Export for debugging (dev only)
+if (typeof window !== 'undefined' && import.meta.env.DEV) {
     ;(window as Window & typeof globalThis).memoryMatrixGame = {
         getGame: () => game,
         getRenderer: () => renderer,
         cleanup,
     }
 }

Based on learnings: "Use game debug objects window.gameNameGame.getGame() for development inspection and debugging."

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/lib/games/memory-matrix/init.ts` around lines 231 - 237, The debug export
exposing memoryMatrixGame (including getGame, getRenderer, cleanup) is currently
gated only by window existence and must be restricted to development; update the
guard around the block that defines (window as Window & typeof
globalThis).memoryMatrixGame to also check a dev-mode flag (e.g.,
process.env.NODE_ENV === 'development' or an existing isDev/isLocal utility) so
the debug object is only attached in dev builds, leaving the functions (getGame,
getRenderer, cleanup) unchanged but unreachable in production.

src/lib/server/db/queries.ts (2)

1013-1016: getAllUserIds() is redundant — query user_stats directly.

getAllUserIds() fetches every user from the user table. The code then immediately re-queries user_stats in chunks to determine which of those IDs actually have a stats row, and the final activeUserIds/inactiveUserIds sets exclusively contain users with user_stats rows. The intermediate step through the user table is unnecessary overhead, especially as the user base grows.

♻️ Suggested simplification

-    const [allUserIds, activeYesterday] = await Promise.all([
-        getAllUserIds(),
-        getActiveUserIdsBetween(yesterdayUTC, todayUTC),
-    ])
-
-    if (allUserIds.length === 0) {
-        return { processed: 0, incremented: 0, reset: 0 }
-    }
-
-    // Query existing user_stats rows in chunks to avoid SQL parameter limits
-    const allUserIdChunks = chunkArray(allUserIds, 100)
-    const existingStatsUserIdsSet = new Set<string>()
-    for (const chunk of allUserIdChunks) {
-        const existingStatsRows = await db
-            .selectFrom('user_stats')
-            .select('user_id')
-            .where('user_id', 'in', chunk)
-            .execute()
-        for (const row of existingStatsRows) {
-            existingStatsUserIdsSet.add(row.user_id)
-        }
-    }
-
-    const activeSet = new Set(activeYesterday)
-    const activeUserIds = allUserIds.filter(
-        uid => activeSet.has(uid) && existingStatsUserIdsSet.has(uid)
-    )
-    const inactiveUserIds = allUserIds.filter(
-        uid => !activeSet.has(uid) && existingStatsUserIdsSet.has(uid)
-    )
+    const [statsRows, activeYesterday] = await Promise.all([
+        db.selectFrom('user_stats').select('user_id').execute(),
+        getActiveUserIdsBetween(yesterdayUTC, todayUTC),
+    ])
+
+    const statsUserIds = statsRows.map(r => r.user_id)
+    if (statsUserIds.length === 0) {
+        return { processed: 0, incremented: 0, reset: 0 }
+    }
+
+    const activeSet = new Set(activeYesterday)
+    const activeUserIds = statsUserIds.filter(uid => activeSet.has(uid))
+    const inactiveUserIds = statsUserIds.filter(uid => !activeSet.has(uid))

This eliminates both getAllUserIds() and the chunked user_stats cross-join, replacing both with a single query.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/lib/server/db/queries.ts` around lines 1013 - 1016, The current flow
unnecessarily calls getAllUserIds() then filters those IDs by querying
user_stats in chunks; instead query user_stats directly to derive the full set
and the active set in one pass. Replace the Promise.all call that uses
getAllUserIds() and getActiveUserIdsBetween(yesterdayUTC, todayUTC) with a
single query against user_stats that returns all user_ids and those active in
the given range (or two direct queries both targeting user_stats), and then
compute activeUserIds/inactiveUserIds from that result—remove getAllUserIds()
and the chunked cross-check logic and update any references to
activeUserIds/inactiveUserIds accordingly.

---

40-72: Lazy-import singletons are susceptible to concurrent double-invocation.

If multiple async call-chains reach e.g. getAchievementService() simultaneously before the first await import(...) resolves, all of them see _achievementService === null, spawn their own dynamic imports, and all assign to the slot. The JS module system makes this functionally safe (same object returned each time), but the extra import calls are wasteful. Storing the Promise instead of the resolved module is the idiomatic fix:

♻️ Suggested pattern (apply to all four lazy loaders)

-let _achievementService:
-    | typeof import('../../services/achievementService')
-    | null = null
+let _achievementServicePromise: Promise<
+    typeof import('../../services/achievementService')
+> | null = null
 async function getAchievementService() {
-    if (!_achievementService) {
-        _achievementService = await import('../../services/achievementService')
+    if (!_achievementServicePromise) {
+        _achievementServicePromise = import('../../services/achievementService')
     }
-    return _achievementService
+    return _achievementServicePromise
 }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/lib/server/db/queries.ts` around lines 40 - 72, The lazy-loader
singletons (_achievementService, _gamesModule, _achievementsModule,
_challengesModule) should store the import Promise instead of only the resolved
module to avoid concurrent double-invocation: change each variable to hold
Promise<typeof import('...')> | null, on first call set the slot to the
import(...) promise (before awaiting), and then await and return that promise in
getAchievementService(), getGamesModule(), getAchievementsModule(), and
getChallengesModule() so simultaneous callers share the same import Promise.

src/lib/server/validations.ts (1)

136-139: Minor naming inconsistency across the two error-extraction sites.

validateBody names the extracted issue firstIssue (line 136) while validateQuery names its identical extraction firstError (line 160). Aligning the name removes any mental context-switching between the two helpers.

♻️ Suggested alignment

-        const firstError = result.error.issues?.[0]
+        const firstIssue = result.error.issues?.[0]
         return {
             success: false,
-            error: firstError?.message || 'Validation failed',
+            error: firstIssue?.message || 'Validation failed',
         }

Also applies to: 160-164

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/lib/server/validations.ts` around lines 136 - 139, The two helpers
validateBody and validateQuery use inconsistent local names for the same
extracted zod issue; rename the variable in validateQuery from firstError to
firstIssue and update its usage so both helpers extract result.error.issues?.[0]
into the identical identifier (firstIssue) before returning the message, keeping
behavior unchanged but eliminating the naming mismatch.