Skip to content

Conversation

@ericallam
Copy link
Member

No description provided.

@changeset-bot
Copy link

changeset-bot bot commented Nov 12, 2025

⚠️ No Changeset found

Latest commit: 9b27d79

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Nov 12, 2025

Walkthrough

This PR introduces token expiration configuration and a caching layer for S2 realtime streams. A new environment variable (REALTIME_STREAMS_S2_ACCESS_TOKEN_EXPIRATION_IN_MS) controls access token lifespan with a 1-day default. The S2RealtimeStreams service is enhanced to support optional token caching via a Redis-backed cache store, eliminating repeated token issuance. Stream naming is refactored, response headers now include the stream name, and data payload serialization switches to JSON format.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

  • Cache initialization and singleton pattern in v1StreamsGlobal.server.ts: Verify the RedisCacheStore configuration (TLS, host, port) and namespace wrapping logic are correctly applied
  • Token expiration flow: Trace the configuration from environment variable through constructor options to token issuance in s2realtimeStreams.server.ts to ensure consistency across the chain
  • Stream naming refactor: Confirm the new toStreamName implementation correctly constructs paths and that all call sites have been updated accordingly
  • JSON serialization change in redisRealtimeStreams.server.ts: Validate that downstream consumers expect JSON-formatted payloads with trailing newlines
  • Header extraction logic in streamInstance.ts: Ensure the fallback behavior (preferring streamName but falling back to original key) handles all edge cases

Pre-merge checks and finishing touches

❌ Failed checks (2 warnings)
Check name Status Explanation Resolution
Description check ⚠️ Warning The pull request description is entirely missing; it contains no content, checklist items, testing information, or changelog details as required by the template. Add a complete pull request description following the repository template, including issue reference, checklist completion, testing steps, and changelog summary.
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (1 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately describes the main changes: scoping S2 access tokens to environment and fixing streams v1 appends, matching the key modifications across multiple files.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch ea-branch-96

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

Comment @coderabbitai help to get the list of available commands and usage tips.

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: 1

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 668559e and 076904c.

📒 Files selected for processing (5)
  • apps/webapp/app/env.server.ts (1 hunks)
  • apps/webapp/app/services/realtime/redisRealtimeStreams.server.ts (1 hunks)
  • apps/webapp/app/services/realtime/s2realtimeStreams.server.ts (7 hunks)
  • apps/webapp/app/services/realtime/v1StreamsGlobal.server.ts (2 hunks)
  • packages/core/src/v3/realtimeStreams/streamInstance.ts (3 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:

  • packages/core/src/v3/realtimeStreams/streamInstance.ts
  • apps/webapp/app/env.server.ts
  • apps/webapp/app/services/realtime/redisRealtimeStreams.server.ts
  • apps/webapp/app/services/realtime/v1StreamsGlobal.server.ts
  • apps/webapp/app/services/realtime/s2realtimeStreams.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:

  • packages/core/src/v3/realtimeStreams/streamInstance.ts
  • apps/webapp/app/env.server.ts
  • apps/webapp/app/services/realtime/redisRealtimeStreams.server.ts
  • apps/webapp/app/services/realtime/v1StreamsGlobal.server.ts
  • apps/webapp/app/services/realtime/s2realtimeStreams.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/env.server.ts
  • apps/webapp/app/services/realtime/redisRealtimeStreams.server.ts
  • apps/webapp/app/services/realtime/v1StreamsGlobal.server.ts
  • apps/webapp/app/services/realtime/s2realtimeStreams.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/env.server.ts
  • apps/webapp/app/services/realtime/redisRealtimeStreams.server.ts
  • apps/webapp/app/services/realtime/v1StreamsGlobal.server.ts
  • apps/webapp/app/services/realtime/s2realtimeStreams.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/env.server.ts
  • apps/webapp/app/services/realtime/redisRealtimeStreams.server.ts
  • apps/webapp/app/services/realtime/v1StreamsGlobal.server.ts
  • apps/webapp/app/services/realtime/s2realtimeStreams.server.ts
🧠 Learnings (4)
📚 Learning: 2025-08-29T10:06:49.293Z
Learnt from: CR
Repo: triggerdotdev/trigger.dev PR: 0
File: .cursor/rules/webapp.mdc:0-0
Timestamp: 2025-08-29T10:06:49.293Z
Learning: Applies to {apps/webapp/app/**/*.server.{ts,tsx},apps/webapp/app/routes/**/*.ts} : Access environment variables only via the env export from app/env.server.ts; do not reference process.env directly

Applied to files:

  • apps/webapp/app/env.server.ts
📚 Learning: 2025-11-10T09:09:07.392Z
Learnt from: myftija
Repo: triggerdotdev/trigger.dev PR: 2663
File: apps/webapp/app/env.server.ts:1205-1206
Timestamp: 2025-11-10T09:09:07.392Z
Learning: In the trigger.dev webapp, S2_ACCESS_TOKEN and S2_DEPLOYMENT_LOGS_BASIN_NAME environment variables must remain optional until an OSS version of S2 is available, to avoid breaking environments that don't have S2 provisioned.

Applied to files:

  • apps/webapp/app/env.server.ts
📚 Learning: 2025-08-29T10:06:49.293Z
Learnt from: CR
Repo: triggerdotdev/trigger.dev PR: 0
File: .cursor/rules/webapp.mdc:0-0
Timestamp: 2025-08-29T10:06:49.293Z
Learning: Applies to apps/webapp/app/**/*.ts : Modules intended for test consumption under apps/webapp/app/**/*.ts must not read environment variables; accept configuration via options instead

Applied to files:

  • apps/webapp/app/env.server.ts
📚 Learning: 2025-08-29T10:06:49.293Z
Learnt from: CR
Repo: triggerdotdev/trigger.dev PR: 0
File: .cursor/rules/webapp.mdc:0-0
Timestamp: 2025-08-29T10:06:49.293Z
Learning: Maintain service/configuration separation (e.g., create testable service modules and separate configuration modules like realtimeClient.server.ts vs realtimeClientGlobal.server.ts)

Applied to files:

  • apps/webapp/app/services/realtime/v1StreamsGlobal.server.ts
🧬 Code graph analysis (1)
apps/webapp/app/services/realtime/v1StreamsGlobal.server.ts (2)
apps/webapp/app/env.server.ts (1)
  • env (1245-1245)
internal-packages/cache/src/index.ts (3)
  • DefaultStatefulContext (3-3)
  • createCache (2-2)
  • Namespace (4-4)
⏰ 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). (22)
  • GitHub Check: units / internal / 🧪 Unit Tests: Internal (7, 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 (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 / webapp / 🧪 Unit Tests: Webapp (7, 8)
  • GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (8, 8)
  • GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (6, 8)
  • GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (1, 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 (4, 8)
  • GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (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 (windows-latest - pnpm)
  • GitHub Check: e2e / 🧪 CLI v3 tests (ubuntu-latest - pnpm)
  • GitHub Check: e2e / 🧪 CLI v3 tests (ubuntu-latest - npm)
  • GitHub Check: typecheck / typecheck
  • GitHub Check: Analyze (javascript-typescript)

@ericallam ericallam merged commit f7cb637 into main Nov 12, 2025
25 of 26 checks passed
@ericallam ericallam deleted the ea-branch-96 branch November 12, 2025 12:34
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.

3 participants