Skip to content

⚡ Optimize GitHub event fetching with Promise.all #450

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
is0692vs wants to merge 1 commit into
base: main
Choose a base branch
from
Open

⚡ Optimize GitHub event fetching with Promise.all #450

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 11 additions & 9 deletions src/lib/github.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -685,20 +685,22 @@ export const fetchActivity = cache(async function fetchActivity(
// 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)));

for (const p of promises) {
try {
const events = await p;
allEvents.push(...events);
if (events.length < 100) break;
} catch (error) {
const results = await Promise.all(promises.map(p => p.catch(e => e)));
Comment on lines 685 to +688

@greptile-apps greptile-apps Bot Jul 10, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 冗長な .catch() ハンドラとミスリーディングなログ

PR の説明では「unhandled rejection の手動抑制を削除した」と述べていますが、promises.forEach((p) => p.catch(...)) の行は削除されていません。Promise.all(promises.map(p => p.catch(e => e))) が全プロミスの rejection を処理するようになったため、この forEach はすでに冗長です。

また、同一の p に両方の .catch() が登録されているため、UserNotFoundError や RateLimitError のような意図的に再スローされるエラーも "Event fetch promise rejected:" としてログに記録されます。正常系のエラーハンドリングが誤ってエラーログとして出力される副作用があります。forEach 行とそのコメントは不要になっているため、削除を検討してください。

Prompt To Fix With AI 
This is a comment left during a code review.
Path: src/lib/github.ts
Line: 685-688

Comment:
**冗長な `.catch()` ハンドラとミスリーディングなログ**

PR の説明では「unhandled rejection の手動抑制を削除した」と述べていますが、`promises.forEach((p) => p.catch(...))` の行は削除されていません。`Promise.all(promises.map(p => p.catch(e => e)))` が全プロミスの rejection を処理するようになったため、この `forEach` はすでに冗長です。

また、同一の `p` に両方の `.catch()` が登録されているため、`UserNotFoundError` や `RateLimitError` のような意図的に再スローされるエラーも `"Event fetch promise rejected:"` としてログに記録されます。正常系のエラーハンドリングが誤ってエラーログとして出力される副作用があります。`forEach` 行とそのコメントは不要になっているため、削除を検討してください。

How can I resolve this? If you propose a fix, please make it concise.


for (const result of results) {
if (result instanceof Error) {
if (
error instanceof UserNotFoundError ||
error instanceof RateLimitError
result instanceof UserNotFoundError ||
result instanceof RateLimitError
) {
throw error;
throw result;
}
break;
}

const events = result as GitHubEvent[];
allEvents.push(...events);
if (events.length < 100) break;
Comment on lines +688 to +703

@greptile-apps greptile-apps Bot Jul 10, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Promise.all によるアーリーエグジット最適化の喪失

Promise.all はすべてのプロミスが解決するまで待機するため、ページ1のイベント数が 100 未満（ほとんどのユーザーが該当）の場合、ページ2・3のレスポンスを待ってからでしかループが開始できません。元のコードでは for...await + break により、ページ1のレスポンスを受け取った直後に関数が返れたため、ページ2・3の完了待ちは発生しませんでした。

具体例として、ページ1が 200ms、ページ2・3が 800ms でそれぞれ 404 を返すユーザーの場合、旧コードは約 200ms で返るのに対し、新コードは約 800ms 待ってから break します。イベント数が少ない一般ユーザーではレイテンシが大幅に悪化します。

Prompt To Fix With AI 
This is a comment left during a code review.
Path: src/lib/github.ts
Line: 688-703

Comment:
**Promise.all によるアーリーエグジット最適化の喪失**

`Promise.all` はすべてのプロミスが解決するまで待機するため、ページ1のイベント数が 100 未満（ほとんどのユーザーが該当）の場合、ページ2・3のレスポンスを待ってからでしかループが開始できません。元のコードでは `for...await` + `break` により、ページ1のレスポンスを受け取った直後に関数が返れたため、ページ2・3の完了待ちは発生しませんでした。

具体例として、ページ1が 200ms、ページ2・3が 800ms でそれぞれ 404 を返すユーザーの場合、旧コードは約 200ms で返るのに対し、新コードは約 800ms 待ってから break します。イベント数が少ない一般ユーザーではレイテンシが大幅に悪化します。

How can I resolve this? If you propose a fix, please make it concise.

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

}
Comment on lines +688 to 704

@gemini-code-assist gemini-code-assist Bot Jul 10, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

Using Promise.all to await all page promises concurrently introduces a performance regression.

Why this is an issue

In the previous implementation, the requests were initiated concurrently, but awaited sequentially in a loop:
for (const p of promises) {
const events = await p;
if (events.length < 100) break;
}
If page 1 returned fewer than 100 events, the loop would break immediately, and the function would return without waiting for pages 2 and 3 to resolve.

With Promise.all, the function is forced to block until all page requests have resolved or rejected. For users with few public events (where page 1 has < 100 events), this unnecessarily increases latency to the slowest of the three network requests.

Recommendation

Revert to the sequential for...of loop over the concurrently initiated promises to preserve the early-exit optimization.

  for (const p of promises) {
    try {
      const events = await p;
      allEvents.push(...events);
      if (events.length < 100) break;
    } catch (error) {
      if (
        error instanceof UserNotFoundError ||
        error instanceof RateLimitError
      ) {
        throw error;
      }
      break;
    }
  }


// 曜日×時間帯ヒートマップ (7×24)
Expand Down
Loading