Conversation
chronark
requested review from
MichaelUnkey,
mcstepp,
ogzhanolguncu and
perkinsjr
as code owners
|
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
1 Skipped Deployment
|
📝 WalkthroughWalkthroughThe changes add a new test case to the key verification API, ensuring that a valid request returns a defined Changes
Possibly related PRs
Suggested labels
Suggested reviewers
✨ Finishing Touches
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 (
|
|
Thank you for following the naming conventions for pull request titles! 🙏 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
apps/api/src/routes/v1_migrations_createKey.happy.test.ts (1)
578-591: 💡 Verification agent🧩 Analysis chain
Consider adding a test for the new requestId feature
According to the PR summary, you're adding requestId to the verify key API. While this test verifies the basic functionality works, consider adding an explicit assertion that checks if the response contains a defined requestId.
expect(verifyRes.status).toBe(200); expect(verifyRes.body.valid).toEqual(true); + expect(verifyRes.body.requestId).toBeDefined();Let's check what's in the response schema:
🏁 Script executed:
#!/bin/bash # Search for the V1KeysVerifyKeyResponse type definition rg -A 15 "V1KeysVerifyKeyResponse" --type=typescriptLength of output: 87
I'll re-run the code search using the correct file extension. This should help us verify whether the
V1KeysVerifyKeyResponsetype includes the newrequestIdmember.
🏁 Script executed:
#!/bin/bash rg -A 15 "V1KeysVerifyKeyResponse" --type tsLength of output: 76041
Action: Add an explicit assertion for requestId in the migration key test
The test in
apps/api/src/routes/v1_migrations_createKey.happy.test.tscurrently verifies that the key is valid, but it does not check for the newrequestIdfield added to the verify key API. We already see that other tests (e.g., inapps/api/src/routes/v1_keys_verifyKey.test.ts) assert thatrequestIdis defined. To ensure consistency across tests and to validate the new feature, please add an assertion such as:expect(verifyRes.status).toBe(200); expect(verifyRes.body.valid).toEqual(true); + expect(verifyRes.body.requestId).toBeDefined();This additional check will confirm that the
requestIdis correctly returned as part of the response.
🧹 Nitpick comments (1)
apps/api/src/routes/v1_migrations_createKey.happy.test.ts (1)
295-296: Consider using the imported randomUUID functionYou're importing
randomUUIDfrom 'node:crypto' on line 3 but usingcrypto.randomUUID()here. For consistency, consider using the imported function instead.-const plaintext = crypto.randomUUID(); +const plaintext = randomUUID();
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/api/src/routes/v1_keys_updateKey.error.test.ts(2 hunks)apps/api/src/routes/v1_migrations_createKey.happy.test.ts(2 hunks)
🧰 Additional context used
🧠 Learnings (1)
apps/api/src/routes/v1_keys_updateKey.error.test.ts (1)
Learnt from: MichaelUnkey
PR: unkeyed/unkey#2114
File: apps/api/src/routes/v1_keys_updateKey.error.test.ts:0-0
Timestamp: 2024-11-10T16:45:16.994Z
Learning: In the `v1/keys.updateKey` endpoint, the server validates the refill configuration before checking if the key exists. Therefore, tests can assert validation errors without needing to create the key first.
⏰ Context from checks skipped due to timeout of 90000ms (18)
- GitHub Check: Test Packages / Test ./packages/rbac
- GitHub Check: Test Packages / Test ./packages/nextjs
- GitHub Check: Test Packages / Test ./packages/hono
- GitHub Check: Test Packages / Test ./packages/cache
- GitHub Check: Test Packages / Test ./packages/api
- GitHub Check: Test Packages / Test ./internal/clickhouse
- GitHub Check: Test Packages / Test ./internal/resend
- GitHub Check: Test Packages / Test ./internal/keys
- GitHub Check: Test Packages / Test ./internal/id
- GitHub Check: Test Packages / Test ./internal/hash
- GitHub Check: Test Packages / Test ./internal/encryption
- GitHub Check: Test Go API Local / test_agent_local
- GitHub Check: Test Packages / Test ./internal/billing
- GitHub Check: Build / Build
- GitHub Check: Test API / API Test Local
- GitHub Check: autofix
- GitHub Check: Test Agent Local / test_agent_local
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (3)
apps/api/src/routes/v1_keys_updateKey.error.test.ts (2)
91-92: Schema update to use millisecond timestampsThe key object now uses
createdAtManddeletedAtMwith millisecond timestamps (viaDate.now()) instead of what appears to be previous datetime fields. This change aligns with the schema updates needed to support the newrequestIdfeature.
109-109: Improved error assertion with contextGreat enhancement to the test assertion! Including the full response in the error message makes debugging failed tests much easier.
apps/api/src/routes/v1_migrations_createKey.happy.test.ts (1)
306-306: LGTM: Using dynamic plaintext value improves test qualityGood change from a hardcoded plaintext to a dynamic UUID. This makes the test more robust by verifying functionality with random values rather than static ones.
2 participants
Summary by CodeRabbit