fix(profile): differentiate 404 from transient GitHub API failures#188
fix(profile): differentiate 404 from transient GitHub API failures#188Adit-Jain-srm wants to merge 7 commits into
Conversation
…loses PRODHOSH#175) fetchGitHubUser now returns a discriminated union instead of nullable JSON. The page component only calls notFound() on a genuine 404. Rate limits and server errors throw, which the error boundary catches with a retry option. generateMetadata falls back gracefully on any failure. Signed-off-by: Adit Jain <aditjain2005@gmail.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthrough
ChangesStructured fetch result and consumer updates
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related issues
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)
Comment |
|
Could you add the elusoc label? Issue #175 has ELUSOC + ADVENTURER already. Thanks! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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 `@src/app/`[username]/page.tsx:
- Around line 28-35: The fetchGitHubUser function does not handle exceptions
that can be thrown by fetch() (network errors, timeouts) or res.json()
(malformed JSON parsing), which causes these errors to propagate uncaught and
bypass the discriminated union error handling. Wrap the entire function body in
a try-catch block, keeping the existing status checks and return statements in
the try block, and add a catch block that returns { status: "error" } to handle
any thrown exceptions consistently with the error handling pattern.
- Around line 51-66: The generateMetadata function handles the case where
fetchGitHubUser returns a non-ok status but does not handle exceptions that may
be thrown by the fetchGitHubUser call itself (such as network errors or JSON
parsing errors). Wrap the fetchGitHubUser call in a try-catch block so that any
thrown exception is caught and the fallback metadata object (with title and
description using the username) is returned in the catch block, ensuring
metadata generation does not crash when fetchGitHubUser throws.
- Line 61: Remove the unsafe type assertion `(user.name as string)` on the
displayName variable assignment. Instead of asserting the type, use a safer
pattern that actually validates the type at runtime, such as checking whether
user.name is a string using typeof or a type guard before assigning it to
displayName. This ensures type safety and provides clear intent that the code is
checking for a string value rather than bypassing type checking with an
assertion.
- Around line 72-78: Add a try-catch error handling block to the
fetchGitHubRepos function to ensure consistency with other fetch functions like
fetchLiveStats, fetchOrganizations, and fetchContributionCalendar. Wrap the
entire function body (including the fetch call and res.json() parsing on line
47) in a try block, and add a catch block that returns an empty array. This
prevents promise rejections from invalid JSON responses and ensures all fetch
functions in the Promise.all statement handle errors consistently.
🪄 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: ASSERTIVE
Plan: Pro Plus
Run ID: 88ecdad7-a816-4b03-88a3-ab1e980fbf15
📒 Files selected for processing (1)
src/app/[username]/page.tsx
… assertion
Addresses CodeRabbit review:
- fetchGitHubUser: wrapped in try-catch so network errors return { status: 'error' } instead of throwing
- fetchGitHubRepos: wrapped in try-catch for consistency with other fetch functions
- Replaced (user.name as string) with typeof runtime check
- Skipped generateMetadata try-catch: redundant since fetchGitHubUser now never throws
Signed-off-by: Adit Jain <aditjain2005@gmail.com>
…ags) Merges the discriminated union error handling from this PR with the OG/Twitter metadata from PR PRODHOSH#174 (now on main). Uses typeof runtime checks to safely access user fields from Record<string, unknown>. Signed-off-by: Adit Jain <aditjain2005@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/app/[username]/page.tsx (2)
111-113: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winClarify error message for network/parse failures.
When
userResult.codeis0(from the catch block infetchGitHubUserline 38), the error message "GitHub API returned 0" is misleading. A code of0indicates a network error or JSON parse failure, not an HTTP status code.📝 Suggested improvement for error message clarity
if (userResult.status === "error") { - throw new Error(`GitHub API returned ${userResult.code} for ${username}`); + const errorMsg = userResult.code === 0 + ? `Failed to fetch GitHub user ${username} (network or parse error)` + : `GitHub API returned ${userResult.code} for ${username}`; + throw new Error(errorMsg); }🤖 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/app/`[username]/page.tsx around lines 111 - 113, The error message in the condition checking if userResult.status is "error" treats userResult.code of 0 as an HTTP status code, but 0 actually indicates a network error or JSON parse failure. Modify the error message logic to check if userResult.code equals 0 and provide a clarified error message indicating a network or parsing issue, while keeping the current HTTP status code message for non-zero codes.
23-26: 📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy liftType the discriminated union data field properly for type safety.
The
datafield is typed asRecord<string, unknown>, which disables TypeScript's type checking. WhenuserResult.datais assigned touser(line 114) and passed toProfileView(line 163), TypeScript cannot verify the shape matches the expectedGitHubUserinterface.This forces defensive runtime type guards throughout the code (lines 69-73) and defeats the purpose of using TypeScript. If the GitHub API returns an unexpected shape, the error won't be caught until runtime.
🔒 Recommended fix to add proper typing
First, extract or define the
GitHubUserinterface in this file or a shared types file:+interface GitHubUser { + login: string; + name: string | null; + bio: string | null; + avatar_url: string; + html_url: string; + blog: string | null; + location: string | null; + twitter_username: string | null; + followers: number; + following: number; + public_repos: number; +} + type UserFetchResult = - | { status: "ok"; data: Record<string, unknown> } + | { status: "ok"; data: GitHubUser } | { status: "not_found" } | { status: "error"; code: number };This change will enable compile-time type checking and make most runtime guards in
generateMetadataunnecessary (though keeping them for defense-in-depth is still reasonable).🤖 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/app/`[username]/page.tsx around lines 23 - 26, The UserFetchResult type definition uses Record<string, unknown> for the data field, which prevents TypeScript from validating that the fetched data matches the expected GitHubUser interface. Replace the Record<string, unknown> type in the "ok" status variant with the proper GitHubUser interface type. This will enable compile-time type checking when userResult.data is assigned to user and passed to ProfileView, making the runtime type guards in the code unnecessary and catching API shape mismatches at compile time rather than runtime.
🤖 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 `@src/app/`[username]/page.tsx:
- Around line 65-94: Update the PR objectives or description to document the
metadata content changes that were implemented in the metadata generation for
the user profile page. Specifically, add notes about the enhanced description
logic (which now includes bio with repo and follower counts when available) and
the addition of OpenGraph and Twitter card metadata properties (including title,
description, images, type, and siteName fields). This ensures reviewers
understand that the PR scope includes not just error handling refactoring but
also these metadata improvements for better SEO and social media sharing.
---
Outside diff comments:
In `@src/app/`[username]/page.tsx:
- Around line 111-113: The error message in the condition checking if
userResult.status is "error" treats userResult.code of 0 as an HTTP status code,
but 0 actually indicates a network error or JSON parse failure. Modify the error
message logic to check if userResult.code equals 0 and provide a clarified error
message indicating a network or parsing issue, while keeping the current HTTP
status code message for non-zero codes.
- Around line 23-26: The UserFetchResult type definition uses Record<string,
unknown> for the data field, which prevents TypeScript from validating that the
fetched data matches the expected GitHubUser interface. Replace the
Record<string, unknown> type in the "ok" status variant with the proper
GitHubUser interface type. This will enable compile-time type checking when
userResult.data is assigned to user and passed to ProfileView, making the
runtime type guards in the code unnecessary and catching API shape mismatches at
compile time rather than runtime.
🪄 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: ASSERTIVE
Plan: Pro Plus
Run ID: 97bd6c02-f924-423a-955a-c3060a62f196
📒 Files selected for processing (1)
src/app/[username]/page.tsx
Signed-off-by: Adit Jain <aditjain2005@gmail.com>
PRODHOSH
added
ELUSOC
Replaced Record<string, unknown> with a proper interface matching the GitHub REST API response. Removes redundant typeof guards in generateMetadata. Fixes tsc --noEmit failure where ProfileView expected typed GitHubUser. Signed-off-by: Adit Jain <aditjain2005@gmail.com>
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 (1)
src/app/[username]/page.tsx (1)
117-123: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winFetch secondary profile data only after the user lookup succeeds.
This
Promise.allstarts four extra remote calls before you know whetherfetchGitHubUseris"ok". For"not_found"and"error"cases, Line 125 and Line 126 still wait on those calls before returning the 404 or throwing, which adds avoidable latency and burns GitHub quota on requests that can never render.Suggested restructuring
- const [userResult, repos, liveStats, orgs, contributionCalendar] = await Promise.all([ - fetchGitHubUser(username), - fetchGitHubRepos(username), - fetchLiveStats(username), - fetchOrganizations(username), - fetchContributionCalendar(username), - ]); + const userResult = await fetchGitHubUser(username); if (userResult.status === "not_found") notFound(); if (userResult.status === "error") { const msg = userResult.code === 0 ? `Network error while fetching profile for ${username}` : `GitHub API returned ${userResult.code} for ${username}`; throw new Error(msg); } const user = userResult.data; + + const [repos, liveStats, orgs, contributionCalendar] = await Promise.all([ + fetchGitHubRepos(username), + fetchLiveStats(username), + fetchOrganizations(username), + fetchContributionCalendar(username), + ]);🤖 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/app/`[username]/page.tsx around lines 117 - 123, Fetch the primary user data first in the page component that calls fetchGitHubUser, then only start fetchGitHubRepos, fetchLiveStats, fetchOrganizations, and fetchContributionCalendar after confirming the user result is "ok". Replace the current Promise.all fan-out with a two-step flow so the not_found and error paths can return or throw immediately without waiting on secondary requests, while still keeping the same data shape for the successful render path.
🤖 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/app/`[username]/page.tsx:
- Around line 117-123: Fetch the primary user data first in the page component
that calls fetchGitHubUser, then only start fetchGitHubRepos, fetchLiveStats,
fetchOrganizations, and fetchContributionCalendar after confirming the user
result is "ok". Replace the current Promise.all fan-out with a two-step flow so
the not_found and error paths can return or throw immediately without waiting on
secondary requests, while still keeping the same data shape for the successful
render path.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 737492cc-d1c6-462f-b25c-3f7f7139f217
📒 Files selected for processing (1)
src/app/[username]/page.tsx
|
All CodeRabbit suggestions addressed, CI is green (build + type-check + CodeQL all passing). The one outside-diff suggestion (sequential fetch instead of Promise.all) was intentionally skipped - it would add ~200ms latency to every page load just to save 4 lightweight calls on the rare 404 case. The parallel fan-out is the correct trade-off for a server-rendered page. @PRODHOSH this is ready for review whenever you get a chance. |
|
@Adit-Jain-srm Please resolve the merge conflicts Thanks! |
…0 repos, rateLimited prop) Combines our discriminated union error handling with main's new additions: - OG/Twitter images now auto-injected by opengraph-image.tsx (removed explicit avatar URLs) - Repos fetch uses per_page=100 (from main) - Added rateLimited prop to ProfileView (from main's rate limit handling) - Kept parallel Promise.all (better latency than main's sequential approach) - Rate limit (429) shows rateLimited state instead of throwing Signed-off-by: Adit Jain <aditjain2005@gmail.com>
|
Merge conflicts resolved. Combined our discriminated union approach with main's recent additions (opengraph-image auto-injection, 100 repos, rateLimited prop). Ready for review. @PRODHOSH |
All error/rate-limit cases now throw (caught by error boundary with retry). This guarantees user is GitHubUserData (non-null) when passed to ProfileView, fixing the TS2322 type-check failure. Signed-off-by: Adit Jain <aditjain2005@gmail.com>
|
merge conflicts still exist Thanks |
|
Closing - main already implements rate-limit-aware error handling with a different pattern (throw-based detection + rateLimited flag). The core issue (#175) is effectively resolved by the current main code which no longer calls notFound() on rate limits. Thanks for the guidance on this one! |
2 participants
Summary
Fixes the issue where rate limits (429) and server errors (5xx) from GitHub's API incorrectly show the "profile not found" page. Now only a genuine 404 triggers the not-found page, while transient failures propagate to the error boundary for recovery.
Note: After merging with main, this PR also incorporates the OG/Twitter metadata improvements from #174 (now adapted to use the discriminated union pattern with runtime type guards).
Related Issue
Closes #175
Type of Change
Changes Made
Modified
src/app/[username]/page.tsx:Discriminated union return type for
fetchGitHubUser:{ status: 'ok', data }for successful responses{ status: 'not_found' }for HTTP 404{ status: 'error', code }for anything else (429, 500, 503, network failures)Try-catch on both fetch functions to handle network errors and malformed JSON gracefully
Updated page component to call
notFound()only on genuine 404, throw on transient errors (caught by error boundary)Enhanced generateMetadata (merged from feat(meta): add OG and Twitter Card tags to profile pages #174) now includes:
Distinct error messages: network failures (code 0) say "Network error" while HTTP failures show the actual status code
AI Usage
Screenshots (if UI change)
No visible change for the happy path. The difference is only observable during GitHub outages or rate limiting.
Checklist
mainconsole.logleft insrc/schema.sqland a new migration file are includedDESIGN.mdand followed the design system (colors, spacing, typography, components)feat:,fix:,docs:, etc.)Summary by CodeRabbit