⚡ ［Performance Optimization］ Parallelize Year in Review GitHub fetching#114
⚡ ［Performance Optimization］ Parallelize Year in Review GitHub fetching#114
Conversation
Co-authored-by: is0692vs <135803462+is0692vs@users.noreply.github.com>
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 27 minutes and 42 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthrough単一のGraphQLクエリを2つに分割し、統計用の Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 推奨ラベル
兎からのポエム
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the GitHub data fetching logic by splitting the single GraphQL query into two separate queries: one for general statistics and one for repository-specific contributions. This allows for better management of the data fetching process. The reviewer suggested using Promise.all to await the concurrent operations more idiomatically, which improves code clarity and robustness.
| const statsResponse = await statsPromise; | ||
| if (!statsResponse.user) { | ||
| throw new UserNotFoundError(username); | ||
| } | ||
|
|
||
| const collection = { | ||
| ...statsResponse.user.contributionsCollection, | ||
| ...reposCollection, | ||
| } as NonNullable<YearInReviewResponse["user"]>["contributionsCollection"]; | ||
|
|
||
| const commitDates = await commitDatesPromise; |
There was a problem hiding this comment.
While the current implementation correctly initiates promises to run in parallel, awaiting them sequentially can be misleading. Using Promise.all to await statsPromise and commitDatesPromise would more clearly express the intent to wait for both concurrent operations to finish. This also makes the code more robust to future changes and is generally considered a best practice for handling concurrent promises.
| const statsResponse = await statsPromise; | |
| if (!statsResponse.user) { | |
| throw new UserNotFoundError(username); | |
| } | |
| const collection = { | |
| ...statsResponse.user.contributionsCollection, | |
| ...reposCollection, | |
| } as NonNullable<YearInReviewResponse["user"]>["contributionsCollection"]; | |
| const commitDates = await commitDatesPromise; | |
| const [statsResponse, commitDates] = await Promise.all([statsPromise, commitDatesPromise]); | |
| if (!statsResponse.user) { | |
| throw new UserNotFoundError(username); | |
| } | |
| const collection = { | |
| ...statsResponse.user.contributionsCollection, | |
| ...reposCollection, | |
| } as NonNullable<YearInReviewResponse["user"]>["contributionsCollection"]; |
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Addressed the review by switching sequential awaits to for concurrently started operations in (commit ). Added ❤️ reaction and resolved the addressed thread. |
Acknowledged. |
Co-authored-by: is0692vs <135803462+is0692vs@users.noreply.github.com>
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/lib/githubYearInReview.ts`:
- Around line 245-272: Attach a rejection handler to the immediately-created
statsPromise to avoid an unhandled rejection before the later await; change the
statsPromise assignment (the call to graphql with YEAR_IN_REVIEW_STATS_QUERY) to
include .catch(err => { throw err; }) or otherwise capture/log the error (e.g.,
.catch(err => Promise.reject(err))) so the rejection is handled synchronously
while preserving semantics for the later await/Promise.all that combines
statsPromise and commitDatesPromise.
🪄 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
Run ID: 624d067c-4e69-41d3-804c-5c35549a88d4
📒 Files selected for processing (1)
src/lib/githubYearInReview.ts
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Addressed the new CodeRabbit finding by attaching an early rejection drain to and awaiting with once both operations are ready (commit ). This prevents potential unhandled rejection timing issues while preserving error propagation. Added ❤️ reactions and resolved the thread. |
Acknowledged. |
Co-authored-by: is0692vs <135803462+is0692vs@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/lib/githubYearInReview.ts`:
- Around line 260-277: The current code throws UserNotFoundError based solely on
reposResponse before waiting for statsPromise, which can hide real errors from
statsPromise (e.g., RateLimitError/GitHubApiError); instead, await both
statsPromise and commitDatesPromise (the Promise.all already used) and only
after awaiting check both statsResponse.user and reposResponse.user and throw
UserNotFoundError(username) if either user is null; adjust the checks around
reposResponse, statsResponse, statsPromise, and commitDatesPromise so
user-not-found validation happens after the Promise.all to preserve proper error
propagation for rate limits and API errors.
🪄 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
Run ID: 996aef31-25bd-4e61-a2d8-bec72b356a18
📒 Files selected for processing (1)
src/lib/githubYearInReview.ts
Attach an early rejection handler to statsPromise and move user-not-found validation after awaiting both concurrent tasks, so API errors are not masked and promise handling stays safe. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Deployment failed with the following error: Learn More: https://vercel.com/hirokis-projects-afd618c7?upgradeToPro=build-rate-limit |
|
Addressed the review feedback and pushed commit
Could you please re-review this PR? |
Acknowledged. |
Co-authored-by: is0692vs <135803462+is0692vs@users.noreply.github.com>
1 participant
💡 What:
Split the large
YEAR_IN_REVIEW_QUERYinto two smaller, focused GraphQL queries (YEAR_IN_REVIEW_STATS_QUERYandYEAR_IN_REVIEW_REPOS_QUERY). We can now trigger both GraphQL queries in parallel. Crucially, the repository fetching GraphQL query and the subsequent REST API calls to fetch commit dates per repository can run concurrently with the more computationally expensive statistics/calendar GraphQL query.🎯 Why:
Previously, the process sequentially waited for the large combined GraphQL query to complete before extracting the top repositories and dispatching concurrent REST API calls for commit dates. This approach was creating a performance bottleneck by stacking the latencies. By executing them in parallel, we reduce the total execution time, improving the efficiency of the Year in Review data fetching path.
📊 Measured Improvement:
Before the optimization, when fetching a mock dataset with simulated delays (600ms for full GraphQL query and 400ms for REST fetch), the data took ~1004ms to load. After splitting the queries,
statsPromisetakes ~400ms and runs concurrently withreposResponse(~200ms) +commitDatesPromise(~400ms), resulting in a loading time of ~600ms, delivering an approximate ~40% performance improvement for the mocked workload.PR created automatically by Jules for task 6122975342386324370 started by @is0692vs