-
-
Notifications
You must be signed in to change notification settings - Fork 778
#2250 elderberry #2355
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
#2250 elderberry #2355
Conversation
|
WalkthroughThe changes introduce support for a Estimated code review effort🎯 2 (Simple) | ⏱️ ~7 minutes Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. ✨ 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 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/schemas/common.ts (1)
337-338
: Add basic validation guardrails for the newconcurrencyKey
.Because this key is used for queue–sharding logic, leaving it completely unrestricted invites accidental or maliciously long / malformed strings that could bloat DB rows, cache keys, or metric labels.
Recommend capping length (e.g. 256 chars) and/or restricting to safe characters.- concurrencyKey: z.string().optional(), + concurrencyKey: z.string().max(256).optional(),Apply the same guard to both
TaskRunExecution
andTaskRunContext
for consistency.Also applies to: 411-412
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
internal-packages/run-engine/src/engine/systems/runAttemptSystem.ts
(4 hunks)packages/core/src/v3/schemas/common.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:
internal-packages/run-engine/src/engine/systems/runAttemptSystem.ts
packages/core/src/v3/schemas/common.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/schemas/common.ts
🧠 Learnings (7)
📓 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 using retry, queue, machine, or maxDuration options, configure them as shown in the examples for 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.
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} : Each task needs a unique ID within your project.
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} : When using metadata in tasks, use the `metadata` API as shown, and only inside run functions or task lifecycle hooks.
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} : The `run` function contains your task logic in 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 triggering a task from backend code, use `tasks.trigger`, `tasks.batchTrigger`, or `tasks.triggerAndPoll` as shown in the examples.
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 inside another task, use `yourTask.trigger`, `yourTask.batchTrigger`, `yourTask.triggerAndWait`, `yourTask.batchTriggerAndWait`, `batch.triggerAndWait`, `batch.triggerByTask`, or `batch.triggerByTaskAndWait` 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} : When using idempotency, use the `idempotencyKeys` API and `idempotencyKey`/`idempotencyKeyTTL` options 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} : If you are able to generate an example payload for a task, do so.
📚 Learning: in apps/webapp/app/services/runsrepository.server.ts, the in-memory status filtering after fetching ...
Learnt from: matt-aitken
PR: triggerdotdev/trigger.dev#2264
File: apps/webapp/app/services/runsRepository.server.ts:172-174
Timestamp: 2025-07-12T18:06:04.133Z
Learning: In apps/webapp/app/services/runsRepository.server.ts, the in-memory status filtering after fetching runs from Prisma is intentionally used as a workaround for ClickHouse data delays. This approach is acceptable because the result set is limited to a maximum of 100 runs due to pagination, making the performance impact negligible.
Applied to files:
internal-packages/run-engine/src/engine/systems/runAttemptSystem.ts
📚 Learning: do not use or add new code to the legacy run engine; focus on using and migrating to run engine 2.0 ...
Learnt from: CR
PR: triggerdotdev/trigger.dev#0
File: .cursor/rules/webapp.mdc:0-0
Timestamp: 2025-07-18T17:49:47.180Z
Learning: Do not use or add new code to the legacy run engine; focus on using and migrating to Run Engine 2.0 in `internal/run-engine`.
Applied to files:
internal-packages/run-engine/src/engine/systems/runAttemptSystem.ts
📚 Learning: applies to **/trigger/**/*.{ts,tsx,js,jsx} : when using metadata in tasks, use the `metadata` api as...
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:
internal-packages/run-engine/src/engine/systems/runAttemptSystem.ts
packages/core/src/v3/schemas/common.ts
📚 Learning: applies to **/trigger/**/*.{ts,tsx,js,jsx} : when implementing schema tasks, use `schematask` from `...
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:
packages/core/src/v3/schemas/common.ts
📚 Learning: applies to **/trigger/**/*.{ts,tsx,js,jsx} : when using retry, queue, machine, or maxduration option...
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 retry, queue, machine, or maxDuration options, configure them as shown in the examples for Trigger.dev tasks.
Applied to files:
packages/core/src/v3/schemas/common.ts
📚 Learning: applies to **/trigger/**/*.{ts,tsx,js,jsx} : when implementing scheduled (cron) tasks, use `schedule...
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.
Applied to files:
packages/core/src/v3/schemas/common.ts
🔇 Additional comments (2)
internal-packages/run-engine/src/engine/systems/runAttemptSystem.ts (2)
184-184
: Ensure the DB columnconcurrencyKey
is indexed.Both
resolveTaskRunContext
andstartRunAttempt
now fetchconcurrencyKey
.
If future logic filters or sorts on this field, missing indexes will trigger full-table scans ontaskRun
, which is usually large.Please confirm that the migration adding the column also created an index, or add one proactively.
Also applies to: 401-401
252-252
: LGTM – value is correctly surfaced to the worker payload.The property is forwarded in both the lightweight context and the full execution envelope, keeping backwards compatibility.
Also applies to: 539-539
special issue for eric
Edits made:
packages/core/src/v3/schemas/common.ts
TaskRunExecution
andTaskRunContext
:concurrencyKey?: string
on the top-level of both objects.internal-packages/run-engine/src/engine/systems/runAttemptSystem.ts
resolveTaskRunContext
:concurrencyKey: true
in the Prismaselect
.concurrencyKey: run.concurrencyKey ?? undefined
at the top-level of the context.startRunAttempt
(where the execution payload is built):concurrencyKey: true
in the Prismaselect
.concurrencyKey: updatedRun.concurrencyKey ?? undefined
to the top-level of the execution payload.Why this fixes it:
TaskRunContext
directly from the execution payload. With the schema updates and engine population,ctx.concurrencyKey
will now be available in the task’srun
function as requested.Notes on validation:
There’s already a log in the managed worker that prints the received execution payload. That log will now include
concurrencyKey
, allowing you to verify it quickly.Lints on modified files show no errors.
Added
ctx.concurrencyKey
support:concurrencyKey?: string
toTaskRunExecution
andTaskRunContext
inpackages/core/src/v3/schemas/common.ts
.concurrencyKey
inresolveTaskRunContext
and the execution payload instartRunAttempt
withininternal-packages/run-engine/src/engine/systems/runAttemptSystem.ts
.ctx.concurrencyKey
inside taskrun
.