Realtime fixes: safari, timezones, and TIMED_OUT status #1585
Conversation
ericallam
commented
Jan 7, 2025
•
ericallam
commented
- Adds a polyfill for ReadableStream[@@asyncIterator], which for some reason Safari still does not support
- Adds missing TIMED_OUT TaskRunStatus case
- Fixes realtime date coercion by forcing UTC (which they will always be).
🦋 Changeset detectedLatest commit: a1a7531 The changes in this PR will be included in the next version bump. This PR includes changesets to release 12 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 |
|
Warning Rate limit exceeded@ericallam has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 26 minutes and 16 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (4)
WalkthroughA patch has been introduced for the Changes
Sequence DiagramsequenceDiagram
participant Browser
participant RunStream
participant ReadableStream
Browser->>RunStream: Check browser type
RunStream->>RunStream: Detect Safari
alt Is Safari
RunStream->>ReadableStream: Apply async iterator polyfill
else Not Safari
RunStream->>ReadableStream: Use native implementation
end
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/core/src/v3/apiClient/runStream.ts (2)
545-559: Consider using feature detection instead of User-Agent sniffing.While the current implementation is defensive with proper environment checks, User-Agent sniffing can be fragile and may require maintenance as browser signatures evolve. Consider using feature detection:
const isSafari = () => { - // Check if we're in a browser environment - if ( - typeof window !== "undefined" && - typeof navigator !== "undefined" && - typeof navigator.userAgent === "string" - ) { - return ( - /^((?!chrome|android).)*safari/i.test(navigator.userAgent) || - /iPad|iPhone|iPod/.test(navigator.userAgent) - ); - } - // If we're not in a browser environment, return false - return false; + return ( + typeof window !== "undefined" && + typeof ReadableStream === "function" && + !ReadableStream.prototype[Symbol.asyncIterator] + ); };
575-576: Document the reason for @ts-expect-error.While the use of @ts-expect-error is valid here, it would be helpful to document why these type assertions are necessary.
Add comments explaining the type assertions:
- // @ts-expect-error + // @ts-expect-error - TypeScript doesn't recognize dynamic additions to ReadableStream.prototype ReadableStream.prototype.values ??= function ({ preventCancel = false } = {}) { - // @ts-expect-error + // @ts-expect-error - TypeScript doesn't recognize dynamic additions to ReadableStream.prototype ReadableStream.prototype[Symbol.asyncIterator] ??= ReadableStream.prototype.values;Also applies to: 607-608
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.changeset/poor-shirts-move.md(1 hunks)packages/core/src/v3/apiClient/runStream.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - pnpm)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - npm)
- GitHub Check: e2e / 🧪 CLI v3 tests (buildjet-8vcpu-ubuntu-2204 - pnpm)
- GitHub Check: units / 🧪 Unit Tests
- GitHub Check: e2e / 🧪 CLI v3 tests (buildjet-8vcpu-ubuntu-2204 - npm)
- GitHub Check: typecheck / typecheck
🔇 Additional comments (2)
packages/core/src/v3/apiClient/runStream.ts (1)
561-609: Implementation follows the spec correctly.The polyfill implementation is robust with proper error handling, resource cleanup, and spec compliance. The code properly releases the reader lock in both success and error cases.
Let's verify that we're not overwriting any existing implementations:
✅ Verification successful
Polyfill implementation is safe and isolated
The implementation is the only ReadableStream polyfill in the codebase, properly guarded for Safari-only execution, and safely handles existing implementations through nullish coalescing.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check if any other files in the codebase are implementing ReadableStream polyfills # to avoid potential conflicts. rg -l "ReadableStream.prototype\[Symbol.asyncIterator\]"Length of output: 102
.changeset/poor-shirts-move.md (1)
1-5: LGTM!The changeset correctly describes the patch and its purpose.
@trigger.dev/react-hooks
trigger.dev
@trigger.dev/rsc
@trigger.dev/build
@trigger.dev/sdk
@trigger.dev/core
commit: |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/core/src/v3/apiClient/runStream.ts (2)
548-562: Consider more robust browser detection methodsWhile the current implementation works, User-Agent string detection has some limitations:
- It can be spoofed
- It may require updates as new devices/browsers are released
- Some browsers might modify their User-Agent strings
Consider using feature detection instead:
const isSafari = () => { - // Check if we're in a browser environment - if ( - typeof window !== "undefined" && - typeof navigator !== "undefined" && - typeof navigator.userAgent === "string" - ) { - return ( - /^((?!chrome|android).)*safari/i.test(navigator.userAgent) || - /iPad|iPhone|iPod/.test(navigator.userAgent) - ); - } - // If we're not in a browser environment, return false - return false; + // Check if we're in a browser environment + if (typeof window === "undefined") return false; + + // Use feature detection to identify Safari + return !window.ReadableStream?.prototype?.[Symbol.asyncIterator]; };
564-612: LGTM: Well-implemented polyfill with proper error handlingThe polyfill implementation is solid:
- Follows the spec correctly
- Properly manages reader lifecycle
- Includes comprehensive error handling
- Well-documented with references
Consider adding type information for the error in the catch block:
- } catch (e) { + } catch (e: unknown) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/core/src/v3/apiClient/runStream.ts(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - pnpm)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - npm)
- GitHub Check: e2e / 🧪 CLI v3 tests (buildjet-8vcpu-ubuntu-2204 - pnpm)
- GitHub Check: e2e / 🧪 CLI v3 tests (buildjet-8vcpu-ubuntu-2204 - npm)
- GitHub Check: typecheck / typecheck
- GitHub Check: units / 🧪 Unit Tests
🔇 Additional comments (2)
packages/core/src/v3/apiClient/runStream.ts (2)
531-533: LGTM: Status mapping addition is correctThe new "TIMED_OUT" status case follows the existing pattern and maintains consistency with other status mappings.
547-612: Verify Safari version compatibilityLet's verify which Safari versions require this polyfill to ensure we're not applying it unnecessarily to newer versions that might support the feature natively.
✅ Verification successful
Safari polyfill implementation verified as correct and necessary
The polyfill is properly implemented following the streams specification and is still required due to an open WebKit bug (#194379). The implementation safely detects Safari browsers and only applies the polyfill when needed, using nullish coalescing to avoid conflicts with native implementations.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check Safari versions that support ReadableStream[Symbol.asyncIterator] # by searching for any version-specific checks or comments in the codebase # Search for Safari version-related comments or checks rg -i "safari.*version|webkit.*bug" -g '!{node_modules,dist,build}/*' # Search for any existing browser compatibility checks rg -i "browser.*compat|feature.*detect" -g '!{node_modules,dist,build}/*'Length of output: 65804
Script:
#!/bin/bash # Search for ReadableStream related code and version checks rg -i "readablestream|safari.*version" -A 5 -B 5 # Search for any browser version detection patterns rg -i "browser.*version|useragent.*version" -A 5 -B 5Length of output: 103782
ericallam
changed the title
ericallam
changed the title
ericallam
force-pushed
the
fix/realtime-safari
branch
from
1bd320b to
a1a7531
Compare
