-
Notifications
You must be signed in to change notification settings - Fork 0
⚡ Optimize GitHub API fetching by adding Next.js revalidate cache #362
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.
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -45,6 +45,7 @@ | |
| "jsdom": "^28.1.0", | ||
| "server-only": "^0.0.1", | ||
| "tailwindcss": "^4", | ||
| "ts-node": "^10.9.2", | ||
| "typescript": "^5", | ||
| "vitest": "^4.1.5" | ||
| } | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Prompt To Fix With AIThis is a comment left during a code review.
Path: package.json
Line: 51
Comment:
**不要な依存関係の追加**
`ts-node` の追加がこの PR の目的(キャッシュ設定の変更)と無関係に見えます。PR の説明には `ts-node` を追加する理由が一切記載されておらず、変更されたソースファイル(`cardDataFetcher.ts`)でも使用されていません。Jules が自動生成したものである可能性が高く、意図しない追加の可能性があります。この依存関係が本当に必要かどうか確認をお願いします。
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! |
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -84,7 +84,7 @@ async function getJson<T>(url: string): Promise<{ status: number; data: T | null | |
| try { | ||
| response = await fetch(url, { | ||
| headers: getHeaders(), | ||
| cache: "no-store", | ||
| next: { revalidate: 3600 }, | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Since the API route calling this function ( To resolve this and enable the Next.js Data Cache, you should change the API route's runtime to Node.js by removing
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Prompt To Fix With AIThis is a comment left during a code review.
Path: src/lib/cardDataFetcher.ts
Line: 87
Comment:
**ヒートマップデータの陳腐化**
`revalidate: 3600` により GitHub リポジトリのデータ(`pushed_at` を含む)が最大 1 時間キャッシュされます。一方、`buildHeatmapFromRepoPushes` 内の `new Date()` は呼び出しごとに「今日」の日付を計算します。その結果、最大 1 時間前の `pushed_at` データを元にヒートマップが描画されるため、直近 1 時間以内のプッシュが表示に反映されない可能性があります。パフォーマンス向上とのトレードオフとして許容範囲内ですが、ユーザーへの影響を考慮した上で `revalidate` の値が適切かどうか検討してください。
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! |
||
| signal: controller.signal, | ||
| }); | ||
|
Comment on lines
85
to
89
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Prompt To Fix With AIThis is a comment left during a code review.
Path: src/lib/cardDataFetcher.ts
Line: 85-89
Comment:
**`signal` オプションとキャッシュの相互作用**
`signal`(`AbortController`)と `next: { revalidate }` を同時に指定した場合、タイムアウトによってリクエストがアボートされると、その応答はキャッシュに保存されません。通常の成功レスポンスはキャッシュされるため、大きな問題ではありませんが、高負荷時に連続してタイムアウトが発生すると、1 時間のキャッシュ効果が得られず、常に GitHub API へのリクエストが発生し続ける可能性があります。現在の動作で問題ない場合は許容範囲ですが、意図した動作であることを確認してください。
How can I resolve this? If you propose a fix, please make it concise. |
||
| } catch (error) { | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The
ts-nodedependency appears to be unused in this project. Next.js and Vitest both natively support TypeScript compilation and execution without requiringts-node. Removing this dependency will keep thepackage.jsonclean and reduce the installation footprint.