fix: add error resilience to all leaderboard song fetches#6
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughRecomputes per-user language from practice Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant LeaderboardService
participant ProgressDB as Progress DB
participant SongsDB as Songs DB
Client->>LeaderboardService: Request leaderboard
LeaderboardService->>ProgressDB: Query lineProgress per user
ProgressDB-->>LeaderboardService: Return lineProgress entries
LeaderboardService->>SongsDB: Fetch deduped song metadata (allSettled)
SongsDB-->>LeaderboardService: Return song data / nulls for failures
LeaderboardService->>LeaderboardService: Aggregate language counts per user\nCompute topLanguage and language-weighted scores
LeaderboardService-->>Client: Return leaderboard entries with language, scores, ranks
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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 |
|
Caution Docstrings generation - FAILED No docstrings were generated. |
|
Note Unit test generation is a public access feature. Expect some limitations and changes as we gather feedback and continue to improve it. Generating unit tests... This may take up to 20 minutes. |
|
Agent execution returned an error. Exit code: 1 |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
EtanHey
changed the title
EtanHey
force-pushed
the
nightshift/2026-02-01-4105
branch
from
bd56624 to
6e1b403
Compare
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
convex/leaderboard.ts (4)
98-145:⚠️ Potential issue | 🟠 MajorPerformance concern: per-user song fetching in a leaderboard query.
For every user with a
displayName, this query fetches all theirlineProgressrecords and then individually fetches each unique song. With many users and songs, this becomes O(users × unique_songs_per_user) database reads in a single query invocation. Consider caching the user's primary language on the user document (denormalized) and updating it on practice events, so the leaderboard query becomes a simple sort.
208-244:⚠️ Potential issue | 🟠 MajorWords learned are always scored with
"mixed"multiplier, bypassing language difficulty scaling.All
wordProgressis bucketed under"mixed"(lines 212–216), sogetLanguageMultiplier("mixed")is applied to the word count. If"mixed"maps to 1.0×, users studying Hard (1.5×) or Super-Hard (2.0×) languages get no word-learning bonus despite the intended formula:(words_learned × multiplier) + (lines_completed × multiplier × 0.5).Since you already resolve each user's song languages from
lineProgress, you could distribute word counts proportionally across those languages, or at minimum use the user'stopLanguagemultiplier for words.Based on learnings: "Use Progress Score Formula: (words_learned × multiplier) + (lines_completed × multiplier × 0.5)" and "Apply language difficulty multipliers based on FSI rankings."
306-420: 🛠️ Refactor suggestion | 🟠 MajorSignificant code duplication: progress score calculation is repeated three times.
The same fetch-words → fetch-lines → fetch-songs → compute-language-scores → sum-weighted-scores pattern appears in:
getProgressLeaderboard(lines 183–244)getUserRankfor the current user (lines 308–359)getUserRankfor each other user (lines 370–415)Extract a shared helper, e.g.
calculateProgressScore(ctx, userId), to eliminate ~90 lines of duplication and ensure scoring logic stays consistent across all paths.Suggested helper extraction
async function calculateProgressScore( ctx: any, userId: string ): Promise<{ score: number; topLanguage: string }> { const wordProgress = await ctx.db .query("wordProgress") .withIndex("by_user", (q: any) => q.eq("userId", userId)) .filter((q: any) => q.eq(q.field("learned"), true)) .collect(); const lineProgress = await ctx.db .query("lineProgress") .withIndex("by_user", (q: any) => q.eq("userId", userId)) .filter((q: any) => q.eq(q.field("learned"), true)) .collect(); const songResults = await Promise.allSettled( lineProgress.map((line: any) => ctx.db.get(line.songId)) ); const songs = songResults.map((r: any) => r.status === "fulfilled" ? r.value : null ); const languageScores = new Map<string, { wordsLearned: number; linesCompleted: number }>(); const defaultLanguage = "mixed"; languageScores.set(defaultLanguage, { wordsLearned: wordProgress.length, linesCompleted: 0 }); for (let i = 0; i < lineProgress.length; i++) { const song = songs[i]; if (song) { const language = song.sourceLanguage; if (!languageScores.has(language)) { languageScores.set(language, { wordsLearned: 0, linesCompleted: 0 }); } languageScores.get(language)!.linesCompleted++; } } let totalScore = 0; let topLanguage = "mixed"; let maxLanguageScore = 0; for (const [language, scores] of languageScores.entries()) { const multiplier = getLanguageMultiplier(language); const languageScore = scores.wordsLearned * multiplier + scores.linesCompleted * multiplier * 0.5; totalScore += languageScore; if (languageScore > maxLanguageScore) { maxLanguageScore = languageScore; topLanguage = language; } } return { score: Math.round(totalScore * 10) / 10, topLanguage, }; }As per coding guidelines: "All new helpers and utilities MUST have corresponding test files."
362-420:⚠️ Potential issue | 🟠 MajorO(users²) query complexity in
getUserRankprogress path will not scale.For each call to
getUserRank({ type: "progress" }), the handler iterates every user with adisplayNameand, for each, executes 3+ database queries (wordProgress, lineProgress, individual song lookups). With n users this is O(n) × O(queries_per_user), and since this is a reactive Convex query, it re-executes whenever any relevant data changes.Consider denormalizing: store each user's computed
progressScoreon their user document (updated via mutation when progress changes), then rank determination becomes a single indexed query.
🤖 Fix all issues with AI agents
In @.claude/settings.json:
- Around line 50-51: The settings currently grant broad 1Password access via the
wildcard entries "Bash(op run:*)" and "Bash(op inject:*)" in
.claude/settings.json; replace these wildcards with minimal, specific op
invocations (e.g., only the exact "op run:<allowed-command>" or "op
inject:<allowed-action>") or remove them entirely if not required, so the agent
cannot call arbitrary 1Password CLI commands and risk exposing secrets; update
the "Bash(op run:*)" and "Bash(op inject:*)" lines accordingly to enumerate only
the safe, required op commands.
- Line 60: The setting enableAllProjectMcpServers currently set to true
auto-trusts all project-defined MCP servers; change its default to false in
.claude/settings.json (update the enableAllProjectMcpServers boolean to false)
and add guidance/comments or a README entry explaining how to explicitly opt-in
trusted MCP servers (e.g., a per-project allowlist or CLAUDE_TRUSTED_MCP
setting) so only vetted servers are enabled; ensure any code that reads
enableAllProjectMcpServers (configuration loader logic) treats false as the
secure default and provides a clear warning/log when a repo-defined MCP server
is ignored.
- Around line 40-41: The settings currently grant overly broad executor
permissions like "Bash(source:*)" and any "python3:*", which allow arbitrary
shell/Python execution; update .claude/settings.json to replace wildcards with
explicit, minimal allowed targets (e.g.,
"Bash(source:/safe/scripts/specific-script.sh)" or
"python3:/safe/scripts/allowed_script.py"), or remove these entries if not
needed, and add a short comment explaining the restricted paths and why they are
safe so reviewers/contributors understand the changed permissions; reference the
exact entries "Bash(source:*)" and any "python3:*" occurrences when making the
edits.
- Around line 56-58: The deny list currently blocks "Bash(rm -rf:*)" but not the
less-forcing "rm -r"; update the deny array entry named "Bash(rm -rf:*)" by also
adding a pattern for "Bash(rm -r:*)" to explicitly block recursive deletes
without forcing, and remove the variant with an extra space ("Bash(rm -r :*)")
to avoid redundant/incorrect patterns; ensure you edit the deny array symbol so
both "Bash(rm -rf:*)" and "Bash(rm -r:*)" are present and rely on the config's
pattern-matching rules rather than adding duplicate spaced variants.
In `@convex/leaderboard.ts`:
- Around line 110-118: Replace the Promise.allSettled pattern with Promise.all
and remove the settled-status branching: call Promise.all on
uniqueSongIds.map(id => ctx.db.get(id)) (matching existing usages), assign the
resulting array to a variable (e.g., songs), and then build songMap from
uniqueSongIds and songs (use songs[i] instead of songSettled[i].value); update
references to songSettled to use the new songs array and ensure null results
from ctx.db.get are handled by existing null-checks.
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (4)
.claude/settings.jsonconvex/leaderboard.tssrc/components/LyricsDisplay.tsxsrc/routes/song.$songId.tsx
💤 Files with no reviewable changes (2)
- src/routes/song.$songId.tsx
- src/components/LyricsDisplay.tsx
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use TanStack Start framework with Bun runtime for the application
Tests must pass locally via
bun run testbefore committing code, as Husky pre-commit hooks will block commits with failing tests
Files:
convex/leaderboard.ts
convex/*.ts
📄 CodeRabbit inference engine (AGENTS.md)
Use Convex for database queries, mutations, and authentication configuration
Files:
convex/leaderboard.ts
**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
All new helpers and utilities MUST have corresponding test files
Files:
convex/leaderboard.ts
convex/**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
convex/**/*.ts: NEVER create .js files in the convex/ folder - only .ts files belong there
Before starting Convex dev server, always runrm -f convex/*.jsto clean compiled JavaScript files
Use Convex schema.ts for database schema definitions and songs.ts for queries and mutations
Files:
convex/leaderboard.ts
🧠 Learnings (4)
📓 Common learnings
Learnt from: CR
Repo: EtanHey/songscript PR: 0
File: prd-json/AGENTS.md:0-0
Timestamp: 2026-01-23T18:12:49.193Z
Learning: Applies to prd-json/**/convex/**/*{score,progress,leaderboard}*.{ts,tsx,js} : Use Progress Score Formula: (words_learned × multiplier) + (lines_completed × multiplier × 0.5)
Learnt from: CR
Repo: EtanHey/songscript PR: 0
File: prd-json/AGENTS.md:0-0
Timestamp: 2026-01-23T18:12:49.193Z
Learning: Applies to prd-json/**/convex/**/leaderboard*.{ts,tsx,js} : Users can play without displayName, but must set displayName before appearing on the leaderboard
Learnt from: CR
Repo: EtanHey/songscript PR: 0
File: prd-json/AGENTS.md:0-0
Timestamp: 2026-01-23T18:12:49.193Z
Learning: Applies to prd-json/**/convex/**/*.{ts,tsx,js} : Apply language difficulty multipliers based on FSI rankings: Standard languages (Spanish, French, Italian, Dutch) use 1.0x, Hard languages (Hebrew, Persian, Russian, Hindi) use 1.5x, Super-Hard languages (Arabic, Japanese, Korean, Chinese) use 2.0x
📚 Learning: 2026-01-23T18:12:49.193Z
Learnt from: CR
Repo: EtanHey/songscript PR: 0
File: prd-json/AGENTS.md:0-0
Timestamp: 2026-01-23T18:12:49.193Z
Learning: Applies to prd-json/**/convex/**/*{score,progress,leaderboard}*.{ts,tsx,js} : Use Progress Score Formula: (words_learned × multiplier) + (lines_completed × multiplier × 0.5)
Applied to files:
convex/leaderboard.ts
📚 Learning: 2026-01-23T18:12:49.193Z
Learnt from: CR
Repo: EtanHey/songscript PR: 0
File: prd-json/AGENTS.md:0-0
Timestamp: 2026-01-23T18:12:49.193Z
Learning: Applies to prd-json/**/convex/**/leaderboard*.{ts,tsx,js} : Users can play without displayName, but must set displayName before appearing on the leaderboard
Applied to files:
convex/leaderboard.ts
📚 Learning: 2026-01-23T18:12:49.193Z
Learnt from: CR
Repo: EtanHey/songscript PR: 0
File: prd-json/AGENTS.md:0-0
Timestamp: 2026-01-23T18:12:49.193Z
Learning: Applies to prd-json/**/convex/**/*.{ts,tsx,js} : Apply language difficulty multipliers based on FSI rankings: Standard languages (Spanish, French, Italian, Dutch) use 1.0x, Hard languages (Hebrew, Persian, Russian, Hindi) use 1.5x, Super-Hard languages (Arabic, Japanese, Korean, Chinese) use 2.0x
Applied to files:
convex/leaderboard.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Cursor Bugbot
🔇 Additional comments (4)
convex/leaderboard.ts (4)
88-93: Leaderboard correctly filters users bydisplayNamepresence.The filter
q.neq(q.field("displayName"), undefined)ensures only users who have set a display name appear on leaderboards, consistent with the requirement that users must set adisplayNamebefore appearing.
197-206:getProgressLeaderboardcorrectly appliesPromise.allSettledfor resilient song fetching.The
Promise.allSettledpattern here preserves index alignment withlineProgress(critical for the loop at lines 219–228) and gracefully handles any failed lookups by mapping them tonull. The existingif (song)check at line 221 correctly skips nulls.
237-237: Progress score formula matches the specified formula.
(scores.wordsLearned * multiplier) + (scores.linesCompleted * multiplier * 0.5)correctly implements the required formula. This is consistent across all three copies of the calculation (lines 237, 356, 412).Based on learnings: "Use Progress Score Formula: (words_learned × multiplier) + (lines_completed × multiplier × 0.5)."
283-305: No action required —getUserRankcorrectly omits thelanguagefield.The return shape difference is intentional.
getUserRankreturns only{ rank, score, hasDisplayName }because it's meant to answer "what's my rank?" for the current user's rank card. Neither consumer (leaderboard.tsx nor LeaderboardMiniSection.tsx) accesses alanguagefield fromgetUserRank—both only userankandscore. Thelanguagefield ingetStreakLeaderboardis for the public leaderboard display, which serves a different purpose. The two queries have different responsibilities and return shapes by design.Likely an incorrect or invalid review comment.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| "Bash(source:*)", | ||
| "Bash(yt-dlp:*)", |
There was a problem hiding this comment.
Broad permissions for source:* and python3:* could execute arbitrary code.
source:* can execute any shell script on disk (including untrusted ones), and python3:* can run arbitrary Python. Consider narrowing these to specific scripts/paths if possible, or ensure contributors understand these permissions are granted.
🤖 Prompt for AI Agents
In @.claude/settings.json around lines 40 - 41, The settings currently grant
overly broad executor permissions like "Bash(source:*)" and any "python3:*",
which allow arbitrary shell/Python execution; update .claude/settings.json to
replace wildcards with explicit, minimal allowed targets (e.g.,
"Bash(source:/safe/scripts/specific-script.sh)" or
"python3:/safe/scripts/allowed_script.py"), or remove these entries if not
needed, and add a short comment explaining the restricted paths and why they are
safe so reviewers/contributors understand the changed permissions; reference the
exact entries "Bash(source:*)" and any "python3:*" occurrences when making the
edits.
| "Bash(op run:*)", | ||
| "Bash(op inject:*)", |
There was a problem hiding this comment.
op run:* and op inject:* grant access to 1Password secrets.
These permissions allow the AI agent to invoke 1Password CLI commands, which could inadvertently expose secrets in logs or outputs. If these are needed, consider restricting to specific op invocations rather than wildcards.
🤖 Prompt for AI Agents
In @.claude/settings.json around lines 50 - 51, The settings currently grant
broad 1Password access via the wildcard entries "Bash(op run:*)" and "Bash(op
inject:*)" in .claude/settings.json; replace these wildcards with minimal,
specific op invocations (e.g., only the exact "op run:<allowed-command>" or "op
inject:<allowed-action>") or remove them entirely if not required, so the agent
cannot call arbitrary 1Password CLI commands and risk exposing secrets; update
the "Bash(op run:*)" and "Bash(op inject:*)" lines accordingly to enumerate only
the safe, required op commands.
| "deny": [ | ||
| "Bash(rm -rf:*)" | ||
| ] |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
🧩 Analysis chain
🏁 Script executed:
cat .claude/settings.jsonRepository: EtanHey/songscript
Length of output: 1630
🌐 Web query:
Claude Code allow deny precedence bash commands
💡 Result:
In Claude Code permission rules for Bash commands, precedence works like this:
denywins over everything: if a Bash invocation matches anypermissions.denyrule, it is blocked—even if it also matchesalloworask. [1]askwins overallow: if it matches anaskrule, Claude Code will prompt you, even if there’s also a matchingallowrule. [1]allowapplies only if nodenyoraskmatched: the command runs without prompting. [1]- Otherwise, Claude Code falls back to the default behavior (controlled by settings like
defaultMode). [1][2]
Also note: Bash patterns are prefix matches (not regex), so broad allows can unintentionally include more dangerous variants unless you carve them out with deny/ask. [2]
Example
{
"permissions": {
"allow": ["Bash(git push:*)"],
"ask": ["Bash(git push --force:*)"],
"deny": ["Bash(curl:*)"]
}
}git push origin main→ allowed (no prompt)git push --force ...→ prompts (askoverridesallow)curl ...→ blocked (denyoverrides everything)
Sources: Anthropic Claude Code IAM / permissions precedence [1]; Claude Code settings reference (Bash prefix matching) [2].
🌐 Web query:
difference between rm -r and rm -rf destructiveness bash
💡 Result:
-
rm -r: recursive. It letsrmremove directories and their contents (descend into the directory tree). Without-r,rmtypically won’t remove directories at all. POSIX defines-r/-Ras “recursive descent through the file hierarchy.” [1] -
rm -rf: recursive + force. It still removes directories recursively, and-f(“force”) makesrmnot prompt for confirmation and suppress some errors, notably “file doesn’t exist” (and typically doesn’t treat that as a failure). [1][7]
“Destructiveness” in practice (Bash context)
rm -ris destructive because it can delete whole directory trees, but you’re more likely to notice problems (e.g., non-existent files, permission issues) via diagnostics, and depending on your environment/aliases and TTY/writability conditions, you may still see prompts in some cases. [1]rm -rfis usually more destructive because it combines recursive deletion with no confirmation prompts and fewer visible warnings, making it easier to delete large amounts of data quickly (including via scripts/globs). [7][5]
rm -r is less destructive than rm -rf, not equally destructive.
rm -r recursively deletes but still shows prompts and error messages, whereas rm -rf forces deletion and suppresses errors—making it more dangerous. However, rm -r can still pose a risk and may be worth blocking. Given that the allow list is whitelist-based (only specific commands allowed) with no explicit rm -r permission, rm -r would likely already be blocked by default depending on the configuration's default behavior. If protection against rm -r is desired, the suggested addition of "Bash(rm -r:*)" is reasonable, though the "Bash(rm -r :*)" variant may be redundant depending on how pattern matching works.
🤖 Prompt for AI Agents
In @.claude/settings.json around lines 56 - 58, The deny list currently blocks
"Bash(rm -rf:*)" but not the less-forcing "rm -r"; update the deny array entry
named "Bash(rm -rf:*)" by also adding a pattern for "Bash(rm -r:*)" to
explicitly block recursive deletes without forcing, and remove the variant with
an extra space ("Bash(rm -r :*)") to avoid redundant/incorrect patterns; ensure
you edit the deny array symbol so both "Bash(rm -rf:*)" and "Bash(rm -r:*)" are
present and rely on the config's pattern-matching rules rather than adding
duplicate spaced variants.
| "Bash(rm -rf:*)" | ||
| ] | ||
| }, | ||
| "enableAllProjectMcpServers": true |
There was a problem hiding this comment.
enableAllProjectMcpServers: true auto-trusts all project-defined MCP servers.
This means any .mcp.json or similar config in the repo will be automatically trusted. A malicious contribution adding an MCP server config could gain these permissions. Consider setting this to false and explicitly enabling only trusted MCP servers.
🤖 Prompt for AI Agents
In @.claude/settings.json at line 60, The setting enableAllProjectMcpServers
currently set to true auto-trusts all project-defined MCP servers; change its
default to false in .claude/settings.json (update the enableAllProjectMcpServers
boolean to false) and add guidance/comments or a README entry explaining how to
explicitly opt-in trusted MCP servers (e.g., a per-project allowlist or
CLAUDE_TRUSTED_MCP setting) so only vetted servers are enabled; ensure any code
that reads enableAllProjectMcpServers (configuration loader logic) treats false
as the secure default and provides a clear warning/log when a repo-defined MCP
server is ignored.
| const songSettled = await Promise.allSettled( | ||
| uniqueSongIds.map((id) => ctx.db.get(id)) | ||
| ); | ||
| const songMap = new Map( | ||
| uniqueSongIds.map((id, i) => [ | ||
| id, | ||
| songSettled[i].status === 'fulfilled' ? songSettled[i].value : null, | ||
| ]) | ||
| ); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
🧩 Analysis chain
🌐 Web query:
Convex ctx.db.get behavior missing documents null throw error
💡 Result:
In Convex, ctx.db.get(...) does not throw when a document is missing — it returns null and you’re expected to handle that in your code (the Convex docs’ own examples check if (doc === null) ...). [1]
It can throw in “developer error” situations, e.g. if you pass an invalid argument like null instead of a real document id. [2]
If you want “missing doc => throw”
Do it explicitly:
import { ConvexError } from "convex/values";
const doc = await ctx.db.get("tasks", args.id);
if (doc === null) throw new ConvexError("Not found");
return doc;Or use a helper that “orThrow”s (community pattern), e.g. getOneFromOrThrow / getAllOrThrow. [3]
Sources: Convex query docs showing db.get returning null for missing docs [1]; Convex error-handling docs noting invalid arguments (like null) are developer errors [2]; Convex community helper patterns for throwing instead of returning null [3].
[1] (docs.convex.dev)
[2] (docs.convex.dev)
[3] (stack.convex.dev)
Citations:
- 1: https://docs.convex.dev/functions/query-functions
- 2: https://docs.convex.dev/functions/error-handling/
- 3: https://stack.convex.dev/functional-relationships-helpers
🏁 Script executed:
# Check how ctx.db.get is used in the codebase
rg "ctx\.db\.get" convex/ -A 2 -B 2Repository: EtanHey/songscript
Length of output: 7863
🏁 Script executed:
# Look at the full context of the code in leaderboard.ts
sed -n '100,125p' convex/leaderboard.tsRepository: EtanHey/songscript
Length of output: 1102
Use Promise.all() with null-checks instead of Promise.allSettled().
In Convex, ctx.db.get() returns null for missing documents—it doesn't throw. The only exceptions are developer errors (e.g., passing null as the id), which allSettled wouldn't help with. Throughout the codebase, all other usages follow the simpler pattern: Promise.all() followed by null checks.
Current code
const songSettled = await Promise.allSettled(
uniqueSongIds.map((id) => ctx.db.get(id))
);
const songMap = new Map(
uniqueSongIds.map((id, i) => [
id,
songSettled[i].status === 'fulfilled' ? songSettled[i].value : null,
])
);
Simplify to match the established pattern in the codebase:
const songs = await Promise.all(
uniqueSongIds.map((id) => ctx.db.get(id))
);
const songMap = new Map(
uniqueSongIds.map((id, i) => [id, songs[i]])
);🤖 Prompt for AI Agents
In `@convex/leaderboard.ts` around lines 110 - 118, Replace the Promise.allSettled
pattern with Promise.all and remove the settled-status branching: call
Promise.all on uniqueSongIds.map(id => ctx.db.get(id)) (matching existing
usages), assign the resulting array to a variable (e.g., songs), and then build
songMap from uniqueSongIds and songs (use songs[i] instead of
songSettled[i].value); update references to songSettled to use the new songs
array and ensure null results from ctx.db.get are handled by existing
null-checks.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
EtanHey
force-pushed
the
nightshift/2026-02-01-4105
branch
from
6e1b403 to
6da90a0
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
convex/leaderboard.ts (2)
369-421: 🛠️ Refactor suggestion | 🟠 MajorO(N × queries) rank computation recalculates every user's score from scratch — extract shared helper.
The progress score calculation logic is duplicated three times:
getProgressLeaderboard(lines 209–245),getUserRankfor the current user (lines 331–360), andgetUserRankfor every other user (lines 392–416). Each iteration issues multiple DB queries per user, making this an expensive N+1 pattern.Extract a
calculateUserProgressScore(ctx, userId)helper and reuse it. Even better,getUserRankcould call the progress leaderboard logic once and find the user's position in the sorted result, rather than recomputing every score independently.Sketch of extracted helper
async function calculateUserProgressScore( ctx: any, userId: string ): Promise<number> { const wordProgress = await ctx.db .query("wordProgress") .withIndex("by_user", (q: any) => q.eq("userId", userId)) .filter((q: any) => q.eq(q.field("learned"), true)) .collect(); const lineProgress = await ctx.db .query("lineProgress") .withIndex("by_user", (q: any) => q.eq("userId", userId)) .filter((q: any) => q.eq(q.field("learned"), true)) .collect(); const uniqueSongIds = [...new Set(lineProgress.map((l) => l.songId))]; const songResults = await Promise.all( uniqueSongIds.map((id) => ctx.db.get(id)) ); const songMap = new Map( uniqueSongIds.map((id, i) => [id, songResults[i]]) ); const languageScores = new Map<string, { wordsLearned: number; linesCompleted: number }>(); languageScores.set("mixed", { wordsLearned: wordProgress.length, linesCompleted: 0 }); for (const line of lineProgress) { const song = songMap.get(line.songId); if (song) { const lang = song.sourceLanguage; if (!languageScores.has(lang)) { languageScores.set(lang, { wordsLearned: 0, linesCompleted: 0 }); } languageScores.get(lang)!.linesCompleted++; } } let total = 0; for (const [language, scores] of languageScores.entries()) { const multiplier = getLanguageMultiplier(language); total += (scores.wordsLearned * multiplier) + (scores.linesCompleted * multiplier * 0.5); } return Math.round(total * 10) / 10; }
88-93: 🧹 Nitpick | 🔵 TrivialUnbounded
.collect()on all users will hit Convex query limits as the user base grows.Both
getStreakLeaderboardandgetUserRankfetch all users with a display name and then run sequential async operations (streak calculation, song lookups) for each. Convex queries have execution time limits, so this will eventually fail. Consider caching/precomputing leaderboard scores via a scheduled mutation, or at minimum adding a reasonable upper bound.
🤖 Fix all issues with AI agents
In @.claude/settings.json:
- Around line 14-15: The allow list currently includes the risky commands
"Bash(git rebase:*)" and "Bash(git merge:*)"; update .claude/settings.json to
prevent automatic destructive history rewrites by removing these two entries
from the allow list and adding them to the "ask" list (or otherwise gating them
behind confirmation), referencing the exact strings "Bash(git rebase:*)" and
"Bash(git merge:*)" so the agent will prompt a human before executing those
commands.
In `@convex/leaderboard.ts`:
- Around line 198-207: The song fetch loop currently calls ctx.db.get for every
entry in lineProgress causing duplicate DB reads; instead compute a deduplicated
set of songIds (like new Set(lineProgress.map(l => l.songId))), fetch each
unique song once (using ctx.db.get for each id) and build a songMap (songId ->
song or null) from those results; then replace usages that index into songs by
position (e.g., the mapping at Lines where songResults/songs are used) to lookup
via songMap.get(lineProgress[i].songId) (also apply the same dedupe/fetch
approach for the other occurrences noted around the blocks referencing
songResults at the other locations).
- Around line 101-138: The leaderboard file contains inline language-detection
logic that needs to be extracted into a pure, testable helper (e.g.,
detectPrimaryLanguage or getPrimaryLanguageFromLineProgress) and unit-tested
alongside calculateCurrentStreak and the query handlers getStreakLeaderboard and
getProgressLeaderboard; refactor lines that build lineProgress, deduplicate song
IDs, resolve songs into a songMap, count languages and pick the top language
into that helper, then add convex/leaderboard.test.ts to cover empty
lineProgress, missing/null songs in songMap, language ties (ensure deterministic
tie-breaking), and normal cases, plus unit tests for calculateCurrentStreak and
full behavior of getStreakLeaderboard/getProgressLeaderboard using mocked DB
responses for lineProgress and song lookups.
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
.claude/settings.jsonconvex/leaderboard.ts
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use TanStack Start framework with Bun runtime for the application
Tests must pass locally via
bun run testbefore committing code, as Husky pre-commit hooks will block commits with failing tests
Files:
convex/leaderboard.ts
convex/*.ts
📄 CodeRabbit inference engine (AGENTS.md)
Use Convex for database queries, mutations, and authentication configuration
Files:
convex/leaderboard.ts
**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
All new helpers and utilities MUST have corresponding test files
Files:
convex/leaderboard.ts
convex/**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
convex/**/*.ts: NEVER create .js files in the convex/ folder - only .ts files belong there
Before starting Convex dev server, always runrm -f convex/*.jsto clean compiled JavaScript files
Use Convex schema.ts for database schema definitions and songs.ts for queries and mutations
Files:
convex/leaderboard.ts
🧠 Learnings (5)
📓 Common learnings
Learnt from: CR
Repo: EtanHey/songscript PR: 0
File: prd-json/AGENTS.md:0-0
Timestamp: 2026-01-23T18:12:49.193Z
Learning: Applies to prd-json/**/convex/**/*{score,progress,leaderboard}*.{ts,tsx,js} : Use Progress Score Formula: (words_learned × multiplier) + (lines_completed × multiplier × 0.5)
Learnt from: CR
Repo: EtanHey/songscript PR: 0
File: prd-json/AGENTS.md:0-0
Timestamp: 2026-01-23T18:12:49.193Z
Learning: Applies to prd-json/**/convex/**/*.{ts,tsx,js} : Apply language difficulty multipliers based on FSI rankings: Standard languages (Spanish, French, Italian, Dutch) use 1.0x, Hard languages (Hebrew, Persian, Russian, Hindi) use 1.5x, Super-Hard languages (Arabic, Japanese, Korean, Chinese) use 2.0x
📚 Learning: 2026-01-23T18:12:49.193Z
Learnt from: CR
Repo: EtanHey/songscript PR: 0
File: prd-json/AGENTS.md:0-0
Timestamp: 2026-01-23T18:12:49.193Z
Learning: Applies to prd-json/**/convex/**/*{score,progress,leaderboard}*.{ts,tsx,js} : Use Progress Score Formula: (words_learned × multiplier) + (lines_completed × multiplier × 0.5)
Applied to files:
convex/leaderboard.ts
📚 Learning: 2026-01-23T18:12:49.193Z
Learnt from: CR
Repo: EtanHey/songscript PR: 0
File: prd-json/AGENTS.md:0-0
Timestamp: 2026-01-23T18:12:49.193Z
Learning: Applies to prd-json/**/convex/**/*.{ts,tsx,js} : Apply language difficulty multipliers based on FSI rankings: Standard languages (Spanish, French, Italian, Dutch) use 1.0x, Hard languages (Hebrew, Persian, Russian, Hindi) use 1.5x, Super-Hard languages (Arabic, Japanese, Korean, Chinese) use 2.0x
Applied to files:
convex/leaderboard.ts
📚 Learning: 2026-01-23T18:12:49.193Z
Learnt from: CR
Repo: EtanHey/songscript PR: 0
File: prd-json/AGENTS.md:0-0
Timestamp: 2026-01-23T18:12:49.193Z
Learning: Applies to prd-json/**/convex/**/leaderboard*.{ts,tsx,js} : Users can play without displayName, but must set displayName before appearing on the leaderboard
Applied to files:
convex/leaderboard.ts
📚 Learning: 2026-01-28T12:17:53.877Z
Learnt from: CR
Repo: EtanHey/songscript PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-28T12:17:53.877Z
Learning: Applies to convex/**/*.ts : Use Convex schema.ts for database schema definitions and songs.ts for queries and mutations
Applied to files:
convex/leaderboard.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Cursor Bugbot
🔇 Additional comments (3)
convex/leaderboard.ts (3)
140-145: LGTM — streak entry now includes the inferredlanguage.The addition of the computed
languagefield to the streak leaderboard entries is a good improvement over the previous hardcoded"mixed"placeholder.
236-245: Progress score formula correctly implements the specified formula with language multipliers.
(scores.wordsLearned * multiplier) + (scores.linesCompleted * multiplier * 0.5)matches the required formula. ThetopLanguagederivation by highest weighted score is sensible.Based on learnings: "Use Progress Score Formula: (words_learned × multiplier) + (lines_completed × multiplier × 0.5)".
209-217: No action needed. ThegetLanguageMultiplierfunction safely handles unknown languages including"mixed"by returning the default multiplier of1.0x. The test suite confirms that unrecognized language strings return1.0rather than0or throwing an error, so the score calculation formula is safe and correct.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| "Bash(git rebase:*)", | ||
| "Bash(git merge:*)", |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
git rebase:* and git merge:* in the allow list can rewrite history.
These are destructive Git operations that can alter commit history or introduce merge conflicts. If the agent is intended only for read/review tasks, consider moving these to "ask" so a human confirms before execution. If the agent does need these for automated workflows, this is acceptable.
🤖 Prompt for AI Agents
In @.claude/settings.json around lines 14 - 15, The allow list currently
includes the risky commands "Bash(git rebase:*)" and "Bash(git merge:*)"; update
.claude/settings.json to prevent automatic destructive history rewrites by
removing these two entries from the allow list and adding them to the "ask" list
(or otherwise gating them behind confirmation), referencing the exact strings
"Bash(git rebase:*)" and "Bash(git merge:*)" so the agent will prompt a human
before executing those commands.
|
|
||
| // Detect user's primary language from their practice data | ||
| const lineProgress = await ctx.db | ||
| .query("lineProgress") | ||
| .withIndex("by_user", (q) => q.eq("userId", user._id)) | ||
| .filter((q) => q.eq(q.field("learned"), true)) | ||
| .collect(); | ||
|
|
||
| // Batch-fetch songs with deduplication + error resilience | ||
| const uniqueSongIds = [...new Set(lineProgress.map((l) => l.songId))]; | ||
| const songSettled = await Promise.allSettled( | ||
| uniqueSongIds.map((id) => ctx.db.get(id)) | ||
| ); | ||
| const songMap = new Map( | ||
| uniqueSongIds.map((id, i) => [ | ||
| id, | ||
| songSettled[i].status === 'fulfilled' ? songSettled[i].value : null, | ||
| ]) | ||
| ); | ||
|
|
||
| // Count lines per language | ||
| const languageCounts = new Map<string, number>(); | ||
| for (const line of lineProgress) { | ||
| const song = songMap.get(line.songId); | ||
| if (song?.sourceLanguage) { | ||
| languageCounts.set(song.sourceLanguage, (languageCounts.get(song.sourceLanguage) || 0) + 1); | ||
| } | ||
| } | ||
|
|
||
| // Find most practiced language | ||
| let language = "mixed"; | ||
| let maxCount = 0; | ||
| for (const [lang, count] of languageCounts.entries()) { | ||
| if (count > maxCount) { | ||
| maxCount = count; | ||
| language = lang; | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Check for test files related to leaderboard
fd -e test.ts -e spec.ts -e test.js -e spec.js | grep -i leaderboard
find . -type f \( -name "*.test.ts" -o -name "*.spec.ts" -o -name "*.test.js" -o -name "*.spec.js" \) | head -20Repository: EtanHey/songscript
Length of output: 593
🏁 Script executed:
# Check for test directory structure
ls -la | grep -i test
find . -maxdepth 3 -type d -name "*test*" -o -name "*spec*" -o -name "*__tests__*"Repository: EtanHey/songscript
Length of output: 360
🏁 Script executed:
# Search for any existing tests that might reference leaderboard
rg "leaderboard" --type ts --type tsx | grep -i "test\|spec\|describe\|it(" | head -10Repository: EtanHey/songscript
Length of output: 89
🏁 Script executed:
# Check the actual file to understand the context better
wc -l convex/leaderboard.tsRepository: EtanHey/songscript
Length of output: 87
🏁 Script executed:
# View the specific lines mentioned in the review
sed -n '101,138p' convex/leaderboard.tsRepository: EtanHey/songscript
Length of output: 1369
🏁 Script executed:
# Check the git status/diff context to see if these lines are new
git diff HEAD -- convex/leaderboard.ts | head -100Repository: EtanHey/songscript
Length of output: 44
🏁 Script executed:
# Look for any test files that might test this logic
find . -type f \( -name "*.test.ts" -o -name "*.spec.ts" \) -exec grep -l "leaderboard\|getStreakLeaderboard\|getProgressLeaderboard" {} \;Repository: EtanHey/songscript
Length of output: 127
🏁 Script executed:
# Get wider context around lines 101-138 to identify which function this is in
sed -n '85,145p' convex/leaderboard.tsRepository: EtanHey/songscript
Length of output: 2085
🏁 Script executed:
# Check what the test files that mention leaderboard are actually testing
grep -A 5 -B 5 "leaderboard" e2e/auth-routes.spec.ts | head -30Repository: EtanHey/songscript
Length of output: 817
🏁 Script executed:
# Check if there's a git history or if this is the initial commit
git log --oneline convex/leaderboard.ts | head -5Repository: EtanHey/songscript
Length of output: 92
🏁 Script executed:
# Check the coding guidelines more carefully - look for test requirements in comments
grep -r "test" CLAUDE.md 2>/dev/null | head -10Repository: EtanHey/songscript
Length of output: 577
🏁 Script executed:
# Check if there's a diff or if lines have markers showing they're new
git status convex/leaderboard.tsRepository: EtanHey/songscript
Length of output: 127
🏁 Script executed:
# Look at the function signatures to understand what needs testing
grep -n "export\|const.*=" convex/leaderboard.ts | head -20Repository: EtanHey/songscript
Length of output: 1024
🏁 Script executed:
# Check if languageDifficulty utility mentioned in learnings is tested
cat src/utils/languageDifficulty.test.ts | head -30Repository: EtanHey/songscript
Length of output: 1192
🏁 Script executed:
# Look for any existing tests in convex/ directory
ls -la convex/*.test.ts
cat convex/admin.test.ts | head -40Repository: EtanHey/songscript
Length of output: 1680
Add unit tests for leaderboard query handlers.
convex/leaderboard.ts lacks a test file despite containing complex logic similar to admin.ts and authHelpers.ts, which both have corresponding test files. Per coding guidelines, all new helpers and utilities in convex/ must have tests. Extract the language-detection logic (lines 101–138) into a testable helper function and add convex/leaderboard.test.ts with coverage for:
- Edge cases: empty
lineProgress, missing songs, language ties - The
calculateCurrentStreakhelper - Both
getStreakLeaderboardandgetProgressLeaderboardquery handlers
🤖 Prompt for AI Agents
In `@convex/leaderboard.ts` around lines 101 - 138, The leaderboard file contains
inline language-detection logic that needs to be extracted into a pure, testable
helper (e.g., detectPrimaryLanguage or getPrimaryLanguageFromLineProgress) and
unit-tested alongside calculateCurrentStreak and the query handlers
getStreakLeaderboard and getProgressLeaderboard; refactor lines that build
lineProgress, deduplicate song IDs, resolve songs into a songMap, count
languages and pick the top language into that helper, then add
convex/leaderboard.test.ts to cover empty lineProgress, missing/null songs in
songMap, language ties (ensure deterministic tie-breaking), and normal cases,
plus unit tests for calculateCurrentStreak and full behavior of
getStreakLeaderboard/getProgressLeaderboard using mocked DB responses for
lineProgress and song lookups.
Apply Promise.allSettled consistently across all song lookup patterns so individual fetch failures don't break the entire leaderboard. Also add git worktree permission to local settings. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
EtanHey
force-pushed
the
nightshift/2026-02-01-4105
branch
from
6da90a0 to
3bbb2b6
Compare
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix is OFF. To automatically fix reported issues with Cloud Agents, enable Autofix in the Cursor dashboard.
| uniqueSongIds.map((id, i) => [ | ||
| id, | ||
| songSettled[i].status === 'fulfilled' ? songSettled[i].value : null, | ||
| ]) |
There was a problem hiding this comment.
Identical song-fetching pattern duplicated four times
Low Severity
The same ~10-line pattern for batch-fetching songs with Promise.allSettled and building a songMap is repeated 4 times in this file (lines 109-119, 198-208, 316-326, and 372-381). This duplication increases maintenance burden—if the error resilience logic needs adjustment (e.g., logging failures, retry logic), the fix must be applied in 4 places, risking inconsistent behavior. Extracting this into a helper function like batchFetchSongsAsMap(ctx, lineProgress) would reduce repetition.
Additional Locations (2)
1 participant
Automated improvement by GolemsZikaron Night Shift.
Added error resilience to leaderboard song fetches**
Note
Low Risk
Localized refactor of leaderboard read logic; main risk is subtle scoring/language attribution changes when song fetches fail or return null.
Overview
Improves leaderboard robustness by making all song lookups deduplicated and failure-tolerant, so a single missing/failed
ctx.db.get(songId)no longer breaks streak/progress/rank calculations.In
convex/leaderboard.ts, replaces per-linePromise.allsong fetches withPromise.allSettledplus asongId -> song|nullmap, and simplifies the default"mixed"language score initialization used in weighted scoring.Written by Cursor Bugbot for commit 3bbb2b6. This will update automatically on new commits. Configure here.
Summary by CodeRabbit
New Features
Bug Fixes
Chores