feat: allow-developer-to-set-a-custom-refill-time-when-using-the #2114
Conversation
|
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
3 Skipped Deployments
|
📝 Walkthrough📝 WalkthroughWalkthroughThe changes introduce a new optional property, Changes
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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! 🙏 |
…ill-time-when-using-the
…-allow-developer-to-set-a-custom-refill-time-when-using-the
…en-using-the' of https://github.com/unkeyed/unkey into eng-1324-allow-developer-to-set-a-custom-refill-time-when-using-the
…-allow-developer-to-set-a-custom-refill-time-when-using-the
…-allow-developer-to-set-a-custom-refill-time-when-using-the
![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
🧹 Outside diff range and nitpick comments (9)
tools/migrate/tinybird-export.ts (3)
6-7: Consider removing or conditionally executing the console.log statement.While this log statement is useful for debugging and tracking the export process, it's generally not recommended to leave console.log statements in production code. Consider one of the following options:
- Remove the console.log statement before merging to production.
- Wrap it in a conditional statement that only executes in non-production environments.
- Use a proper logging library that can be configured based on the environment.
If you decide to keep it, you could refactor it to use a logging library or a conditional statement:
if (process.env.NODE_ENV !== 'production') { console.log({ cursor }); }Or using a logging library:
import logger from './your-logging-library'; logger.debug({ cursor });
17-18: Consider removing or conditionally executing the console.log statement.Similar to the previous log statement, while this is useful for debugging and tracking the number of records fetched, it's generally not recommended to leave console.log statements in production code. Consider applying the same solution as suggested for the previous log statement.
If you decide to keep it, you could refactor it to use a logging library or a conditional statement:
if (process.env.NODE_ENV !== 'production') { console.log("found", body.data.length); }Or using a logging library:
import logger from './your-logging-library'; logger.debug("found", body.data.length);
Line range hint
1-28: Consider adding error handling to the main function.The overall structure and logic of the
mainfunction look good. However, it lacks error handling for the fetch operation and file writing. This could lead to silent failures or incomplete data exports if an error occurs during the process.Consider adding try-catch blocks to handle potential errors. Here's a suggested refactor:
async function main() { const exportFile = Bun.file("./export_audit_log.json"); const writer = exportFile.writer(); let cursor = ""; try { do { if (process.env.NODE_ENV !== 'production') { console.log({ cursor }); } const res = await fetch( `https://api.tinybird.co/v0/pipes/export_audit_log.json?cursor=${cursor}`, { headers: { Authorization: `Bearer ${process.env.TINYBIRD_TOKEN}`, }, }, ); if (!res.ok) { throw new Error(`HTTP error! status: ${res.status}`); } const body = (await res.json()) as { data: { auditLogId: string }[] }; if (process.env.NODE_ENV !== 'production') { console.log("found", body.data.length); } for (const row of body.data) { await writer.write(JSON.stringify(row) + "\n"); } cursor = body.data.at(-1)?.auditLogId ?? ""; } while (cursor); } catch (error) { console.error("An error occurred during the export process:", error); } finally { await writer.end(); } } main().catch(error => console.error("Unhandled error in main:", error));This refactored version:
- Adds a try-catch block to handle potential errors during the fetch and write operations.
- Checks if the fetch response is ok before proceeding.
- Uses async/await for the file writing to properly handle potential errors.
- Ensures the writer is closed in a finally block.
- Adds error handling for the main function call itself.
These changes will make the script more robust and easier to debug if issues occur during the export process.
tools/migrate/main.ts (1)
Line range hint
31-43: Overall, the changes enhance tracking without affecting functionality.The added console.log statements improve the ability to track and debug the migration process without impacting the core functionality. This aligns well with the PR objectives.
As a minor suggestion, consider using a logging library like Winston or Pino instead of console.log for more structured and configurable logging in the future. This would allow for easier log management and potential integration with monitoring tools.
apps/api/src/routes/v1_migrations_createKey.error.test.ts (1)
115-150: Approve with suggestions for improvementThe new test case effectively checks for invalid refill configuration when a daily interval has a non-null refillDay. The structure and assertions are correct. However, consider the following suggestions for improvement:
- Make the test name more specific, e.g., "reject refill config with daily interval and non-null refillDay".
- Use a named constant for the refillDay value (4) to improve readability.
- Consider asserting all relevant properties of the error object in the response.
Example improvement:
test("reject refill config with daily interval and non-null refillDay", async (t) => { // ... (existing setup code) const INVALID_REFILL_DAY = 4; const res = await h.post<V1MigrationsCreateKeysRequest, ErrorResponse>({ // ... (existing request code) body: [ { // ... (other properties) refill: { amount: 100, refillDay: INVALID_REFILL_DAY, interval: "daily", }, }, ], }); expect(res.status).toEqual(400); expect(res.body).toMatchObject({ error: { code: "BAD_REQUEST", docs: "https://unkey.dev/docs/api-reference/errors/code/BAD_REQUEST", message: "when interval is set to 'daily', 'refillDay' must be null.", // Consider adding other relevant properties to assert }, }); });These changes would enhance the test's clarity and maintainability.
apps/api/src/routes/v1_migrations_createKey.happy.test.ts (2)
501-501: Test case name could be more specificThe current test name "should provide default value" is somewhat vague. It would be helpful to specify what default value is being tested.
Consider renaming the test case to be more specific:
- test("should provide default value", async (t) => { + test("should default refillDay to 1 when not provided", async (t) => {
525-525: Explicit undefined valueWhile setting
refillDaytoundefinedis correct, it might be beneficial to add a comment explaining why this is done explicitly, asundefinedis the default value for an omitted property.Consider adding a comment to explain the explicit
undefined:- refillDay: undefined, + refillDay: undefined, // Explicitly set to undefined to test default behaviorapps/api/src/routes/v1_keys_updateKey.ts (2)
356-360: Consider renaminglastDayOfMonthfor clarityThe logic for determining the refill day is correct. However, when
refillDayis provided and valid, the variablelastDayOfMonthno longer represents the last day of the month. Consider renaming it to something likerefillDayOfMonthfor better clarity.Here's a suggested change:
const date = new Date(); -let lastDayOfMonth = new Date(date.getFullYear(), date.getMonth() + 1, 0).getDate(); -if (req.refill?.refillDay && req?.refill?.refillDay <= lastDayOfMonth) { - lastDayOfMonth = req.refill.refillDay; +let refillDayOfMonth = new Date(date.getFullYear(), date.getMonth() + 1, 0).getDate(); +if (req.refill?.refillDay && req?.refill?.refillDay <= refillDayOfMonth) { + refillDayOfMonth = req.refill.refillDay; }
Line range hint
1-1: Reminder: Add test cases for the newrefillDayfeatureThe implementation of the
refillDayfeature looks good, but I don't see any new tests added. Please ensure that you add appropriate test cases to cover this new functionality, including edge cases and error scenarios.Would you like assistance in creating these test cases or opening a GitHub issue to track this task?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (19)
- apps/api/src/routes/schema.ts (2 hunks)
- apps/api/src/routes/v1_apis_listKeys.ts (1 hunks)
- apps/api/src/routes/v1_keys_createKey.happy.test.ts (1 hunks)
- apps/api/src/routes/v1_keys_createKey.ts (4 hunks)
- apps/api/src/routes/v1_keys_updateKey.error.test.ts (2 hunks)
- apps/api/src/routes/v1_keys_updateKey.happy.test.ts (1 hunks)
- apps/api/src/routes/v1_keys_updateKey.ts (4 hunks)
- apps/api/src/routes/v1_migrations_createKey.error.test.ts (1 hunks)
- apps/api/src/routes/v1_migrations_createKey.happy.test.ts (1 hunks)
- apps/api/src/routes/v1_migrations_createKey.ts (3 hunks)
- apps/dashboard/app/(app)/apis/[apiId]/keys/[keyAuthId]/new/client.tsx (9 hunks)
- apps/dashboard/lib/trpc/routers/key/create.ts (2 hunks)
- apps/docs/libraries/ts/sdk/keys/get.mdx (1 hunks)
- apps/docs/libraries/ts/sdk/keys/update.mdx (2 hunks)
- apps/workflows/jobs/refill-daily.ts (3 hunks)
- internal/db/src/schema/key_migrations.ts (1 hunks)
- tools/migrate/auditlog-import.ts (2 hunks)
- tools/migrate/main.ts (2 hunks)
- tools/migrate/tinybird-export.ts (2 hunks)
✅ Files skipped from review due to trivial changes (1)
- tools/migrate/auditlog-import.ts
🚧 Files skipped from review as they are similar to previous changes (13)
- apps/api/src/routes/schema.ts
- apps/api/src/routes/v1_apis_listKeys.ts
- apps/api/src/routes/v1_keys_createKey.happy.test.ts
- apps/api/src/routes/v1_keys_createKey.ts
- apps/api/src/routes/v1_keys_updateKey.error.test.ts
- apps/api/src/routes/v1_keys_updateKey.happy.test.ts
- apps/api/src/routes/v1_migrations_createKey.ts
- apps/dashboard/app/(app)/apis/[apiId]/keys/[keyAuthId]/new/client.tsx
- apps/dashboard/lib/trpc/routers/key/create.ts
- apps/docs/libraries/ts/sdk/keys/get.mdx
- apps/docs/libraries/ts/sdk/keys/update.mdx
- apps/workflows/jobs/refill-daily.ts
- internal/db/src/schema/key_migrations.ts
🧰 Additional context used
📓 Learnings (1)
apps/api/src/routes/v1_migrations_createKey.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-09-27T15:20:05.475Z 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.
🔇 Additional comments (5)
tools/migrate/main.ts (2)
31-32: LGTM: Useful logging for tracking migration progress.The added console.log statement provides valuable information for tracking the creation of new identities during the migration process. The comment to ignore the linter warning is appropriate in this context.
42-43: LGTM: Helpful logging for tracking key-identity connections.The added console.log statement effectively tracks the connection between identities and keys during the migration process. The comment to ignore the linter warning is appropriate in this context.
apps/api/src/routes/v1_keys_updateKey.ts (3)
154-157: LGTM: NewrefillDayfield added correctlyThe new
refillDayfield is correctly implemented as an optional number between 1 and 31, with a clear OpenAPI description explaining its purpose for monthly refills.
340-345: LGTM: Proper validation forrefillDayThe added validation logic correctly prevents setting
refillDaywhen the interval is "daily", throwing an informative error message.
391-391: LGTM: CorrectrefillDayassignmentThe
refillDayis correctly set based on the refill interval. If you accept the suggestion to renamelastDayOfMonthtorefillDayOfMonth, remember to update it here as well.
…-allow-developer-to-set-a-custom-refill-time-when-using-the
…en-using-the' of https://github.com/unkeyed/unkey into eng-1324-allow-developer-to-set-a-custom-refill-time-when-using-the
What does this PR do?
Eng-1324
Fixes # (issue)
If there is not an issue for this, please create one first. This is used to tracking purposes and also helps use understand why this PR exists
Type of change
How should this be tested?
Checklist
Required
pnpm buildpnpm fmtconsole.logsgit pull origin mainAppreciated
Summary by CodeRabbit
Release Notes
New Features
refillDayfor specifying the day of the month for refills, applicable when the refill interval is set to "monthly."refillDay.Documentation
refillDay, including its default behavior and usage examples.Bug Fixes
Tests
refillDayparameter and ensure proper error handling for invalid configurations.