🧹 [code health] Extract HTTP status magic numbers to constants in github.ts#342
🧹 [code health] Extract HTTP status magic numbers to constants in github.ts#342is0692vs wants to merge 4 commits 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? |
|
Wondering what really moved? Review this PR in Change Stack to inspect semantic changes, definitions, and references. 📝 WalkthroughSummary by CodeRabbitリリースノート
WalkthroughHTTP_STATUS定数（BAD_REQUEST/UNAUTHORIZED/FORBIDDEN/NOT_FOUND）を導入し、GitHub API エラーレスポンスの処理を統一化。 ChangesGitHub API エラーハンドリング改善
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 src/lib/github.ts by introducing an HTTP_STATUS constant object to replace hardcoded HTTP status codes (400, 401, 403, and 404) with descriptive constants. The reviewer suggests extending this refactoring to include other hardcoded status codes (422 and 500) present elsewhere in the file to ensure complete consistency.
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 HTTP_STATUS = { | ||
| BAD_REQUEST: 400, | ||
| UNAUTHORIZED: 401, | ||
| FORBIDDEN: 403, | ||
| NOT_FOUND: 404, | ||
| } as const; |
There was a problem hiding this comment.
To complete the extraction of HTTP status magic numbers in this file, consider also defining constants for 422 (Unprocessable Entity) and 500 (Internal Server Error) which are still hardcoded on lines 141 and 144 respectively. This ensures full consistency across github.ts.
| const HTTP_STATUS = { | |
| BAD_REQUEST: 400, | |
| UNAUTHORIZED: 401, | |
| FORBIDDEN: 403, | |
| NOT_FOUND: 404, | |
| } as const; | |
| const HTTP_STATUS = { | |
| BAD_REQUEST: 400, | |
| UNAUTHORIZED: 401, | |
| FORBIDDEN: 403, | |
| NOT_FOUND: 404, | |
| UNPROCESSABLE_ENTITY: 422, | |
| INTERNAL_SERVER_ERROR: 500, | |
| } as const; |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| NOT_FOUND: 404, | ||
| } as const; | ||
|
|
||
|
|
||
| export function headers(token?: string): HeadersInit { |
There was a problem hiding this comment.
HTTP_STATUS 定数宣言の直後に空行が2行連続しています。コードスタイルの統一のため1行にまとめることを推奨します。
| NOT_FOUND: 404, | |
| } as const; | |
| export function headers(token?: string): HeadersInit { | |
| NOT_FOUND: 404, | |
| } as const; | |
| export function headers(token?: string): HeadersInit { |
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/lib/github.ts
Line: 31-35
Comment:
`HTTP_STATUS` 定数宣言の直後に空行が2行連続しています。コードスタイルの統一のため1行にまとめることを推奨します。
```suggestion
NOT_FOUND: 404,
} as const;
export function headers(token?: string): HeadersInit {
```
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!
Co-authored-by: is0692vs <135803462+is0692vs@users.noreply.github.com>
…raints Co-authored-by: is0692vs <135803462+is0692vs@users.noreply.github.com>
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: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/lib/__tests__/github/fetchRepositories.test.ts`:
- Around line 22-28: Update the tests that currently only assert the thrown
error type to also assert the GitHubApiError.status values: call
headers("invalid token format!!") and catch the thrown GitHubApiError
(referencing GitHubApiError and headers) and assert error.status === 400
(BAD_REQUEST) for the invalid-token-format test; similarly update the test that
simulates a 401 response to assert error.status === 401. Make the assertions by
capturing the thrown error (try/catch or expect(...).toThrow and then inspect
the thrown value) so the status field is explicitly checked alongside the type.
In `@src/lib/__tests__/github/fetchUserProfile.test.ts`:
- Around line 16-20: The test for fetchContributions currently only asserts that
it throws GitHubApiError but doesn't check the HTTP status; update the test that
imports fetchContributions and GitHubApiError to capture the thrown error (via
await expect(...).rejects.toThrow or try/catch) and assert that error.status ===
401 for the GraphQL no-token case (and similarly update the other test around
fetchContributions to assert error.status === 400 for the bad-request case),
referencing the GitHubApiError instance to read its status field.
In `@src/lib/github.ts`:
- Line 34: src/lib/github.ts 内の定数宣言の直後にある不要な空行を削除してください。ファイル内のすべての const
宣言（ファイル先頭やモジュールスコープにある const ...; や const <識別子> =
...;）を探し、定数宣言の直後にある単一の空行を取り除いてコードの一貫性を戻してください。該当箇所は "const"
キーワードを含む宣言を目印に修正してください。
🪄 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: 8eb7d4c4-5fd0-4325-944d-88617ac1b91b
📒 Files selected for processing (3)
src/lib/__tests__/github/fetchRepositories.test.tssrc/lib/__tests__/github/fetchUserProfile.test.tssrc/lib/github.ts
| it("GraphQL query fails when forbidden due to bad token", async () => { | ||
| // If token is missing, fetchRepositories doesn't throw, it falls back to REST API. | ||
| // What about an invalid token format? | ||
| const { headers } = await import("../../github"); | ||
| const { GitHubApiError } = await import("../../types"); | ||
| expect(() => headers("invalid token format!!")).toThrow(GitHubApiError); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
GitHubApiErrorのステータスコード検証を追加してください
エラー型の検証は実装されていますが、GitHubApiErrorのstatusフィールドが期待値と一致することを確認するべきです。具体的には：
- Lines 22-28 / 37-43: 不正なトークン形式では
statusが400(BAD_REQUEST)であること - Lines 45-50: 401レスポンスでは
statusが401であること
コーディングガイドラインに従い、401、403などの具体的なステータスコードの検証が必要です。
♻️ 提案される修正例
it("GraphQL query fails when forbidden due to bad token", async () => {
const { headers } = await import("../../github");
const { GitHubApiError } = await import("../../types");
- expect(() => headers("invalid token format!!")).toThrow(GitHubApiError);
+ try {
+ headers("invalid token format!!");
+ expect.fail("Should have thrown");
+ } catch (e) {
+ expect(e).toBeInstanceOf(GitHubApiError);
+ expect((e as GitHubApiError).status).toBe(400);
+ }
}); it("GraphQL API throws GitHubApiError on 401 response", async () => {
mockFetch.mockResolvedValueOnce(jsonResponse({ message: "Bad credentials" }, 401));
const { fetchRepositories } = await import("../../github");
const { GitHubApiError } = await import("../../types");
- await expect(fetchRepositories("testuser", "fake-token")).rejects.toThrow(GitHubApiError);
+ try {
+ await fetchRepositories("testuser", "fake-token");
+ expect.fail("Should have thrown");
+ } catch (e) {
+ expect(e).toBeInstanceOf(GitHubApiError);
+ expect((e as GitHubApiError).status).toBe(401);
+ }
});Also applies to: 37-43, 45-50
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/lib/__tests__/github/fetchRepositories.test.ts` around lines 22 - 28,
Update the tests that currently only assert the thrown error type to also assert
the GitHubApiError.status values: call headers("invalid token format!!") and
catch the thrown GitHubApiError (referencing GitHubApiError and headers) and
assert error.status === 400 (BAD_REQUEST) for the invalid-token-format test;
similarly update the test that simulates a 401 response to assert error.status
=== 401. Make the assertions by capturing the thrown error (try/catch or
expect(...).toThrow and then inspect the thrown value) so the status field is
explicitly checked alongside the type.
Source: Coding guidelines
| it("GraphQL API without token throws UNAUTHORIZED correctly through API error", async () => { | ||
| const { fetchContributions } = await import("../../github"); | ||
| const { GitHubApiError } = await import("../../types"); | ||
| await expect(fetchContributions("testuser")).rejects.toThrow(GitHubApiError); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
エラーテストでHTTPステータスコードの検証を追加してください
エラー型の検証は実装されていますが、GitHubApiErrorのstatusフィールドの値が検証されていません。コーディングガイドラインに従い、401、403、404などの具体的なステータスコードを確認するべきです。
例：
- Line 16-20のテストでは
error.statusが401(UNAUTHORIZED)であることを確認 - Line 32-39のテストでは
error.statusが400(BAD_REQUEST)であることを確認
♻️ 提案される修正例
it("GraphQL API without token throws UNAUTHORIZED correctly through API error", async () => {
const { fetchContributions } = await import("../../github");
const { GitHubApiError } = await import("../../types");
- await expect(fetchContributions("testuser")).rejects.toThrow(GitHubApiError);
+ try {
+ await fetchContributions("testuser");
+ expect.fail("Should have thrown");
+ } catch (e) {
+ expect(e).toBeInstanceOf(GitHubApiError);
+ expect((e as GitHubApiError).status).toBe(401);
+ }
}); it("無効なトークンフォーマットの場合 GitHubApiError をスローする", async () => {
const { fetchUserProfile } = await import("../../github");
const { GitHubApiError } = await import("../../types");
- await expect(fetchUserProfile("testuser", "invalid token with spaces")).rejects.toThrow(
- GitHubApiError
- );
+ try {
+ await fetchUserProfile("testuser", "invalid token with spaces");
+ expect.fail("Should have thrown");
+ } catch (e) {
+ expect(e).toBeInstanceOf(GitHubApiError);
+ expect((e as GitHubApiError).status).toBe(400);
+ }
});Also applies to: 32-39
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/lib/__tests__/github/fetchUserProfile.test.ts` around lines 16 - 20, The
test for fetchContributions currently only asserts that it throws GitHubApiError
but doesn't check the HTTP status; update the test that imports
fetchContributions and GitHubApiError to capture the thrown error (via await
expect(...).rejects.toThrow or try/catch) and assert that error.status === 401
for the GraphQL no-token case (and similarly update the other test around
fetchContributions to assert error.status === 400 for the bad-request case),
referencing the GitHubApiError instance to read its status field.
Source: Coding guidelines
| NOT_FOUND: 404, | ||
| } as const; | ||
|
|
||
|
|
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | 💤 Low value
定数宣言直後の空行を削除することを検討してください
PRの目的にも記載されている通り、定数宣言の直後に不要な空行があります。コードの一貫性のため削除を推奨します。
♻️ 提案される修正
export const HTTP_STATUS = {
BAD_REQUEST: 400,
UNAUTHORIZED: 401,
FORBIDDEN: 403,
NOT_FOUND: 404,
} as const;
-📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export const HTTP_STATUS = { | |
| BAD_REQUEST: 400, | |
| UNAUTHORIZED: 401, | |
| FORBIDDEN: 403, | |
| NOT_FOUND: 404, | |
| } as const; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/lib/github.ts` at line 34, src/lib/github.ts
内の定数宣言の直後にある不要な空行を削除してください。ファイル内のすべての const 宣言（ファイル先頭やモジュールスコープにある const ...; や
const <識別子> = ...;）を探し、定数宣言の直後にある単一の空行を取り除いてコードの一貫性を戻してください。該当箇所は "const"
キーワードを含む宣言を目印に修正してください。
1 participant
🎯 What: Extracted hardcoded HTTP status codes to a constant
HTTP_STATUSobject insrc/lib/github.tsand replaced all magic numbers (400, 401, 403, 404).💡 Why: Magic numbers like HTTP status codes are error-prone and harder to interpret quickly. Extracting them to named constants improves code readability, ensures consistency, and makes future maintenance easier.
✅ Verification:
npm run lintsuccessfully.npm run testwhich passed successfully (551 tests across 71 files).✨ Result: Improved maintainability of
src/lib/github.tsby centralizing the definition of HTTP status codes used in error handling and API requests.PR created automatically by Jules for task 2109023747470424910 started by @is0692vs
Greptile Summary
このPRは
src/lib/github.ts内のHTTPステータスコードのマジックナンバー（400, 401, 403, 404）をHTTP_STATUS定数オブジェクトに集約し、ファイル内の全5箇所を置き換えたコードヘルス改善です。ロジックの変更はなく、リファクタリングのみです。HTTP_STATUS定数をas constで定義し、型安全性を確保しながら全使用箇所に適用したheaders(),handleResponse(),graphql(),fetchContributions()の各関数でマジックナンバーが削除されたConfidence Score: 4/5
純粋なリファクタリングであり、ロジックの変更はないためマージは安全です。
すべてのマジックナンバーが正しく定数に置き換えられており、
as constによる型安全性も確保されています。空行が二重になっている軽微なスタイル上の問題が1点あるのみで、機能への影響はありません。特に注意が必要なファイルはありません。
src/lib/github.tsの変更は意図通りに機能しています。Important Files Changed
HTTP_STATUS定数オブジェクトに抽出し、ファイル内の全5箇所のマジックナンバーを置き換えた。ロジックの変更は一切なく、スタイル上の空行二重のみが軽微な指摘点。Flowchart
%%{init: {'theme': 'neutral'}}%% flowchart TD A[API レスポンス受信] --> B{ステータスコード確認} B -->|HTTP_STATUS.NOT_FOUND 404| C[UserNotFoundError をスロー] B -->|HTTP_STATUS.FORBIDDEN 403| D[handleRateLimit → RateLimitError] B -->|res.ok = false| E[GitHubApiError をスロー] B -->|res.ok = true| F[JSON パース & 返却] G[headers 関数] -->|不正なトークン形式| H[GitHubApiError HTTP_STATUS.BAD_REQUEST 400] I[graphql 関数] -->|トークンなし| J[GitHubApiError HTTP_STATUS.UNAUTHORIZED 401] I -->|HTTP_STATUS.FORBIDDEN 403| D K[fetchContributions] -->|トークンなし| L[GitHubApiError HTTP_STATUS.UNAUTHORIZED 401]Prompt To Fix All With AI
Reviews (1): Last reviewed commit: "🧹 Extract hardcoded HTTP status codes t..." | Re-trigger Greptile