Conversation
📝 WalkthroughWalkthroughAdded a TREND sort and Hot badge to the leaderboard header, refactored prefetch and polling into simplified single-page prefetch and deferred timeout-based polling, standardized test formatting, and included .env.production in worktree copy configuration. No exported API types were removed except OpenAPI schemas. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Component as WaveleaderboardSort
participant Hook as useWaveDropsLeaderboard
participant Query as QueryClient/Fetcher
participant Polling as PollingQuery
User->>Component: hover / mouseenter on TREND
Component->>Hook: call debounced prefetch
Hook->>Query: prefetchInfiniteQuery (single-page TREND)
Query-->>Hook: prefetch complete
User->>Component: change sort selection
Component->>Hook: set sort
Hook->>Polling: start/configure polling query (select -> haveNewDrops)
Polling->>Query: poll endpoint
Query-->>Polling: poll results
Polling->>Hook: detect new drops -> set timeout for refetch
Hook->>Query: refetch (on timeout)
Query-->>Hook: updated pages
Hook-->>Component: provide updated drops / isFetching
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested reviewers
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. 📜 Recent review detailsConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
💤 Files with no reviewable changes (1)
⏰ 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)
✏️ Tip: You can disable this entire section by setting 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: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
components/waves/leaderboard/header/WaveleaderboardSort.tsx (1)
126-163: Addtype="button"on buttons to prevent accidental form submission.Proposed fix
- <button + <button + type="button" className={getButtonClassName(WaveDropsLeaderboardSort.TREND)} onClick={() => onSortChange(WaveDropsLeaderboardSort.TREND)} onMouseEnter={() => prefetchSort(WaveDropsLeaderboardSort.TREND)} > 🔥 Hot </button> <button + type="button" className={getButtonClassName(WaveDropsLeaderboardSort.RANK)} onClick={() => onSortChange(WaveDropsLeaderboardSort.RANK)} onMouseEnter={() => prefetchSort(WaveDropsLeaderboardSort.RANK)} > Current Vote </button> <button + type="button" className={getButtonClassName( WaveDropsLeaderboardSort.RATING_PREDICTION )} onClick={() => onSortChange(WaveDropsLeaderboardSort.RATING_PREDICTION)} onMouseEnter={() => prefetchSort(WaveDropsLeaderboardSort.RATING_PREDICTION) } > Projected Vote </button> <button + type="button" className={getButtonClassName(WaveDropsLeaderboardSort.CREATED_AT)} onClick={() => onSortChange(WaveDropsLeaderboardSort.CREATED_AT)} onMouseEnter={() => prefetchSort(WaveDropsLeaderboardSort.CREATED_AT)} > Newest </button>openapi.yaml (1)
5739-5822:ApiCommunityMetricsnow has new required fields—verify backend always returns them (breaking change otherwise).hooks/useWaveDropsLeaderboard.ts (1)
226-266: Three correctness and lifecycle bugs need fixing: missing auth gate on polling, wrong "newest" detection for non-CREATED_AT sorts, and untracked setTimeout side-effects.
Missing
connectedProfileHandlegate: Polling query (line 257) hasenabled: canPoll && !pausePollingbut main infinite query (line 165) gates with!!connectedProfileHandle. Polling will execute API calls even when no user is authenticated.Wrong "newest" detection for RANK/TREND sorts: The
checkForNewDropsfunction (lines 206-224) comparesdrops[0].created_atagainstpollingData.drops[0].created_at, but when sort is RANK or TREND (not CREATED_AT), the first item isn't newest by time. This causes incorrecthaveNewDropsdetection and failed auto-refetch triggers. The polling query must either always use CREATED_AT sort, or compare against the actual newest drop by timestamp (computed across all current drops, not just position 0).Untracked setTimeout side-effects: The
setTimeout(refetch(), POLLING_DELAY)in queryFn (lines 247-252) will execute on every polling interval. Multiple scheduled timeouts accumulate if the query refetches before previous timeout fires. They are never cleaned up on unmount (current cleanup at lines 268-274 only removes queries), allowing refetch calls to fire after component unmount.Proposed fix
-import { useCallback, useEffect, useState, useMemo } from "react"; +import { useCallback, useEffect, useRef, useState, useMemo } from "react"; @@ export function useWaveDropsLeaderboard({ @@ }: UseWaveDropsLeaderboardProps) { @@ const [canPoll, setCanPoll] = useState(false); const isTabVisible = useTabVisibility(); + const refetchTimeoutRef = useRef<ReturnType<typeof setTimeout> | null>(null); @@ const canAutoRefetch = isTabVisible && !hasTempDrop; @@ const checkForNewDrops = useCallback( (pollingData: ApiDropsLeaderboardPage): boolean => { if (pollingData.drops.length === 0) return false; const latestPolledDrop = pollingData.drops[0]; - const newestExistingDrop = drops[0]; + const newestExistingCreatedAt = drops.reduce((max, d) => { + const t = new Date(d.created_at).getTime(); + return Number.isFinite(t) ? Math.max(max, t) : max; + }, 0); - if (latestPolledDrop && newestExistingDrop) { + if (latestPolledDrop && newestExistingCreatedAt > 0) { const polledCreatedAt = new Date(latestPolledDrop.created_at).getTime(); - const existingCreatedAt = new Date( - newestExistingDrop.created_at - ).getTime(); - return polledCreatedAt > existingCreatedAt; + return polledCreatedAt > newestExistingCreatedAt; } return true; }, [drops] ); @@ const { data: haveNewDrops = false } = useQuery({ @@ if (sortDirection) { params["sort_direction"] = sortDirection; } @@ - if (canAutoRefetch && checkForNewDrops(result)) { - // Use setTimeout to defer the refetch slightly and avoid React Query batching issues - setTimeout(() => { - refetch(); - }, POLLING_DELAY); + if ( + canAutoRefetch && + checkForNewDrops(result) && + !refetchTimeoutRef.current + ) { + refetchTimeoutRef.current = setTimeout(() => { + refetchTimeoutRef.current = null; + refetch(); + }, POLLING_DELAY); } @@ select: checkForNewDrops, - enabled: canPoll && !pausePolling, + enabled: canPoll && !!connectedProfileHandle && !pausePolling, @@ }); + + useEffect(() => { + return () => { + if (refetchTimeoutRef.current) { + clearTimeout(refetchTimeoutRef.current); + refetchTimeoutRef.current = null; + } + }; + }, []);
🧹 Nitpick comments (2)
hooks/useWaveDropsLeaderboard.ts (1)
105-134: Prefetch effect dependencies/error handling: consider tightening.This effect uses
queryClient,queryKey,sortDirection,getNextPageParambut only depends on[waveId, sort]; alsoprefetchInfiniteQuery(...)isn’t awaited/caught here (unlike the component).openapi.yaml (1)
333-371: New /community-metrics/mints endpoint looks reasonable; consider adding pagination constraints/defaults for consistency.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (4)
generated/models/ApiCommunityMetrics.tsis excluded by!**/generated/**generated/models/ApiMintMetrics.tsis excluded by!**/generated/**generated/models/ApiMintMetricsPage.tsis excluded by!**/generated/**generated/models/ObjectSerializer.tsis excluded by!**/generated/**
📒 Files selected for processing (5)
__tests__/components/waves/leaderboard/header/WaveleaderboardSort.test.tsxcomponents/waves/leaderboard/header/WaveleaderboardSort.tsxhooks/useWaveDropsLeaderboard.tsopenapi.yamlscripts/worktree/copy.conf
⏰ 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: Analyze (javascript-typescript)
🔇 Additional comments (8)
scripts/worktree/copy.conf (1)
11-11: Verify that copying production secrets to worktrees is intentional.Adding
.env.productionensures production environment variables are available in worktrees, but this also means production secrets (API keys, credentials) will be copied to each worktree. Confirm this aligns with your security practices—particularly if worktrees are used for local development or testing where production credentials shouldn't be active.__tests__/components/waves/leaderboard/header/WaveleaderboardSort.test.tsx (1)
1-38: Tests align with new TREND/“🔥 Hot” UI.components/waves/leaderboard/header/WaveleaderboardSort.tsx (3)
21-30: SORT_DIRECTION_MAP update (TREND: "DESC") looks correct.
104-114: Debounce cancel on unmount is a solid guardrail.
39-103: No issues found.prefetchInfiniteQueryin@tanstack/react-queryv5.45.1 (your repo's version) fully supports bothinitialPageParamandpagesoptions. The usage here is correct:initialPageParam: nullis valid, andpages: 1specifies that exactly one page should be prefetched (which is also the default behavior). No type mismatches or silent no-ops will occur.hooks/useWaveDropsLeaderboard.ts (2)
23-47: TREND enum + sort-direction mapping are consistent with the UI.
170-224: Derived drops via useMemo is a nice simplification.openapi.yaml (1)
4279-4330: Adding TREND to wave leaderboard sort enum matches frontend changes.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
hooks/useWaveDropsLeaderboard.ts (1)
280-283: Add cleanup forQueryKey.DROPS_LEADERBOARDin the unmount effect.The hook queries with
QueryKey.DROPS_LEADERBOARD(line 82) but only removesQueryKey.DROPSon unmount. The cleanup should also remove the hook's own query key. Consider removing both keys to match the pattern inReactQueryWrapper.invalidateDrops()(lines 918–925), which invalidates both together, or at minimum ensureDROPS_LEADERBOARDis cleaned up when this component unmounts.
🧹 Nitpick comments (3)
hooks/useWaveDropsLeaderboard.ts (3)
209-228: Unreachablereturn trueon line 225 may cause unintended refetches if reached.The
return trueat line 225 appears unreachable: the early return at line 211 ensurespollingData.drops.length > 0, sopollingData.drops[0]will always be defined. If this line is somehow reached due to an unexpected falsy value in the array, returningtruewould trigger unnecessary refetches.Consider returning
falsefor safety, or removing the dead code path entirely.Suggested fix
if (latestPolledDrop) { const polledCreatedAt = new Date(latestPolledDrop.created_at).getTime(); return polledCreatedAt > newestExistingTimestamp; } - return true; + return false; },
247-258: Side effects inqueryFnis an anti-pattern and may cause stale closure issues.Triggering
refetch()insidequeryFnviolates React Query's expected usage pattern where query functions should be pure data-fetching operations. Additionally,canAutoRefetchandcheckForNewDropsare captured in the closure at query creation time and may become stale.Consider moving the refetch logic to a
useEffectthat reacts tohaveNewDrops:Suggested refactor
const { data: haveNewDrops = false } = useQuery({ queryKey: [...queryKey, "polling"], queryFn: async () => { const params: Record<string, string> = { page_size: "1", sort: WaveDropsLeaderboardSort.CREATED_AT, sort_direction: "DESC", }; - const result = await commonApiFetch<ApiDropsLeaderboardPage>({ + return await commonApiFetch<ApiDropsLeaderboardPage>({ endpoint: `waves/${waveId}/leaderboard`, params, }); - - // Trigger refetch directly in the query callback when conditions are met - // This replaces the effect-based approach - if (canAutoRefetch && checkForNewDrops(result)) { - // Clear any existing timeout to prevent accumulation - if (pollingTimeoutRef.current) { - clearTimeout(pollingTimeoutRef.current); - } - // Use setTimeout to defer the refetch slightly and avoid React Query batching issues - pollingTimeoutRef.current = setTimeout(() => { - refetch(); - }, POLLING_DELAY); - } - - return result; }, select: checkForNewDrops, // ...rest of options }); + + // Trigger refetch when new drops are detected + useEffect(() => { + if (haveNewDrops && canAutoRefetch) { + if (pollingTimeoutRef.current) { + clearTimeout(pollingTimeoutRef.current); + } + pollingTimeoutRef.current = setTimeout(() => { + refetch(); + }, POLLING_DELAY); + } + }, [haveNewDrops, canAutoRefetch, refetch]);
134-134: Consider adding derived values to the dependency array for lint compliance.
queryKey,sortDirection, andgetNextPageParamare used inside the effect but not listed in dependencies. While they're derived fromwaveIdandsort, exhaustive-deps lint rules would flag this.Suggested fix
- }, [waveId, sort]); + }, [waveId, sort, queryKey, sortDirection, getNextPageParam, queryClient]);
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
hooks/useWaveDropsLeaderboard.ts
🧰 Additional context used
🧬 Code graph analysis (1)
hooks/useWaveDropsLeaderboard.ts (1)
generated/models/ApiDropsLeaderboardPage.ts (1)
ApiDropsLeaderboardPage(18-67)
🔇 Additional comments (8)
hooks/useWaveDropsLeaderboard.ts (8)
3-3: LGTM!Import additions (
useMemo,useRef) align with the new derived state pattern and polling timeout management.
23-29: LGTM!TREND sort option is properly added to the enum.
38-47: LGTM!The explicit
Record<WaveDropsLeaderboardSort, ...>typing ensures compile-time safety when new sort options are added, and TREND correctly maps to DESC.
171-192: LGTM!Clean derived state pattern using
useMemo. Eliminates the need for synchronizing state withuseEffect, and the dependency array correctly includesdataandsort.
194-195: LGTM!Simple derived boolean from
data?.pagesis cleaner than tracking initialization state separately.
199-204: LGTM!Smart guard against refetching during optimistic updates by checking for temp drops.
296-296: LGTM!
isFetching || !hasInitializedcorrectly shows loading state during initial fetch, simplifying the previous approach.
119-121: LGTM!Consistent
typeof pageParam === "number"check correctly handles thenumber | nulltype frominitialPageParam: null.Also applies to: 155-157
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
|
2 participants
Summary by CodeRabbit
New Features
Improvements
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.