docs(claude): add workflow rules#3293
Conversation
…gs (#3291) Wrap GitHub API calls in try/catch and return empty array on failure. Prevents 422 errors when rate-limited or repos are unreachable.
Tests now expect 200 with empty data when GitHub API fails, matching the service's catch-and-return-empty-array behavior.
|
Warning Rate limit exceeded
⌛ 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 (3)
✨ 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 |
There was a problem hiding this comment.
Pull request overview
This PR updates the project’s Claude workflow guidance and also changes the Home module’s GitHub-backed endpoints to return successful empty lists when the GitHub API is unavailable.
Changes:
- Added “Workflow rules” guidance to
CLAUDE.md(PR workflow, coverage thresholds, module audit, verify). - Updated
HomeService.releases()/HomeService.changelogs()to return[]on failures instead of propagating errors. - Updated Home integration tests to expect
200success with empty data when GitHub API calls fail.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
modules/home/tests/home.integration.tests.js |
Adjusts integration test expectations to match the new “graceful empty list” behavior for GitHub failures. |
modules/home/services/home.service.js |
Wraps GitHub API calls in try/catch and returns empty arrays on error for releases/changelogs. |
CLAUDE.md |
Adds explicit workflow rules for PR usage, coverage thresholds, auditing existing modules, and running /verify. |
| ## Workflow rules | ||
|
|
||
| - **Never push directly to master/main.** Always create a branch, push, create a PR, wait for CI green + review, then merge. | ||
| - **Never lower coverage thresholds** in `jest.config.js`. If coverage drops after adding code, write tests for the new project modules to bring it back above thresholds. | ||
| - **Audit existing modules before implementing.** Before creating new storage, file handling, or utility code, check `modules/` for existing solutions (e.g., `uploads` module for file storage via GridFS). | ||
| - **Always run `/verify` after any code change** before declaring done. CI must be green. |
There was a problem hiding this comment.
The PR title/description indicate a docs-only change to CLAUDE workflow rules, but this PR also changes runtime behavior and integration tests for the Home GitHub endpoints. Please either split the Home service/test changes into a separate PR or update the PR title/description to reflect the code behavior change.
| try { | ||
| const requests = config.repos.map((item) => | ||
| axios.get(`https://api.github.com/repos/${item.owner}/${item.repo}/releases`, { | ||
| headers: item.token ? { Authorization: `token ${item.token}` } : {}, | ||
| }), | ||
| ); | ||
| let results = await axios.all(requests); | ||
| results = results.map((result, i) => ({ | ||
| title: config.repos[i].title, | ||
| list: result.data.map((release) => ({ | ||
| name: release.name, | ||
| prerelease: release.prerelease, | ||
| published_at: release.published_at, | ||
| })), | ||
| })); | ||
| return results; | ||
| } catch (_err) { | ||
| return []; | ||
| } |
There was a problem hiding this comment.
The try/catch is broad enough that it will swallow non-network bugs/misconfigurations (e.g. config.repos missing/invalid or unexpected response shape) and silently return an empty list, making those failures hard to detect in production. Consider narrowing the catch to expected Axios/GitHub failures and/or logging the error (e.g. via lib/services/logger) before returning the fallback so operational issues remain observable.
| try { | ||
| const repos = _.filter(config.repos, (repo) => repo.changelog); | ||
| const requests = repos.map((item) => | ||
| axios.get(`https://api.github.com/repos/${item.owner}/${item.repo}/contents/${item.changelog}`, { | ||
| headers: item.token ? { Authorization: `token ${item.token}` } : {}, | ||
| }), | ||
| ); | ||
| let results = await axios.all(requests); | ||
| results = results.map((result, i) => ({ | ||
| title: repos[i].title, | ||
| markdown: Base64.decode(result.data.content), | ||
| })); | ||
| return results; | ||
| } catch (_err) { | ||
| return []; | ||
| } |
There was a problem hiding this comment.
Same concern here: catching all errors and returning [] will also hide unexpected decoding/shape errors (not just GitHub API unavailability). Suggest logging the caught error and limiting the fallback to expected request failures so real bugs still surface.
Add rules: always use PRs, never lower coverage thresholds, audit existing modules before implementing, always run /verify.