fix(engine): default to paid placement on billing errors #2604
Conversation
|
Walkthrough
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used📓 Path-based instructions (5)**/*.{ts,tsx}📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
{packages/core,apps/webapp}/**/*.{ts,tsx}📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
apps/webapp/**/*.{ts,tsx}📄 CodeRabbit inference engine (.cursor/rules/webapp.mdc)
Files:
{apps/webapp/app/**/*.server.{ts,tsx},apps/webapp/app/routes/**/*.ts}📄 CodeRabbit inference engine (.cursor/rules/webapp.mdc)
Files:
apps/webapp/app/**/*.ts📄 CodeRabbit inference engine (.cursor/rules/webapp.mdc)
Files:
🧬 Code graph analysis (1)apps/webapp/app/v3/runEngine.server.ts (1)
⏰ 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)
🔇 Additional comments (5)
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: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/webapp/app/services/platform.v3.server.ts(8 hunks)apps/webapp/app/v3/runEngine.server.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/v3/runEngine.server.tsapps/webapp/app/services/platform.v3.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/runEngine.server.tsapps/webapp/app/services/platform.v3.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/runEngine.server.tsapps/webapp/app/services/platform.v3.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/runEngine.server.tsapps/webapp/app/services/platform.v3.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/runEngine.server.tsapps/webapp/app/services/platform.v3.server.ts
🧬 Code graph analysis (1)
apps/webapp/app/v3/runEngine.server.ts (2)
packages/trigger-sdk/src/v3/index.ts (1)
logger(40-40)apps/webapp/app/v3/tracer.server.ts (1)
logger(106-111)
⏰ 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 / webapp / 🧪 Unit Tests: Webapp (8, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (5, 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 (7, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (7, 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 (3, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (3, 8)
- GitHub Check: units / packages / 🧪 Unit Tests: Packages (1, 1)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (1, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (5, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (6, 8)
- 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 (2, 8)
- 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 (5)
apps/webapp/app/v3/runEngine.server.ts (4)
8-8: LGTM!The logger import is appropriately sourced from the service layer and enables the enhanced logging throughout this file.
124-131: Verify the business implications of defaulting to paid on billing errors.This change intentionally defaults to paid placement when the billing service is unavailable or returns no plan data. While this prevents service degradation for paying customers during billing outages, it could also allow free-tier users to access paid resources if billing checks fail.
Confirm this trade-off aligns with your risk tolerance and business requirements. You may also want to consider:
- Monitoring the frequency of
"engine.getCurrentPlan: no plan"warnings to detect billing service issues early- Implementing circuit breakers or alerts when this fallback is triggered repeatedly
133-140: LGTM!The guard and logging appropriately handle the edge case where subscription data is missing.
151-155: LGTM!Removing the optional chaining is safe because the guard at lines 142-149 ensures
plan.v3Subscription.planexists before reaching this point. The added comment clarifies the expected code path.apps/webapp/app/services/platform.v3.server.ts (1)
208-208: LGTM!The enhanced error logging consistently distinguishes between "no success" responses (API returned an error response) and "caught error" scenarios (exception thrown during execution). This improves observability and debugging by making failure modes explicit.
Also applies to: 224-224, 235-235, 241-241, 282-282, 287-287, 365-365, 370-370, 399-399, 404-404, 423-423, 428-428, 451-451, 458-458
Also improves logs to distinguish between billing request failure modes