feat: implement leaderboard page with infinite scrolling, filter pers… - #192
Closed
TheBigWealth89 wants to merge 2 commits into
Closed
TheBigWealth89 wants to merge 2 commits into
TheBigWealth89 wants to merge 2 commits into
Conversation
…istence, and user rank tracking
|
@TheBigWealth89 is attempting to deploy a commit to the Threadflow Team on Vercel. A member of the Team first needs to authorize it. |
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 55 minutes and 54 seconds. ⌛ 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. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe PR adds a GraphQL API endpoint for leaderboard queries, refactors leaderboard components to authenticate users via session, implements auto-scrolling to the current user's table row, and introduces centralized route definitions throughout the application. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Page as Leaderboard Page
participant Auth as Auth Session
participant GraphQL as GraphQL API
participant MockData as Mock Helpers
Client->>Page: Load leaderboard
Page->>Auth: Validate session
alt Session pending
Auth-->>Page: Session resolving
Page->>Page: Suppress userId (undefined)
Page->>Client: Render skeleton
else Session resolved
Auth-->>Page: userId (or null)
Page->>GraphQL: POST leaderboard query
GraphQL->>MockData: getMockLeaderboard(page, limit, tier)
MockData-->>GraphQL: Ranked entries + totalCount
GraphQL-->>Page: data.leaderboard response
alt User found in entries
Page->>Page: Scroll to user's row
Page->>Client: Render table with highlight
else User not in entries
Page->>Client: Render table without scroll
end
end
sequenceDiagram
participant Client
participant Sidebar as User Rank Sidebar
participant Auth as Auth Session
participant GraphQL as GraphQL API
participant MockData as Mock Helpers
Client->>Sidebar: Render sidebar
Sidebar->>Auth: Check session status
alt isSessionPending === true
Auth-->>Sidebar: Session loading
Sidebar->>Client: Show skeleton
else No userId
Auth-->>Sidebar: Unauthenticated
Sidebar->>Client: Show sign-in CTA
else userId exists
Sidebar->>GraphQL: POST userLeaderboardRank query
GraphQL->>MockData: getMockUserRank(userId)
MockData-->>GraphQL: User rank data
GraphQL-->>Sidebar: data.userLeaderboardRank
Sidebar->>Client: Render rank card
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (4)
app/api/graphql/route.ts (1)
11-16:filtersother thantierare silently dropped.The leaderboard hook may grow
filters(search, time range, etc.) but this mock only forwardstiertogetMockLeaderboard. That's fine for now sincegetMockLeaderboardonly supportsfilterTier, but consumers debugging "why doesn't my filter work?" will have a hard time. Consider either narrowing the destructure to make the contract explicit, or adding a brief comment noting that onlytieris honored by the mock.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/api/graphql/route.ts` around lines 11 - 16, The current destructuring pulls filters and pagination but silently ignores any filter keys other than tier when calling getMockLeaderboard (variables -> { filters, pagination } -> tier), which can confuse callers; either narrow the destructure to explicitly extract only tier from filters (e.g., pull tier from filters directly) so the contract is explicit, or add a concise inline comment above the call to getMockLeaderboard stating that the mock only honors the tier filter today; update uses of variables, page, limit, and tier around the getMockLeaderboard invocation accordingly.lib/routes.ts (1)
5-14: LGTM — consider migrating remaining hardcoded routes in a follow-up.The
ROUTESshape is clean andas constpreserves literal types. Several other call sites still hardcode/authand/leaderboard(e.g.,components/global-navbar.tsx,components/leaderboard/mini-leaderboard.tsx,components/ui/global-resizable-navbar.tsx,app/(auth)/auth/magic-link/verify/page.tsx); migrating those toROUTES.AUTH/ROUTES.LEADERBOARDin a follow-up will complete the centralization.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/routes.ts` around lines 5 - 14, Several files still use hardcoded route strings; replace literal "/auth" and "/leaderboard" occurrences with the centralized constants ROUTES.AUTH and ROUTES.LEADERBOARD respectively. Update references in components/global-navbar.tsx, components/leaderboard/mini-leaderboard.tsx, components/ui/global-resizable-navbar.tsx and app/(auth)/auth/magic-link/verify/page.tsx to import ROUTES from lib/routes and use ROUTES.AUTH or ROUTES.LEADERBOARD (or ROUTES.PROFILE(userId) where applicable) so all routing strings are centralized and type-safe.components/leaderboard/leaderboard-table.tsx (1)
166-168: Redundantrole="row"onTableRow.
<TableRow>already renders a<tr>with the implicitrole="row", so the explicit attribute is a no-op. If the intent of overriding the role was to communicate the row's clickability,role="row"doesn't convey that — a screen-reader-friendly approach is to keep the implicit row semantics and rely on the existingtabIndex/onKeyDown/aria-current, or render an inner<button>for the activation target. Easiest fix is to drop the prop.🧹 Suggested cleanup
tabIndex={onRowClick ? 0 : undefined} - role={onRowClick ? "row" : undefined} onClick={onRowClick ? () => onRowClick(entry) : undefined}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/leaderboard/leaderboard-table.tsx` around lines 166 - 168, Remove the redundant role="row" prop from the TableRow render (the implicit <tr> already has that role); keep the existing conditional tabIndex and onClick behavior tied to onRowClick and, if needed for accessibility, ensure keyboard activation via the component's onKeyDown or by rendering an inner <button> for the interactive target instead of overriding role. Target the TableRow element using the TableRow render block where tabIndex={onRowClick ? 0 : undefined}, role={...}, and onClick={...} and simply drop the role prop.components/leaderboard/user-rank-sidebar.tsx (1)
199-227: TODO acknowledged — track removal of mock thresholds before launch.The fallback math (
currentTierPoints ?? totalScoreandnextTierThreshold ?? totalScore * 1.5) is reasonable as a placeholder, buttotalScore * 1.5will always render a non-empty progress bar at ~67% for any logged-in user, which can be misleading. Consider hiding the progress block entirely when the API hasn't supplied real thresholds, or returningnullfrom the IIFE when the API fields are missing.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/leaderboard/user-rank-sidebar.tsx` around lines 199 - 227, The progress bar should be hidden when the API hasn't provided real threshold data; update the IIFE so it returns null unless contributor.stats.nextTierThreshold is present (i.e., only render the Progress block when contributor.stats.nextTierThreshold != null), remove the fallback that uses contributor.totalScore * 1.5 for nextTierThreshold, and keep using contributor.stats.currentTierPoints ?? contributor.totalScore to compute progressPercent against the real nextTierThreshold; reference contributor.stats.nextTierThreshold, contributor.stats.currentTierPoints, contributor.totalScore, and progressPercent to locate where to change the logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/api/graphql/route.ts`:
- Around line 6-10: The handler reads body and then does query.includes(...),
which throws if query is missing or not a string; update the GraphQL route
handler to validate that query exists and is a string before calling .includes
(e.g., check typeof query === "string"), and if the check fails return a 400 Bad
Request with a clear error message; keep the existing logic for handling
leaderboard queries (the same includes checks) only after the validation passes
so malformed requests no longer produce a 500.
- Around line 10-43: The current routing based on query.includes(...) is brittle
and misorders branches; update the handler to derive the GraphQL operation name
once (e.g., extract the operation name from the `query` string via a simple
regex like /^\s*(query|mutation)\s+([A-Za-z0-9_]+)/ or by using a lightweight
parser) and switch on that name instead of substring checks, then route to
getMockUserRank(userId) when the operation name === "UserLeaderboardRank" and to
getMockLeaderboard(page, limit, tier) when the operation name === "Leaderboard";
alternatively, if you want the smallest change, simply check the more specific
`UserLeaderboardRank` branch (calling getMockUserRank) before the broader
`leaderboard` branch to avoid accidental matches.
In `@components/leaderboard/leaderboard-table.tsx`:
- Around line 70-96: The effect currently sets hasScrolledRef.current = true and
never resets it, so after the first auto-scroll future filter changes (which
produce new entries) won't re-trigger scrolling; update the component to reset
hasScrolledRef.current to false whenever the filter-driven entry set changes
(e.g., when timeframe, tier, or tags change or when entries identity changes) so
the useEffect that reads hasScrolledRef, currentUserId, currentUserRowRef and
entries can run the scroll logic again; locate where filters or entries are
updated and set hasScrolledRef.current = false there (or include the specific
filter values in the useEffect dependency array and reset the ref at the start).
In `@components/leaderboard/user-rank-sidebar.tsx`:
- Around line 80-85: The current JSX nests a <Button> inside a <Link> which
creates invalid HTML (<a><button>) — change to use the shadcn asChild pattern by
rendering the Link as the Button's child: wrap the Link (href={ROUTES.AUTH})
inside <Button asChild variant="default" size="sm" className="w-full"> so the
Button forwards its styling/behavior to the anchor element and you end up with a
single interactive <a> element; preserve the w-full classes and Sign In label
when moving Link inside Button.
---
Nitpick comments:
In `@app/api/graphql/route.ts`:
- Around line 11-16: The current destructuring pulls filters and pagination but
silently ignores any filter keys other than tier when calling getMockLeaderboard
(variables -> { filters, pagination } -> tier), which can confuse callers;
either narrow the destructure to explicitly extract only tier from filters
(e.g., pull tier from filters directly) so the contract is explicit, or add a
concise inline comment above the call to getMockLeaderboard stating that the
mock only honors the tier filter today; update uses of variables, page, limit,
and tier around the getMockLeaderboard invocation accordingly.
In `@components/leaderboard/leaderboard-table.tsx`:
- Around line 166-168: Remove the redundant role="row" prop from the TableRow
render (the implicit <tr> already has that role); keep the existing conditional
tabIndex and onClick behavior tied to onRowClick and, if needed for
accessibility, ensure keyboard activation via the component's onKeyDown or by
rendering an inner <button> for the interactive target instead of overriding
role. Target the TableRow element using the TableRow render block where
tabIndex={onRowClick ? 0 : undefined}, role={...}, and onClick={...} and simply
drop the role prop.
In `@components/leaderboard/user-rank-sidebar.tsx`:
- Around line 199-227: The progress bar should be hidden when the API hasn't
provided real threshold data; update the IIFE so it returns null unless
contributor.stats.nextTierThreshold is present (i.e., only render the Progress
block when contributor.stats.nextTierThreshold != null), remove the fallback
that uses contributor.totalScore * 1.5 for nextTierThreshold, and keep using
contributor.stats.currentTierPoints ?? contributor.totalScore to compute
progressPercent against the real nextTierThreshold; reference
contributor.stats.nextTierThreshold, contributor.stats.currentTierPoints,
contributor.totalScore, and progressPercent to locate where to change the logic.
In `@lib/routes.ts`:
- Around line 5-14: Several files still use hardcoded route strings; replace
literal "/auth" and "/leaderboard" occurrences with the centralized constants
ROUTES.AUTH and ROUTES.LEADERBOARD respectively. Update references in
components/global-navbar.tsx, components/leaderboard/mini-leaderboard.tsx,
components/ui/global-resizable-navbar.tsx and
app/(auth)/auth/magic-link/verify/page.tsx to import ROUTES from lib/routes and
use ROUTES.AUTH or ROUTES.LEADERBOARD (or ROUTES.PROFILE(userId) where
applicable) so all routing strings are centralized and type-safe.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 42f4cbc5-444d-4bbe-9d2b-928ba70e666a
📒 Files selected for processing (5)
app/api/graphql/route.tsapp/leaderboard/page.tsxcomponents/leaderboard/leaderboard-table.tsxcomponents/leaderboard/user-rank-sidebar.tsxlib/routes.ts
Comment on lines
+6
to
+10
| const body = await request.json(); | ||
| const { query, variables } = body; | ||
|
|
||
| // Simple mock GraphQL server handling the leaderboard queries | ||
| if (query.includes("query Leaderboard") || query.includes("leaderboard")) { |
There was a problem hiding this comment.
Validate query shape before calling .includes.
If a client sends a body without a query field (or with a non-string query), line 10 throws TypeError: Cannot read properties of undefined (reading 'includes'), which is caught and surfaced as a generic 500. A malformed request should be a 400.
🛡️ Proposed fix
const body = await request.json();
const { query, variables } = body;
+
+ if (typeof query !== "string") {
+ return NextResponse.json(
+ { error: "Missing or invalid 'query' field" },
+ { status: 400 },
+ );
+ }📝 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.
Suggested change
| const body = await request.json(); | |
| const { query, variables } = body; | |
| // Simple mock GraphQL server handling the leaderboard queries | |
| if (query.includes("query Leaderboard") || query.includes("leaderboard")) { | |
| const body = await request.json(); | |
| const { query, variables } = body; | |
| if (typeof query !== "string") { | |
| return NextResponse.json( | |
| { error: "Missing or invalid 'query' field" }, | |
| { status: 400 }, | |
| ); | |
| } | |
| // Simple mock GraphQL server handling the leaderboard queries | |
| if (query.includes("query Leaderboard") || query.includes("leaderboard")) { |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/api/graphql/route.ts` around lines 6 - 10, The handler reads body and
then does query.includes(...), which throws if query is missing or not a string;
update the GraphQL route handler to validate that query exists and is a string
before calling .includes (e.g., check typeof query === "string"), and if the
check fails return a 400 Bad Request with a clear error message; keep the
existing logic for handling leaderboard queries (the same includes checks) only
after the validation passes so malformed requests no longer produce a 500.
Comment on lines
+10
to
+43
| if (query.includes("query Leaderboard") || query.includes("leaderboard")) { | ||
| const { filters, pagination } = variables || {}; | ||
| const page = pagination?.page || 1; | ||
| const limit = pagination?.limit || 20; | ||
| const tier = filters?.tier; | ||
|
|
||
| const mockData = getMockLeaderboard(page, limit, tier); | ||
|
|
||
| return NextResponse.json({ | ||
| data: { | ||
| leaderboard: { | ||
| entries: mockData.data.map((entry, index) => ({ | ||
| rank: (page - 1) * limit + index + 1, | ||
| contributor: entry, | ||
| })), | ||
| totalCount: mockData.total, | ||
| }, | ||
| }, | ||
| }); | ||
| } | ||
|
|
||
| if ( | ||
| query.includes("query UserLeaderboardRank") || | ||
| query.includes("userLeaderboardRank") | ||
| ) { | ||
| const { userId } = variables || {}; | ||
| const rankData = getMockUserRank(userId); | ||
|
|
||
| return NextResponse.json({ | ||
| data: { | ||
| userLeaderboardRank: rankData, | ||
| }, | ||
| }); | ||
| } |
There was a problem hiding this comment.
Fragile query routing — relies on case-sensitive substring collisions and wrong branch order.
Dispatching by query.includes(...) over the raw GraphQL document is brittle:
- The leaderboard branch is checked first and falls back to a very broad lowercase
"leaderboard"match. It only fails to swallow theUserLeaderboardRankquery by the accident that JS.includesis case-sensitive ("userLeaderboardRank"has a capitalL, so it doesn't contain lowercase"leaderboard"). Any future rename, comment, fragment, or introspection query containing the word "leaderboard" (e.g. in a description or selection alias) will be misrouted into the leaderboard handler and never reach the rank handler. - Either swap the checks (most specific first —
UserLeaderboardRankbeforeLeaderboard) or, preferably, parse the operation name once and switch on it.
♻️ Suggested fix — check the more specific operation first and match the operation name directly
- // Simple mock GraphQL server handling the leaderboard queries
- if (query.includes("query Leaderboard") || query.includes("leaderboard")) {
- const { filters, pagination } = variables || {};
- const page = pagination?.page || 1;
- const limit = pagination?.limit || 20;
- const tier = filters?.tier;
-
- const mockData = getMockLeaderboard(page, limit, tier);
-
- return NextResponse.json({
- data: {
- leaderboard: {
- entries: mockData.data.map((entry, index) => ({
- rank: (page - 1) * limit + index + 1,
- contributor: entry,
- })),
- totalCount: mockData.total,
- },
- },
- });
- }
-
- if (
- query.includes("query UserLeaderboardRank") ||
- query.includes("userLeaderboardRank")
- ) {
- const { userId } = variables || {};
- const rankData = getMockUserRank(userId);
-
- return NextResponse.json({
- data: {
- userLeaderboardRank: rankData,
- },
- });
- }
+ // Match by operation name; check the more specific operation first.
+ const operationMatch = /\b(?:query|mutation)\s+(\w+)/.exec(query);
+ const operationName = operationMatch?.[1];
+
+ if (operationName === "UserLeaderboardRank") {
+ const { userId } = variables || {};
+ const rankData = getMockUserRank(userId);
+ return NextResponse.json({
+ data: { userLeaderboardRank: rankData },
+ });
+ }
+
+ if (operationName === "Leaderboard") {
+ const { filters, pagination } = variables || {};
+ const page = pagination?.page || 1;
+ const limit = pagination?.limit || 20;
+ const tier = filters?.tier;
+
+ const mockData = getMockLeaderboard(page, limit, tier);
+ return NextResponse.json({
+ data: {
+ leaderboard: {
+ entries: mockData.data.map((entry, index) => ({
+ rank: (page - 1) * limit + index + 1,
+ contributor: entry,
+ })),
+ totalCount: mockData.total,
+ },
+ },
+ });
+ }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/api/graphql/route.ts` around lines 10 - 43, The current routing based on
query.includes(...) is brittle and misorders branches; update the handler to
derive the GraphQL operation name once (e.g., extract the operation name from
the `query` string via a simple regex like
/^\s*(query|mutation)\s+([A-Za-z0-9_]+)/ or by using a lightweight parser) and
switch on that name instead of substring checks, then route to
getMockUserRank(userId) when the operation name === "UserLeaderboardRank" and to
getMockLeaderboard(page, limit, tier) when the operation name === "Leaderboard";
alternatively, if you want the smallest change, simply check the more specific
`UserLeaderboardRank` branch (calling getMockUserRank) before the broader
`leaderboard` branch to avoid accidental matches.
Comment on lines
+70
to
+96
| useEffect(() => { | ||
| if ( | ||
| hasScrolledRef.current || | ||
| !currentUserId || | ||
| !currentUserRowRef.current | ||
| ) { | ||
| return; | ||
| } | ||
|
|
||
| return ( | ||
| <TableRow | ||
| key={entry.contributor.id} | ||
| className={cn( | ||
| "border-b border-border/60 hover:bg-muted/20", | ||
| isCurrentUser && "bg-secondary/40", | ||
| onRowClick && "cursor-pointer focus:outline-none focus:ring-2 focus:ring-primary focus:z-10 relative" | ||
| )} | ||
| tabIndex={onRowClick ? 0 : undefined} | ||
| role={onRowClick ? "row" : undefined} | ||
| onClick={onRowClick ? () => onRowClick(entry) : undefined} | ||
| onKeyDown={onRowClick ? (e) => handleKeyDown(e, entry) : undefined} | ||
| > | ||
| <TableCell className="text-center font-medium"> | ||
| <div className="flex justify-center"> | ||
| <RankBadge rank={entry.rank} /> | ||
| </div> | ||
| </TableCell> | ||
| <TableCell> | ||
| <div className="flex items-center gap-3"> | ||
| <Avatar className="h-9 w-9 border border-border/60"> | ||
| <AvatarImage src={entry.contributor.avatarUrl || undefined} /> | ||
| <AvatarFallback className="bg-secondary text-secondary-foreground">{entry.contributor.displayName[0]}</AvatarFallback> | ||
| </Avatar> | ||
| <div className="flex flex-col"> | ||
| <span className={cn("font-semibold text-foreground", isCurrentUser && "text-primary")}> | ||
| {entry.contributor.displayName} | ||
| {isCurrentUser && " (You)"} | ||
| </span> | ||
| <div className="flex gap-1 md:hidden"> | ||
| <span className="text-xs text-muted-foreground">{entry.contributor.tier}</span> | ||
| </div> | ||
| <div className="flex gap-1 mt-1 md:hidden"> | ||
| {entry.contributor.topTags.slice(0, 3).map(tag => ( | ||
| <span key={tag} className="text-[10px] bg-muted px-1 rounded">{tag}</span> | ||
| ))} | ||
| </div> | ||
| </div> | ||
| </div> | ||
| {/* Desktop tags */} | ||
| <div className="hidden md:flex gap-1 mt-2"> | ||
| {entry.contributor.topTags.slice(0, 3).map(tag => ( | ||
| <Badge key={tag} variant="secondary" className="text-[10px] px-1 h-5 font-normal"> | ||
| {tag} | ||
| </Badge> | ||
| ))} | ||
| </div> | ||
| </TableCell> | ||
| <TableCell className="hidden md:table-cell text-foreground"> | ||
| <TierBadge tier={entry.contributor.tier} /> | ||
| </TableCell> | ||
| <TableCell className="text-right font-mono text-foreground font-medium"> | ||
| {entry.contributor.totalScore.toLocaleString()} | ||
| </TableCell> | ||
| <TableCell className="text-right hidden sm:table-cell text-foreground"> | ||
| {entry.contributor.stats.totalCompleted} | ||
| </TableCell> | ||
| <TableCell className="text-right hidden lg:table-cell font-mono text-foreground"> | ||
| ${entry.contributor.stats.totalEarnings.toLocaleString()} | ||
| </TableCell> | ||
| <TableCell className="text-right"> | ||
| <div className="flex justify-end"> | ||
| <StreakBadge streak={entry.contributor.stats.currentStreak} /> | ||
| </div> | ||
| </TableCell> | ||
| </TableRow> | ||
| ); | ||
| })} | ||
| {isFetchingNextPage && ( | ||
| <TableRow> | ||
| <TableCell colSpan={7} className="text-center py-4"> | ||
| <div className="flex items-center justify-center text-muted-foreground text-sm"> | ||
| Loading more... | ||
| </div> | ||
| </TableCell> | ||
| </TableRow> | ||
| )} | ||
| </TableBody> | ||
| </Table> | ||
| {hasNextPage && <div ref={loadMoreRef} className="h-4" />} | ||
| </div> | ||
| const userEntry = entries.find( | ||
| (e) => e.contributor.userId === currentUserId, | ||
| ); | ||
| const userRank = userEntry?.rank ?? 0; | ||
|
|
||
| if (userRank > 10) { | ||
| currentUserRowRef.current.scrollIntoView({ | ||
| behavior: "smooth", | ||
| // "nearest" scrolls the minimum distance needed — avoids | ||
| // aggressively snapping the hero header offscreen. | ||
| block: "nearest", | ||
| }); | ||
| } | ||
|
|
||
| // Mark as done regardless of whether we scrolled, so future | ||
| // re-renders (e.g. next-page loads) don't re-trigger. | ||
| hasScrolledRef.current = true; | ||
| }, [currentUserId, entries]); |
There was a problem hiding this comment.
Auto-scroll won't re-trigger when filters change.
hasScrolledRef is only set to true (it's never reset), so once the user has been scrolled to once, switching timeframe/tier/tags produces a brand-new entries list whose new row position will not be auto-scrolled to. If that's intentional (avoid jarring jumps on filter changes), consider documenting it; otherwise reset the flag when the filter-driven entries identity changes.
♻️ Optional fix: reset on filter-driven entry-set changes
useEffect(() => {
if (
hasScrolledRef.current ||
!currentUserId ||
!currentUserRowRef.current
) {
return;
}
// ...
}, [currentUserId, entries]);
+
+ // Re-arm the one-shot scroll when the underlying dataset is replaced
+ // (e.g. user changes timeframe/tier/tags filters).
+ // Pass a `resetKey` prop derived from filters from the parent, or compare
+ // entries[0]?.contributor.id to a previous ref.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@components/leaderboard/leaderboard-table.tsx` around lines 70 - 96, The
effect currently sets hasScrolledRef.current = true and never resets it, so
after the first auto-scroll future filter changes (which produce new entries)
won't re-trigger scrolling; update the component to reset hasScrolledRef.current
to false whenever the filter-driven entry set changes (e.g., when timeframe,
tier, or tags change or when entries identity changes) so the useEffect that
reads hasScrolledRef, currentUserId, currentUserRowRef and entries can run the
scroll logic again; locate where filters or entries are updated and set
hasScrolledRef.current = false there (or include the specific filter values in
the useEffect dependency array and reset the ref at the start).
Comment on lines
+80
to
+85
| {/* Link wraps Button to preserve correct anchor semantics */} | ||
| <Link href={ROUTES.AUTH} className="w-full"> | ||
| <Button variant="default" size="sm" className="w-full"> | ||
| Sign In | ||
| </Button> | ||
| </Link> |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm the project's Button component supports `asChild` (shadcn convention via Radix Slot).
fd -t f 'button.tsx' components/ui | xargs rg -nP -C2 '\basChild\b|@radix-ui/react-slot|Slot\s*[:=]'Repository: boundlessfi/bounties
Length of output: 531
Invalid HTML / a11y: nested interactive elements (<a><button>).
Link renders an anchor and Button renders a <button>, producing <a><button>Sign In</button></a>. Per the HTML spec, interactive content must not contain other interactive content; this also causes inconsistent focus and announcement behavior across assistive tech. Use the shadcn asChild pattern to render a single <a> with button styling.
🛠️ Proposed fix
- {/* Link wraps Button to preserve correct anchor semantics */}
- <Link href={ROUTES.AUTH} className="w-full">
- <Button variant="default" size="sm" className="w-full">
- Sign In
- </Button>
- </Link>
+ {/* Render a single <a> styled as a button via Radix Slot (asChild). */}
+ <Button asChild variant="default" size="sm" className="w-full">
+ <Link href={ROUTES.AUTH}>Sign In</Link>
+ </Button>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@components/leaderboard/user-rank-sidebar.tsx` around lines 80 - 85, The
current JSX nests a <Button> inside a <Link> which creates invalid HTML
(<a><button>) — change to use the shadcn asChild pattern by rendering the Link
as the Button's child: wrap the Link (href={ROUTES.AUTH}) inside <Button asChild
variant="default" size="sm" className="w-full"> so the Button forwards its
styling/behavior to the anchor element and you end up with a single interactive
<a> element; preserve the w-full classes and Sign In label when moving Link
inside Button.
Contributor
|
@TheBigWealth89 |
Author
|
What do you mean? |
Contributor
Kindly Tag the issue number here using |
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
2 participants
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.
PR: Leaderboard Dynamic Auth & Routing Fix
Overview
This PR implements dynamic authenticated user highlighting on the leaderboard, addresses unauthenticated/loading states, and fixes a routing bug where profile links were incorrectly pointing to
/user/[id]instead of/profile/[id].Key Changes
1. Centralized Routing (
lib/routes.ts)ROUTESconstant to eliminate hardcoded path strings.ROUTES.PROFILE(userId),ROUTES.AUTH, andROUTES.LEADERBOARD.2. Leaderboard Page (
app/leaderboard/page.tsx)currentUserId = "user-1"withauthClient.useSession().isPendingfrom the auth session to defer row highlighting and sidebar rendering until the user's status is confirmed.ROUTES.PROFILE(userId).3. Accessible Table Highlighting (
components/leaderboard/leaderboard-table.tsx)aria-labelandsr-onlytext for screen readers.useEffectwith ascrollIntoViewcall that fires once on mount, but only if the current user is ranked lower than the top 10 (keeping the fold clean for top performers).4. Polished Sidebar State (
components/leaderboard/user-rank-sidebar.tsx)isSessionPendingto show a skeleton loader instead of the "Sign In" CTA while auth status is resolving.Files Modified
lib/routes.tsapp/leaderboard/page.tsxcomponents/leaderboard/leaderboard-table.tsxcomponents/leaderboard/user-rank-sidebar.tsxVerification
npx tsc --noEmit./profile/[userId].Summary by CodeRabbit
Release Notes
New Features
Accessibility