Refactor leaderboard fetching logic and improve error handling#3829
Merged
Conversation
Contributor
📝 WalkthroughWalkthroughThe PR adds abort-aware leaderboard fetching, dynamic carousel pagination, direct toggle-to-page updates, revised stage animations and card shadows, and consolidates polling backoff state around the error counter. ChangesLeaderboard UI and data flow
Polling backoff state
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant FacesOfCleanAirPage
participant fetchCleanAirForum2026Leaderboard
participant LeaderboardToggles
FacesOfCleanAirPage->>fetchCleanAirForum2026Leaderboard: fetch leaderboard entries
fetchCleanAirForum2026Leaderboard-->>FacesOfCleanAirPage: return normalized entries or error
FacesOfCleanAirPage->>LeaderboardToggles: provide slide index callback
LeaderboardToggles->>FacesOfCleanAirPage: report selected index
FacesOfCleanAirPage->>FacesOfCleanAirPage: update leaderboard slide
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsErrors were encountered while retrieving linked issues. Errors (1)
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 |
Contributor
|
New azure website changes available for preview here |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/website/src/features/clean-air-forum-2026/screens/LeaderboardScreen.tsx (1)
386-397: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winToggle clicks skip
setDirection, so slide animation direction can be wrong.
goToRelativePagealways pairssetDirectionwithsetPage, butonIndexChange={setPage}here callssetPagealone. Jumping via a toggle can therefore animate in the wrong direction relative to the actual page change (staledirectionvalue carried over from the last auto-advance/swipe/arrow interaction).🔧 Proposed fix
isPaused={isPaused} reduceMotion={shouldReduceMotion} - onIndexChange={setPage} + onIndexChange={(index) => { + setDirection(index > page ? 1 : -1); + setPage(index); + }} />🤖 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 `@src/website/src/features/clean-air-forum-2026/screens/LeaderboardScreen.tsx` around lines 386 - 397, Update the LeaderboardToggles onIndexChange handler in LeaderboardScreen so toggle navigation updates direction consistently with page changes, reusing the existing goToRelativePage logic or equivalent direction calculation before calling setPage. Preserve the current activeIndex and toggle behavior while ensuring animation direction reflects the selected page.src/website/src/features/faces-of-clean-air/FacesOfCleanAirPage.tsx (1)
883-937: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winKeep the auto-advance timer independent of live slide-count growth.
leaderboardSlideCountnow changes withleaderboardEntries.length, so whenever the list crosses a slide boundary this effect tears down and restarts the timeout. That can keep pushing the next carousel advance out while entries keep coming in.🤖 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 `@src/website/src/features/faces-of-clean-air/FacesOfCleanAirPage.tsx` around lines 883 - 937, The auto-advance effect should not restart when leaderboardSlideCount changes as leaderboardEntries grows. Update the effect dependencies and callback logic around leaderboardSlideCount to use a stable or current-value reference for the slide-count boundary while keeping the timer’s lifecycle independent of live count growth; preserve the existing stage transitions and slide advancement behavior.
🧹 Nitpick comments (1)
src/website/src/features/faces-of-clean-air/FacesOfCleanAirPage.tsx (1)
207-283: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftDuplicate
FaceCard/SkeletonCardimplementations across two files — this PR's shadow tweak had to be applied twice.FacesOfCleanAirPage.tsxkeeps its own localSkeletonCard/FaceCard, nearly identical to the exported versions inselfie-components.tsx; both needed the same shadow-token edit here, and they've already diverged elsewhere (fallback display name/location text), which is exactly the kind of drift this duplication invites.
src/website/src/features/faces-of-clean-air/FacesOfCleanAirPage.tsx#L207-L283: replace the localSkeletonCardwith the exported one fromselfie-components.tsx.src/website/src/features/faces-of-clean-air/FacesOfCleanAirPage.tsx#L303-L400: replace the localFaceCardwith the exported one fromselfie-components.tsx(reconcile thedisplayName/locationfallback differences first).src/website/src/components/clean-air-forum-2026/selfie-components.tsx#L67-L147: keep as the canonicalFaceCardonce consolidated.src/website/src/components/clean-air-forum-2026/selfie-components.tsx#L149-L208: keep as the canonicalSkeletonCardonce consolidated.🤖 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 `@src/website/src/features/faces-of-clean-air/FacesOfCleanAirPage.tsx` around lines 207 - 283, Consolidate the duplicated card components by removing the local SkeletonCard and FaceCard implementations from src/website/src/features/faces-of-clean-air/FacesOfCleanAirPage.tsx at lines 207-283 and 303-400, importing and using the canonical exports from src/website/src/components/clean-air-forum-2026/selfie-components.tsx instead; reconcile displayName and location fallbacks before removal. Keep the canonical FaceCard at lines 67-147 and SkeletonCard at lines 149-208 unchanged.
🤖 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.
Outside diff comments:
In `@src/website/src/features/clean-air-forum-2026/screens/LeaderboardScreen.tsx`:
- Around line 386-397: Update the LeaderboardToggles onIndexChange handler in
LeaderboardScreen so toggle navigation updates direction consistently with page
changes, reusing the existing goToRelativePage logic or equivalent direction
calculation before calling setPage. Preserve the current activeIndex and toggle
behavior while ensuring animation direction reflects the selected page.
In `@src/website/src/features/faces-of-clean-air/FacesOfCleanAirPage.tsx`:
- Around line 883-937: The auto-advance effect should not restart when
leaderboardSlideCount changes as leaderboardEntries grows. Update the effect
dependencies and callback logic around leaderboardSlideCount to use a stable or
current-value reference for the slide-count boundary while keeping the timer’s
lifecycle independent of live count growth; preserve the existing stage
transitions and slide advancement behavior.
---
Nitpick comments:
In `@src/website/src/features/faces-of-clean-air/FacesOfCleanAirPage.tsx`:
- Around line 207-283: Consolidate the duplicated card components by removing
the local SkeletonCard and FaceCard implementations from
src/website/src/features/faces-of-clean-air/FacesOfCleanAirPage.tsx at lines
207-283 and 303-400, importing and using the canonical exports from
src/website/src/components/clean-air-forum-2026/selfie-components.tsx instead;
reconcile displayName and location fallbacks before removal. Keep the canonical
FaceCard at lines 67-147 and SkeletonCard at lines 149-208 unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 77f07084-6038-430e-a2d7-b8866913beab
📒 Files selected for processing (6)
src/website/src/components/clean-air-forum-2026/LeaderboardToggles.tsxsrc/website/src/components/clean-air-forum-2026/selfie-components.tsxsrc/website/src/features/clean-air-forum-2026/lib/learn-progress.tssrc/website/src/features/clean-air-forum-2026/screens/LeaderboardScreen.tsxsrc/website/src/features/faces-of-clean-air/FacesOfCleanAirPage.tsxsrc/website/src/hooks/usePollingWithVisibility.ts
💤 Files with no reviewable changes (1)
- src/website/src/hooks/usePollingWithVisibility.ts
Contributor
There was a problem hiding this comment.
Pull request overview
This PR refactors the Clean Air Forum 2026 leaderboard/selfie-wall carousel behavior in the website, adding direct slide navigation via toggle controls and improving fetch robustness (timeouts/abort handling) while updating card/placeholder styling.
Changes:
- Added toggle click support to jump directly to a leaderboard page/slide.
- Updated leaderboard fetching to support optional limits and a 30s timeout with AbortSignal forwarding.
- Refined Faces-of-Clean-Air carousel/leaderboard state handling and updated shadow styling for cards/placeholders.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| src/website/src/hooks/usePollingWithVisibility.ts | Refactors polling logic by removing backoffRef usage (but leaves backoff-related logic to be reconciled). |
| src/website/src/features/faces-of-clean-air/FacesOfCleanAirPage.tsx | Updates carousel staging, leaderboard fetching state, dynamic slide counts, and styling; adds toggle-driven leaderboard navigation. |
| src/website/src/features/clean-air-forum-2026/screens/LeaderboardScreen.tsx | Wires leaderboard toggle controls to update the current page. |
| src/website/src/features/clean-air-forum-2026/lib/learn-progress.ts | Improves leaderboard fetch cancellation/timeout behavior and makes limit optional. |
| src/website/src/components/clean-air-forum-2026/selfie-components.tsx | Updates FaceCard/SkeletonCard shadow styling to match the new design. |
| src/website/src/components/clean-air-forum-2026/LeaderboardToggles.tsx | Adds onIndexChange and calls it on toggle click to enable direct navigation. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
926
to
+930
| }, [ | ||
| displayStage, | ||
| fetchState, | ||
| fetchLeaderboard, | ||
| goToRelativePage, | ||
| hasLoadedRef, |
Comment on lines
1390
to
1394
| <div className="w-full"> | ||
| <LeaderboardRowsBlock | ||
| rows={leaderboardRows} | ||
| slideKey={leaderboardSlideIndex} | ||
| reduceMotion={shouldReduceMotion} |
| intervalMs={CAROUSEL_INTERVAL_MS} | ||
| isPaused={isPaused} | ||
| reduceMotion={shouldReduceMotion} | ||
| onIndexChange={setPage} |
Comment on lines
55
to
59
|
|
||
| const refresh = useCallback(() => { | ||
| errorCountRef.current = 0; | ||
| backoffRef.current = 1; | ||
| void invoke(); | ||
| }, [invoke]); |
Comment on lines
+91
to
+95
| if (signal) { | ||
| signal.addEventListener('abort', () => controller.abort(signal.reason), { | ||
| once: true, | ||
| }); | ||
| } |
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
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.
Status of maturity (all need to be checked before merging):
Screenshots (optional)
Summary by CodeRabbit
New Features
Bug Fixes
Style