fix(replication): allow disabling of task run payload inserts via env var #2626
Conversation
|
WalkthroughThis pull request introduces a new environment-based configuration flag to optionally disable payload insertion in the runs replication service. The change adds the Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes The changes follow a consistent, additive pattern across three files: environment configuration → instance initialization → service logic. The modifications are non-breaking, introduce a straightforward boolean flag mechanism, and add a simple conditional check in the insertion logic. The homogeneous nature of these changes (environment variable propagation through layers) and low logic density reduce review complexity. Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ 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 (2)
apps/webapp/app/env.server.ts (1)
1105-1105: Consider using BoolEnv for type-safety (optional).While the
z.string().default("0")pattern is consistent with otherRUN_REPLICATION_*flags in this section, usingBoolEnv.default(false)would provide better type safety and accept more boolean-like values ("true","false","1","0", etc.). However, maintaining local consistency with nearby flags is also reasonable.If you prefer to use BoolEnv for consistency with other boolean flags in the codebase:
- RUN_REPLICATION_DISABLE_PAYLOAD_INSERT: z.string().default("0"), + RUN_REPLICATION_DISABLE_PAYLOAD_INSERT: BoolEnv.default(false),If this change is made, also update line 69 in
apps/webapp/app/services/runsReplicationInstance.server.ts:disablePayloadInsert: env.RUN_REPLICATION_DISABLE_PAYLOAD_INSERT,apps/webapp/app/services/runsReplicationService.server.ts (1)
756-769: Logic is correct; consider restructuring for clarity (optional).The implementation correctly disables payload inserts for all events when the flag is true. The condition works but groups the feature flag with event-type checks, which slightly obscures intent.
Consider restructuring for improved readability:
- if (event === "update" || event === "delete" || this._disablePayloadInsert) { + // Skip payload inserts for updates/deletes, or if disabled via config + if (this._disablePayloadInsert || event === "update" || event === "delete") {Placing
this._disablePayloadInsertfirst makes it clear that it's a global override that affects all events, while update/delete are event-specific conditions.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
apps/webapp/app/env.server.ts(1 hunks)apps/webapp/app/services/runsReplicationInstance.server.ts(1 hunks)apps/webapp/app/services/runsReplicationService.server.ts(4 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/services/runsReplicationInstance.server.tsapps/webapp/app/services/runsReplicationService.server.tsapps/webapp/app/env.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/services/runsReplicationInstance.server.tsapps/webapp/app/services/runsReplicationService.server.tsapps/webapp/app/env.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/services/runsReplicationInstance.server.tsapps/webapp/app/services/runsReplicationService.server.tsapps/webapp/app/env.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/services/runsReplicationInstance.server.tsapps/webapp/app/services/runsReplicationService.server.tsapps/webapp/app/env.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/services/runsReplicationInstance.server.tsapps/webapp/app/services/runsReplicationService.server.tsapps/webapp/app/env.server.ts
⏰ 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 (2, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (8, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (6, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (7, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (3, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (8, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (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 (1, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (7, 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 (1, 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 - pnpm)
- GitHub Check: e2e / 🧪 CLI v3 tests (ubuntu-latest - npm)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - npm)
- GitHub Check: e2e / 🧪 CLI v3 tests (ubuntu-latest - pnpm)
- GitHub Check: typecheck / typecheck
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (3)
apps/webapp/app/services/runsReplicationInstance.server.ts (1)
69-69: LGTM!The boolean conversion and option wiring are correct and consistent with the codebase patterns.
apps/webapp/app/services/runsReplicationService.server.ts (2)
60-60: LGTM!The optional field addition to the public options interface is correct and appropriately typed.
104-104: LGTM!The private field initialization with a default of
falsecorrectly preserves existing behavior when the option is not provided.Also applies to: 117-117
No description provided.