Issue/190 rate limit config and feature tests#191
Conversation
|
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: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThe PR introduces rate-limit configuration and feature tests for the API. A new ChangesRate-limit configuration and feature tests
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 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}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
tests/Feature/modules/Api/RateLimitTest.php (1)
64-75: ⚡ Quick winRemove duplicated rate-limit cleanup logic from this test class.
This re-implements cleanup already handled in
AppTestCase, and the two versions can drift. Promote the base helper toprotectedand reuse it here to keep isolation behavior centralized.Refactor direction
- private function clearRateLimitStorage(): void - { - $rateLimitDir = PROJECT_ROOT . DS . 'cache' . DS . 'data'; - - foreach (glob($rateLimitDir . DS . '*.rate') ?: [] as $file) { - `@unlink`($file); - } - - foreach (glob($rateLimitDir . DS . '*.lock') ?: [] as $file) { - `@unlink`($file); - } - } + // Use inherited AppTestCase::clearRateLimitStorage()- private function clearRateLimitStorage(): void + protected function clearRateLimitStorage(): void🤖 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 `@tests/Feature/modules/Api/RateLimitTest.php` around lines 64 - 75, The private clearRateLimitStorage() in RateLimitTest duplicates cleanup logic already in AppTestCase; remove this method from tests/Feature/modules/Api/RateLimitTest.php and instead call the base helper. Make the base helper in AppTestCase protected (rename/ensure method name clearRateLimitStorage exists there) so subclasses can reuse it, and update RateLimitTest to rely on parent::clearRateLimitStorage() or simply omit the duplicate implementation so the centralized cleanup is used.
🤖 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 `@tests/Feature/AppTestCase.php`:
- Around line 68-74: The cleanup loops that remove limiter files currently use
error suppression (`@unlink`) on $file which can hide failures; update the two
loops that iterate glob($rateLimitDir . DS . '*.rate') and glob($rateLimitDir .
DS . '*.lock') to remove the "@" and explicitly check unlink($file) return
value, and if unlink returns false throw an exception (e.g., RuntimeException)
or call the test failure helper (e.g., $this->fail) with a message including
$file so the test fails fast when a file cannot be removed.
In `@tests/Feature/modules/Api/RateLimitTest.php`:
- Around line 81-83: The setup currently calls mkdir($rateLimitDir, 0777, true)
without verifying success; update RateLimitTest to assert the directory was
created by checking the mkdir return (or is_dir afterwards) and fail the test if
creation failed. Specifically, after the existing is_dir check and mkdir call
for $rateLimitDir, add an assertion (e.g., using $this->assertTrue or
$this->assertDirectoryExists) that the directory exists or that mkdir returned
true, and include a clear message so the test stops immediately on creation
failure.
---
Nitpick comments:
In `@tests/Feature/modules/Api/RateLimitTest.php`:
- Around line 64-75: The private clearRateLimitStorage() in RateLimitTest
duplicates cleanup logic already in AppTestCase; remove this method from
tests/Feature/modules/Api/RateLimitTest.php and instead call the base helper.
Make the base helper in AppTestCase protected (rename/ensure method name
clearRateLimitStorage exists there) so subclasses can reuse it, and update
RateLimitTest to rely on parent::clearRateLimitStorage() or simply omit the
duplicate implementation so the centralized cleanup is used.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 68b55c37-11aa-4674-a14b-21227977b280
📒 Files selected for processing (3)
shared/config/rate_limit.phptests/Feature/AppTestCase.phptests/Feature/modules/Api/RateLimitTest.php
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6f393ef6df
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
1 participant
Closes #190
Summary by CodeRabbit
New Features
Tests