Skip to content

🧹 Refactor fetchActivity to use native Promise.allSettled #374

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
+8 −6
Open

🧹 Refactor fetchActivity to use native Promise.allSettled #374

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
14 changes: 8 additions & 6 deletions src/lib/github.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -682,21 +682,23 @@ 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)));
// Use allSettled to natively handle all promises and avoid unhandled rejections
const results = await Promise.allSettled(promises);

for (const p of promises) {
try {
const events = await p;
for (const result of results) {
if (result.status === "fulfilled") {
const events = result.value;
allEvents.push(...events);
if (events.length < 100) break;
} catch (error) {
} else {
const error = result.reason;
if (
error instanceof UserNotFoundError ||
error instanceof RateLimitError
) {
throw error;
}
logger.error("Event fetch promise rejected:", error);
break;
}
Comment on lines +686 to 703

@greptile-apps greptile-apps Bot Jun 12, 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 早期終了時のレイテンシ増加

変更前は for...of ループで各 Promise を逐次 await していたため、ページ 1 のレスポンスが < 100 件だった時点で即座に break・返却できていました（ページ 2・3 はリクエスト自体は飛んでいたが、関数の返却は t_page1 だった）。

変更後は Promise.allSettled(promises) が全 3 つの Promise が settle するまで待機するため、ページ 1 が < 100 件または UserNotFoundError で失敗した場合でも max(t_page1, t_page2, t_page3) 待ってから初めてループ処理に入ります。イベント数が少ないユーザー（最も一般的なケース）で毎回余分な待ち時間が生じます。PR 説明の「same functional behavior including early exits」という主張とも整合しません。

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

Comment:
**早期終了時のレイテンシ増加**

変更前は `for...of` ループで各 Promise を逐次 `await` していたため、ページ 1 のレスポンスが < 100 件だった時点で即座に `break`・返却できていました（ページ 2・3 はリクエスト自体は飛んでいたが、関数の返却は `t_page1` だった）。

変更後は `Promise.allSettled(promises)` が全 3 つの Promise が settle するまで待機するため、ページ 1 が < 100 件または `UserNotFoundError` で失敗した場合でも `max(t_page1, t_page2, t_page3)` 待ってから初めてループ処理に入ります。イベント数が少ないユーザー（最も一般的なケース）で毎回余分な待ち時間が生じます。PR 説明の「same functional behavior including early exits」という主張とも整合しません。

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 703

@greptile-apps greptile-apps Bot Jun 12, 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 ログ出力の動作変更

変更前は .forEach(p => p.catch((e) => logger.error(...))) が全 3 Promise すべてにキャッチハンドラを登録していたため、どの Promise が reject しても必ずログに記録されていました。

変更後は for...of ループ内で最初の rejected 結果を logger.error してから break するため、2 番目・3 番目の reject はサイレントに吸収されます。たとえばページ 1 が non-critical error で break した後、ページ 2・3 も別の理由で失敗していた場合にそれらがログに残りません。

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

Comment:
**ログ出力の動作変更**

変更前は `.forEach(p => p.catch((e) => logger.error(...)))` が全 3 Promise すべてにキャッチハンドラを登録していたため、どの Promise が reject しても必ずログに記録されていました。

変更後は `for...of` ループ内で最初の rejected 結果を `logger.error` してから `break` するため、2 番目・3 番目の reject はサイレントに吸収されます。たとえばページ 1 が non-critical error で `break` した後、ページ 2・3 も別の理由で失敗していた場合にそれらがログに残りません。

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

}
Comment on lines +685 to 704

@gemini-code-assist gemini-code-assist Bot Jun 12, 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.

medium

Using Promise.allSettled introduces a performance and latency regression. Promise.allSettled blocks and waits for all promises in the array to settle (either fulfill or reject) before proceeding.

In the original implementation, if the first page returned fewer than 100 events, the loop would break and the function would return immediately without waiting for the subsequent pages to finish fetching. With Promise.allSettled, the function will always wait for all pages (up to 3 pages) to complete, even if the first page is empty or has very few events, thereby increasing the latency of the API call.

To preserve the early-exit latency optimization while safely handling unhandled promise rejections, it is better to stick to the original pattern of attaching a .catch handler to each promise and awaiting them sequentially in the loop.

  // 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) {
      if (
        error instanceof UserNotFoundError ||
        error instanceof RateLimitError
      ) {
        throw error;
      }
      break;
    }
  }
References
  1. When suppressing unhandled promise rejections, log the error instead of silently ignoring it to facilitate debugging.

Expand Down
Loading