feat: UX and operations improvements batch#76
Conversation
Add 10 new event-based AlertMetric enum values (deploy_requested, deploy_completed, deploy_rejected, deploy_cancelled, new_version_available, scim_sync_failed, backup_failed, certificate_expiring, node_joined, node_left) for alerts that fire on occurrence rather than polling. Make condition, threshold, and durationSeconds nullable on AlertRule so event-based rules can omit threshold configuration. Add null guards in the alert evaluator to skip event-based rules during polling, and update the UI and API to handle nullable fields.
github-actions
bot
added
documentation
Greptile SummaryThis PR delivers five significant features: user/team default environment preferences (with server-side persistence and localStorage caching), a split settings sidebar with animated navigation and role-based visibility, decoupled deploy approvals (approve and deploy as separate actions with a new Key issues found:
Confidence Score: 3/5
Last reviewed commit: 1eb09ca |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| set: protectedProcedure | ||
| .input(z.object({ key: z.string().max(100), value: z.string().max(500) })) | ||
| .mutation(async ({ ctx, input }) => { | ||
| await prisma.userPreference.upsert({ | ||
| where: { | ||
| userId_key: { userId: ctx.session.user!.id!, key: input.key }, | ||
| }, | ||
| create: { userId: ctx.session.user!.id!, key: input.key, value: input.value }, | ||
| update: { value: input.value }, | ||
| }); | ||
| }), | ||
|
|
||
| delete: protectedProcedure | ||
| .input(z.object({ key: z.string().max(100) })) | ||
| .mutation(async ({ ctx, input }) => { | ||
| await prisma.userPreference.deleteMany({ | ||
| where: { userId: ctx.session.user!.id!, key: input.key }, | ||
| }); | ||
| }), |
There was a problem hiding this comment.
Mutations missing withAudit middleware
The set and delete mutations don't use the withAudit middleware, which is required on every mutating procedure in the codebase. This leaves preference changes unlogged in the audit trail. Compare with team.updateDefaultEnvironment (also added in this PR), which correctly includes the middleware.
Add .use(withAudit("userPreference.set", "UserPreference")) before the .mutation() call on the set procedure (line 13), and .use(withAudit("userPreference.delete", "UserPreference")) before the .mutation() call on the delete procedure (line 25).
Rule Used: ## Security & Cryptography Review Rules
When revi... (source)
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/server/routers/user-preference.ts
Line: 13-31
Comment:
**Mutations missing `withAudit` middleware**
The `set` and `delete` mutations don't use the `withAudit` middleware, which is required on every mutating procedure in the codebase. This leaves preference changes unlogged in the audit trail. Compare with `team.updateDefaultEnvironment` (also added in this PR), which correctly includes the middleware.
Add `.use(withAudit("userPreference.set", "UserPreference"))` before the `.mutation()` call on the `set` procedure (line 13), and `.use(withAudit("userPreference.delete", "UserPreference"))` before the `.mutation()` call on the `delete` procedure (line 25).
**Rule Used:** ## Security & Cryptography Review Rules
When revi... ([source](https://app.greptile.com/review/custom-context?memory=7cb20c56-ca6a-40aa-8660-7fa75e6e3db2))
How can I resolve this? If you propose a fix, please make it concise.
|
@greptile review |
…per-rule alert delivery
|
@greptile review |
| set: protectedProcedure | ||
| .input(z.object({ key: z.string().max(100), value: z.string().max(500) })) | ||
| .mutation(async ({ ctx, input }) => { | ||
| await prisma.userPreference.upsert({ | ||
| where: { | ||
| userId_key: { userId: ctx.session.user!.id!, key: input.key }, | ||
| }, | ||
| create: { userId: ctx.session.user!.id!, key: input.key, value: input.value }, | ||
| update: { value: input.value }, | ||
| }); | ||
| }), |
There was a problem hiding this comment.
The set and delete mutations are missing the withAudit middleware. Every mutating procedure in this codebase must use audit middleware to log changes to the audit trail. Without it, preference changes are unlogged and untracked.
Add the middleware to both mutations by importing withAudit from the TRPC init file and wrapping each mutation with .use(withAudit(...)) before the .mutation() call, following the same pattern as team.updateDefaultEnvironment elsewhere in the codebase.
Rule Used: ## Code Style & Conventions
TypeScript Conven... (source)
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/server/routers/user-preference.ts
Line: 13-23
Comment:
The `set` and `delete` mutations are missing the `withAudit` middleware. Every mutating procedure in this codebase must use audit middleware to log changes to the audit trail. Without it, preference changes are unlogged and untracked.
Add the middleware to both mutations by importing `withAudit` from the TRPC init file and wrapping each mutation with `.use(withAudit(...))` before the `.mutation()` call, following the same pattern as `team.updateDefaultEnvironment` elsewhere in the codebase.
**Rule Used:** ## Code Style & Conventions
### TypeScript Conven... ([source](https://app.greptile.com/review/custom-context?memory=6ae51394-d0b6-4686-bc4c-1ad840c2e310))
How can I resolve this? If you propose a fix, please make it concise.
| @@ -372,8 +411,17 @@ export const deployRouter = router({ | |||
| request.configYaml, | |||
| ); | |||
There was a problem hiding this comment.
Deployment attributed to original requester instead of executor
deployAgent is called with request.requestedById ?? ctx.session.user.id — this uses the original requester's ID as the deployer instead of the actual executor's ID. The fallback masks the primary issue: when request.requestedById is set (which it always is for approved requests), it becomes the deployer. This means the PipelineVersion snapshot, audit log entries, and internal deploy records will all attribute the deployment to the person who requested it, not the person who clicked "Deploy Now".
The DeployRequest.deployedById is already correctly set to ctx.session.user.id on line 390, so the two records will disagree about who executed the deployment. The fix is to always pass the executor's ID:
| const result = await deployAgent( | |
| request.pipelineId, | |
| ctx.session.user.id, | |
| request.changelog, | |
| request.configYaml, | |
| ); |
Rule Used: ## Pipeline Editor & Deployment Review Guide
The ... (source)
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/server/routers/deploy.ts
Line: 407-412
Comment:
**Deployment attributed to original requester instead of executor**
`deployAgent` is called with `request.requestedById ?? ctx.session.user.id` — this uses the original requester's ID as the deployer instead of the actual executor's ID. The fallback masks the primary issue: when `request.requestedById` is set (which it always is for approved requests), it becomes the deployer. This means the `PipelineVersion` snapshot, audit log entries, and internal deploy records will all attribute the deployment to the person who *requested* it, not the person who clicked "Deploy Now".
The `DeployRequest.deployedById` is already correctly set to `ctx.session.user.id` on line 390, so the two records will disagree about who executed the deployment. The fix is to always pass the executor's ID:
```suggestion
const result = await deployAgent(
request.pipelineId,
ctx.session.user.id,
request.changelog,
request.configYaml,
);
```
**Rule Used:** ## Pipeline Editor & Deployment Review Guide
The ... ([source](https://app.greptile.com/review/custom-context?memory=99b9234e-e536-4295-b97d-d10499542ee1))
How can I resolve this? If you propose a fix, please make it concise.| // Fetch the full request to get configYaml, pipelineId, changelog | ||
| const request = await prisma.deployRequest.findUnique({ | ||
| where: { id: input.requestId }, | ||
| }); | ||
| if (!request) { | ||
| throw new TRPCError({ code: "NOT_FOUND", message: "Deploy request not found" }); | ||
| } |
There was a problem hiding this comment.
Status stuck at DEPLOYED if record deleted between atomic update and fetch
The code atomically sets the request to DEPLOYED (line 388–391), then immediately tries to fetch the full record (line 397–401). If the record is deleted between these operations — possible through a concurrent admin operation — the findUnique returns null and the procedure throws NOT_FOUND without reverting the DEPLOYED status. The request is left permanently in DEPLOYED state with no recovery path.
The two revert blocks at lines 416–419 and 442–446 only cover the deployAgent execution path. Add a revert before re-throwing for the early-exit path:
| // Fetch the full request to get configYaml, pipelineId, changelog | |
| const request = await prisma.deployRequest.findUnique({ | |
| where: { id: input.requestId }, | |
| }); | |
| if (!request) { | |
| throw new TRPCError({ code: "NOT_FOUND", message: "Deploy request not found" }); | |
| } | |
| // Fetch the full request to get configYaml, pipelineId, changelog | |
| const request = await prisma.deployRequest.findUnique({ | |
| where: { id: input.requestId }, | |
| }); | |
| if (!request) { | |
| await prisma.deployRequest.updateMany({ | |
| where: { id: input.requestId, status: "DEPLOYED" }, | |
| data: { status: "APPROVED", deployedById: null, deployedAt: null }, | |
| }); | |
| throw new TRPCError({ code: "NOT_FOUND", message: "Deploy request not found" }); | |
| } |
Rule Used: ## Pipeline Editor & Deployment Review Guide
The ... (source)
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/server/routers/deploy.ts
Line: 396-402
Comment:
**Status stuck at DEPLOYED if record deleted between atomic update and fetch**
The code atomically sets the request to `DEPLOYED` (line 388–391), then immediately tries to fetch the full record (line 397–401). If the record is deleted between these operations — possible through a concurrent admin operation — the `findUnique` returns `null` and the procedure throws `NOT_FOUND` without reverting the `DEPLOYED` status. The request is left permanently in `DEPLOYED` state with no recovery path.
The two revert blocks at lines 416–419 and 442–446 only cover the `deployAgent` execution path. Add a revert before re-throwing for the early-exit path:
```suggestion
// Fetch the full request to get configYaml, pipelineId, changelog
const request = await prisma.deployRequest.findUnique({
where: { id: input.requestId },
});
if (!request) {
await prisma.deployRequest.updateMany({
where: { id: input.requestId, status: "DEPLOYED" },
data: { status: "APPROVED", deployedById: null, deployedAt: null },
});
throw new TRPCError({ code: "NOT_FOUND", message: "Deploy request not found" });
}
```
**Rule Used:** ## Pipeline Editor & Deployment Review Guide
The ... ([source](https://app.greptile.com/review/custom-context?memory=99b9234e-e536-4295-b97d-d10499542ee1))
How can I resolve this? If you propose a fix, please make it concise.1 participant
Summary
UserPreferencemodel with localStorage caching for instant hydration.ScimUserentity type (wasUser). Combined "SCIM (All)" filter in audit log UI shows bothScimUserandScimGroupactivity.DEPLOYEDstatus tracks who executed. Approved requests show in deploy dialog with "Deploy Now" and "Cancel" buttons.fireEventAlerthelper wired into deploy, SCIM, backup, fleet, and version check services.Schema changes
UserPreferencemodel (userId + key + value)Team.defaultEnvironmentIdfieldDeployRequest.deployedById,deployedBy,deployedAtfields +DEPLOYEDstatusAlertMetricenum: 10 new event-based valuesAlertRule.condition,threshold,durationSecondsnow nullableTest plan