Skip to content

Feat: unified deploys for self-hosted and cloud users incl. multi-platform support #2138

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 34 commits into from
Jun 4, 2025

Conversation

nicktrn
Copy link
Collaborator

@nicktrn nicktrn commented Jun 2, 2025

Self-hosters can now deploy just like cloud users: 'npx trigger.dev@v4-beta deploy`

Local vs remote build detection is now fully driven by the webapp. This means registry details must be set via webapp env vars in all cases.

  • Additional supervisor DOCKER_* env vars, incl auth
  • Lock Docker API version
  • Local and remote build multi-platform support
  • Delete registry proxy
  • DEPLOY_REGISTRY_HOST now required
  • Always display deployment errors, if any
  • Add timeout to deploying step
  • Improve image digest retrieval
  • Fix stuck deploy command after remote build/push errors
  • Improve deploy args, make them more docker-like
  • Improve switch command, accept profile name as arg

Copy link

changeset-bot bot commented Jun 2, 2025

🦋 Changeset detected

Latest commit: 71de319

The changes in this PR will be included in the next version bump.

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

Copy link
Contributor

coderabbitai bot commented Jun 2, 2025

Walkthrough

This update implements a major refactor and simplification of the deployment, build, and registry handling logic across the CLI, web application, and backend services. The registry proxy feature and its supporting code are removed, including environment variables and middleware. Deployment and build flows are unified to support a streamlined local or remote build process, with explicit platform and builder handling. Environment schemas and database models are updated to include and require image platform information. The CLI commands for deployment and worker builds are simplified, removing self-hosted and registry-specific flags in favor of a unified local build mode. Docker build logic is refactored to manage buildx builders, support multi-platform builds, and extract image digests from metadata. Error handling and logging are enhanced throughout, and API schemas are updated to reflect the new build and deployment contract.


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.

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Explain this complex logic.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai explain this code block.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and explain its main purpose.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

Support

Need help? Create a ticket on our support page for assistance with any issues or questions.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (Invoked using PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai generate docstrings to generate docstrings for this PR.
  • @coderabbitai generate sequence diagram to generate a sequence diagram of the changes in this PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 5

🧹 Nitpick comments (6)
packages/cli-v3/src/entryPoints/managed-index-controller.ts (1)

95-96: Consider adding validation for platform environment variables.

The platform environment variables are read directly from process.env without validation. Consider validating that they contain expected platform values if they're provided.

-    const buildPlatform = process.env.BUILDPLATFORM;
-    const targetPlatform = process.env.TARGETPLATFORM;
+    const buildPlatform = process.env.BUILDPLATFORM;
+    const targetPlatform = process.env.TARGETPLATFORM;
+    
+    // Validate platform formats if provided
+    if (buildPlatform && !/^[a-z0-9]+\/[a-z0-9]+$/.test(buildPlatform)) {
+      throw new Error(`Invalid BUILDPLATFORM format: ${buildPlatform}`);
+    }
+    if (targetPlatform && !/^[a-z0-9]+\/[a-z0-9]+$/.test(targetPlatform)) {
+      throw new Error(`Invalid TARGETPLATFORM format: ${targetPlatform}`);
+    }
apps/webapp/app/v3/services/createDeploymentBackgroundWorkerV4.server.ts (1)

166-173: Good timeout management improvement!

Scheduling a timeout when entering the "DEPLOYING" state prevents deployments from hanging indefinitely. The configurable timeout via DEPLOY_TIMEOUT_MS provides flexibility.

Consider adding a comment explaining the timeout behavior for future maintainers.

+      // Schedule a timeout to prevent deployments from hanging indefinitely
+      // The timeout will transition the deployment to "TIMED_OUT" status if not completed
       await TimeoutDeploymentService.enqueue(
         deployment.id,
         "DEPLOYING",
         "Indexing timed out",
         new Date(Date.now() + env.DEPLOY_TIMEOUT_MS),
         this._prisma
       );
apps/webapp/app/v3/services/finalizeDeployment.server.ts (1)

125-136: Consider throwing an error for invalid digests instead of silently returning undefined.

While the validation logic is correct, returning undefined for an invalid digest might make issues harder to debug. Since this is a finalization step, it might be better to fail fast with a clear error.

Consider this alternative approach:

 function validatedImageDigest(imageDigest?: string): string | undefined {
   if (!imageDigest) {
     return;
   }
 
   if (!/^sha256:[a-f0-9]{64}$/.test(imageDigest.trim())) {
-    logger.error("Invalid image digest", { imageDigest });
-    return;
+    throw new ServiceValidationError(`Invalid image digest format: ${imageDigest}`);
   }
 
   return imageDigest.trim();
 }
packages/core/src/v3/schemas/api.ts (1)

331-331: Consider adding migration guidance for the deprecated field.

The addition of imagePlatform and deprecation of selfHosted are appropriate changes. However, the deprecation comment could be more helpful.

Consider expanding the deprecation comment to guide users:

-  /** @deprecated This is now determined by the webapp. This is only used to warn users with old CLI versions. */
+  /** 
+   * @deprecated This field is now determined by the webapp based on environment configuration.
+   * It's only used for backward compatibility to warn users with CLI versions < 4.0.0.
+   * This field will be removed in a future version.
+   */
   selfHosted: z.boolean().optional(),

Also applies to: 340-341

packages/cli-v3/src/deploy/buildImage.ts (2)

913-917: Simplify boolean expression.

The ternary operator with boolean literals is unnecessary here.

Apply this diff to simplify:

-      return imageTag.startsWith("localhost") ||
-        imageTag.startsWith("127.0.0.1") ||
-        imageTag.startsWith("0.0.0.0")
-        ? false
-        : true;
+      return !(imageTag.startsWith("localhost") ||
+        imageTag.startsWith("127.0.0.1") ||
+        imageTag.startsWith("0.0.0.0"));
🧰 Tools
🪛 Biome (1.9.4)

[error] 913-917: Unnecessary use of boolean literals in conditional expression.

Simplify your code by directly assigning the result without using a ternary operator.
If your goal is negation, you may use the logical NOT (!) or double NOT (!!) operator for clearer and concise code.
Check for more details about NOT operator.
Unsafe fix: Remove the conditional expression with

(lint/complexity/noUselessTernary)


935-935: Simplify boolean expression.

The ternary operator returning boolean literals is unnecessary.

Apply this diff to simplify:

-      return push ? false : true;
+      return !push;
🧰 Tools
🪛 Biome (1.9.4)

[error] 935-935: Unnecessary use of boolean literals in conditional expression.

Simplify your code by directly assigning the result without using a ternary operator.
If your goal is negation, you may use the logical NOT (!) or double NOT (!!) operator for clearer and concise code.
Check for more details about NOT operator.
Unsafe fix: Remove the conditional expression with

(lint/complexity/noUselessTernary)

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 26b0523 and d00ed33.

⛔ Files ignored due to path filters (2)
  • pnpm-lock.yaml is excluded by !**/pnpm-lock.yaml
  • references/hello-world/trigger.config.ts is excluded by !references/**
📒 Files selected for processing (38)
  • .env.example (1 hunks)
  • apps/supervisor/src/env.ts (1 hunks)
  • apps/supervisor/src/util.ts (1 hunks)
  • apps/supervisor/src/workloadManager/docker.ts (4 hunks)
  • apps/webapp/app/components/GitMetadata.tsx (1 hunks)
  • apps/webapp/app/entry.server.tsx (1 hunks)
  • 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/api.v1.deployments.$deploymentId.background-workers.ts (3 hunks)
  • apps/webapp/app/routes/api.v1.deployments.ts (2 hunks)
  • apps/webapp/app/services/runsReplicationInstance.server.ts (1 hunks)
  • apps/webapp/app/v3/handleSocketIo.server.ts (2 hunks)
  • apps/webapp/app/v3/registryProxy.server.ts (0 hunks)
  • apps/webapp/app/v3/remoteImageBuilder.server.ts (2 hunks)
  • apps/webapp/app/v3/services/createDeploymentBackgroundWorkerV3.server.ts (1 hunks)
  • apps/webapp/app/v3/services/createDeploymentBackgroundWorkerV4.server.ts (4 hunks)
  • apps/webapp/app/v3/services/failDeployment.server.ts (1 hunks)
  • apps/webapp/app/v3/services/finalizeDeployment.server.ts (5 hunks)
  • apps/webapp/app/v3/services/finalizeDeploymentV2.server.ts (9 hunks)
  • apps/webapp/app/v3/services/initializeDeployment.server.ts (5 hunks)
  • apps/webapp/app/v3/services/timeoutDeployment.server.ts (1 hunks)
  • apps/webapp/package.json (0 hunks)
  • apps/webapp/server.ts (0 hunks)
  • internal-packages/database/prisma/migrations/20250530131049_add_worker_deployment_image_platform/migration.sql (1 hunks)
  • internal-packages/database/prisma/schema.prisma (1 hunks)
  • internal-packages/run-engine/src/engine/systems/dequeueSystem.ts (1 hunks)
  • packages/cli-v3/src/apiClient.ts (3 hunks)
  • packages/cli-v3/src/commands/deploy.ts (7 hunks)
  • packages/cli-v3/src/commands/switch.ts (5 hunks)
  • packages/cli-v3/src/commands/workers/build.ts (3 hunks)
  • packages/cli-v3/src/deploy/buildImage.ts (17 hunks)
  • packages/cli-v3/src/entryPoints/managed-index-controller.ts (2 hunks)
  • packages/cli-v3/src/utilities/fileSystem.ts (1 hunks)
  • packages/cli-v3/src/utilities/tempDirectories.ts (1 hunks)
  • packages/core/src/v3/apiClient/core.ts (2 hunks)
  • packages/core/src/v3/schemas/api.ts (3 hunks)
  • packages/core/src/v3/schemas/runEngine.ts (1 hunks)
💤 Files with no reviewable changes (3)
  • apps/webapp/package.json
  • apps/webapp/server.ts
  • apps/webapp/app/v3/registryProxy.server.ts
🧰 Additional context used
🧬 Code Graph Analysis (12)
apps/webapp/app/v3/handleSocketIo.server.ts (1)
apps/webapp/app/v3/services/createDeploymentBackgroundWorkerV3.server.ts (1)
  • CreateDeploymentBackgroundWorkerServiceV3 (21-171)
apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.deployments.$deploymentParam/route.tsx (1)
apps/webapp/app/v3/services/createDeploymentBackgroundWorkerV4.server.ts (1)
  • deployment (178-196)
apps/webapp/app/entry.server.tsx (2)
apps/webapp/app/env.server.ts (1)
  • env (784-784)
apps/webapp/app/v3/remoteImageBuilder.server.ts (1)
  • remoteBuildsEnabled (61-63)
apps/webapp/app/v3/services/timeoutDeployment.server.ts (1)
apps/webapp/app/db.server.ts (1)
  • PrismaClientOrTransaction (21-21)
apps/webapp/app/v3/remoteImageBuilder.server.ts (1)
apps/webapp/app/env.server.ts (1)
  • env (784-784)
apps/webapp/app/v3/services/createDeploymentBackgroundWorkerV4.server.ts (3)
internal-packages/run-engine/src/engine/errors.ts (1)
  • ServiceValidationError (59-67)
apps/webapp/app/v3/services/timeoutDeployment.server.ts (1)
  • TimeoutDeploymentService (7-71)
apps/webapp/app/env.server.ts (1)
  • env (784-784)
packages/cli-v3/src/apiClient.ts (2)
packages/core/src/v3/apiClient/core.ts (1)
  • ApiResult (706-711)
packages/core/src/v3/schemas/api.ts (2)
  • FailDeploymentResponseBody (363-365)
  • FailDeploymentResponseBody (367-367)
apps/webapp/app/v3/services/initializeDeployment.server.ts (3)
apps/webapp/app/v3/remoteImageBuilder.server.ts (2)
  • remoteBuildsEnabled (61-63)
  • createRemoteImageBuild (6-27)
internal-packages/run-engine/src/engine/errors.ts (1)
  • ServiceValidationError (59-67)
apps/webapp/app/env.server.ts (1)
  • env (784-784)
apps/supervisor/src/env.ts (1)
apps/supervisor/src/envUtil.ts (1)
  • BoolEnv (6-12)
apps/webapp/app/v3/services/finalizeDeployment.server.ts (3)
apps/webapp/app/v3/services/failDeployment.server.ts (1)
  • FailDeploymentService (15-56)
apps/webapp/app/v3/services/createDeploymentBackgroundWorkerV4.server.ts (1)
  • deployment (178-196)
apps/webapp/app/v3/services/timeoutDeployment.server.ts (1)
  • TimeoutDeploymentService (7-71)
apps/supervisor/src/workloadManager/docker.ts (3)
apps/supervisor/src/workloadManager/types.ts (1)
  • WorkloadManagerOptions (3-14)
apps/supervisor/src/env.ts (1)
  • env (101-101)
packages/core/src/utils.ts (1)
  • tryCatch (5-18)
packages/cli-v3/src/commands/switch.ts (1)
packages/cli-v3/src/cli/common.ts (2)
  • handleTelemetry (39-49)
  • wrapCommandAction (53-103)
🪛 Biome (1.9.4)
packages/cli-v3/src/deploy/buildImage.ts

[error] 913-917: Unnecessary use of boolean literals in conditional expression.

Simplify your code by directly assigning the result without using a ternary operator.
If your goal is negation, you may use the logical NOT (!) or double NOT (!!) operator for clearer and concise code.
Check for more details about NOT operator.
Unsafe fix: Remove the conditional expression with

(lint/complexity/noUselessTernary)


[error] 935-935: Unnecessary use of boolean literals in conditional expression.

Simplify your code by directly assigning the result without using a ternary operator.
If your goal is negation, you may use the logical NOT (!) or double NOT (!!) operator for clearer and concise code.
Check for more details about NOT operator.
Unsafe fix: Remove the conditional expression with

(lint/complexity/noUselessTernary)

⏰ Context from checks skipped due to timeout of 90000ms (25)
  • GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (10, 10)
  • GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (7, 10)
  • GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (9, 10)
  • GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (8, 10)
  • GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (6, 10)
  • GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (5, 10)
  • GitHub Check: units / internal / 🧪 Unit Tests: Internal (7, 8)
  • GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (1, 10)
  • GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (4, 10)
  • GitHub Check: units / internal / 🧪 Unit Tests: Internal (6, 8)
  • GitHub Check: units / internal / 🧪 Unit Tests: Internal (5, 8)
  • GitHub Check: units / internal / 🧪 Unit Tests: Internal (8, 8)
  • GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (2, 10)
  • GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (3, 10)
  • GitHub Check: units / internal / 🧪 Unit Tests: Internal (1, 8)
  • GitHub Check: units / internal / 🧪 Unit Tests: Internal (4, 8)
  • GitHub Check: units / internal / 🧪 Unit Tests: Internal (3, 8)
  • GitHub Check: units / internal / 🧪 Unit Tests: Internal (2, 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: 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 (64)
apps/webapp/app/services/runsReplicationInstance.server.ts (2)

18-21: Trivial formatting change
An extra space was added after the “🗃️” emoji in the “Runs replication service not enabled” log. This is purely cosmetic and does not affect functionality.


22-24: Trivial formatting change
An extra space was added after the “🗃️” emoji in the “Runs replication service enabled” log. This is purely cosmetic and does not affect functionality.

packages/core/src/v3/apiClient/core.ts (2)

20-20: Good type safety improvement!

Adding the explicit import of ErrorEvent type improves type specificity for error handling.


668-668: Excellent type alignment with EventSource API!

Changing the parameter type from Event to ErrorEvent correctly aligns with the EventSource specification, where onerror handlers specifically receive ErrorEvent objects. This improves type safety and provides better IntelliSense support.

packages/cli-v3/src/utilities/fileSystem.ts (1)

59-59: Typo correction - good catch!

Fixed the typographical error in the function name from safeFeadJSONFile to safeReadJSONFile.

apps/webapp/app/presenters/v3/DeploymentPresenter.server.ts (1)

77-77: Consistent implementation of imagePlatform support!

The addition of imagePlatform field is implemented correctly:

  • Added to the database query selection (line 77)
  • Included in the returned deployment object (line 161)

This follows the established pattern and supports the multi-platform deployment feature mentioned in the PR objectives.

Also applies to: 161-161

internal-packages/database/prisma/schema.prisma (1)

861-861: LGTM! Well-designed schema addition for multi-platform support.

The imagePlatform field addition is well-implemented:

  • Non-nullable with sensible default value prevents breaking changes
  • Default "linux/amd64" covers the most common deployment scenario
  • Supports the unified deployment process for multi-platform builds mentioned in the PR objectives
apps/webapp/app/components/GitMetadata.tsx (1)

12-12: LGTM! Improved UI ordering.

Moving the commit metadata to render after branch metadata provides a more logical display order, showing branch information before commit details.

internal-packages/database/prisma/migrations/20250530131049_add_worker_deployment_image_platform/migration.sql (1)

1-2: LGTM! Migration correctly implements the schema change.

The migration safely adds the imagePlatform column with appropriate constraints:

  • NOT NULL with DEFAULT ensures no data integrity issues
  • Default value matches the schema definition
  • Existing WorkerDeployment records will get the default platform value
apps/webapp/app/v3/services/failDeployment.server.ts (1)

21-26: LGTM! More defensive database query approach.

The change from findUnique to findFirst is a sensible defensive programming practice:

  • Functionally equivalent when data is unique (as expected with friendlyId)
  • More resilient to potential edge cases or data inconsistencies
  • Aligns with similar changes mentioned in other services
  • The environmentId filter provides additional security validation
packages/cli-v3/src/utilities/tempDirectories.ts (1)

40-40: LGTM! Good addition for debugging support.

The addition of process.env.KEEP_TMP_DIRS check provides external control over temporary directory cleanup, which is valuable for debugging build issues or implementing build caching scenarios. The logic correctly maintains backward compatibility.

apps/webapp/app/entry.server.tsx (2)

224-224: Import aligns with registry proxy removal.

Good replacement of the removed registryProxy export with remoteBuildsEnabled function import, supporting the unified deployment approach.


230-234: Useful logging for build mode visibility.

The conditional logging provides clear visibility into whether remote or local builds are enabled, which will help with debugging deployment issues.

internal-packages/run-engine/src/engine/systems/dequeueSystem.ts (1)

487-487: Platform metadata addition looks good.

The addition of imagePlatform to the deployment object in the DequeuedMessage correctly uses optional chaining and aligns with the schema changes for multi-platform support. This provides necessary platform information to workers during task execution.

packages/core/src/v3/schemas/runEngine.ts (1)

242-242: Schema extension properly implemented.

The addition of optional imagePlatform field to the deployment schema is correctly implemented using Zod's optional() method, ensuring backward compatibility while supporting the new multi-platform deployment functionality.

.env.example (1)

83-83: LGTM: Configuration update aligns with registry proxy removal.

The replacement of ENABLE_REGISTRY_PROXY with DEPOT_ORG_ID is consistent with the PR's objective to remove registry proxy functionality and unify the deployment process using Depot.

apps/supervisor/src/util.ts (1)

1-5: Excellent refactoring: Improved platform detection using std-env.

The change from manual process.platform checks to using isMacOS and isWindows constants from std-env improves code readability and centralizes platform detection logic while maintaining the same functional behavior.

apps/webapp/app/routes/api.v1.deployments.$deploymentId.background-workers.ts (2)

8-8: LGTM: Migration to V4 service for multi-platform support.

The upgrade to CreateDeploymentBackgroundWorkerServiceV4 aligns with the PR's objective to add multi-platform deployment support and represents the latest version of the background worker creation service.

Also applies to: 45-45


66-66: Good improvement: Respects error status codes.

The change from hardcoded { status: 400 } to { status: e.status ?? 400 } properly respects the error's status code when available, providing more accurate HTTP responses.

apps/webapp/app/v3/remoteImageBuilder.server.ts (2)

7-7: LGTM: Good refactoring to use centralized function.

The change from direct environment variable checking to using the remoteBuildsEnabled() function improves code maintainability and readability.


61-63: LGTM: Well-designed utility function.

The remoteBuildsEnabled() function centralizes the logic for determining remote build capability. The function name is descriptive and the implementation correctly checks all required environment variables.

apps/webapp/app/v3/services/createDeploymentBackgroundWorkerV3.server.ts (1)

15-21: LGTM: Proper deprecation approach.

The deprecation is well-documented with clear JSDoc comments explaining the reason and replacement. The class renaming with the V3 suffix provides good version clarity.

apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.deployments.$deploymentParam/route.tsx (2)

94-97: LGTM: Useful addition to debug information.

Adding the platform information to the admin debug tooltip enhances troubleshooting capabilities by surfacing the imagePlatform field that's now part of the deployment data structure.


237-269: LGTM: Improved conditional rendering logic.

The refactoring from mutually exclusive conditional rendering to independent conditions is a good improvement. This allows both deployment errors and task information to be displayed simultaneously when both exist, providing better visibility into deployment state.

apps/webapp/app/v3/services/timeoutDeployment.server.ts (1)

49-50: LGTM: Good API improvements with transaction support.

The changes enhance the service by:

  1. Adding optional transaction support via the tx parameter for better database consistency
  2. Correcting the parameter name from id to deploymentId for better clarity
  3. Properly passing the transaction context through to the underlying queue operation

These changes improve the API design and maintain transaction integrity.

Also applies to: 55-55, 63-63

packages/cli-v3/src/entryPoints/managed-index-controller.ts (1)

114-140: Excellent error handling improvement with appropriate logging.

The change from direct await to capturing the response and adding structured error handling is a significant improvement. The decision to not fail the deployment for multi-platform scenarios is well-reasoned and documented.

apps/webapp/app/routes/api.v1.deployments.ts (2)

37-37: LGTM: Consistent with unified image reference handling.

The change from imageTag to imageRef aligns with the broader refactoring to use unified image references throughout the system.


46-47: Good addition of platform awareness to deployment response.

Adding imagePlatform to the response provides essential platform information for deployments, supporting the multi-platform deployment feature.

apps/supervisor/src/env.ts (1)

44-51: Well-structured Docker configuration additions.

The new Docker environment variables provide comprehensive configuration support:

  • DOCKER_API_VERSION with a reasonable default
  • Optional platform override capability
  • Registry authentication support
  • Configurable image digest handling

The use of BoolEnv for DOCKER_STRIP_IMAGE_DIGEST is consistent with the existing pattern in the codebase.

apps/webapp/app/v3/services/initializeDeployment.server.ts (5)

49-54: Appropriate validation for deployment configuration conflict.

The validation correctly prevents self-hosted deployments when remote builds are enabled, maintaining configuration consistency. The error message is clear and informative.


56-56: Simplified external build data creation logic.

Removing the conditional logic and always calling createRemoteImageBuild simplifies the code flow. The function itself handles the case when remote builds are not enabled by returning undefined.


71-75: Simplified and unified image reference construction.

The unified imageRef construction eliminates separate managed/unmanaged logic, making the code cleaner and more maintainable. The approach aligns with the unified deployment strategy.


98-99: Good integration of platform awareness.

Adding imagePlatform from environment configuration ensures consistent platform specification across the deployment lifecycle. This supports the multi-platform deployment feature effectively.


59-59:

✅ Verification successful

Verify consistency of user lookup method change.

The change from findUnique to findFirst maintains the same filtering criteria but changes the query semantics. Ensure this change is intentional and consistent with similar lookups throughout the codebase.


🏁 Script executed:

#!/bin/bash
# Description: Check for other user lookup patterns in the codebase to ensure consistency
# Expected: Find other instances of user lookup by ID with org membership filtering

rg -A 10 -B 2 "user\.find(Unique|First)" --type ts | grep -A 15 -B 5 "orgMemberships"

Length of output: 1672


findFirst is correct for nested orgMemberships filter

Prisma’s findUnique only accepts unique fields, so switching to findFirst is intentional and required when filtering on orgMemberships. Other lookups (e.g. in apps/webapp/app/services/apiAuth.server.ts) use findUnique solely by id, so there’s no inconsistency.

packages/cli-v3/src/apiClient.ts (2)

293-293: Good improvement to promise handling!

Removing the reject parameter and always resolving with an ApiResult type is a cleaner pattern that avoids unhandled promise rejections and provides more predictable error handling for consumers.


312-321: Excellent error handling enhancement!

Prefixing error messages with HTTP status codes provides valuable debugging context. The implementation correctly handles cases where status might be undefined.

Also applies to: 332-336

apps/webapp/app/env.server.ts (1)

269-269: Verify breaking change is properly communicated.

Making DEPLOY_REGISTRY_HOST required is a breaking change that will cause the application to fail at startup if this environment variable is not set. Please ensure:

  1. This breaking change is documented in the PR description ✓ (confirmed)
  2. Migration guide is provided for existing deployments
  3. The error message when this variable is missing is clear and actionable

The addition of DEPLOY_IMAGE_PLATFORM with a sensible default is a good approach for backward compatibility.

Also applies to: 273-273

apps/webapp/app/v3/services/createDeploymentBackgroundWorkerV4.server.ts (2)

12-12: LGTM!

Good practice using versioning (V4 suffix) for the service class to maintain backward compatibility.

Also applies to: 14-14


23-31: Good telemetry practice!

Recording platform information in span attributes will be valuable for debugging multi-platform deployments.

apps/webapp/app/v3/services/finalizeDeployment.server.ts (3)

10-11: LGTM!

The new imports align with the removal of the registry proxy and the addition of proper failure and timeout handling services.


63-63: LGTM!

The image digest validation and conditional appending to the image reference is implemented correctly. The code properly handles cases where the digest might be missing or invalid.

Also applies to: 73-74


78-78: LGTM!

Correctly dequeues any pending timeout jobs after successful deployment, preventing unnecessary timeout notifications.

apps/webapp/app/v3/services/finalizeDeploymentV2.server.ts (4)

20-24: LGTM!

Good backward compatibility approach - delegating to v1 service when remote builds are not enabled ensures existing deployments continue to work.


91-94: LGTM!

Appropriate validation ensuring all deployments have an image reference, which aligns with the new unified deployment approach.


115-115: LGTM!

The simplified image reference handling is cleaner and aligns with the requirement that all deployments must have an image reference.

Also applies to: 155-155, 181-181


133-135: ⚠️ Potential issue

Fix undefined variable error.

The finalizedDeployment variable is used on line 135 but never defined since line 133 doesn't assign the result of finalizeService.call().

Apply this fix:

 const finalizeService = new FinalizeDeploymentService();
-await finalizeService.call(authenticatedEnv, id, body);
+const finalizedDeployment = await finalizeService.call(authenticatedEnv, id, body);

 return finalizedDeployment;

Likely an incorrect or invalid review comment.

packages/core/src/v3/schemas/api.ts (2)

57-58: LGTM!

The addition of optional buildPlatform and targetPlatform fields supports the new multi-platform build capabilities mentioned in the PR objectives.


296-299: LGTM!

The schema simplification correctly reflects the new deployment finalization process where only the image digest is needed, as the image reference is now set during deployment initialization.

packages/cli-v3/src/commands/workers/build.ts (2)

322-345: LGTM! Improved build parameter structure aligns with unified deployment process.

The refactoring from selfHosted/buildPlatform to isLocalBuild/imagePlatform provides better clarity. The platform is now sourced from the deployment response, ensuring consistency across the system.


440-440: Using deployment.imageTag in output message is the correct approach.

This ensures consistency by using the deployment's authoritative image tag rather than the build result.

apps/supervisor/src/workloadManager/docker.ts (3)

21-54: Excellent implementation of Docker configuration with proper security practices.

The constructor properly:

  • Configures Docker API version for compatibility
  • Logs platform override information for debugging
  • Securely handles registry credentials (logging username/URL but not password)
  • Provides clear warnings when authentication is not configured

120-217: Well-architected platform-aware Docker image handling with robust error recovery.

The implementation excellently handles:

  • Optional image digest stripping for flexibility
  • Smart platform compatibility checking that inspects local images and pulls when architecture mismatches
  • Comprehensive error handling using the tryCatch utility pattern
  • Detailed logging for debugging multi-platform deployment issues

266-272: Clean implementation of stream reading helper.

The readAllChunks function provides a simple way to consume the Docker image pull response stream.

packages/cli-v3/src/commands/switch.ts (2)

27-84: Excellent enhancement to support direct profile switching with robust error handling.

The implementation provides:

  • Convenient direct profile switching via positional argument
  • Appropriate behavior for CI/TTY environments
  • Clear, actionable error messages directing users to the login command
  • Efficient early returns when already on the target profile

54-55: Improved error handling with consistent behavior across all code paths.

The changes ensure:

  • Proper error propagation by throwing instead of silent returns
  • Consistent outro messages before errors
  • Duplicate current profile checks in both direct and interactive modes

Also applies to: 103-106

packages/cli-v3/src/commands/deploy.ts (6)

48-60: LGTM! Clean refactoring of deployment options.

The replacement of explicit self-hosted flags with local build options (load, push, network, builder) simplifies the interface and aligns well with the unified deployment approach.


316-316: Good simplification of local build detection.

Deriving isLocalBuild from the absence of externalBuildData is a cleaner approach than using explicit flags.


319-328: Excellent fail-fast verification for local builds.

Checking for Docker buildx availability before attempting local builds prevents cryptic errors later in the process.


397-433: LGTM! Build options properly aligned with new structure.

The buildImage call correctly uses the new options including isLocalBuild, imagePlatform, and local build options.


514-514: Correct usage of imageDigest for finalization.

Using imageDigest instead of imageReference properly aligns with the removal of registry proxy dependencies.


635-646: Good UX improvement for error diagnostics.

Displaying the last few lines of logs (filtered to remove comment lines) provides immediate context without overwhelming the user.

packages/cli-v3/src/deploy/buildImage.ts (3)

254-268: Excellent metadata parsing implementation.

Using a Zod schema for BuildKit metadata parsing is a robust approach that provides type safety and validation. The error handling with logging is also well implemented.

Also applies to: 489-505


307-404: Comprehensive Docker builder management.

The implementation properly handles:

  • Builder existence checks
  • Network configuration validation
  • Builder recreation when network changes
  • Clear error messages at each step

This is a significant improvement in local build reliability.


860-887: Good multi-platform build support.

The QEMU registration check and setup for multi-platform builds on Linux is well implemented. This prevents build failures when targeting multiple platforms.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🔭 Outside diff range comments (2)
packages/cli-v3/src/deploy/buildImage.ts (2)

100-104: ⚠️ Potential issue

Update the error message to reflect the new deployment process.

The error message references the --self-hosted flag which has been removed according to the PR objectives. Users now deploy using npx trigger.dev@v4-beta deploy for both cloud and self-hosted deployments.

Apply this diff to fix the error message:

-      "Failed to initialize deployment. The deployment does not have any external build data. To deploy this project, you must use the --self-hosted flag to build and push the image yourself."
+      "Failed to initialize deployment. The deployment does not have any external build data. To deploy this project, you must use the local build mode."

307-528: 🛠️ Refactor suggestion

Consider breaking down this function for better maintainability.

The localBuildImage function is over 200 lines and handles multiple responsibilities. Consider extracting the following into separate functions:

  • Builder management logic (lines 310-404)
  • Docker build arguments preparation (lines 406-458)
  • Build execution and error handling (lines 465-482)
  • Metadata parsing and digest extraction (lines 484-500)

This would improve readability, testability, and make the code easier to maintain.

🧹 Nitpick comments (7)
packages/cli-v3/src/deploy/buildImage.ts (7)

808-812: Make URL normalization more robust.

The current implementation could replace "localhost" in unexpected places (e.g., path segments). Consider parsing the URL properly to only replace the hostname.

Apply this more robust implementation:

-function normalizeApiUrlForBuild(apiUrl: string): string {
-  return apiUrl.replace("localhost", "host.docker.internal");
-}
+function normalizeApiUrlForBuild(apiUrl: string): string {
+  try {
+    const url = new URL(apiUrl);
+    if (url.hostname === "localhost") {
+      url.hostname = "host.docker.internal";
+    }
+    return url.toString();
+  } catch {
+    // Fallback for invalid URLs
+    return apiUrl;
+  }
+}

898-918: Simplify the boolean logic in shouldPush function.

The ternary operators are unnecessary when the condition already evaluates to a boolean.

Apply this simplification:

-function shouldPush(imageTag: string, push?: boolean) {
-  switch (push) {
-    case true: {
-      return true;
-    }
-    case false: {
-      return false;
-    }
-    case undefined: {
-      return imageTag.startsWith("localhost") ||
-        imageTag.startsWith("127.0.0.1") ||
-        imageTag.startsWith("0.0.0.0")
-        ? false
-        : true;
-    }
-    default: {
-      assertExhaustive(push);
-    }
-  }
-}
+function shouldPush(imageTag: string, push?: boolean) {
+  switch (push) {
+    case true: {
+      return true;
+    }
+    case false: {
+      return false;
+    }
+    case undefined: {
+      return !(imageTag.startsWith("localhost") ||
+        imageTag.startsWith("127.0.0.1") ||
+        imageTag.startsWith("0.0.0.0"));
+    }
+    default: {
+      assertExhaustive(push);
+    }
+  }
+}
🧰 Tools
🪛 Biome (1.9.4)

[error] 908-912: Unnecessary use of boolean literals in conditional expression.

Simplify your code by directly assigning the result without using a ternary operator.
If your goal is negation, you may use the logical NOT (!) or double NOT (!!) operator for clearer and concise code.
Check for more details about NOT operator.
Unsafe fix: Remove the conditional expression with

(lint/complexity/noUselessTernary)


920-936: Simplify the boolean logic in shouldLoad function.

The ternary operator is unnecessary when the condition already evaluates to a boolean.

Apply this simplification:

-function shouldLoad(load?: boolean, push?: boolean) {
-  switch (load) {
-    case true: {
-      return true;
-    }
-    case false: {
-      return false;
-    }
-    case undefined: {
-      return push ? false : true;
-    }
-    default: {
-      assertExhaustive(load);
-    }
-  }
-}
+function shouldLoad(load?: boolean, push?: boolean) {
+  switch (load) {
+    case true: {
+      return true;
+    }
+    case false: {
+      return false;
+    }
+    case undefined: {
+      return !push;
+    }
+    default: {
+      assertExhaustive(load);
+    }
+  }
+}
🧰 Tools
🪛 Biome (1.9.4)

[error] 930-930: Unnecessary use of boolean literals in conditional expression.

Simplify your code by directly assigning the result without using a ternary operator.
If your goal is negation, you may use the logical NOT (!) or double NOT (!!) operator for clearer and concise code.
Check for more details about NOT operator.
Unsafe fix: Remove the conditional expression with

(lint/complexity/noUselessTernary)


814-831: Add logging when falling back to default IP.

When no suitable network interface is found, the function silently falls back to "127.0.0.1". This could cause connectivity issues in certain Docker configurations.

Consider adding debug logging:

 function getHostIP() {
   const interfaces = networkInterfaces();
 
   for (const [name, iface] of Object.entries(interfaces)) {
     if (!iface) {
       continue;
     }
 
     for (const net of iface) {
       // Skip internal/loopback and non-IPv4 addresses
       if (!net.internal && net.family === "IPv4") {
         return net.address;
       }
     }
   }
 
+  logger.debug("No suitable network interface found, falling back to 127.0.0.1");
   return "127.0.0.1";
 }

841-849: Improve QEMU registration check robustness.

The current implementation only checks for qemu-aarch64 which may not be sufficient for all multi-platform build scenarios. Also, errors are silently ignored.

Consider checking multiple architectures and adding debug logging:

 function isQemuRegistered() {
   try {
-    // Check a single QEMU handler
-    const binfmt = readFileSync("/proc/sys/fs/binfmt_misc/qemu-aarch64", "utf8");
-    return binfmt.includes("enabled");
+    // Check common QEMU handlers for cross-platform builds
+    const handlers = ["qemu-aarch64", "qemu-arm", "qemu-x86_64"];
+    for (const handler of handlers) {
+      try {
+        const binfmt = readFileSync(`/proc/sys/fs/binfmt_misc/${handler}`, "utf8");
+        if (binfmt.includes("enabled")) {
+          return true;
+        }
+      } catch {
+        // Handler doesn't exist, continue checking
+      }
+    }
+    return false;
   } catch (e) {
+    logger.debug("Failed to check QEMU registration", { error: e });
     return false;
   }
 }

254-268: Extract common metadata parsing logic.

Both remoteBuildImage and localBuildImage have identical metadata parsing logic. Consider extracting this into a shared helper function to reduce duplication.

Add a helper function:

async function parseImageMetadata(metadataPath: string, onLog?: (log: string) => void): Promise<string | undefined> {
  const rawMetadata = await safeReadJSONFile(metadataPath);
  const meta = BuildKitMetadata.safeParse(rawMetadata);
  
  if (!meta.success) {
    logger.error("Failed to parse metadata.json", {
      errors: meta.error.message,
      path: metadataPath,
    });
    return undefined;
  }
  
  logger.debug("Parsed metadata.json", { metadata: meta.data, path: metadataPath });
  return meta.data["containerimage.digest"];
}

Then use it in both functions:

-    const metadataPath = join(options.cwd, "metadata.json");
-    const rawMetadata = await safeReadJSONFile(metadataPath);
-
-    const meta = BuildKitMetadata.safeParse(rawMetadata);
-
-    let digest: string | undefined;
-    if (!meta.success) {
-      logger.error("Failed to parse metadata.json", {
-        errors: meta.error.message,
-        path: metadataPath,
-      });
-    } else {
-      logger.debug("Parsed metadata.json", { metadata: meta.data, path: metadataPath });
-      digest = meta.data["containerimage.digest"];
-    }
+    const metadataPath = join(options.cwd, "metadata.json");
+    const digest = await parseImageMetadata(metadataPath, options.onLog);

Also applies to: 484-500


332-373: Consider adding comments to explain the builder network logic.

The builder network configuration logic is complex and would benefit from additional comments explaining:

  • Why network configuration matters for the builder
  • When and why a builder needs to be recreated
  • The expected format of the docker buildx inspect output

This would help future maintainers understand the reasoning behind this implementation.

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between d00ed33 and 078c675.

📒 Files selected for processing (4)
  • apps/webapp/app/v3/services/failDeployment.server.ts (1 hunks)
  • apps/webapp/app/v3/services/finalizeDeployment.server.ts (5 hunks)
  • packages/cli-v3/src/commands/deploy.ts (7 hunks)
  • packages/cli-v3/src/deploy/buildImage.ts (17 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
  • apps/webapp/app/v3/services/failDeployment.server.ts
  • apps/webapp/app/v3/services/finalizeDeployment.server.ts
  • packages/cli-v3/src/commands/deploy.ts
🧰 Additional context used
🧠 Learnings (1)
packages/cli-v3/src/deploy/buildImage.ts (1)
Learnt from: nicktrn
PR: triggerdotdev/trigger.dev#2138
File: packages/cli-v3/src/deploy/buildImage.ts:342-342
Timestamp: 2025-06-02T14:41:17.319Z
Learning: The `docker buildx inspect` command does not support the `--format` flag, unlike the regular `docker inspect` command. When parsing output from `docker buildx inspect`, regex-based parsing is the appropriate approach since structured JSON output is not available.
🪛 Biome (1.9.4)
packages/cli-v3/src/deploy/buildImage.ts

[error] 908-912: Unnecessary use of boolean literals in conditional expression.

Simplify your code by directly assigning the result without using a ternary operator.
If your goal is negation, you may use the logical NOT (!) or double NOT (!!) operator for clearer and concise code.
Check for more details about NOT operator.
Unsafe fix: Remove the conditional expression with

(lint/complexity/noUselessTernary)


[error] 930-930: Unnecessary use of boolean literals in conditional expression.

Simplify your code by directly assigning the result without using a ternary operator.
If your goal is negation, you may use the logical NOT (!) or double NOT (!!) operator for clearer and concise code.
Check for more details about NOT operator.
Unsafe fix: Remove the conditional expression with

(lint/complexity/noUselessTernary)

⏰ Context from checks skipped due to timeout of 90000ms (24)
  • GitHub Check: units / internal / 🧪 Unit Tests: Internal (5, 8)
  • GitHub Check: units / internal / 🧪 Unit Tests: Internal (3, 8)
  • GitHub Check: units / internal / 🧪 Unit Tests: Internal (7, 8)
  • GitHub Check: units / internal / 🧪 Unit Tests: Internal (8, 8)
  • GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (10, 10)
  • GitHub Check: units / internal / 🧪 Unit Tests: Internal (6, 8)
  • GitHub Check: units / internal / 🧪 Unit Tests: Internal (4, 8)
  • GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (5, 10)
  • GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (4, 10)
  • GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (9, 10)
  • GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (3, 10)
  • GitHub Check: units / internal / 🧪 Unit Tests: Internal (1, 8)
  • GitHub Check: units / internal / 🧪 Unit Tests: Internal (2, 8)
  • GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (2, 10)
  • GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (6, 10)
  • GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (8, 10)
  • GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (7, 10)
  • GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (1, 10)
  • 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 (windows-latest - npm)
  • GitHub Check: typecheck / typecheck
  • GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (1)
packages/cli-v3/src/deploy/buildImage.ts (1)

657-660: Multi-platform build support looks good!

The addition of TARGETPLATFORM and BUILDPLATFORM ARGs and environment variables follows Docker best practices for multi-platform builds. This enables proper cross-compilation support.

Also applies to: 769-772

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🧹 Nitpick comments (3)
apps/supervisor/src/workloadManager/docker.ts (3)

41-54: Consider security implications of logging registry username.

The Docker registry authentication setup is well-implemented with proper conditional logic. However, consider whether logging the registry username in plain text could pose security concerns in your environment, especially if logs are stored or transmitted insecurely.

If you prefer to avoid logging sensitive information, consider this adjustment:

      this.logger.info("🐋 Using Docker registry credentials", {
-       username: env.DOCKER_REGISTRY_USERNAME,
+       username: "[REDACTED]",
        url: env.DOCKER_REGISTRY_URL,
      });

121-125: Consider more robust image digest stripping.

The current image digest stripping logic works for standard cases but makes assumptions about image reference format. Consider adding validation or documentation about the expected format.

For a more robust approach, consider:

  let imageRef = opts.image;

  if (env.DOCKER_STRIP_IMAGE_DIGEST) {
-   imageRef = opts.image.split("@")[0]!;
+   // Strip digest (everything after the last "@" if it contains "sha256:")
+   const atIndex = opts.image.lastIndexOf("@sha256:");
+   if (atIndex !== -1) {
+     imageRef = opts.image.substring(0, atIndex);
+   }
  }

267-273: LGTM: Clean helper function with potential for improvement.

The readAllChunks helper function is well-implemented for reading Docker API responses. Consider adding error handling and making it more generic if it might be reused elsewhere.

For enhanced robustness, consider:

-async function readAllChunks(reader: NodeJS.ReadableStream) {
+async function readAllChunks(reader: NodeJS.ReadableStream): Promise<string[]> {
  const chunks = [];
-  for await (const chunk of reader) {
-    chunks.push(chunk.toString());
+  try {
+    for await (const chunk of reader) {
+      chunks.push(chunk.toString());
+    }
+  } catch (error) {
+    throw new Error(`Failed to read stream chunks: ${error instanceof Error ? error.message : String(error)}`);
  }
  return chunks;
}
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 96d0263 and 5b7ea14.

📒 Files selected for processing (5)
  • .env.example (1 hunks)
  • .github/workflows/unit-tests-webapp.yml (2 hunks)
  • apps/supervisor/src/env.ts (2 hunks)
  • apps/supervisor/src/workloadManager/docker.ts (5 hunks)
  • packages/cli-v3/src/entryPoints/managed/logger.ts (3 hunks)
✅ Files skipped from review due to trivial changes (1)
  • packages/cli-v3/src/entryPoints/managed/logger.ts
🚧 Files skipped from review as they are similar to previous changes (3)
  • .env.example
  • .github/workflows/unit-tests-webapp.yml
  • apps/supervisor/src/env.ts
🧰 Additional context used
🧬 Code Graph Analysis (1)
apps/supervisor/src/workloadManager/docker.ts (3)
apps/supervisor/src/workloadManager/types.ts (1)
  • WorkloadManagerOptions (3-14)
apps/supervisor/src/env.ts (1)
  • env (102-102)
packages/core/src/utils.ts (1)
  • tryCatch (5-18)
⏰ Context from checks skipped due to timeout of 90000ms (5)
  • GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (6, 10)
  • GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (4, 10)
  • GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - npm)
  • GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - pnpm)
  • GitHub Check: typecheck / typecheck
🔇 Additional comments (5)
apps/supervisor/src/workloadManager/docker.ts (5)

17-18: LGTM: Well-structured field additions.

The new private readonly fields for authentication and platform override are properly typed and follow good encapsulation practices.


33-39: LGTM: Excellent platform override implementation.

The platform override setup includes informative logging that will be valuable for debugging multi-platform builds. The inclusion of both target and host platform information is particularly helpful.


75-75: LGTM: Consistent environment variable addition.

The PRETTY_LOGS environment variable addition follows the established pattern and will improve logging output formatting in containers.


137-139: LGTM: Proper platform override implementation.

The conditional platform override in container creation options correctly follows Docker API conventions and supports the multi-platform build objectives.


21-23:

✅ Verification successful

Verify environment variable validation for Docker API version.

The Docker API version is now configurable via environment variables, which supports the PR objective of locking the API version for compatibility. Ensure that env.DOCKER_API_VERSION has proper validation in the environment schema to prevent runtime issues with invalid versions.


🏁 Script executed:

#!/bin/bash
# Description: Verify that DOCKER_API_VERSION has proper validation in the environment schema.

# Search for DOCKER_API_VERSION validation in environment configuration
rg -A 5 -B 5 "DOCKER_API_VERSION" apps/supervisor/src/env.ts

Length of output: 524


Environment variable validation confirmed for DOCKER_API_VERSION

The apps/supervisor/src/env.ts schema defines:

  • DOCKER_API_VERSION: z.string().default("v1.41")

This ensures:

  • Any provided value is validated as a string.
  • A safe default ("v1.41") is used when the environment variable is missing.

No further validation or fallback handling is needed.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (1)
.changeset/ninety-games-grow.md (1)

6-11: Nit: use “deployment” instead of “deploy”
In the fifth bullet (line 10), consider replacing “The deploy --self-hosted flag” with “The deployment --self-hosted flag” for grammatical accuracy.

🧰 Tools
🪛 LanguageTool

[grammar] ~10-~10: The word ‘deploy’ is a verb. Did you mean the noun “deployment” (= release, placement)?
Context: ...s now fully managed by the webapp - The deploy --self-hosted flag is no longer requi...

(PREPOSITION_VERB)

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 9c90301 and 71de319.

⛔ Files ignored due to path filters (1)
  • pnpm-lock.yaml is excluded by !**/pnpm-lock.yaml
📒 Files selected for processing (1)
  • .changeset/ninety-games-grow.md (1 hunks)
🧰 Additional context used
🪛 LanguageTool
.changeset/ninety-games-grow.md

[grammar] ~10-~10: The word ‘deploy’ is a verb. Did you mean the noun “deployment” (= release, placement)?
Context: ...s now fully managed by the webapp - The deploy --self-hosted flag is no longer requi...

(PREPOSITION_VERB)

⏰ Context from checks skipped due to timeout of 90000ms (8)
  • GitHub Check: units / internal / 🧪 Unit Tests: Internal (5, 8)
  • GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (6, 10)
  • GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (4, 10)
  • GitHub Check: units / internal / 🧪 Unit Tests: Internal (6, 8)
  • GitHub Check: units / internal / 🧪 Unit Tests: Internal (2, 8)
  • GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - npm)
  • GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - pnpm)
  • GitHub Check: typecheck / typecheck
🔇 Additional comments (1)
.changeset/ninety-games-grow.md (1)

1-4: Frontmatter format is correct
The YAML header properly declares the package bumps for trigger.dev and @trigger.dev/core and uses the expected changeset syntax.

@nicktrn nicktrn merged commit f603725 into main Jun 4, 2025
33 checks passed
@nicktrn nicktrn deleted the feat/detect-self-hosted branch June 4, 2025 14:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants