Night Shift: fix leaderboard type safety (6 as-any)#25
Closed
Night Shift: fix leaderboard type safety (6 as-any)#25
Conversation
Replace 6 `as any` casts with proper StreakEntry/ProgressEntry types. Split rendering by selectedType so TypeScript can narrow correctly. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Review Summary by QodoRemove type safety issues in leaderboard components
WalkthroughsDescription• Remove 6 as any type casts in leaderboard components • Add explicit StreakEntry and ProgressEntry type definitions • Split rendering logic by selectedType for proper type narrowing • Extract reusable row rendering components to improve maintainability Diagramflowchart LR
A["Leaderboard Components<br/>with as-any casts"] -->|"Define explicit types"| B["StreakEntry &<br/>ProgressEntry types"]
A -->|"Split by selectedType"| C["Type-safe rendering<br/>per leaderboard type"]
A -->|"Extract components"| D["MiniRow & renderRow<br/>helper functions"]
B --> E["Type-safe code<br/>without casts"]
C --> E
D --> E
File Changes1. src/components/dashboard/LeaderboardMiniSection.tsx
|
Code Review by Qodo
1. renderRow missing tests
|
📝 WalkthroughWalkthroughThis PR refactors the leaderboard UI components by introducing typed data structures (StreakEntry, ProgressEntry) for better type safety, adding a new MiniRow component to the mini leaderboard section, consolidating row rendering logic into a reusable helper, and implementing client-side pagination in the main leaderboard view. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
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 |
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Comment on lines
+92
to
+123
| const renderRow = ( | ||
| rank: number, | ||
| displayName: string, | ||
| flag: string, | ||
| scoreText: string, | ||
| isUser: boolean, | ||
| medal: string | null, | ||
| ) => ( | ||
| <div | ||
| key={`${displayName}-${rank}`} | ||
| className={`flex items-center gap-4 p-4 rounded-lg transition-colors ${ | ||
| isUser ? "bg-primary/10 border border-primary/20" : "hover:bg-muted/50" | ||
| }`} | ||
| > | ||
| <div className="w-8 text-center font-mono text-sm text-muted-foreground"> | ||
| {rank} | ||
| </div> | ||
| <div className="w-6 text-center"> | ||
| {medal && <span className="text-lg">{medal}</span>} | ||
| </div> | ||
| <div className="flex items-center gap-3 flex-1"> | ||
| <span className="text-lg">{flag}</span> | ||
| <span className="font-medium"> | ||
| {displayName} | ||
| {isUser && <span className="ml-2 text-primary">⭐</span>} | ||
| </span> | ||
| </div> | ||
| <div className="text-right"> | ||
| <div className="font-mono font-medium">{scoreText}</div> | ||
| </div> | ||
| </div> | ||
| ); |
There was a problem hiding this comment.
1. renderrow missing tests 📘 Rule violation ⛯ Reliability
A new renderRow helper was introduced in the leaderboard route as part of the type-safety refactor without adding any automated tests. This increases regression risk for ranking/flag/score rendering across leaderboard modes.
Agent Prompt
## Issue description
A new helper (`renderRow`) and a refactor of leaderboard row rendering were added without automated test coverage.
## Issue Context
This PR is a leaderboard type-safety bug fix. The compliance checklist requires tests for new helpers and regression tests for bug fixes when feasible.
## Fix Focus Areas
- src/routes/_authed/leaderboard.tsx[92-123]
- src/routes/_authed/leaderboard.tsx[213-235]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
46
to
60
| const { data: streakData = [], isLoading: streakLoading } = useQuery( | ||
| convexQuery(api.leaderboard.getStreakLeaderboard, { | ||
| limit, | ||
| convexQuery(api.leaderboard.getStreakLeaderboard, { | ||
| limit, | ||
| offset, | ||
| period: selectedPeriod | ||
| }) | ||
| period: selectedPeriod, | ||
| }), | ||
| ); | ||
|
|
||
| const { data: progressData = [], isLoading: progressLoading } = useQuery( | ||
| convexQuery(api.leaderboard.getProgressLeaderboard, { | ||
| limit, | ||
| convexQuery(api.leaderboard.getProgressLeaderboard, { | ||
| limit, | ||
| offset, | ||
| period: selectedPeriod | ||
| }) | ||
| period: selectedPeriod, | ||
| }), | ||
| ); |
There was a problem hiding this comment.
2. Period toggle ineffective 🐞 Bug ✓ Correctness
The leaderboard page offers Weekly/Monthly/All-time and passes period into Convex queries, but the backend leaderboard queries ignore period, so the UI filter is misleading and results won’t change. Users will see “This Week/This Month” labels without any corresponding data change.
Agent Prompt
### Issue description
The leaderboard UI exposes Weekly/Monthly/All-time, but the backend queries ignore the `period` argument, so changing the period does not change the data.
### Issue Context
Frontend passes `period: selectedPeriod` into Convex queries and displays period labels, so users expect filtering.
### Fix Focus Areas
- src/routes/_authed/leaderboard.tsx[45-68]
- convex/leaderboard.ts[145-156]
- convex/leaderboard.ts[228-235]
- convex/leaderboard.ts[271-279]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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)
src/routes/_authed/leaderboard.tsx (1)
37-289:⚠️ Potential issue | 🟠 MajorMissing test file: Create
leaderboard.test.tsxwith required coverage.The
LeaderboardPagecomponent is missing its test file. Per coding guidelines, components with logic must have corresponding test files following the naming convention. Createsrc/routes/_authed/leaderboard.test.tsxwith coverage for:
- Filter changes (selectedType, selectedPeriod state updates)
- Pagination (currentPage increment/decrement, button disabled states)
- Rank rendering and user highlighting (isCurrentUser logic, medal icons)
- Data loading states and empty state
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/routes/_authed/leaderboard.tsx` around lines 37 - 289, Add a missing test file for the LeaderboardPage component by creating src/routes/_authed/leaderboard.test.tsx and implement unit/interaction tests that cover: updating filters (simulate toggling selectedType and selectedPeriod and assert LeaderboardPage rerenders accordingly), pagination (simulate clicking Next/Previous, assert currentPage changes and button disabled states), rank rendering and user highlighting (verify renderRow output via LeaderboardPage, assert isCurrentUser behavior shows the user highlight and getMedalIcon returns appropriate emojis for ranks 1–3), and loading/empty states (mock the convex queries to return loading, empty arrays, and populated data and assert skeletons and "No users" message). Target the component functions/props named LeaderboardPage, isCurrentUser, getMedalIcon, renderRow and mock api.leaderboard.getStreakLeaderboard/getProgressLeaderboard/getUserRank to achieve required coverage.src/components/dashboard/LeaderboardMiniSection.tsx (1)
60-205:⚠️ Potential issue | 🟠 MajorAdd
LeaderboardMiniSection.test.tsx—this component requires test coverage per guidelines.The component has multiple conditional rendering paths (display-name gate, type toggle between streak/progress data, current-user highlighting, and rank display logic) but no corresponding test file. Per coding guidelines, all components with logic must have a test file following the naming convention
ComponentName.test.tsx.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/dashboard/LeaderboardMiniSection.tsx` around lines 60 - 205, Add a new test file LeaderboardMiniSection.test.tsx that covers the component LeaderboardMiniSection: mock the convex queries (streakData, progressData, userRank, userInfo) and test the four conditional paths — (1) when userInfo.displayName is null show the "Set Display Name" gate and link, (2) toggling selectedType between "streak" and "progress" renders streakData vs progressData rows (verify MiniRow props like rank/displayName/scoreText), (3) ensure top-3 ranks render medal icons via getMedalIcon and (4) when userRank.rank > 5 && <= 50 render the separate current-user rank block showing userInfo.displayName and correct score text; also include a case asserting isCurrentUser highlights the correct row (matching userRank.rank). Use the component name LeaderboardMiniSection, helper MiniRow, and state key selectedType when triggering toggles and name the file exactly LeaderboardMiniSection.test.tsx.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/components/dashboard/LeaderboardMiniSection.tsx`:
- Around line 150-160: The MiniRow is rendering raw fractional progress scores
(user.score) causing UI mismatch with the rounded full leaderboard; update the
mapping in the progressData.map (and the other similar mapping around the
MiniRow usage) to format the score consistently—compute a display string by
rounding or fixing to a consistent number format (e.g., Math.round or
toFixed(0/1)) and pass that formatted value into the MiniRow via scoreText
instead of raw user.score so ProgressEntry/user.score is displayed consistently
across the UI.
In `@src/routes/_authed/leaderboard.tsx`:
- Around line 139-141: When changing leaderboard filters you must reset
pagination to avoid invalid offsets: update the onValueChange handler that calls
setSelectedType and the handler that calls setSelectedPeriod to also call
setCurrentPage(1) (or the first page number used by the component) so
currentPage is reset whenever filters change; reference setSelectedType,
setSelectedPeriod and setCurrentPage to locate and modify the handlers
accordingly.
- Around line 214-223: The map over streakData recomputes rank using
offset+index (streakData.map in this file) which can drift from the
server-provided ranking; change the code to use the StreakEntry.rank field (use
user.rank) when calling renderRow, getMedalIcon and isCurrentUser instead of the
locally computed rank, and ensure isCurrentUser compares against the server rank
(or accept a rank argument) so both medal display and current-user highlighting
use the backend's authoritative rank; update both occurrences where rank is
computed from offset/index and pass user.rank through to
renderRow/getMedalIcon/isCurrentUser.
---
Outside diff comments:
In `@src/components/dashboard/LeaderboardMiniSection.tsx`:
- Around line 60-205: Add a new test file LeaderboardMiniSection.test.tsx that
covers the component LeaderboardMiniSection: mock the convex queries
(streakData, progressData, userRank, userInfo) and test the four conditional
paths — (1) when userInfo.displayName is null show the "Set Display Name" gate
and link, (2) toggling selectedType between "streak" and "progress" renders
streakData vs progressData rows (verify MiniRow props like
rank/displayName/scoreText), (3) ensure top-3 ranks render medal icons via
getMedalIcon and (4) when userRank.rank > 5 && <= 50 render the separate
current-user rank block showing userInfo.displayName and correct score text;
also include a case asserting isCurrentUser highlights the correct row (matching
userRank.rank). Use the component name LeaderboardMiniSection, helper MiniRow,
and state key selectedType when triggering toggles and name the file exactly
LeaderboardMiniSection.test.tsx.
In `@src/routes/_authed/leaderboard.tsx`:
- Around line 37-289: Add a missing test file for the LeaderboardPage component
by creating src/routes/_authed/leaderboard.test.tsx and implement
unit/interaction tests that cover: updating filters (simulate toggling
selectedType and selectedPeriod and assert LeaderboardPage rerenders
accordingly), pagination (simulate clicking Next/Previous, assert currentPage
changes and button disabled states), rank rendering and user highlighting
(verify renderRow output via LeaderboardPage, assert isCurrentUser behavior
shows the user highlight and getMedalIcon returns appropriate emojis for ranks
1–3), and loading/empty states (mock the convex queries to return loading, empty
arrays, and populated data and assert skeletons and "No users" message). Target
the component functions/props named LeaderboardPage, isCurrentUser,
getMedalIcon, renderRow and mock
api.leaderboard.getStreakLeaderboard/getProgressLeaderboard/getUserRank to
achieve required coverage.
ℹ️ Review info
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
src/components/dashboard/LeaderboardMiniSection.tsxsrc/routes/_authed/leaderboard.tsx
📜 Review details
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{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:
src/components/dashboard/LeaderboardMiniSection.tsxsrc/routes/_authed/leaderboard.tsx
**/*.tsx
📄 CodeRabbit inference engine (CLAUDE.md)
Components with logic MUST have corresponding test files following the naming convention
ComponentName.test.tsx
Files:
src/components/dashboard/LeaderboardMiniSection.tsxsrc/routes/_authed/leaderboard.tsx
🧠 Learnings (6)
📓 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
📚 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:
src/components/dashboard/LeaderboardMiniSection.tsxsrc/routes/_authed/leaderboard.tsx
📚 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:
src/components/dashboard/LeaderboardMiniSection.tsxsrc/routes/_authed/leaderboard.tsx
📚 Learning: 2026-01-23T18:12:38.519Z
Learnt from: CR
Repo: EtanHey/songscript PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-23T18:12:38.519Z
Learning: Applies to convex/*.ts : Use Convex for database queries, mutations, and authentication configuration
Applied to files:
src/routes/_authed/leaderboard.tsx
📚 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: Complete all backend convex/ queries before implementing dashboard/page components
Applied to files:
src/routes/_authed/leaderboard.tsx
📚 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:
src/routes/_authed/leaderboard.tsx
🧬 Code graph analysis (2)
src/components/dashboard/LeaderboardMiniSection.tsx (1)
src/components/dashboard/index.tsx (1)
LeaderboardMiniSection(65-65)
src/routes/_authed/leaderboard.tsx (2)
src/components/ui/card.tsx (3)
CardTitle(88-88)CardHeader(86-86)CardContent(91-91)src/components/LanguageFlag.tsx (1)
getLanguageFlagString(75-80)
Comment on lines
+150
to
+160
| : progressData.map((user: ProgressEntry) => { | ||
| const isCurrentUser = | ||
| hasDisplayName && userRank && user.rank === userRank.rank; | ||
| const medal = getMedalIcon(user.rank); | ||
| return ( | ||
| <MiniRow | ||
| key={`${user.displayName}-${user.rank}`} | ||
| rank={user.rank} | ||
| displayName={user.displayName} | ||
| scoreText={`${user.score || 0} pts`} | ||
| medal={medal} |
There was a problem hiding this comment.
Normalize progress score formatting to avoid raw fractional output drift.
Progress scores can be fractional; this mini section currently renders raw values while the full leaderboard rounds them. Keep formatting consistent to avoid UI mismatch.
💡 Proposed fix
- scoreText={`${user.score || 0} pts`}
+ scoreText={`${Math.round(user.score || 0)} pts`}
@@
- : `${userRank.score} pts`}
+ : `${Math.round(userRank.score)} pts`}Based on learnings, progress score uses (words_learned × multiplier) + (lines_completed × multiplier × 0.5), so fractional values are expected and should be formatted consistently.
Also applies to: 186-188
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/dashboard/LeaderboardMiniSection.tsx` around lines 150 - 160,
The MiniRow is rendering raw fractional progress scores (user.score) causing UI
mismatch with the rounded full leaderboard; update the mapping in the
progressData.map (and the other similar mapping around the MiniRow usage) to
format the score consistently—compute a display string by rounding or fixing to
a consistent number format (e.g., Math.round or toFixed(0/1)) and pass that
formatted value into the MiniRow via scoreText instead of raw user.score so
ProgressEntry/user.score is displayed consistently across the UI.
Comment on lines
+139
to
+141
| onValueChange={(value) => | ||
| value && setSelectedType(value as LeaderboardType) | ||
| } |
There was a problem hiding this comment.
Reset pagination when leaderboard filters change.
Line 139 and Line 158 handlers change type/period but keep currentPage. If the user is on a later page, switching filters can land on invalid offsets and show misleading empty results.
💡 Proposed fix
- onValueChange={(value) =>
- value && setSelectedType(value as LeaderboardType)
- }
+ onValueChange={(value) => {
+ if (!value) return;
+ setSelectedType(value as LeaderboardType);
+ setCurrentPage(0);
+ }}
@@
- onValueChange={(value) =>
- value && setSelectedPeriod(value as TimePeriod)
- }
+ onValueChange={(value) => {
+ if (!value) return;
+ setSelectedPeriod(value as TimePeriod);
+ setCurrentPage(0);
+ }}Also applies to: 158-160
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/routes/_authed/leaderboard.tsx` around lines 139 - 141, When changing
leaderboard filters you must reset pagination to avoid invalid offsets: update
the onValueChange handler that calls setSelectedType and the handler that calls
setSelectedPeriod to also call setCurrentPage(1) (or the first page number used
by the component) so currentPage is reset whenever filters change; reference
setSelectedType, setSelectedPeriod and setCurrentPage to locate and modify the
handlers accordingly.
Comment on lines
+214
to
+223
| ? streakData.map((user: StreakEntry, index: number) => { | ||
| const rank = offset + index + 1; | ||
| return renderRow( | ||
| rank, | ||
| user.displayName, | ||
| getLanguageFlagString(user.language), | ||
| `${user.streak} days`, | ||
| isCurrentUser(rank), | ||
| getMedalIcon(rank), | ||
| ); |
There was a problem hiding this comment.
Use backend user.rank instead of index-derived rank.
Line 215 and Line 226 recompute rank from page offset/index even though entries include rank. That can drift from server ranking rules and break medal/current-user consistency.
💡 Proposed fix
- ? streakData.map((user: StreakEntry, index: number) => {
- const rank = offset + index + 1;
+ ? streakData.map((user: StreakEntry) => {
return renderRow(
- rank,
+ user.rank,
user.displayName,
getLanguageFlagString(user.language),
`${user.streak} days`,
- isCurrentUser(rank),
- getMedalIcon(rank),
+ isCurrentUser(user.rank),
+ getMedalIcon(user.rank),
);
})
- : progressData.map((user: ProgressEntry, index: number) => {
- const rank = offset + index + 1;
+ : progressData.map((user: ProgressEntry) => {
return renderRow(
- rank,
+ user.rank,
user.displayName,
getLanguageFlagString(user.topLanguage),
`${Math.round(user.score)} pts`,
- isCurrentUser(rank),
- getMedalIcon(rank),
+ isCurrentUser(user.rank),
+ getMedalIcon(user.rank),
);
})}Also applies to: 225-234
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/routes/_authed/leaderboard.tsx` around lines 214 - 223, The map over
streakData recomputes rank using offset+index (streakData.map in this file)
which can drift from the server-provided ranking; change the code to use the
StreakEntry.rank field (use user.rank) when calling renderRow, getMedalIcon and
isCurrentUser instead of the locally computed rank, and ensure isCurrentUser
compares against the server rank (or accept a rank argument) so both medal
display and current-user highlighting use the backend's authoritative rank;
update both occurrences where rank is computed from offset/index and pass
user.rank through to renderRow/getMedalIcon/isCurrentUser.
Owner
Author
|
Closing: Night Shift launchd agent permanently disabled on 2026-03-12. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Automated improvement by Golems Night Shift.
fix leaderboard type safety (6 as-any)
Summary by CodeRabbit
Release Notes