Skip to content

Night Shift: fix YouTube API hang on script failure #17

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.

Sign up for GitHub

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

Open
EtanHey wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Night Shift: fix YouTube API hang on script failure #17

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
61 changes: 61 additions & 0 deletions src/components/YouTubePlayer.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -172,4 +172,65 @@ describe('YouTubePlayer', () => {
// Check destroy was called
expect(mockPlayerInstance.destroy).toHaveBeenCalled()
})

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()
})
})
Comment on lines +175 to +235
Copy link

@coderabbitai coderabbitai bot Feb 21, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ 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.

Comment on lines +201 to +235
Copy link

@coderabbitai coderabbitai bot Feb 21, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 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.

})
27 changes: 24 additions & 3 deletions src/components/YouTubePlayer.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,41 +83,62 @@ interface YouTubePlayerProps {

let apiLoadPromise: Promise<void> | null = null
Copy link

@coderabbitai coderabbitai bot Feb 21, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

🧩 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.tsx

Repository: 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.tsx

Repository: 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.tsx

Repository: 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.tsx

Repository: 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 API_LOAD_TIMEOUT_MS = 10000

function loadYouTubeAPI(): Promise<void> {
if (apiLoadPromise) return apiLoadPromise

if (typeof window !== 'undefined' && window.YT && window.YT.Player) {
return Promise.resolve()
}

apiLoadPromise = new Promise((resolve) => {
apiLoadPromise = new Promise((resolve, reject) => {
if (typeof window === 'undefined') {
resolve()
return
}

const timeout = setTimeout(() => {
apiLoadPromise = null
reject(new Error('YouTube API load timed out'))
}, API_LOAD_TIMEOUT_MS)

const onLoad = () => {
clearTimeout(timeout)
resolve()
}

const onError = () => {
clearTimeout(timeout)
apiLoadPromise = null
reject(new Error('Failed to load YouTube API script'))
}
Copy link

@cursor cursor bot Feb 21, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Fix in Cursor Fix in Web


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
}
Comment on lines 117 to 131
Copy link

@coderabbitai coderabbitai bot Feb 21, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

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.onYouTubeIframeAPIReady

Alternatively, 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.


window.onYouTubeIframeAPIReady = () => {
resolve()
onLoad()
}

const script = document.createElement('script')
script.id = 'youtube-iframe-api'
script.src = 'https://www.youtube.com/iframe_api'
script.async = true
script.addEventListener('error', onError)
document.body.appendChild(script)
})

Expand Down