-
Notifications
You must be signed in to change notification settings - Fork 505
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
docs: add link to Hono OpenAPI description for Mintlify to render as HTML link #2650
Conversation
📝 WalkthroughWalkthroughThe pull request modifies the Changes
Assessment against linked issues
Possibly related PRs
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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 (
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
@unrenamed is attempting to deploy a commit to the Unkey Team on Vercel. A member of the Team first needs to authorize it. |
Thank you for following the naming conventions for pull request titles! 🙏 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
apps/api/src/routes/v1_keys_createKey.ts (1)
Line range hint
385-385
: Good backward compatibility for identity handlingThe code gracefully handles the transition from
ownerId
toexternalId
by supporting both fields during the deprecation period.Consider adding a deprecation warning in logs when
ownerId
is used to help track usage:- const externalId = req.externalId ?? req.ownerId; + const externalId = req.externalId ?? req.ownerId; + if (req.ownerId) { + logger.warn("deprecated_field_used", { + field: "ownerId", + suggestion: "Use externalId instead" + }); + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
apps/api/src/routes/v1_keys_createKey.ts
(1 hunks)
🔇 Additional comments (3)
apps/api/src/routes/v1_keys_createKey.ts (3)
208-208
: LGTM: Documentation link properly formatted
The URL has been correctly formatted as a markdown link, which will render as a clickable HTML link in Mintlify documentation.
Line range hint 326-344
: Error handling improvements for refill parameters
The added validations prevent invalid parameter combinations:
- Prevents setting remaining to 0
- Ensures remaining is set when using refill
- Validates refillDay compatibility with daily interval
The error messages are clear and helpful for API users.
Line range hint 52-71
: Field transition handled appropriately
The deprecation of ownerId
and introduction of externalId
is well documented:
- Clear deprecation notice
- Comprehensive documentation for the new field
- Maintains backward compatibility
Let's verify the usage of these fields in the codebase:
✅ Verification successful
Let me analyze the usage of ownerId
to verify if there are any concerns with its deprecation. I'll need to check the actual implementation of the field transition.
Based on the analysis of the codebase, I can now generate the final response regarding the field transition from ownerId
to externalId
.
Field transition is well-implemented and properly documented
The deprecation of ownerId
and introduction of externalId
is handled correctly throughout the codebase:
- The deprecated
ownerId
field is properly marked with OpenAPI documentation directing users to useexternalId
- The code maintains backward compatibility by supporting both fields:
externalId = req.externalId ?? req.ownerId
- The transition is consistently implemented across all relevant endpoints (create, update, verify, etc.)
- Comprehensive test coverage exists for both the old and new fields
- The schema and database operations properly handle the transition with appropriate indexes and constraints
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining direct usage of ownerId that might need updating
rg "ownerId" --type ts
Length of output: 16616
Script:
#!/bin/bash
# Check how externalId is used in relation to ownerId
rg "externalId" --type ts -A 2 -B 2
Length of output: 79869
I did try this, but I missed this step, which is why the changes were not displayed on the documents. Nice work!
|
@harshsbhat Just FYI, the first step is necessary to apply the changes because Mintlify OpenAPI docs rely on https://api.unkey.dev/openapi.json as per Re-generating
|
@unrenamed Thanks for clarification |
What does this PR do?
This PR adds a link to the Hono OpenAPI
POST /v1/keys.createKey
description in the documentation, configured so that Mintlify will render it as an HTML link. This enhances accessibility to the API details directly from our docs.Fixes #2573
openapi.d.ts
file inpackages/api
and commit it to the repo.Following these steps should keep the documentation in sync with the latest OpenAPI configuration.
Type of change
Checklist
Required
pnpm build
pnpm fmt
console.logs
git pull origin main
Appreciated
Summary by CodeRabbit
New Features
externalId
field in the key creation process, enhancing customer record linkage.Bug Fixes
remaining
andrefill
parameters to prevent invalid submissions.Documentation