Conversation
- Implemented user creation and retrieval functions in the database. - Added betting tables for users and bets, including necessary fields.
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📝 WalkthroughWalkthroughThis pull request introduces a comprehensive betting system to the application. It adds database tables for users and bets with associated management functions, new API endpoints for user registration and bet operations, real-time bet state broadcasting, betting UI components with user registration flow, leaderboard tracking, and integration with game round completion logic. Changes
Sequence DiagramsequenceDiagram
participant Client
participant Server
participant Database
Client->>Server: POST /api/register (userId, nickname)
Server->>Database: createUser(userId, nickname)
Database-->>Server: User created
Server-->>Client: 200 OK (userId, balance)
Client->>Server: GET /api/me
Server->>Database: getUser(userId)
Database-->>Server: User data
Server-->>Client: User info + current balance
Client->>Server: POST /api/bet (userId, roundNum, contestant, amount)
Server->>Database: placeBet(userId, roundNum, contestant, amount)
Database->>Database: Validate user balance & prior bet
Database->>Database: BEGIN TRANSACTION
Database->>Database: INSERT bet
Database->>Database: UPDATE user balance
Database->>Database: COMMIT
Database-->>Server: Bet placed
Server->>Database: getBetsForRound(roundNum)
Database-->>Server: Aggregate bet totals
Server-->>Client: 200 OK (updated balance)
Server->>Client: Broadcast betState
Note over Client,Database: Round completes
Server->>Database: resolveBets(roundNum, winnerName)
alt Winner exists
Database->>Database: UPDATE bets matching winner (payout 2x)
Database->>Database: UPDATE winning user balances
else Tie
Database->>Database: UPDATE all bets (refund)
Database->>Database: RESTORE all user balances
end
Database-->>Server: Bets resolved
Client->>Server: GET /api/leaderboard?limit=10
Server->>Database: getLeaderboard(limit)
Database-->>Server: Top 10 players by balance
Server-->>Client: Leaderboard data
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Add betting functionality by persisting users and bets with transactional APIs and wiring UI panels and HTTP endpoints across
|
|
ps: fake coins, virtual coins only, do not encourage gambling |
![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
frontend.tsx
Outdated
| setPlacing(false); | ||
| return; | ||
| } | ||
| setMyBet({ contestant: selected, amount }); |
There was a problem hiding this comment.
🟠 High frontend.tsx:268
Race condition: if the round changes while handlePlace's fetch is in-flight, setMyBet on line 268 will overwrite the reset state from the useEffect, locking the user out of betting on the new round. Consider checking that round.num still matches before calling setMyBet.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file frontend.tsx around line 268:
Race condition: if the round changes while `handlePlace`'s fetch is in-flight, `setMyBet` on line 268 will overwrite the reset state from the `useEffect`, locking the user out of betting on the new round. Consider checking that `round.num` still matches before calling `setMyBet`.
Evidence trail:
frontend.tsx lines 252-275: `handlePlace` function with async fetch to `/api/bet` and `setMyBet` call on line 268 with no round check. frontend.tsx lines 197-219: `useEffect` that resets `myBet` to `null` (line 201) when `round.num` changes. Commit: 674af823ff9e2566cbf5efe2c5a267bbd66fa79a
| userId = String(b.userId ?? ""); | ||
| roundNum = Number(b.roundNum ?? 0); | ||
| contestant = String(b.contestant ?? ""); | ||
| amount = Number(b.amount ?? 0); |
There was a problem hiding this comment.
🟠 High server.ts:592
Number(b.amount) returns NaN for non-numeric strings, which bypasses both amount <= 0 and amount > user.balance checks. This corrupts the user's balance to NULL in SQLite. Consider adding a !Number.isFinite(amount) check.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file server.ts around line 592:
`Number(b.amount)` returns `NaN` for non-numeric strings, which bypasses both `amount <= 0` and `amount > user.balance` checks. This corrupts the user's balance to `NULL` in SQLite. Consider adding a `!Number.isFinite(amount)` check.
Evidence trail:
server.ts line 592: `amount = Number(b.amount ?? 0);` - https://github.com/T3-Content/quipslop/tree/674af823ff9e2566cbf5efe2c5a267bbd66fa79a/server.ts#L592
db.ts lines 84-86: `if (amount <= 0) throw new Error("Amount must be positive"); if (amount > user.balance) throw new Error("Insufficient balance");` - https://github.com/T3-Content/quipslop/tree/674af823ff9e2566cbf5efe2c5a267bbd66fa79a/db.ts#L84-86
db.ts lines 95-96: `UPDATE users SET balance = balance - $amount WHERE id = $userId` - https://github.com/T3-Content/quipslop/tree/674af823ff9e2566cbf5efe2c5a267bbd66fa79a/db.ts#L95-96
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (6)
server.ts (3)
536-575: Nickname input not sanitized for content.The nickname is trimmed and length-checked but not filtered for control characters, excessive Unicode, or potentially offensive content. For a public leaderboard, consider at minimum stripping control characters:
Minimal sanitization
nickname = String((body as Record<string, unknown>).nickname ?? "").trim(); + nickname = nickname.replace(/[\x00-\x1F\x7F]/g, ""); // strip control chars🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server.ts` around lines 536 - 575, Sanitize the nickname before validation and creation: normalize the string (e.g., NFKC) and strip control characters and zero-width/formatting codepoints (apply a reusable sanitizer function used where nickname is assigned in the /api/register handler), then re-check length and emptiness on the sanitized nickname before calling createUser(id, nickname); if the sanitized nickname becomes empty or exceeds allowed grapheme/character limits return the existing 400 error. Update references to nickname in the error paths and in createUser/getUser flows to use the sanitized value so stored and compared names are consistent.
236-258:getBetsForRoundSQL query runs on every broadcast, including WS open/close.
broadcast()is called on every WebSocket connect and disconnect (Lines 687, 696). WithMAX_WS_GLOBALset to 100K, frequent connect/disconnect churn means aGROUP BYquery runs for each event. Consider caching the bet totals and invalidating only when a bet is placed or round changes.Sketch: cache bet totals
+let cachedBetState: { roundNum: number; open: boolean; totals: Record<string, { count: number; total: number }> } | null = null; +let cachedBetRound = -1; + +function getBetState() { + const active = gameState.active; + if (!active) return null; + if (cachedBetRound === active.num && cachedBetState) { + cachedBetState.open = active.phase === "prompting" || active.phase === "answering"; + return cachedBetState; + } + cachedBetRound = active.num; + cachedBetState = { + roundNum: active.num, + open: active.phase === "prompting" || active.phase === "answering", + totals: getBetsForRound(active.num), + }; + return cachedBetState; +} + +function invalidateBetCache() { cachedBetRound = -1; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server.ts` around lines 236 - 258, The broadcast() function currently calls getBetsForRound() every time (including on WS connect/disconnect), causing expensive GROUP BY queries under high churn; add an in-memory cache keyed by round number (e.g., betTotalsCache: Map<number, { totals: Record<string,{count:number,total:number}>, ts:number }>) and change broadcast() to read betState.totals from this cache instead of calling getBetsForRound; update/invalidatethe cache only when bets change or the round changes (hook into the functions/events that place bets and advance rounds to call getBetsForRound(active.num) and set/replace the cache entry, optionally with a short TTL), and ensure broadcast() falls back to a single getBetsForRound() call if cache miss.
560-574: UNIQUE constraint error handling conflates duplicateidand duplicatenickname.When
createUserthrows a UNIQUE error, the code triesgetUser(id). If the collision was on nickname (differentid),getUser(id)returnsnulland the 409 response is returned — correct. But if the collision was on id (astronomically unlikely UUID collision), it returns the existing user's data to a potentially different client — leaking another user's nickname and balance.Given UUIDs, this is practically impossible, but the logic could be clearer by checking the error message for which constraint was violated.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server.ts` around lines 560 - 574, The UNIQUE error handling conflates duplicate id vs duplicate nickname in the createUser catch block; change it to inspect the thrown error message from createUser (e.message) for the specific constraint name (e.g., id primary key or nickname unique index) instead of treating all UNIQUE errors the same: if the constraint indicates a nickname conflict, return 409 with "Nickname taken" (optionally check getUser(id) only for verifying caller's id if you have auth), but if the constraint indicates an id/primary-key conflict, do NOT return another user's data — return a 409 or 500 without leaking the existing user's nickname/balance; update the catch branch that currently calls getUser(id) and the responses to branch based on the parsed constraint name from the error.db.ts (3)
117-133: Tie refunds and losses both setwon = 0, making them indistinguishable by that column alone.Lines 120 and 131 both set
won = 0— for ties (refund) and losses respectively. Queries like "how many bets did a user lose?" would need to also checkpayoutto differentiate. Consider using a distinct sentinel (e.g.,won = -1for tie/refund, or a TEXTstatuscolumn).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@db.ts` around lines 117 - 133, The tie branch currently sets "won = 0" just like the loss branch, making ties indistinguishable; change the tie/refund update in the loop (the branch guarded by winnerName === null) to use a distinct sentinel (e.g., set won = -1) while keeping the payout and user balance refund behavior the same, and then audit any other code that reads bets.won (e.g., queries that count losses or wins) to handle the new sentinel value; specifically modify the DB UPDATE for bets in the tie branch (the prepare/run call that sets won and payout) to write the sentinel and leave the loss branch (in the else after bet.contestant !== winnerName) as won = 0.
82-107: Balance check is outside the transaction, relying on single-threaded execution.The
amount > user.balancecheck (Line 86) and the duplicate-bet check (Line 88) both run beforeBEGIN. This is safe today because bun:sqlite is synchronous and JS is single-threaded, so no interleaving can occur. However, if the server ever moves to a multi-process setup (e.g., cluster mode), this would become a TOCTOU race.Consider moving the balance guard into the SQL itself as a defensive measure:
Defensive UPDATE
- db.prepare("UPDATE users SET balance = balance - $amount WHERE id = $userId") - .run({ $amount: amount, $userId: userId }); + const result = db.prepare("UPDATE users SET balance = balance - $amount WHERE id = $userId AND balance >= $amount") + .run({ $amount: amount, $userId: userId }); + if (result.changes === 0) throw new Error("Insufficient balance");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@db.ts` around lines 82 - 107, The balance and duplicate-bet guards in placeBet are done before BEGIN, leaving a TOCTOU risk; move the verification into the transaction by starting the transaction first, then perform a conditional UPDATE on users (e.g., UPDATE users SET balance = balance - $amount WHERE id = $userId AND balance >= $amount) and check the changed-rows count to fail on insufficient funds, and enforce the duplicate bet check inside the same transaction (either by inserting into bets with a UNIQUE constraint on (user_id, round_num) and handling the constraint error, or by selecting/locking and then inserting) before COMMIT; keep the existing COMMIT/ROLLBACK pattern in place and ensure placeBet handles and surfaces these SQL-level failures consistently.
47-68: AddFOREIGN KEYconstraint onbets.user_idto maintain referential integrity.The
betstable referencesusers.idbut has noFOREIGN KEYconstraint. While the current code is safe (both tables are deleted atomically viaclearAllBets), this leaves the schema vulnerable if individual user deletion is added in the future. Also enablePRAGMA foreign_keys = ONto enforce constraints.Proposed schema fix
db.exec(` CREATE TABLE IF NOT EXISTS bets ( id INTEGER PRIMARY KEY AUTOINCREMENT, - user_id TEXT NOT NULL, + user_id TEXT NOT NULL REFERENCES users(id), round_num INTEGER NOT NULL, contestant TEXT NOT NULL, amount INTEGER NOT NULL, won INTEGER, payout INTEGER DEFAULT 0, timestamp DATETIME DEFAULT CURRENT_TIMESTAMP, UNIQUE(user_id, round_num) ); `);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@db.ts` around lines 47 - 68, Add referential integrity by altering the bets table creation to include a FOREIGN KEY(user_id) REFERENCES users(id) clause and enable enforcement by executing PRAGMA foreign_keys = ON before running schema statements; update the db.exec block that creates the "bets" table (and the "users" table block if needed) to include the FOREIGN KEY on bets.user_id referencing users.id and ensure db.exec or the initialization routine runs "PRAGMA foreign_keys = ON" so SQLite enforces the constraint (references: db.exec, table names "users" and "bets", column "user_id", primary key "id", and the initialization path used by clearAllBets).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@db.ts`:
- Around line 72-76: The current createUser function trusts a client-provided
id; instead generate and persist the user id and an opaque server-side session
token on the server, store the token (e.g., in a sessions table or as a
session_token column on the users table) and return/set the token as an
HttpOnly, Secure cookie; update handlers that currently read client id (e.g.,
/api/me and /api/bet) to look up the user by the server-issued session token
rather than a query id. Concretely: stop accepting client-sent id in createUser,
generate the id and a random session token server-side, insert both via
createUser (or an accompanying createSession function), set the token with
Set-Cookie: HttpOnly Secure (and SameSite) on user creation/login, and change
authentication in endpoints (handlers referencing the id query param) to resolve
user by that token.
In `@frontend.tsx`:
- Around line 197-219: The effect that resets state on round change (useEffect
referencing lastRoundRef, setMyBet, setSelected, setError and fetching
`/api/me`) calls the callback onBalanceChange but doesn't list it in the
dependency array; update this by adding onBalanceChange to the deps array of
that useEffect so the effect sees the latest callback, or alternatively memoize
the parent-provided onBalanceChange with useCallback in the parent to keep the
identity stable—choose one approach and adjust either the dependency list in the
useEffect or the parent to eliminate the stale-closure risk.
- Around line 763-766: The variable canBet (computed from userId and
bettingRound.phase) is unused; either remove its declaration or use it as the
render gate where the betting panel is shown: replace the current render
condition that uses userId && bettingRound with canBet so the panel only appears
during "prompting" or "answering" phases, or if you prefer always showing for
any active round delete the canBet line and any references to it (adjust code in
the component surrounding bettingRound/state.active and the render condition
accordingly).
In `@game.ts`:
- Around line 447-455: The call to resolveBets inside the runGame round outcome
branch can throw and crash the game loop; wrap each resolveBets(r, ...)
invocation (both for contA, contB, and the tie case) in a try-catch so any
transaction failure is caught, logged, and does not prevent subsequent steps:
still update state.scores and then in the catch ensure you log the error
(including r.id or context), proceed to call saveRound(r) and clear state.active
so the round is archived and the game continues; reference the resolveBets
function, the runGame routine, saveRound, and state.active/state.scores when
locating where to add the try-catch.
In `@server.ts`:
- Around line 585-595: The code parses amount with Number(b.amount) which allows
non-integer values; update the parsing/validation after reading the JSON (in the
try block where userId, roundNum, contestant, amount are assigned) to ensure
amount is a safe integer >= 0: parse/coerce the input, verify it's a finite
integer (e.g., use Number.isInteger or compare Math.floor(value) === value) and
not NaN, and if it fails return a 400 JSON response (like the existing Invalid
JSON path) with an explanatory error; ensure the validated integer value
replaces the current amount variable before any DB operations or balance math.
---
Nitpick comments:
In `@db.ts`:
- Around line 117-133: The tie branch currently sets "won = 0" just like the
loss branch, making ties indistinguishable; change the tie/refund update in the
loop (the branch guarded by winnerName === null) to use a distinct sentinel
(e.g., set won = -1) while keeping the payout and user balance refund behavior
the same, and then audit any other code that reads bets.won (e.g., queries that
count losses or wins) to handle the new sentinel value; specifically modify the
DB UPDATE for bets in the tie branch (the prepare/run call that sets won and
payout) to write the sentinel and leave the loss branch (in the else after
bet.contestant !== winnerName) as won = 0.
- Around line 82-107: The balance and duplicate-bet guards in placeBet are done
before BEGIN, leaving a TOCTOU risk; move the verification into the transaction
by starting the transaction first, then perform a conditional UPDATE on users
(e.g., UPDATE users SET balance = balance - $amount WHERE id = $userId AND
balance >= $amount) and check the changed-rows count to fail on insufficient
funds, and enforce the duplicate bet check inside the same transaction (either
by inserting into bets with a UNIQUE constraint on (user_id, round_num) and
handling the constraint error, or by selecting/locking and then inserting)
before COMMIT; keep the existing COMMIT/ROLLBACK pattern in place and ensure
placeBet handles and surfaces these SQL-level failures consistently.
- Around line 47-68: Add referential integrity by altering the bets table
creation to include a FOREIGN KEY(user_id) REFERENCES users(id) clause and
enable enforcement by executing PRAGMA foreign_keys = ON before running schema
statements; update the db.exec block that creates the "bets" table (and the
"users" table block if needed) to include the FOREIGN KEY on bets.user_id
referencing users.id and ensure db.exec or the initialization routine runs
"PRAGMA foreign_keys = ON" so SQLite enforces the constraint (references:
db.exec, table names "users" and "bets", column "user_id", primary key "id", and
the initialization path used by clearAllBets).
In `@server.ts`:
- Around line 536-575: Sanitize the nickname before validation and creation:
normalize the string (e.g., NFKC) and strip control characters and
zero-width/formatting codepoints (apply a reusable sanitizer function used where
nickname is assigned in the /api/register handler), then re-check length and
emptiness on the sanitized nickname before calling createUser(id, nickname); if
the sanitized nickname becomes empty or exceeds allowed grapheme/character
limits return the existing 400 error. Update references to nickname in the error
paths and in createUser/getUser flows to use the sanitized value so stored and
compared names are consistent.
- Around line 236-258: The broadcast() function currently calls
getBetsForRound() every time (including on WS connect/disconnect), causing
expensive GROUP BY queries under high churn; add an in-memory cache keyed by
round number (e.g., betTotalsCache: Map<number, { totals:
Record<string,{count:number,total:number}>, ts:number }>) and change broadcast()
to read betState.totals from this cache instead of calling getBetsForRound;
update/invalidatethe cache only when bets change or the round changes (hook into
the functions/events that place bets and advance rounds to call
getBetsForRound(active.num) and set/replace the cache entry, optionally with a
short TTL), and ensure broadcast() falls back to a single getBetsForRound() call
if cache miss.
- Around line 560-574: The UNIQUE error handling conflates duplicate id vs
duplicate nickname in the createUser catch block; change it to inspect the
thrown error message from createUser (e.message) for the specific constraint
name (e.g., id primary key or nickname unique index) instead of treating all
UNIQUE errors the same: if the constraint indicates a nickname conflict, return
409 with "Nickname taken" (optionally check getUser(id) only for verifying
caller's id if you have auth), but if the constraint indicates an id/primary-key
conflict, do NOT return another user's data — return a 409 or 500 without
leaking the existing user's nickname/balance; update the catch branch that
currently calls getUser(id) and the responses to branch based on the parsed
constraint name from the error.
| export function createUser(id: string, nickname: string) { | ||
| const stmt = db.prepare("INSERT INTO users (id, nickname) VALUES ($id, $nickname)"); | ||
| stmt.run({ $id: id, $nickname: nickname }); | ||
| return { id, nickname, balance: 1000 }; | ||
| } |
There was a problem hiding this comment.
Client-controlled UUID used as sole authentication.
The client generates the user ID (crypto.randomUUID() in frontend.tsx Line 129) and the server trusts it for all subsequent operations (/api/bet, /api/me). While UUIDs are hard to brute-force, the ID is transmitted as a query parameter in /api/me?id=..., exposing it in browser history, server logs, and referrer headers. Anyone with access to the ID can impersonate the user.
For a virtual-currency game this is likely acceptable, but worth noting if the system ever handles anything of value. A server-generated opaque session token (e.g., via Set-Cookie: HttpOnly) would be more robust.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@db.ts` around lines 72 - 76, The current createUser function trusts a
client-provided id; instead generate and persist the user id and an opaque
server-side session token on the server, store the token (e.g., in a sessions
table or as a session_token column on the users table) and return/set the token
as an HttpOnly, Secure cookie; update handlers that currently read client id
(e.g., /api/me and /api/bet) to look up the user by the server-issued session
token rather than a query id. Concretely: stop accepting client-sent id in
createUser, generate the id and a random session token server-side, insert both
via createUser (or an accompanying createSession function), set the token with
Set-Cookie: HttpOnly Secure (and SameSite) on user creation/login, and change
authentication in endpoints (handlers referencing the id query param) to resolve
user by that token.
| if (votesA > votesB) { | ||
| state.scores[contA.name] = (state.scores[contA.name] || 0) + 1; | ||
| resolveBets(r, contA.name); | ||
| } else if (votesB > votesA) { | ||
| state.scores[contB.name] = (state.scores[contB.name] || 0) + 1; | ||
| resolveBets(r, contB.name); | ||
| } else { | ||
| resolveBets(r, null); // tie — refund | ||
| } |
There was a problem hiding this comment.
resolveBets can throw and crash the game loop.
resolveBets (db.ts Line 136-138) rethrows on transaction failure. An unhandled exception here would abort the entire runGame loop — the round never gets archived (saveRound on Line 464 is skipped), state.active is never cleared, and all future rounds stop.
🛡️ Proposed fix: wrap in try-catch
if (votesA > votesB) {
state.scores[contA.name] = (state.scores[contA.name] || 0) + 1;
- resolveBets(r, contA.name);
+ try { resolveBets(r, contA.name); } catch (e) { log("ERROR", "bets", `Failed to resolve bets: ${e}`); }
} else if (votesB > votesA) {
state.scores[contB.name] = (state.scores[contB.name] || 0) + 1;
- resolveBets(r, contB.name);
+ try { resolveBets(r, contB.name); } catch (e) { log("ERROR", "bets", `Failed to resolve bets: ${e}`); }
} else {
- resolveBets(r, null); // tie — refund
+ try { resolveBets(r, null); } catch (e) { log("ERROR", "bets", `Failed to resolve bets (tie): ${e}`); }
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@game.ts` around lines 447 - 455, The call to resolveBets inside the runGame
round outcome branch can throw and crash the game loop; wrap each resolveBets(r,
...) invocation (both for contA, contB, and the tie case) in a try-catch so any
transaction failure is caught, logged, and does not prevent subsequent steps:
still update state.scores and then in the catch ensure you log the error
(including r.id or context), proceed to call saveRound(r) and clear state.active
so the round is archived and the game continues; reference the resolveBets
function, the runGame routine, saveRound, and state.active/state.scores when
locating where to add the try-catch.
matteomekhail
force-pushed
the
betting
branch
from
c35d284 to
5e8b17e
Compare
1 participant
Probably not getting merged but in my head it sounded too funny
Auth can be sub with shoo when is released
Summary by CodeRabbit
Release Notes