refactor: update github, api client #89
Conversation
…de clarity - Simplified token retrieval logic to only read from environment variables on the server side. - Updated token information retrieval to enhance clarity and consistency. - Removed unused rate limit handling code to streamline the API client.
WalkthroughIntroduces Docker-based development workflows via a new Makefile with comprehensive targets, updates the VSCode workspace settings for Makefile support, changes the npm dev script from turbopack to turbo with host binding, expands README documentation with setup and Docker instructions, and reformats TypeScript types and public API signatures in the GitHub API client module. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes The pull request spans heterogeneous change categories (configuration, documentation, scripts, type system), with the API client file requiring careful type review. Most changes are straightforward reformatting or documentation updates, but the expanded public API surface and type refinements in Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ 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 |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
package.json (1)
5-10: Makefile mentions “npm run format” but script is missing. Add it or remove the help line.Apply one of:
"scripts": { - "dev": "next dev --turbo -H 0.0.0.0", + "dev": "next dev --turbo -H 0.0.0.0", "build": "next build", "start": "next start", - "lint": "next lint" + "lint": "next lint", + "format": "prettier -w ." },Or delete the Makefile help entry for “npm run format”.
src/lib/api/github-api-client.ts (2)
179-186: Guard process.env in browser contexts to avoid ReferenceError.- getTokenInfo(): { hasToken: boolean; tokenPrefix: string; source: string } { + getTokenInfo(): { hasToken: boolean; tokenPrefix: string; source: string } { + const envToken = + typeof process !== "undefined" && process.env?.GITHUB_TOKEN + ? process.env.GITHUB_TOKEN + : undefined; return { hasToken: !!this.githubToken, tokenPrefix: this.githubToken ? this.githubToken.substring(0, 10) + '...' : 'NO_TOKEN', - source: this.githubToken === process.env.GITHUB_TOKEN ? 'ENV_VAR' : 'USER_SET' + source: envToken && this.githubToken === envToken ? 'ENV_VAR' : 'USER_SET' } }
927-991: Commit count: fallback not executed when stats exist but don’t include the user.Run the lightweight listing fallback when stats don’t contain the author:
- if (stats && Array.isArray(stats)) { - const userStats = stats.find( - (stat) => stat.author?.login === username - ); - if (userStats && userStats.total > 0) { - return userStats.total; - } - } + if (stats && Array.isArray(stats)) { + const userStats = stats.find((stat) => stat.author?.login === username); + if (userStats && userStats.total > 0) { + return userStats.total; + } + // fall through to broad fallback + } @@ - return 0; + return 0;(The key change is removing the early “return 0” path and allowing the outer fallback logic to run.)
🧹 Nitpick comments (8)
Makefile (2)
1-1: Declare alias targets as PHONY and optionally add conventional “all/clean/test” to satisfy checkmake.Apply:
-.PHONY: help setup env docker-dev docker-dev-build docker-dev-logs docker-dev-down docker-prod docker-up docker-down docker-logs docker-restart clean-docker +.PHONY: help setup env docker-dev docker-dev-build docker-dev-logs docker-dev-down docker-prod docker-up docker-down docker-logs docker-restart clean-docker \ + dev prod up down logs restart all clean testAnd add minimal conventional targets:
+# Conventional aliases +all: help +clean: clean-docker +test: + @echo "No tests defined"
93-98: System-wide prune is aggressive; confirm intent.
docker system prune -fremoves unused resources globally, not just this project. Consider scoping cleanup (networks/volumes with project label) or documenting the blast radius.README.md (2)
231-246: Fix “emphasis used as heading” (MD036) in Windows section.Replace bold lines with proper subheadings:
-**Option 1: Use npm directly** (Easiest) +#### Option 1: Use npm directly (Easiest) ... -**Option 2: Install Make** +#### Option 2: Install Make ... -**Option 3: Use Git Bash** +#### Option 3: Use Git Bash
183-201: Minor: “make env” description.Table says “Same as setup” but
setupprints guidance afterenv. Consider clarifying “create .env.local (setup also prints guidance)”.src/lib/api/github-api-client.ts (4)
7-11: Don’t duplicate GitHubSearchResponse; import the shared type.-import type { TrendingRepo, TopContributor } from "@/types/oss-insight"; +import type { TrendingRepo, TopContributor } from "@/types/oss-insight"; +import type { GitHubSearchResponse } from "@/types/api"; @@ -interface GitHubSearchResponse<T> { - items: T[]; - total_count: number; - incomplete_results: boolean; -} +// use shared GitHubSearchResponse from @/types/apiPrevents drift with central API typings. Based on relevant snippets.
1001-1003: Minor: unused variable.
oneWeekAgois computed but unused in getWeeklyBehaviorData; remove it.
596-687: Performance: avoid duplicate repo fetches in getUserAnalytics.
getUserAnalyticsfetches repos andgetUserLanguagesfetches repos again. Pass the already-fetched repos into a newcomputeUserLanguages(repos)to cut API calls and latency. Offer to provide a patch if you prefer this now.
372-543: Types: methods return unknown[]. Define stable shapes.
getAssignedItems/getMentionItems/getAuthoredItems/getReviewRequestItemsall returnunknown[]. Export explicit interfaces for these items (e.g., AssignedItem, MentionItem) to improve safety and DX.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
.vscode/settings.json(1 hunks)Makefile(1 hunks)README.md(4 hunks)package.json(1 hunks)src/lib/api/github-api-client.ts(13 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/lib/api/github-api-client.ts (3)
src/types/api.ts (1)
GitHubSearchResponse(126-130)src/types/oss-insight.ts (2)
TrendingRepo(1-36)TopContributor(69-87)src/types/github.ts (2)
GitHubUserDetailed(73-106)GitHubRepositoryDetailed(108-195)
🪛 checkmake (0.2.2)
Makefile
[warning] 8-8: Target body for "help" exceeds allowed length of 5 (28).
(maxbodylength)
[warning] 44-44: Target body for "env" exceeds allowed length of 5 (6).
(maxbodylength)
[warning] 1-1: Missing required phony target "all"
(minphony)
[warning] 1-1: Missing required phony target "clean"
(minphony)
[warning] 1-1: Missing required phony target "test"
(minphony)
[warning] 101-101: Target "dev" should be declared PHONY.
(phonydeclared)
[warning] 102-102: Target "prod" should be declared PHONY.
(phonydeclared)
[warning] 103-103: Target "up" should be declared PHONY.
(phonydeclared)
[warning] 104-104: Target "down" should be declared PHONY.
(phonydeclared)
[warning] 105-105: Target "logs" should be declared PHONY.
(phonydeclared)
[warning] 106-106: Target "restart" should be declared PHONY.
(phonydeclared)
🪛 LanguageTool
README.md
[style] ~216-~216: It’s more common nowadays to write this noun as one word.
Context: ...ely ## Settings - Change organization/user name and token anytime from the Settings pag...
(RECOMMENDED_COMPOUNDS)
🪛 markdownlint-cli2 (0.18.1)
README.md
237-237: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
243-243: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
🔇 Additional comments (2)
.vscode/settings.json (1)
2-2: LGTM — aligns editor with new Makefile workflow.package.json (1)
6-6: Dev script: verify flags in your environment.Next 15 supports --turbo and -H 0.0.0.0; confirm it binds as expected inside docker-dev and that port mapping matches your compose file.
| if (!token || typeof token !== "string" || token.trim().length === 0) { | ||
| throw new Error("Invalid GitHub token: must be a non-empty string"); | ||
| } | ||
|
|
||
| const trimmedToken = token.trim() | ||
|
|
||
| const isClassicToken = /^ghp_[A-Za-z0-9]{36}$/.test(trimmedToken) | ||
| const isFineGrainedToken = /^github_pat_[A-Za-z0-9_]{82}$/.test(trimmedToken) | ||
| const isGitHubAppToken = /^ghs_[A-Za-z0-9]{36}$/.test(trimmedToken) | ||
| const isLegacyToken = /^[a-f0-9]{40}$/.test(trimmedToken) | ||
| const trimmedToken = token.trim(); | ||
|
|
||
| const isOAuthToken = /^gho_[A-Za-z0-9_-]{16,}$/.test(trimmedToken) || | ||
| (!isClassicToken && !isFineGrainedToken && !isGitHubAppToken && !isLegacyToken && | ||
| /^[A-Za-z0-9_-]{20,255}$/.test(trimmedToken)) | ||
|
|
||
| if (!isClassicToken && !isFineGrainedToken && !isGitHubAppToken && !isLegacyToken && !isOAuthToken) { | ||
| const isClassicToken = /^ghp_[A-Za-z0-9]{36}$/.test(trimmedToken); | ||
| const isFineGrainedToken = /^github_pat_[A-Za-z0-9_]{82}$/.test( | ||
| trimmedToken | ||
| ); | ||
| const isGitHubAppToken = /^ghs_[A-Za-z0-9]{36}$/.test(trimmedToken); | ||
| const isLegacyToken = /^[a-f0-9]{40}$/.test(trimmedToken); | ||
|
|
||
| const isOAuthToken = | ||
| /^gho_[A-Za-z0-9_-]{16,}$/.test(trimmedToken) || | ||
| (!isClassicToken && | ||
| !isFineGrainedToken && | ||
| !isGitHubAppToken && | ||
| !isLegacyToken && | ||
| /^[A-Za-z0-9_-]{20,255}$/.test(trimmedToken)); | ||
|
|
||
| if ( | ||
| !isClassicToken && | ||
| !isFineGrainedToken && | ||
| !isGitHubAppToken && | ||
| !isLegacyToken && | ||
| !isOAuthToken | ||
| ) { | ||
| throw new Error( | ||
| 'Invalid GitHub token format. Expected:\n' + | ||
| '- Classic token: ghp_xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx (40 chars)\n' + | ||
| '- Fine-grained token: github_pat_xxxxxxxxxx... (94 chars)\n' + | ||
| '- GitHub App token: ghs_xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx (40 chars)\n' + | ||
| '- OAuth token: 20-255 character alphanumeric string\n' + | ||
| '- Legacy token: 40 character hexadecimal string' | ||
| ) | ||
| "Invalid GitHub token format. Expected:\n" + | ||
| "- Classic token: ghp_xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx (40 chars)\n" + | ||
| "- Fine-grained token: github_pat_xxxxxxxxxx... (94 chars)\n" + | ||
| "- GitHub App token: ghs_xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx (40 chars)\n" + | ||
| "- OAuth token: 20-255 character alphanumeric string\n" + | ||
| "- Legacy token: 40 character hexadecimal string" | ||
| ); | ||
| } | ||
|
|
||
| if (trimmedToken.length < 40) { | ||
| throw new Error('GitHub token is too short. Minimum length is 40 characters.') | ||
| throw new Error( | ||
| "GitHub token is too short. Minimum length is 40 characters." | ||
| ); | ||
| } | ||
|
|
||
| if (trimmedToken.length > 255) { | ||
| throw new Error('GitHub token is too long. Maximum length is 255 characters.') | ||
| throw new Error( | ||
| "GitHub token is too long. Maximum length is 255 characters." | ||
| ); | ||
| } | ||
|
|
||
| this.githubToken = trimmedToken | ||
| this.githubToken = trimmedToken; | ||
| } |
There was a problem hiding this comment.
Token validation is inconsistent and overly rigid (length checks conflict, fine-grained length mismatch). Simplify and align with hasValidToken.
- Remove hard min 40/max 255 constraints; rely on prefix-based plausibility and a general 20–255 bound.
- Accept common prefixes: ghp_, github_pat_, ghs_, gho_, legacy 40-hex.
- Clear cache on token change.
Apply:
setUserToken(token: string) {
- if (!token || typeof token !== "string" || token.trim().length === 0) {
+ if (!token || typeof token !== "string" || token.trim().length === 0) {
throw new Error("Invalid GitHub token: must be a non-empty string");
}
-
- const trimmedToken = token.trim();
-
- const isClassicToken = /^ghp_[A-Za-z0-9]{36}$/.test(trimmedToken);
- const isFineGrainedToken = /^github_pat_[A-Za-z0-9_]{82}$/.test(
- trimmedToken
- );
- const isGitHubAppToken = /^ghs_[A-Za-z0-9]{36}$/.test(trimmedToken);
- const isLegacyToken = /^[a-f0-9]{40}$/.test(trimmedToken);
-
- const isOAuthToken =
- /^gho_[A-Za-z0-9_-]{16,}$/.test(trimmedToken) ||
- (!isClassicToken &&
- !isFineGrainedToken &&
- !isGitHubAppToken &&
- !isLegacyToken &&
- /^[A-Za-z0-9_-]{20,255}$/.test(trimmedToken));
-
- if (
- !isClassicToken &&
- !isFineGrainedToken &&
- !isGitHubAppToken &&
- !isLegacyToken &&
- !isOAuthToken
- ) {
+ const trimmedToken = token.trim();
+ const plausible =
+ // Classic
+ /^ghp_[A-Za-z0-9]+$/.test(trimmedToken) ||
+ // Fine‑grained
+ /^github_pat_[A-Za-z0-9_]+$/.test(trimmedToken) ||
+ // GitHub App
+ /^ghs_[A-Za-z0-9]+$/.test(trimmedToken) ||
+ // OAuth
+ /^gho_[A-Za-z0-9_-]+$/.test(trimmedToken) ||
+ // Legacy 40-hex
+ /^[a-f0-9]{40}$/.test(trimmedToken) ||
+ // Fallback: accept sane bounds
+ /^[A-Za-z0-9._-]{20,255}$/.test(trimmedToken);
+
+ if (!plausible) {
throw new Error(
"Invalid GitHub token format. Expected:\n" +
- "- Classic token: ghp_xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx (40 chars)\n" +
- "- Fine-grained token: github_pat_xxxxxxxxxx... (94 chars)\n" +
- "- GitHub App token: ghs_xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx (40 chars)\n" +
- "- OAuth token: 20-255 character alphanumeric string\n" +
- "- Legacy token: 40 character hexadecimal string"
+ "- Classic token: ghp_...\n" +
+ "- Fine-grained token: github_pat_...\n" +
+ "- GitHub App token: ghs_...\n" +
+ "- OAuth token: gho_...\n" +
+ "- Legacy token: 40-character hex"
);
}
-
- if (trimmedToken.length < 40) {
- throw new Error(
- "GitHub token is too short. Minimum length is 40 characters."
- );
- }
-
- if (trimmedToken.length > 255) {
- throw new Error(
- "GitHub token is too long. Maximum length is 255 characters."
- );
- }
-
- this.githubToken = trimmedToken;
+ this.githubToken = trimmedToken;
+ // prevent cross-token cache reuse
+ this.cache.clear();
}And align hasValidToken (see next comment).
🤖 Prompt for AI Agents
In src/lib/api/github-api-client.ts around lines 118 to 169, the token
validation is inconsistent (rigid min/max length conflicts with prefix checks
and fine-grained token length) and doesn't clear cached state when the token
changes; update validation to accept tokens by common prefixes (ghp_,
github_pat_, ghs_, gho_, 40-hex legacy) and otherwise apply a general
plausibility length range of 20–255 characters (remove the hard min 40 and hard
max 255 checks in their current places), consolidate the regex checks into a
single plausible-token condition that matches those prefixes or a 40-hex legacy,
throw a single clear error when the token fails that combined check, and ensure
you clear any related cached authentication/state when assigning a new token so
changing tokens invalidates caches; also update or align the hasValidToken
implementation to use the same combined validation logic.
| hasValidToken(): boolean { | ||
| return this.githubToken.length >= 20 | ||
| return this.githubToken.length >= 20; | ||
| } | ||
|
|
||
| clearToken(): void { | ||
| this.githubToken = '' | ||
| this.githubToken = ""; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Align hasValidToken with setUserToken and clear cache on clearToken.
hasValidToken(): boolean {
- return this.githubToken.length >= 20;
+ return !!this.githubToken && this.githubToken.length >= 20;
}
clearToken(): void {
- this.githubToken = "";
+ this.githubToken = "";
+ this.cache.clear();
}Optionally extract a shared isTokenPlausible() and use it here for consistency.
🤖 Prompt for AI Agents
In src/lib/api/github-api-client.ts around lines 171 to 177, hasValidToken()
currently only checks length and should be aligned with the validation logic
used in setUserToken; create a single helper (isTokenPlausible) that
encapsulates the same plausibility rules used when setting the token, have
hasValidToken() call that helper, and update clearToken() to not only zero out
this.githubToken but also clear any related cached state (e.g. nullify any
cached GitHub client, session, or token-derived caches) so clearing the token
fully invalidates prior credentials.
| const mentions = | ||
| mentionsResponse.items?.map((item: GitHubIssueResponse) => ({ | ||
| id: item.id, | ||
| title: item.title, | ||
| repo: item.repository_url.split("/").slice(-2).join("/"), | ||
| type: item.pull_request ? "pr" : "issue", | ||
| priority: this.calculatePriority(item), | ||
| url: item.html_url, | ||
| createdAt: item.created_at, | ||
| updatedAt: item.updated_at, | ||
| author: item.user?.login, | ||
| labels: item.labels?.map((l: { name: string }) => l.name) || [], | ||
| mentionType: "mention", | ||
| mentionedAt: item.updated_at, | ||
| })) || []; | ||
|
|
There was a problem hiding this comment.
Null-guard repository_url in mentions mapping.
- repo: item.repository_url.split("/").slice(-2).join("/"),
+ repo: item.repository_url
+ ? item.repository_url.split("/").slice(-2).join("/")
+ : "unknown/unknown",🤖 Prompt for AI Agents
In src/lib/api/github-api-client.ts around lines 431 to 446, the mapping assumes
item.repository_url is always present and calls split on it, which can throw if
repository_url is null/undefined; update the mapping to null-guard
repository_url (e.g., if item.repository_url is truthy then derive repo with
split(...).slice(-2).join("/"), otherwise fall back to
item.repository?.full_name or an empty string) and adjust types accordingly so
repo is always a string and no runtime error occurs.
| if (overview.length === 0) { | ||
| console.warn('No repositories found, using demo overview'); | ||
| console.warn("No repositories found, using demo overview"); | ||
| return this.getUserAnalytics(username); | ||
| } | ||
|
|
There was a problem hiding this comment.
Infinite recursion when no repos -> potential crash. Replace recursion with safe fallback.
Apply:
- if (overview.length === 0) {
- console.warn("No repositories found, using demo overview");
- return this.getUserAnalytics(username);
- }
+ if (overview.length === 0) {
+ console.warn("No repositories found; returning empty analytics");
+ return { profile, overview: [], languages, behavior: await this.getWeeklyBehaviorData(username) };
+ }📝 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.
| if (overview.length === 0) { | |
| console.warn('No repositories found, using demo overview'); | |
| console.warn("No repositories found, using demo overview"); | |
| return this.getUserAnalytics(username); | |
| } | |
| if (overview.length === 0) { | |
| console.warn("No repositories found; returning empty analytics"); | |
| return { profile, overview: [], languages, behavior: await this.getWeeklyBehaviorData(username) }; | |
| } |
🤖 Prompt for AI Agents
In src/lib/api/github-api-client.ts around lines 850 to 854, the branch that
handles an empty overview currently calls this.getUserAnalytics(username) which
can recurse indefinitely; replace that recursive call with a safe, non-recursive
fallback (e.g., return a precomputed demoOverview or an empty overview/analytics
object). Specifically, remove the this.getUserAnalytics call, return a
deterministic fallback value of the same shape as a normal successful response
(and keep the warning log), and ensure any callers can handle the demo/empty
structure without triggering another call into this method.
2 participants
Summary by CodeRabbit
New Features
Documentation
Chores