Repository navigation
Conversation
|
|
Hi @cocvn157, 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. |
|
Caution Review failedThe pull request is closed.
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. |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| for (const s of signals) { | ||
| s.addEventListener("abort", () => controller.abort(s.reason), { | ||
| once: true, | ||
| signal: controller.signal, | ||
| }); |
There was a problem hiding this comment.
🟡 Finished streams retain abort listeners
On older Safari, anyAbortSignal keeps listeners after streams close without an abort. Reusing a caller signal accumulates retained callbacks and stream state across subscriptions.
Learn more
The fallback removes a source listener only when the composed controller aborts. An AgentChat subscription can close on a settled or empty response without aborting its controller. A browser reconnect also creates an outer composed signal whose controller remains un-aborted after an ordinary turn completes. A long-lived caller signal therefore retains the fallback's listener and captured controller for each such subscription.
Example: A page reuses one AbortController.signal for ten reconnects on Safari 16. Each completed reconnect leaves a listener on that signal, even though no stream remains open.
Recommended fix: Give the fallback an explicit disposal mechanism and call it on stream teardown, including the outer reconnect composition; alternatively ensure every composed controller aborts on every terminal path. Verify normal completion and settled-EOF cleanup as well as cancellation.
Was this helpful? React with 👍 or 👎 to provide feedback.
| * to manual listener forwarding on runtimes without `AbortSignal.any` (such as | ||
| * Safari / iOS Safari < 17.4). | ||
| */ | ||
| export function anyAbortSignal(signals: AbortSignal[]): AbortSignal { |
There was a problem hiding this comment.
🟡 Safari fix lacks a release note
The anyAbortSignal change fixes a customer-facing SDK failure, but this PR adds no changeset. The fix lacks the required patch release entry.
Learn more
The repository's AGENTS.md and CONTRIBUTING.md require a changeset for user-facing fixes in public packages. This change lets customers use chat transport on older Safari versions, so it meets that requirement.
Example: A customer upgrading the SDK to avoid AbortSignal.any is not a function cannot find this fix in the package's changeset-driven release notes.
Recommended fix: Add a patch changeset for @trigger.dev/sdk with a user-focused description of Safari chat compatibility.
Was this helpful? React with 👍 or 👎 to provide feedback.
| }); | ||
|
|
||
| try { | ||
| global.fetch = vi.fn().mockImplementation(async (url: string | URL) => { |
There was a problem hiding this comment.
🟡 Browser regression test uses prohibited mocks
The new Safari test replaces global.fetch with vi.fn() twice. Repository testing rules prohibit mocks in new tests.
Learn more
The repository's AGENTS.md and test guidance prohibit new tests from using mocks, spies, stubs, or fakes. The added Safari test assigns a mocked global.fetch here and again later in the same test.
Example: The compatibility check serves synthetic responses from a vi.fn() implementation instead of exercising the real fetch boundary.
Recommended fix: Use an integration-style test with a real local HTTP endpoint and restore the original environment afterward, or move signal-composition assertions into a focused test that needs no fetch replacement.
Was this helpful? React with 👍 or 👎 to provide feedback.
| const callerAbort = new AbortController(); | ||
| const stream = await transport.sendMessages({ | ||
| trigger: "submit-message", | ||
| chatId: "chat-safari", | ||
| messageId: undefined, | ||
| messages: [createUserMessage("Hi from Safari 16")], | ||
| abortSignal: callerAbort.signal, | ||
| }); | ||
| const chunks = await drainChunks(stream); | ||
| expect(chunks).toEqual([{ type: "text-delta", id: "p1", delta: "Safari fallback" }]); |
There was a problem hiding this comment.
🔍 Active caller abort remains untested
The test never aborts callerAbort while a stream is open. Its pre-aborted case cannot detect broken forwarding of later abort events.
Was this helpful? React with 👍 or 👎 to provide feedback.
| }); | ||
|
|
||
| describe("Safari < 17.4 compatibility (no AbortSignal.any)", () => { | ||
| it("streams and propagates caller aborts when AbortSignal.any is undefined", async () => { |
There was a problem hiding this comment.
🔍 Safari test lacks required regression comment
The new test has no // Catches: <bug> comment, as required by the repository's testing instructions.
Was this helpful? React with 👍 or 👎 to provide feedback.
Fixes #5098
TriggerChatTransportandAgentChatcalledAbortSignal.any()unconditionally when combining a caller-suppliedAbortSignalwith the internal stream controller signal, which throwsTypeError: AbortSignal.any is not a functionon Safari and iOS Safari versions prior to 17.4.This adds an
anyAbortSignalhelper inai-shared.tsthat usesAbortSignal.anywhen available and falls back to listener forwarding on older runtimes, and updateschat.tsandchat-client.tsto use it.