chore: pending actions UI #704
Conversation
WalkthroughThis PR introduces a new approval workflow by adding a PendingActionsSection UI component to the deployment environment panel, backed by two new TRPC routers: one querying environment-scoped deployment decisions, and another handling approval mutations that emit user approval events. Changes
Sequence DiagramsequenceDiagram
participant User
participant UI as PendingActionsSection
participant DecisionsAPI as Decisions Query
participant ApprovalAPI as Approve Mutation
participant GoEvent as Go Event Emitter
participant Toast as Toast Notification
User->>UI: Views pending actions
UI->>DecisionsAPI: Query environmentVersion(workspaceId, environmentId, versionId)
DecisionsAPI-->>UI: Returns pending decisions
UI->>UI: Compute pending actions, show Approve button
User->>UI: Clicks Approve
UI->>ApprovalAPI: Call approve(workspaceId, deploymentVersionId, environmentId, status)
ApprovalAPI->>ApprovalAPI: Build UserApprovalRecord
ApprovalAPI->>GoEvent: Emit Event.UserApprovalRecordCreated
ApprovalAPI-->>UI: Return UserApprovalRecord
UI->>Toast: Show success message
Toast-->>User: Approval confirmed
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/web/app/routes/ws/deployments/_components/EnvironmentActionsPanel.tsx (1)
245-247: Template string rendered literally.Backticks and
${}appear in the UI. Use a template literal inside JSX.- {totalResources} resource{totalResources !== 1 ? "s" : ""} ·{" "} - {upToDateCount} up to date ` · ${transitioningCount} updating` + {totalResources} resource{totalResources !== 1 ? "s" : ""} ·{" "} + {`${upToDateCount} up to date · ${transitioningCount} updating`}
🧹 Nitpick comments (4)
apps/web/app/routes/ws/deployments/_components/EnvironmentActionsPanel.tsx (3)
72-80: Render and act only on approval actions; add error handling + invalidation.Currently all
ruleResultsare shown with an Approve button. Filter toactionType === "approval", handle mutation errors, and invalidate the decisions query on success.- const pendingActions = (decisionsQuery.data ?? []).flatMap( - (action) => action.ruleResults, - ); - const approvalAction = pendingActions.find( - (action) => action.actionType === "approval", - ); + const utils = trpc.useUtils(); + const pendingActions = (decisionsQuery.data ?? []) + .flatMap((p) => p.ruleResults) + .filter((r) => r.actionType === "approval"); + const approvalAction = pendingActions[0]; - const onClick = () => - approveMutation - .mutateAsync({ + const onClick = () => + approveMutation + .mutateAsync({ workspaceId: workspace.id, deploymentVersionId: version.id, environmentId: environment.id, status: "approved", }) - .then(() => toast.success("Approval record queued successfully")); + .then(async () => { + toast.success("Approval record queued successfully"); + await utils.decisions.environmentVersion.invalidate({ + workspaceId: workspace.id, + environmentId: environment.id, + versionId: version.id, + }); + }) + .catch((err) => + toast.error(err?.message ?? "Failed to queue approval"), + );Also applies to: 98-111
255-258: Do not hard‑codeversions[0]; derive the relevant version for this environment/deployment.Using the first element is brittle and may target the wrong version.
- <PendingActionsSection - version={versions[0]} - environment={environment} - /> + <PendingActionsSection + version={ + // prefer desired, then current, else fallback + versions.find( + (v) => + v.id === + (envReleaseTargets[0]?.state?.desiredRelease?.version.id ?? + envReleaseTargets[0]?.state?.currentRelease?.version.id), + ) ?? versions[0] + } + environment={environment} + />
267-276: Version grouping labels show IDs, not tags; fix label and sort.You group by
version.idbut display it as a tag. Map id→tag for display and sorting.- {Object.entries(resourcesByVersion) - .sort(([tagA], [tagB]) => tagB.localeCompare(tagA)) - .map(([versionTag, resources]) => ( + {Object.entries(resourcesByVersion) + .sort(([idA], [idB]) => { + const getTag = (id: string) => + versions.find((v) => v.id === id)?.tag ?? id; + return getTag(idB).localeCompare(getTag(idA)); + }) + .map(([versionId, resources]) => { + const versionTag = + versions.find((v) => v.id === versionId)?.tag ?? versionId; + return ( <VersionGroup - key={versionTag} - versionTag={versionTag} + key={versionId} + versionTag={versionTag} resources={resources} versions={versions} /> - ))} + ); + })}packages/trpc/src/routes/deployment-versions.ts (1)
12-16: Tighten ID validation to UUIDs for consistency with queries.
deploymentVersionIdandenvironmentIdlook like UUIDs elsewhere. Validate accordingly.(Combined in the diff above.)
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
apps/web/app/routes/ws/deployments/_components/EnvironmentActionsPanel.tsx(4 hunks)packages/trpc/src/root.ts(3 hunks)packages/trpc/src/routes/decisions.ts(1 hunks)packages/trpc/src/routes/deployment-versions.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: Use TypeScript with explicit types (prefer interfaces for public APIs)
Import styles: Use named imports, group imports by source (std lib > external > internal)
Consistent type imports:import type { Type } from "module"
Prefer async/await over raw promises
Handle errors explicitly (use try/catch and typed error responses)
Files:
packages/trpc/src/routes/decisions.tspackages/trpc/src/root.tsapps/web/app/routes/ws/deployments/_components/EnvironmentActionsPanel.tsxpackages/trpc/src/routes/deployment-versions.ts
⚙️ CodeRabbit configuration file
**/*.{ts,tsx}: Note on Error Handling:
Avoid strict enforcement of try/catch blocks. Code may use early returns, Promise chains (.then().catch()), or other patterns for error handling. These are acceptable as long as they maintain clarity and predictability.
Files:
packages/trpc/src/routes/decisions.tspackages/trpc/src/root.tsapps/web/app/routes/ws/deployments/_components/EnvironmentActionsPanel.tsxpackages/trpc/src/routes/deployment-versions.ts
**/*.{js,jsx,ts,tsx,json,md,yml,yaml}
📄 CodeRabbit inference engine (CLAUDE.md)
Formatting: Prettier is used with
@ctrlplane/prettier-config
Files:
packages/trpc/src/routes/decisions.tspackages/trpc/src/root.tsapps/web/app/routes/ws/deployments/_components/EnvironmentActionsPanel.tsxpackages/trpc/src/routes/deployment-versions.ts
🧬 Code graph analysis (4)
packages/trpc/src/routes/decisions.ts (2)
packages/workspace-engine-sdk/src/index.ts (1)
getClientFor(22-36)packages/trpc/src/trpc.ts (1)
router(45-45)
packages/trpc/src/root.ts (2)
packages/trpc/src/routes/deployment-versions.ts (1)
deploymentVersionsRouter(8-38)packages/trpc/src/routes/decisions.ts (1)
decisionsRouter(65-90)
apps/web/app/routes/ws/deployments/_components/EnvironmentActionsPanel.tsx (3)
apps/web/app/routes/ws/deployments/_components/types.ts (2)
Environment(57-63)DeploymentVersion(23-33)apps/web/app/components/WorkspaceProvider.tsx (1)
useWorkspace(33-38)apps/web/app/api/trpc.tsx (1)
trpc(15-15)
packages/trpc/src/routes/deployment-versions.ts (2)
packages/trpc/src/trpc.ts (1)
router(45-45)packages/events/src/kafka/client.ts (1)
sendGoEvent(35-65)
⏰ 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). (6)
- GitHub Check: Typecheck
- GitHub Check: build (linux/amd64)
- GitHub Check: build-and-push (linux/amd64)
- GitHub Check: Lint
- GitHub Check: workspace-engine-tests
- GitHub Check: build (linux/amd64)
🔇 Additional comments (2)
packages/trpc/src/root.ts (1)
24-25: Router wiring is correct and properly typed.New routes (
deploymentVersionsandsystem) are correctly imported and wired. AppRouter type is properly exported for client use. Verify that client types were regenerated to pick up these new routes.packages/trpc/src/routes/decisions.ts (1)
26-31: All endpoint paths and field names verified—code is correct.The OpenAPI routes and field names in
decisions.tsare correctly spelled:
- Line 27:
/v1/workspaces/{workspaceId}/deploymentversions/{deploymentVersionId}✓- Line 40:
/v1/workspaceId}/release-targets/evaluate✓ReleaseTargetWithStateschema withdeploymentId,environmentId,resourceIdfields all confirmed in the SDK schema and used consistently throughout the codebase.
| return (response.data?.items ?? []).find( | ||
| (target) => target.deployment.id === deploymentId, | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Wrong property path; likely never matches any release target.
ReleaseTargetWithState elsewhere in the app exposes IDs under releaseTarget.{deploymentId, environmentId, resourceId}. Matching on target.deployment.id will fail.
Apply:
- return (response.data?.items ?? []).find(
- (target) => target.deployment.id === deploymentId,
- );
+ return (response.data?.items ?? []).find(
+ (target) => target.releaseTarget.deploymentId === deploymentId,
+ );📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| return (response.data?.items ?? []).find( | |
| (target) => target.deployment.id === deploymentId, | |
| ); | |
| return (response.data?.items ?? []).find( | |
| (target) => target.releaseTarget.deploymentId === deploymentId, | |
| ); |
🤖 Prompt for AI Agents
In packages/trpc/src/routes/decisions.ts around lines 17 to 19, the code is
matching on target.deployment.id which is the wrong property path and therefore
never finds matches; change the predicate to use the ReleaseTargetWithState
shape by comparing target.releaseTarget.deploymentId === deploymentId (and
defensively handle undefined releaseTarget if necessary), i.e. access
releaseTarget.deploymentId instead of deployment.id so the find will match the
actual IDs exposed elsewhere in the app.
| releaseTarget: { | ||
| deploymentId: releaseTarget.deployment.id, | ||
| environmentId: releaseTarget.environment.id, | ||
| resourceId: releaseTarget.resource.id, | ||
| }, | ||
| version, |
There was a problem hiding this comment.
Evaluate payload uses wrong fields; sends undefined IDs.
Build the releaseTarget for evaluation from releaseTarget.releaseTarget.*, not .deployment/.environment.
- releaseTarget: {
- deploymentId: releaseTarget.deployment.id,
- environmentId: releaseTarget.environment.id,
- resourceId: releaseTarget.resource.id,
- },
+ releaseTarget: {
+ deploymentId: releaseTarget.releaseTarget.deploymentId,
+ environmentId: releaseTarget.releaseTarget.environmentId,
+ resourceId: releaseTarget.releaseTarget.resourceId,
+ },📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| releaseTarget: { | |
| deploymentId: releaseTarget.deployment.id, | |
| environmentId: releaseTarget.environment.id, | |
| resourceId: releaseTarget.resource.id, | |
| }, | |
| version, | |
| releaseTarget: { | |
| deploymentId: releaseTarget.releaseTarget.deploymentId, | |
| environmentId: releaseTarget.releaseTarget.environmentId, | |
| resourceId: releaseTarget.releaseTarget.resourceId, | |
| }, | |
| version, |
🤖 Prompt for AI Agents
In packages/trpc/src/routes/decisions.ts around lines 44–49, the evaluation
payload is using releaseTarget.deployment/environment/resource which are
undefined; construct releaseTarget using the nested releaseTarget.releaseTarget
object instead. Replace the three fields so they read from
releaseTarget.releaseTarget (e.g. releaseTarget.releaseTarget.deployment.id,
releaseTarget.releaseTarget.environment.id,
releaseTarget.releaseTarget.resource.id) and add null/undefined guards or throw
a clear error if those nested values are missing.
| z.object({ | ||
| workspaceId: z.string().uuid(), | ||
| deploymentVersionId: z.string(), | ||
| environmentId: z.string(), | ||
| status: z.enum(["approved", "rejected"]).default("approved"), | ||
| }), | ||
| ) |
There was a problem hiding this comment.
Authorization gap: don’t trust client‑supplied workspaceId for event routing.
A malicious client could emit approvals to another workspace. Derive the workspace from server‑side state (e.g., fetch the version and use its workspace; or use a ctx.workspaceId established by auth middleware) and ignore the input field.
- .input(
+ .input(
z.object({
- workspaceId: z.string().uuid(),
- deploymentVersionId: z.string(),
- environmentId: z.string(),
+ // Remove workspaceId from client input
+ deploymentVersionId: z.string().uuid(),
+ environmentId: z.string().uuid(),
status: z.enum(["approved", "rejected"]).default("approved"),
}),
)
- .mutation(async ({ ctx, input }) => {
+ .mutation(async ({ ctx, input }) => {
const userId = ctx.session.user.id;
+ // Derive workspaceId securely (examples; pick one that fits your stack)
+ // const workspaceId = ctx.workspace.id;
+ // or: const { workspaceId } = await getDeploymentVersion(input.deploymentVersionId);
const record: WorkspaceEngine["schemas"]["UserApprovalRecord"] = {
userId,
versionId: input.deploymentVersionId,
environmentId: input.environmentId,
status: input.status,
createdAt: new Date().toISOString(),
};
- await sendGoEvent({
- workspaceId: input.workspaceId,
+ await sendGoEvent({
+ workspaceId, // derived server-side
eventType: Event.UserApprovalRecordCreated,
timestamp: Date.now(),
data: record,
});Also applies to: 29-34
🤖 Prompt for AI Agents
In packages/trpc/src/routes/deployment-versions.ts around lines 11-17 (and
similarly lines 29-34), the handler currently accepts a client-supplied
workspaceId which can be abused to route events to other workspaces; remove
trust in that input by deriving the workspaceId from server-side state instead —
fetch the deployment version (or use ctx.workspaceId set by auth middleware),
verify it belongs to the authenticated user/team, and use that server-derived
workspaceId for event routing and authorization checks while completely ignoring
the workspaceId value from the request body/validation.
Summary by CodeRabbit