Conversation
|
|
The latest updates on your projects. Learn more about Vercel for GitHub.
1 Skipped Deployment
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a TypeScript OpenAPI generation script and npm tasks, changes ENSApi OpenAPI generation to a no-argument form, annotates route schemas with OpenAPI metadata via Changes
Sequence Diagram(s)sequenceDiagram
participant Dev as Developer
participant Script as generate:openapi script
participant Generator as ENSApi OpenAPI generator
participant FS as Filesystem
participant Biome as Biome (via pnpm)
Dev->>Script: run `pnpm run generate:openapi`
Script->>Generator: call generateOpenApi31Document()
Generator-->>Script: returns OpenAPI 3.1 JSON
Script->>FS: write `docs/docs.ensnode.io/ensapi-openapi.json`
Script->>Biome: run `pnpm --silent --filter docs ... biome format ...`
Biome-->>Script: success / failure
Script-->>Dev: log result / exit
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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 |
There was a problem hiding this comment.
Mintlify has no scope to go to the monorepo directory, so we need to symlink tpo it.
There was a problem hiding this comment.
Looks like Mintlify still can't follow the symlink. Will try another method.
There was a problem hiding this comment.
Pull request overview
Adds a generated-from-source, committed OpenAPI 3.1 spec for ENSApi and wires it into docs + scripts to support preview builds and future multi-spec expansion across the monorepo.
Changes:
- Add an ENSApi script to generate a static OpenAPI 3.1 document and commit the generated spec at
openapi/ensapi-openapi.json. - Add
generate:openapiscripts at the app and monorepo root for easy regeneration. - Update Mintlify docs config to point the live API reference at the production OpenAPI endpoint and add a hidden preview group that references the committed spec file.
Reviewed changes
Copilot reviewed 5 out of 6 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| package.json | Bumps pinned pnpm version and adds a root generate:openapi script. |
| apps/ensapi/package.json | Adds generate:openapi script for ENSApi. |
| apps/ensapi/scripts/generate-openapi.ts | New generator script that writes the spec and formats it. |
| openapi/ensapi-openapi.json | Newly committed generated OpenAPI 3.1 spec. |
| docs/docs.ensnode.io/docs.json | Points API Reference to live OpenAPI endpoint and adds hidden preview spec reference. |
💡 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: 2
🤖 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/scripts/generate-openapi.ts`:
- Around line 19-20: The path for outputPath is brittle because it uses
hardcoded "../../.." traversal from __dirname; replace that with a robust root
discovery (e.g., implement a findMonorepoRoot function that walks parent
directories from dirname(fileURLToPath(import.meta.url)) or uses an env var like
MONOREPO_ROOT) and then build outputPath by resolve(monorepoRoot, "openapi",
"ensapi-openapi.json"); update references to __dirname/outputPath to use
fileURLToPath, dirname, and the new findMonorepoRoot (or env fallback) so the
script no longer depends on the file's exact relative depth.
In `@docs/docs.ensnode.io/docs.json`:
- Around line 30-31: The "Preview" group in docs.json references a non-existent
page "ensapi/preview" in its "pages" array; either remove "ensapi/preview" from
the "pages" array in the Preview group configuration or create a matching docs
file (e.g., add ensapi/preview.mdx) with the expected frontmatter and content so
the reference resolves; locate the "group": "Preview" entry and update its
"pages" array or add the new "ensapi/preview.mdx" file accordingly.
ℹ️ Review info
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (5)
apps/ensapi/package.jsonapps/ensapi/scripts/generate-openapi.tsdocs/docs.ensnode.io/docs.jsonopenapi/ensapi-openapi.jsonpackage.json
Greptile SummaryThis PR adds automated OpenAPI spec generation, replacing the manually-uploaded Gist with a source-generated committed spec that enables PR preview builds. Key Changes:
Implementation Quality:
Confidence Score: 5/5
Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[pnpm generate:openapi] --> B[docs package script]
B --> C[Execute tsx in ensapi context]
C --> D[scripts/generate-ensapi-openapi.ts]
D --> E[generateOpenApi31Document]
E --> F[Create stub routes]
F --> G[Generate OpenAPI 3.1 JSON]
G --> H[Write to ensapi-openapi.json]
H --> I[Format with Biome]
I --> J[Committed spec file]
J --> K[docs.json references spec]
K --> L[Preview page displays spec]
Last reviewed commit: 9019bef |
![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
Copilot reviewed 5 out of 6 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![vercel[bot]](https://avatars.githubusercontent.com/in/8329?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 9 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| { | ||
| "schema": { | ||
| "description": "Maximum acceptable worst-case indexing distance in seconds" | ||
| }, | ||
| "required": false, | ||
| "description": "Maximum acceptable worst-case indexing distance in seconds", | ||
| "name": "maxWorstCaseDistance", | ||
| "in": "query" | ||
| } |
There was a problem hiding this comment.
In the committed OpenAPI spec, the maxWorstCaseDistance query parameter schema is missing a concrete type (it only has a description). This makes the spec harder to validate and can break client generation. Consider adjusting the underlying Zod/OpenAPI metadata so this parameter emits a type: number (and any relevant constraints) in the generated spec.
There was a problem hiding this comment.
This is also valid feedback from Copilot. Please take the lead in solving it for us.
| { | ||
| "schema": { "type": "string", "default": false }, | ||
| "required": false, | ||
| "name": "trace", | ||
| "in": "query" | ||
| }, | ||
| { | ||
| "schema": { "type": "string", "default": false }, | ||
| "required": false, | ||
| "name": "accelerate", | ||
| "in": "query" | ||
| } |
There was a problem hiding this comment.
The trace and accelerate query parameters are emitted as type: string with default: false in the OpenAPI spec. This type/default mismatch is confusing for docs and can cause issues for client generators. Consider updating the underlying parameter schema/OpenAPI annotations so these flags are represented as booleans (or keep them as strings but use a string default like "false").
There was a problem hiding this comment.
This is valid feedback from Copilot. Please take the lead in solving it for us.
lightwalker-eth
left a comment
lightwalker-eth
left a comment
There was a problem hiding this comment.
@notrab Excited for this. Reviewed and shared feedback please merge when ready.
| // Format with Biome for consistency | ||
| console.log("Formatting with Biome..."); | ||
| try { | ||
| execFileSync("pnpm", ["-w", "exec", "biome", "format", "--write", outputPath], { |
There was a problem hiding this comment.
Any special reason why not just pnpm lint? See https://github.com/namehash/ensnode/blob/b0de5e9cda0f2919aa201e1ec26a05a6473dc03c/package.json#L7C6-L7C10
Please ignore if not relevant.
There was a problem hiding this comment.
@lightwalker-eth the current action formats a single file, where as pnpm lint targets the entire repo.
I remember a while back we were hesitant to run such commands in CI, so I opted to stay focused on the file we were writing here.
| console.error("Error: Failed to format with Biome."); | ||
| if (error instanceof Error) { | ||
| const err = error as NodeJS.ErrnoException & { status?: number }; | ||
| if (err.code === "ENOENT") { | ||
| console.error("'pnpm' or 'biome' is not available on your PATH."); | ||
| } else if (err.status !== undefined) { | ||
| console.error(`Biome exited with code ${err.status}.`); | ||
| console.error("Try running 'pnpm biome format --write' manually to debug."); | ||
| } else if (err.message) { |
There was a problem hiding this comment.
Please review this feedback from Copilot and take the lead in deciding if / how to action it.
| "scripts": { | ||
| "mint": "pnpm dlx mint@^4.1.0" | ||
| "mint": "pnpm dlx mint@^4.1.0", | ||
| "generate:openapi": "pnpm --filter ensapi exec tsx --tsconfig tsconfig.json ../../scripts/generate-ensapi-openapi.ts" |
There was a problem hiding this comment.
I assume we should be documenting this in the related README.md file?
| { | ||
| "schema": { "type": "string", "default": false }, | ||
| "required": false, | ||
| "name": "trace", | ||
| "in": "query" | ||
| }, | ||
| { | ||
| "schema": { "type": "string", "default": false }, | ||
| "required": false, | ||
| "name": "accelerate", | ||
| "in": "query" | ||
| } |
There was a problem hiding this comment.
This is valid feedback from Copilot. Please take the lead in solving it for us.
| { | ||
| "schema": { | ||
| "description": "Maximum acceptable worst-case indexing distance in seconds" | ||
| }, | ||
| "required": false, | ||
| "description": "Maximum acceptable worst-case indexing distance in seconds", | ||
| "name": "maxWorstCaseDistance", | ||
| "in": "query" | ||
| } |
There was a problem hiding this comment.
This is also valid feedback from Copilot. Please take the lead in solving it for us.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/ensapi/src/handlers/amirealtime-api.routes.ts (1)
28-36:⚠️ Potential issue | 🟡 MinorOpenAPI type doesn't match runtime validation.
The OpenAPI metadata specifies
type: "integer"butz.coerce.number()accepts any numeric value including floats. This creates a contract mismatch between the documented API and actual behavior.If integers are intended, add
.int()to the Zod chain. If floats are acceptable for durations, change OpenAPI type to"number".Option A: Enforce integers at runtime
.pipe( z.coerce .number({ error: "maxWorstCaseDistance query param must be a number", }) + .int({ + error: "maxWorstCaseDistance query param must be an integer", + }) .pipe(makeDurationSchema("maxWorstCaseDistance query param")), ) .openapi({ type: "integer", minimum: 0 })Option B: Allow floats in OpenAPI spec
- .openapi({ type: "integer", minimum: 0 }) + .openapi({ type: "number", minimum: 0 })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ensapi/src/handlers/amirealtime-api.routes.ts` around lines 28 - 36, The OpenAPI type for the maxWorstCaseDistance query param is "integer" but the runtime schema uses z.coerce.number() which allows floats; update the Zod chain for the maxWorstCaseDistance parameter (the z.coerce.number() call that then pipes into makeDurationSchema) to enforce integers by adding .int() (e.g., z.coerce.number().int().pipe(...)), so the runtime validation (in the code that references maxWorstCaseDistance and makeDurationSchema) matches the openapi({ type: "integer" }) metadata; alternatively, if floats are intended, change openapi({ type: "integer" }) to openapi({ type: "number" }) to match z.coerce.number().
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@apps/ensapi/src/handlers/amirealtime-api.routes.ts`:
- Around line 28-36: The OpenAPI type for the maxWorstCaseDistance query param
is "integer" but the runtime schema uses z.coerce.number() which allows floats;
update the Zod chain for the maxWorstCaseDistance parameter (the
z.coerce.number() call that then pipes into makeDurationSchema) to enforce
integers by adding .int() (e.g., z.coerce.number().int().pipe(...)), so the
runtime validation (in the code that references maxWorstCaseDistance and
makeDurationSchema) matches the openapi({ type: "integer" }) metadata;
alternatively, if floats are intended, change openapi({ type: "integer" }) to
openapi({ type: "number" }) to match z.coerce.number().
ℹ️ Review info
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (7)
apps/ensapi/src/handlers/amirealtime-api.routes.tsapps/ensapi/src/lib/handlers/params.schema.tsdocs/docs.ensnode.io/ensapi-openapi.jsondocs/docs.ensnode.io/ensapi/preview.mdxdocs/docs.ensnode.io/package.jsonpackage.jsonscripts/generate-ensapi-openapi.ts
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 11 changed files in this pull request and generated 2 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 11 out of 12 changed files in this pull request and generated 3 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 15 out of 16 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.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@package.json`:
- Line 23: Update the package.json script named "generate:openapi" to run pnpm
recursively but skip packages that don't define that script by adding the
--if-present flag; replace the current command "pnpm -r generate:openapi" with
"pnpm -r --if-present generate:openapi" so recursive execution tolerates
packages missing the generate:openapi script.
In `@scripts/generate-ensapi-openapi.ts`:
- Around line 1-10: Add a CI check that prevents OpenAPI spec drift by running
the existing generate:openapi script (which invokes generateOpenApi31Document in
scripts/generate-ensapi-openapi.ts) during PR/CI and failing the job if the
generated artifact changes; implement this by adding a step that runs `pnpm
generate:openapi` and then `git diff --exit-code <output-filename>` (replace
<output-filename> with the committed ensapi-openapi.json artifact name) so any
divergence causes CI to fail.
ℹ️ Review info
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (12)
apps/ensapi/src/handlers/amirealtime-api.routes.tsapps/ensapi/src/handlers/ensanalytics-api-v1.routes.tsapps/ensapi/src/handlers/ensanalytics-api.routes.tsapps/ensapi/src/handlers/name-tokens-api.routes.tsapps/ensapi/src/handlers/registrar-actions-api.routes.tsapps/ensapi/src/handlers/resolution-api.routes.tsapps/ensapi/src/lib/handlers/params.schema.tsdocs/docs.ensnode.io/ensapi-openapi.jsondocs/docs.ensnode.io/ensapi/preview.mdxdocs/docs.ensnode.io/package.jsonpackage.jsonscripts/generate-ensapi-openapi.ts
| /** | ||
| * Generates a static OpenAPI 3.1 JSON document for ENSApi. | ||
| * | ||
| * Usage: pnpm generate:openapi | ||
| * | ||
| * Output: docs/docs.ensnode.io/ensapi-openapi.json | ||
| * | ||
| * This script has no runtime dependencies — it calls generateOpenApi31Document() | ||
| * which uses only stub route handlers and static metadata. | ||
| */ |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Add a CI drift check for committed OpenAPI output.
Since this script produces a committed artifact, add a CI step that runs generation and fails on diff to prevent spec drift.
Example check:
pnpm generate:openapi && git diff --exit-code docs/docs.ensnode.io/ensapi-openapi.json
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/generate-ensapi-openapi.ts` around lines 1 - 10, Add a CI check that
prevents OpenAPI spec drift by running the existing generate:openapi script
(which invokes generateOpenApi31Document in scripts/generate-ensapi-openapi.ts)
during PR/CI and failing the job if the generated artifact changes; implement
this by adding a step that runs `pnpm generate:openapi` and then `git diff
--exit-code <output-filename>` (replace <output-filename> with the committed
ensapi-openapi.json artifact name) so any divergence causes CI to fail.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 15 out of 16 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| "docker:build:ensapi": "docker build -f apps/ensapi/Dockerfile -t ghcr.io/namehash/ensnode/ensapi:latest .", | ||
| "otel-desktop-viewer": "docker run -p 8000:8000 -p 4317:4317 -p 4318:4318 davetron5000/otel-desktop-viewer:alpine-3" | ||
| "otel-desktop-viewer": "docker run -p 8000:8000 -p 4317:4317 -p 4318:4318 davetron5000/otel-desktop-viewer:alpine-3", | ||
| "generate:openapi": "pnpm -r --if-present generate:openapi" |
There was a problem hiding this comment.
The PR description mentions running pnpm openapi:generate, but the repo script added here is generate:openapi. Consider updating the PR description/README to match the actual command (or add an alias script if you want to support both names).
| "description": "Resolves ENS records for a given name", | ||
| "parameters": [ | ||
| { "schema": { "type": "string" }, "required": true, "name": "name", "in": "path" }, | ||
| { "schema": { "type": "string" }, "required": false, "name": "name", "in": "query" }, |
There was a problem hiding this comment.
The generated spec documents /api/resolve/records/{name} with both a path parameter named name and a query parameter also named name (used for field selection). Even though this is technically allowed (different in), it’s very confusing for API consumers and makes the docs ambiguous. Consider renaming the selection query parameter (e.g. includeName) and updating the route/query parsing accordingly so the OpenAPI output is clearer.
| { "schema": { "type": "string" }, "required": false, "name": "name", "in": "query" }, | |
| { "schema": { "type": "string" }, "required": false, "name": "includeName", "in": "query" }, |
Lite PR
Tip: Review docs on the ENSNode PR process
Summary
scripts/generate-ensapi-openapi.tsscript that generates a static OpenAPI 3.1 JSON spec from the existinggenerateOpenApi31Document()functiongenerate:openapiscript to rootpackage.jsonfor running the generators (recursively)docs/docs.ensnode.io/ensapi-openapi.jsonand formatted with Biome{appname}-openapi.jsonnaming convention per Set foundation for multiple OpenAPI.json files #1600, ready for future specsserversarray, slight regression, but unblocks next stages to compare spec diffsWhy
pnpm openapi:generatesets foundation for multiple specs per Set foundation for multiple OpenAPI.json files #1600 (recursive lookup for each service spec commands)Testing
pnpm generate:openapi— producesdocs/docs.ensnode.io/ensapi-openapi.jsonwith Biome formattingNotes for Reviewer (Optional)
generateOpenApi31Document()which has zero runtime deps (stub handlers, no config/env)/openapi.jsonminus the localhost server entry (posted in Slack about this)Pre-Review Checklist (Blocking)