Skip to content

Pr 3220 fix#3339

Closed
deepshekhardas wants to merge 12 commits intotriggerdotdev:mainfrom
deepshekhardas:pr-3220-fix
Closed

Pr 3220 fix#3339
deepshekhardas wants to merge 12 commits intotriggerdotdev:mainfrom
deepshekhardas:pr-3220-fix

Conversation

@deepshekhardas
Copy link
Copy Markdown

Closes #

✅ Checklist

  • I have followed every step in the contributing guide
  • The PR title follows the convention.
  • I ran and tested the code works

Testing

[Describe the steps you took to test this change]


Changelog

[Short description of what has changed]


Screenshots

[Screenshots]

💯

@changeset-bot
Copy link
Copy Markdown

changeset-bot bot commented Apr 8, 2026

🦋 Changeset detected

Latest commit: 587deb4

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 29 packages
Name Type
@trigger.dev/core Minor
@trigger.dev/build Minor
trigger.dev Minor
@trigger.dev/python Minor
@trigger.dev/redis-worker Minor
@trigger.dev/schema-to-json Minor
@trigger.dev/sdk Minor
@internal/cache Patch
@internal/clickhouse Patch
@internal/llm-pricing Patch
@internal/redis Patch
@internal/replication Patch
@internal/run-engine Patch
@internal/schedule-engine Patch
@internal/testcontainers Patch
@internal/tracing Patch
@internal/tsql Patch
@internal/zod-worker Patch
d3-chat Patch
references-d3-openai-agents Patch
references-nextjs-realtime Patch
references-realtime-hooks-test Patch
references-realtime-streams Patch
references-telemetry Patch
@internal/sdk-compat-tests Patch
@trigger.dev/react-hooks Minor
@trigger.dev/rsc Minor
@trigger.dev/database Minor
@trigger.dev/otlp-importer Minor

Not sure what this means? Click here to learn what changesets are.

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

@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Apr 8, 2026

Hi @deepshekhardas, thanks for your interest in contributing!

This project requires that pull request authors are vouched, and you are not in the list of vouched users.

This PR will be closed automatically. See https://github.com/triggerdotdev/trigger.dev/blob/main/CONTRIBUTING.md for more details.

@github-actions github-actions bot closed this Apr 8, 2026
@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai bot commented Apr 8, 2026

Caution

Review failed

The pull request is closed.

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: c2cbafac-14ea-4cd9-a6c2-5185cddbdc06

📥 Commits

Reviewing files that changed from the base of the PR and between def21b2 and 587deb4.

📒 Files selected for processing (8)
  • .changeset/define-runevent-schema.md
  • .changeset/runevent-schema-apiclient.md
  • packages/cli-v3/src/commands/deploy.ts
  • packages/cli-v3/tsc_output.txt
  • packages/core/src/v3/apiClient/index.ts
  • packages/core/src/v3/realtimeStreams/streamsWriterV2.ts
  • packages/core/src/v3/schemas/api-type.test.ts
  • packages/core/src/v3/schemas/api.ts

Walkthrough

This pull request introduces a new RunEvent schema for the @trigger.dev/core package. It defines RunEvent as a Zod schema that validates event objects with flexible timestamp parsing (supporting string, number, bigint, and Date formats), creates response schemas for listing run events, and updates ApiClient.listRunEvents to parse responses using typed schemas instead of untyped responses. The change includes comprehensive test coverage for the new schemas and their transformations, along with formatting adjustments to template literals in the deploy command and realtime streams utilities.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
⚔️ Resolve merge conflicts
  • Resolve merge conflict in branch pr-3220-fix

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
Copy Markdown
Contributor

@devin-ai-integration devin-ai-integration bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Devin Review found 2 potential issues.

View 3 additional findings in Devin Review.

Open in Devin Review

Comment on lines +1152 to 1155
start: {
from: { seqNum: 0 },
},
},
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 Removal of S2 read session timeout allows CLI to hang indefinitely

The old code had stop: { waitSecs: 60 * 20 } which provided a 20-minute safety timeout for the S2 read session. This PR removes it entirely. The for await (const record of readSession) loop at packages/cli-v3/src/commands/deploy.ts:1175 will now block indefinitely if no finalized event is ever emitted (e.g., build server crashes, stream infrastructure issues, or any scenario where the deployment process hangs without producing a terminal event). The abortController is only triggered upon receiving a finalized event (packages/cli-v3/src/commands/deploy.ts:1227), so without the server-side timeout, there is no fallback to break the loop.

Suggested change
start: {
from: { seqNum: 0 },
},
},
start: {
from: { seqNum: 0 },
},
stop: { waitSecs: 60 * 20 }, // 20 minutes
Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

listRunEvents(runId: string, requestOptions?: ZodFetchOptions) {
return zodfetch(
z.any(), // TODO: define a proper schema for this
ListRunEventsResponse,
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🚩 Schema tightening from z.any() to ListRunEventsResponse may strip fields from API response

The change from z.any() to ListRunEventsResponse at packages/core/src/v3/apiClient/index.ts:707 introduces strict Zod parsing. The API route at apps/webapp/app/routes/api.v1.runs.$runId.events.ts:48-54 returns all fields from RunPreparedEvent which includes idempotencyKey and environmentType (selected at apps/webapp/app/v3/eventRepository/eventRepository.server.ts:704,715). These fields are NOT in the new RunEvent schema and will be silently stripped by Zod's default behavior. While this is likely intentional (moving from untyped to typed), any existing consumers of listRunEvents() that relied on idempotencyKey or environmentType will lose access to those fields. This is a semantic API contract change worth noting in the changeset.

Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

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.

2 participants