refactor: add proper logging to key delete#3196
Conversation
ogzhanolguncu
requested review from
chronark,
mcstepp and
perkinsjr
as code owners
|
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
📝 WalkthroughWalkthroughThis update introduces extensive logging enhancements and stricter input validation to two core modules. The Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant deleteKeys Procedure
participant DB
participant AuditLog
Client->>deleteKeys Procedure: Request to delete keys (with keyIds)
deleteKeys Procedure->>deleteKeys Procedure: Validate keyIds (must not be empty)
deleteKeys Procedure->>DB: Find workspace by tenantId and userId
alt Workspace not found
deleteKeys Procedure-->>Client: Throw NOT_FOUND error
else Workspace found
deleteKeys Procedure->>DB: Find keys by keyIds in workspace
alt No keys found
deleteKeys Procedure-->>Client: Throw NOT_FOUND error
else Keys found
deleteKeys Procedure->>DB: Begin transaction
deleteKeys Procedure->>DB: Mark keys as deleted
deleteKeys Procedure->>AuditLog: Insert audit logs for deleted keys
DB-->>deleteKeys Procedure: Commit transaction
deleteKeys Procedure-->>Client: Respond with success
end
end
Note over deleteKeys Procedure: Logs at each stage and on errors
Possibly related PRs
Suggested labels
Suggested reviewers
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 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
🧹 Nitpick comments (2)
apps/dashboard/lib/audit.ts (1)
109-128: Reduce verbosity and avoid leaking identifiers in info-level logs
- These
console.infostatements emit fullworkspaceIds,auditLogIds and rawresources, which might be considered sensitive metadata or breach GDPR/PII guidelines if they end up in shared log collectors.- High-volume endpoints (e.g. bulk key deletion) will generate thousands of lines, potentially throttling STDOUT and slowing the event-loop.
Consider:
- Downgrading most of these to
logger.debug/trace.- Masking or hashing IDs, or logging counts instead of full arrays when the array size is > N.
- Wrapping the whole block with
logger.isLevelEnabled("debug")if the underlying logger supports it.Example:
- console.info({ + logger.debug({ message: "Inserting audit logs", - events: logs.map((log) => log.event), - workspaceIds: [...new Set(logs.map((log) => log.workspaceId))], + eventCount: logs.length, });Also applies to: 146-151, 169-172
apps/dashboard/lib/trpc/routers/key/delete.ts (1)
28-38: Duplicate empty-array guard is unreachable
z.array(...).min(1)already rejects empty arrays, so this runtime check will never execute. Removing it reduces noise and keeps the control-flow lean.- if (keyIds.length === 0) { - console.warn({ ... }); - throw new TRPCError({ - code: "BAD_REQUEST", - message: "No keys were provided for deletion", - }); - }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/dashboard/lib/audit.ts(3 hunks)apps/dashboard/lib/trpc/routers/key/delete.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (14)
- GitHub Check: Test Packages / Test ./apps/dashboard
- GitHub Check: Test Packages / Test ./packages/rbac
- GitHub Check: Test Packages / Test ./packages/nextjs
- GitHub Check: Test Packages / Test ./packages/hono
- GitHub Check: Test Packages / Test ./packages/cache
- GitHub Check: Test Packages / Test ./packages/api
- GitHub Check: Test Packages / Test ./internal/clickhouse
- GitHub Check: Test Packages / Test ./internal/resend
- GitHub Check: Test Packages / Test ./internal/keys
- GitHub Check: Test Packages / Test ./internal/id
- GitHub Check: Test Packages / Test ./internal/hash
- GitHub Check: Test Packages / Test ./internal/billing
- GitHub Check: autofix
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (1)
apps/dashboard/lib/trpc/routers/key/delete.ts (1)
12-13: Good input hardening withmin(1)Requiring at least one key ID at the schema level eliminates empty-payload edge cases before they hit business logic.
|
Can you resolve Rabbit comments first :) cc @ogzhanolguncu |
2 participants
What does this PR do?
This PR adds some proper logging to key deletion tRPC endpoint and audit insertion.
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
Bug Fixes
New Features