⚡ Optimize GitHub Activity Fetching with Fail-Fast Concurrency#364
⚡ Optimize GitHub Activity Fetching with Fail-Fast Concurrency#364is0692vs wants to merge 1 commit into
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. 1 Skipped Deployment
|
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
|
Warning Review limit reached
More reviews will be available in 53 minutes and 16 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more credits in the billing tab to continue. ⌛ How to resolve this issue?After more reviews become available, 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 include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
✨ 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 |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
![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 introduces a criticalErrorPromise to fast-fail the event fetching loop when a critical error (such as UserNotFoundError or RateLimitError) occurs, racing it against individual page fetch promises. The reviewer pointed out that if criticalErrorPromise rejects after the loop has already broken early, it could cause unhandled promise rejection warnings. They suggested attaching a .catch() handler to log the error and prevent this issue.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| const criticalErrorPromise = new Promise<never>((_, reject) => { | ||
| promises.forEach((p) => p.catch((e) => { | ||
| if (e instanceof UserNotFoundError || e instanceof RateLimitError) { | ||
| reject(e); | ||
| } | ||
| })); | ||
| }); |
There was a problem hiding this comment.
If criticalErrorPromise rejects after the loop has already broken early (e.g., because page 1 had fewer than 100 events), the rejection of criticalErrorPromise will occur in the background. To prevent potential unhandled promise rejection warnings in environments with strict rejection tracking, attach a .catch() handler directly to criticalErrorPromise immediately after its creation. Ensure you log the error instead of silently ignoring it to facilitate debugging.
| const criticalErrorPromise = new Promise<never>((_, reject) => { | |
| promises.forEach((p) => p.catch((e) => { | |
| if (e instanceof UserNotFoundError || e instanceof RateLimitError) { | |
| reject(e); | |
| } | |
| })); | |
| }); | |
| const criticalErrorPromise = new Promise<never>((_, reject) => { | |
| promises.forEach((p) => p.catch((e) => { | |
| if (e instanceof UserNotFoundError || e instanceof RateLimitError) { | |
| reject(e); | |
| } | |
| })); | |
| }); | |
| criticalErrorPromise.catch((err) => console.error(err)); |
References
- When suppressing unhandled promise rejections, log the error instead of silently ignoring it to facilitate debugging.
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| // Suppress unhandled promise rejections for subsequent pages if we break early or throw | ||
| promises.forEach((p) => p.catch((e) => logger.error("Event fetch promise rejected:", e))); |
There was a problem hiding this comment.
UserNotFoundError / RateLimitError が不要にエラーログへ出力される問題です。criticalErrorPromise 内の .catch と、後続の logging forEach の .catch は同じ Promise に対して独立して両方とも発火します。そのため、UserNotFoundError や RateLimitError が発生すると、caller へ正しく再スローされながらも logger.error("Event fetch promise rejected:", ...) にもエラーとして記録され、「想定外のエラー」のように見える誤解を招くノイズになります。ロギング側でクリティカルエラーを除外するか、コメントの意図通り「後続ページの未処理リジェクション抑制」に絞った条件付きログにすることが望ましいです。
| // Suppress unhandled promise rejections for subsequent pages if we break early or throw | |
| promises.forEach((p) => p.catch((e) => logger.error("Event fetch promise rejected:", e))); | |
| // Suppress unhandled promise rejections for subsequent pages if we break early or throw | |
| // クリティカルエラーは呼び出し元へ再スローされるため、ログ出力から除外する | |
| promises.forEach((p) => | |
| p.catch((e) => { | |
| if (!(e instanceof UserNotFoundError || e instanceof RateLimitError)) { | |
| logger.error("Event fetch promise rejected:", e); | |
| } | |
| }) | |
| ); |
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/lib/github.ts
Line: 693-694
Comment:
`UserNotFoundError` / `RateLimitError` が不要にエラーログへ出力される問題です。`criticalErrorPromise` 内の `.catch` と、後続の logging `forEach` の `.catch` は**同じ Promise に対して独立して両方とも発火**します。そのため、`UserNotFoundError` や `RateLimitError` が発生すると、caller へ正しく再スローされながらも `logger.error("Event fetch promise rejected:", ...)` にもエラーとして記録され、「想定外のエラー」のように見える誤解を招くノイズになります。ロギング側でクリティカルエラーを除外するか、コメントの意図通り「後続ページの未処理リジェクション抑制」に絞った条件付きログにすることが望ましいです。
```suggestion
// Suppress unhandled promise rejections for subsequent pages if we break early or throw
// クリティカルエラーは呼び出し元へ再スローされるため、ログ出力から除外する
promises.forEach((p) =>
p.catch((e) => {
if (!(e instanceof UserNotFoundError || e instanceof RateLimitError)) {
logger.error("Event fetch promise rejected:", e);
}
})
);
```
How can I resolve this? If you propose a fix, please make it concise.1 participant
💡 What: Introduced a
criticalErrorPromiseinfetchActivityto abort immediately upon critical errors (likeUserNotFoundErrororRateLimitError) usingPromise.race.🎯 Why: Previously, the code fetched multiple activity pages concurrently but awaited them sequentially in a loop. This meant that if page 2 threw a critical error early, the loop would still block on awaiting page 1, unnecessarily delaying the error response.
📊 Measured Improvement: In synthetic benchmarks, simulating a 100ms request for page 1 and a 50ms critical failure for page 2:
This effectively cuts response times for rate limit or missing user errors by skipping unnecessary awaits.
PR created automatically by Jules for task 2330449860277899984 started by @is0692vs
Greptile Summary
fetchActivity内でcriticalErrorPromiseを導入し、UserNotFoundErrorやRateLimitErrorが発生した場合にPromise.raceで早期中断できるよう変更しています。これにより、他のページリクエストが完了するまで待たずにクリティカルエラーを即座に検出できるようになりました。criticalErrorPromiseが全ページ Promise の.catchを監視し、クリティカルエラー発生時はPromise.race経由でループを即座に脱出するforEachと二重に.catchが発火するため、UserNotFoundError/RateLimitErrorがエラーログに不要に記録される副作用があるConfidence Score: 4/5
フェイルファスト最適化の実装自体は正しく、既存動作を破壊するリスクは低い。
非同期フロー制御の変更は意図通りに機能しており、Promise.race による早期中断ロジックに論理的な誤りはない。ただし UserNotFoundError / RateLimitError が正しく再スローされるにもかかわらずエラーログにも記録されるという副作用があり、本番環境でのアラート誤検知につながる可能性がある。
src/lib/github.ts — ロギング forEach とクリティカルエラーの扱い
Important Files Changed
Sequence Diagram
sequenceDiagram participant FA as fetchActivity participant P1 as page1 participant P2 as page2 participant CEP as criticalErrorPromise Note over P1,P2: 全ページを同時起動 P2-->>CEP: reject RateLimitError CEP-->>FA: Promise.race が即座に reject FA-->>FA: catch then re-throw RateLimitError Note over P1: p1 はまだ実行中 Note over FA: 改善前は p1 完了後に p2 の失敗を検出していたPrompt To Fix All With AI
Reviews (1): Last reviewed commit: "feat: Optimize fetchActivity to fail-fas..." | Re-trigger Greptile