perf(api): sum star totals via REST; shrink profile GraphQL documents - #317
Conversation
Peak hours run close to the GitHub hourly quota ceiling (#308 follow-up); halving steady-state refresh traffic is the cheapest lever. Behind the 48h CDN window, data refreshed daily is indistinguishable from 12h. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Star totals came from GraphQL repo pages (100 nodes per query, plus a
follow-up pagination document), spending constrained GraphQL points on
data REST hands out on the same repos listing the service already reads
elsewhere. Sum stargazer_count over REST repo pages instead — a separate,
otherwise-idle hourly pool — running concurrently with whichever GraphQL
path (combined or split) fetches the rest of the profile.
The combined UserDetails and split UserDetailsCore documents drop their
100-node repositories page down to repositories(first: 1) { totalCount }:
the exact repo count survives, and the lighter document scores lower with
GitHub's cost estimator, so heavy accounts should hit the resource-limit
split path less often. Fork filtering and public-only semantics match the
old isFork: false / privacy: PUBLIC exactly; the #164 every-page star
accuracy is preserved (REST pagination, same Vercel page/time budgets).
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughProfile star totals move from GraphQL repository pagination to concurrent REST repository pagination, excluding forks and using the existing fetch-budget window. GraphQL queries now request only repository counts. The default cache freshness window increases from 12 to 24 hours. ChangesProfile data and cache behavior
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant getProfileDetails
participant fetchTotalStars
participant restRequest
participant GitHubREST
getProfileDetails->>fetchTotalStars: start star aggregation
fetchTotalStars->>restRequest: request repository page
restRequest->>GitHubREST: GET /users/:login/repos
GitHubREST-->>restRequest: repository data
restRequest-->>fetchTotalStars: paginated repositories
fetchTotalStars-->>getProfileDetails: fork-excluded totalStars
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
src/github-api/profile-details.ts (3)
364-383: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valuePagination relies on
repos.length === 100instead of the actualLinkheader.Using a full page as the "has more" signal means that when a user's public/non-fork repo count is an exact multiple of 100, one extra REST call is made that always returns an empty page before pagination stops. It's harmless functionally (no double counting), just a minor extra round trip. Parsing the
Link: rel="next"header from therestRequestresponse (available via axiosres.headers.link) would make the stop condition exact rather than inferred.🤖 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/github-api/profile-details.ts` around lines 364 - 383, The fetchTotalStars pagination currently infers continuation from repos.length; update it to parse res.headers.link for a rel="next" URL and pass that presence to shouldFetchNextPage. Preserve the existing page, budget, and star-count logic while stopping immediately when the response has no next link.
240-266: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueStale comment: "three smaller queries" undercounts the actual fan-out.
fetchUserDetailsSplitissues four concurrent fetches (coreFetcher,fetchCalendarWeeks,contributionYearsFetcher,countsFetcher); the comment at Lines 424-425 still says "three smaller queries," likely left over from before stars were split out of this path. Worth a quick wording fix so the count stays accurate for future readers.Also applies to: 423-428
🤖 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/github-api/profile-details.ts` around lines 240 - 266, The comment describing fetchUserDetailsSplit’s concurrent fan-out is stale and understates the number of requests. Update the nearby comment to accurately describe all four fetches—coreFetcher, fetchCalendarWeeks, contributionYearsFetcher, and countsFetcher—without changing the implementation.
399-407: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winStar-fetch failures fail the entire cached fetch, not just the star total.
Since
starsPromiseisn't independently caught beforeawait starsPromiseat Line 428, any REST-side error (network blip, GitHub REST outage, 403/404) will reject the wholewithDataCachecallback even if the GraphQL profile fetch succeeded — the comment at Lines 399-403 confirms this is deliberate. It's cushioned by the stale-cache fallback indata-cache.ts, but a cold cache (new profile, first fetch) would surface a full error card purely due to a transient REST hiccup that has nothing to do with the GraphQL data that did succeed. Consider defaultingtotalStarsto a fallback (e.g.0or the previous cached value) on astarsPromiserejection instead of letting it fail the combined result, to keep GraphQL-only degradation graceful.Also applies to: 428-428
🤖 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/github-api/profile-details.ts` around lines 399 - 407, Update the starsPromise handling in the profile fetch flow so a fetchTotalStars rejection is converted to a safe totalStars fallback instead of rejecting the withDataCache callback. Preserve the successful GraphQL profile result and use the existing cached value when available, otherwise the established zero/default value, while keeping genuine GraphQL failures propagating normally.tests/github-api/profile-details.test.ts (1)
69-76: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winAdd coverage for a REST star-fetch failure.
Given
getProfileDetailsnow lets afetchTotalStarsrejection fail the whole cached fetch (seeprofile-details.tsLines 399-428), it'd be valuable to add a test that mocks a/reposfailure (e.g.mock.onGet(...).networkError()or a 500) alongside a successful GraphQL response, and assert the documented behavior (rejection propagates / stale-cache fallback engages). This locks in the current intentional design and would catch regressions if that coupling is changed later.🤖 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 `@tests/github-api/profile-details.test.ts` around lines 69 - 76, Add a test in the profile-details suite that provides a successful GraphQL response but makes the mocked /users/{username}/repos request fail, then assert the documented getProfileDetails outcome—propagated rejection or stale-cache fallback—matching the existing cache setup and assertions. Reuse mockRestStars or the surrounding request-mocking symbols and cover the fetchTotalStars failure 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.
Inline comments:
In `@src/github-api/profile-details.ts`:
- Around line 364-383: The fetchTotalStars logic reads the wrong GitHub
repository star field; update its comment and property access to use
stargazers_count. Also update the repository mock fixture in
tests/github-api/profile-details.test.ts (lines 69-76) to provide
stargazers_count, with no other test changes.
---
Nitpick comments:
In `@src/github-api/profile-details.ts`:
- Around line 364-383: The fetchTotalStars pagination currently infers
continuation from repos.length; update it to parse res.headers.link for a
rel="next" URL and pass that presence to shouldFetchNextPage. Preserve the
existing page, budget, and star-count logic while stopping immediately when the
response has no next link.
- Around line 240-266: The comment describing fetchUserDetailsSplit’s concurrent
fan-out is stale and understates the number of requests. Update the nearby
comment to accurately describe all four fetches—coreFetcher, fetchCalendarWeeks,
contributionYearsFetcher, and countsFetcher—without changing the implementation.
- Around line 399-407: Update the starsPromise handling in the profile fetch
flow so a fetchTotalStars rejection is converted to a safe totalStars fallback
instead of rejecting the withDataCache callback. Preserve the successful GraphQL
profile result and use the existing cached value when available, otherwise the
established zero/default value, while keeping genuine GraphQL failures
propagating normally.
In `@tests/github-api/profile-details.test.ts`:
- Around line 69-76: Add a test in the profile-details suite that provides a
successful GraphQL response but makes the mocked /users/{username}/repos request
fail, then assert the documented getProfileDetails outcome—propagated rejection
or stale-cache fallback—matching the existing cache setup and assertions. Reuse
mockRestStars or the surrounding request-mocking symbols and cover the
fetchTotalStars failure path.
🪄 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 Plus
Run ID: ee4a210e-5e4d-4b7c-886d-baff75732183
📒 Files selected for processing (4)
src/github-api/profile-details.tssrc/utils/data-cache.tstests/github-api/profile-details.test.tstests/utils/data-cache.test.ts
… behavior Review catch: GitHub REST returns stargazers_count (plural); the code and the test fixture both said stargazer_count, so the suite was green while production would have summed ?? 0 into a zero-star card. Also fixes the stale "three smaller queries" comment and adds a test pinning the deliberate choice that a REST star failure rejects the fetch (stale rescue owns the fallback) rather than caching a wrong zero for a day. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Fixed in d59fcec — and the Two nitpicks intentionally skipped:
|
1 participant
What
Star totals move off GraphQL onto the (otherwise idle) REST quota pool, and the profile GraphQL documents get lighter — the next burn-rate lever after #309/#310/#316.
fetchTotalStars()sumsstargazer_countoverGET /users/:login/repospages, running concurrently with whichever GraphQL path (combined or split) fetches the rest of the profile — separate quota pools, no contention. Fork filtering and public-only semantics match the oldisFork: false/privacy: PUBLICexactly; [BUG] Inaccurate star count #164's every-page accuracy and the Vercel page/time budgets are unchanged.UserDetailsand splitUserDetailsCoredocuments drop their 100-node repositories page torepositories(first: 1) { totalCount }— the exact repo count survives, and the lighter document scores lower with GitHub's cost estimator, so heavy accounts should hit the resource-limit split less often.UserStarspagination document is deleted; the split path no longer threads star state throughfetchUserDetailsSplit.v2:pdcompact payload) is unchanged — same fields, same numbers — so existing cache entries stay valid.Testing
26 suites / 223 tests green, typecheck + lint clean. Profile-details tests now mock both transports; the star pagination test covers a full REST page (100 repos) spilling to page 2 and excludes a fork's stars; split/gateway-timeout/half-window-calendar fallbacks re-verified against the slimmer documents.
🤖 Generated with Claude Code
Summary by CodeRabbit