refactor: use zod-openapi for ensanalytics-api-v1#1686
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. 3 Skipped Deployments
|
|
|
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. 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds a new ENS analytics v1 routes module with Zod/OpenAPI route descriptors, migrates the handler to register those routes via Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant App as ENSAPI App
participant Config as EditionConfigStore
participant Cache as EditionLeaderboardCache
participant Handler as Route Handler
Client->>App: HTTP GET /ensanalytics/v1/referral-leaderboard?edition=slug&page=1
App->>Handler: invoke getReferralLeaderboardRoute handler
Handler->>Config: check edition config availability
alt config load failed
Config-->>Handler: error (-> 503)
else config ok
Handler->>Cache: read leaderboard for edition
alt cache read failed / not found
Cache-->>Handler: error or empty (-> 503 or 404)
else cache hit
Cache-->>Handler: leaderboard data
Handler-->>Client: 200 + paginated leaderboard
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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 |
Greptile SummarySuccessfully migrated Key Changes:
Verification:
Confidence Score: 5/5
Important Files Changed
Last reviewed commit: 287852f |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull request overview
Refactors ensanalytics-api-v1 to follow the repository’s @hono/zod-openapi route-definition pattern (like amirealtime-api, resolution-api, ensnode-api) so OpenAPI definitions live in createRoute() modules and handlers register them via app.openapi().
Changes:
- Added
ensanalytics-api-v1.routes.tswithcreateRoute()definitions (+basePath/routesexports) for the 3 v1 endpoints. - Updated
ensanalytics-api-v1.tsto usecreateApp()andapp.openapi(route, handler)instead ofhono-openapi+describeRoute()+validate(). - Updated v1 tests to use
app.request()instead oftestClient(), and registered the new route group instub-routes.tsfor spec generation.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| apps/ensapi/src/stub-routes.ts | Registers the new ensanalytics-api-v1 route group for OpenAPI stub/spec generation. |
| apps/ensapi/src/handlers/ensanalytics-api-v1.ts | Migrates v1 handler implementation to @hono/zod-openapi route registration via app.openapi(). |
| apps/ensapi/src/handlers/ensanalytics-api-v1.test.ts | Switches tests from testClient(app) to app.request() calls. |
| apps/ensapi/src/handlers/ensanalytics-api-v1.routes.ts | New: defines v1 OpenAPI routes and request schemas using createRoute(). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![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 the current code and only fix it if needed.
Inline comments:
In `@apps/ensapi/src/handlers/ensanalytics-api-v1.routes.ts`:
- Around line 62-75: Add a 400 validation response to the /referral-leaderboard
route's OpenAPI responses: update the responses object used when registering the
`/referral-leaderboard` route in ensanalytics-api-v1.routes.ts to include a 400
entry (e.g., description: "Bad request — validation failed") and, if present in
your project schema conventions, reference the validation error schema or
example payload used elsewhere for request validation errors so consumers see
the expected error shape.
In `@apps/ensapi/src/handlers/ensanalytics-api-v1.ts`:
- Around line 107-116: The catch blocks that build the 500 response (the ones
calling serializeReferrerLeaderboardPageResponse with
ReferrerLeaderboardPageResponseCodes.Error and returning error/errorMessage) are
exposing raw exception messages; instead, remove usage of error.message in the
JSON response, log the full error internally (e.g., via processLogger or the
existing logger), and return a generic server-facing message such as { error:
"Internal server error", errorMessage: "An unexpected error occurred while
processing your request" } (or similar) while keeping the existing response
shape defined by ReferrerLeaderboardPageResponse; apply the same change to the
other two catch blocks that use the same serializer to ensure consistent mapping
per the guidelines.
- Line 222: The error log incorrectly names the endpoint as
"/v1/ensanalytics/referral-leaderboard/:referrer"; update the logger.error call
in the referrer-detail handler to reference the correct route (e.g.
"/v1/ensanalytics/referrer-detail/:referrer") so the log reflects the actual
handler; locate and edit the logger.error invocation (the logger.error({ error
}, "...") statement) in the referrer-detail handler to replace the endpoint
string accordingly.
ℹ️ Review info
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (4)
apps/ensapi/src/handlers/ensanalytics-api-v1.routes.tsapps/ensapi/src/handlers/ensanalytics-api-v1.test.tsapps/ensapi/src/handlers/ensanalytics-api-v1.tsapps/ensapi/src/stub-routes.ts
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
lightwalker-eth
left a comment
lightwalker-eth
left a comment
There was a problem hiding this comment.
@notrab Looks good, thank you 👍
There was a problem hiding this comment.
♻️ Duplicate comments (1)
apps/ensapi/src/handlers/ensanalytics-api-v1.ts (1)
107-116:⚠️ Potential issue | 🟠 MajorDo not expose raw exception messages in 500 responses.
Line [107], Line [223], and Line [279] return
error.messageto clients. This can leak internal details; keep full error detail in logs and return a fixed generic message in 500 responses.🔒 Suggested hardening
- const errorMessage = - error instanceof Error - ? error.message - : "An unexpected error occurred while processing your request"; + const errorMessage = "An unexpected error occurred while processing your request";As per coding guidelines "Map errors to response codes: validation errors (ZodError/Standard Schema) → 400 with
{ message, details }; known client errors → 4xx with{ message }; server errors → 500 with{ message }."Also applies to: 223-232, 279-288
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ensapi/src/handlers/ensanalytics-api-v1.ts` around lines 107 - 116, The handler currently exposes raw exception text by using error.message in responses built by serializeReferrerLeaderboardPageResponse with responseCode ReferrerLeaderboardPageResponseCodes.Error; replace that by returning a fixed generic message (e.g., "Internal server error") in the response payload and move the full error details to server logs (call your logger.error or console.error with the original error) so clients never receive internal exception text; update the same pattern wherever serializeReferrerLeaderboardPageResponse (and similar 500 branches) are used to ensure consistency.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@apps/ensapi/src/handlers/ensanalytics-api-v1.ts`:
- Around line 107-116: The handler currently exposes raw exception text by using
error.message in responses built by serializeReferrerLeaderboardPageResponse
with responseCode ReferrerLeaderboardPageResponseCodes.Error; replace that by
returning a fixed generic message (e.g., "Internal server error") in the
response payload and move the full error details to server logs (call your
logger.error or console.error with the original error) so clients never receive
internal exception text; update the same pattern wherever
serializeReferrerLeaderboardPageResponse (and similar 500 branches) are used to
ensure consistency.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Lite PR
Tip: Review docs on the ENSNode PR process
Summary
ensanalytics-api-v1(3 routes) fromhono-openapito@hono/zod-openapiroute definitions.ensanalytics-api-v1.routes.tswithcreateRoute()definitions, updated handler to usecreateApp()/app.openapi(), registered instub-routes.ts.Why
Consistent with the pattern established across other migrated routes (
ensnode-api,amirealtime-api,resolution-api), enabling OpenAPI spec generation from route definitions without runtime dependencies.Testing
git diffthat everyc.json()call,responseCode,error,errorMessage,satisfiestype assertion, and serialization function is byte-for-byte identical to main (only indentation changed from removing chained.get()nesting).Notes for Reviewer (Optional)
Test file updated to replace
testClient(app)withapp.request().OpenAPIHono'sapp.openapi()doesn't produce the chained route types thattestClientneeds for inference. Theapp.request()pattern is already used byamirealtime-api.test.tsand by several tests in this same file.Pre-Review Checklist (Blocking)