feat: separate deployment initialize and start steps #2522
Conversation
|
WalkthroughAdds DEPLOY_QUEUE_TIMEOUT_MS (int, default 60100015) to the public environment schema. Adds optional startedAt DateTime? to WorkerDeployment (Prisma schema + migration) and surfaces startedAt in DB selects and presenters. Introduces StartDeploymentRequestBody and optional initialStatus on InitializeDeploymentRequestBody. Implements DeploymentService.startDeployment to transition PENDING → BUILDING, set startedAt, persist updates, and enqueue a timeout extension (uses DEPLOY_QUEUE_TIMEOUT_MS for queued deployments). Adds POST /api.v1.deployments.$deploymentId.start route with params/body validation, API-key auth, service invocation, and mapped error responses. UI: displays startedAt when present, adds PENDING status and "Queued…" label, and exports a shared UserTag. Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests
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: 4
🧹 Nitpick comments (14)
internal-packages/database/prisma/schema.prisma (1)
1767-1767: startedAt addition looks correct; consider index for operational queries.Optional: add an index to support queries like "active BUILDING older than X" and reduce table scans.
Add near the other indexes in WorkerDeployment:
model WorkerDeployment { ... @@unique([projectId, shortCode]) @@unique([environmentId, version]) + @@index([status, startedAt]) }internal-packages/database/prisma/migrations/20250918122438_add_started_at_to_deployment_schema/migration.sql (1)
1-1: Forward-only migration is fine; consider a lightweight backfill for existing rows.To improve historical consistency in UIs, you can backfill
startedAtwhere possible.Add a follow-up migration:
-- Backfill when we know build started at least by builtAt UPDATE "public"."WorkerDeployment" SET "startedAt" = "builtAt" WHERE "startedAt" IS NULL AND "builtAt" IS NOT NULL;apps/webapp/app/env.server.ts (1)
315-318: New DEPLOY_QUEUE_TIMEOUT_MS: LGTM.Default and typing align with existing timeout vars. Please add this to environment docs/sample envs for visibility.
apps/webapp/app/v3/services/initializeDeployment.server.ts (1)
115-116: Minor: log message could clarify queued vs building.You might also log
timeoutMsor which timeout branch was selected for easier debugging.apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.deployments/route.tsx (1)
361-368: Extract UserTag into a shared component (avoid cross‑route coupling).Exporting UI from a route makes other routes depend on this module. Consider moving
UserTagto a shared components folder (e.g.,~/components/UserTag.tsx) and import it from there in both routes.Would you like me to generate the file move + import updates?
apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.deployments.$deploymentParam/route.tsx (3)
35-35: Avoid importing UI from another route module.Importing
UserTagfrom the parent route ties route modules together. Prefer a shared component (e.g.,~/components/UserTag) to keep routes decoupled.
191-197: “UTC” label is misleading; DateTimeAccurate renders local time.
DateTimeAccurateformats using the client’s local timezone, so appending" UTC"suggests the wrong timezone.Apply this diff to remove the misleading suffix:
- {deployment.startedAt ? ( - <> - <DateTimeAccurate date={deployment.startedAt} /> UTC - </> - ) : ( + {deployment.startedAt ? ( + <DateTimeAccurate date={deployment.startedAt} /> + ) : ( "–" )}Follow up: consider aligning “Built at” and “Deployed at” fields similarly for consistency.
236-245: Fallback to a non‑empty user label.Passing
""can produce empty text/alt. Use a safe fallback.- <UserTag - name={deployment.deployedBy.name ?? deployment.deployedBy.displayName ?? ""} - avatarUrl={deployment.deployedBy.avatarUrl ?? undefined} - /> + <UserTag + name={ + deployment.deployedBy.name ?? + deployment.deployedBy.displayName ?? + "Unknown user" + } + avatarUrl={deployment.deployedBy.avatarUrl ?? undefined} + />apps/webapp/app/routes/api.v1.deployments.$deploymentId.start.ts (3)
37-42: Harden JSON parsing for invalid bodies.
request.json()throws on invalid JSON. Return 400 instead of a 500.- const rawBody = await request.json(); - const body = StartDeploymentRequestBody.safeParse(rawBody); + let rawBody: unknown; + try { + rawBody = await request.json(); + } catch { + return json({ error: "Invalid JSON" }, { status: 400 }); + } + const body = StartDeploymentRequestBody.safeParse(rawBody);
49-55: Use an empty 204 response body.
json(null, { status: 204 })sets a JSON body for a No‑Content response. Prefernew Response(null, { status: 204 }).- return json(null, { status: 204 }); + return new Response(null, { status: 204 });- return json(null, { status: 204 }); // ignore these errors for now + return new Response(null, { status: 204 }); // ignore these errors for now
60-63: Log unexpected errors for operability.Surface the underlying cause to logs before returning 500.
- error.type satisfies "other"; - return json({ error: "Internal server error" }, { status: 500 }); + error.type satisfies "other"; + logger.error("startDeployment failed with unknown error", { cause: error.cause }); + return json({ error: "Internal server error" }, { status: 500 });apps/webapp/app/v3/services/deployment.server.ts (3)
66-79: Dequeue any existing PENDING-timeout before enqueuing BUILDING-timeoutAvoid a stale timeout firing and logging an error when status no longer matches.
Sketch:
const dequeuePendingTimeout = (d: Pick<WorkerDeployment,"id">) => fromPromise(TimeoutDeploymentService.dequeue(d.id, this._prisma), () => ({ type: "other" as const })) .map(() => d); // …chain: return getDeployment() .andThen(validateDeployment) .andThen(updateDeployment) .andThen(dequeuePendingTimeout) .andThen(extendTimeout);Also applies to: 80-84
39-47: Include currentStatus in the error for better diagnosticsReturn
{ type: "deployment_not_pending", currentStatus: deployment.status }to aid API/error mapping.- return errAsync({ type: "deployment_not_pending" as const }); + return errAsync({ type: "deployment_not_pending" as const, currentStatus: deployment.status });
17-26: Use findUnique by friendlyId (optional)friendlyId is declared @unique on WorkerDeployment in internal-packages/database/prisma/schema.prisma — replace the findFirst call with findUnique({ where: { friendlyId }, select: { status: true, id: true } }) for clearer intent and a small perf gain.
Location: apps/webapp/app/v3/services/deployment.server.ts (lines 17–26)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
apps/webapp/app/env.server.ts(1 hunks)apps/webapp/app/presenters/v3/DeploymentPresenter.server.ts(2 hunks)apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.deployments.$deploymentParam/route.tsx(3 hunks)apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.deployments/route.tsx(1 hunks)apps/webapp/app/routes/api.v1.deployments.$deploymentId.start.ts(1 hunks)apps/webapp/app/routes/api.v1.deployments.ts(1 hunks)apps/webapp/app/v3/services/deployment.server.ts(1 hunks)apps/webapp/app/v3/services/initializeDeployment.server.ts(4 hunks)internal-packages/database/prisma/migrations/20250918122438_add_started_at_to_deployment_schema/migration.sql(1 hunks)internal-packages/database/prisma/schema.prisma(1 hunks)packages/core/src/v3/schemas/api.ts(2 hunks)
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{ts,tsx}: Always prefer using isomorphic code like fetch, ReadableStream, etc. instead of Node.js specific code
For TypeScript, we usually use types over interfaces
Avoid enums
No default exports, use function declarations
Files:
apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.deployments/route.tsxpackages/core/src/v3/schemas/api.tsapps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.deployments.$deploymentParam/route.tsxapps/webapp/app/v3/services/deployment.server.tsapps/webapp/app/routes/api.v1.deployments.tsapps/webapp/app/v3/services/initializeDeployment.server.tsapps/webapp/app/presenters/v3/DeploymentPresenter.server.tsapps/webapp/app/env.server.tsapps/webapp/app/routes/api.v1.deployments.$deploymentId.start.ts
{packages/core,apps/webapp}/**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
We use zod a lot in packages/core and in the webapp
Files:
apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.deployments/route.tsxpackages/core/src/v3/schemas/api.tsapps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.deployments.$deploymentParam/route.tsxapps/webapp/app/v3/services/deployment.server.tsapps/webapp/app/routes/api.v1.deployments.tsapps/webapp/app/v3/services/initializeDeployment.server.tsapps/webapp/app/presenters/v3/DeploymentPresenter.server.tsapps/webapp/app/env.server.tsapps/webapp/app/routes/api.v1.deployments.$deploymentId.start.ts
apps/webapp/**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/webapp.mdc)
When importing from @trigger.dev/core in the webapp, never import the root package path; always use one of the documented subpath exports from @trigger.dev/core’s package.json
Files:
apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.deployments/route.tsxapps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.deployments.$deploymentParam/route.tsxapps/webapp/app/v3/services/deployment.server.tsapps/webapp/app/routes/api.v1.deployments.tsapps/webapp/app/v3/services/initializeDeployment.server.tsapps/webapp/app/presenters/v3/DeploymentPresenter.server.tsapps/webapp/app/env.server.tsapps/webapp/app/routes/api.v1.deployments.$deploymentId.start.ts
{apps/webapp/app/**/*.server.{ts,tsx},apps/webapp/app/routes/**/*.ts}
📄 CodeRabbit inference engine (.cursor/rules/webapp.mdc)
Access environment variables only via the env export from app/env.server.ts; do not reference process.env directly
Files:
apps/webapp/app/v3/services/deployment.server.tsapps/webapp/app/routes/api.v1.deployments.tsapps/webapp/app/v3/services/initializeDeployment.server.tsapps/webapp/app/presenters/v3/DeploymentPresenter.server.tsapps/webapp/app/env.server.tsapps/webapp/app/routes/api.v1.deployments.$deploymentId.start.ts
apps/webapp/app/**/*.ts
📄 CodeRabbit inference engine (.cursor/rules/webapp.mdc)
Modules intended for test consumption under apps/webapp/app/**/*.ts must not read environment variables; accept configuration via options instead
Files:
apps/webapp/app/v3/services/deployment.server.tsapps/webapp/app/routes/api.v1.deployments.tsapps/webapp/app/v3/services/initializeDeployment.server.tsapps/webapp/app/presenters/v3/DeploymentPresenter.server.tsapps/webapp/app/env.server.tsapps/webapp/app/routes/api.v1.deployments.$deploymentId.start.ts
🧬 Code graph analysis (5)
packages/core/src/v3/schemas/api.ts (1)
packages/core/src/v3/schemas/common.ts (2)
GitMeta(239-253)GitMeta(255-255)
apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.deployments.$deploymentParam/route.tsx (2)
apps/webapp/app/components/primitives/DateTime.tsx (1)
DateTimeAccurate(196-242)apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.deployments/route.tsx (1)
UserTag(361-368)
apps/webapp/app/v3/services/deployment.server.ts (2)
apps/webapp/app/v3/services/createDeploymentBackgroundWorkerV4.server.ts (1)
deployment(182-200)apps/webapp/app/v3/services/timeoutDeployment.server.ts (1)
TimeoutDeploymentService(8-69)
apps/webapp/app/v3/services/initializeDeployment.server.ts (2)
apps/webapp/app/v3/services/timeoutDeployment.server.ts (1)
TimeoutDeploymentService(8-69)apps/webapp/app/v3/services/createDeploymentBackgroundWorkerV4.server.ts (1)
deployment(182-200)
apps/webapp/app/routes/api.v1.deployments.$deploymentId.start.ts (3)
apps/webapp/app/services/apiAuth.server.ts (1)
authenticateRequest(379-441)packages/core/src/v3/schemas/api.ts (2)
StartDeploymentRequestBody(380-384)StartDeploymentRequestBody(386-386)apps/webapp/app/v3/services/deployment.server.ts (1)
DeploymentService(9-85)
⏰ 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)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (3, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (8, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (6, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (4, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (7, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (2, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (1, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (5, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (7, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (8, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (6, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (2, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (5, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (4, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (3, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (1, 8)
- GitHub Check: units / packages / 🧪 Unit Tests: Packages (1, 1)
- GitHub Check: e2e / 🧪 CLI v3 tests (ubuntu-latest - npm)
- GitHub Check: e2e / 🧪 CLI v3 tests (ubuntu-latest - pnpm)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - pnpm)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - npm)
- GitHub Check: typecheck / typecheck
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (10)
apps/webapp/app/presenters/v3/DeploymentPresenter.server.ts (1)
105-105: Including startedAt in select/response: LGTM; verify downstream consumers tolerate undefined.No issues. Please confirm API clients and UI handle
startedAt?: Date | null.Also applies to: 149-149
apps/webapp/app/v3/services/initializeDeployment.server.ts (3)
102-105: Initial status default: LGTM.Keeps backward-compat by defaulting to BUILDING while enabling queued PENDING.
124-125: Persisting initial status: LGTM.Matches request body and new flow.
134-135: Conditionally setting startedAt: LGTM; ensure startDeployment also sets it on transition.Double-check the start path sets
startedAtwhen moving PENDING → BUILDING.packages/core/src/v3/schemas/api.ts (2)
380-387: Schema → service field mismatch (“gitMeta” vs “git”).
StartDeploymentRequestBodyusesgitMeta, whileDeploymentService.startDeploymentexpectsupdates.git. Ensure the route mapsgitMeta→gitbefore calling the service to avoid compile/runtime issues.If you prefer to support both for forwards/backwards compatibility, consider allowing an alias in the route normalizer rather than changing the public schema.
430-430: LGTM: optional initialStatus addition.Optional
"PENDING" | "BUILDING"aligns with the initialize flow. No issues.apps/webapp/app/routes/api.v1.deployments.ts (1)
1-6: LGTM: type‑only imports.Good move to
import typeforActionFunctionArgsandInitializeDeploymentResponseBody.apps/webapp/app/v3/services/deployment.server.ts (3)
51-54: LGTM: optimistic concurrency via updateMany + status guardGood pattern to avoid races without locks.
70-71: TypeScriptsatisfiesis supported — repo uses TS 5.5.4.
package.json lists "typescript": "5.5.4", so thesatisfiesoperator (TS ≥ 4.9) is supported; no CI break expected.
13-14: Align updates.git type with the Prisma JSON field (cast/serialize at DB boundary)GitMeta is exported at packages/core/src/v3/schemas/common.ts (export type GitMeta). updates is declared in apps/webapp/app/v3/services/deployment.server.ts (lines ~13–14; also applies to ~53–54). I couldn't find the WorkerDeployment Prisma/type in this search — confirm WorkerDeployment.git is a Prisma Json field (Prisma.JsonValue / Prisma.InputJsonValue) and either (A) update the TS typing to match Prisma’s JSON type or (B) convert/cast/serialize GitMeta to Prisma.InputJsonValue when calling Prisma to avoid type drift.
apps/webapp/app/routes/api.v1.deployments.$deploymentId.start.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
apps/webapp/app/components/runs/v3/DeploymentStatus.tsx (3)
53-55: Hide decorative icon from screen readersAdd aria-hidden to avoid redundant announcements (label text follows). Consider applying to all icon returns in this switch.
- <RectangleStackIcon className={cn(deploymentStatusClassNameColor(status), className)} /> + <RectangleStackIcon aria-hidden="true" className={cn(deploymentStatusClassNameColor(status), className)} />
127-127: Stale comment + typoComment contradicts the new behavior and has a typo (“ommited”).
-// PENDING and CANCELED are not used so are ommited from the UI +// Note: PENDING is shown in the UI; only CANCELED is omitted here. Keep in sync with WorkerDeploymentStatus.
129-135: Adding PENDING to deploymentStatuses — verified; update stale comment & consider type-safety
- deploymentStatuses is only consumed by apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.deployments/route.tsx (mapped for display), so the order change is UI-only; no code appears to assume BUILDING is first.
- Excluding CANCELED appears intentional (DB enum / API include it but the UI list omits it).
- Action: update the stale comment in apps/webapp/app/components/runs/v3/DeploymentStatus.tsx (around the deploymentStatuses array) — it currently says "PENDING and CANCELED are not used…" but PENDING is now included.
- Optional: apply the suggested type-safety refactor (export as const … satisfies readonly WorkerDeploymentStatus[]).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/webapp/app/components/runs/v3/DeploymentStatus.tsx(5 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{ts,tsx}: Always prefer using isomorphic code like fetch, ReadableStream, etc. instead of Node.js specific code
For TypeScript, we usually use types over interfaces
Avoid enums
No default exports, use function declarations
Files:
apps/webapp/app/components/runs/v3/DeploymentStatus.tsx
{packages/core,apps/webapp}/**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
We use zod a lot in packages/core and in the webapp
Files:
apps/webapp/app/components/runs/v3/DeploymentStatus.tsx
apps/webapp/**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/webapp.mdc)
When importing from @trigger.dev/core in the webapp, never import the root package path; always use one of the documented subpath exports from @trigger.dev/core’s package.json
Files:
apps/webapp/app/components/runs/v3/DeploymentStatus.tsx
🧬 Code graph analysis (1)
apps/webapp/app/components/runs/v3/DeploymentStatus.tsx (1)
apps/webapp/app/utils/cn.ts (1)
cn(77-79)
⏰ 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). (22)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (3, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (8, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (5, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (3, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (6, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (4, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (5, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (7, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (7, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (1, 8)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - pnpm)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (2, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (6, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (4, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (2, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (1, 8)
- GitHub Check: units / packages / 🧪 Unit Tests: Packages (1, 1)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - npm)
- GitHub Check: e2e / 🧪 CLI v3 tests (ubuntu-latest - npm)
- GitHub Check: typecheck / typecheck
- GitHub Check: e2e / 🧪 CLI v3 tests (ubuntu-latest - pnpm)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (3)
apps/webapp/app/components/runs/v3/DeploymentStatus.tsx (3)
5-7: Import for PENDING icon — LGTMConsistent with the existing heroicons set.
77-83: Confirm muted color for QueuedPENDING maps to text-charcoal-500. If you want parity with BUILDING/DEPLOYING emphasis, consider text-pending; otherwise this subdued tone reads as intentional. Please confirm with design.
98-103: User-facing label “Queued…” — LGTMGood decoupling of label from backend enum name.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
apps/webapp/app/v3/services/deployment.server.ts (2)
10-10: Resolved: async/ResultAsync mismatch is fixedRemoving
asyncavoids double-wrapped thenables and aligns with neverthrow chaining.
66-79: Option: preserve{ id }through the chain (or keep void, but decide explicitly)If callers benefit from the id, map the enqueue result back to the input. Otherwise, keep void and document it.
- ).map(() => undefined); + ).map(() => deployment);If you adopt this, also update the method’s return type to
ResultAsync<Pick<WorkerDeployment, "id">, …>.
🧹 Nitpick comments (2)
apps/webapp/app/v3/services/deployment.server.ts (2)
3-3: Add an explicit ResultAsync return type on the public methodLock the contract and improve call‑site ergonomics by annotating the return type and importing it.
import { errAsync, fromPromise, okAsync } from "neverthrow"; +import type { ResultAsync } from "neverthrow"; @@ - public startDeployment( + public startDeployment( authenticatedEnv: AuthenticatedEnvironment, friendlyId: string, updates: Partial<Pick<WorkerDeployment, "contentHash" | "runtime"> & { git: GitMeta }> - ) { + ): ResultAsync< + void, + | { type: "deployment_not_found" } + | { type: "deployment_not_pending" } + | { type: "failed_to_extend_deployment_timeout"; cause: unknown } + | { type: "other"; cause: unknown } + > {Also applies to: 10-14
66-79: Dequeue any existing timeout before scheduling BUILDING timeoutAvoid leaving a stale PENDING-timeout job around (it will no-op but creates noise/overhead). Dequeue then enqueue.
- const extendTimeout = (deployment: Pick<WorkerDeployment, "id">) => - fromPromise( - TimeoutDeploymentService.enqueue( - deployment.id, - "BUILDING" satisfies WorkerDeploymentStatus, - "Building timed out", - new Date(Date.now() + env.DEPLOY_TIMEOUT_MS) - ), - (error) => ({ - type: "failed_to_extend_deployment_timeout" as const, - cause: error, - }) - ).map(() => undefined); + const extendTimeout = (deployment: Pick<WorkerDeployment, "id">) => + fromPromise( + (async () => { + await TimeoutDeploymentService.dequeue(deployment.id, this._prisma); + await TimeoutDeploymentService.enqueue( + deployment.id, + "BUILDING" satisfies WorkerDeploymentStatus, + "Building timed out", + new Date(Date.now() + env.DEPLOY_TIMEOUT_MS) + ); + })(), + (error) => ({ + type: "failed_to_extend_deployment_timeout" as const, + cause: error, + }) + ).map(() => undefined);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
apps/webapp/app/routes/api.v1.deployments.$deploymentId.start.ts(1 hunks)apps/webapp/app/v3/services/deployment.server.ts(1 hunks)apps/webapp/app/v3/services/initializeDeployment.server.ts(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/webapp/app/v3/services/initializeDeployment.server.ts
- apps/webapp/app/routes/api.v1.deployments.$deploymentId.start.ts
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{ts,tsx}: Always prefer using isomorphic code like fetch, ReadableStream, etc. instead of Node.js specific code
For TypeScript, we usually use types over interfaces
Avoid enums
No default exports, use function declarations
Files:
apps/webapp/app/v3/services/deployment.server.ts
{packages/core,apps/webapp}/**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
We use zod a lot in packages/core and in the webapp
Files:
apps/webapp/app/v3/services/deployment.server.ts
apps/webapp/**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/webapp.mdc)
When importing from @trigger.dev/core in the webapp, never import the root package path; always use one of the documented subpath exports from @trigger.dev/core’s package.json
Files:
apps/webapp/app/v3/services/deployment.server.ts
{apps/webapp/app/**/*.server.{ts,tsx},apps/webapp/app/routes/**/*.ts}
📄 CodeRabbit inference engine (.cursor/rules/webapp.mdc)
Access environment variables only via the env export from app/env.server.ts; do not reference process.env directly
Files:
apps/webapp/app/v3/services/deployment.server.ts
apps/webapp/app/**/*.ts
📄 CodeRabbit inference engine (.cursor/rules/webapp.mdc)
Modules intended for test consumption under apps/webapp/app/**/*.ts must not read environment variables; accept configuration via options instead
Files:
apps/webapp/app/v3/services/deployment.server.ts
🧬 Code graph analysis (1)
apps/webapp/app/v3/services/deployment.server.ts (2)
apps/webapp/app/v3/services/createDeploymentBackgroundWorkerV4.server.ts (1)
deployment(182-200)apps/webapp/app/v3/services/timeoutDeployment.server.ts (1)
TimeoutDeploymentService(8-69)
⏰ 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). (16)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (8, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (4, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (6, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (2, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (1, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (6, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (4, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (5, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (7, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (2, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (3, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (1, 8)
- GitHub Check: units / packages / 🧪 Unit Tests: Packages (1, 1)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - pnpm)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - npm)
- GitHub Check: typecheck / typecheck
🔇 Additional comments (4)
apps/webapp/app/v3/services/deployment.server.ts (4)
15-36: Lookup + error mapping LGTMNarrow select, safe error mapping, and explicit not-found handling look good.
38-47: State gate LGTMClear PENDING gate with a warning and precise error typing.
49-65: Atomic PENDING → BUILDING transition is correctUsing
updateManywith a status predicate prevents races; returning{ id }on success is useful.
66-73: No change required — timeout constants are used correctly.initializeDeployment.server.ts uses DEPLOY_QUEUE_TIMEOUT_MS when status === "PENDING" and DEPLOY_TIMEOUT_MS otherwise; BUILDING/DEPLOYING paths enqueue with DEPLOY_TIMEOUT_MS (apps/webapp/app/v3/services/initializeDeployment.server.ts:137-141; deployment.server.ts:70-74; createDeploymentBackgroundWorkerV4.server.ts:173-176).
apps/webapp/app/routes/api.v1.deployments.$deploymentId.start.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
apps/webapp/app/routes/api.v1.deployments.$deploymentId.start.ts (2)
48-50: Resolved: gitMeta → git mapping is correctly applied.This addresses the earlier feedback about persisting git metadata.
46-47: Return is present; earlier concern addressed.The call is properly returned (
return await …).
🧹 Nitpick comments (6)
apps/webapp/app/routes/api.v1.deployments.$deploymentId.start.ts (6)
13-15: Include Allow header for 405.HTTP 405 should declare supported methods.
- return json({ error: "Method Not Allowed" }, { status: 405 }); + return json({ error: "Method Not Allowed" }, { status: 405, headers: { Allow: "POST" } });
8-10: Stricter param validation for deploymentId.Avoid accepting empty strings.
- deploymentId: z.string(), + deploymentId: z.string().min(1, "deploymentId is required"),
37-43: Handle invalid JSON bodies explicitly.request.json() will throw on malformed/empty JSON; return 400 instead of 500.
- const rawBody = await request.json(); - const body = StartDeploymentRequestBody.safeParse(rawBody); + let rawBody: unknown; + try { + rawBody = await request.json(); + } catch { + return json({ error: "Invalid JSON body" }, { status: 400 }); + } + const body = StartDeploymentRequestBody.safeParse(rawBody);
46-51: Avoid passing undefined fields to the service/DB.Omit git when absent to prevent undefined from leaking into the Prisma update payload.
- return await deploymentService - .startDeployment(authenticatedEnv, deploymentId, { - contentHash: body.data.contentHash, - git: body.data.gitMeta, - runtime: body.data.runtime, - }) + const updates = { + contentHash: body.data.contentHash, + runtime: body.data.runtime, + ...(body.data.gitMeta ? { git: body.data.gitMeta } : {}), + }; + return await deploymentService + .startDeployment(authenticatedEnv, deploymentId, updates)
52-60: Use an empty body for 204 responses.204 must not include a response body; prefer Response over json().
- () => { - return json(null, { status: 204 }); - }, + () => new Response(null, { status: 204 }),- case "failed_to_extend_deployment_timeout": - return json(null, { status: 204 }); // ignore these errors for now + case "failed_to_extend_deployment_timeout": + return new Response(null, { status: 204 }); // ignore these errors for now
64-67: Remove redundant default clause (duplicate of 'other').The switch already handles "other"; default is unnecessary noise.
- case "other": - default: + case "other": error.type satisfies "other"; return json({ error: "Internal server error" }, { status: 500 });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/webapp/app/routes/api.v1.deployments.$deploymentId.start.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{ts,tsx}: Always prefer using isomorphic code like fetch, ReadableStream, etc. instead of Node.js specific code
For TypeScript, we usually use types over interfaces
Avoid enums
No default exports, use function declarations
Files:
apps/webapp/app/routes/api.v1.deployments.$deploymentId.start.ts
{packages/core,apps/webapp}/**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
We use zod a lot in packages/core and in the webapp
Files:
apps/webapp/app/routes/api.v1.deployments.$deploymentId.start.ts
apps/webapp/**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/webapp.mdc)
When importing from @trigger.dev/core in the webapp, never import the root package path; always use one of the documented subpath exports from @trigger.dev/core’s package.json
Files:
apps/webapp/app/routes/api.v1.deployments.$deploymentId.start.ts
{apps/webapp/app/**/*.server.{ts,tsx},apps/webapp/app/routes/**/*.ts}
📄 CodeRabbit inference engine (.cursor/rules/webapp.mdc)
Access environment variables only via the env export from app/env.server.ts; do not reference process.env directly
Files:
apps/webapp/app/routes/api.v1.deployments.$deploymentId.start.ts
apps/webapp/app/**/*.ts
📄 CodeRabbit inference engine (.cursor/rules/webapp.mdc)
Modules intended for test consumption under apps/webapp/app/**/*.ts must not read environment variables; accept configuration via options instead
Files:
apps/webapp/app/routes/api.v1.deployments.$deploymentId.start.ts
🧠 Learnings (1)
📚 Learning: 2025-08-14T12:13:20.455Z
Learnt from: myftija
PR: triggerdotdev/trigger.dev#2392
File: packages/cli-v3/src/utilities/gitMeta.ts:195-218
Timestamp: 2025-08-14T12:13:20.455Z
Learning: In the GitMeta schema (packages/core/src/v3/schemas/common.ts), all fields are intentionally optional to handle partial data from various deployment contexts (local, GitHub Actions, GitHub App). Functions like getGitHubAppMeta() are designed to work with missing environment variables rather than validate their presence.
Applied to files:
apps/webapp/app/routes/api.v1.deployments.$deploymentId.start.ts
🧬 Code graph analysis (1)
apps/webapp/app/routes/api.v1.deployments.$deploymentId.start.ts (3)
apps/webapp/app/services/apiAuth.server.ts (1)
authenticateRequest(379-441)packages/core/src/v3/schemas/api.ts (2)
StartDeploymentRequestBody(380-384)StartDeploymentRequestBody(386-386)apps/webapp/app/v3/services/deployment.server.ts (1)
DeploymentService(9-85)
⏰ 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). (22)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (7, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (2, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (5, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (6, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (4, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (5, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (3, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (6, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (1, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (2, 8)
- GitHub Check: e2e / 🧪 CLI v3 tests (ubuntu-latest - npm)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (1, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (7, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (3, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (8, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (4, 8)
- GitHub Check: units / packages / 🧪 Unit Tests: Packages (1, 1)
- GitHub Check: e2e / 🧪 CLI v3 tests (ubuntu-latest - pnpm)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - npm)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - pnpm)
- GitHub Check: typecheck / typecheck
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (1)
apps/webapp/app/routes/api.v1.deployments.$deploymentId.start.ts (1)
52-70: Add DEPLOY_QUEUE_TIMEOUT_MS to env docs — code already declares/uses itapps/webapp/app/env.server.ts defines DEPLOY_TIMEOUT_MS = 480000 (8m) and DEPLOY_QUEUE_TIMEOUT_MS = 900000 (15m); initializeDeployment.server.ts uses DEPLOY_QUEUE_TIMEOUT_MS for PENDING deployments while other services use DEPLOY_TIMEOUT_MS. docs/self-hosting/env/webapp.mdx only lists DEPLOY_TIMEOUT_MS — add DEPLOY_QUEUE_TIMEOUT_MS (default 900000, "Deploy queue timeout (ms) — used for pending deployments").
Likely an incorrect or invalid review comment.
Changes in this PR:
This enables showing queued (pending) deployments before they are started.
Relevant for limiting build concurrency.
The
PENDINGstatus is currently not used, but it is present in the database and schemas incore. The schemas unfortunately treat the status field as a closed enum, which means extending it can potentially introduce compatibility issues with older SDK versions. So for now we stick withPENDINGeven thoughQUEUEDwould have been more fitting. In the dashboard we useQueuedto label this status.For future enums we can go for extensible types: https://opensource.zalando.com/restful-api-guidelines/#112