Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 10 minutes and 11 seconds. ⌛ 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 (6)
📝 WalkthroughWalkthroughLoad the installed Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
Coverage Report for CI Build 24315208937Coverage decreased (-0.02%) to 31.681%Details
Uncovered Changes
Coverage RegressionsNo coverage regressions found. Coverage Stats
💛 - Coveralls |
aspiers
force-pushed
the
fix/xrpc-health-pds-version
branch
from
bdb7adf to
631af4f
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/pds-core/src/index.ts`:
- Around line 28-29: Add "type": "module" to the pds-core package.json and
update the runtime JSON import used for atprotoPdsVersion: replace the bare
require() usage of the '@atproto/pds/package.json' (the atprotoPdsVersion const)
with a Node ESM-safe import approach — either use Node's createRequire to load
the JSON or use an import with assertion (JSON import) so that atprotoPdsVersion
continues to be populated without triggering "require is not defined" under ESM.
🪄 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: d240989d-17fe-43ae-89c5-81bb2f71c866
📒 Files selected for processing (1)
packages/pds-core/src/index.ts
aspiers
force-pushed
the
fix/xrpc-health-pds-version
branch
from
631af4f to
1e874b4
Compare
aspiers
force-pushed
the
fix/xrpc-health-pds-version
branch
from
1e874b4 to
f55c108
Compare
aspiers
force-pushed
the
fix/xrpc-health-pds-version
branch
2 times, most recently
from
8ea95f3 to
fd44f43
Compare
aspiers
force-pushed
the
fix/xrpc-health-pds-version
branch
2 times, most recently
from
fd44f43 to
0208730
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
e2e/step-definitions/health.steps.ts (1)
7-11: Consider extracting a shared request helper for theseWhensteps.The fetch/status/json assignment pattern is repeated three times; a small helper would reduce drift and keep failures consistent.
♻️ Suggested refactor
+async function fetchJsonIntoWorld(this: EpdsWorld, url: string): Promise<void> { + const res = await fetch(url) + this.lastHttpStatus = res.status + this.lastHttpJson = (await res.json()) as Record<string, unknown> +} + When('the PDS /health endpoint is queried', async function (this: EpdsWorld) { - const res = await fetch(`${testEnv.pdsUrl}/health`) - this.lastHttpStatus = res.status - this.lastHttpJson = (await res.json()) as Record<string, unknown> + await fetchJsonIntoWorld.call(this, `${testEnv.pdsUrl}/health`) }) @@ async function (this: EpdsWorld) { - const res = await fetch(`${testEnv.authUrl}/health`) - this.lastHttpStatus = res.status - this.lastHttpJson = (await res.json()) as Record<string, unknown> + await fetchJsonIntoWorld.call(this, `${testEnv.authUrl}/health`) }, ) @@ async function (this: EpdsWorld) { - const res = await fetch(`${testEnv.pdsUrl}/xrpc/_health`) - this.lastHttpStatus = res.status - this.lastHttpJson = (await res.json()) as Record<string, unknown> + await fetchJsonIntoWorld.call(this, `${testEnv.pdsUrl}/xrpc/_health`) }, )Also applies to: 33-40, 44-51
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/step-definitions/health.steps.ts` around lines 7 - 11, Extract the repeated fetch+status+json logic into a shared async helper (e.g., in the same file or a test utils module) and call it from each When step; the helper should accept a URL (or base + path) and the EpdsWorld instance and set world.lastHttpStatus and world.lastHttpJson consistently. Replace the body of the When handler in health.steps.ts (the When('the PDS /health endpoint is queried', async function (this: EpdsWorld) { ... })) and the other When handlers referenced (lines 33-40 and 44-51) to call this helper with testEnv.pdsUrl + '/health' (or the appropriate path) so all fetch/status/json assignments are centralized and identical. Ensure the helper returns or throws errors consistently so test failure behavior remains uniform.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@e2e/step-definitions/health.steps.ts`:
- Around line 13-29: Before checking the version field, assert the HTTP response
status is a success (e.g., 200 or any 2xx) so non-200 responses cannot
accidentally pass validation; in the Then step that runs inside the EpdsWorld
using this.lastHttpJson and this.lastHttpResponse (or similar response object),
add a guard validating this.lastHttpResponse.status is 200 (or within 200-299)
and throw a clear error if not, then proceed with the existing version existence
and semver checks on this.lastHttpJson.version. Ensure you apply the same change
to the other Then step referenced (lines 53-68) so both health assertions
validate response status first.
---
Nitpick comments:
In `@e2e/step-definitions/health.steps.ts`:
- Around line 7-11: Extract the repeated fetch+status+json logic into a shared
async helper (e.g., in the same file or a test utils module) and call it from
each When step; the helper should accept a URL (or base + path) and the
EpdsWorld instance and set world.lastHttpStatus and world.lastHttpJson
consistently. Replace the body of the When handler in health.steps.ts (the
When('the PDS /health endpoint is queried', async function (this: EpdsWorld) {
... })) and the other When handlers referenced (lines 33-40 and 44-51) to call
this helper with testEnv.pdsUrl + '/health' (or the appropriate path) so all
fetch/status/json assignments are centralized and identical. Ensure the helper
returns or throws errors consistently so test failure behavior remains uniform.
🪄 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: 4cfb8436-3d82-48dc-a074-067767bf4958
📒 Files selected for processing (6)
.changeset/version-health-endpoint.md.changeset/xrpc-health-pds-version.mde2e/cucumber.mjse2e/step-definitions/health.steps.tsfeatures/health.featurepackages/pds-core/src/index.ts
✅ Files skipped from review due to trivial changes (5)
- e2e/cucumber.mjs
- packages/pds-core/src/index.ts
- features/health.feature
- .changeset/xrpc-health-pds-version.md
- .changeset/version-health-endpoint.md
The vanilla @atproto/pds entrypoint does `env.version ??= pkg.version`
to fall back to the package.json version when PDS_VERSION is unset.
ePDS omitted this, so /xrpc/_health returned {} instead of reporting
the upstream PDS version.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
aspiers
force-pushed
the
fix/xrpc-health-pds-version
branch
from
0208730 to
f709066
Compare
|
|
For some weird reason, Railway was absolutely refusing to deploy a preview environment for this PR, so I had to deploy one manually. |
|
Manually triggered E2E tests ran fine: https://github.com/hypercerts-org/ePDS/actions/runs/24315210319/job/70991623246 and the other failures are trivial so I'm merging this. |
1 participant
Summary
@atproto/pdsentrypoint doesenv.version ??= pkg.versionto fall back to the package.json version whenPDS_VERSIONis unset/xrpc/_healthreturned{}instead of{ "version": "0.4.211" }@atproto/pds/package.jsonversion and apply the same??=fallback beforeenvToCfg()Note: this is the upstream PDS version on
/xrpc/_health, separate from the ePDS version on/health(HYPER-304, PR #74).Test plan
curl https://<pds>/xrpc/_healthreturns{ "version": "0.4.211" }(or current @atproto/pds version)/healthstill returns the ePDS version separately🤖 Generated with Claude Code
Summary by CodeRabbit
Chores
New Features
Tests
Documentation