Conversation
- Reorganize aizu_online_judge and atcoder_problems into subdirectories - Split AOJ client into clients.ts (API), types.ts (types), utils.ts (helpers) - Move test data to unified fixtures/ directory - Extract HTTP client helpers to fixtures/helpers.ts - Update imports across codebase to reflect new structure - Add comprehensive plan for clients restructuring (Phase 1 complete) Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com>
- Remove unused imports from test_helpers files - Update AOJ utils test cleanup - Update plan with progress notes Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com>
- Update assertion patterns in AOJ clients and utils tests - Improve fixtures helpers test clarity - Align test style with project conventions Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com>
Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com>
Contributor
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, 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 have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAOJ（Aizu Online Judge）クライアントを型定義、ユーティリティ、実装に分割してディレクトリ再編。古い ChangesAPI クライアント再編と AOJ 実装追加
Sequence Diagram(s)sequenceDiagram
participant Client as Consumer
participant AojApi as AojApiClient
participant Sub as SubClients (Courses/Challenges)
participant Http as HttpRequestClient
participant Cache as ContestTaskCache
participant API as AOJ API
Client->>AojApi: getContests() / getTasks()
AojApi->>Sub: call getContests/getTasks (parallel)
Sub->>Cache: try read cached data
alt cache miss
Sub->>Http: fetchApiWithConfig(endpoint)
Http->>API: HTTP request
API-->>Http: JSON response
Http-->>Sub: parsed response
Sub->>Cache: store normalized results
end
Sub-->>AojApi: results (some may be rejected)
AojApi->>AojApi: log per-source failures
AojApi-->>Client: merged ContestsForImport / TasksForImport
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes 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 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/lib/clients/fixtures/helpers.ts (1)
38-49: 🧹 Nitpick | 🔵 Trivial | 💤 Low valuetry/finally 内の重複クリーンアップ
Lines 42-48 の
try/finallyブロック内でinvalidJson.jsonを削除していますが、Line 16-26 のafterAllでも同じファイルをクリーンアップしています。afterAllが実行されるため、このfinallyブロックは冗長です。♻️ 簡略化案
test('expects to throw an error if the file content is not valid JSON', () => { const invalidJsonFilePath = path.resolve(__dirname, 'invalidJson.json'); fs.writeFileSync(invalidJsonFilePath, 'invalid json'); - - try { - expect(() => loadMockData<typeof mockData>(invalidJsonFilePath)).toThrow(); - } finally { - if (fs.existsSync(invalidJsonFilePath)) { - fs.unlinkSync(invalidJsonFilePath); - } - } + expect(() => loadMockData<typeof mockData>(invalidJsonFilePath)).toThrow(); });🤖 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/clients/fixtures/helpers.ts` around lines 38 - 49, try/finally ブロック内で "invalidJson.json" を削除している処理は、既に同ファイルで定義されている afterAll のクリーンアップと重複しているため冗長です；該当する try/finally の finally セクションから "invalidJson.json" を削除するコードを取り除き、afterAll による一元的なクリーンアップに任せてください（対象を見つけるために "invalidJson.json" と該当の try/finally ブロック、そして既存の afterAll を参照してください）。src/lib/clients/atcoder/atcoder_problems.test.ts (1)
36-43: 🛠️ Refactor suggestion | 🟠 Major | 🏗️ Heavy liftテストで Nock を使用して HTTP モッキングを行うべき
コーディングガイドラインでは「Use Nock for HTTP mocking in unit tests」と明記されています。現在のテストはクライアントメソッドを直接モック（例:
client.getContests = async () => contestsMock）していますが、Nock を使用して HTTP リクエストをインターセプトする方式に変更してください。これにより、実際の HTTP クライアント層の動作を含めたより堅牢なテストが可能になり、プロジェクト全体のテスト標準との一貫性が保たれます。
As per coding guidelines: "Use Nock for HTTP mocking in unit tests."
Also applies to: 54-58, 70-77, 88-92
🤖 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/clients/atcoder/atcoder_problems.test.ts` around lines 36 - 43, The tests currently overwrite client.getContests (and similar client methods) directly; update these unit tests to use Nock to intercept the HTTP requests made by the AtCoder problems client instead: remove assignments like client.getContests = async () => contestsMock and instead create a Nock scope that matches the external API base URL and path used inside getContests, reply with contestsMock JSON, then call client.getContests() and assert the response; apply the same replacement pattern for the other tests that mock client methods (the tests referencing client.getContests, client.getProblems, etc.) so the HTTP layer is exercised via Nock rather than method stubbing.
🤖 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/clients/aizu_online_judge/clients.ts`:
- Line 249: The hardcoded magic number assigned to the variable size (10 ** 4)
lacks context; replace it with a named constant (e.g., AOJ_MAX_FETCH or
DEFAULT_FETCH_SIZE) declared near the top of the module or where constants live,
add a comment explaining the rationale (AOJ API max page size or estimated total
problems) and make it configurable (env/parameter) if the limit may change;
update references to use the constant instead of size so future maintainers
understand and can adjust the value safely.
In `@src/lib/clients/aizu_online_judge/utils.ts`:
- Around line 61-69: mapToTask currently assigns the same value to problem_index
and task_id which can cause ambiguity downstream; change mapToTask so
problem_index stays as problem.id but task_id is generated to be unique (e.g.,
combine contestId and problem.id like `${contestId}:${problem.id}`) and add a
brief inline comment mentioning this is a synthetic unique id because AOJ lacks
a separate task identifier; reference AOJTaskAPI, mapToTask, problem_index and
task_id when making the change.
- Around line 18-24: The regex in validateSegment rejects single-character
segments; update the pattern used in validateSegment to allow a single leading
letter by making the final `[a-zA-Z0-9]` optional (or otherwise changing the
grouping so the whole expression matches 1–MAX length) while preserving the
max-length constraint and the existing rules for internal characters and no
consecutive dots; then add/adjust tests in utils.test.ts to include
single-character cases (e.g., 'a', 'v1') to verify the fix.
In `@src/lib/clients/fixtures/helpers.test.ts`:
- Around line 8-26: Tests write and remove fixed filenames under __dirname which
causes flakiness when run in parallel; change the setup/teardown to create a
unique temp directory using fs.mkdtempSync(path.join(os.tmpdir(), '...'))
(replace testDir = __dirname with a per-run tempDir), build mockFilePath and
other file paths (invalidJson.json) inside that tempDir, write files in
beforeAll using mockFilePath, and in afterAll remove the files and rmdir the
temp directory; update references to testDir/mockFilePath in the file (including
the other block that creates invalidJson.json) so each test run uses its own
isolated directory.
In `@src/lib/clients/fixtures/helpers.ts`:
- Line 12: The code uses path.resolve(filePath) to form testDataPath which
resolves relative paths against process.cwd(), causing environment-dependent
behavior; update the helper to resolve relative paths robustly by treating
filePath as module-relative when needed—either require callers to pass an
absolute path (use path.join(__dirname, ...) in clients.test.ts) or change the
helper to resolve via the current module location (use import.meta.url) before
calling path.resolve; locate the testDataPath creation in helpers.ts and adjust
resolution logic or update clients.test.ts to pass an absolute path accordingly.
---
Outside diff comments:
In `@src/lib/clients/atcoder/atcoder_problems.test.ts`:
- Around line 36-43: The tests currently overwrite client.getContests (and
similar client methods) directly; update these unit tests to use Nock to
intercept the HTTP requests made by the AtCoder problems client instead: remove
assignments like client.getContests = async () => contestsMock and instead
create a Nock scope that matches the external API base URL and path used inside
getContests, reply with contestsMock JSON, then call client.getContests() and
assert the response; apply the same replacement pattern for the other tests that
mock client methods (the tests referencing client.getContests,
client.getProblems, etc.) so the HTTP layer is exercised via Nock rather than
method stubbing.
In `@src/lib/clients/fixtures/helpers.ts`:
- Around line 38-49: try/finally ブロック内で "invalidJson.json"
を削除している処理は、既に同ファイルで定義されている afterAll のクリーンアップと重複しているため冗長です；該当する try/finally の
finally セクションから "invalidJson.json" を削除するコードを取り除き、afterAll
による一元的なクリーンアップに任せてください（対象を見つけるために "invalidJson.json" と該当の try/finally
ブロック、そして既存の afterAll を参照してください）。
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 2056ae85-a1a4-4d8b-b069-ef42c17b55ca
📒 Files selected for processing (20)
src/lib/clients/aizu_online_judge.tssrc/lib/clients/aizu_online_judge/clients.test.tssrc/lib/clients/aizu_online_judge/clients.tssrc/lib/clients/aizu_online_judge/types.tssrc/lib/clients/aizu_online_judge/utils.test.tssrc/lib/clients/aizu_online_judge/utils.tssrc/lib/clients/atcoder/atcoder_problems.test.tssrc/lib/clients/atcoder/atcoder_problems.tssrc/lib/clients/cache.test.tssrc/lib/clients/fixtures/aizu_online_judge/contests.jsonsrc/lib/clients/fixtures/aizu_online_judge/tasks.jsonsrc/lib/clients/fixtures/atcoder_problems/contests.jsonsrc/lib/clients/fixtures/atcoder_problems/tasks.jsonsrc/lib/clients/fixtures/helpers.test.tssrc/lib/clients/fixtures/helpers.tssrc/lib/clients/fixtures/record_requests.tssrc/lib/clients/http_client.tssrc/lib/clients/index.tssrc/test/lib/common/test_helpers.test.tssrc/test/lib/common/test_helpers.ts
💤 Files with no reviewable changes (4)
- src/test/lib/common/test_helpers.test.ts
- src/lib/clients/http_client.ts
- src/test/lib/common/test_helpers.ts
- src/lib/clients/aizu_online_judge.ts
…nally Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…lient tests Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/lib/clients/atcoder/atcoder_problems.test.ts (2)
61-66:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftクライアントを呼び出さずモックデータを直接検証しています。
これらのテストは
asyncでテスト名も API レスポンスの検証を示唆していますが、実際にはclient.getContests()を呼ばずにcontestsMockを直接イテレートしています。クライアントのパース・変換ロジックをテストしておらず、誤った安心感を与えます。各テストで nock モックをセットアップし、
client.getContests()を呼び出して戻り値を検証してください。🔧 修正例（line 61-66）
test('each contest expects to have id and title', async () => { + nock(API_BASE).get(`${API_PATH}contests.json`).reply(200, contestsMock); + const contests = await client.getContests(); - contestsMock.forEach((contest) => { + contests.forEach((contest) => { expect(contest.id).toBeDefined(); expect(contest.title).toBeDefined(); }); });同様の修正を line 74-81 にも適用してください。
Also applies to: 74-81
🤖 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/clients/atcoder/atcoder_problems.test.ts` around lines 61 - 66, Tests currently iterate contestsMock directly instead of exercising the parsing logic in AtCoderProblemsClient; update the tests to set up the existing nock HTTP mock, call client.getContests(), await its result, and assert on the returned contests (e.g., each item has id and title) rather than iterating contestsMock. Replace the direct contestsMock checks in the tests that reference contestsMock with calls to client.getContests(), ensure the nock fixture used in other tests is applied, and apply the same change for the second similar test that currently mirrors lines 74-81 so the client parsing/transform logic is actually validated.
92-99:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftクライアントを呼び出さずモックデータを直接検証しています。
上記のコンテスト関連テストと同じ問題です。
client.getTasks()を呼び出して戻り値を検証すべきです。🔧 修正例（line 92-99）
test('each task expects to have id, contest_id, problem_index and title', async () => { + nock(API_BASE).get(`${API_PATH}problems.json`).reply(200, tasksMock); + const tasks = await client.getTasks(); - tasksMock.forEach((task) => { + tasks.forEach((task) => { expect(task.id).toBeDefined(); expect(task.contest_id).toBeDefined(); expect(task.problem_index).toBeDefined(); expect(task.title).toBeDefined(); }); });同様の修正を line 107-116 にも適用してください。
Also applies to: 107-116
🤖 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/clients/atcoder/atcoder_problems.test.ts` around lines 92 - 99, The tests are directly iterating tasksMock instead of exercising the client; change the assertion to call client.getTasks() and validate the returned array items rather than tasksMock (replace uses of tasksMock in the test named "each task expects to have id, contest_id, problem_index and title" with the result of await client.getTasks()). Ensure you use await client.getTasks() and assert each returned task has id, contest_id, problem_index and title; apply the identical change to the other later test that currently iterates tasksMock (the similar block around the other tasks validation).
🤖 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.
Outside diff comments:
In `@src/lib/clients/atcoder/atcoder_problems.test.ts`:
- Around line 61-66: Tests currently iterate contestsMock directly instead of
exercising the parsing logic in AtCoderProblemsClient; update the tests to set
up the existing nock HTTP mock, call client.getContests(), await its result, and
assert on the returned contests (e.g., each item has id and title) rather than
iterating contestsMock. Replace the direct contestsMock checks in the tests that
reference contestsMock with calls to client.getContests(), ensure the nock
fixture used in other tests is applied, and apply the same change for the second
similar test that currently mirrors lines 74-81 so the client parsing/transform
logic is actually validated.
- Around line 92-99: The tests are directly iterating tasksMock instead of
exercising the client; change the assertion to call client.getTasks() and
validate the returned array items rather than tasksMock (replace uses of
tasksMock in the test named "each task expects to have id, contest_id,
problem_index and title" with the result of await client.getTasks()). Ensure you
use await client.getTasks() and assert each returned task has id, contest_id,
problem_index and title; apply the identical change to the other later test that
currently iterates tasksMock (the similar block around the other tasks
validation).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 874f3531-aa59-417f-810b-455b57a72dc6
📒 Files selected for processing (2)
src/lib/clients/atcoder/atcoder_problems.test.tssrc/lib/clients/fixtures/helpers.test.ts
…oblems client Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
close #3523
Summary by CodeRabbit
リリースノート
新機能
リファクタリング
テスト