feat(streams): make v2 streams the default when using 4.1.0+ if they are supported #2677
Conversation
|
|
Warning Rate limit exceeded@ericallam has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 22 minutes and 52 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (11)
WalkthroughThis pull request introduces configurable realtime streams versioning throughout the application. A new environment variable Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
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/services/realtime/v1StreamsGlobal.server.ts (1)
63-77: Consider logging unexpected stream version values.The function correctly implements the version resolution logic: defaulting to the environment configuration when no version is specified, and returning "v2" only when explicitly requested and S2 infrastructure is available. However, if an unexpected value (e.g., "v3", "invalid") is passed, it silently falls back to "v1". While this provides forward compatibility, consider adding a debug log when an unexpected non-empty value is passed to help diagnose configuration issues.
Example:
export function determineRealtimeStreamsVersion(streamVersion?: string): "v1" | "v2" { if (!streamVersion) { return env.REALTIME_STREAMS_DEFAULT_VERSION; } if ( streamVersion === "v2" && env.REALTIME_STREAMS_S2_BASIN && env.REALTIME_STREAMS_S2_ACCESS_TOKEN ) { return "v2"; } + if (streamVersion !== "v1") { + logger.debug("Unexpected realtime streams version, falling back to v1", { + requestedVersion: streamVersion + }); + } + return "v1"; }apps/webapp/app/routes/api.v2.tasks.batch.ts (1)
70-79: Consider addingrealtimeStreamsVersionto debug logging.The newly extracted
realtimeStreamsVersionheader is not included in the debug log statement, while other headers (includingbatchProcessingStrategyandrequestIdempotencyKey) are logged. For consistency and better observability, consider adding it.Apply this diff:
logger.debug("Batch trigger request", { triggerVersion, spanParentAsLink, isFromWorker, triggerClient, traceparent, tracestate, batchProcessingStrategy, requestIdempotencyKey, + realtimeStreamsVersion, });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
references/realtime-streams/src/app/actions.tsis excluded by!references/**
📒 Files selected for processing (10)
apps/webapp/app/env.server.ts(1 hunks)apps/webapp/app/routes/api.v1.tasks.$taskId.batch.ts(3 hunks)apps/webapp/app/routes/api.v1.tasks.$taskId.trigger.ts(2 hunks)apps/webapp/app/routes/api.v1.tasks.batch.ts(3 hunks)apps/webapp/app/routes/api.v2.tasks.batch.ts(3 hunks)apps/webapp/app/runEngine/services/batchTrigger.server.ts(2 hunks)apps/webapp/app/services/realtime/v1StreamsGlobal.server.ts(1 hunks)apps/webapp/app/v3/services/batchTriggerV3.server.ts(2 hunks)apps/webapp/app/v3/services/triggerTask.server.ts(1 hunks)packages/core/src/v3/apiClient/index.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-08-14T10:09:02.528Z
Learnt from: nicktrn
Repo: triggerdotdev/trigger.dev PR: 2390
File: internal-packages/run-engine/src/engine/index.ts:466-467
Timestamp: 2025-08-14T10:09:02.528Z
Learning: In the triggerdotdev/trigger.dev codebase, it's acceptable to pass `string | undefined` types directly to Prisma operations (both create and update). The codebase consistently uses this pattern and the team is comfortable with how Prisma handles undefined values.
Applied to files:
apps/webapp/app/v3/services/triggerTask.server.ts
📚 Learning: 2025-08-14T18:35:44.370Z
Learnt from: nicktrn
Repo: triggerdotdev/trigger.dev PR: 2390
File: apps/webapp/app/env.server.ts:764-765
Timestamp: 2025-08-14T18:35:44.370Z
Learning: The BoolEnv helper in apps/webapp/app/utils/boolEnv.ts uses z.preprocess with inconsistent default value types across the codebase - some usages pass boolean defaults (correct) while others pass string defaults (incorrect), leading to type confusion. The helper should enforce boolean-only defaults or have clearer documentation.
Applied to files:
apps/webapp/app/env.server.ts
🧬 Code graph analysis (8)
apps/webapp/app/v3/services/batchTriggerV3.server.ts (1)
apps/webapp/app/runEngine/services/batchTrigger.server.ts (1)
options(342-348)
apps/webapp/app/services/realtime/v1StreamsGlobal.server.ts (1)
apps/webapp/app/env.server.ts (1)
env(1246-1246)
apps/webapp/app/routes/api.v2.tasks.batch.ts (1)
apps/webapp/app/services/realtime/v1StreamsGlobal.server.ts (1)
determineRealtimeStreamsVersion(63-77)
packages/core/src/v3/apiClient/index.ts (1)
packages/core/src/v3/utils/getEnv.ts (1)
getEnvVar(11-13)
apps/webapp/app/routes/api.v1.tasks.$taskId.trigger.ts (1)
apps/webapp/app/services/realtime/v1StreamsGlobal.server.ts (1)
determineRealtimeStreamsVersion(63-77)
apps/webapp/app/runEngine/services/batchTrigger.server.ts (1)
apps/webapp/app/v3/services/batchTriggerV3.server.ts (1)
options(896-902)
apps/webapp/app/routes/api.v1.tasks.batch.ts (1)
apps/webapp/app/services/realtime/v1StreamsGlobal.server.ts (1)
determineRealtimeStreamsVersion(63-77)
apps/webapp/app/routes/api.v1.tasks.$taskId.batch.ts (1)
apps/webapp/app/services/realtime/v1StreamsGlobal.server.ts (1)
determineRealtimeStreamsVersion(63-77)
⏰ 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 (1, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (4, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (7, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (7, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (2, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (6, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (6, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (8, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (2, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (5, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (3, 8)
- 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 (1, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (8, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (4, 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 - pnpm)
- GitHub Check: e2e / 🧪 CLI v3 tests (ubuntu-latest - npm)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - pnpm)
- GitHub Check: typecheck / typecheck
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (12)
apps/webapp/app/v3/services/triggerTask.server.ts (1)
36-36: LGTM: Type restriction improves safety.Restricting
realtimeStreamsVersionfromstringto"v1" | "v2"provides better type safety and ensures only valid versions are accepted. All call sites properly usedetermineRealtimeStreamsVersion()which returns this union type.apps/webapp/app/env.server.ts (1)
1239-1239: LGTM: Safe default for realtime streams version.The new environment variable
REALTIME_STREAMS_DEFAULT_VERSIONwith enum["v1", "v2"]and default"v1"provides a sensible fallback. This ensures existing deployments without S2 configuration will continue using v1 streams.apps/webapp/app/routes/api.v1.tasks.batch.ts (1)
112-114: LGTM: Consistent header normalization.The route correctly extracts the
x-trigger-realtime-streams-versionheader and normalizes it throughdetermineRealtimeStreamsVersion()before passing to the service. This ensures proper version resolution based on both client preference and server configuration.apps/webapp/app/routes/api.v1.tasks.$taskId.trigger.ts (1)
130-132: LGTM: Proper version normalization.The implementation correctly normalizes the realtime streams version through
determineRealtimeStreamsVersion()before passing toTriggerTaskService, ensuring consistent behavior across the application.apps/webapp/app/v3/services/batchTriggerV3.server.ts (1)
59-59: LGTM: Proper option propagation.The new
realtimeStreamsVersion?: "v1" | "v2"option is correctly added toBatchTriggerTaskServiceOptionsand properly forwarded toTriggerTaskService.call()at line 855, maintaining consistency across the batch triggering flow.apps/webapp/app/routes/api.v1.tasks.$taskId.batch.ts (1)
105-105: LGTM: Consistent implementation.The route follows the established pattern of normalizing the realtime streams version through
determineRealtimeStreamsVersion()before passing to the service, ensuring proper fallback behavior.packages/core/src/v3/apiClient/index.ts (1)
1216-1228: Consolidate to single environment variable or document backwards compatibility requirement.The client-defaulting-to-v2 while server-defaults-to-v1 pattern is intentional and correct—the server's
determineRealtimeStreamsVersion()validates S2 availability and falls back to v1 gracefully. However, the dual environment variable check needs clarification.Both
TRIGGER_V2_REALTIME_STREAMSandTRIGGER_REALTIME_STREAMS_V2appear only in this single location (lines 1220–1223) and nowhere else in the codebase. They're not defined inenv.server.ts, not documented, and not checked by the server (which usesREALTIME_STREAMS_S2_BASINandREALTIME_STREAMS_S2_ACCESS_TOKENinstead). The inconsistent naming suggests either intentional backwards compatibility with two naming schemes or incomplete refactoring. If backwards compatibility is needed, document it; otherwise, consolidate to one environment variable name.apps/webapp/app/runEngine/services/batchTrigger.server.ts (2)
50-50: LGTM!The addition of the
realtimeStreamsVersionoptional field toBatchTriggerTaskServiceOptionsis correctly typed and appropriately optional. This aligns with the PR objective to support configurable realtime streams versioning.
712-712: LGTM!The
realtimeStreamsVersionis correctly propagated from the batch options to individual task triggers. The optional chaining ensures safe handling when the option is not provided.apps/webapp/app/routes/api.v2.tasks.batch.ts (3)
21-21: LGTM!The import of
determineRealtimeStreamsVersionis correct and the function is properly utilized below.
63-63: LGTM!The header extraction for
x-trigger-realtime-streams-versionfollows the established pattern and naming convention.
124-126: LGTM!The call to
determineRealtimeStreamsVersioncorrectly normalizes the streams version based on the header value, environment configuration, and infrastructure availability. The result is properly passed to the service.
ericallam
force-pushed
the
ea-branch-99
branch
from
a9ba909 to
8fdba72
Compare
No description provided.