fix(deployments): misc fixes for the native build server deployment flow #2748
Conversation
|
WalkthroughThree files were modified to improve error handling and resource management. The artifacts API endpoint now generates context-aware error messages for size limit violations based on error data and deployment type, replacing a static message. The deployment server's token retrieval logic was restructured to cache S2 tokens only on cache misses rather than attempting re-caching on hits. The CLI deploy command's error handling was refactored to consistently log structured errors and throw OutroCommandError instead of generic errors across artifact creation, archive reading, and file upload failure paths. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Pre-merge checks and finishing touches❌ Failed checks (2 warnings, 1 inconclusive)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
apps/webapp/app/v3/services/deployment.server.ts (1)
359-423: S2 token caching flow now correctly issues & caches only on cache missThe new composition cleanly uses the cached token on hits and only issues+caches on misses, while safely returning a token even if
setexfails. If you ever want more observability, you could log separately whengetTokenFromCachefails due to a Redis error vs a plain cache miss, but the current behavior is correct and aligned with the stated fix.packages/cli-v3/src/commands/deploy.ts (2)
967-977: Tighten upload error message when only one of error/response is presentRight now the logged message can include
undefined (… …)when onlyuploadErroror onlyuploadResponseis set. Consider building the message defensively to avoid awkward output, e.g.:const message = uploadError?.message ?? (uploadResponse ? `${uploadResponse.statusText} ${uploadResponse.status}` : "Unknown upload error"); log.error(chalk.bold(chalkError(message)));This keeps the structured info while avoiding
undefinedtext in the CLI.
919-983: Optionally clean up the archive file on early failuresIn the native build flow, the temporary archive is only unlinked after a successful upload; if artifact creation, read, or upload fails, the
deploy-*.tar.gzfile is left behind. Not critical, but you might want to best-effort deletearchivePathin those early error branches to prevent.trigger/tmpfrom growing over time:if (!artifactResult.success) { await tryCatch(unlink(archivePath)); // ignore error $deploymentSpinner.stop("Failed creating deployment artifact"); log.error(chalk.bold(chalkError(artifactResult.error))); throw new OutroCommandError("Deployment failed"); }(and similarly in the read/upload failure paths).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
apps/webapp/app/routes/api.v1.artifacts.ts(1 hunks)apps/webapp/app/v3/services/deployment.server.ts(1 hunks)packages/cli-v3/src/commands/deploy.ts(3 hunks)
🧰 Additional context used
📓 Path-based instructions (7)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{ts,tsx}: Use types over interfaces for TypeScript
Avoid using enums; prefer string unions or const objects instead
Files:
packages/cli-v3/src/commands/deploy.tsapps/webapp/app/routes/api.v1.artifacts.tsapps/webapp/app/v3/services/deployment.server.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use function declarations instead of default exports
Files:
packages/cli-v3/src/commands/deploy.tsapps/webapp/app/routes/api.v1.artifacts.tsapps/webapp/app/v3/services/deployment.server.ts
**/*.{js,ts,jsx,tsx,json,md,css,scss}
📄 CodeRabbit inference engine (AGENTS.md)
Format code using Prettier
Files:
packages/cli-v3/src/commands/deploy.tsapps/webapp/app/routes/api.v1.artifacts.tsapps/webapp/app/v3/services/deployment.server.ts
{packages/core,apps/webapp}/**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use zod for validation in packages/core and apps/webapp
Files:
apps/webapp/app/routes/api.v1.artifacts.tsapps/webapp/app/v3/services/deployment.server.ts
apps/webapp/app/**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/webapp.mdc)
Access all environment variables through the
envexport ofenv.server.tsinstead of directly accessingprocess.envin the Trigger.dev webapp
Files:
apps/webapp/app/routes/api.v1.artifacts.tsapps/webapp/app/v3/services/deployment.server.ts
apps/webapp/**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/webapp.mdc)
apps/webapp/**/*.{ts,tsx}: When importing from@trigger.dev/corein the webapp, use subpath exports from the package.json instead of importing from the root path
Follow the Remix 2.1.0 and Express server conventions when updating the main trigger.dev webapp
Files:
apps/webapp/app/routes/api.v1.artifacts.tsapps/webapp/app/v3/services/deployment.server.ts
apps/webapp/app/v3/services/**/*.server.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/webapp.mdc)
Organize services in the webapp following the pattern
app/v3/services/*/*.server.ts
Files:
apps/webapp/app/v3/services/deployment.server.ts
🧬 Code graph analysis (2)
packages/cli-v3/src/commands/deploy.ts (2)
packages/cli-v3/src/utilities/cliOutput.ts (1)
chalkError(24-26)packages/cli-v3/src/cli/common.ts (1)
OutroCommandError(35-35)
apps/webapp/app/routes/api.v1.artifacts.ts (1)
packages/core/src/v3/apps/http.ts (1)
json(65-75)
⏰ 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). (21)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (4, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (6, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (4, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (8, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (7, 8)
- 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 (5, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (3, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (2, 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 (5, 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: typecheck / typecheck
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - npm)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - pnpm)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (2)
apps/webapp/app/routes/api.v1.artifacts.ts (1)
53-73: Contextual artifact size error handling looks solidThe size/limit calculations and deployment-context-specific message are clear, and the
body.data.type satisfies neverpattern gives good exhaustiveness guarantees without changing behavior beyond the improved copy.packages/cli-v3/src/commands/deploy.ts (1)
931-949: Consistent OutroCommandError usage in native build error pathsThe new handling for artifact creation and archive read failures is consistent with the rest of the native deploy flow: spinner is stopped with a specific message, the error is logged, and an
OutroCommandError("Deployment failed")is thrown, which should integrate cleanly with existing CLI control flow.
Improved a couple of error messages.
Also fixed an issue with the s2 token caching.