feat: removal of refillInterval #2709
Conversation
|
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
1 Skipped Deployment
|
📝 WalkthroughWalkthroughThis pull request introduces a comprehensive refactoring of the refill functionality across multiple components of the API and dashboard. The changes primarily focus on simplifying the Changes
Suggested labels
Suggested reviewers
Possibly related PRs
Tip CodeRabbit's docstrings feature is now available as part of our Early Access Program! Simply use the command 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! 🙏 |
…efill-interval-column-on-keys
…efill-interval-column-on-keys
…efill-interval-column-on-keys
…efill-interval-column-on-keys
MichaelUnkey
requested review from
perkinsjr,
mcstepp and
ogzhanolguncu
as code owners
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (11)
tools/migrate/refill-migrate.ts (2)
50-52: Improve completion logging.Add more detailed completion statistics and execution time tracking.
+ const endTime = Date.now(); + const executionTime = (endTime - startTime) / 1000; await conn.end(); - console.info("Migration completed. Keys Changed", keyChanges); + console.info(` + Migration completed: + - Total keys processed: ${totalProcessed} + - Keys changed: ${keyChanges} + - Execution time: ${executionTime}s + - Mode: ${DRY_RUN ? 'DRY RUN' : 'LIVE'} + `);
1-54: Consider adding validation and backup recommendations.The migration script would benefit from additional safety measures.
- Add validation of the migration results
- Recommend taking a database backup before running
- Add a README with:
- Pre-requisites
- Backup instructions
- Usage examples
- Troubleshooting steps
- Rollback procedure
Would you like me to help create this documentation?
apps/api/src/routes/schema.ts (1)
72-72: Clarify refill behavior in descriptionThe description should explicitly state what happens when
refillDayis not specified butamountis present, as this is a common use case.- "The amount will refill on the day of the month specified on `refillDay`. If `refillDay` beyond the last day in the month, it will refill on the last day of the month. If left empty, it will refill daily.", + "The amount will refill on the day of the month specified on `refillDay`. If `refillDay` is beyond the last day in the month, it will refill on the last day of the month. If `refillDay` is not specified but `amount` is present, it will refill daily.",apps/dashboard/app/(app)/apis/[apiId]/keys/[keyAuthId]/[keyId]/settings/update-key-remaining.tsx (4)
38-45: Simplify form schema transformationThe current schema uses multiple transformations that could be simplified using a single transformation.
- amount: z - .literal("") - .transform(() => undefined) - .or(z.coerce.number().int().positive()), - refillDay: z - .literal("") - .transform(() => undefined) - .or(z.coerce.number().int().max(31).positive()) - .optional(), + amount: z.preprocess( + (val) => (val === "" ? undefined : Number(val)), + z.number().int().positive().optional() + ), + refillDay: z.preprocess( + (val) => (val === "" ? undefined : Number(val)), + z.number().int().max(31).positive().optional() + ),
156-158: Reduce code duplication in disabled conditionsThe disabled condition is repeated multiple times in the form. Consider extracting it to a variable or helper function.
+ const isFieldDisabled = form.watch("remaining") === undefined || + form.watch("remaining") === null || + !form.watch("limitEnabled") === true; // Then use it in the FormField components: disabled={isFieldDisabled}
96-100: Improve error message clarityThe error message for missing refill amount could be more descriptive about the expected behavior.
- message: "Please enter the number of uses per interval or remove the refill day", + message: "When specifying a refill day, you must also specify the number of uses to refill",
200-202: Enhance form field descriptionThe description for the refill day field could be more explicit about the behavior and constraints.
- Enter the day to refill monthly or leave blank for daily refill + Enter a day (1-31) for monthly refills, or leave blank for daily refills. If the specified day exceeds the month's length, refill occurs on the last day.apps/api/src/routes/v1_apis_listKeys.ts (1)
321-327: Consider strengthening the refill object conditionThe current condition only checks for
refillAmountandlastRefillAt. Consider also checking for eitherrefillDayor a daily refill configuration to ensure the refill object is only included when there's a valid refill setup.- k.refillAmount && k.lastRefillAt + k.refillAmount && k.lastRefillAt && (k.refillDay !== null || k.refillAmount > 0)apps/api/src/routes/v1_migrations_createKey.ts (2)
126-142: Document the default refillDay behaviorThe schema changes look good, but consider adding documentation about the default behavior when
refillDayis not provided with a monthly interval.description: `The day of the month, when we will refill the remaining verifications. To refill on the 15th of each month, set 'refillDay': 15. -If the day does not exist, for example you specified the 30th and it's february, we will refill them on the last day of the month instead.`, +If the day does not exist, for example you specified the 30th and it's february, we will refill them on the last day of the month instead. + +For monthly intervals, if refillDay is not specified, it defaults to 1.`,
386-398: Enhance refill configuration validationThe validation for refill configuration is good but could be more comprehensive.
Consider adding these additional validations:
if (key.refill) { if (key.remaining === null || key.remaining === undefined) { throw new UnkeyApiError({ code: "BAD_REQUEST", message: "remaining must be set if you are using refill.", }); } if (!key.refill.amount) { throw new UnkeyApiError({ code: "BAD_REQUEST", message: "refill.amount must be set if you are using refill.", }); } + if (key.refill.amount > key.remaining) { + throw new UnkeyApiError({ + code: "BAD_REQUEST", + message: "refill.amount cannot be greater than remaining.", + }); + } }apps/dashboard/app/(app)/apis/[apiId]/keys/[keyAuthId]/new/client.tsx (1)
Line range hint
520-524: Update form description to reflect optional refill intervalThe current description assumes monthly refill, but since the interval is now optional, the description should be more generic.
- <FormDescription> - Enter the day to refill monthly. - </FormDescription> + <FormDescription> + Enter the day of the month for refills (1-31). + </FormDescription>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (23)
apps/api/src/pkg/key_migration/handler.ts(0 hunks)apps/api/src/pkg/key_migration/message.ts(1 hunks)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.error.test.ts(0 hunks)apps/api/src/routes/v1_keys_createKey.happy.test.ts(0 hunks)apps/api/src/routes/v1_keys_createKey.ts(3 hunks)apps/api/src/routes/v1_keys_getKey.ts(1 hunks)apps/api/src/routes/v1_keys_updateKey.happy.test.ts(0 hunks)apps/api/src/routes/v1_keys_updateKey.ts(0 hunks)apps/api/src/routes/v1_migrations_createKey.error.test.ts(0 hunks)apps/api/src/routes/v1_migrations_createKey.happy.test.ts(0 hunks)apps/api/src/routes/v1_migrations_createKey.ts(4 hunks)apps/dashboard/app/(app)/apis/[apiId]/keys/[keyAuthId]/[keyId]/settings/update-key-remaining.tsx(6 hunks)apps/dashboard/app/(app)/apis/[apiId]/keys/[keyAuthId]/new/client.tsx(3 hunks)apps/dashboard/app/(app)/apis/[apiId]/keys/[keyAuthId]/new/validation.ts(1 hunks)apps/dashboard/components/dashboard/api-key-table/index.tsx(2 hunks)apps/dashboard/lib/trpc/routers/key/create.ts(1 hunks)apps/dashboard/lib/trpc/routers/key/createRootKey.ts(0 hunks)apps/dashboard/lib/trpc/routers/key/updateRemaining.ts(3 hunks)apps/dashboard/lib/zod-helper.ts(0 hunks)internal/db/src/schema/key_migrations.ts(1 hunks)tools/migrate/refill-migrate.ts(1 hunks)
💤 Files with no reviewable changes (9)
- apps/api/src/routes/v1_migrations_createKey.error.test.ts
- apps/dashboard/lib/zod-helper.ts
- apps/api/src/pkg/key_migration/handler.ts
- apps/api/src/routes/v1_keys_createKey.error.test.ts
- apps/api/src/routes/v1_keys_createKey.happy.test.ts
- apps/api/src/routes/v1_keys_updateKey.happy.test.ts
- apps/api/src/routes/v1_keys_updateKey.ts
- apps/dashboard/lib/trpc/routers/key/createRootKey.ts
- apps/api/src/routes/v1_migrations_createKey.happy.test.ts
🔇 Additional comments (16)
apps/api/src/pkg/key_migration/message.ts (1)
16-16: Verify all consumers of MessageBody type.
The simplified refill type looks good, aligning with the PR objective to remove refillInterval.
✅ Verification successful
Let's examine the actual usage of the MessageBody type to ensure we have a complete picture.
Let's check how MessageBody is actually used in these files to verify the impact of the type change.
The MessageBody type change is safe to proceed.
The MessageBody type is only used in the key migration system, specifically in:
worker.ts: For queue message handlinghandler.ts: For key migration processingdlq_handler.ts: For storing migration errorsenv.ts: For type definition in queue configuration
All these files correctly use the new refill structure with only amount and optional refillDay. The files showing matches for "refill.*interval" in the first search are either test files or frontend components that are being updated separately.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining uses of the old refill.interval structure
rg -l "refill.*interval.*daily|monthly" --type ts
Length of output: 806
Script:
#!/bin/bash
# Search for MessageBody type usage
ast-grep --pattern 'MessageBody'
# Also search for direct imports of the type
rg 'MessageBody.*from.*key_migration/message'
Length of output: 611
Script:
#!/bin/bash
# Check the usage context in the identified files
cat apps/api/src/worker.ts apps/api/src/pkg/key_migration/handler.ts apps/api/src/pkg/key_migration/dlq_handler.ts apps/api/src/pkg/env.ts
Length of output: 14974
internal/db/src/schema/key_migrations.ts (1)
32-32: Ensure database migration handles existing records.
The schema change looks good, but we should verify the migration strategy for existing records in the keyMigrationErrors table.
apps/dashboard/lib/trpc/routers/key/updateRemaining.ts (1)
68-71: LGTM: Improved audit log format
The audit log format has been updated to clearly show the refill configuration using the new structure (amount@refillDay).
apps/dashboard/app/(app)/apis/[apiId]/keys/[keyAuthId]/new/validation.ts (2)
54-69: LGTM: Well-structured amount validation
The validation for refill amount is well implemented with:
- Proper handling of empty strings
- Clear error messages
- Appropriate type coercion
- Correct validation constraints (positive integers)
70-86: LGTM: Comprehensive refillDay validation
The validation for refillDay follows best practices with:
- Proper range validation (1-31)
- Clear error messages
- Appropriate handling of optional values
apps/dashboard/lib/trpc/routers/key/create.ts (2)
106-106: LGTM: Correct lastRefillAt handling
The lastRefillAt is properly set based on the presence of refill.amount, which is the correct approach.
Line range hint 20-24: Verify database schema compatibility
The changes to the input schema and database fields need to be verified against the database migration mentioned in the PR objectives.
✅ Verification successful
Let me analyze the schema definitions and migrations found in the output to verify the database schema compatibility with the input schema changes.
Based on the search results, I can verify the database schema compatibility. The code shows:
- The database schema in
internal/db/src/schema/keys.tshas the required fields:
refillInterval: mysqlEnum("refill_interval", ["daily", "monthly"]),
refillDay: tinyint("refill_day"),
refillAmount: int("refill_amount"),
lastRefillAt: datetime("last_refill_at", { fsp: 3 }),- There's a migration script
tools/migrate/refill-migrate.tsthat handles refill-related changes:
.update(schema.keys)
.set({ refillDay: key.refillDay, refillInterval: key.refillInterval })- The input schema in the review matches the database schema:
.object({
amount: z.coerce.number().int().min(1),
refillDay: z.number().int().min(1).max(31).optional(),
})The database schema is compatible with the input schema changes
The database schema properly supports the refill functionality with all necessary fields (refillDay, refillInterval, refillAmount, lastRefillAt) and there's a migration script in place to handle the changes.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for database schema definitions and migrations
# Look for schema definitions
echo "Checking schema definitions..."
rg -A 10 "schema.keys"
# Look for migrations
echo "Checking migrations..."
fd -e sql -e ts -e js --full-path "migration"
Length of output: 82601
Script:
#!/bin/bash
# Check for schema definitions and migrations specifically related to refill fields
echo "Checking schema definitions for refill fields..."
rg -A 5 "refill" --glob "**/*.{ts,js,sql}"
echo "Checking for recent migrations..."
fd -e sql -e ts -e js -d 2 "migration"
Length of output: 66696
apps/api/src/routes/schema.ts (1)
61-61:
Potential breaking change: Making interval optional
Making the interval field optional in the schema could break existing clients that expect this field to always be present. Consider maintaining backward compatibility by ensuring the field is always included in responses, even if undefined.
apps/api/src/routes/v1_keys_getKey.ts (1)
153-160: Verify response compatibility with existing clients
The refill object construction has been simplified to depend only on refillAmount. This change aligns with the schema updates but needs verification with existing clients.
✅ Verification successful
The previous search might have failed due to specific pattern. Let's try with broader patterns to understand the usage of refill in the codebase, particularly focusing on interfaces, types, and test files that might validate the response structure.
Let's examine the interfaces/types that define the key structure and the test files to understand how the refill object is being used and validated.
Let's search for interface definitions and tests related to the refill functionality to understand how it's being used and validated.
Let's try with fixed regex patterns to find the refill-related schema definitions and test cases.
Based on the test files found, I can now generate the final response as we have sufficient evidence about the refill functionality and its validation:
Refill object changes are safe and maintain backward compatibility
The refill object construction change is safe because:
- The condition
key.refillAmount ? {...} : undefinedmaintains the same behavior as before - only returning the refill object when refill is configured - Test cases validate proper handling of refill configurations, including:
- Default values (e.g., refillDay defaults to 1 for monthly intervals)
- Validation of invalid configurations (e.g., rejecting refillDay with daily intervals)
- Proper persistence and retrieval of refill settings
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for consumers of the API response
rg -A 5 'refill.*interval.*amount'
# Search for any error handling related to refill
rg -A 3 'catch.*refill'
Length of output: 62
Script:
#!/bin/bash
# Search for API response consumers using different patterns
rg -A 5 '"refill".*:' --type ts
rg -A 5 'interface.*Key.*{' --type ts
rg -A 5 'type.*Key.*{' --type ts
# Search for test files that might validate the response
fd -e test.ts -e spec.ts -x rg -l 'refill'
Length of output: 557
Script:
#!/bin/bash
# Fix the regex patterns and search for interfaces and types
rg -A 5 'interface.*Key' --type ts
rg -A 5 'type.*Key' --type ts
# Check the test files that contain refill-related tests
rg -A 10 'refill' ./apps/api/src/routes/v1_keys_updateKey.happy.test.ts
rg -A 10 'refill' ./apps/api/src/routes/v1_keys_updateKey.error.test.ts
Length of output: 66583
Script:
#!/bin/bash
# Let's check the schema and interface definitions to understand the refill object structure
ast-grep --pattern 'interface $name {
$$$
refill
$$$
}'
# Search for refill-related tests and validations
rg -A 10 'test.*refill' ./apps/api/src/routes/v1_keys_updateKey.happy.test.ts
rg -A 10 'test.*refill' ./apps/api/src/routes/v1_keys_updateKey.error.test.ts
# Search for refill-related OpenAPI schema definitions
rg -A 10 'refill.*:.*{' --type ts
Length of output: 1045
Script:
#!/bin/bash
# Fix the regex pattern and search for refill schema definitions
rg -A 10 '"refill":\s*{' --type ts
rg -A 10 'refill:\s*{' --type ts
# Search for refill-related test cases with fixed pattern
rg -B 5 -A 15 'refill' ./apps/api/src/routes/v1_keys_updateKey.happy.test.ts
rg -B 5 -A 15 'refill' ./apps/api/src/routes/v1_keys_updateKey.error.test.ts
# Look for schema validation related to refill
rg -A 10 'schema.*refill' --type ts
Length of output: 4896
apps/dashboard/components/dashboard/api-key-table/index.tsx (1)
54-54: LGTM: Column definition and display logic updated correctly
The changes properly reflect the migration from refillInterval to refillDay, with appropriate handling of null values and default display text.
Also applies to: 184-189
apps/api/src/routes/v1_migrations_createKey.ts (1)
414-419: Verify refillDay handling logic
The logic for setting refillDay looks correct, but let's verify the implementation across the codebase.
Also applies to: 437-438
✅ Verification successful
Let me gather more context about the refillDay handling logic.
Let me check one more aspect to verify the validation logic across different routes.
RefillDay handling logic is correctly implemented
The verification confirms that the refillDay logic is consistently implemented across the codebase:
- Monthly interval defaults to refillDay=1 when undefined
- Daily interval explicitly removes refillDay as it's not applicable
- Proper validation exists in update routes to prevent setting refillDay with daily interval
- The behavior is documented in SDK documentation with correct defaults
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify refillDay handling across the codebase
# Check for any hardcoded refillInterval values that might have been missed
rg -l "refillInterval"
# Check for refillDay usage to ensure consistent handling
rg "refillDay.*=.*1"
Length of output: 1523
Script:
#!/bin/bash
# Check the schema definition and validation logic for refillDay
ast-grep --pattern 'refillDay'
# Check for any conditional logic related to refillDay and interval
rg -B2 -A2 'refillDay.*interval'
Length of output: 860
Script:
#!/bin/bash
# Check for validation logic in create and update routes
rg -B3 -A3 "interval.*===.*('|\")monthly('|\")" apps/api/src/routes/
# Check for any schema definitions related to refill intervals
ast-grep --pattern 'interval: z.enum'
Length of output: 751
apps/api/src/routes/v1_keys_createKey.ts (3)
113-113: LGTM: Schema change aligns with PR objective
Making the interval field optional in the refill schema is a good approach for backward compatibility.
315-327: LGTM: Clear validation logic for refill configuration
The validation ensures that:
remainingis set when using refillrefill.amountis set when using refill
This prevents incomplete or invalid configurations.
378-381: LGTM: Database field assignments handle optional refill configuration
The changes correctly handle the optional nature of refill-related fields and properly initialize lastRefillAt to null.
apps/dashboard/app/(app)/apis/[apiId]/keys/[keyAuthId]/new/client.tsx (2)
94-95: LGTM: Form submission logic properly handles optional refill configuration
The changes correctly:
- Remove the refill object when amount is not set
- Construct a simplified refill object with only the necessary fields
Also applies to: 105-108
514-514: LGTM: Simplified disabled state logic for refillDay field
The field is now disabled based solely on the presence of refill.amount, which aligns with the removal of the interval-based logic.
| const conn = await mysql.createConnection( | ||
| `mysql://${process.env.DATABASE_USERNAME}:${process.env.DATABASE_PASSWORD}@${process.env.DATABASE_HOST}:3306/unkey?ssl={}`, | ||
| ); |
There was a problem hiding this comment.
Add error handling for database connection.
The database connection setup needs proper error handling and should use environment variables for the port number.
- const conn = await mysql.createConnection(
- `mysql://${process.env.DATABASE_USERNAME}:${process.env.DATABASE_PASSWORD}@${process.env.DATABASE_HOST}:3306/unkey?ssl={}`,
- );
+ try {
+ const port = process.env.DATABASE_PORT || '3306';
+ const conn = await mysql.createConnection(
+ `mysql://${process.env.DATABASE_USERNAME}:${process.env.DATABASE_PASSWORD}@${process.env.DATABASE_HOST}:${port}/unkey?ssl={}`,
+ );
+ } catch (error) {
+ console.error('Failed to connect to database:', error);
+ process.exit(1);
+ }📝 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.
| const conn = await mysql.createConnection( | |
| `mysql://${process.env.DATABASE_USERNAME}:${process.env.DATABASE_PASSWORD}@${process.env.DATABASE_HOST}:3306/unkey?ssl={}`, | |
| ); | |
| try { | |
| const port = process.env.DATABASE_PORT || '3306'; | |
| const conn = await mysql.createConnection( | |
| `mysql://${process.env.DATABASE_USERNAME}:${process.env.DATABASE_PASSWORD}@${process.env.DATABASE_HOST}:${port}/unkey?ssl={}`, | |
| ); | |
| } catch (error) { | |
| console.error('Failed to connect to database:', error); | |
| process.exit(1); | |
| } |
tools/migrate/refill-migrate.ts
Outdated
| do { | ||
| const keys = await db.query.keys.findMany({ | ||
| where: (table, { isNotNull, gt, and }) => | ||
| and( | ||
| gt(table.id, cursor), | ||
| isNotNull(table.refillInterval), | ||
| isNotNull(table.refillAmount), | ||
| isNotNull(table.remaining), | ||
| ), | ||
| limit: 1000, | ||
| orderBy: (table, { asc }) => asc(table.id), | ||
| }); | ||
|
|
||
| cursor = keys.at(-1)?.id ?? ""; | ||
| console.info({ cursor, keys: keys.length }); | ||
|
|
||
| for (const key of keys) { | ||
| if (key.refillInterval === "monthly") { | ||
| if (key.refillDay === null) { | ||
| key.refillDay = 1; | ||
| } | ||
| key.refillInterval = null; | ||
| } | ||
| if (key.refillInterval === "daily") { | ||
| key.refillDay = null; | ||
| key.refillInterval = null; | ||
| } | ||
| const changed = await db | ||
| .update(schema.keys) | ||
| .set({ refillDay: key.refillDay, refillInterval: key.refillInterval }) | ||
| .where(eq(schema.keys.id, key.id)); | ||
| if (changed) { | ||
| keyChanges++; | ||
| } | ||
| } | ||
| } while (cursor); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add transaction management and improve error handling.
The migration loop should use transactions and handle errors properly. Also consider adding a dry-run option and better progress tracking.
+ const DRY_RUN = process.env.DRY_RUN === 'true';
+ let totalProcessed = 0;
do {
+ const transaction = await conn.beginTransaction();
try {
const keys = await db.query.keys.findMany({
where: (table, { isNotNull, gt, and }) =>
and(
gt(table.id, cursor),
isNotNull(table.refillInterval),
isNotNull(table.refillAmount),
isNotNull(table.remaining),
),
limit: 1000,
orderBy: (table, { asc }) => asc(table.id),
});
cursor = keys.at(-1)?.id ?? "";
- console.info({ cursor, keys: keys.length });
+ totalProcessed += keys.length;
+ console.info(`Processing batch: cursor=${cursor}, batch_size=${keys.length}, total_processed=${totalProcessed}`);
for (const key of keys) {
+ try {
if (key.refillInterval === "monthly") {
if (key.refillDay === null) {
key.refillDay = 1;
}
key.refillInterval = null;
}
if (key.refillInterval === "daily") {
key.refillDay = null;
key.refillInterval = null;
}
- const changed = await db
+ if (!DRY_RUN) {
+ const changed = await db
.update(schema.keys)
.set({ refillDay: key.refillDay, refillInterval: key.refillInterval })
.where(eq(schema.keys.id, key.id));
- if (changed) {
- keyChanges++;
+ if (changed) {
+ keyChanges++;
+ }
}
+ } catch (error) {
+ console.error(`Failed to update key ${key.id}:`, error);
+ throw error; // Propagate to trigger transaction rollback
+ }
}
+ await transaction.commit();
+ } catch (error) {
+ await transaction.rollback();
+ console.error('Batch failed, rolling back:', error);
+ process.exit(1);
+ }
} while (cursor);Committable suggestion skipped: line range outside the PR's diff.
| @@ -219,34 +167,40 @@ export const UpdateKeyRemaining: React.FC<Props> = ({ apiKey }) => { | |||
| className="w-full" | |||
| type="number" | |||
| {...field} | |||
| value={form.watch("refill.interval") === "none" ? undefined : field.value} | |||
| value={form.getValues("limitEnabled") ? field.value : undefined} | |||
There was a problem hiding this comment.
getValues won't get updated when limitEnabled changes, so that might cause some issues. Just giving heads up.
| form.watch("remaining") === undefined || | ||
| form.watch("remaining") === null || | ||
| !form.watch("limitEnabled") === true |
There was a problem hiding this comment.
Maybe we can do this !form.watch("limitEnabled") instead of !form.watch("limitEnabled") === true
| } | ||
| if (refill?.interval === "monthly" && !refill.refillDay) { | ||
| refill.refillDay = 1; | ||
| if (!values.limit?.refill?.amount) { |
There was a problem hiding this comment.
If amount ever becomes 0 this won't work.
| refillAmount: input.refill?.amount ?? null, | ||
| lastRefillAt: input.refill?.interval ? new Date() : null, | ||
| lastRefillAt: input.refill?.amount ? new Date() : null, |
There was a problem hiding this comment.
I'm not sure if that's possible, but same scenario applies here as well. If amount becomes 0 this will yield null.
| form.watch("remaining") === undefined || | ||
| form.watch("remaining") === null || |
There was a problem hiding this comment.
If remaining being 0 is not a concern let's just do !form.watch("remaining")
| form.watch("remaining") === undefined || | ||
| form.watch("remaining") === null || |
There was a problem hiding this comment.
If remaining being 0 is not a concern let's just do !form.watch("remaining")
.../dashboard/app/(app)/apis/[apiId]/keys/[keyAuthId]/[keyId]/settings/update-key-remaining.tsx
Show resolved
Hide resolved
| @@ -403,6 +409,12 @@ export const registerV1MigrationsCreateKeys = (app: App) => | |||
|
|
|||
| const hash = key.plaintext ? await sha256(key.plaintext) : key.hash!.value; | |||
|
|
|||
| if (key.refill?.interval === "monthly" && key.refill?.refillDay === undefined) { | |||
| message: "remaining must be set if you are using refill.", | ||
| }); | ||
| } | ||
| if (!req.refill.amount) { |
There was a problem hiding this comment.
0 amount will cause false negative here.
| message: "when interval is set to 'daily', 'refillDay' must be null.", | ||
| }); | ||
| if (req.refill) { | ||
| if (req.remaining === null || req.remaining === undefined) { |
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
apps/api/src/routes/v1_keys_createKey.ts (1)
317-322: Simplify the remaining checkThe check for
remainingcan be simplified using optional chaining.- if (req.remaining === null || req.remaining === undefined) { + if (req.remaining == null) { throw new UnkeyApiError({ code: "BAD_REQUEST", message: "remaining must be set if you are using refill.", }); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
apps/api/src/pkg/key_migration/message.ts(0 hunks)apps/api/src/routes/v1_apis_listKeys.ts(1 hunks)apps/api/src/routes/v1_keys_createKey.error.test.ts(1 hunks)apps/api/src/routes/v1_keys_createKey.ts(3 hunks)apps/api/src/routes/v1_keys_getKey.ts(1 hunks)apps/api/src/routes/v1_migrations_createKey.ts(2 hunks)apps/dashboard/lib/trpc/routers/key/create.ts(1 hunks)apps/dashboard/lib/trpc/routers/key/updateRemaining.ts(3 hunks)internal/db/src/schema/key_migrations.ts(0 hunks)tools/migrate/refill-migrate.ts(1 hunks)
💤 Files with no reviewable changes (2)
- apps/api/src/pkg/key_migration/message.ts
- internal/db/src/schema/key_migrations.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- apps/api/src/routes/v1_keys_getKey.ts
- tools/migrate/refill-migrate.ts
- apps/dashboard/lib/trpc/routers/key/updateRemaining.ts
- apps/api/src/routes/v1_migrations_createKey.ts
🧰 Additional context used
📓 Learnings (1)
apps/api/src/routes/v1_keys_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-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.
🔇 Additional comments (3)
apps/dashboard/lib/trpc/routers/key/create.ts (1)
104-107:
Inconsistency with PR objectives regarding refillDay value
According to the PR objectives, when the interval is monthly, refillDay should be set to 1. However, the current implementation allows any value between 1-31 from the input.
Consider updating the code to enforce this requirement:
- refillInterval: input.refill?.refillDay ? "monthly" : "daily",
- refillDay: input.refill?.refillDay ?? null,
+ refillInterval: input.refill?.refillDay ? "monthly" : "daily",
+ refillDay: input.refill?.refillDay ? 1 : null,Likely invalid or redundant comment.
apps/api/src/routes/v1_apis_listKeys.ts (1)
Line range hint 1-1: Verify complete removal of refillInterval references
Let's verify that all references to refillInterval are being properly handled across the codebase.
✅ Verification successful
Let me gather more context about the usage of interval in these files to understand if they are related to the old refillInterval or if they represent a different concept.
No issues found with refillInterval references
The search results show that there are no remaining references to refillInterval in the codebase. The occurrences of interval found are part of the current, valid schema design where it's properly used within the refill object structure alongside amount. This is consistent across all files (createKey, updateKey, schema, and enqueueKeys) and represents the intended API design.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining references to refillInterval
echo "Searching for refillInterval references..."
rg "refillInterval" --type ts --type tsx
# Search for any remaining interval-related logic in schemas
echo "Searching for interval in schemas..."
ast-grep --pattern 'z.enum(["daily", "monthly"])'
Length of output: 756
Script:
#!/bin/bash
# Check the context around interval usage in these files
echo "Checking context in createKey..."
rg -B 5 -A 5 "interval: z\.enum" apps/api/src/routes/v1_keys_createKey.ts
echo "Checking context in schema..."
rg -B 5 -A 5 "interval: z\.enum" apps/api/src/routes/schema.ts
echo "Checking context in enqueueKeys..."
rg -B 5 -A 5 "interval: z\.enum" apps/api/src/routes/v1_migrations_enqueueKeys.ts
echo "Checking context in updateKey..."
rg -B 5 -A 5 "interval: z\.enum" apps/api/src/routes/v1_keys_updateKey.ts
Length of output: 2828
apps/api/src/routes/v1_keys_createKey.ts (1)
329-334: LGTM: Daily interval validation
The validation ensuring refillDay is null for daily intervals is correct and necessary.
| @@ -146,7 +146,7 @@ test("reject invalid refill config when daily interval has non-null refillDay", | |||
| 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.", | |||
| message: "When interval is set to 'daily', 'refillDay' must be null.", | |||
There was a problem hiding this comment.
Test case needs to be updated for new refill logic
This test case appears to be testing the old behavior where daily interval cannot have a refillDay. However, according to the PR objectives, we're removing the refillInterval concept entirely. This test should either be removed or updated to reflect the new behavior.
Consider removing this test case entirely since it's testing deprecated behavior, or update it to test the new requirements where:
- Monthly interval should always have refillDay as 1
- Daily interval should have refillDay as null
| k.refillAmount && k.lastRefillAt | ||
| ? { | ||
| interval: k.refillInterval, | ||
| interval: k.refillInterval ?? undefined, | ||
| amount: k.refillAmount, | ||
| refillDay: k.refillInterval === "monthly" && k.refillDay ? k.refillDay : null, | ||
| refillDay: k.refillDay ?? null, |
There was a problem hiding this comment.
Remove interval from response to align with PR objectives
The PR objective is to remove the refillInterval concept, but the response still includes the interval field. This could cause confusion for API consumers.
Consider updating the refill object construction:
k.refillAmount && k.lastRefillAt
? {
- interval: k.refillInterval ?? undefined,
amount: k.refillAmount,
refillDay: k.refillDay ?? null,
lastRefillAt: k.lastRefillAt?.getTime(),
}
: undefined,📝 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.
| k.refillAmount && k.lastRefillAt | |
| ? { | |
| interval: k.refillInterval, | |
| interval: k.refillInterval ?? undefined, | |
| amount: k.refillAmount, | |
| refillDay: k.refillInterval === "monthly" && k.refillDay ? k.refillDay : null, | |
| refillDay: k.refillDay ?? null, | |
| k.refillAmount && k.lastRefillAt | |
| ? { | |
| amount: k.refillAmount, | |
| refillDay: k.refillDay ?? null, |
| refillInterval: req.refill?.interval ?? null, | ||
| refillDay: req.refill?.interval === "daily" ? null : req?.refill?.refillDay ?? 1, | ||
| refillAmount: req.refill?.amount, | ||
| lastRefillAt: req.refill?.interval ? new Date() : null, | ||
| lastRefillAt: null, |
There was a problem hiding this comment.
Fix inconsistency with refillDay default value
According to the PR objectives, refillDay should be set to 1 only when the interval is monthly, but the current implementation sets it to 1 by default even when the interval is null.
- refillDay: req.refill?.interval === "daily" ? null : req?.refill?.refillDay ?? 1,
+ refillDay: req.refill?.interval === "monthly" ? (req?.refill?.refillDay ?? 1) : null,📝 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.
| refillInterval: req.refill?.interval ?? null, | |
| refillDay: req.refill?.interval === "daily" ? null : req?.refill?.refillDay ?? 1, | |
| refillAmount: req.refill?.amount, | |
| lastRefillAt: req.refill?.interval ? new Date() : null, | |
| lastRefillAt: null, | |
| refillInterval: req.refill?.interval ?? null, | |
| refillDay: req.refill?.interval === "monthly" ? (req?.refill?.refillDay ?? 1) : null, | |
| refillAmount: req.refill?.amount, | |
| lastRefillAt: null, |
| if (!req.refill.amount) { | ||
| throw new UnkeyApiError({ | ||
| code: "BAD_REQUEST", | ||
| message: "refill.amount must be set if you are using refill.", | ||
| }); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove redundant validation
This validation for refill.amount is redundant as it's already enforced by the schema definition (required field with min(1).positive()).
- if (!req.refill.amount) {
- throw new UnkeyApiError({
- code: "BAD_REQUEST",
- message: "refill.amount must be set if you are using refill.",
- });
- }| @@ -110,7 +110,7 @@ | |||
| }), | |||
| refill: z | |||
| .object({ | |||
| interval: z.enum(["daily", "monthly"]).openapi({ | |||
| interval: z.enum(["daily", "monthly"]).optional().openapi({ | |||
There was a problem hiding this comment.
💡 Codebase verification
Schema consistency needs to be updated across multiple files
The verification reveals inconsistencies in the interval schema definition across different files:
v1_keys_createKey.ts: interval is correctly marked as optionalv1_migrations_enqueueKeys.ts: interval is not optionalv1_keys_updateKey.ts: interval is not optionalschema.ts: interval is correctly marked as optional
These files need to be updated to maintain consistency with the PR objective of removing refillInterval:
apps/api/src/routes/v1_migrations_enqueueKeys.tsapps/api/src/routes/v1_keys_updateKey.ts
🔗 Analysis chain
Verify schema consistency across the codebase
The change to make interval optional aligns with the PR objective. Let's verify this change is consistent across all schema definitions.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining non-optional interval definitions in schemas
# and verify the change is consistent across the codebase
# Search for schema definitions containing interval
rg -A 5 'interval.*enum.*daily.*monthly'
# Search for any remaining refillInterval references
rg 'refillInterval'
Length of output: 10139
What does this PR do?
Removes uses of refillInterval for changes to refill on keys.
Changes will neen to be migrated in the DB.
If (refillInterval is monthly) set refillDay to 1
else set to null
Fixes # (issue)
Type of change
How should this be tested?
Create branch on DB and migrate keys
Use Db to test and make sure there was no breaking changes
Make sure docs were reflected correctly with this change
Checklist
Required
pnpm buildpnpm fmtconsole.logsgit pull origin mainAppreciated
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Refactor