Night Shift: fix YouTube API hang on script failure#17
Night Shift: fix YouTube API hang on script failure#17
Conversation
The loadYouTubeAPI() function cached a promise that would never resolve if the script failed to load (network error, ad blocker, etc). This permanently broke the YouTube player until page reload. - Add script error handler that rejects promise and resets cache - Add 10s timeout as fallback for hung loads - Reset apiLoadPromise on failure so subsequent mounts can retry - Add tests for error handling and retry behavior Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughThis PR adds timeout and error handling mechanisms to the YouTube IFrame API loading process in YouTubePlayer. The implementation introduces a configurable load timeout, script error detection, and promise rejection logic, with corresponding test coverage validating error scenarios and retry capability. Changes
Sequence DiagramsequenceDiagram
participant Component as YouTubePlayer<br/>Component
participant Script as Script<br/>Element
participant API as YouTube<br/>IFrame API
participant Timeout as Timeout<br/>Mechanism
Component->>Script: Create & append script tag
Component->>Timeout: Start API_LOAD_TIMEOUT_MS
Script->>Timeout: Poll for completion
alt API Load Success
API->>Script: Trigger onYouTubeIframeAPIReady
Script->>Component: onLoad() - Promise resolves
Timeout->>Component: Clear timeout
else API Load Error
Script->>Component: onError() - Script error event
Component->>Component: Promise rejects
Timeout->>Component: Clear timeout
else Timeout Expires
Timeout->>Component: Promise rejects (timeout)
Component->>Component: Reset apiLoadPromise
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
PR Compliance Guide 🔍Below is a summary of compliance checks for this PR:
Compliance status legend🟢 - Fully Compliant🟡 - Partial Compliant 🔴 - Not Compliant ⚪ - Requires Further Human Verification 🏷️ - Compliance label |
|||||||||||||||||||||||||
PR Code Suggestions ✨Explore these optional code suggestions:
|
|||||||||
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix is OFF. To automatically fix reported issues with Cloud Agents, enable Autofix in the Cursor dashboard.
| clearTimeout(timeout) | ||
| apiLoadPromise = null | ||
| reject(new Error('Failed to load YouTube API script')) | ||
| } |
There was a problem hiding this comment.
Failed script tag not removed, breaking retry mechanism
Medium Severity
The onError handler resets apiLoadPromise to null to allow retries, but doesn't remove the failed <script id="youtube-iframe-api"> element from the DOM. On retry, loadYouTubeAPI() finds the dead script via existingScript, attaches new listeners, but the already-errored script won't fire events again — so the retry silently hangs until the 10-second timeout. The test masks this by manually calling script.remove() before retrying.
Additional Locations (1)
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
src/components/YouTubePlayer.test.tsxsrc/components/YouTubePlayer.tsx
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use TanStack Start framework with Bun runtime for the application
Tests must pass locally via
bun run testbefore committing code, as Husky pre-commit hooks will block commits with failing tests
Files:
src/components/YouTubePlayer.tsxsrc/components/YouTubePlayer.test.tsx
**/*.tsx
📄 CodeRabbit inference engine (CLAUDE.md)
Components with logic MUST have corresponding test files following the naming convention
ComponentName.test.tsx
Files:
src/components/YouTubePlayer.tsxsrc/components/YouTubePlayer.test.tsx
**/*.test.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use Vitest for unit testing and Playwright for integration testing
Files:
src/components/YouTubePlayer.test.tsx
**/*.test.tsx
📄 CodeRabbit inference engine (CLAUDE.md)
Component tests should be located in the same directory as the component file with naming convention
ComponentName.test.tsx
Files:
src/components/YouTubePlayer.test.tsx
⏰ 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). (1)
- GitHub Check: Cursor Bugbot
🔇 Additional comments (5)
src/components/YouTubePlayer.tsx (4)
86-87: LGTM on the timeout constant.Clean separation of the magic number into a named constant.
One consideration: the 10-second timeout is reasonable for most networks but may be tight on very slow connections. If you ever want to make this testable or configurable, exporting it (or accepting it as a parameter) would help, but it's fine as-is for now.
95-115: Solid promise guarding withonLoad/onErrorhelpers.The timeout → reject → cache-reset pattern is correct: once a promise is settled, subsequent
resolve()/reject()calls are no-ops, so there's no double-settlement risk if both timeout and error (or timeout and success) fire. Cache reset on failure enables the retry path correctly.
133-142: LGTM on new script creation path.Error listener is attached before appending to DOM, so no race between insertion and listener registration. The
onYouTubeIframeAPIReadycallback is set before the script is added, ensuring no missed callback.
249-254: LGTM on the catch block.The
isMountedguard correctly prevents state updates after unmount. The generic error message'Failed to load YouTube player'is user-friendly and matches what the tests assert.src/components/YouTubePlayer.test.tsx (1)
176-199: Good coverage of the script error path.The test correctly simulates the failure scenario: YT is undefined, the component creates a script tag, the error event fires, and the error message is displayed. Script cleanup at line 198 prevents DOM pollution across tests.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/components/YouTubePlayer.test.tsx`:
- Around line 175-235: Add a test that covers the timeout rejection path for the
API loader: in YouTubePlayer.test.tsx create a new test (e.g., "rejects and
allows retry when API load times out") that uses vi.useFakeTimers(), ensures
window.YT is undefined, renders <YouTubePlayer videoId="..."/>, advances timers
past API_LOAD_TIMEOUT_MS to trigger the timeout, waits for the "Failed to load
YouTube player" UI, unmounts and removes the 'youtube-iframe-api' script, then
assert that apiLoadPromise has been cleared so a subsequent render (after
setting window.YT to include MockYTPlayer/PlayerState) will call MockYTPlayer;
finally restore real timers and clean up the script tag. Ensure you reference
API_LOAD_TIMEOUT_MS, apiLoadPromise, YouTubePlayer and MockYTPlayer when
locating where to add assertions and cleanup.
- Around line 201-235: The test "allows retry after script load failure"
currently only asserts MockYTPlayer was called; update the assertion to verify
it was invoked for the retry render with videoId "retry123" by checking the
mock's call arguments (e.g., inspect MockYTPlayer.mock.calls or use a matcher)
to confirm the player was created with the expected videoId for the retry
render.
In `@src/components/YouTubePlayer.tsx`:
- Around line 117-131: When an existingScript is present but window.YT.Player
isn't ready the code currently attaches listeners which can miss a
previously-fired error; instead, if existingScript exists and window.YT?.Player
is falsy, remove the stale element (call existingScript.remove()) and clear any
stale window.onYouTubeIframeAPIReady if it points to a leftover callback, then
fall through to the normal script-creation path so a fresh <script> is inserted
and proper load/error handlers are attached; keep the original branch that
resolves immediately when window.YT.Player exists and only use the
remove-and-recreate behavior when the player is not ready.
- Line 84: The module-scoped variable apiLoadPromise in YouTubePlayer.tsx is
cached between tests and should be reset to ensure test isolation; update your
test suite's beforeEach hook to set apiLoadPromise = null so each test starts
with a clean module state and the YouTube API loader logic (apiLoadPromise) is
re-evaluated per test run.
|
|
||
| it('shows error when YouTube API script fails to load', async () => { | ||
| // Remove YT so the component tries to load the script | ||
| ;(window as unknown as { YT: unknown }).YT = undefined | ||
|
|
||
| render(<YouTubePlayer videoId="test123" />) | ||
|
|
||
| // The component should have created a script tag - fire error on it | ||
| await waitFor(() => { | ||
| const script = document.getElementById('youtube-iframe-api') | ||
| expect(script).not.toBeNull() | ||
| }) | ||
|
|
||
| const script = document.getElementById('youtube-iframe-api')! | ||
| act(() => { | ||
| script.dispatchEvent(new Event('error')) | ||
| }) | ||
|
|
||
| await waitFor(() => { | ||
| expect(screen.getByText('Failed to load YouTube player')).toBeInTheDocument() | ||
| }) | ||
|
|
||
| // Clean up the script tag for other tests | ||
| script.remove() | ||
| }) | ||
|
|
||
| it('allows retry after script load failure', async () => { | ||
| // First: fail the load | ||
| ;(window as unknown as { YT: unknown }).YT = undefined | ||
|
|
||
| const { unmount } = render(<YouTubePlayer videoId="test123" />) | ||
|
|
||
| await waitFor(() => { | ||
| const script = document.getElementById('youtube-iframe-api') | ||
| expect(script).not.toBeNull() | ||
| }) | ||
|
|
||
| const script = document.getElementById('youtube-iframe-api')! | ||
| act(() => { | ||
| script.dispatchEvent(new Event('error')) | ||
| }) | ||
|
|
||
| await waitFor(() => { | ||
| expect(screen.getByText('Failed to load YouTube player')).toBeInTheDocument() | ||
| }) | ||
|
|
||
| unmount() | ||
| script.remove() | ||
|
|
||
| // Second: retry with YT available should succeed | ||
| ;(window as unknown as { YT: unknown }).YT = { | ||
| Player: MockYTPlayer, | ||
| PlayerState: { UNSTARTED: -1, ENDED: 0, PLAYING: 1, PAUSED: 2, BUFFERING: 3, CUED: 5 }, | ||
| } | ||
|
|
||
| render(<YouTubePlayer videoId="retry123" />) | ||
|
|
||
| await waitFor(() => { | ||
| expect(MockYTPlayer).toHaveBeenCalled() | ||
| }) | ||
| }) |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Missing test: timeout rejection path.
The API_LOAD_TIMEOUT_MS (10 s) timeout is a core part of this fix — it's the fallback when neither load nor error fires. There's no test that verifies this behavior. Consider adding a test using vi.useFakeTimers() to advance time past the timeout and assert that the promise rejects and apiLoadPromise is reset for retry.
Sketch:
it('rejects and allows retry when API load times out', async () => {
vi.useFakeTimers()
;(window as unknown as { YT: unknown }).YT = undefined
const { unmount } = render(<YouTubePlayer videoId="test123" />)
// Advance past the 10s timeout
await act(async () => { vi.advanceTimersByTime(10_000) })
await waitFor(() => {
expect(screen.getByText('Failed to load YouTube player')).toBeInTheDocument()
})
unmount()
document.getElementById('youtube-iframe-api')?.remove()
vi.useRealTimers()
})🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/YouTubePlayer.test.tsx` around lines 175 - 235, Add a test
that covers the timeout rejection path for the API loader: in
YouTubePlayer.test.tsx create a new test (e.g., "rejects and allows retry when
API load times out") that uses vi.useFakeTimers(), ensures window.YT is
undefined, renders <YouTubePlayer videoId="..."/>, advances timers past
API_LOAD_TIMEOUT_MS to trigger the timeout, waits for the "Failed to load
YouTube player" UI, unmounts and removes the 'youtube-iframe-api' script, then
assert that apiLoadPromise has been cleared so a subsequent render (after
setting window.YT to include MockYTPlayer/PlayerState) will call MockYTPlayer;
finally restore real timers and clean up the script tag. Ensure you reference
API_LOAD_TIMEOUT_MS, apiLoadPromise, YouTubePlayer and MockYTPlayer when
locating where to add assertions and cleanup.
| it('allows retry after script load failure', async () => { | ||
| // First: fail the load | ||
| ;(window as unknown as { YT: unknown }).YT = undefined | ||
|
|
||
| const { unmount } = render(<YouTubePlayer videoId="test123" />) | ||
|
|
||
| await waitFor(() => { | ||
| const script = document.getElementById('youtube-iframe-api') | ||
| expect(script).not.toBeNull() | ||
| }) | ||
|
|
||
| const script = document.getElementById('youtube-iframe-api')! | ||
| act(() => { | ||
| script.dispatchEvent(new Event('error')) | ||
| }) | ||
|
|
||
| await waitFor(() => { | ||
| expect(screen.getByText('Failed to load YouTube player')).toBeInTheDocument() | ||
| }) | ||
|
|
||
| unmount() | ||
| script.remove() | ||
|
|
||
| // Second: retry with YT available should succeed | ||
| ;(window as unknown as { YT: unknown }).YT = { | ||
| Player: MockYTPlayer, | ||
| PlayerState: { UNSTARTED: -1, ENDED: 0, PLAYING: 1, PAUSED: 2, BUFFERING: 3, CUED: 5 }, | ||
| } | ||
|
|
||
| render(<YouTubePlayer videoId="retry123" />) | ||
|
|
||
| await waitFor(() => { | ||
| expect(MockYTPlayer).toHaveBeenCalled() | ||
| }) | ||
| }) |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Strengthen the retry assertion to verify the correct videoId.
The assertion at line 233 only checks that MockYTPlayer was called, but doesn't verify it was called with the retry videoId ("retry123"). A more precise assertion prevents false positives if the mock was somehow called during the first (failing) render path.
♻️ Proposed fix
await waitFor(() => {
- expect(MockYTPlayer).toHaveBeenCalled()
+ const lastCall = MockYTPlayer.mock.calls[MockYTPlayer.mock.calls.length - 1]
+ expect((lastCall[1] as { videoId?: string }).videoId).toBe('retry123')
})🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/YouTubePlayer.test.tsx` around lines 201 - 235, The test
"allows retry after script load failure" currently only asserts MockYTPlayer was
called; update the assertion to verify it was invoked for the retry render with
videoId "retry123" by checking the mock's call arguments (e.g., inspect
MockYTPlayer.mock.calls or use a matcher) to confirm the player was created with
the expected videoId for the retry render.
| @@ -83,41 +83,62 @@ interface YouTubePlayerProps { | |||
|
|
|||
| let apiLoadPromise: Promise<void> | null = null | |||
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check references to apiLoadPromise in both files
rg -n 'apiLoadPromise' src/components/YouTubePlayer.tsx
echo "---"
rg -n 'apiLoadPromise' src/components/YouTubePlayer.test.tsxRepository: EtanHey/songscript
Length of output: 312
🏁 Script executed:
#!/bin/bash
# Get the full test file to understand current test setup
wc -l src/components/YouTubePlayer.test.tsxRepository: EtanHey/songscript
Length of output: 103
🏁 Script executed:
#!/bin/bash
# Read the test file to understand test setup and state management
cat -n src/components/YouTubePlayer.test.tsxRepository: EtanHey/songscript
Length of output: 8124
🏁 Script executed:
#!/bin/bash
# Verify the loadYouTubeAPI function implementation to confirm the caching behavior
sed -n '84,145p' src/components/YouTubePlayer.tsxRepository: EtanHey/songscript
Length of output: 1662
Reset apiLoadPromise in beforeEach to ensure test isolation and prevent module state pollution.
The module-scoped apiLoadPromise is not reset between tests, creating a potential fragility. While current tests pass because most rely on beforeEach setting window.YT (which triggers the early return on line 91) and error tests explicitly reset the state, this masks an underlying issue: if a test completes the promise successfully, it remains cached for subsequent tests. Adding apiLoadPromise = null to the beforeEach hook would eliminate this fragility and make tests explicitly independent of module state.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/YouTubePlayer.tsx` at line 84, The module-scoped variable
apiLoadPromise in YouTubePlayer.tsx is cached between tests and should be reset
to ensure test isolation; update your test suite's beforeEach hook to set
apiLoadPromise = null so each test starts with a clean module state and the
YouTube API loader logic (apiLoadPromise) is re-evaluated per test run.
| const existingScript = document.getElementById('youtube-iframe-api') | ||
| if (existingScript) { | ||
| if (window.YT && window.YT.Player) { | ||
| clearTimeout(timeout) | ||
| resolve() | ||
| } else { | ||
| const originalCallback = window.onYouTubeIframeAPIReady | ||
| window.onYouTubeIframeAPIReady = () => { | ||
| originalCallback?.() | ||
| resolve() | ||
| onLoad() | ||
| } | ||
| existingScript.addEventListener('error', onError) | ||
| } | ||
| return | ||
| } |
There was a problem hiding this comment.
Edge case: existing script that already errored before this code runs.
If a previous attempt inserted the <script> tag and it fired its error event before this new promise executor attaches the listener at line 128, the error event won't re-fire, and the promise will hang until the 10 s timeout. The timeout does provide a safety net, but the caller experiences a needless 10-second delay in that scenario.
A simple mitigation: when an existing script is found and window.YT.Player isn't ready, also check whether the script's network state indicates a failure. For example:
🛡️ Suggested improvement
} else {
+ // If the existing script already failed, reject immediately
+ if ((existingScript as HTMLScriptElement).error ||
+ ((existingScript as HTMLScriptElement).readyState === undefined &&
+ !(existingScript as HTMLScriptElement).src)) {
+ onError()
+ return
+ }
const originalCallback = window.onYouTubeIframeAPIReadyAlternatively, a simpler approach: remove the stale script tag and fall through to create a fresh one:
♻️ Alternative: remove stale script and re-create
const existingScript = document.getElementById('youtube-iframe-api')
if (existingScript) {
if (window.YT && window.YT.Player) {
clearTimeout(timeout)
resolve()
- } else {
- const originalCallback = window.onYouTubeIframeAPIReady
- window.onYouTubeIframeAPIReady = () => {
- originalCallback?.()
- onLoad()
- }
- existingScript.addEventListener('error', onError)
+ } else {
+ // Script exists but YT isn't ready — remove it and create a fresh one
+ existingScript.remove()
}
- return
+ if (window.YT && window.YT.Player) return
}This is a narrow edge case and the timeout does cover it, so this is not blocking.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/YouTubePlayer.tsx` around lines 117 - 131, When an
existingScript is present but window.YT.Player isn't ready the code currently
attaches listeners which can miss a previously-fired error; instead, if
existingScript exists and window.YT?.Player is falsy, remove the stale element
(call existingScript.remove()) and clear any stale
window.onYouTubeIframeAPIReady if it points to a leftover callback, then fall
through to the normal script-creation path so a fresh <script> is inserted and
proper load/error handlers are attached; keep the original branch that resolves
immediately when window.YT.Player exists and only use the remove-and-recreate
behavior when the player is not ready.
1 participant
User description
Automated improvement by GolemsZikaron Night Shift.
fix YouTube API hang on script failure
PR Type
Bug fix, Tests
Description
Fix YouTube API promise hang when script fails to load
Add error handler and timeout to reject cached promise
Reset apiLoadPromise on failure to enable retries
Add comprehensive tests for error handling and retry scenarios
Diagram Walkthrough
File Walkthrough
YouTubePlayer.tsx
Add error handling and timeout to YouTube API loadersrc/components/YouTubePlayer.tsx
scripts
YouTubePlayer.test.tsx
Add tests for API load failure and retry behaviorsrc/components/YouTubePlayer.test.tsx
Note
Low Risk
Localized changes to client-side script loading with added test coverage; low risk aside from potential edge cases around script event timing/timeouts.
Overview
Prevents
YouTubePlayerfrom hanging indefinitely when the YouTube IFrame API script fails to load by adding timeout-based rejection anderrorevent handling toloadYouTubeAPI, and resetting the cachedapiLoadPromiseon failure to allow retries.Adds tests covering script load failure UI (
Failed to load YouTube player) and verifying a subsequent mount can succeed after an initial load error.Written by Cursor Bugbot for commit 27ba3fc. This will update automatically on new commits. Configure here.
Summary by CodeRabbit
Bug Fixes
New Features
Tests