Impersonation log #2896
Impersonation log #2896
Conversation
|
WalkthroughAdds impersonation audit support and a docs tweak. Changes: wording updates in CONTRIBUTING.md; new Prisma enum and model ImpersonationAuditLog plus relations on User; SQL migration creating the ImpersonationAuditLog table and indexes/foreign keys; server utility Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ❌ 3❌ Failed checks (2 warnings, 1 inconclusive)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 📜 Recent review detailsConfiguration used: Repository UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used🧠 Learnings (4)📚 Learning: 2026-01-15T11:50:06.044ZApplied to files:
📚 Learning: 2025-11-27T16:26:37.432ZApplied to files:
📚 Learning: 2026-01-15T11:50:06.044ZApplied to files:
📚 Learning: 2025-11-27T16:26:58.661ZApplied to files:
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (23)
🔇 Additional comments (2)
✏️ Tip: You can disable this entire section by setting 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. Comment |
Review CompleteYour review story is ready! Comment !reviewfast on this PR to re-generate the story. |
![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
🤖 Fix all issues with AI agents
In `@apps/webapp/app/models/admin.server.ts`:
- Around line 13-17: The extractClientIp function currently returns the last
element of X-Forwarded-For, which picks a proxy IP instead of the original
client; change the logic in extractClientIp to return the first (leftmost)
trimmed element (parts[0]) after splitting on commas and keep the null guard,
and update the identical function in
apps/webapp/app/routes/login.magic/route.tsx so both places consistently return
the original client IP from X-Forwarded-For.
🧹 Nitpick comments (2)
CONTRIBUTING.md (1)
95-95: Bare URL flagged by markdownlint.The static analysis tool flagged this bare URL. Consider wrapping it in angle brackets or using markdown link syntax for consistency.
-1. Visit http://localhost:3030 in your browser and create a new project called "hello-world". +1. Visit <http://localhost:3030> in your browser and create a new project called "hello-world".internal-packages/database/prisma/migrations/20260108164613_add_impersonation_audit_log/migration.sql (1)
16-23: Indexes should useCONCURRENTLYand be in a separate migration file.Based on learnings from the repository coding guidelines, database indexes must use
CONCURRENTLYto avoid table locks and must be in their own separate migration file.While this is a new table (so locking concerns are minimal for the initial deployment), following the established pattern ensures consistency and avoids issues if this migration runs against a database that already has data in related tables.
Consider splitting this migration:
Migration 1 (current file): Table creation, enum, and foreign keys only
Migration 2 (new file): Indexes withCONCURRENTLY-- CreateIndex CREATE INDEX CONCURRENTLY "ImpersonationAuditLog_adminId_idx" ON "public"."ImpersonationAuditLog"("adminId"); -- CreateIndex CREATE INDEX CONCURRENTLY "ImpersonationAuditLog_targetId_idx" ON "public"."ImpersonationAuditLog"("targetId"); -- CreateIndex CREATE INDEX CONCURRENTLY "ImpersonationAuditLog_createdAt_idx" ON "public"."ImpersonationAuditLog"("createdAt");
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
CONTRIBUTING.mdapps/webapp/app/models/admin.server.tsinternal-packages/database/prisma/migrations/20260108164613_add_impersonation_audit_log/migration.sqlinternal-packages/database/prisma/schema.prisma
🧰 Additional context used
📓 Path-based instructions (8)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{ts,tsx}: Use types over interfaces for TypeScript
Avoid using enums; prefer string unions or const objects instead
**/*.{ts,tsx}: Always import tasks from@trigger.dev/sdk, never use@trigger.dev/sdk/v3or deprecatedclient.defineJobpattern
Every Trigger.dev task must be exported and have a uniqueidproperty with no timeouts in the run function
Files:
apps/webapp/app/models/admin.server.ts
{packages/core,apps/webapp}/**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use zod for validation in packages/core and apps/webapp
Files:
apps/webapp/app/models/admin.server.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use function declarations instead of default exports
Import from
@trigger.dev/coreusing subpaths only, never import from root
Files:
apps/webapp/app/models/admin.server.ts
apps/webapp/app/**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/webapp.mdc)
Access all environment variables through the
envexport ofenv.server.tsinstead of directly accessingprocess.envin the Trigger.dev webapp
Files:
apps/webapp/app/models/admin.server.ts
apps/webapp/**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/webapp.mdc)
apps/webapp/**/*.{ts,tsx}: When importing from@trigger.dev/corein the webapp, use subpath exports from the package.json instead of importing from the root path
Follow the Remix 2.1.0 and Express server conventions when updating the main trigger.dev webappAccess environment variables via
envexport fromapps/webapp/app/env.server.ts, never useprocess.envdirectly
Files:
apps/webapp/app/models/admin.server.ts
**/*.ts
📄 CodeRabbit inference engine (.cursor/rules/otel-metrics.mdc)
**/*.ts: When creating or editing OTEL metrics (counters, histograms, gauges), ensure metric attributes have low cardinality by using only enums, booleans, bounded error codes, or bounded shard IDs
Do not use high-cardinality attributes in OTEL metrics such as UUIDs/IDs (envId, userId, runId, projectId, organizationId), unbounded integers (itemCount, batchSize, retryCount), timestamps (createdAt, startTime), or free-form strings (errorMessage, taskName, queueName)
When exporting OTEL metrics via OTLP to Prometheus, be aware that the exporter automatically adds unit suffixes to metric names (e.g., 'my_duration_ms' becomes 'my_duration_ms_milliseconds', 'my_counter' becomes 'my_counter_total'). Account for these transformations when writing Grafana dashboards or Prometheus queries
Files:
apps/webapp/app/models/admin.server.ts
**/*.{js,ts,jsx,tsx,json,md,yaml,yml}
📄 CodeRabbit inference engine (AGENTS.md)
Format code using Prettier before committing
Files:
apps/webapp/app/models/admin.server.tsCONTRIBUTING.md
internal-packages/database/prisma/migrations/**/*.sql
📄 CodeRabbit inference engine (CLAUDE.md)
internal-packages/database/prisma/migrations/**/*.sql: When editing the Prisma schema, remove extraneous migration lines related to specific tables:_BackgroundWorkerToBackgroundWorkerFile,_BackgroundWorkerToTaskQueue,_TaskRunToTaskRunTag,_WaitpointRunConnections,_completedWaitpoints,SecretStore_key_idx, and unrelatedTaskRunindexes
Database indexes must use CONCURRENTLY to avoid table locks and must be in their own separate migration file
Files:
internal-packages/database/prisma/migrations/20260108164613_add_impersonation_audit_log/migration.sql
🧠 Learnings (3)
📚 Learning: 2026-01-15T11:50:06.044Z
Learnt from: CR
Repo: triggerdotdev/trigger.dev PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-15T11:50:06.044Z
Learning: Applies to internal-packages/database/prisma/migrations/**/*.sql : When editing the Prisma schema, remove extraneous migration lines related to specific tables: `_BackgroundWorkerToBackgroundWorkerFile`, `_BackgroundWorkerToTaskQueue`, `_TaskRunToTaskRunTag`, `_WaitpointRunConnections`, `_completedWaitpoints`, `SecretStore_key_idx`, and unrelated `TaskRun` indexes
Applied to files:
internal-packages/database/prisma/schema.prismainternal-packages/database/prisma/migrations/20260108164613_add_impersonation_audit_log/migration.sql
📚 Learning: 2025-11-27T16:26:37.432Z
Learnt from: CR
Repo: triggerdotdev/trigger.dev PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-11-27T16:26:37.432Z
Learning: Applies to internal-packages/database/**/*.{ts,tsx} : Use Prisma for database interactions in internal-packages/database with PostgreSQL
Applied to files:
internal-packages/database/prisma/schema.prismainternal-packages/database/prisma/migrations/20260108164613_add_impersonation_audit_log/migration.sql
📚 Learning: 2026-01-15T11:50:06.044Z
Learnt from: CR
Repo: triggerdotdev/trigger.dev PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-15T11:50:06.044Z
Learning: Applies to internal-packages/database/prisma/migrations/**/*.sql : Database indexes must use CONCURRENTLY to avoid table locks and must be in their own separate migration file
Applied to files:
internal-packages/database/prisma/migrations/20260108164613_add_impersonation_audit_log/migration.sql
🧬 Code graph analysis (1)
apps/webapp/app/models/admin.server.ts (2)
apps/webapp/app/services/session.server.ts (1)
requireUser(39-62)apps/webapp/app/services/impersonation.server.ts (1)
getImpersonationId(24-28)
🪛 markdownlint-cli2 (0.18.1)
CONTRIBUTING.md
95-95: Bare URL used
(MD034, no-bare-urls)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (5)
CONTRIBUTING.md (1)
130-130: LGTM!The documentation updates removing "V3" and "in v3" references simplify the instructions appropriately.
internal-packages/database/prisma/schema.prisma (2)
62-64: LGTM!The new relations for impersonation tracking are correctly defined with named relations (
ImpersonationAdminandImpersonationTarget) to disambiguate the two User references in the ImpersonationAuditLog model.
2390-2415: LGTM!The
ImpersonationAuditLogmodel is well-structured:
- Appropriate enum for action types
- Cascade delete/update ensures cleanup when users are removed
- Indexes on
adminId,targetId, andcreatedAtsupport common query patterns for audit log retrievalapps/webapp/app/models/admin.server.ts (2)
226-237: Audit logging failure doesn't block impersonation — verify this is acceptable.The try-catch ensures impersonation proceeds even if audit logging fails. While this prevents operational disruption, audit logs for privileged actions like impersonation are often compliance-critical.
Consider whether a failed audit log should:
- Block the impersonation action entirely (stricter security posture)
- Proceed with a more severe alert (current approach logs error but could also trigger an alert)
If the current behavior is intentional, this is fine as-is.
246-266: LGTM!The STOP audit logging correctly:
- Retrieves the admin user via
requireUser(the actual logged-in admin, not the impersonated user)- Checks
targetIdexists before attempting to log- Uses the same error handling pattern as START for consistency
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@internal-packages/database/prisma/schema.prisma`:
- Around line 2397-2422: The Prisma model ImpersonationAuditLog is missing the
createdAt index declaration leading to schema/db drift; update the
ImpersonationAuditLog model to add an @@index on createdAt (i.e., add
@@index([createdAt]) alongside the existing @@index([adminId]) and
@@index([targetId])) so the Prisma schema matches the migration that created
"ImpersonationAuditLog_createdAt_idx".
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
CONTRIBUTING.mdinternal-packages/database/prisma/schema.prisma
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{js,ts,jsx,tsx,json,md,yaml,yml}
📄 CodeRabbit inference engine (AGENTS.md)
Format code using Prettier before committing
Files:
CONTRIBUTING.md
🧠 Learnings (2)
📚 Learning: 2026-01-15T11:50:06.044Z
Learnt from: CR
Repo: triggerdotdev/trigger.dev PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-15T11:50:06.044Z
Learning: Applies to internal-packages/database/prisma/migrations/**/*.sql : When editing the Prisma schema, remove extraneous migration lines related to specific tables: `_BackgroundWorkerToBackgroundWorkerFile`, `_BackgroundWorkerToTaskQueue`, `_TaskRunToTaskRunTag`, `_WaitpointRunConnections`, `_completedWaitpoints`, `SecretStore_key_idx`, and unrelated `TaskRun` indexes
Applied to files:
internal-packages/database/prisma/schema.prisma
📚 Learning: 2025-11-27T16:26:37.432Z
Learnt from: CR
Repo: triggerdotdev/trigger.dev PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-11-27T16:26:37.432Z
Learning: Applies to internal-packages/database/**/*.{ts,tsx} : Use Prisma for database interactions in internal-packages/database with PostgreSQL
Applied to files:
internal-packages/database/prisma/schema.prisma
🪛 markdownlint-cli2 (0.18.1)
CONTRIBUTING.md
95-95: Bare URL used
(MD034, no-bare-urls)
🔇 Additional comments (3)
CONTRIBUTING.md (2)
95-95: Documentation update looks good.The removal of "V3" from the project creation instruction is appropriate.
Note: The static analysis tool flagged a bare URL (
http://localhost:3030). Consider wrapping it for consistency:<http://localhost:3030>or[http://localhost:3030](http://localhost:3030). This is pre-existing and minor—can be addressed separately.
130-130: LGTM!Clean removal of the outdated "in v3" reference.
internal-packages/database/prisma/schema.prisma (1)
62-65: LGTM!The new relations for tracking impersonation audit logs are well-named and correctly reference the
ImpersonationAuditLogmodel.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
Closes #
✅ Checklist
Testing
[Describe the steps you took to test this change]
Changelog
[Short description of what has changed]
Screenshots
[Screenshots]
💯