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 ↗︎
|
📝 WalkthroughWalkthroughThis pull request updates several type definitions and schema comments across multiple modules. The modifications include introducing new 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 (
|
|
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: 1
🧹 Nitpick comments (2)
internal/db/src/schema/ratelimit.ts (1)
14-19: Consider implementing the suggested timestamp handling.The commented code provides a good pattern for timestamp handling with proper precision. Consider uncommenting and implementing these changes to ensure consistent timestamp management across the codebase.
internal/db/src/schema/keys.ts (1)
51-51: Consider implementing the suggested timestamp handling.The commented timestamp field with millisecond precision aligns with the codebase's timestamp handling pattern. Consider uncommenting and implementing this change for consistent timestamp management.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
apps/api/src/pkg/testutil/harness.ts(2 hunks)apps/api/src/routes/v1_migrations_createKey.ts(2 hunks)apps/api/src/routes/v1_ratelimits_limit.ts(2 hunks)apps/billing/src/lib/keywords.ts(6 hunks)apps/billing/src/trigger/glossary/keyword-research.ts(1 hunks)apps/dashboard/lib/trpc/routers/rbac.ts(3 hunks)internal/db/src/schema/keys.ts(1 hunks)internal/db/src/schema/ratelimit.ts(2 hunks)internal/db/src/schema/rbac.ts(5 hunks)internal/db/src/schema/workspaces.ts(1 hunks)internal/db/src/types.ts(1 hunks)
✅ Files skipped from review due to trivial changes (4)
- apps/billing/src/trigger/glossary/keyword-research.ts
- apps/billing/src/lib/keywords.ts
- internal/db/src/schema/workspaces.ts
- internal/db/src/schema/rbac.ts
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: Test Packages / Test ./internal/clickhouse
- GitHub Check: Test Go API Local / test_agent_local
- GitHub Check: Test Agent Local / test_agent_local
- GitHub Check: Test API / API Test Local
- GitHub Check: Build / Build
- GitHub Check: autofix
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (6)
internal/db/src/types.ts (1)
1-69: LGTM! Well-structured type definitions.The introduction of
Insert*types for each schema model is a good practice. It provides clear type separation between read and write operations, enhancing type safety and making the codebase more maintainable.apps/api/src/pkg/testutil/harness.ts (2)
8-18: LGTM! Good type safety in test utilities.The addition of
InsertPermissiontype and its usage in the test harness maintains type safety and aligns with the new type system.
108-117: LGTM! Proper use of insert type.The change from
Permission[]toInsertPermission[]correctly reflects the type system changes and maintains type safety in test data creation.apps/api/src/routes/v1_ratelimits_limit.ts (1)
9-9: Type changes align with insertion operation requirements.The change from
RatelimitNamespacetoInsertRatelimitNamespacebetter reflects the intent of creating a new namespace record. This type specificity helps prevent accidental misuse of fields that should only be present after insertion.Also applies to: 188-188
apps/api/src/routes/v1_migrations_createKey.ts (1)
10-14: Consistent application of Insert types for database operations.*The changes properly implement the Insert* type pattern across all relevant type declarations, improving type safety during insertion operations. The array declarations now correctly reflect that these are collections of entities being prepared for insertion rather than existing records.
Also applies to: 319-323
apps/dashboard/lib/trpc/routers/rbac.ts (1)
1-1: Type refinements improve clarity and consistency.The changes enhance type safety and readability:
- Import statement now correctly includes both Permission and InsertPermission types
- newPermissions array properly typed as InsertPermission[]
- Explicit Permission type annotation adds clarity to the object structure
Also applies to: 765-765, 775-775
| // createdAt: datetime("created_at", { mode: "date", fsp: 3 }) | ||
| // .notNull() | ||
| // .default(sql`CURRENT_TIMESTAMP(3)`) | ||
| // .$defaultFn(() => new Date()), | ||
| // updatedAt: datetime("updated_at", { mode: "date", fsp: 3 }).$onUpdateFn(() => new Date()), | ||
| // updatedAt: datetime("updated_at", { mode: "date", fsp: 3 }), | ||
| // deletedAt: datetime("deleted_at", { mode: "date", fsp: 3 }), | ||
| ...lifecycleDatesMigration, |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify timestamp handling consistency.
The commented timestamp handling in ratelimitOverrides should match the pattern in ratelimitNamespaces. Consider implementing both to maintain consistency.
Run this script to check timestamp handling across the codebase:
🏁 Script executed:
#!/bin/bash
# Description: Check timestamp handling patterns across schema files
# Search for timestamp-related patterns in schema files
rg -A 3 'CURRENT_TIMESTAMP\(3\)'
rg -A 3 '\$defaultFn\(\(\) => new Date\(\)\)'
rg -A 3 '\$onUpdateFn\(\(\) => new Date\(\)\)'Length of output: 7164
Timestamp Handling Consistency in Schema Files
In file internal/db/src/schema/ratelimit.ts, the commented-out timestamp logic (using patterns like $defaultFn(() => new Date()) and $onUpdateFn(() => new Date())) does not currently align with the pattern used in other parts of the codebase (for example, the SQL-based CURRENT_TIMESTAMP(3) approach seen in files such as apps/billing/src/lib/keywords.ts). Please review and update the timestamp handling so that the implementation in ratelimitOverrides mirrors that in ratelimitNamespaces—whether that means uncommenting and adapting the current logic or switching entirely to the SQL default pattern. This adjustment will help ensure a uniform, predictable behavior for timestamp fields across our schemas.
1 participant
Summary by CodeRabbit
This update brings behind‐the-scenes improvements that optimize system reliability and data consistency while preserving current functionality.