fix(plex): warm connection before opening SSE to avoid stuck CONNECTING - #1208
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughPlexEventSource now performs an authenticated, timeout-bounded pre-connection probe to ChangesPlex SSE health probe pre-connection
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/services/plex-server/sse/plex-event-source.ts (1)
201-216: 💤 Low valueConsider using
AbortSignal.timeout()for cleaner timeout handling.The existing
testConnectionReachabilityfunction inconnection-cache.tsusesAbortSignal.timeout(timeoutMs)for the same/identityprobe pattern. Using that approach here would be more concise and consistent with the codebase.Also,
testConnectionReachabilityincludesX-Plex-Client-IdentifieralongsideX-Plex-Token. While the probe continues on failure so this isn't critical, adding it would match the established contract.♻️ Suggested simplification
private async warmConnection(): Promise<void> { - const controller = new AbortController() - const timer = setTimeout(() => controller.abort(), HEALTH_PROBE_TIMEOUT_MS) try { const res = await fetch(`${this.serverUrl}/identity`, { - headers: { 'X-Plex-Token': this.token }, - signal: controller.signal, + headers: { + 'X-Plex-Token': this.token, + 'X-Plex-Client-Identifier': PLEX_CLIENT_IDENTIFIER, + }, + signal: AbortSignal.timeout(HEALTH_PROBE_TIMEOUT_MS), }) // Drain the response so the connection returns to the pool. await res.text() } catch { this.log.debug('SSE pre-connect health probe failed - continuing') - } finally { - clearTimeout(timer) } }Note: This would require importing
PLEX_CLIENT_IDENTIFIERfrom the appropriate location.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/services/plex-server/sse/plex-event-source.ts` around lines 201 - 216, Replace the manual AbortController + setTimeout logic in warmConnection with AbortSignal.timeout(HEALTH_PROBE_TIMEOUT_MS) to simplify timeout handling, and add the X-Plex-Client-Identifier header (use PLEX_CLIENT_IDENTIFIER constant) alongside X-Plex-Token when fetching `${this.serverUrl}/identity`; update imports to bring in PLEX_CLIENT_IDENTIFIER and ensure the fetch uses the AbortSignal returned by AbortSignal.timeout and still drains the response with await res.text(), keeping the existing try/catch/finally behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/services/plex-server/sse/plex-event-source.ts`:
- Around line 201-216: Replace the manual AbortController + setTimeout logic in
warmConnection with AbortSignal.timeout(HEALTH_PROBE_TIMEOUT_MS) to simplify
timeout handling, and add the X-Plex-Client-Identifier header (use
PLEX_CLIENT_IDENTIFIER constant) alongside X-Plex-Token when fetching
`${this.serverUrl}/identity`; update imports to bring in PLEX_CLIENT_IDENTIFIER
and ensure the fetch uses the AbortSignal returned by AbortSignal.timeout and
still drains the response with await res.text(), keeping the existing
try/catch/finally behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 72346326-ba90-4f3c-ad73-f1661d8b89e4
📒 Files selected for processing (1)
src/services/plex-server/sse/plex-event-source.ts
Description
Related Issues
Type of Change
Testing Performed
Screenshots
Checklist
Summary by CodeRabbit