feat: Leaderboard privacy toggle, 60s cache, and own-rank display (closes #13)#64
Conversation
|
Someone is attempting to deploy a commit to the rishabhjtripathi2903-3434's projects Team on Vercel. A member of the Team first needs to authorize it. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
🚧 Files skipped from review as they are similar to previous changes (5)
📝 WalkthroughWalkthroughThis PR adds an ChangesLeaderboard privacy and caching
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Leaderboard
participant userRoutes
participant userController
participant Database
Leaderboard->>userRoutes: GET /api/users/leaderboard/me
userRoutes->>userController: protect middleware, invoke handler
userController->>Database: aggregate public users above current user
Database-->>userController: above count
userController-->>Leaderboard: rank, xp, streak, solved, avatar
Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
backend/src/controllers/userController.ts (1)
248-254: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winStale public leaderboard for up to 60s after a privacy opt-out.
updateUserProfilepersistsisPublicbut never invalidatesleaderboardCache. Since the cache isn't keyed per-user, a user who just setisPublic: falsecan still appear in the publicly served (cached) leaderboard for up to 60 seconds — undermining the privacy guarantee this PR is meant to deliver.🔒 Proposed fix: clear the leaderboard cache on visibility change
const updatedUser = await prisma.user.update({ where: { id: userId }, data: { bio: bio !== undefined ? bio : user.bio, avatarUrl: avatarUrl !== undefined ? avatarUrl : user.avatarUrl, isPublic: isPublic !== undefined ? Boolean(isPublic) : user.isPublic, }, }); + + if (isPublic !== undefined && Boolean(isPublic) !== user.isPublic) { + leaderboardCache.clear(); + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/src/controllers/userController.ts` around lines 248 - 254, The `updateUserProfile` flow updates `isPublic` but leaves `leaderboardCache` intact, so a user who opts out can still be served from the cached public leaderboard. In `userController.ts`, after the `prisma.user.update` in `updateUserProfile`, detect when `isPublic` changes and invalidate or clear `leaderboardCache` so the next leaderboard request rebuilds it with the updated visibility state.
🧹 Nitpick comments (5)
backend/src/controllers/userController.ts (1)
253-253: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick win
Boolean("false")coerces totrue.If
isPublicever arrives as the string"false"(vs. a real boolean),Boolean(isPublic)evaluates totrue, silently un-hiding a user who intended to opt out. Safe as long as the client always sends a JSON boolean, but worth guarding explicitly given this controls a privacy setting.🛡️ Defensive coercion
- isPublic: isPublic !== undefined ? Boolean(isPublic) : user.isPublic, + isPublic: isPublic !== undefined + ? (typeof isPublic === 'string' ? isPublic === 'true' : Boolean(isPublic)) + : user.isPublic,🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/src/controllers/userController.ts` at line 253, The `updateUser`/user visibility assignment is coercing any truthy string to `true`, so `"false"` can incorrectly enable `isPublic`. Update the `isPublic` handling in `userController.ts` to explicitly accept real booleans (and, if needed, normalize string inputs like `"true"`/`"false"` safely) instead of using `Boolean(isPublic)`, while still falling back to `user.isPublic` when the field is absent.app/src/sections/ProfileView.tsx (2)
251-275: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueToggle state not exposed to assistive tech.
The switch conveys on/off purely through color and icon. Adding
aria-pressed={isPublic}(and optionallyrole="switch") on the button would communicate state to screen readers.♻️ Proposed addition
<button onClick={handleTogglePrivacy} + aria-pressed={isPublic} className="w-full flex items-center justify-between px-4 py-3 rounded-xl bg-white/5 border border-white/10 hover:bg-white/10 transition-colors" >🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/src/sections/ProfileView.tsx` around lines 251 - 275, The privacy toggle button in ProfileView currently exposes its on/off state only through color and icons, so screen readers can’t tell whether it’s enabled. Update the existing button that uses handleTogglePrivacy to expose the state with aria-pressed={isPublic}, and if appropriate make it behave as a switch by adding role="switch" and the matching checked state semantics while keeping the current visual UI unchanged.
98-117: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueNo guard against rapid repeated toggling.
Clicking the toggle multiple times in quick succession fires overlapping PUT requests with no in-flight guard, so responses can resolve out of order and leave
isPublicinconsistent with the last user intent. Consider tracking a pending flag and disabling the button while a request is outstanding (similar tosavinginhandleSave).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/src/sections/ProfileView.tsx` around lines 98 - 117, The handleTogglePrivacy flow in ProfileView allows overlapping PUT requests because it has no in-flight guard, which can leave isPublic out of sync with the latest click. Add a pending state alongside handleSave’s saving pattern, set it before the fetch and clear it in finally, and use it to disable the privacy toggle while the request is outstanding. Keep the optimistic update/revert behavior in handleTogglePrivacy, but prevent a new toggle until the current request finishes.app/src/sections/Leaderboard.tsx (2)
46-66: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winStale response can overwrite fresher rank on rapid category switching.
No abort/ignore-stale-response guard. If
categorychanges quickly, an earlier in-flight request for the old category can resolve after a newer one and overwritemyRankwith stale data.🔒️ Guard against out-of-order responses
useEffect(() => { if (!profile) return; + let isCurrent = true; const fetchMyRank = async () => { try { const token = localStorage.getItem('token'); if (!token) return; const res = await fetch(`${API_BASE_URL}/api/users/leaderboard/me?sortBy=${category}`, { headers: { Authorization: `Bearer ${token}` }, }); - if (res.ok) { + if (res.ok && isCurrent) { const data = await res.json(); setMyRank(data); } } catch { // Silently ignore — own rank is best-effort } }; fetchMyRank(); + return () => { isCurrent = false; }; }, [profile, category]);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/src/sections/Leaderboard.tsx` around lines 46 - 66, The fetchMyRank effect in Leaderboard.tsx can apply stale results when category changes quickly, so add an out-of-order response guard around the existing useEffect. Use a local cancellation flag or AbortController inside the effect and ensure setMyRank only runs for the latest request, keeping the current profile/category state in the dependency list and ignoring any completed request from an older category.
20-20: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUnused
tokenfromuseAuth(); duplicate read fromlocalStorageinstead.
tokenis destructured fromuseAuth()at Line 20 but never used — the new effect re-reads it directly vialocalStorage.getItem('token')(Line 51), shadowing the outer variable. This duplicates the auth source of truth and can drift if context state and storage ever diverge (e.g., logout clearing context state before storage, or a future storage-key change).♻️ Use the already-destructured token
useEffect(() => { if (!profile) return; const fetchMyRank = async () => { try { - const token = localStorage.getItem('token'); if (!token) return; const res = await fetch(`${API_BASE_URL}/api/users/leaderboard/me?sortBy=${category}`, { headers: { Authorization: `Bearer ${token}` }, }); if (res.ok) { const data = await res.json(); setMyRank(data); } } catch { // Silently ignore — own rank is best-effort } }; fetchMyRank(); - }, [profile, category]); + }, [profile, category, token]);Also applies to: 46-66
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/src/sections/Leaderboard.tsx` at line 20, The Leaderboard component is re-reading auth state from localStorage even though useAuth() already provides token, leaving the destructured token unused and duplicating the source of truth. Update the effect and any related logic in Leaderboard to use the token from useAuth() directly instead of calling localStorage.getItem('token'), and remove the shadowing local variable so the auth flow stays consistent with profile/token from useAuth().
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/src/sections/ProfileView.tsx`:
- Around line 98-117: The optimistic privacy toggle in handleTogglePrivacy only
rolls back on thrown errors, so non-2xx fetch responses from the profile PUT
request can leave isPublic out of sync with the server. Update the logic in
ProfileView.tsx to inspect the response from fetch for the
/api/users/${userId}/profile call, treat HTTP error statuses as failures, and
revert setIsPublic when the request is not successful. Keep the rollback
behavior tied to handleTogglePrivacy so the UI reflects the backend result.
In `@backend/src/controllers/userController.ts`:
- Around line 4-17: Validate and clamp the raw query inputs used by the
leaderboard flow before building the cache key in userController’s
leaderboard-related logic: limit should be parsed safely, checked for NaN, and
bounded to an allowed range, and sortBy should be restricted to known values.
Update the cacheKey generation and the aggregation setup so they use sanitized
values only, and add a size cap or eviction strategy to leaderboardCache (used
by getCachedLeaderboard/setCachedLeaderboard) so varying query params cannot
grow the Map without bound.
---
Outside diff comments:
In `@backend/src/controllers/userController.ts`:
- Around line 248-254: The `updateUserProfile` flow updates `isPublic` but
leaves `leaderboardCache` intact, so a user who opts out can still be served
from the cached public leaderboard. In `userController.ts`, after the
`prisma.user.update` in `updateUserProfile`, detect when `isPublic` changes and
invalidate or clear `leaderboardCache` so the next leaderboard request rebuilds
it with the updated visibility state.
---
Nitpick comments:
In `@app/src/sections/Leaderboard.tsx`:
- Around line 46-66: The fetchMyRank effect in Leaderboard.tsx can apply stale
results when category changes quickly, so add an out-of-order response guard
around the existing useEffect. Use a local cancellation flag or AbortController
inside the effect and ensure setMyRank only runs for the latest request, keeping
the current profile/category state in the dependency list and ignoring any
completed request from an older category.
- Line 20: The Leaderboard component is re-reading auth state from localStorage
even though useAuth() already provides token, leaving the destructured token
unused and duplicating the source of truth. Update the effect and any related
logic in Leaderboard to use the token from useAuth() directly instead of calling
localStorage.getItem('token'), and remove the shadowing local variable so the
auth flow stays consistent with profile/token from useAuth().
In `@app/src/sections/ProfileView.tsx`:
- Around line 251-275: The privacy toggle button in ProfileView currently
exposes its on/off state only through color and icons, so screen readers can’t
tell whether it’s enabled. Update the existing button that uses
handleTogglePrivacy to expose the state with aria-pressed={isPublic}, and if
appropriate make it behave as a switch by adding role="switch" and the matching
checked state semantics while keeping the current visual UI unchanged.
- Around line 98-117: The handleTogglePrivacy flow in ProfileView allows
overlapping PUT requests because it has no in-flight guard, which can leave
isPublic out of sync with the latest click. Add a pending state alongside
handleSave’s saving pattern, set it before the fetch and clear it in finally,
and use it to disable the privacy toggle while the request is outstanding. Keep
the optimistic update/revert behavior in handleTogglePrivacy, but prevent a new
toggle until the current request finishes.
In `@backend/src/controllers/userController.ts`:
- Line 253: The `updateUser`/user visibility assignment is coercing any truthy
string to `true`, so `"false"` can incorrectly enable `isPublic`. Update the
`isPublic` handling in `userController.ts` to explicitly accept real booleans
(and, if needed, normalize string inputs like `"true"`/`"false"` safely) instead
of using `Boolean(isPublic)`, while still falling back to `user.isPublic` when
the field is absent.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 1df9841b-01a8-4a62-9f93-f91e69eca7da
📒 Files selected for processing (5)
app/src/sections/Leaderboard.tsxapp/src/sections/ProfileView.tsxbackend/prisma/schema.prismabackend/src/controllers/userController.tsbackend/src/routes/userRoutes.ts
| const handleTogglePrivacy = async () => { | ||
| if (!profile) return; | ||
| const newValue = !isPublic; | ||
| setIsPublic(newValue); | ||
| try { | ||
| const token = localStorage.getItem('token'); | ||
| await fetch(`${API_BASE_URL}/api/users/${userId}/profile`, { | ||
| method: 'PUT', | ||
| headers: { | ||
| 'Content-Type': 'application/json', | ||
| Authorization: `Bearer ${token}`, | ||
| }, | ||
| body: JSON.stringify({ isPublic: newValue }), | ||
| }); | ||
| } catch { | ||
| // Revert on error | ||
| setIsPublic(!newValue); | ||
| } | ||
| }; | ||
|
|
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Optimistic toggle doesn't roll back on HTTP error responses.
handleTogglePrivacy only reverts isPublic inside the catch block, which only fires on network failure. The backend's updateUserProfile can return 403 (wrong user), 404, or 500 (per backend/src/controllers/userController.ts) without fetch throwing — in those cases the optimistic state is never reverted, so the UI silently shows a privacy state the server never persisted.
🐛 Proposed fix
const handleTogglePrivacy = async () => {
if (!profile) return;
const newValue = !isPublic;
setIsPublic(newValue);
try {
const token = localStorage.getItem('token');
- await fetch(`${API_BASE_URL}/api/users/${userId}/profile`, {
+ const res = await fetch(`${API_BASE_URL}/api/users/${userId}/profile`, {
method: 'PUT',
headers: {
'Content-Type': 'application/json',
Authorization: `Bearer ${token}`,
},
body: JSON.stringify({ isPublic: newValue }),
});
+ if (!res.ok) {
+ setIsPublic(!newValue);
+ }
} catch {
// Revert on error
setIsPublic(!newValue);
}
};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const handleTogglePrivacy = async () => { | |
| if (!profile) return; | |
| const newValue = !isPublic; | |
| setIsPublic(newValue); | |
| try { | |
| const token = localStorage.getItem('token'); | |
| await fetch(`${API_BASE_URL}/api/users/${userId}/profile`, { | |
| method: 'PUT', | |
| headers: { | |
| 'Content-Type': 'application/json', | |
| Authorization: `Bearer ${token}`, | |
| }, | |
| body: JSON.stringify({ isPublic: newValue }), | |
| }); | |
| } catch { | |
| // Revert on error | |
| setIsPublic(!newValue); | |
| } | |
| }; | |
| const handleTogglePrivacy = async () => { | |
| if (!profile) return; | |
| const newValue = !isPublic; | |
| setIsPublic(newValue); | |
| try { | |
| const token = localStorage.getItem('token'); | |
| const res = await fetch(`${API_BASE_URL}/api/users/${userId}/profile`, { | |
| method: 'PUT', | |
| headers: { | |
| 'Content-Type': 'application/json', | |
| Authorization: `Bearer ${token}`, | |
| }, | |
| body: JSON.stringify({ isPublic: newValue }), | |
| }); | |
| if (!res.ok) { | |
| setIsPublic(!newValue); | |
| } | |
| } catch { | |
| // Revert on error | |
| setIsPublic(!newValue); | |
| } | |
| }; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@app/src/sections/ProfileView.tsx` around lines 98 - 117, The optimistic
privacy toggle in handleTogglePrivacy only rolls back on thrown errors, so
non-2xx fetch responses from the profile PUT request can leave isPublic out of
sync with the server. Update the logic in ProfileView.tsx to inspect the
response from fetch for the /api/users/${userId}/profile call, treat HTTP error
statuses as failures, and revert setIsPublic when the request is not successful.
Keep the rollback behavior tied to handleTogglePrivacy so the UI reflects the
backend result.
| // ─── In-memory leaderboard cache (60s TTL) ─── | ||
| const leaderboardCache = new Map<string, { data: any; expiresAt: number }>(); | ||
| const CACHE_TTL_MS = 60_000; | ||
|
|
||
| function getCachedLeaderboard(key: string) { | ||
| const entry = leaderboardCache.get(key); | ||
| if (entry && Date.now() < entry.expiresAt) return entry.data; | ||
| leaderboardCache.delete(key); | ||
| return null; | ||
| } | ||
|
|
||
| function setCachedLeaderboard(key: string, data: any) { | ||
| leaderboardCache.set(key, { data, expiresAt: Date.now() + CACHE_TTL_MS }); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Unbounded cache growth from unvalidated query params.
cacheKey = lb:${sortBy}:${limit} is built directly from raw, unvalidated query-string input. A client varying limit (or sortBy) across many values can mint unlimited distinct entries in leaderboardCache, each retained for 60s with no cap on map size — unbounded memory growth (and a large/garbage limit also inflates the $limit aggregation stage, since Number(limit) isn't clamped or validated against NaN).
🛡️ Proposed fix: validate/clamp inputs and bound cache size
- const { limit = 10, sortBy = 'xp' } = req.query;
- const cacheKey = `lb:${sortBy}:${limit}`;
+ const allowedSortBy = ['xp', 'solved', 'streak'];
+ const sortBy = allowedSortBy.includes(req.query.sortBy as string) ? req.query.sortBy as string : 'xp';
+ const limit = Math.min(Math.max(Number(req.query.limit) || 10, 1), 100);
+ const cacheKey = `lb:${sortBy}:${limit}`;Also applies to: 24-31, 59-59
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/src/controllers/userController.ts` around lines 4 - 17, Validate and
clamp the raw query inputs used by the leaderboard flow before building the
cache key in userController’s leaderboard-related logic: limit should be parsed
safely, checked for NaN, and bounded to an allowed range, and sortBy should be
restricted to known values. Update the cacheKey generation and the aggregation
setup so they use sanitized values only, and add a size cap or eviction strategy
to leaderboardCache (used by getCachedLeaderboard/setCachedLeaderboard) so
varying query params cannot grow the Map without bound.
rishabhx29
added
documentation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
arcgod-design
force-pushed
the
feat/issue-13-leaderboard-privacy
branch
from
b8ec3b1 to
58daf33
Compare
…line (closes type error TS2561)
|
Hey @Rishabhworkspace, this PR has been ready for review for a while -- offering this friendly bump. The implementation matches the issue's specified behavior and CI is passing. PR: #64 When you have a moment, a review would be much appreciated. I am happy to iterate on any feedback. If the timing is not right, no worries at all -- just a gentle nudge from a contributor who has been waiting patiently. � |
2 participants
Related Issue
Closes #13
Description
Adds leaderboard privacy control and performance caching. Users can now hide themselves from the public leaderboard via a toggle in their profile. The leaderboard API response is cached for 60 seconds in-memory. Authenticated users always see their own rank below the leaderboard, even if hidden from the public list.
Type of Change
Screenshots
A "Leaderboard Visibility" toggle with eye icon appears on the user's own profile page. The leaderboard "Your Rank" card now shows the actual rank number instead of
#?.Checklist
Summary by CodeRabbit