refactor: use zod-openapi for ensnode-api#1672
Conversation
|
|
The latest updates on your projects. Learn more about Vercel for GitHub.
2 Skipped Deployments
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review infoConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughExtracts two ENS API routes into a new routes metadata module, updates the main handler to wire those routes via app.openapi using a new createApp import, and includes the new route group in the stub route aggregator. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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 |
Greptile SummaryMigrated Key changes:
Confidence Score: 5/5
Important Files Changed
Last reviewed commit: 8af0edc |
![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
Migrates ensnode-api’s /config and /indexing-status endpoints to the @hono/zod-openapi (createRoute + app.openapi) route-definition pattern, and wires these route definitions into the stub OpenAPI route registration flow.
Changes:
- Refactor
ensnode-apihandler to register endpoints viaapp.openapi(...)using newensnode-api.routes.ts. - Add
ensnode-api.routes.tswithcreateRoute(...)definitions for/configand/indexing-status. - Include
ensnode-api.routesincreateStubRoutesForSpec()route registration.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| apps/ensapi/src/stub-routes.ts | Adds ENSNode route definitions to stub OpenAPI route registration. |
| apps/ensapi/src/handlers/ensnode-api.ts | Switches /config and /indexing-status to app.openapi registrations and uses createApp(). |
| apps/ensapi/src/handlers/ensnode-api.routes.ts | Introduces zod-openapi createRoute definitions for ENSNode meta endpoints. |
Comments suppressed due to low confidence (1)
apps/ensapi/src/stub-routes.ts:19
createStubRoutesForSpec()builds OpenAPI paths fromgroup.basePath + route.path, but this doesn’t match the real routing tree for nested apps (e.g.,resolution-apiis mounted at/api/resolveviaensnode-api.ts+index.ts, yet this will register/resolve/...). If this stub app is used for spec generation, the resulting OpenAPI paths will be incorrect. Consider encoding the mount hierarchy in this file (e.g., prefix resolution routes with/api), or switching the route groups to store an explicit full prefix per group instead of relying on each module’sbasePathalone.
const routeGroups = [amIRealtimeRoutes, ensnodeRoutes, resolutionRoutes];
for (const group of routeGroups) {
for (const route of group.routes) {
const path = route.path === "/" ? group.basePath : `${group.basePath}${route.path}`;
💡 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: 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 `@apps/ensapi/src/handlers/ensnode-api.routes.ts`:
- Around line 5-32: Both getConfigRoute and getIndexingStatusRoute are missing
response body schemas; add a content entry for "application/json" with a Zod
schema so `@hono/zod-openapi` can infer response types. Update the route objects
(getConfigRoute, getIndexingStatusRoute) to include responses like 200: {
description: "...", content: { "application/json": { schema: /* Zod schema
matching serializeENSApiPublicConfig() return type */ } } } and for the 503
response add a content schema for the error shape if applicable; use or derive
Zod schemas that match the existing serializer return shapes
(serializeENSApiPublicConfig and serializeIndexingStatusResponse) so the OpenAPI
output and handler c.json(...) payloads are type-checked.
ℹ️ Review info
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
apps/ensapi/src/handlers/ensnode-api.routes.tsapps/ensapi/src/handlers/ensnode-api.tsapps/ensapi/src/stub-routes.ts
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 `@apps/ensapi/src/handlers/ensnode-api.routes.ts`:
- Around line 28-49: The 503 response for getIndexingStatusRoute incorrectly
reuses makeSerializedEnsApiIndexingStatusResponseSchema() (a union of ok+error);
change the 503 response to use the SDK's error-only schema for this endpoint
instead so OpenAPI advertises only the error payload (replace the 503 content
schema with the SDK error-only schema for indexing status, e.g.
makeSerializedEnsApiIndexingStatusErrorResponseSchema or the equivalent
error-only factory provided by the SDK).
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 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.
lightwalker-eth
left a comment
lightwalker-eth
left a comment
There was a problem hiding this comment.
@notrab Thanks for your updates. Shared a few suggestions. Please merge when ready.
| @@ -0,0 +1,55 @@ | |||
| import { createRoute } from "@hono/zod-openapi"; | |||
|
|
|||
There was a problem hiding this comment.
Let's rename this file to meta.routes.ts as all the routes here represent "meta" routes that return various metadata.
| makeENSApiPublicConfigSchema, | ||
| makeIndexingStatusResponseSchema, | ||
| } from "@ensnode/ensnode-sdk/internal"; | ||
|
|
There was a problem hiding this comment.
I note an inconsistency here.
For example: I see:
import ensnodeGraphQLApi from "./ensnode-graphql-api";
import nameTokensApi from "./name-tokens-api";
import registrarActionsApi from "./registrar-actions-api";
import resolutionApi from "./resolution-api";
... for many APIs. But why isn't this pattern followed for the "meta" API handlers for getting the config or indexing status (which are the "meta" APIs)?
Why are these API handlers defined directly in this file while no other APIs are directly implemented in this file?
There was a problem hiding this comment.
@lightwalker-eth this was pre-existing inconsistency which is why I did not refactor it at this stage. I only refactored the meta handlers from hono-openapi (describeRoute / app.get) to zod-openapi (createRoute / app.openapi) and extracted the route definitions to ensnode-api.routes.ts.
Lite PR
Tip: Review docs on the ENSNode PR process
Summary
ensnode-apiroutes (/config,/indexing-status) fromdescribeRoute()(hono-openapi) tocreateRoute()(@hono/zod-openapi) pattern.openapi-routes.tstostub-routes.tsandcreateRoutesForSpectocreateStubRoutesForSpecto clarify stub-only purpose.openapi-documentation.tstoopenapi-meta.tsandopenapiDocumentationtoopenapiMeta.Why
@hono/zod-openapifor consistent OpenAPI spec generation across all endpoints.Testing
Notes for Reviewer (Optional)
now usescreateApp()(from@/lib/hono-factory) instead offactory.createApp(), matching the pattern used by the other migrated handlers (amirealtime-api.ts,resolution-api.ts`).name-tokens-api,registrar-actions-api,ensanalytics-api,ensanalytics-api-v1.Pre-Review Checklist (Blocking)