-
-
Notifications
You must be signed in to change notification settings - Fork 791
fix: prevent circular reference errors on task indexing when using schemaTask #2383
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
🦋 Changeset detectedLatest commit: 19b6e35 The changes in this PR will be included in the next version bump. 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 |
Walkthrough
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes ✨ Finishing Touches
🧪 Generate unit tests
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. CodeRabbit Commands (Invoked using PR/Issue comments)Type 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: 1
🔭 Outside diff range comments (1)
packages/core/src/v3/resource-catalog/standardResourceCatalog.ts (1)
82-102
: Keep updates symmetrical: ignore schema/id in metadata updates and sync schema mapupdateTaskMetadata can accidentally re-introduce schema into TaskMetadata or let callers mutate id inside the stored metadata. Strip schema and id from updates; merge fns separately (as you already do), and keep the schema map in sync when schema is provided in updates.
Apply this diff:
updateTaskMetadata(id: string, updates: Partial<TaskMetadataWithFunctions>): void { const existingMetadata = this._taskMetadata.get(id); - if (existingMetadata) { - this._taskMetadata.set(id, { - ...existingMetadata, - ...updates, - }); - } + // Prevent schema and fns from being merged into TaskMetadata and ignore id updates + const { fns: fnsUpdates, schema: schemaUpdate, id: _ignoredId, ...metadataUpdates } = updates; + if (existingMetadata) { + this._taskMetadata.set(id, { + ...existingMetadata, + ...metadataUpdates, + }); + } - if (updates.fns) { + if (fnsUpdates) { const existingFunctions = this._taskFunctions.get(id); if (existingFunctions) { this._taskFunctions.set(id, { ...existingFunctions, - ...updates.fns, + ...fnsUpdates, }); } } + + // Keep schema storage in sync when updates include it + if (schemaUpdate !== undefined) { + if (schemaUpdate) { + this._taskSchemas.set(id, schemaUpdate); + } else { + this._taskSchemas.delete(id); + } + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.changeset/clean-beans-run.md
(1 hunks)packages/core/src/v3/resource-catalog/standardResourceCatalog.ts
(2 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{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/resource-catalog/standardResourceCatalog.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/resource-catalog/standardResourceCatalog.ts
🧠 Learnings (6)
📓 Common learnings
Learnt from: CR
PR: triggerdotdev/trigger.dev#0
File: .cursor/rules/writing-tasks.mdc:0-0
Timestamp: 2025-07-18T17:50:25.014Z
Learning: Applies to **/trigger/**/*.{ts,tsx,js,jsx} : When implementing schema tasks, use `schemaTask` from `trigger.dev/sdk/v3` and validate payloads as shown.
Learnt from: CR
PR: triggerdotdev/trigger.dev#0
File: .cursor/rules/writing-tasks.mdc:0-0
Timestamp: 2025-07-18T17:50:25.014Z
Learning: Applies to **/trigger/**/*.{ts,tsx,js,jsx} : ALWAYS generate Trigger.dev tasks using the `task` function from `trigger.dev/sdk/v3` and export them as shown in the correct pattern.
Learnt from: CR
PR: triggerdotdev/trigger.dev#0
File: .cursor/rules/writing-tasks.mdc:0-0
Timestamp: 2025-07-18T17:50:25.014Z
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-07-18T17:50:25.014Z
Learning: Applies to **/trigger/**/*.{ts,tsx,js,jsx} : When implementing scheduled (cron) tasks, use `schedules.task` from `trigger.dev/sdk/v3` and follow the shown patterns.
📚 Learning: 2025-07-18T17:50:25.014Z
Learnt from: CR
PR: triggerdotdev/trigger.dev#0
File: .cursor/rules/writing-tasks.mdc:0-0
Timestamp: 2025-07-18T17:50:25.014Z
Learning: Applies to **/trigger/**/*.{ts,tsx,js,jsx} : When implementing schema tasks, use `schemaTask` from `trigger.dev/sdk/v3` and validate payloads as shown.
Applied to files:
.changeset/clean-beans-run.md
packages/core/src/v3/resource-catalog/standardResourceCatalog.ts
📚 Learning: 2025-07-18T17:50:25.014Z
Learnt from: CR
PR: triggerdotdev/trigger.dev#0
File: .cursor/rules/writing-tasks.mdc:0-0
Timestamp: 2025-07-18T17:50:25.014Z
Learning: Applies to **/trigger/**/*.{ts,tsx,js,jsx} : ALWAYS generate Trigger.dev tasks using the `task` function from `trigger.dev/sdk/v3` and export them as shown in the correct pattern.
Applied to files:
.changeset/clean-beans-run.md
📚 Learning: 2025-07-18T17:50:25.014Z
Learnt from: CR
PR: triggerdotdev/trigger.dev#0
File: .cursor/rules/writing-tasks.mdc:0-0
Timestamp: 2025-07-18T17:50:25.014Z
Learning: Applies to **/trigger/**/*.{ts,tsx,js,jsx} : You MUST use `trigger.dev/sdk/v3` when writing Trigger.dev tasks.
Applied to files:
.changeset/clean-beans-run.md
📚 Learning: 2025-07-18T17:50:25.014Z
Learnt from: CR
PR: triggerdotdev/trigger.dev#0
File: .cursor/rules/writing-tasks.mdc:0-0
Timestamp: 2025-07-18T17:50:25.014Z
Learning: Applies to **/trigger/**/*.{ts,tsx,js,jsx} : When triggering a task from backend code, use `tasks.trigger`, `tasks.batchTrigger`, or `tasks.triggerAndPoll` as shown in the examples.
Applied to files:
.changeset/clean-beans-run.md
📚 Learning: 2025-07-18T17:50:25.014Z
Learnt from: CR
PR: triggerdotdev/trigger.dev#0
File: .cursor/rules/writing-tasks.mdc:0-0
Timestamp: 2025-07-18T17:50:25.014Z
Learning: Applies to **/trigger/**/*.{ts,tsx,js,jsx} : When using metadata in tasks, use the `metadata` API as shown, and only inside run functions or task lifecycle hooks.
Applied to files:
packages/core/src/v3/resource-catalog/standardResourceCatalog.ts
🧬 Code Graph Analysis (1)
packages/core/src/v3/resource-catalog/standardResourceCatalog.ts (1)
packages/trigger-sdk/src/v3/schedules/index.ts (1)
task
(64-91)
⏰ 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). (21)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (5, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (2, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (3, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (4, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (6, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (7, 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 (1, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (3, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (8, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (5, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (1, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (2, 8)
- GitHub Check: e2e / 🧪 CLI v3 tests (ubuntu-latest - npm)
- 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 - pnpm)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - npm)
- GitHub Check: typecheck / typecheck
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (3)
.changeset/clean-beans-run.md (1)
1-5
: Changelog entry LGTMClear, scoped patch note that matches the code changes. Good job keeping this a patch-level fix.
packages/core/src/v3/resource-catalog/standardResourceCatalog.ts (2)
64-64
: Good separation of schema from task metadataDestructuring out schema and excluding it from persisted metadata directly addresses circular JSON issues during indexing.
64-79
: Consumers exclusively usegetTaskSchema
for schemas
I searched across the repo and found no direct.schema
property accesses on anyTaskManifest
returned bylistTaskManifests()
orgetTaskManifest()
. Both CLI entry points (managed-index-worker.ts
anddev-index-worker.ts
) invokegetTaskSchema(task.id)
in theirconvertSchemasToJsonSchemas
helper, and no other code relies on aschema
field in the manifest objects.Files verified:
- packages/cli-v3/src/entryPoints/managed-index-worker.ts
- packages/cli-v3/src/entryPoints/dev-index-worker.ts
No further changes are required.
No description provided.