Fix debounce forwarding in batchTriggerAndWait arrays#3446
Fix debounce forwarding in batchTriggerAndWait arrays#3446GautamBytes wants to merge 1 commit intotriggerdotdev:mainfrom
Conversation
Signed-off-by: Gautam Manchandani <gautammanch@Gautams-MacBook-Air.local>
🦋 Changeset detectedLatest commit: 6802690 The changes in this PR will be included in the next version bump. This PR includes changesets to release 29 packages
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 |
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
WalkthroughThis pull request adds support for forwarding per-item Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
|
Hi @GautamBytes, 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. |
|
@ericallam can you check this , not sure why its closed even after claude not added me to vouch.yml |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
There was a problem hiding this comment.
🔴 batchTrigger_internal array path silently drops per-item debounce options
The PR fixes the missing debounce forwarding in batchTriggerAndWait_internal's array path (line 2510), but the same bug still exists in the sister function batchTrigger_internal at lines 2214-2236. When users call batchTrigger() with an array of items that include per-item debounce options, those options are silently dropped. The streaming counterpart transformSingleTaskBatchItemsStream correctly includes debounce: item.options?.debounce at packages/trigger-sdk/src/v3/shared.ts:2041, confirming this is an oversight. The BatchItem type uses TriggerOptions which includes a debounce field (packages/core/src/v3/types/tasks.ts:946), so users can set it but it has no effect in the array path.
(Refers to lines 2234-2236)
Was this helpful? React with 👍 or 👎 to provide feedback.
| describe("batchTriggerAndWait debounce forwarding", () => { | ||
| afterEach(() => { | ||
| vi.doUnmock("@trigger.dev/core/v3"); | ||
| vi.resetModules(); | ||
| vi.restoreAllMocks(); | ||
| }); | ||
|
|
||
| async function setupBatchTriggerAndWaitHarness() { | ||
| vi.resetModules(); | ||
|
|
||
| const capturedItems: any[] = []; | ||
|
|
||
| const createBatch = vi.fn(async ({ runCount }: { runCount: number }) => ({ | ||
| id: "batch_test123", | ||
| runCount, | ||
| publicAccessToken: "access_token", | ||
| isCached: false, | ||
| })); | ||
|
|
||
| const streamBatchItems = vi.fn(async (_batchId: string, items: any[]) => { | ||
| capturedItems.push(...items); | ||
|
|
||
| return { | ||
| id: "batch_test123", | ||
| itemsAccepted: items.length, | ||
| itemsDeduplicated: 0, | ||
| sealed: true, | ||
| }; | ||
| }); | ||
|
|
||
| const waitForBatch = vi.fn(async ({ id }: { id: string }) => ({ | ||
| id, | ||
| items: [], | ||
| })); | ||
|
|
||
| vi.doMock("@trigger.dev/core/v3", async (importOriginal) => { | ||
| const original = await importOriginal<typeof import("@trigger.dev/core/v3")>(); | ||
|
|
||
| return { | ||
| ...original, | ||
| apiClientManager: { | ||
| clientOrThrow: vi.fn(() => ({ | ||
| createBatch, | ||
| streamBatchItems, | ||
| })), | ||
| } as any, | ||
| runtime: { | ||
| ...original.runtime, | ||
| waitForBatch, | ||
| } as any, | ||
| taskContext: { | ||
| ctx: { | ||
| run: { | ||
| id: "run_123", | ||
| isTest: false, | ||
| }, | ||
| }, | ||
| worker: { | ||
| version: "worker_123", | ||
| }, | ||
| } as any, | ||
| }; | ||
| }); | ||
|
|
||
| const tasksModule = await import("./tasks.js"); | ||
|
|
||
| return { | ||
| ...tasksModule, | ||
| capturedItems, | ||
| createBatch, | ||
| streamBatchItems, | ||
| waitForBatch, | ||
| }; | ||
| } | ||
|
|
||
| it("forwards per-item debounce for task.batchTriggerAndWait array items", async () => { | ||
| const { task, capturedItems, streamBatchItems, waitForBatch } = | ||
| await setupBatchTriggerAndWaitHarness(); | ||
| const debounce = { key: "same-key", delay: "30s", mode: "trailing" as const }; | ||
| const taskId = `batch-debounce-task-${++taskIdCounter}`; | ||
|
|
||
| const myTask = task({ | ||
| id: taskId, | ||
| run: async (payload: { id: string }) => payload, | ||
| }); | ||
|
|
||
| await myTask.batchTriggerAndWait([{ payload: { id: "a" }, options: { debounce } }]); | ||
|
|
||
| expect(streamBatchItems).toHaveBeenCalledTimes(1); | ||
| expect(waitForBatch).toHaveBeenCalledTimes(1); | ||
| expect(capturedItems).toHaveLength(1); | ||
| expect(capturedItems[0]?.task).toBe(taskId); | ||
| expect(capturedItems[0]?.options?.debounce).toEqual(debounce); | ||
| }); | ||
|
|
||
| it("forwards per-item debounce for tasks.batchTriggerAndWait array items", async () => { | ||
| const { tasks, capturedItems, streamBatchItems, waitForBatch } = | ||
| await setupBatchTriggerAndWaitHarness(); | ||
| const debounce = { key: "same-key", delay: "30s", mode: "trailing" as const }; | ||
|
|
||
| await tasks.batchTriggerAndWait("batch-debounce-by-id-task", [ | ||
| { payload: { id: "a" }, options: { debounce } }, | ||
| ]); | ||
|
|
||
| expect(streamBatchItems).toHaveBeenCalledTimes(1); | ||
| expect(waitForBatch).toHaveBeenCalledTimes(1); | ||
| expect(capturedItems).toHaveLength(1); | ||
| expect(capturedItems[0]?.task).toBe("batch-debounce-by-id-task"); | ||
| expect(capturedItems[0]?.options?.debounce).toEqual(debounce); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
🚩 New tests use vi.doMock/vi.fn contrary to repo testing conventions
The new batchTriggerAndWait debounce forwarding test suite at packages/trigger-sdk/src/v3/shared.test.ts:222-332 uses vi.doMock, vi.fn(), vi.resetModules(), and vi.restoreAllMocks(). The repository's testing guidelines in ai/references/tests.md:78-83 explicitly state 'Do not mock anything', 'Do not use mocks in tests', and 'Do not use spies in tests'. However, this is a case where testing SDK option forwarding through internal functions practically requires mocking the API client and runtime — there's no Redis/Postgres dependency where testcontainers could substitute. The prose in ai/references/tests.md:26 also says 'We almost NEVER mock anything' (emphasis on 'almost'), suggesting rare exceptions exist. Worth discussing whether this test approach is acceptable or if an alternative testing strategy should be used.
Was this helpful? React with 👍 or 👎 to provide feedback.
1 participant
Summary
Fixes the array input path for
batchTriggerAndWaitso per-itemoptions.debounceis forwarded instead of being dropped.The async iterable path already forwarded debounce correctly; this brings the array serializer in line with that behavior for both
task.batchTriggerAndWait([...])andtasks.batchTriggerAndWait(id, [...]).Closes TRI-8310.