refactor: decouple github.ts and enhance type safety + test coverage#19
Open
0xarchit wants to merge 9 commits into
Open
refactor: decouple github.ts and enhance type safety + test coverage#190xarchit wants to merge 9 commits into
0xarchit wants to merge 9 commits into
Conversation
…star-gate,index}.ts - http.ts: shared fetch utilities, token pool, GraphQL/REST helpers - profile.ts: getProfileSummary, calculateDetailedStreaks, rank/trophy/badge logic - star-gate.ts: checkStarStatus, verifyAndInjectStar, getRepoStarCount - index.ts: barrel re-export (import path unchanged: @/lib/github) - Collapse 4 near-duplicate recursive paginators into 2 generic paginateForward/paginateBackward helpers (~250 lines removed) - Move achievement cache from in-process Map to Redis (Finding 6 combined) Breaking: none — all public exports preserved at @/lib/github
…hGitHubGraphQL - Remove local GITHUB_TOKENS re-declaration - Replace raw fetch() (no timeout, no retry) with fetchGitHubGraphQL from @/lib/github - Use getFallbackToken() for consistent token selection - Route now inherits 10s timeout and structured error logging automatically
auth.ts: - Add isValidSession(p: JWTPayload) type predicate with runtime field checks (githubId: number, username: string, accessToken: string, avatarUrl: string) - Replace 'payload as unknown as Session' double-cast with predicate guard - Replace 'payload.username as string' cast with typeof guard db.ts: - Validate id, github_id (number/bigint) and username (string) in materializeUser() before casting raw DB row to User - Returns null on schema mismatch instead of silently producing undefined fields
Before: populateAnalysis() + saveScan() + return were copy-pasted in both the force=true and force=false branches (~40 duplicate lines). After: force flag only controls whether the Redis cache read is skipped. A single populateAnalysis() call, single saveScan() guard, and single NextResponse.json(finalData) return cover both paths. Also: standardise indentation to spaces throughout the file.
Setup: - vitest.config.ts: node environment, @/ path alias, v8 coverage - src/test-setup.ts: stubs required env vars (GITHUB_TOKENS, JWT_SECRET) - package.json: add test / test:watch / test:coverage scripts Tests (10 passing): - src/lib/github/__tests__/profile.test.ts 10 cases for calculateDetailedStreaks: empty input, no contributions, single day, consecutive days, gap mid-sequence, grace period (today=0), two-zero-day reset, weekly streak counts, weekly grace period, weekly two-zero-week reset - src/lib/__tests__/auth.test.ts (pending env fixes for jose import) - src/lib/__tests__/ai.test.ts (pending fetch mock wiring)
ai.ts: getAIAnalysis — documents userToken, alertCollector, CORRUPT_INTELLIGENCE error
db.ts: updateUserSettings — documents normalizeSettingsPatch whitelist + INVALID_PRIMARY_SCAN
saveScan — documents 10-scan rolling window retention
auth.ts: createSession — documents cookie name (gitscore_session) and 7-day expiry
verifySession — documents isValidSession predicate and return contract
getSession — documents delegation to verifySession
Also: remove unused imports from auth.test.ts (flagged by tsc)
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughAdds Vitest tooling, hardens auth and DB validation, extracts shared GitHub helpers and modules, and updates the analyze and contributions API routes with tests. ChangesGitHub analyzer backend
Sequence Diagram(s)sequenceDiagram
participant AnalyzeRoute
participant checkStarStatus
participant getProfileSummary
participant getAIAnalysis
participant saveScan
AnalyzeRoute->>checkStarStatus: verify star access
alt cached analysis available
AnalyzeRoute-->>AnalyzeRoute: return cached payload
else cache miss
AnalyzeRoute->>getProfileSummary: fetch profile snapshot
AnalyzeRoute->>getAIAnalysis: generate analysis
AnalyzeRoute->>saveScan: persist scan history when allowed
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
Deploying github-profile-analyzer with
|
| Latest commit: |
d25f1af
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://63a6020b.github-profile-analyzer.pages.dev |
| Branch Preview URL: | https://fix-decouple.github-profile-analyzer.pages.dev |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/app/api/contributions/route.ts (1)
81-100: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winMove token selection inside the guarded request block.
getFallbackToken()can throw, but this call happens before thetry, so token misconfiguration escapes the route’s JSON error handling.Proposed fix
- const token = getFallbackToken(); const query = ` @@ try { + const token = getFallbackToken(); const res = await fetchGitHubGraphQL(token, query, { login: targetUser });🤖 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/api/contributions/route.ts` around lines 81 - 100, Move the token lookup into the guarded request path so token misconfiguration is handled by the route’s JSON error response. In the contributions route, `getFallbackToken()` is currently called before the `try`, so wrap it with the `fetchGitHubGraphQL` call inside the same `try` block (and keep the existing error handling) to ensure any thrown token error is caught and returned consistently.
🧹 Nitpick comments (2)
src/app/api/analyze/__tests__/access-control.test.ts (1)
160-177: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAssert the star-check principal and token arguments.
This public non-owner path should lock down whether the route verifies the viewer or the target. Add an expectation for
checkStarStatusarguments so mismatched username/token principals are caught.Also applies to: 189-197
🤖 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/api/analyze/__tests__/access-control.test.ts` around lines 160 - 177, The public non-owner access-control test is missing an assertion for the `checkStarStatus` call, so it can’t catch cases where the route checks the wrong principal or token. Update the relevant `access-control.test.ts` cases around `getSession`, `getUserByGithubId`, `getUserByUsername`, and `checkStarStatus` to explicitly expect the viewer’s username and access token to be passed into `checkStarStatus`, ensuring the route uses the viewer identity rather than the target profile.src/lib/github/profile.ts (1)
201-208: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winRemove or consume the contributed-repos query.
repositoriesContributedTois fetched with nested fields but never used in the returnedProfileSummary, so every profile call spends extra GraphQL cost for discarded data.🤖 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/lib/github/profile.ts` around lines 201 - 208, The repositoriesContributedTo query is fetched in the profile summary GraphQL request but its data is never used. Remove the repositoriesContributedTo selection from the profile query in profile.ts, or wire its results into ProfileSummary if they are actually needed, so the ProfileSummary builder only requests fields it consumes.
🤖 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 `@package.json`:
- Around line 10-12: The coverage script is configured to use Vitest’s V8
provider, but the required companion package is missing from devDependencies.
Update package.json to add the coverage package needed by vitest.config.ts’s
coverage.provider setting, and keep the test:coverage script unchanged so it
works on a clean install.
In `@src/app/api/analyze/__tests__/access-control.test.ts`:
- Around line 84-93: The mocked return values in access-control.test.ts are
using unsafe as any casts, which triggers ESLint and can break CI. Replace the
casts on getProfileSummary and getAIAnalysis with properly typed fixtures, using
either shared fixture builders or an Awaited<ReturnType<typeof ...>> alias for
each mocked response shape. Update both mocked blocks in this test file so the
mocked objects satisfy the expected return types without any.
In `@src/lib/__tests__/ai.test.ts`:
- Around line 127-135: The “throws on HTTP 429 after all retries” test in
getAIAnalysis currently only checks the final rejection, so it can pass even if
the retry logic never runs. Update the test in ai.test.ts to assert the retry
path by verifying mockFetch is called the expected number of times and, if
possible, that the backoff/timer scheduling is triggered before the final throw.
Use the existing getAIAnalysis and mockFetch symbols to locate the relevant test
and make the assertion match the intended “after all retries” contract.
In `@src/lib/auth.ts`:
- Around line 89-90: The guest-session payload check in auth.ts is too loose
because `payload.verified` is treated as truthy instead of matching the exact
shape produced by `createGuestSession()`. Update the logic in the username
extraction path to require `payload.verified` to be the boolean `true` and keep
the `typeof payload.username === "string"` guard, so malformed tokens like
string-valued `verified` are rejected.
In `@src/lib/github/http.ts`:
- Around line 13-19: Move the GITHUB_TOKENS presence check out of module
initialization in the github/http module so imports do not fail when the env var
is missing; keep the validation inside getFallbackToken() where token selection
actually happens. While parsing GITHUB_TOKENS, trim each entry before filtering
so whitespace-only values are ignored, and ensure getFallbackToken() is the
place that throws the missing-token error.
In `@src/lib/github/profile.ts`:
- Around line 193-201: The GitHub profile aggregation in profile.ts is
incorrectly using repositories(first: 60) as the source for full-profile
metrics, which undercounts stats for users with more than 60 owned repos. Update
the aggregation logic around the repositories and profile totals so
authoredRepos remains capped if needed, but total_stars, public_repo_count,
rank, and language weighting are computed from complete profile-level data
rather than the first 60 nodes; make the same correction in the related
aggregation code paths referenced by repositoriesContributedTo and the
downstream stats/ranking helpers.
- Around line 59-75: Use a cacheable sentinel value for missing achievements in
getAchievementStatus so negative results can be read back from cache; since
getCachedData() treats null as a miss, replace the null writes in the successful
200 check and the catch path with a distinct stored value and map it back to
null when reading. Keep the fix localized to the cache read/write logic around
getCachedData, setCachedData, and githubFetchWithTimeout in profile.ts.
In `@src/lib/github/star-gate.ts`:
- Around line 437-459: The fallback in executeRestFallbackStarCheck is
under-scanning when repoStars/userStars are unavailable because it only checks
one page, which can miss valid stars beyond the first 100. Update the paging
logic in executeRestFallbackStarCheck (and the related call path at the other
referenced location) so unknown counts trigger a broader scan strategy instead
of defaulting to a single page, while still respecting STAR_REST_LIMIT and using
scanRepoStargazersRest and scanUserStarredRest to iterate through enough pages.
---
Outside diff comments:
In `@src/app/api/contributions/route.ts`:
- Around line 81-100: Move the token lookup into the guarded request path so
token misconfiguration is handled by the route’s JSON error response. In the
contributions route, `getFallbackToken()` is currently called before the `try`,
so wrap it with the `fetchGitHubGraphQL` call inside the same `try` block (and
keep the existing error handling) to ensure any thrown token error is caught and
returned consistently.
---
Nitpick comments:
In `@src/app/api/analyze/__tests__/access-control.test.ts`:
- Around line 160-177: The public non-owner access-control test is missing an
assertion for the `checkStarStatus` call, so it can’t catch cases where the
route checks the wrong principal or token. Update the relevant
`access-control.test.ts` cases around `getSession`, `getUserByGithubId`,
`getUserByUsername`, and `checkStarStatus` to explicitly expect the viewer’s
username and access token to be passed into `checkStarStatus`, ensuring the
route uses the viewer identity rather than the target profile.
In `@src/lib/github/profile.ts`:
- Around line 201-208: The repositoriesContributedTo query is fetched in the
profile summary GraphQL request but its data is never used. Remove the
repositoriesContributedTo selection from the profile query in profile.ts, or
wire its results into ProfileSummary if they are actually needed, so the
ProfileSummary builder only requests fields it consumes.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2764f00b-8ddf-47e7-83d5-aad1bb2bf0df
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (17)
package.jsonsrc/app/api/analyze/__tests__/access-control.test.tssrc/app/api/analyze/route.tssrc/app/api/contributions/route.tssrc/lib/__tests__/ai.test.tssrc/lib/__tests__/auth.test.tssrc/lib/ai.tssrc/lib/auth.tssrc/lib/db.tssrc/lib/github.tssrc/lib/github/__tests__/profile.test.tssrc/lib/github/http.tssrc/lib/github/index.tssrc/lib/github/profile.tssrc/lib/github/star-gate.tssrc/test-setup.tsvitest.config.ts
💤 Files with no reviewable changes (1)
- src/lib/github.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/lib/ai.ts (1)
181-203: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winReset per-attempt response state to avoid stale error propagation.
responsesurvives across retries. If a non-OK response happens on an earlier attempt and a later attempt aborts/throws before assignment, the stale response is reused after the loop, masking the final error path (including abort classification inhandleFailure).Proposed fix
for (let attempt = 1; attempt <= AI_RETRY_ATTEMPTS; attempt++) { + response = null; try { console.log("[AI_ANALYSIS] Sending request to GitHub AI API", { attempt, url: "https://models.github.ai/inference/chat/completions", model: GITHUB_MODEL, }); - response = await callAIWithTimeout( + response = await callAIWithTimeout( apiKey, systemPrompt, minified, attempt, );🤖 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/lib/ai.ts` around lines 181 - 203, Reset the per-attempt response state inside the retry loop in ai.ts so a failed response from an earlier attempt cannot leak into later attempts or the post-loop failure handling. Update the retry logic around the callAIWithTimeout flow to avoid reusing a stale `response` value if a later attempt throws before assignment, and keep the final error path in `handleFailure` based only on the last attempt’s outcome. Use the `response` variable and the retry loop in the AI analysis request path as the place to fix this.
🤖 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/lib/github/profile.ts`:
- Line 295: The allRepos binding in the profile-related GitHub repo collection
logic is never reassigned, so change it from a mutable binding to a constant
while keeping the array contents mutable. Update the declaration in the code
path that builds allRepos from initialRepos, and leave the rest of the
surrounding profile.ts logic unchanged.
- Around line 304-311: The repository pagination in the profile aggregation flow
is swallowing later-page failures by breaking out of the loop, which leaves
`total_stars`, language weights, trophies, and `original_repos` looking complete
when they are only partial. Update the pagination logic in `profile.ts` to
propagate the failure from the repository fetch/GraphQL response instead of
continuing with collected data, and make the aggregation path that builds the
summary from the repository pages fail fast with a clear error. Use the existing
`pageRes` / `pageBody` handling and the repository aggregation function that
computes the profile summary as the places to surface the error.
---
Outside diff comments:
In `@src/lib/ai.ts`:
- Around line 181-203: Reset the per-attempt response state inside the retry
loop in ai.ts so a failed response from an earlier attempt cannot leak into
later attempts or the post-loop failure handling. Update the retry logic around
the callAIWithTimeout flow to avoid reusing a stale `response` value if a later
attempt throws before assignment, and keep the final error path in
`handleFailure` based only on the last attempt’s outcome. Use the `response`
variable and the retry loop in the AI analysis request path as the place to fix
this.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 54e288d0-c30f-4b72-958a-aa3e3a2b747f
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (10)
package.jsonsrc/app/api/analyze/__tests__/access-control.test.tssrc/app/api/analyze/route.tssrc/app/api/contributions/route.tssrc/lib/__tests__/ai.test.tssrc/lib/ai.tssrc/lib/auth.tssrc/lib/github/http.tssrc/lib/github/profile.tssrc/lib/github/star-gate.ts
🚧 Files skipped from review as they are similar to previous changes (7)
- package.json
- src/lib/tests/ai.test.ts
- src/lib/auth.ts
- src/app/api/analyze/tests/access-control.test.ts
- src/lib/github/star-gate.ts
- src/app/api/contributions/route.ts
- src/app/api/analyze/route.ts
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
1 participant
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.
This PR resolves all 7 findings from the architectural review:
Summary by CodeRabbit
New Features
Bug Fixes
Tests