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 ↗︎
|
|
|
Thank you for following the naming conventions for pull request titles! 🙏 |
📝 WalkthroughWalkthroughThis pull request extends lifecycle tracking across multiple parts of the codebase. It adds three new timestamp properties— Changes
Possibly related PRs
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (6)
tools/migrate/main.ts (1)
10-21: Iterate over tables consistently.
The loop enumerates a static list of tables for the migration. If future migrations add new tables requiring the same logic, consider dynamic enumeration to reduce maintenance overhead.apps/dashboard/lib/trpc/routers/key/deleteRootKey.ts (1)
31-31: Maintain consistent time source.
Usingnew Date()andDate.now()ensures partial consistency, but be mindful of potential discrepancies (e.g., if you need strict ordering, use a single source of truth).apps/dashboard/lib/trpc/routers/key/delete.ts (1)
48-48: LGTM! Consider adding type safety for the timestamp fields.The addition of
deletedAtMalongsidedeletedAtprovides both timezone-aware dates and precise chronological ordering capabilities.Consider defining a shared type for lifecycle dates to ensure consistency:
type LifecycleDates = { deletedAt: Date; deletedAtM: number; };apps/dashboard/lib/trpc/routers/api/delete.ts (1)
48-48: LGTM! Consider extracting the lifecycle date updates into a helper.The implementation correctly handles both API and associated keys deletion within the same transaction.
Consider creating a helper function to reduce duplication:
const setDeletedTimestamps = { deletedAt: new Date(), deletedAtM: Date.now() }; // Usage .set(setDeletedTimestamps)Also applies to: 78-78
apps/dashboard/lib/trpc/routers/vercel.ts (1)
528-528: LGTM! Consider using a type-safe approach for the deletion timestamps.The addition of
deletedAtMalongsidedeletedAtenhances timestamp tracking with millisecond precision.Consider creating a shared type or interface for deletion timestamps to ensure consistency:
interface DeletionTimestamps { deletedAt: Date; deletedAtM: number; }Then use it in your schema definitions:
const vercelBindings = { // ... other fields ...deletionTimestamps }Also applies to: 585-585
apps/api/src/routes/v1_keys_verifyKey.test.ts (1)
830-830: LGTM! Consider adding a test helper for soft deletion.The addition of
deletedAtMalongsidedeletedAtconsistently implements the new deletion timestamp pattern across test cases.Consider creating a test helper function to reduce duplication and ensure consistency:
async function softDelete(tx: typeof h.db.primary, table: any, id: string) { await tx .update(table) .set({ deletedAt: new Date(), deletedAtM: Date.now() }) .where(eq(table.id, id)); } // Usage in tests: await softDelete(h.db.primary, schema.keys, key.keyId); await softDelete(h.db.primary, schema.keyAuth, h.resources.userKeyAuth.id); await softDelete(h.db.primary, schema.apis, h.resources.userApi.id);Also applies to: 856-856, 882-882
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (21)
apps/api/src/pkg/testutil/harness.ts(7 hunks)apps/api/src/routes/v1_apis_deleteApi.ts(2 hunks)apps/api/src/routes/v1_apis_deleteKeys.ts(1 hunks)apps/api/src/routes/v1_keys_deleteKey.ts(1 hunks)apps/api/src/routes/v1_keys_verifyKey.test.ts(3 hunks)apps/api/src/routes/v1_migrations_createKey.ts(3 hunks)apps/api/src/routes/v1_ratelimits_deleteOverride.ts(1 hunks)apps/dashboard/app/api/v1/vercel/integration/route.ts(1 hunks)apps/dashboard/lib/trpc/routers/api/delete.ts(2 hunks)apps/dashboard/lib/trpc/routers/key/delete.ts(1 hunks)apps/dashboard/lib/trpc/routers/key/deleteRootKey.ts(1 hunks)apps/dashboard/lib/trpc/routers/ratelimit/deleteNamespace.ts(2 hunks)apps/dashboard/lib/trpc/routers/ratelimit/deleteOverride.ts(1 hunks)apps/dashboard/lib/trpc/routers/rbac/upsertPermission.ts(1 hunks)apps/dashboard/lib/trpc/routers/vercel.ts(2 hunks)apps/dashboard/lib/trpc/routers/workspace/create.ts(1 hunks)internal/db/src/schema/keys.ts(2 hunks)internal/db/src/schema/rbac.ts(5 hunks)internal/db/src/schema/util/lifecycle_dates.ts(1 hunks)internal/db/src/schema/workspaces.ts(2 hunks)tools/migrate/main.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- apps/dashboard/lib/trpc/routers/rbac/upsertPermission.ts
- apps/api/src/pkg/testutil/harness.ts
- apps/api/src/routes/v1_migrations_createKey.ts
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Test Agent Local / test_agent_local
- GitHub Check: Test API / API Test Local
🔇 Additional comments (21)
apps/dashboard/lib/trpc/routers/workspace/create.ts (1)
51-53: Consider timestamp format consistency.The workspace object now has two sets of similar timestamp fields with different formats:
createdAt/deletedAtusingDateobjectscreatedAtM/deletedAtMusing millisecondsThis could lead to confusion and potential bugs when handling dates.
Could you clarify:
- Why both formats are needed?
- If they serve different purposes, should this be documented?
- If they're part of a migration, what's the plan to consolidate them?
If these are indeed part of a migration as suggested by the PR title, consider:
- createdAtM: Date.now(), + // TODO: Migration - Remove old timestamp fields (createdAt, deletedAt) once migration is complete + createdAtM: Date.now(),tools/migrate/main.ts (1)
22-28: Check for potential edge cases.
If no rows havecreatedAtMas null, the code simply logs{ count: 0 }. This is expected, but ensure that any downstream logic (like reporting or validation) doesn't assume rows will be processed.internal/db/src/schema/util/lifecycle_dates.ts (1)
10-16: Clarify default value usage.
Defining both.default(0)and.$defaultFn(() => Date.now())may introduce ambiguity when the function isn't triggered. Decide if zero is truly acceptable as a fallback, or consider removing.default(0)if you always intend to use the function.apps/dashboard/app/api/v1/vercel/integration/route.ts (1)
37-37: LGTM! Consistent implementation of lifecycle dates.The addition of
deletedAtMis consistently applied across all removal cases, maintaining data integrity during the deletion process.Also applies to: 44-44, 48-48
apps/dashboard/lib/trpc/routers/ratelimit/deleteOverride.ts (1)
50-50: LGTM! Well-structured implementation with proper error handling.The addition of
deletedAtMis properly implemented within the transaction, maintaining consistency with other deletion operations.apps/dashboard/lib/trpc/routers/ratelimit/deleteNamespace.ts (2)
42-42: LGTM! Consistent timestamp tracking added.The addition of
deletedAtMalongsidedeletedAtenhances timestamp precision while maintaining backward compatibility.
73-73: LGTM! Cascading deletion timestamp tracking.Consistent implementation of timestamp tracking for related overrides within the same transaction.
apps/api/src/routes/v1_apis_deleteKeys.ts (1)
120-124: LGTM! Proper transaction handling with enhanced timestamp tracking.The implementation correctly:
- Tracks the count of affected keys
- Updates both timestamp fields atomically
- Maintains proper transaction boundaries
internal/db/src/schema/workspaces.ts (2)
21-21: LGTM! Clean import of lifecycle dates migration.Proper modularization of the lifecycle dates schema.
110-110: LGTM! Consistent application of lifecycle dates.The spread operator ensures uniform application of lifecycle date fields across the workspace schema.
apps/api/src/routes/v1_ratelimits_deleteOverride.ts (1)
110-110: LGTM! Consistent timestamp tracking implementation.The change maintains consistency with other deletion operations across the codebase, properly tracking both date and millisecond precision timestamps within a transaction.
apps/api/src/routes/v1_apis_deleteApi.ts (2)
98-101: LGTM! Soft delete operation enhanced with millisecond precision.The update operation now includes both
deletedAtanddeletedAtMfields, providing better timestamp granularity.
128-131: LGTM! Consistent timestamp handling across tables.The update operation for the
keystable follows the same pattern, ensuring consistent timestamp handling.apps/api/src/routes/v1_keys_deleteKey.ts (1)
134-137: LGTM! Soft delete operation enhanced with millisecond precision.The update operation now includes both
deletedAtanddeletedAtMfields, providing better timestamp granularity.internal/db/src/schema/keys.ts (2)
18-18: LGTM! Import updated to include new lifecycle dates schema.The import now includes both the migration and v2 lifecycle dates schemas, enabling gradual migration.
152-152: LGTM! Migrated to new lifecycle dates schema.The
encryptedKeystable now uses the newlifecycleDatesV2schema, while maintaining backward compatibility in other tables.internal/db/src/schema/rbac.ts (5)
28-32: LGTM! Added millisecond precision timestamps to permissions table.The implementation correctly handles both creation and update timestamps with millisecond precision.
72-76: LGTM! Added millisecond precision timestamps to keysPermissions table.The implementation follows the same pattern, ensuring consistent timestamp handling.
115-119: LGTM! Added millisecond precision timestamps to roles table.The implementation maintains consistency with other tables.
161-165: LGTM! Added millisecond precision timestamps to rolesPermissions table.The implementation follows the established pattern.
210-214: LGTM! Added millisecond precision timestamps to keysRoles table.The implementation completes the consistent timestamp handling across all RBAC tables.
| let processed = 0; | ||
| let cursor = ""; | ||
| do { | ||
| const rows = await db | ||
| .select() | ||
| .from(table) | ||
| .where( | ||
| and( | ||
| isNull(table.createdAtM), | ||
|
|
||
| gt(table.id, cursor), | ||
| ), | ||
| ) | ||
| .orderBy(asc(table.id)) | ||
| .limit(100); | ||
|
|
||
| cursor = rows.at(-1)?.id ?? ""; | ||
| console.info({ cursor, rows: rows.length, processed, count }); | ||
|
|
||
| await Promise.all( | ||
| rows.map(async (row) => { | ||
| await db | ||
| .update(table) | ||
| .set({ | ||
| createdAtM: new Date(row.createdAt ?? 0).getTime() ?? 0, | ||
| updatedAtM: "updatedAt" in row ? row.updatedAt?.getTime() ?? null : null, | ||
| deletedAtM: row.deletedAt?.getTime() ?? null, | ||
| }) | ||
| .where(eq(table.id, row.id)); | ||
| }), | ||
| ); | ||
| processed += rows.length; | ||
| } while (cursor); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Optimize bulk updates.
Updating rows individually in a Promise.all can be slow under significant loads. Consider batching updates using a single SQL statement if supported by your schema to reduce overhead.
- await Promise.all(
- rows.map(async (row) => {
- await db
- .update(table)
- .set({
- createdAtM: new Date(row.createdAt ?? 0).getTime() ?? 0,
- updatedAtM: "updatedAt" in row ? row.updatedAt?.getTime() ?? null : null,
- deletedAtM: row.deletedAt?.getTime() ?? null,
- })
- .where(eq(table.id, row.id));
- }),
- );
+ // Example: gather IDs and prepared values, then execute a bulk update
+ const updateData = rows.map((row) => ({
+ id: row.id,
+ createdAtM: new Date(row.createdAt ?? 0).getTime() ?? 0,
+ updatedAtM: "updatedAt" in row ? row.updatedAt?.getTime() ?? null : null,
+ deletedAtM: row.deletedAt?.getTime() ?? null,
+ }));
+
+ // Pseudocode: pass updateData to a single update statement if feasible
+ // attn: Implementation may vary depending on your DB/table structureCommittable suggestion skipped: line range outside the PR's diff.
1 participant
Summary by CodeRabbit
New Features
Refactor