Refactor: Use central githubSearch helper for issues & PRs pagination #191
Conversation
✅ Deploy Preview for github-spy ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
WalkthroughAdds a new GitHub search helper that paginates by date windows to bypass the 1000-result cap, with rate-limit/retry handling; refactors the data hook to use token-based fetching and client-side pagination; updates ContributorProfile to use the helper with abortable requests and improved UX. Changes
Sequence Diagram(s)sequenceDiagram
participant UI as ContributorProfile
participant Hook as useGitHubData
participant Lib as githubSearch.searchUserIssuesAndPRs
participant GH as GitHub API
UI->>Hook: fetchData(username, page, perPage, state, signal)
Hook->>Lib: searchUserIssuesAndPRs({ username, mode, token, state, start, end, signal })
Lib->>GH: GET /search/issues?q=...&per_page=100 (date-windowed)
GH-->>Lib: Results (+ rate-limit headers / errors)
Lib->>Lib: Retry/backoff or bisect date window if total > 1000
Lib-->>Hook: Deduped, sorted items
Hook-->>UI: totals and client-side paginated slice
Estimated code review effort🎯 4 (Complex) | ⏱️ ~40 minutes Assessment against linked issues
Out-of-scope changes
Possibly related PRs
Suggested labels
Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
|
This PR addresses #89 by implementing central pagination handling via searchUserIssuesAndPRs. Tested with accounts exceeding the 1000-item search cap, and verified UI loads complete data. Ready for review. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Nitpick comments (6)
library/githubSearch.ts (3)
49-52: Improve rate limit handling precisionThe current implementation adds a fixed 1000ms buffer to the rate limit reset time, but this might be insufficient if there's clock skew between client and server.
if (resp.status === 403) { const reset = Number(resp.headers.get("X-RateLimit-Reset") || "0") * 1000; - const waitMs = Math.max(0, reset - Date.now()) + 1000; + // Add 2 seconds buffer to account for clock skew and network latency + const waitMs = Math.max(0, reset - Date.now()) + 2000; if (waitMs > 0) await sleep(waitMs);
145-149: Optimize deduplication logicThe current deduplication uses a comma operator which is less readable. Consider a clearer approach.
// de-dupe and sort newest-first const seen = new Set<number>(); - const unique = results.filter((it) => (seen.has(it.id) ? false : (seen.add(it.id), true))); + const unique = results.filter((it) => { + if (seen.has(it.id)) return false; + seen.add(it.id); + return true; + }); unique.sort((a, b) => b.created_at.localeCompare(a.created_at)); return unique;
165-170: Improve environment variable access safetyThe current implementation uses multiple
anycasts and property access that could fail. Consider a safer approach.const token = (opts.userProvidedToken && opts.userProvidedToken.trim()) || - (typeof import.meta !== "undefined" && - (import.meta as any).env && - (import.meta as any).env.VITE_GITHUB_TOKEN) || + (typeof import.meta !== "undefined" && + import.meta.env?.VITE_GITHUB_TOKEN as string | undefined) || undefined;src/pages/ContributorProfile/ContributorProfile.tsx (2)
19-20: Avoid redundant loading state update
setLoading(true)is called when loading is already true by default (line 10), making this redundant on the first render.- setLoading(true); toastId = toast.loading("Fetching PRs…");
100-100: Potential runtime error with repository_url accessWhile optional chaining is used, the fallback to empty string could cause issues. Consider extracting just the repository name instead of the full path.
- const repoName = pr.repository_url?.split("/").slice(-2).join("/") ?? ""; + const repoName = pr.repository_url?.split("/").pop() ?? "Unknown";This extracts just the repository name rather than "owner/repo" format, which might be cleaner for display.
src/hooks/useGitHubData.ts (1)
17-26: Consider using a more reliable token reading approachThe current implementation catches all errors silently, which could hide legitimate issues. Also, the eslint disable comment suggests this approach might not be ideal.
const readToken = (): string | undefined => { - try { - // Vite exposes env under import.meta.env - // eslint-disable-next-line @typescript-eslint/no-explicit-any - const env = (import.meta as any)?.env as Record<string, string> | undefined; - return env?.VITE_GITHUB_TOKEN || undefined; - } catch { - return undefined; - } + // Safe access to Vite environment variables + if (typeof import.meta !== 'undefined' && import.meta.env) { + return import.meta.env.VITE_GITHUB_TOKEN as string | undefined; + } + return undefined; };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
library/githubSearch.ts(1 hunks)src/hooks/useGitHubData.ts(2 hunks)src/pages/ContributorProfile/ContributorProfile.tsx(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (3)
src/pages/ContributorProfile/ContributorProfile.tsx (2)
library/githubSearch.ts (1)
searchUserIssuesAndPRs(117-150)src/pages/Tracker/Tracker.tsx (1)
GitHubItem(38-46)
library/githubSearch.ts (1)
src/pages/Tracker/Tracker.tsx (4)
GitHubItem(38-46)theme(48-342)item(295-323)item(104-107)
src/hooks/useGitHubData.ts (3)
library/githubSearch.ts (1)
searchUserIssuesAndPRs(117-150)src/hooks/useGitHubAuth.ts (3)
username(4-25)octokit(14-14)username(9-12)src/pages/Tracker/Tracker.tsx (1)
GitHubItem(38-46)
🔇 Additional comments (2)
src/pages/ContributorProfile/ContributorProfile.tsx (1)
60-63: Good implementation of cleanup logicThe cancellation mechanism and toast cleanup in the useEffect return function properly prevents state updates after unmount and cleans up UI indicators.
src/hooks/useGitHubData.ts (1)
39-42: Good use of parallel fetchingThe implementation correctly uses
Promise.allto fetch issues and PRs concurrently, improving performance compared to sequential requests.
|
All review suggestions have been implemented. Marking this PR as ready for merge. |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
src/hooks/useGitHubData.ts (3)
41-43: Good pagination validationThe validation for
pageandperPageparameters prevents invalid array slicing operations downstream.
51-55: Rate limit handling provides clear user feedbackSetting a specific error message when rate-limited ensures users understand why the request failed.
73-76: Client-side pagination correctly implementedThe slicing logic with proper index calculations effectively implements client-side pagination on the fully-fetched results.
🧹 Nitpick comments (3)
vite.config.ts (1)
10-30: Consider allowing source maps in development buildsWhile disabling source maps in production is reasonable for performance and security, the
sourcemap: falsesetting applies to all builds. Consider enabling source maps conditionally for development builds to aid debugging.build: { target: "es2020", - sourcemap: false, // no source maps in prod build + sourcemap: process.env.NODE_ENV === 'development', // source maps only in dev cssCodeSplit: true, // keep CSS split for better cachingAlternatively, you can use Vite's mode-based configuration:
- sourcemap: false, + sourcemap: process.env.NODE_ENV !== 'production',src/hooks/useGitHubData.ts (1)
84-92: Consider more specific rate limit detectionThe current rate limit detection using string matching on "rate limit" or "403" might produce false positives. Consider checking for more specific GitHub API rate limit responses.
GitHub's rate limit errors typically include specific headers and status codes. Consider enhancing the detection:
- if (msg.toLowerCase().includes("rate limit") || msg.includes("403")) { + // GitHub returns 403 with specific rate limit message + if (msg.toLowerCase().includes("rate limit") || + msg.includes("API rate limit exceeded") || + (msg.includes("403") && msg.toLowerCase().includes("limit"))) { setRateLimited(true); }src/pages/ContributorProfile/ContributorProfile.tsx (1)
145-145: Add null check for repository_url parsingWhile optional chaining is used, consider adding explicit validation to handle edge cases where
repository_urlmight be malformed.- const repoName = pr.repository_url?.split("/").slice(-2).join("/") ?? ""; + const repoName = pr.repository_url && pr.repository_url.includes("/") + ? pr.repository_url.split("/").slice(-2).join("/") + : "unknown";
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
library/githubSearch.ts(1 hunks)src/hooks/useGitHubData.ts(2 hunks)src/pages/ContributorProfile/ContributorProfile.tsx(3 hunks)vite.config.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- library/githubSearch.ts
🧰 Additional context used
🧬 Code Graph Analysis (3)
vite.config.ts (1)
src/vite-env.d.ts (2)
ImportMetaEnv(3-6)ImportMeta(8-10)
src/pages/ContributorProfile/ContributorProfile.tsx (1)
src/pages/Tracker/Tracker.tsx (1)
GitHubItem(38-46)
src/hooks/useGitHubData.ts (2)
src/hooks/useGitHubAuth.ts (3)
username(4-25)octokit(14-14)username(9-12)src/pages/Tracker/Tracker.tsx (1)
GitHubItem(38-46)
🔇 Additional comments (12)
vite.config.ts (4)
21-27: Manual chunks configuration looks goodThe chunking strategy effectively separates React core, router, and vendor dependencies for optimal caching. The function-based approach correctly avoids Vite warnings.
33-39: HMR overlay configuration is properly documentedGood documentation explaining how to control the error overlay during development. The default
truesetting is appropriate for most development scenarios.
46-48: React deduplication prevents version conflictsThe deduplication of React and React DOM is crucial for preventing the "multiple React instances" error that can occur when dependencies have their own React versions. This is especially important with the new GitHub data fetching architecture.
50-53: Pre-bundling configuration optimizes cold startsIncluding React, React DOM, and React Router DOM in
optimizeDepsensures these core dependencies are pre-bundled during dev server startup, significantly improving cold start performance.src/hooks/useGitHubData.ts (2)
16-26: Secure token access implementationThe
readTokenfunction safely accesses the environment variable with proper error handling. The try-catch block prevents runtime errors ifimport.metais unavailable, and the defensive approach with optional chaining is appropriate.
58-83: Robust request cancellation patternThe implementation correctly handles both external and internal abort signals, ensuring proper cleanup to prevent memory leaks and state updates on unmounted components. The finally block appropriately aborts only internally-created controllers.
src/pages/ContributorProfile/ContributorProfile.tsx (6)
21-24: Well-structured cleanup trackingGood use of
controllerfor AbortSignal,canceledflag for additional safety, andtoastIdfor proper toast cleanup. This prevents race conditions and ensures clean component unmounting.
33-43: Token authentication properly implementedThe token is correctly retrieved from environment variables and conditionally added to headers. The use of spread operator for conditional header inclusion is clean and follows best practices.
44-51: Comprehensive error handling for user fetchExcellent handling of different error scenarios - specifically checking for 404 to provide user-friendly "User not found" messages while also handling other HTTP errors appropriately.
57-65: Efficient PR fetching with proper cancellation checksThe migration to
searchUserIssuesAndPRscentralizes pagination logic effectively. The post-fetch sorting byupdated_atensures recent PRs appear first, and the cancellation check prevents state updates after unmount.
91-108: Clipboard API with fallback implementationExcellent implementation using the modern Clipboard API with a fallback for older browsers. The async/await pattern with proper error handling and user feedback via toasts is well done.
155-157: Merged PR badge implementationGood visual indicator for merged PRs using conditional rendering and appropriate styling with dark mode support.
|
All nitpicks addressed, ready to merge |
|
Refactor: centralize GitHub search helper and improve safety checks
|
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
src/pages/ContributorProfile/ContributorProfile.tsx (1)
16-16: Type-safety improvement acknowledged.Switching
prstoGitHubSearchItem[]aligns withsearchUserIssuesAndPRsand resolves the earlier regression.
🧹 Nitpick comments (3)
src/pages/ContributorProfile/ContributorProfile.tsx (3)
56-65: Document the custom sort criteria (updated_at vs created_at).
searchUserIssuesAndPRsreturns items sorted by created_at; here you re-sort by updated_at. Add a brief comment to avoid future confusion.Apply this diff:
- setPRs([...prItems].sort((a, b) => new Date(b.updated_at).getTime() - new Date(a.updated_at).getTime())); + // Sort by last update time to surface the most recent activity + setPRs([...prItems].sort((a, b) => new Date(b.updated_at).getTime() - new Date(a.updated_at).getTime()));
91-107: Clipboard fallback: hide the temporary textarea to avoid layout shifts.Moving the textarea off-screen prevents jank and accidental scrolling during selection.
Apply this diff:
- const el = document.createElement("textarea"); - el.value = window.location.href; - document.body.appendChild(el); - el.select(); - document.execCommand("copy"); - document.body.removeChild(el); + const el = document.createElement("textarea"); + el.value = window.location.href; + el.setAttribute("readonly", ""); + el.style.position = "fixed"; + el.style.top = "-9999px"; + document.body.appendChild(el); + el.select(); + el.setSelectionRange(0, el.value.length); + document.execCommand("copy"); + document.body.removeChild(el);
121-126: A11y: Provide a more descriptive alt text for the avatar.Improves screen reader context.
Apply this diff:
- alt="Avatar" + alt={`${profile.login}'s avatar`}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/hooks/useGitHubData.ts(2 hunks)src/pages/ContributorProfile/ContributorProfile.tsx(3 hunks)vite.config.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/hooks/useGitHubData.ts
- vite.config.ts
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/pages/ContributorProfile/ContributorProfile.tsx (2)
library/githubSearch.ts (2)
GitHubSearchItem(6-17)searchUserIssuesAndPRs(128-162)src/pages/Tracker/Tracker.tsx (1)
GitHubItem(38-46)
🔇 Additional comments (1)
src/pages/ContributorProfile/ContributorProfile.tsx (1)
81-88: Cleanup is solid: aborts pending requests and dismisses toasts.Good use of
AbortController,canceledguard, and toast cleanup to prevent setState-on-unmounted and stray toasts.
|
All suggested changes have been applied |
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
src/pages/ContributorProfile/ContributorProfile.tsx (1)
121-125: Don't hide non-404 errors: early return masks errorMsg when profile is nullIf the profile fetch fails due to rate limits or network errors,
profileremains null but the UI always shows "User not found.", hiding the actual error stored inerrorMsg. Make the early-return rendererrorMsgwhen present.Apply this diff:
- if (!profile) - return ( - <div className="text-center mt-10 text-red-600">User not found.</div> - ); + if (!profile) { + return ( + <div className="text-center mt-10 text-red-600"> + {errorMsg ?? "User not found."} + </div> + ); + }
♻️ Duplicate comments (1)
src/pages/ContributorProfile/ContributorProfile.tsx (1)
26-31: Good fix: prevent indefinite loading when username is missingEarly-return now resets loading and surfaces a helpful error message. This avoids a stuck "Loading..." UI.
🧹 Nitpick comments (2)
src/pages/ContributorProfile/ContributorProfile.tsx (2)
50-56: Improve rate-limit error messaging for user profile fetch (403)When the user request is rate-limited (403), the current message is generic. Offer a clearer message and hint for dev token usage.
Apply this diff:
- if (!userRes.ok) { - if (userRes.status === 404) { - setProfile(null); - throw new Error("User not found"); - } - throw new Error(`Failed to fetch user: ${userRes.status}`); - } + if (!userRes.ok) { + if (userRes.status === 404) { + setProfile(null); + throw new Error("User not found"); + } + if (userRes.status === 403) { + const rem = userRes.headers.get("x-ratelimit-remaining"); + const msg = + rem === "0" + ? "GitHub API rate limit exceeded. Please try again later." + : "Access forbidden. If in development, set VITE_GITHUB_TOKEN."; + throw new Error(msg); + } + throw new Error(`Failed to fetch user: ${userRes.status}`); + }
153-156: Repository name derivation is safe; consider URL parsing for clarity (optional)Current split logic works; a URL parse is slightly more robust and self-documenting.
Apply this diff:
- const repoName = pr.repository_url && pr.repository_url.includes("/") - ? pr.repository_url.split("/").slice(-2).join("/") - : "unknown"; + const repoName = (() => { + if (!pr.repository_url) return "unknown"; + try { + const url = new URL(pr.repository_url); + const parts = url.pathname.split("/").filter(Boolean); + return parts.slice(-2).join("/"); + } catch { + return "unknown"; + } + })();
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/pages/ContributorProfile/ContributorProfile.tsx(3 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/pages/ContributorProfile/ContributorProfile.tsx (2)
library/githubSearch.ts (2)
GitHubSearchItem(6-17)searchUserIssuesAndPRs(128-162)src/pages/Tracker/Tracker.tsx (1)
GitHubItem(38-46)
🔇 Additional comments (3)
src/pages/ContributorProfile/ContributorProfile.tsx (3)
37-39: Token gated to dev builds — avoids shipping secretsReading
VITE_GITHUB_TOKENonly whenimport.meta.env.DEVis true prevents leaking a PAT in production bundles.
21-24: Solid abort/cancel disciplineUsing
AbortController, acanceledflag, and ignoringAbortErrorprevents spurious error toasts and state updates after unmount/navigation.Also applies to: 71-79, 80-85
4-4: Import path is correct — no change required.Verified that library/githubSearch.ts exists at the repository root and the import in src/pages/ContributorProfile/ContributorProfile.tsx correctly resolves to it.
- Found file: library/githubSearch.ts
- Import (src/pages/ContributorProfile/ContributorProfile.tsx:4):
import { searchUserIssuesAndPRs, GitHubSearchItem } from "../../../library/githubSearch";
|
I’ve incorporated the suggested changes in commit 7be569b and confirmed that all checks have passed with no merge conflicts. The PR should now be good to go |
1 participant
Related Issue
• Closes #89
⸻
Description
This PR refactors the data fetching logic for GitHub issues and PRs to use a central searchUserIssuesAndPRs helper for pagination.
Previously, direct search calls were limited by GitHub’s 1000 results cap, causing missing data for users with high activity.
The updated implementation uses robust date-window pagination, ensuring all results are fetched without hitting the cap.
Key changes:
• Removed duplicate search logic from individual hooks
• Integrated searchUserIssuesAndPRs in useGitHubData for both issues & PRs
• Implemented client-side slicing for requested pages after fetching the complete dataset
• Improved rate-limit error handling
⸻
How Has This Been Tested?
• Ran npm run dev locally and tested with accounts exceeding 1000 results
• Verified pagination returns all expected results for both issues & PRs
• Confirmed UI loads correct results per page without truncation
⸻
Type of Change
• Bug fix
• New feature
• Code style update
• Breaking change
• Documentation update
Summary by CodeRabbit
New Features
Bug Fixes
Chores