-
-
Notifications
You must be signed in to change notification settings - Fork 764
fix: regression affecting replayed runs with superjson payloads #2266
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
WalkthroughThe changes introduce a new function, ✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/core/src/v3/utils/ioSerialization.ts (1)
527-530
: Consider adding error handling for invalid SuperJSONWhile the function handles invalid JSON in the
newPayload
parameter, it doesn't explicitly handle the case where theoriginal
parameter contains invalid SuperJSON.export async function replaceSuperJsonPayload(original: string, newPayload: string) { const superjson = await loadSuperJSON(); - const originalObject = superjson.parse(original); + let originalObject; + try { + originalObject = superjson.parse(original); + } catch (error) { + throw new Error(`Invalid SuperJSON in original string: ${error instanceof Error ? error.message : 'Unknown error'}`); + } const newPayloadObject = JSON.parse(newPayload);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yaml
is excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (5)
apps/webapp/app/v3/replayTask.ts
(1 hunks)apps/webapp/app/v3/services/replayTaskRun.server.ts
(4 hunks)packages/core/package.json
(2 hunks)packages/core/src/v3/utils/ioSerialization.ts
(2 hunks)packages/core/test/ioSerialization.test.ts
(1 hunks)
🧰 Additional context used
📓 Path-based instructions (5)
apps/webapp/**/*.ts
Instructions used from:
Sources:
📄 CodeRabbit Inference Engine
- .cursor/rules/webapp.mdc
apps/webapp/**/*.{ts,tsx}
Instructions used from:
Sources:
📄 CodeRabbit Inference Engine
- .cursor/rules/webapp.mdc
**/*.{ts,tsx}
Instructions used from:
Sources:
📄 CodeRabbit Inference Engine
- .github/copilot-instructions.md
{packages/core,apps/webapp}/**/*.{ts,tsx}
Instructions used from:
Sources:
📄 CodeRabbit Inference Engine
- .github/copilot-instructions.md
**/*.test.{ts,tsx}
Instructions used from:
Sources:
📄 CodeRabbit Inference Engine
- .github/copilot-instructions.md
🧠 Learnings (6)
📓 Common learnings
Learnt from: CR
PR: triggerdotdev/trigger.dev#0
File: .cursor/rules/writing-tasks.mdc:0-0
Timestamp: 2025-06-30T13:22:21.528Z
Learning: Applies to **/trigger/**/*.{ts,tsx,js,jsx} : Use the `metadata` API from `@trigger.dev/sdk/v3` to attach, update, and access structured metadata within task runs.
Learnt from: CR
PR: triggerdotdev/trigger.dev#0
File: .cursor/rules/writing-tasks.mdc:0-0
Timestamp: 2025-06-30T13:22:21.528Z
Learning: Applies to **/trigger/**/*.{ts,tsx,js,jsx} : Schema tasks must use `schemaTask` from `@trigger.dev/sdk/v3` and validate payloads using a schema (e.g., Zod).
apps/webapp/app/v3/replayTask.ts (7)
Learnt from: CR
PR: triggerdotdev/trigger.dev#0
File: .cursor/rules/writing-tasks.mdc:0-0
Timestamp: 2025-06-30T13:22:21.528Z
Learning: Applies to **/trigger/**/*.{ts,tsx,js,jsx} : Schema tasks must use `schemaTask` from `@trigger.dev/sdk/v3` and validate payloads using a schema (e.g., Zod).
Learnt from: CR
PR: triggerdotdev/trigger.dev#0
File: .cursor/rules/writing-tasks.mdc:0-0
Timestamp: 2025-06-30T13:22:21.528Z
Learning: Applies to **/trigger/**/*.{ts,tsx,js,jsx} : If you are able to generate an example payload for a task, do so.
Learnt from: CR
PR: triggerdotdev/trigger.dev#0
File: .cursor/rules/writing-tasks.mdc:0-0
Timestamp: 2025-06-30T13:22:21.528Z
Learning: Applies to **/trigger/**/*.{ts,tsx,js,jsx} : Use the `metadata` API from `@trigger.dev/sdk/v3` to attach, update, and access structured metadata within task runs.
Learnt from: CR
PR: triggerdotdev/trigger.dev#0
File: .cursor/rules/writing-tasks.mdc:0-0
Timestamp: 2025-06-30T13:22:21.528Z
Learning: Applies to **/trigger/**/*.{ts,tsx,js,jsx} : ALWAYS generate Trigger.dev tasks using the `task` function from `@trigger.dev/sdk/v3` and NEVER use the deprecated `client.defineJob` pattern.
Learnt from: CR
PR: triggerdotdev/trigger.dev#0
File: .cursor/rules/writing-tasks.mdc:0-0
Timestamp: 2025-06-30T13:22:21.528Z
Learning: Applies to **/trigger/**/*.{ts,tsx,js,jsx} : When using retry, queue, machine, or maxDuration options, configure them within the `task` function in Trigger.dev task files.
Learnt from: CR
PR: triggerdotdev/trigger.dev#0
File: .cursor/rules/writing-tasks.mdc:0-0
Timestamp: 2025-06-30T13:22:21.528Z
Learning: Applies to **/trigger/**/*.{ts,tsx,js,jsx} : Scheduled tasks must use `schedules.task` from `@trigger.dev/sdk/v3` and define a `cron` property for declarative schedules.
Learnt from: CR
PR: triggerdotdev/trigger.dev#0
File: .cursor/rules/writing-tasks.mdc:0-0
Timestamp: 2025-06-30T13:22:21.528Z
Learning: Applies to **/trigger/**/*.{ts,tsx,js,jsx} : When implementing idempotency, use the `idempotencyKeys` API from `@trigger.dev/sdk/v3` and provide an `idempotencyKey` when triggering tasks.
packages/core/package.json (13)
Learnt from: CR
PR: triggerdotdev/trigger.dev#0
File: .cursor/rules/writing-tasks.mdc:0-0
Timestamp: 2025-06-30T13:22:21.528Z
Learning: Applies to **/trigger/**/*.{ts,tsx,js,jsx} : Use the `runs` and `tasks` APIs from `@trigger.dev/sdk/v3` to subscribe to run updates and implement realtime features.
Learnt from: CR
PR: triggerdotdev/trigger.dev#0
File: .cursor/rules/webapp.mdc:0-0
Timestamp: 2025-06-30T13:21:59.438Z
Learning: Applies to apps/webapp/**/*.{ts,tsx} : When importing from `@trigger.dev/core` in the webapp, never import from the root `@trigger.dev/core` path; always use one of the subpath exports as defined in the package's package.json.
Learnt from: CR
PR: triggerdotdev/trigger.dev#0
File: .cursor/rules/writing-tasks.mdc:0-0
Timestamp: 2025-06-30T13:22:21.528Z
Learning: Applies to **/trigger/**/*.{ts,tsx,js,jsx} : Use the `metadata` API from `@trigger.dev/sdk/v3` to attach, update, and access structured metadata within task runs.
Learnt from: CR
PR: triggerdotdev/trigger.dev#0
File: .cursor/rules/writing-tasks.mdc:0-0
Timestamp: 2025-06-30T13:22:21.528Z
Learning: Applies to **/trigger/**/*.{ts,tsx,js,jsx} : ALWAYS generate Trigger.dev tasks using the `task` function from `@trigger.dev/sdk/v3` and NEVER use the deprecated `client.defineJob` pattern.
Learnt from: CR
PR: triggerdotdev/trigger.dev#0
File: .cursor/rules/writing-tasks.mdc:0-0
Timestamp: 2025-06-30T13:22:21.528Z
Learning: Applies to **/trigger/**/*.{ts,tsx,js,jsx} : You MUST use `@trigger.dev/sdk/v3` when writing Trigger.dev tasks.
Learnt from: CR
PR: triggerdotdev/trigger.dev#0
File: .cursor/rules/writing-tasks.mdc:0-0
Timestamp: 2025-06-30T13:22:21.528Z
Learning: Applies to **/trigger/**/*.{ts,tsx,js,jsx} : Scheduled tasks must use `schedules.task` from `@trigger.dev/sdk/v3` and define a `cron` property for declarative schedules.
Learnt from: CR
PR: triggerdotdev/trigger.dev#0
File: .cursor/rules/writing-tasks.mdc:0-0
Timestamp: 2025-06-30T13:22:21.528Z
Learning: Applies to trigger.config.ts : The `trigger.config.ts` file must use `defineConfig` from `@trigger.dev/sdk/v3` to configure project settings, directories, retries, telemetry, runtime, machine settings, log level, max duration, and build options.
Learnt from: CR
PR: triggerdotdev/trigger.dev#0
File: .cursor/rules/writing-tasks.mdc:0-0
Timestamp: 2025-06-30T13:22:21.528Z
Learning: Applies to **/trigger/**/*.{ts,tsx,js,jsx} : Each Trigger.dev task must have a unique `id` within your project.
Learnt from: CR
PR: triggerdotdev/trigger.dev#0
File: .cursor/rules/writing-tasks.mdc:0-0
Timestamp: 2025-06-30T13:22:21.528Z
Learning: Applies to trigger.config.ts : Build extensions for Trigger.dev must be configured in the `build` property of `trigger.config.ts` using provided or custom extensions.
Learnt from: CR
PR: triggerdotdev/trigger.dev#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-06-30T13:21:33.994Z
Learning: Applies to {packages/core,apps/webapp}/**/*.{ts,tsx} : We use zod a lot in packages/core and in the webapp
Learnt from: CR
PR: triggerdotdev/trigger.dev#0
File: .cursor/rules/writing-tasks.mdc:0-0
Timestamp: 2025-06-30T13:22:21.528Z
Learning: Applies to **/trigger/**/*.{ts,tsx,js,jsx} : Schema tasks must use `schemaTask` from `@trigger.dev/sdk/v3` and validate payloads using a schema (e.g., Zod).
Learnt from: CR
PR: triggerdotdev/trigger.dev#0
File: .cursor/rules/writing-tasks.mdc:0-0
Timestamp: 2025-06-30T13:22:21.528Z
Learning: Applies to **/trigger/**/*.{ts,tsx,js,jsx} : You MUST NEVER use `client.defineJob` in Trigger.dev task files, as it is deprecated and will break the application.
Learnt from: CR
PR: triggerdotdev/trigger.dev#0
File: .cursor/rules/writing-tasks.mdc:0-0
Timestamp: 2025-06-30T13:22:21.528Z
Learning: Applies to **/trigger/**/*.{ts,tsx,js,jsx} : When using retry, queue, machine, or maxDuration options, configure them within the `task` function in Trigger.dev task files.
packages/core/test/ioSerialization.test.ts (2)
Learnt from: CR
PR: triggerdotdev/trigger.dev#0
File: .cursor/rules/webapp.mdc:0-0
Timestamp: 2025-06-30T13:21:59.438Z
Learning: Applies to apps/webapp/app/**/*.test.{ts,tsx} : Tests should only import classes and functions from files matching `app/**/*.ts` of the webapp, and those files should not use environment variables; everything should be passed through as options instead.
Learnt from: CR
PR: triggerdotdev/trigger.dev#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-06-30T13:21:33.994Z
Learning: Applies to **/*.test.{ts,tsx} : Our tests are all vitest
apps/webapp/app/v3/services/replayTaskRun.server.ts (14)
Learnt from: CR
PR: triggerdotdev/trigger.dev#0
File: .cursor/rules/writing-tasks.mdc:0-0
Timestamp: 2025-06-30T13:22:21.528Z
Learning: Applies to **/trigger/**/*.{ts,tsx,js,jsx} : Schema tasks must use `schemaTask` from `@trigger.dev/sdk/v3` and validate payloads using a schema (e.g., Zod).
Learnt from: CR
PR: triggerdotdev/trigger.dev#0
File: .cursor/rules/writing-tasks.mdc:0-0
Timestamp: 2025-06-30T13:22:21.528Z
Learning: Applies to **/trigger/**/*.{ts,tsx,js,jsx} : If you are able to generate an example payload for a task, do so.
Learnt from: CR
PR: triggerdotdev/trigger.dev#0
File: .cursor/rules/writing-tasks.mdc:0-0
Timestamp: 2025-06-30T13:22:21.528Z
Learning: Applies to **/trigger/**/*.{ts,tsx,js,jsx} : When using retry, queue, machine, or maxDuration options, configure them within the `task` function in Trigger.dev task files.
Learnt from: CR
PR: triggerdotdev/trigger.dev#0
File: .cursor/rules/writing-tasks.mdc:0-0
Timestamp: 2025-06-30T13:22:21.528Z
Learning: Applies to **/trigger/**/*.{ts,tsx,js,jsx} : Use the `metadata` API from `@trigger.dev/sdk/v3` to attach, update, and access structured metadata within task runs.
Learnt from: CR
PR: triggerdotdev/trigger.dev#0
File: .cursor/rules/writing-tasks.mdc:0-0
Timestamp: 2025-06-30T13:22:21.528Z
Learning: Applies to **/trigger/**/*.{ts,tsx,js,jsx} : When triggering a task from inside another task, use the task instance's `trigger`, `batchTrigger`, `triggerAndWait`, `batchTriggerAndWait`, `batch.trigger`, `batch.triggerAndWait`, `batch.triggerByTask`, or `batch.triggerByTaskAndWait` methods.
Learnt from: CR
PR: triggerdotdev/trigger.dev#0
File: .cursor/rules/writing-tasks.mdc:0-0
Timestamp: 2025-06-30T13:22:21.528Z
Learning: Applies to **/trigger/**/*.{ts,tsx,js,jsx} : ALWAYS generate Trigger.dev tasks using the `task` function from `@trigger.dev/sdk/v3` and NEVER use the deprecated `client.defineJob` pattern.
Learnt from: CR
PR: triggerdotdev/trigger.dev#0
File: .cursor/rules/writing-tasks.mdc:0-0
Timestamp: 2025-06-30T13:22:21.528Z
Learning: Applies to **/trigger/**/*.{ts,tsx,js,jsx} : Use the `runs` and `tasks` APIs from `@trigger.dev/sdk/v3` to subscribe to run updates and implement realtime features.
Learnt from: CR
PR: triggerdotdev/trigger.dev#0
File: .cursor/rules/writing-tasks.mdc:0-0
Timestamp: 2025-06-30T13:22:21.528Z
Learning: Applies to **/trigger/**/*.{ts,tsx,js,jsx} : When implementing idempotency, use the `idempotencyKeys` API from `@trigger.dev/sdk/v3` and provide an `idempotencyKey` when triggering tasks.
Learnt from: CR
PR: triggerdotdev/trigger.dev#0
File: .cursor/rules/writing-tasks.mdc:0-0
Timestamp: 2025-06-30T13:22:21.528Z
Learning: Applies to **/trigger/**/*.{ts,tsx,js,jsx} : Scheduled tasks must use `schedules.task` from `@trigger.dev/sdk/v3` and define a `cron` property for declarative schedules.
Learnt from: CR
PR: triggerdotdev/trigger.dev#0
File: .cursor/rules/writing-tasks.mdc:0-0
Timestamp: 2025-06-30T13:22:21.528Z
Learning: Applies to **/trigger/**/*.{ts,tsx,js,jsx} : The `run` function must contain the main logic for each Trigger.dev task.
Learnt from: CR
PR: triggerdotdev/trigger.dev#0
File: .cursor/rules/writing-tasks.mdc:0-0
Timestamp: 2025-06-30T13:22:21.528Z
Learning: Applies to **/trigger/**/*.{ts,tsx,js,jsx} : When triggering a task from backend code, use `tasks.trigger`, `tasks.batchTrigger`, or `tasks.triggerAndPoll` from `@trigger.dev/sdk/v3` and set the `TRIGGER_SECRET_KEY` environment variable.
Learnt from: CR
PR: triggerdotdev/trigger.dev#0
File: .cursor/rules/writing-tasks.mdc:0-0
Timestamp: 2025-06-30T13:22:21.528Z
Learning: Before generating any code for Trigger.dev tasks, verify that you are importing from `@trigger.dev/sdk/v3`, exporting every task, and not generating any deprecated code patterns.
Learnt from: CR
PR: triggerdotdev/trigger.dev#0
File: .cursor/rules/writing-tasks.mdc:0-0
Timestamp: 2025-06-30T13:22:21.528Z
Learning: Applies to **/trigger/**/*.{ts,tsx,js,jsx} : You MUST use `@trigger.dev/sdk/v3` when writing Trigger.dev tasks.
Learnt from: CR
PR: triggerdotdev/trigger.dev#0
File: .cursor/rules/writing-tasks.mdc:0-0
Timestamp: 2025-06-30T13:22:21.528Z
Learning: Applies to **/trigger/**/*.{ts,tsx,js,jsx} : You MUST `export` every task, including subtasks, in Trigger.dev task files.
packages/core/src/v3/utils/ioSerialization.ts (2)
Learnt from: CR
PR: triggerdotdev/trigger.dev#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-06-30T13:21:33.994Z
Learning: Applies to {packages/core,apps/webapp}/**/*.{ts,tsx} : We use zod a lot in packages/core and in the webapp
Learnt from: CR
PR: triggerdotdev/trigger.dev#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-06-30T13:21:33.994Z
Learning: Applies to **/*.{ts,tsx} : Always prefer using isomorphic code like fetch, ReadableStream, etc. instead of Node.js specific code
🧬 Code Graph Analysis (1)
packages/core/src/v3/utils/ioSerialization.ts (2)
packages/core/src/v3/utils/flattenAttributes.ts (1)
value
(207-230)packages/core/src/v3/apiClient/core.ts (1)
get
(389-392)
⏰ 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). (25)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (3, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (7, 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 (8, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (4, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (6, 10)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (1, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (10, 10)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (8, 10)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (2, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (3, 10)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (7, 10)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (4, 10)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (2, 10)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (9, 10)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (5, 10)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (1, 10)
- GitHub Check: e2e / 🧪 CLI v3 tests (ubuntu-latest - pnpm)
- 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: typecheck / typecheck
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (6)
packages/core/package.json (1)
191-191
: Dependencies Verified & No Vulnerabilities FoundThe installed versions of lodash.get (^4.4.2) and @types/lodash.get (^4.4.9) match the latest npm releases, and no security advisories were reported for lodash.get.
apps/webapp/app/v3/replayTask.ts (1)
7-25
: Review downstream handling of string-based payload transformThe Zod schema in apps/webapp/app/v3/replayTask.ts now returns
payload
as a JSON string instead of an object. Please verify that all consumers of this schema correctly parse the string and that the breaking change is documented.Key spots to check:
• apps/webapp/app/routes/resources.taskruns.$runParam.replay.ts –parse(formData, { schema: ReplayRunData })
now yieldssubmission.payload: string
. Ensure any code that readspayload
JSON-parses it before use.
• apps/webapp/app/components/runs/v3/ReplayRunDialog.tsx – afterparse(formData, { schema: ReplayRunData })
, confirm the dialog parses the string payload and handles SuperJSON metadata as intended.Confirm these updates, add or update documentation for the breaking change, and ensure no component assumes
payload
is an object.packages/core/test/ioSerialization.test.ts (1)
1-191
: Excellent test coverage!The test suite comprehensively covers all aspects of the
replaceSuperJsonPayload
function:
- Metadata preservation for various types (Date, BigInt, Set, Map)
- Nested object handling
- The undefined field override fix for issue #1968
- Error handling for invalid JSON
The tests are well-structured and clearly document the expected behavior.
packages/core/src/v3/utils/ioSerialization.ts (1)
509-553
: Well-implemented SuperJSON payload replacement!The function correctly preserves type metadata while replacing the payload data. The special handling for overridden undefined fields (issue #1968) is a thoughtful solution.
apps/webapp/app/v3/services/replayTaskRun.server.ts (2)
40-48
: Clean refactoring of payload handling!The new
overrideExistingPayloadPacket
method elegantly handles the SuperJSON metadata preservation when overriding payloads. The conditional logic properly falls back to the standard import packet flow when needed.The separation of concerns is improved by:
- Centralizing payload override logic
- Properly handling different payload types
- Preserving SuperJSON metadata for replayed runs
Also applies to: 121-138
13-13
: Import path should use subpath exportAccording to the coding guidelines for webapp files, imports from
@trigger.dev/core
should use subpath exports.-import { replaceSuperJsonPayload } from "@trigger.dev/core/v3/utils/ioSerialization"; +import { replaceSuperJsonPayload } from "@trigger.dev/core/v3/utils/ioSerialization";Actually, the import is already using the correct subpath. My apologies for the confusion.
This PR fixes a regression introduced in PR #2260, which affected replayed runs with superjson payloads.
Type metadata of the original payload was being ignored when replaying the run.
Drive-by improvements:
undefined
value in the payload doesn't work #1968 with a workaround for overriding originallyundefined
fields in the payload.replaceSuperJsonPayload
.There are still cases where our superjson overriding approach behaves in a somewhat unexpected way. E.g., if a user overrides a field that previously was of type
Date
with a value of typenumber
, the resulting merged payload will still have aDate
type for that field. So we might eventually need to revisit our current approach. There is no proper solution without exposing the superjson type metadata alongside the original payload and letting the user override both. That would make the behavior explicit, but it exposes some additional complexity.