refactor: do not send audit logs to tinybird #2206
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
1 Skipped Deployment
|
|
|
Warning Rate limit exceeded@chronark has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 5 minutes and 14 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📝 Walkthrough📝 Walkthrough📝 WalkthroughWalkthroughThe pull request introduces significant changes across multiple files, primarily focusing on the removal of the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
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: 2
🧹 Outside diff range and nitpick comments (3)
apps/dashboard/lib/trpc/routers/key/createRootKey.ts (1)
3-3: LGTM! Consider removing unused imports.The changes to the import statements are correct and align with the PR objective. The
UnkeyAuditLogis now correctly imported as a type-only import, and theingestAuditLogsTinybirdimport has been removed.Consider removing any unused imports from the file to keep it clean. You can use a linter or IDE features to identify and remove unused imports automatically.
apps/dashboard/lib/tinybird.ts (1)
1-1: Summary of changes and potential impactThe main changes in this file involve the removal of the
ingestAuditLogsTinybirdfunction and its associated imports. This suggests a significant change in how audit logs are handled in the system. While the changes themselves look clean and straightforward, it's crucial to ensure that:
- The removal of this functionality doesn't create any gaps in the audit logging process.
- Any components or services that previously relied on this function have been updated accordingly.
- If this change is part of a larger refactoring effort, all related changes across the codebase have been implemented consistently.
Consider documenting this change in the project's changelog or architecture documentation, explaining the new approach to handling audit logs (if any) and the rationale behind this change.
apps/dashboard/lib/trpc/routers/llmGateway/create.ts~ (1)
71-71: Consider providing a more user-friendly error codeThe error code used is
"PRECONDITION_FAILED", which might not clearly indicate the nature of the error to the client. Consider using a more specific error code like"CONFLICT"to represent a duplicate resource conflict.Apply this diff to adjust the error code:
code: - "PRECONDITION_FAILED", + "CONFLICT",
📜 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 (49)
- apps/dashboard/app/(app)/settings/billing/stripe/success/page.tsx (0 hunks)
- apps/dashboard/app/new/page.tsx (0 hunks)
- apps/dashboard/lib/tinybird.ts (1 hunks)
- apps/dashboard/lib/trpc/routers/api/create.ts (0 hunks)
- apps/dashboard/lib/trpc/routers/api/delete.ts (0 hunks)
- apps/dashboard/lib/trpc/routers/api/updateDeleteProtection.ts (0 hunks)
- apps/dashboard/lib/trpc/routers/api/updateIpWhitelist.ts (0 hunks)
- apps/dashboard/lib/trpc/routers/api/updateName.ts (0 hunks)
- apps/dashboard/lib/trpc/routers/key/create.ts (0 hunks)
- apps/dashboard/lib/trpc/routers/key/createRootKey.ts (1 hunks)
- apps/dashboard/lib/trpc/routers/key/delete.ts (0 hunks)
- apps/dashboard/lib/trpc/routers/key/deleteRootKey.ts (0 hunks)
- apps/dashboard/lib/trpc/routers/key/updateEnabled.ts (0 hunks)
- apps/dashboard/lib/trpc/routers/key/updateExpiration.ts (0 hunks)
- apps/dashboard/lib/trpc/routers/key/updateMetadata.ts (0 hunks)
- apps/dashboard/lib/trpc/routers/key/updateName.ts (0 hunks)
- apps/dashboard/lib/trpc/routers/key/updateOwnerId.ts (0 hunks)
- apps/dashboard/lib/trpc/routers/key/updateRatelimit.ts (0 hunks)
- apps/dashboard/lib/trpc/routers/key/updateRemaining.ts (0 hunks)
- apps/dashboard/lib/trpc/routers/key/updateRootKeyName.ts (0 hunks)
- apps/dashboard/lib/trpc/routers/llmGateway/create.ts (0 hunks)
- apps/dashboard/lib/trpc/routers/llmGateway/create.ts~ (1 hunks)
- apps/dashboard/lib/trpc/routers/llmGateway/delete.ts (0 hunks)
- apps/dashboard/lib/trpc/routers/ratelimit/createNamespace.ts (0 hunks)
- apps/dashboard/lib/trpc/routers/ratelimit/createOverride.ts (0 hunks)
- apps/dashboard/lib/trpc/routers/ratelimit/deleteNamespace.ts (0 hunks)
- apps/dashboard/lib/trpc/routers/ratelimit/deleteOverride.ts (0 hunks)
- apps/dashboard/lib/trpc/routers/ratelimit/updateNamespaceName.ts (0 hunks)
- apps/dashboard/lib/trpc/routers/ratelimit/updateOverride.ts (0 hunks)
- apps/dashboard/lib/trpc/routers/rbac.ts (1 hunks)
- apps/dashboard/lib/trpc/routers/rbac/addPermissionToRootKey.ts (0 hunks)
- apps/dashboard/lib/trpc/routers/rbac/connectRoleToKey.ts (0 hunks)
- apps/dashboard/lib/trpc/routers/rbac/createPermission.ts (0 hunks)
- apps/dashboard/lib/trpc/routers/rbac/createRole.ts (0 hunks)
- apps/dashboard/lib/trpc/routers/rbac/deletePermission.ts (0 hunks)
- apps/dashboard/lib/trpc/routers/rbac/deleteRole.ts (0 hunks)
- apps/dashboard/lib/trpc/routers/rbac/disconnectPermissionFromRole.ts (0 hunks)
- apps/dashboard/lib/trpc/routers/rbac/disconnectRoleFromKey.ts (0 hunks)
- apps/dashboard/lib/trpc/routers/rbac/removePermissionFromRootKey.ts (0 hunks)
- apps/dashboard/lib/trpc/routers/rbac/updatePermission.ts (0 hunks)
- apps/dashboard/lib/trpc/routers/rbac/updateRole.ts (0 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/changeName.ts (1 hunks)
- apps/dashboard/lib/trpc/routers/workspace/changePlan.ts (0 hunks)
- apps/dashboard/lib/trpc/routers/workspace/create.ts (0 hunks)
- apps/dashboard/lib/trpc/routers/workspace/optIntoBeta.ts (0 hunks)
- apps/dashboard/package.json (1 hunks)
- packages/api/src/openapi.d.ts (1 hunks)
💤 Files with no reviewable changes (40)
- apps/dashboard/app/(app)/settings/billing/stripe/success/page.tsx
- apps/dashboard/app/new/page.tsx
- apps/dashboard/lib/trpc/routers/api/create.ts
- apps/dashboard/lib/trpc/routers/api/delete.ts
- apps/dashboard/lib/trpc/routers/api/updateDeleteProtection.ts
- apps/dashboard/lib/trpc/routers/api/updateIpWhitelist.ts
- apps/dashboard/lib/trpc/routers/api/updateName.ts
- apps/dashboard/lib/trpc/routers/key/create.ts
- apps/dashboard/lib/trpc/routers/key/delete.ts
- apps/dashboard/lib/trpc/routers/key/deleteRootKey.ts
- apps/dashboard/lib/trpc/routers/key/updateEnabled.ts
- apps/dashboard/lib/trpc/routers/key/updateExpiration.ts
- apps/dashboard/lib/trpc/routers/key/updateMetadata.ts
- apps/dashboard/lib/trpc/routers/key/updateName.ts
- apps/dashboard/lib/trpc/routers/key/updateOwnerId.ts
- apps/dashboard/lib/trpc/routers/key/updateRatelimit.ts
- apps/dashboard/lib/trpc/routers/key/updateRemaining.ts
- apps/dashboard/lib/trpc/routers/key/updateRootKeyName.ts
- apps/dashboard/lib/trpc/routers/llmGateway/create.ts
- apps/dashboard/lib/trpc/routers/llmGateway/delete.ts
- apps/dashboard/lib/trpc/routers/ratelimit/createNamespace.ts
- apps/dashboard/lib/trpc/routers/ratelimit/createOverride.ts
- apps/dashboard/lib/trpc/routers/ratelimit/deleteNamespace.ts
- apps/dashboard/lib/trpc/routers/ratelimit/deleteOverride.ts
- apps/dashboard/lib/trpc/routers/ratelimit/updateNamespaceName.ts
- apps/dashboard/lib/trpc/routers/ratelimit/updateOverride.ts
- apps/dashboard/lib/trpc/routers/rbac/addPermissionToRootKey.ts
- apps/dashboard/lib/trpc/routers/rbac/connectRoleToKey.ts
- apps/dashboard/lib/trpc/routers/rbac/createPermission.ts
- apps/dashboard/lib/trpc/routers/rbac/createRole.ts
- apps/dashboard/lib/trpc/routers/rbac/deletePermission.ts
- apps/dashboard/lib/trpc/routers/rbac/deleteRole.ts
- apps/dashboard/lib/trpc/routers/rbac/disconnectPermissionFromRole.ts
- apps/dashboard/lib/trpc/routers/rbac/disconnectRoleFromKey.ts
- apps/dashboard/lib/trpc/routers/rbac/removePermissionFromRootKey.ts
- apps/dashboard/lib/trpc/routers/rbac/updatePermission.ts
- apps/dashboard/lib/trpc/routers/rbac/updateRole.ts
- apps/dashboard/lib/trpc/routers/workspace/changePlan.ts
- apps/dashboard/lib/trpc/routers/workspace/create.ts
- apps/dashboard/lib/trpc/routers/workspace/optIntoBeta.ts
🔇 Additional comments (17)
apps/dashboard/lib/trpc/routers/workspace/changeName.ts (1)
Line range hint
1-70: LGTM! Changes align with PR objective.The removal of the
ingestAuditLogsTinybirdfunction call successfully implements the PR objective of not sending audit logs to Tinybird. TheinsertAuditLogsfunction is still in place, ensuring that audit logs are recorded locally.To ensure all Tinybird-related code has been removed, please run the following script:
If any Tinybird-related code or unused imports are found, please remove them to complete the refactoring process.
apps/dashboard/package.json (1)
127-127: Verify the reason for downgrading @types/react-domThe change to downgrade
@types/react-domfrom 18.3.0 to 18.2.25 has been correctly implemented. However, I have a few points to consider:
- This change doesn't seem to relate to the PR objective of "refactor: do not send audit logs to tinybird". Could you clarify the reason for this modification?
- Downgrading type definitions is unusual and could potentially introduce type incompatibilities. Have you verified that this version is compatible with your current
react-dompackage version?- If this change is necessary, consider adding a comment in the
package.jsonfile explaining why this specific version is required to prevent accidental updates in the future.To ensure compatibility, please run the following command and verify that there are no type errors:
If this command doesn't exist, you can add it to your
scriptsinpackage.json:"scripts": { "typecheck": "tsc --noEmit" }This will help catch any potential type incompatibilities introduced by this change.
apps/dashboard/lib/trpc/routers/key/createRootKey.ts (2)
Line range hint
1-200: Summary: Successfully refactored to remove Tinybird integrationThe changes in this file successfully achieve the PR objective of not sending audit logs to Tinybird. The core functionality of creating a root key remains intact, and the refactoring appears to be clean and focused.
Key points:
- Removed Tinybird-related imports and function calls.
- Retained local audit log functionality.
- No apparent issues introduced by these changes.
Great job on this refactoring! The changes are focused and align well with the PR objective. Just ensure that all logging requirements are still met with the remaining audit log functionality.
Line range hint
1-200: Verify logging requirements after removing Tinybird ingestion.The removal of the
ingestAuditLogsTinybird(auditLogs)call aligns with the PR objective. However, it's important to ensure that this change doesn't impact any critical logging or auditing requirements.Please confirm that:
- There are no compliance or security requirements that mandate sending these logs to an external service.
- The remaining audit log functionality (e.g.,
insertAuditLogs) is sufficient for your current needs.Run the following script to check for any remaining references to Tinybird in the codebase:
If any results are found, please review them to ensure complete removal of Tinybird integration or add appropriate comments explaining why they remain.
apps/dashboard/lib/tinybird.ts (2)
1-1: Verify the impact of removingingestAuditLogsTinybirdfunction.The
ingestAuditLogsTinybirdfunction has been removed, which suggests a change in the audit log ingestion process. Please ensure that:
- This removal doesn't break any existing functionality.
- There are no dangling references to this function elsewhere in the codebase.
- If necessary, an alternative method for audit log ingestion has been implemented or this functionality has been moved elsewhere.
Let's check for any remaining references to the removed function:
#!/bin/bash # Search for any remaining references to ingestAuditLogsTinybird rg --type typescript 'ingestAuditLogsTinybird'Additionally, please provide information on how audit logs will be handled going forward, or if this functionality has been deprecated.
3-3: LGTM: Import changes look good.The change to import
unkeyAuditLogEventsas a type and the removal of unused imports (newIdandauditLogSchemaV1) are appropriate given the removal of theingestAuditLogsTinybirdfunction.Let's verify that these imports are not used elsewhere in the file:
✅ Verification successful
LGTM: Import removals are correctly implemented.
The imports
newIdandauditLogSchemaV1are no longer used inapps/dashboard/lib/tinybird.ts, confirming that their removal is appropriate and does not affect the functionality.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that removed imports are not used elsewhere in the file rg --type typescript 'newId|auditLogSchemaV1' apps/dashboard/lib/tinybird.tsLength of output: 218
Script:
#!/bin/bash # Verify that removed imports are not used elsewhere in the file rg 'newId|auditLogSchemaV1' apps/dashboard/lib/tinybird.tsLength of output: 60
apps/dashboard/lib/trpc/routers/vercel.ts (3)
Line range hint
177-196: LGTM: Audit logging system updatedThe changes look good. The
ingestAuditLogsTinybirdcall has been replaced withinsertAuditLogs, maintaining the audit logging functionality while shifting to a different system. The audit log information remains consistent with the previous implementation.
Line range hint
297-319: LGTM: Audit logging system updatedThe changes look good. The
ingestAuditLogsTinybirdcall has been replaced withinsertAuditLogs, maintaining the audit logging functionality while shifting to a different system. The audit log information remains consistent with the previous implementation.
Line range hint
1-621: Overall changes: Audit logging system updated with some inconsistenciesThe changes in this file align with the PR objective of not sending audit logs to Tinybird. The
ingestAuditLogsTinybirdfunction calls have been replaced withinsertAuditLogsin thesetupProjectandupsertApiIdprocedures, maintaining the audit logging functionality while shifting to a different system.However, there are inconsistencies between the AI-generated summary and the actual code changes:
- The summary mentioned changes to
upsertNewRootKey,unbind, anddisconnectProjectprocedures, but no changes are visible in the provided code snippets for these procedures.Please verify if any changes were intended for these procedures and update the code or the summary accordingly.
apps/dashboard/lib/trpc/routers/rbac.ts (6)
4-4: LGTM: Import statement updated correctlyThe import statement has been properly updated to remove
ingestAuditLogsTinybirdand changeUnkeyAuditLogto a type-only import. This change aligns with the PR objective of not sending audit logs to Tinybird.
Line range hint
32-91: LGTM: Audit logging updated correctly inaddPermissionToRootKeyThe
ingestAuditLogsTinybirdcall has been removed, and audit logging is now handled solely byinsertAuditLogs. This change is consistent with the PR objective and maintains proper audit logging within the transaction.
Line range hint
364-437: LGTM: Audit logging updated correctly increateRoleThe
ingestAuditLogsTinybirdcalls have been removed, and audit logging is now handled solely byinsertAuditLogs. This change is consistent with the PR objective and maintains proper audit logging within the transaction. The logic for creating roles and connecting permissions remains intact.
Line range hint
569-604: LGTM: Audit logging updated correctly indeletePermissionThe
ingestAuditLogsTinybirdcall has been removed, and audit logging is now handled solely byinsertAuditLogs. This change is consistent with the PR objective and maintains proper audit logging within the transaction. The logic for deleting permissions remains intact.
Line range hint
485-520: LGTM: Audit logging updated correctly increatePermissionThe
ingestAuditLogsTinybirdcall has been removed, and audit logging is now handled solely byinsertAuditLogs. This change is consistent with the PR objective and maintains proper audit logging within the transaction. The logic for creating permissions remains intact.
Line range hint
1-638: Summary: Successfully removed Tinybird audit loggingThe changes in this file consistently remove all instances of
ingestAuditLogsTinybirdand replace them withinsertAuditLogs. This modification achieves the PR objective of not sending audit logs to Tinybird while maintaining proper audit logging functionality. The changes are applied consistently across all relevant procedures, and the existing logic and data integrity are preserved. All modifications are made within transactions, ensuring data consistency.Great job on implementing these changes consistently and maintaining the overall functionality of the RBAC system!
apps/dashboard/lib/trpc/routers/llmGateway/create.ts~ (1)
67-78: Ensure all database errors are appropriately handledWhile duplicate entries are specifically caught, other potential database errors might not provide sufficient information to the client or might reveal internal details. Ensure that error handling does not expose sensitive information and that the client receives meaningful feedback.
Run the following script to check for unhandled database errors:
✅ Verification successful
All DatabaseError instances are appropriately handled.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for other DatabaseError usages without specific handling. # Expected: All DatabaseError instances are handled appropriately. rg --type js --type ts 'DatabaseError' -A 5Length of output: 5837
packages/api/src/openapi.d.ts (1)
4191-4191:⚠️ Potential issueTypo in the description comment
There is a typographical error in the description on line 4191. The word "identifiy" should be "identify".
Please apply the following fix:
- * @description The name of the key, give keys a name to easily identifiy their purpose + * @description The name of the key, give keys a name to easily identify their purposeLikely invalid or redundant comment.
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (1)
apps/workflows/jobs/refill-daily.ts (1)
64-64: Ensure consistent usage of date objectsIn the audit log insertion,
timeis set usingDate.now(), which returns a timestamp in milliseconds. If thetimefield expects aDateobject, consider usingnew Date()for consistency with other date fields likelastRefillAt.Apply this change if appropriate:
- time: Date.now(), + time: new Date(),
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (8)
- apps/workflows/jobs/create-invoice.ts (0 hunks)
- apps/workflows/jobs/downgrade-requests.ts (0 hunks)
- apps/workflows/jobs/end-trials.ts (0 hunks)
- apps/workflows/jobs/index.ts (0 hunks)
- apps/workflows/jobs/invoicing.ts.disabled (0 hunks)
- apps/workflows/jobs/refill-daily.ts (2 hunks)
- apps/workflows/jobs/refill-monthly.ts (2 hunks)
- apps/workflows/lib/tinybird.ts (0 hunks)
💤 Files with no reviewable changes (6)
- apps/workflows/jobs/create-invoice.ts
- apps/workflows/jobs/downgrade-requests.ts
- apps/workflows/jobs/end-trials.ts
- apps/workflows/jobs/index.ts
- apps/workflows/jobs/invoicing.ts.disabled
- apps/workflows/lib/tinybird.ts
🔇 Additional comments (2)
apps/workflows/jobs/refill-daily.ts (1)
4-4: ImportingnewIdis appropriateThe addition of
newIdfrom@unkey/idis necessary for generating unique identifiers for audit logs.apps/workflows/jobs/refill-monthly.ts (1)
47-85: Good use of transactions to ensure atomicityWrapping the refill operation and audit logging within a database transaction ensures that both updates are performed atomically, which is essential for data integrity.
Summary by CodeRabbit
Release Notes
New Features
createPermission,deletePermission,createRole, anddeleteRole.Bug Fixes
Documentation
Chores
@types/react-domdependency version for improved compatibility.