feat(openapi): OpenAPI Reference Plugin#442
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
WalkthroughThis update introduces a new Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Server
participant OpenAPIReferencePlugin
participant OpenAPIGenerator
Client->>Server: GET /api (UI path)
Server->>OpenAPIReferencePlugin: Intercept request
OpenAPIReferencePlugin->>Server: Serve API reference HTML
Client->>Server: GET /api/spec.json
Server->>OpenAPIReferencePlugin: Intercept request
OpenAPIReferencePlugin->>OpenAPIGenerator: Generate OpenAPI spec
OpenAPIGenerator-->>OpenAPIReferencePlugin: OpenAPI JSON
OpenAPIReferencePlugin->>Server: Serve OpenAPI JSON
Poem
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Codecov ReportAttention: Patch coverage is
📢 Thoughts on this report? Let us know! |
More templates
@orpc/arktype
@orpc/client
@orpc/openapi
@orpc/contract
@orpc/openapi-client
@orpc/react
@orpc/react-query
@orpc/server
@orpc/shared
@orpc/solid-query
@orpc/standard-server
@orpc/standard-server-fetch
@orpc/standard-server-node
@orpc/svelte-query
@orpc/valibot
@orpc/vue-colada
@orpc/vue-query
@orpc/zod
commit: |
![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
🧹 Nitpick comments (11)
playgrounds/nextjs/src/app/api/[[...rest]]/route.ts (1)
16-35: Integration of OpenAPI Reference PluginThe OpenAPIReferencePlugin is now integrated into the handler, consolidating OpenAPI spec and API reference UI serving into a single plugin. The configuration includes schema conversion, API metadata, and security requirements.
This approach simplifies the codebase by removing separate routes for API docs and spec while maintaining the functionality.
Would you like me to verify that all previous API documentation routes have been properly removed from the codebase?
packages/openapi/src/plugins/openapi-reference.test.ts (2)
3-3: Consider importing from the package instead of a relative path.You're importing from
../../../zod/srcwhich crosses package boundaries. Consider importing from the published package instead (e.g.,@orpc/zod) if available, to better match how external consumers would use this code.-import { ZodToJsonSchemaConverter } from '../../../zod/src' +import { ZodToJsonSchemaConverter } from '@orpc/zod'
8-101: Consider adding tests for additional configuration options.The current tests cover the basic functionality well, but consider adding tests for custom configuration options like custom paths (
docsPath,specPath), custom titles, and custom rendering.packages/server/src/adapters/standard/plugin.ts (1)
5-7: Add documentation comment for the router parameter.The interface change to add the router parameter is good, but it would benefit from a documentation comment explaining why plugins need access to the router during initialization.
export interface StandardHandlerPlugin<T extends Context> { order?: number - init?(options: StandardHandlerOptions<T>, router: Router<any, T>): void + /** + * Initialize the plugin with handler options and router. + * @param options Handler configuration options + * @param router The router instance, allowing plugins to access router procedures and metadata + */ + init?(options: StandardHandlerOptions<T>, router: Router<any, T>): void }packages/openapi/src/openapi-generator.ts (1)
24-24: Add documentation comment for the new interface.The new interface would benefit from a documentation comment explaining its purpose and relationship to the OpenAPI Document type.
+/** + * Options for generating an OpenAPI Document, excluding the 'openapi' version field + * which is automatically set during generation. + */ export interface OpenAPIGeneratorGenerateOptions extends Partial<Omit<OpenAPI.Document, 'openapi'>> {}packages/openapi/src/plugins/openapi-reference.ts (6)
8-12: Incomplete JSDoc description for specGenerateOptionsThe JSDoc comment for
specGenerateOptionsdoesn't fully explain its purpose or potential values. Consider expanding the description to clarify what these options control and provide examples of common use cases./** * Options to pass to the OpenAPI generate. - * - */ + * These options customize the generated OpenAPI document, including servers, + * security schemes, and other OpenAPI-specific configurations. + * @example + * { + * servers: [{ url: 'https://api.example.com' }], + * security: [{ bearerAuth: [] }] + * } + */
106-106: Consider documenting the caching strategyThe spec is cached in a closure-scoped variable, which means it's generated once per server instance. This is likely intentional for performance but should be documented.
options.interceptors ??= [] + // Cache the generated spec to avoid regenerating it on every request let spec: Awaited<ReturnType<typeof this.generator.generate>>
115-119: Consider extracting path normalization logicThe path handling logic (removing trailing slashes and combining prefixes) is repeated in multiple places. Consider extracting this into a utility function for better maintainability.
+ const normalizePath = (path: string) => path.replace(/\/$/, '') || '/' + const prefix = options.prefix ?? '' - const requestPathname = options.request.url.pathname.replace(/\/$/, '') || '/' - const docsUrl = new URL(`${prefix}${this.docsPath}`.replace(/\/$/, ''), options.request.url.origin) - const specUrl = new URL(`${prefix}${this.specPath}`.replace(/\/$/, ''), options.request.url.origin) + const requestPathname = normalizePath(options.request.url.pathname) + const docsUrl = new URL(normalizePath(`${prefix}${this.docsPath}`), options.request.url.origin) + const specUrl = new URL(normalizePath(`${prefix}${this.specPath}`), options.request.url.origin)
126-134: Add cache-control headers for better performanceThe OpenAPI spec response could benefit from caching headers to improve client-side performance.
return { matched: true, response: { status: 200, - headers: {}, + headers: { + 'Cache-Control': 'public, max-age=3600', + 'Content-Type': 'application/json', + }, body: new File([stringifyJSON(spec)], 'spec.json', { type: 'application/json' }), }, }
146-152: Add cache-control headers for HTML responseSimilar to the OpenAPI spec response, the HTML response could benefit from caching headers.
return { matched: true, response: { status: 200, - headers: {}, + headers: { + 'Cache-Control': 'public, max-age=3600', + 'Content-Type': 'text/html', + }, body: new File([html], 'api-reference.html', { type: 'text/html' }), }, }
104-157: Consider adding validation for path conflictsThe plugin doesn't validate that
specPathanddocsPathare different, which could lead to undefined behavior if they're set to the same value.constructor(options: OpenAPIReferencePluginOptions = {}) { this.specGenerateOptions = options.specGenerateOptions this.docsPath = options.docsPath ?? '/' this.docsTitle = options.docsTitle ?? 'API Reference' this.docsConfig = options.docsConfig ?? {} this.docsScriptUrl = options.docsScriptUrl ?? 'https://cdn.jsdelivr.net/npm/@scalar/api-reference' this.docsHead = options.docsHead ?? '' this.specPath = options.specPath ?? '/spec.json' + + // Validate that paths don't conflict + if (this.specPath === this.docsPath) { + console.warn('OpenAPIReferencePlugin: specPath and docsPath are the same, this may cause unexpected behavior') + } this.generator = new OpenAPIGenerator(options)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (30)
apps/content/.vitepress/config.ts(1 hunks)apps/content/docs/openapi/plugins/openapi-reference.md(1 hunks)apps/content/docs/openapi/scalar.md(1 hunks)packages/openapi/package.json(2 hunks)packages/openapi/src/openapi-generator.ts(2 hunks)packages/openapi/src/plugins/index.ts(1 hunks)packages/openapi/src/plugins/openapi-reference.test.ts(1 hunks)packages/openapi/src/plugins/openapi-reference.ts(1 hunks)packages/server/src/adapters/standard/handler.test.ts(1 hunks)packages/server/src/adapters/standard/handler.ts(1 hunks)packages/server/src/adapters/standard/plugin.test.ts(1 hunks)packages/server/src/adapters/standard/plugin.ts(2 hunks)playgrounds/contract-first/src/main.ts(3 hunks)playgrounds/nextjs/src/app/actions.ts(1 hunks)playgrounds/nextjs/src/app/api/[[...rest]]/route.ts(2 hunks)playgrounds/nextjs/src/app/page.tsx(1 hunks)playgrounds/nextjs/src/app/scalar/route.ts(0 hunks)playgrounds/nextjs/src/app/spec/route.ts(0 hunks)playgrounds/nuxt/app.vue(1 hunks)playgrounds/nuxt/server/routes/api/[...].ts(2 hunks)playgrounds/nuxt/server/routes/scalar.ts(0 hunks)playgrounds/nuxt/server/routes/spec.ts(0 hunks)playgrounds/solid-start/src/routes/api/[...rest].ts(2 hunks)playgrounds/solid-start/src/routes/index.tsx(1 hunks)playgrounds/solid-start/src/routes/scalar.ts(0 hunks)playgrounds/solid-start/src/routes/spec.ts(0 hunks)playgrounds/svelte-kit/src/routes/+page.svelte(1 hunks)playgrounds/svelte-kit/src/routes/api/[...rest]/+server.ts(2 hunks)playgrounds/svelte-kit/src/routes/scalar/+server.ts(0 hunks)playgrounds/svelte-kit/src/routes/spec/+server.ts(0 hunks)
💤 Files with no reviewable changes (8)
- playgrounds/solid-start/src/routes/spec.ts
- playgrounds/svelte-kit/src/routes/spec/+server.ts
- playgrounds/nextjs/src/app/spec/route.ts
- playgrounds/solid-start/src/routes/scalar.ts
- playgrounds/nuxt/server/routes/scalar.ts
- playgrounds/nuxt/server/routes/spec.ts
- playgrounds/svelte-kit/src/routes/scalar/+server.ts
- playgrounds/nextjs/src/app/scalar/route.ts
🧰 Additional context used
🧬 Code Graph Analysis (7)
playgrounds/nextjs/src/app/api/[[...rest]]/route.ts (2)
packages/openapi/src/plugins/openapi-reference.ts (1)
OpenAPIReferencePlugin(60-158)packages/zod/src/converter.ts (1)
ZodToJsonSchemaConverter(66-642)
playgrounds/nuxt/server/routes/api/[...].ts (2)
packages/openapi/src/plugins/openapi-reference.ts (1)
OpenAPIReferencePlugin(60-158)packages/zod/src/converter.ts (1)
ZodToJsonSchemaConverter(66-642)
packages/openapi/src/plugins/openapi-reference.test.ts (4)
packages/zod/src/converter.ts (1)
ZodToJsonSchemaConverter(66-642)packages/openapi/src/openapi-generator.ts (1)
OpenAPIGenerator(31-339)packages/server/src/builder.ts (2)
os(336-352)handler(273-280)packages/openapi/src/plugins/openapi-reference.ts (1)
OpenAPIReferencePlugin(60-158)
packages/openapi/src/openapi-generator.ts (3)
packages/contract/src/router.ts (1)
AnyContractRouter(17-17)packages/server/src/router.ts (1)
AnyRouter(19-19)packages/shared/src/object.ts (1)
clone(48-64)
playgrounds/svelte-kit/src/routes/api/[...rest]/+server.ts (2)
packages/openapi/src/plugins/openapi-reference.ts (1)
OpenAPIReferencePlugin(60-158)packages/zod/src/converter.ts (1)
ZodToJsonSchemaConverter(66-642)
packages/server/src/adapters/standard/plugin.ts (3)
packages/server/src/context.ts (1)
Context(1-1)packages/server/src/adapters/standard/handler.ts (1)
StandardHandlerOptions(26-48)packages/server/src/router.ts (1)
Router(12-17)
packages/openapi/src/plugins/openapi-reference.ts (5)
packages/openapi/src/openapi-generator.ts (3)
OpenAPIGeneratorOptions(20-22)OpenAPIGeneratorGenerateOptions(24-24)OpenAPIGenerator(31-339)packages/server/src/context.ts (1)
Context(1-1)packages/server/src/adapters/standard/plugin.ts (1)
StandardHandlerPlugin(5-8)packages/shared/src/json.ts (1)
stringifyJSON(9-12)packages/server/src/adapters/standard/handler.ts (1)
StandardHandlerOptions(26-48)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: publish-commit
🔇 Additional comments (38)
playgrounds/solid-start/src/routes/index.tsx (1)
11-11: Update link to consolidated API reference route
The anchor now correctly targets/apiinstead of/scalar, matching the new plugin‐based routing for OpenAPI docs.playgrounds/svelte-kit/src/routes/+page.svelte (1)
15-15: Update link to consolidated API reference route
The hyperlink now points to/apiin line with the unified OpenAPIReferencePlugin endpoint.playgrounds/nuxt/app.vue (1)
7-7: Update link to consolidated API reference route
The<a>tag now directs to/api, aligning with the new plugin architecture for serving API docs and spec.playgrounds/nextjs/src/app/page.tsx (1)
15-15: Update button text to reflect new API reference route
The button label now reads “Redirect to Scalar API Reference” and should correspond to the updated/apitarget.apps/content/.vitepress/config.ts (1)
187-187:Details
✅ Verification successful
Verify the documentation page exists
Ensure that/docs/openapi/plugins/openapi-referenceactually resolves to an existing markdown file to avoid a broken sidebar link.Run this script from the repo root to confirm:
🏁 Script executed:
#!/bin/bash DOC="apps/content/docs/openapi/plugins/openapi-reference.md" if [ ! -f "$DOC" ]; then echo "✖ Missing documentation file: $DOC" exit 1 fi echo "✔ Documentation file found: $DOC"Length of output: 307
✔ Documentation page exists
The fileapps/content/docs/openapi/plugins/openapi-reference.mdwas found, so the sidebar link/docs/openapi/plugins/openapi-referenceis valid. No further action required.packages/server/src/adapters/standard/handler.test.ts (1)
339-340: Test correctly updated to match new plugin initialization signature.The test now correctly verifies that the plugin's
initmethod is called with both theoptionsobject and therouterinstance, which aligns with the updated plugin interface. This change is necessary to support the new OpenAPI Reference Plugin that needs router access during initialization.apps/content/docs/openapi/scalar.md (1)
10-12: Good documentation update that guides users to the new plugin.This informational note provides clear guidance to users about the existence of a simpler setup option using the new OpenAPI Reference Plugin. The note is appropriately placed near the beginning of the document and includes a helpful link to the plugin's documentation.
playgrounds/nextjs/src/app/actions.ts (1)
34-34: Correct redirection target update.The redirection target has been properly updated from
/scalarto/apito align with the new architecture where the OpenAPI Reference Plugin serves both the API reference UI and OpenAPI specification under the main API route instead of a separate/scalarroute.packages/openapi/src/plugins/index.ts (1)
1-2: Good plugin export pattern.Correctly exports all items from the OpenAPI Reference Plugin module, making them accessible through the plugins index file. This follows standard module export patterns and ensures the plugin is easily importable by consumers.
packages/server/src/adapters/standard/handler.ts (1)
63-63: Enhanced plugin initialization with router accessThe plugin initialization is now provided with both the options and router parameters, allowing plugins to interact with the router during initialization. This is a positive enhancement that enables more powerful plugin capabilities like route registration.
packages/server/src/adapters/standard/plugin.test.ts (3)
23-23: LGTM - Mock router object addedThe test now includes a mock router object that will be passed to the plugin initialization.
25-25: LGTM - Router parameter added to init callCorrectly updated to pass the router to the composite plugin's init method, matching the implementation changes.
31-33: LGTM - Test assertions updatedTest assertions now verify that each plugin's init method is called with both options and router parameters, ensuring the correct behavior.
playgrounds/nextjs/src/app/api/[[...rest]]/route.ts (1)
4-5: LGTM - Added necessary importsAdded imports for ZodToJsonSchemaConverter and OpenAPIReferencePlugin to support the new OpenAPI functionality.
playgrounds/solid-start/src/routes/api/[...rest].ts (2)
4-7: LGTM - Added necessary importsAdded imports for ZodToJsonSchemaConverter and OpenAPIReferencePlugin to support the OpenAPI functionality, along with the required polyfill import.
17-36:Details
✅ Verification successful
Integration of OpenAPI Reference Plugin
The OpenAPIReferencePlugin is properly integrated with the same configuration pattern as in the NextJS playground. This provides a consistent approach across different frameworks while simplifying the codebase.
🏁 Script executed:
#!/bin/bash # Check if the old scalar and spec routes have been removed echo "Checking for removed scalar and spec routes..." find playgrounds/solid-start/src/routes -name "scalar.ts" -o -name "spec.ts" # Verify that the OpenAPIReferencePlugin is properly imported in all playground implementations echo "Checking OpenAPIReferencePlugin usage across playgrounds..." grep -r "OpenAPIReferencePlugin" --include="*.ts" playgrounds/Length of output: 1341
OpenAPIReferencePlugin integration verified across all playgrounds
- Removed legacy
scalar.tsandspec.tsroutes fromplaygrounds/solid-start/src/routes- Confirmed
OpenAPIReferencePluginis imported and configured consistently in:
playgrounds/contract-first/src/main.tsplaygrounds/nextjs/src/app/api/[[...rest]]/route.tsplaygrounds/nuxt/server/routes/api/[...].tsplaygrounds/solid-start/src/routes/api/[...rest].tsplaygrounds/svelte-kit/src/routes/api/[...rest]/+server.ts- Configuration pattern matches NextJS implementation, ensuring uniformity
playgrounds/nuxt/server/routes/api/[...].ts (2)
3-5: Clean imports added for the OpenAPI Reference PluginThe imports for
ZodToJsonSchemaConverterandOpenAPIReferencePluginare correctly added to enable the OpenAPI documentation functionality.
15-34: Well-structured OpenAPI Reference Plugin configurationThe plugin is properly configured with schema conversion and comprehensive OpenAPI metadata. The security scheme configuration follows best practices by defining both the security requirements and the corresponding security scheme definition.
This implementation effectively centralizes both the OpenAPI spec JSON and API reference UI serving, eliminating the need for separate route handlers.
playgrounds/svelte-kit/src/routes/api/[...rest]/+server.ts (2)
5-6: Appropriate imports for OpenAPI functionalityThe imports for
ZodToJsonSchemaConverterandOpenAPIReferencePluginare correctly added to enable OpenAPI documentation functionality.
17-36: Consistent OpenAPI Reference Plugin configurationThe configuration mirrors the pattern used in other playgrounds, maintaining consistency across the codebase. The security configuration is properly structured with both security requirements and scheme definitions.
This implementation follows the project-wide pattern of centralizing OpenAPI documentation within the API handler.
apps/content/docs/openapi/plugins/openapi-reference.md (3)
1-12: Clear introduction and context for the OpenAPI Reference PluginThe documentation provides a concise introduction to the plugin's purpose and its dependency on the OpenAPI Generator. The information notice correctly directs users to review prerequisite documentation.
14-35: Comprehensive setup example with key configuration optionsThe code example effectively demonstrates how to integrate the plugin with proper schema conversion and OpenAPI metadata configuration. This provides users with a practical reference for implementation.
37-39: Helpful information about default paths and customization optionsThe documentation clearly explains the default serving paths and mentions how they can be customized, which is essential information for users who may need to adapt the plugin to their specific routing requirements.
playgrounds/contract-first/src/main.ts (4)
7-7: Consistent import of the OpenAPI Reference PluginThe import follows the same pattern used in other playground implementations, maintaining consistency across the codebase.
18-37: Standard OpenAPI Reference Plugin configurationThe configuration mirrors the pattern used in other playgrounds, ensuring consistent behavior across different environments. The security configuration is properly structured with both security requirements and scheme definitions.
72-73: Simplified error handling for unmatched routesThe code now returns a simple 404 response for unmatched routes, replacing the previous manual OpenAPI spec and UI serving logic. This is more maintainable as the OpenAPI functionality is now handled by the plugin.
77-77: Updated console log with the correct API endpointThe console log now correctly directs users to the
/apiendpoint where the OpenAPI Reference UI is served by the plugin.packages/openapi/package.json (2)
23-27: LGTM! Clean export configuration for the new plugins module.The addition of the subpath export for
./pluginsis properly configured with the correct types and module paths, following the same pattern as other exports in the package.
47-47: LGTM! Consistent source file export.The source file export for plugins correctly points to the source directory entry point.
packages/openapi/src/plugins/openapi-reference.test.ts (3)
8-42: LGTM! Comprehensive test for basic functionality.The test verifies that the plugin correctly serves both the API reference docs and OpenAPI spec JSON at the expected endpoints with the right content types and data. It also checks that unmatched paths are handled correctly.
44-77: LGTM! Good test coverage for URL prefix handling.This test ensures the plugin correctly handles URL prefixes, serving both docs and spec at the prefixed paths and adjusting server URLs in the OpenAPI spec accordingly.
79-101: LGTM! Important edge case covered for router precedence.This test verifies that router procedures take precedence over the plugin's endpoints when they match the same paths. This is essential for avoiding conflicts with existing API endpoints.
packages/server/src/adapters/standard/plugin.ts (2)
2-2: LGTM! Adding the needed Router type import.The import of the Router type is necessary to support the updated plugin interface.
17-20: LGTM! Updated CompositeStandardHandlerPlugin implementation.The CompositeStandardHandlerPlugin implementation correctly passes the router parameter to each plugin's init method.
packages/openapi/src/openapi-generator.ts (2)
45-50: LGTM! Improved generate method signature with sensible defaults.The updated method signature now accepts options instead of a base document, with improved defaults for the info object and explicit OpenAPI version setting. This makes the API more flexible and easier to use.
45-50:Details
❓ Verification inconclusive
Verify backward compatibility with existing code.
This change to the method signature might affect existing code that extends or directly uses this class. Consider documenting this change in the changelog or migration guide if it's a breaking change.
🏁 Script executed:
#!/bin/bash # Check for external usages of the generate method with the old signature rg -A 3 "openApiGenerator.generate\\(" --type=ts --type=jsLength of output: 59
Check for external impact of the
generatesignature changeI ran a global search for
openApiGenerator.generate(across all TS/JS files and found no internal callsites. Since this is part of the public API, please:
- Manually verify whether any downstream or external consumers rely on the previous
generatesignature.- If it’s a breaking change, document it in the changelog or migration guide.
Locations to review:
- packages/openapi/src/openapi-generator.ts (lines 45–50)packages/openapi/src/plugins/openapi-reference.ts (2)
81-83: Good security practice with string escaping functionThe implementation of an escaping function to prevent XSS vulnerabilities is a good practice.
1-158: Overall well-structured plugin implementationThe OpenAPIReferencePlugin is well-designed with good separation of concerns and a flexible configuration interface. It effectively leverages the oRPC interceptor pattern to integrate OpenAPI documentation into the server.
A few minor suggestions:
- Consider adding typings for the
docsConfigobject to provide better developer experience- Add more comprehensive error handling throughout the implementation
- Document the caching behavior of the spec generation
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
packages/openapi/src/plugins/openapi-reference.ts (2)
97-97: Security vulnerability: unescaped HTML injectionThe
headcontent is inserted directly into the HTML template without escaping, which could lead to XSS vulnerabilities if user-controlled content is passed indocsHead.- ${head} + ${esc(head)}Alternatively, if you intentionally want to support HTML injection for flexibility (e.g., to allow adding custom stylesheets), document this clearly as a security consideration in JSDoc comments.
128-131: Add error handling for OpenAPI generationThere's no error handling for potential failures during OpenAPI spec generation. If the generator encounters an error, it will propagate to the caller and might result in an unhandled rejection.
- spec ??= await this.generator.generate(router, { - servers: [{ url: new URL(prefix, options.request.url.origin).toString() }], - ...await value(this.specGenerateOptions, options), - }) + try { + spec ??= await this.generator.generate(router, { + servers: [{ url: new URL(prefix, options.request.url.origin).toString() }], + ...await value(this.specGenerateOptions, options), + }) + } catch (error) { + console.error('Failed to generate OpenAPI specification:', error) + return { + matched: true, + response: { + status: 500, + headers: { 'Content-Type': 'application/json' }, + body: new File([JSON.stringify({ error: 'Failed to generate OpenAPI specification' })], + 'error.json', { type: 'application/json' }), + }, + } + }
🧹 Nitpick comments (7)
packages/openapi/src/plugins/openapi-reference.ts (7)
136-139: Set explicit Content-Type headerWhile the File constructor sets the MIME type, it's better to explicitly set the Content-Type header for clarity and to ensure proper handling by all clients.
response: { status: 200, - headers: {}, + headers: { 'Content-Type': 'application/json' }, body: new File([stringifyJSON(spec)], 'spec.json', { type: 'application/json' }), },
155-158: Set explicit Content-Type headerSimilarly, set the Content-Type header for the HTML response.
response: { status: 200, - headers: {}, + headers: { 'Content-Type': 'text/html' }, body: new File([html], 'api-reference.html', { type: 'text/html' }), },
8-65: Consider adding usage example in JSDocThe plugin options are well-documented, but a brief example of how to use this plugin in a router setup would be helpful for developers. Consider adding a JSDoc example at the interface or class level.
/** * Plugin for serving OpenAPI specification and API reference UI. * * @example * ```ts * // Example usage in a server setup * import { OpenAPIReferencePlugin } from '@orpc/openapi' * * const server = createServer({ * plugins: [ * new OpenAPIReferencePlugin({ * specPath: '/api/spec.json', * docsPath: '/api/docs', * docsTitle: 'My API Documentation' * }) * ] * }) * ``` */
13-13: Complete the JSDoc commentThe JSDoc comment for
specGenerateOptionsis incomplete. Add a descriptive sentence explaining what this option does./** - * Options to pass to the OpenAPI generate. + * Options to pass to the OpenAPI generator when generating the specification. + * Use this to customize the generated OpenAPI schema. * */
122-126: Handle URLs with trailing slashes consistentlyThe code removes trailing slashes from paths but doesn't handle the case where a user might access the docs or spec with a trailing slash. Consider normalizing the request path as well.
const prefix = options.prefix ?? '' - const requestPathname = options.request.url.pathname.replace(/\/$/, '') || '/' + const requestPathname = options.request.url.pathname + const normalizedRequestPath = requestPathname.replace(/\/$/, '') || '/' const docsUrl = new URL(`${prefix}${this.docsPath}`.replace(/\/$/, ''), options.request.url.origin) const specUrl = new URL(`${prefix}${this.specPath}`.replace(/\/$/, ''), options.request.url.origin) - if (requestPathname === specUrl.pathname) { + if (normalizedRequestPath === specUrl.pathname) {This ensures users can access both
/spec.jsonand/spec.json/(with trailing slash).
143-143: Use normalized path here tooUpdate this condition for consistency with the change suggested above.
- if (requestPathname === docsUrl.pathname) { + if (normalizedRequestPath === docsUrl.pathname) {
113-113: Consider memoizing the generated specThe current implementation caches the spec only for the duration of a single request. For better performance, consider memoizing the spec at the class level so it persists across requests.
- let spec: Awaited<ReturnType<typeof this.generator.generate>> + // Cache the generated spec at the class level + if (!this.cachedSpec) { + this.cachedSpec = new Map<string, Awaited<ReturnType<typeof this.generator.generate>>>() + }And update the spec generation logic:
- spec ??= await this.generator.generate(router, { - servers: [{ url: new URL(prefix, options.request.url.origin).toString() }], - ...await value(this.specGenerateOptions, options), - }) + const cacheKey = `${prefix}-${options.request.url.origin}` + if (!this.cachedSpec.has(cacheKey)) { + try { + const generatedSpec = await this.generator.generate(router, { + servers: [{ url: new URL(prefix, options.request.url.origin).toString() }], + ...await value(this.specGenerateOptions, options), + }) + this.cachedSpec.set(cacheKey, generatedSpec) + } catch (error) { + console.error('Failed to generate OpenAPI specification:', error) + return { + matched: true, + response: { + status: 500, + headers: { 'Content-Type': 'application/json' }, + body: new File([JSON.stringify({ error: 'Failed to generate OpenAPI specification' })], + 'error.json', { type: 'application/json' }), + }, + } + } + } + + const spec = this.cachedSpec.get(cacheKey)!Don't forget to add the class property:
export class OpenAPIReferencePlugin<T extends Context> implements StandardHandlerPlugin<T> { private readonly generator: OpenAPIGenerator private readonly specGenerateOptions: OpenAPIReferencePluginOptions<T>['specGenerateOptions'] private readonly specPath: Exclude<OpenAPIReferencePluginOptions<T>['specPath'], undefined> private readonly docsPath: Exclude<OpenAPIReferencePluginOptions<T>['docsPath'], undefined> private readonly docsTitle: Exclude<OpenAPIReferencePluginOptions<T>['docsTitle'], undefined> private readonly docsHead: Exclude<OpenAPIReferencePluginOptions<T>['docsHead'], undefined> private readonly docsScriptUrl: Exclude<OpenAPIReferencePluginOptions<T>['docsScriptUrl'], undefined> private readonly docsConfig: OpenAPIReferencePluginOptions<T>['docsConfig'] private readonly renderDocsHtml: Exclude<OpenAPIReferencePluginOptions<T>['renderDocsHtml'], undefined> + private cachedSpec?: Map<string, Awaited<ReturnType<typeof OpenAPIGenerator.prototype.generate>>>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/openapi/src/plugins/openapi-reference.test.ts(1 hunks)packages/openapi/src/plugins/openapi-reference.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/openapi/src/plugins/openapi-reference.test.ts
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
packages/openapi/src/plugins/openapi-reference.ts (2)
97-97:⚠️ Potential issueSecurity vulnerability: unescaped HTML injection
The
headcontent is inserted directly into the HTML template without escaping, which could lead to XSS vulnerabilities if user-controlled content is passed indocsHead.- ${head} + ${esc(head)}Alternatively, if you intentionally want to support HTML injection for flexibility (e.g., to allow adding custom stylesheets), document this clearly as a security consideration in JSDoc comments.
126-140: 🛠️ Refactor suggestionAdd error handling for OpenAPI generation
There's no error handling for potential failures during OpenAPI spec generation. If the generator throws an exception, it will propagate to the caller and might result in an unhandled rejection.
- if (requestPathname === specUrl.pathname) { - const spec = await this.generator.generate(router, { - servers: [{ url: new URL(prefix, options.request.url.origin).toString() }], - ...await value(this.specGenerateOptions, options), - }) + if (requestPathname === specUrl.pathname) { + try { + const spec = await this.generator.generate(router, { + servers: [{ url: new URL(prefix, options.request.url.origin).toString() }], + ...await value(this.specGenerateOptions, options), + }) + + return { + matched: true, + response: { + status: 200, + headers: {}, + body: new File([stringifyJSON(spec)], 'spec.json', { type: 'application/json' }), + }, + } + } catch (error) { + console.error('Failed to generate OpenAPI specification:', error) + return { + matched: true, + response: { + status: 500, + headers: {}, + body: new File([JSON.stringify({ error: 'Failed to generate OpenAPI specification' })], + 'error.json', { type: 'application/json' }), + }, + } + } - - return { - matched: true, - response: { - status: 200, - headers: {}, - body: new File([stringifyJSON(spec)], 'spec.json', { type: 'application/json' }), - }, - } }
🧹 Nitpick comments (4)
packages/openapi/src/plugins/openapi-reference.ts (4)
126-130: Consider caching the generated OpenAPI specThe OpenAPI specification is regenerated on every request, which could be inefficient for frequently accessed endpoints. Consider adding a caching mechanism to reuse the generated spec.
+ private specCache: Record<string, any> = {} + if (requestPathname === specUrl.pathname) { + const cacheKey = `${prefix}-${JSON.stringify(await value(this.specGenerateOptions, options))}` + if (this.specCache[cacheKey]) { + return { + matched: true, + response: { + status: 200, + headers: {}, + body: new File([stringifyJSON(this.specCache[cacheKey])], 'spec.json', { type: 'application/json' }), + }, + } + } + const spec = await this.generator.generate(router, { servers: [{ url: new URL(prefix, options.request.url.origin).toString() }], ...await value(this.specGenerateOptions, options), }) + this.specCache[cacheKey] = spec
111-111: Consider using a more specific type thananyfor routerThe router parameter in the
initmethod usesRouter<any, T>, which could be more type-safe.- init(options: StandardHandlerOptions<T>, router: Router<any, T>): void { + init(options: StandardHandlerOptions<T>, router: Router<unknown, T>): void {
82-83: Consider adding validation for docsConfigIf
docsConfigis provided but isn't JSON-serializable, it will cause errors when used in the HTML template. Consider adding validation or documentation about this constraint.Add a comment in the JSDoc for the docsConfig option:
/** * Arbitrary configuration object for the UI. + * This object must be JSON-serializable as it will be stringified when injected into the HTML. */ docsConfig?: Value<object, [StandardHandlerInterceptorOptions<T>]>
88-108: Consider extracting the default HTML template to a separate functionThe default HTML template is defined as an inline template string in the constructor, which makes it harder to read and maintain. Consider extracting it to a separate function or constant.
+ private static defaultRenderDocsHtml( + specUrl: string, + title: string, + head: string, + scriptUrl: string, + config: object | undefined, + esc: (s: string) => string + ): string { + return ` + <!doctype html> + <html> + <head> + <meta charset="utf-8" /> + <meta name="viewport" content="width=device-width, initial-scale=1" /> + <title>${esc(title)}</title> + ${esc(head)} + </head> + <body> + <script + id="api-reference" + data-url="${esc(specUrl)}" + ${config !== undefined ? `data-configuration="${esc(stringifyJSON(config))}"` : ''} + ></script> + <script src="${esc(scriptUrl)}"></script> + </body> + </html> + ` + } constructor(options: OpenAPIReferencePluginOptions<T> = {}) { this.specGenerateOptions = options.specGenerateOptions this.docsPath = options.docsPath ?? '/' this.docsTitle = options.docsTitle ?? 'API Reference' this.docsConfig = options.docsConfig ?? undefined this.docsScriptUrl = options.docsScriptUrl ?? 'https://cdn.jsdelivr.net/npm/@scalar/api-reference' this.docsHead = options.docsHead ?? '' this.specPath = options.specPath ?? '/spec.json' this.generator = new OpenAPIGenerator(options) const esc = (s: string) => s.replace(/&/g, '&').replace(/"/g, '"').replace(/</g, '<').replace(/>/g, '>') - this.renderDocsHtml = options.renderDocsHtml ?? ((specUrl, title, head, scriptUrl, config) => ` - <!doctype html> - <html> - <head> - <meta charset="utf-8" /> - <meta name="viewport" content="width=device-width, initial-scale=1" /> - <title>${esc(title)}</title> - ${head} - </head> - <body> - <script - id="api-reference" - data-url="${esc(specUrl)}" - ${config !== undefined ? `data-configuration="${esc(stringifyJSON(config))}"` : ''} - ></script> - <script src="${esc(scriptUrl)}"></script> - </body> - </html> - `) + this.renderDocsHtml = options.renderDocsHtml ?? ((specUrl, title, head, scriptUrl, config) => + OpenAPIReferencePlugin.defaultRenderDocsHtml(specUrl, title, head, scriptUrl, config, esc) + ) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/openapi/src/plugins/openapi-reference.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: publish-commit
🔇 Additional comments (3)
packages/openapi/src/plugins/openapi-reference.ts (3)
67-77: Good use of private fields to implement the pluginThe implementation properly encapsulates the plugin configuration using private fields, making the code more maintainable and following good OOP principles.
1-6: Imports are well-organized and typed correctlyThe imports are well-organized, with types imported separately from implementation imports, making the code more maintainable.
8-65: Well-documented interface with comprehensive optionsThe interface is well-documented with JSDoc comments that clearly explain each option and provide default values where applicable.
| const requestPathname = options.request.url.pathname.replace(/\/$/, '') || '/' | ||
| const docsUrl = new URL(`${prefix}${this.docsPath}`.replace(/\/$/, ''), options.request.url.origin) | ||
| const specUrl = new URL(`${prefix}${this.specPath}`.replace(/\/$/, ''), options.request.url.origin) | ||
|
|
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Consider adding unit tests for URL path normalization
The code includes logic to handle trailing slashes in URL paths, but it's not clear if this has been tested with all possible combinations of input paths. Make sure you have comprehensive unit tests for this logic.
Run the following shell script to search for relevant test cases:
🏁 Script executed:
#!/bin/bash
# Search for test files related to OpenAPIReferencePlugin and path normalization
echo "Searching for OpenAPI reference plugin tests..."
find packages/openapi -name "*.test.ts" -o -name "*.spec.ts" | grep -i openapi | xargs grep -l "Reference" || echo "No test files found"
# Look for tests that verify URL path normalization
find packages/openapi -name "*.test.ts" -o -name "*.spec.ts" | xargs grep -l "path.*trail\|trail.*slash\|normalize.*path" || echo "No path normalization tests found"Length of output: 467
Add Missing Unit Tests for URL Path Normalization
The openapi-reference plugin strips trailing slashes in several places—requestPathname, docsUrl, and specUrl—but there are no existing tests covering these behaviors. Please add unit tests in packages/openapi/src/plugins/openapi-reference.test.ts to verify:
- Normalization of incoming request URLs:
- Paths with and without trailing slashes (e.g.
/foo,/foo/) - Root path cases (
/and empty)
- Paths with and without trailing slashes (e.g.
- Construction of
docsUrlandspecUrl:this.docsPathandthis.specPathboth with and without leading/trailing slashes- Prefixed routes (e.g. when
prefixis non‑empty) - Combination of
prefix,docsPath/specPath, and origin
Example test cases to add:
- request URL
/openapi/→requestPathnameshould be/openapi prefix = '/api',docsPath = '/docs/'→docsUrl.pathnameshould be/api/docsprefix = '',specPath = 'spec/'→specUrl.pathnameshould be/spec
Address these gaps to ensure robust handling of all trailing‑slash scenarios.
| if (requestPathname === docsUrl.pathname) { | ||
| const html = this.renderDocsHtml( | ||
| specUrl.toString(), | ||
| await value(this.docsTitle, options), | ||
| await value(this.docsHead, options), | ||
| await value(this.docsScriptUrl, options), | ||
| await value(this.docsConfig, options), | ||
| ) | ||
|
|
||
| return { | ||
| matched: true, | ||
| response: { | ||
| status: 200, | ||
| headers: {}, | ||
| body: new File([html], 'api-reference.html', { type: 'text/html' }), | ||
| }, | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for HTML rendering
Similar to the OpenAPI generation, there's no error handling for the HTML rendering process. If any of the value() calls or the renderDocsHtml function throws an exception, it would result in an unhandled promise rejection.
if (requestPathname === docsUrl.pathname) {
+ try {
const html = this.renderDocsHtml(
specUrl.toString(),
await value(this.docsTitle, options),
await value(this.docsHead, options),
await value(this.docsScriptUrl, options),
await value(this.docsConfig, options),
)
return {
matched: true,
response: {
status: 200,
headers: {},
body: new File([html], 'api-reference.html', { type: 'text/html' }),
},
}
+ } catch (error) {
+ console.error('Failed to render API reference HTML:', error)
+ return {
+ matched: true,
+ response: {
+ status: 500,
+ headers: {},
+ body: new File([JSON.stringify({ error: 'Failed to render API reference HTML' })],
+ 'error.json', { type: 'application/json' }),
+ },
+ }
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (requestPathname === docsUrl.pathname) { | |
| const html = this.renderDocsHtml( | |
| specUrl.toString(), | |
| await value(this.docsTitle, options), | |
| await value(this.docsHead, options), | |
| await value(this.docsScriptUrl, options), | |
| await value(this.docsConfig, options), | |
| ) | |
| return { | |
| matched: true, | |
| response: { | |
| status: 200, | |
| headers: {}, | |
| body: new File([html], 'api-reference.html', { type: 'text/html' }), | |
| }, | |
| } | |
| } | |
| if (requestPathname === docsUrl.pathname) { | |
| try { | |
| const html = this.renderDocsHtml( | |
| specUrl.toString(), | |
| await value(this.docsTitle, options), | |
| await value(this.docsHead, options), | |
| await value(this.docsScriptUrl, options), | |
| await value(this.docsConfig, options), | |
| ) | |
| return { | |
| matched: true, | |
| response: { | |
| status: 200, | |
| headers: {}, | |
| body: new File([html], 'api-reference.html', { type: 'text/html' }), | |
| }, | |
| } | |
| } catch (error) { | |
| console.error('Failed to render API reference HTML:', error) | |
| return { | |
| matched: true, | |
| response: { | |
| status: 500, | |
| headers: {}, | |
| body: new File( | |
| [JSON.stringify({ error: 'Failed to render API reference HTML' })], | |
| 'error.json', | |
| { type: 'application/json' } | |
| ), | |
| }, | |
| } | |
| } | |
| } |
1 participant
Summary by CodeRabbit
New Features
Improvements
/scalarand/specendpoints.Bug Fixes
Documentation
Chores