feat: installing status for deployments #2544
Conversation
🦋 Changeset detectedLatest commit: 303986e The changes in this PR will be included in the next version bump. This PR includes changesets to release 23 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
WalkthroughThis change introduces a new deployment status, "INSTALLING", across the application's deployment lifecycle. The status is added to enums, database schema, UI components, API response types, and deployment progression logic. The database migration updates the WorkerDeploymentStatus enum and the WorkerDeployment table with a new installedAt column. Backend logic now transitions deployments through INSTALLING, and appropriate error handling, state progression, and timeout handling are adjusted. UI and CLI sections now recognize and correctly display or process the new INSTALLING state. API input/output schemas are renamed and refactored to reflect deployment progression rather than just start. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. 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: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/core/src/v3/schemas/api.ts (2)
1167-1170: Add INSTALLING to deployment list filtersClients won’t be able to filter for INSTALLING deployments with the current enum.
Apply this diff:
- .enum(["PENDING", "BUILDING", "DEPLOYING", "DEPLOYED", "FAILED", "CANCELED", "TIMED_OUT"]) + .enum([ + "PENDING", + "INSTALLING", + "BUILDING", + "DEPLOYING", + "DEPLOYED", + "FAILED", + "CANCELED", + "TIMED_OUT", + ])
1209-1217: Add INSTALLING to list response item statusList responses can contain INSTALLING; the schema must permit it.
Apply this diff:
status: z.enum([ "PENDING", + "INSTALLING", "BUILDING", "DEPLOYING", "DEPLOYED", "FAILED", "CANCELED", "TIMED_OUT", ]),
🧹 Nitpick comments (3)
internal-packages/database/prisma/migrations/20250923182708_add_installing_status_to_deployments/migration.sql (1)
1-1: Postgres enum migrations can fail inside a transactionALTER TYPE ... ADD VALUE cannot run inside a transaction in some Postgres versions. Ensure Prisma Migrate executes this statement outside a transaction, or consider IF NOT EXISTS for idempotency if supported by your PG version.
Would you like me to check for Prisma’s migration engine behavior in this repo with a quick script?
apps/webapp/app/components/runs/v3/DeploymentStatus.tsx (1)
131-140: Comment contradicts array contentComment says PENDING and CANCELED are omitted, but PENDING is included. Fix the comment or the array.
Apply this diff to fix the comment:
-// PENDING and CANCELED are not used so are ommited from the UI +// CANCELED is not used in this listapps/webapp/app/routes/api.v1.deployments.$deploymentId.progress.ts (1)
62-66: 409 error message: centralize to avoid driftMessage encodes the “progressable” states. Consider deriving this from a shared constant in the service to prevent future inconsistency.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
.changeset/kind-kids-teach.md(1 hunks)apps/webapp/app/components/runs/v3/DeploymentStatus.tsx(5 hunks)apps/webapp/app/routes/api.v1.deployments.$deploymentId.progress.ts(4 hunks)apps/webapp/app/v3/services/deployment.server.ts(4 hunks)internal-packages/database/prisma/migrations/20250923182708_add_installing_status_to_deployments/migration.sql(1 hunks)internal-packages/database/prisma/schema.prisma(2 hunks)packages/cli-v3/src/commands/deploy.ts(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/components/runs/v3/DeploymentStatus.tsxapps/webapp/app/routes/api.v1.deployments.$deploymentId.progress.tspackages/core/src/v3/schemas/api.tspackages/cli-v3/src/commands/deploy.tsapps/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/components/runs/v3/DeploymentStatus.tsxapps/webapp/app/routes/api.v1.deployments.$deploymentId.progress.tspackages/core/src/v3/schemas/api.tsapps/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/components/runs/v3/DeploymentStatus.tsxapps/webapp/app/routes/api.v1.deployments.$deploymentId.progress.tsapps/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/routes/api.v1.deployments.$deploymentId.progress.tsapps/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/routes/api.v1.deployments.$deploymentId.progress.tsapps/webapp/app/v3/services/deployment.server.ts
🧬 Code graph analysis (3)
apps/webapp/app/routes/api.v1.deployments.$deploymentId.progress.ts (1)
packages/core/src/v3/schemas/api.ts (2)
ProgressDeploymentRequestBody(380-384)ProgressDeploymentRequestBody(386-386)
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/v3/services/deployment.server.ts (3)
apps/webapp/app/v3/services/createDeploymentBackgroundWorkerV4.server.ts (1)
deployment(182-200)apps/webapp/app/v3/remoteImageBuilder.server.ts (1)
createRemoteImageBuild(7-30)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). (23)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (1, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (7, 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 (2, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (4, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (3, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (7, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (5, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (5, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (8, 8)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - npm)
- 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 (3, 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 (ubuntu-latest - npm)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - pnpm)
- GitHub Check: e2e / 🧪 CLI v3 tests (ubuntu-latest - pnpm)
- GitHub Check: typecheck / typecheck
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (25)
.changeset/kind-kids-teach.md (1)
6-6: Changelog entry looks goodINSTALLING addition is clearly stated. Nothing else to change here.
internal-packages/database/prisma/migrations/20250923182708_add_installing_status_to_deployments/migration.sql (1)
3-3: Installed timestamp: nullable is fineinstalledAt as TIMESTAMP(3) nullable matches the transitional nature. No changes needed.
packages/cli-v3/src/commands/deploy.ts (1)
677-684: Handle INSTALLING in failDeploy: LGTMTreating INSTALLING like PENDING/BUILDING/DEPLOYING here is consistent with the new lifecycle.
internal-packages/database/prisma/schema.prisma (2)
1767-1771: Timeline fields: placement and nullability look correctstartedAt → installedAt → builtAt → deployedAt sequencing makes sense. No changes needed.
1791-1794: Enum value INSTALLING added: LGTMMatches the new lifecycle. Note that API list filters must also include INSTALLING (see core schemas).
packages/core/src/v3/schemas/api.ts (5)
380-386: Rename to ProgressDeploymentRequestBody: LGTMSchema shape and naming align with the new /progress endpoint.
386-386: Type alias updated: LGTMType export matches the renamed schema.
466-475: GetDeploymentResponseBody includes INSTALLING: LGTMPublic response now reflects the intermediate state.
422-431: Initial status union (PENDING | BUILDING): confirm intentIf initializeDeployment should never start at INSTALLING (server handles transition), this is fine. If remote build server may initialize directly to INSTALLING in future, consider adding it.
1-1281: No lingering StartDeploymentRequestBody / startDeployment references foundSearch found only StartDeploymentIndexing* and startDeploymentIndexing in:
- packages/core/src/v3/schemas/api.ts (lines ~357,362,364,369,370)
- packages/cli-v3/src/apiClient.ts (lines ~23,24,461,467)
apps/webapp/app/components/runs/v3/DeploymentStatus.tsx (4)
56-59: Spinner grouping includes INSTALLING: LGTMINSTALLING grouped with BUILDING/DEPLOYING is appropriate.
82-85: Color mapping includes INSTALLING: LGTMConsistent with existing pending-like states.
103-105: Title for INSTALLING: LGTM“Installing…” reads well.
147-148: Description for INSTALLING: LGTMClear and consistent with the new state.
apps/webapp/app/routes/api.v1.deployments.$deploymentId.progress.ts (3)
2-2: Import schema from subpath: LGTMFollows webapp guideline to import from @trigger.dev/core subpaths.
38-39: Request body parsing with ProgressDeploymentRequestBody: LGTMValidation aligns with the updated schema.
46-51: Use progressDeployment instead of start: LGTMNew flow (PENDING → INSTALLING → BUILDING) is reflected here.
apps/webapp/app/v3/services/deployment.server.ts (8)
11-23: Helpful docblockClear behavior and non-idempotence note. Good addition.
24-28: progressDeployment signature: LGTMUpdates, friendlyId, and env usage are appropriate.
52-61: Only progress PENDING/INSTALLING: LGTMValidation logic matches the new state machine. Warn log is helpful.
66-85: PENDING → INSTALLING transition guarded by status: LGTMupdateMany with status guard avoids TOCTOU; returning count==0 as cannot_be_progressed is correct.
87-92: Create remote build just-in-time: LGTMMoving token acquisition here addresses short‑lived token expiry.
93-117: INSTALLING → BUILDING transition and installedAt semanticsProgressing to BUILDING and setting installedAt now (i.e., install finished) is correct. externalBuildData may be undefined when remote builds are disabled; Prisma will skip updating that field on undefined, which is fine.
If you need externalBuildData explicitly nulled when remote builds are disabled, I can adjust to set null instead of skipping.
119-133: Timeout extension message based on state: LGTMMessage clarity is good; enqueueing by current state is correct.
137-143: Branching on current state result ensures correct timeout contextandThen chain returns the new state to extendTimeout; avoids using stale status. Nice.
This PR adds a new status if the deployment lifecycle:
INSTALLING. This isrelevant only for deployments triggered by the build server, with locally
triggered deployments the install stage is done on the client side.
Now we have a
/progressendpoint which allows progressing fromPENDINGtoINSTALLINGand then toBUILDING, which can be further extended in the futureif we have new statuses. The short-lived Depot build token is only created before
moving to the
BUILDINGstatus to avoid expiration issues.The
/startendpoint is superseded by the newer/progressendpoint. Removingit won't cause trouble as it is not yet used in the SDK or documented in the
API docs.