Skip to content

fix(httpclient): accept ICY stream responses#284

Merged
bjarneo merged 2 commits into
mainfrom
fix/icy-radio-streams
Jul 25, 2026
Merged

fix(httpclient): accept ICY stream responses#284
bjarneo merged 2 commits into
mainfrom
fix/icy-radio-streams

Conversation

@bjarneo

@bjarneo bjarneo commented Jul 25, 2026

Copy link
Copy Markdown
Owner

Fixes #277. Normalizes SHOUTcast ICY status lines before net/http parses them and adds HTTP/HTTPS regression coverage.

Summary by CodeRabbit

  • New Features
    • Streaming connections now accept SHOUTcast/ICY responses over both HTTP and HTTPS.
    • HTTPS streaming now applies better TLS settings and keeps HTTP/1.x behavior for improved compatibility.
  • Bug Fixes
    • Streaming responses with ICY status are now correctly transformed into a standard HTTP status so net/http can parse them and read the full response body.
  • Tests
    • Added coverage for ICY handling in both HTTP and HTTPS scenarios.

@coderabbitai

coderabbitai Bot commented Jul 25, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 064c25b8-6671-4afc-948f-275e466edde6

📥 Commits

Reviewing files that changed from the base of the PR and between a32d3a3 and 4bc3aaa.

📒 Files selected for processing (1)
  • internal/httpclient/client.go

📝 Walkthrough

Walkthrough

The streaming HTTP client now wraps plain TCP and TLS connections to translate SHOUTcast ICY responses into HTTP/1.0 responses. TLS configuration is cloned and constrained to HTTP/1.x, with HTTP and HTTPS behavior covered by tests.

Changes

ICY streaming support

Layer / File(s) Summary
ICY connection and transport wiring
internal/httpclient/icy.go, internal/httpclient/client.go
icyConn rewrites the initial ICY prefix to HTTP/1.0 . newStreamingTransport() applies the wrapper to TCP and TLS connections and configures TLS for HTTP/1.x.
ICY HTTP and HTTPS validation
internal/httpclient/client_test.go
Tests send hijacked ICY responses over HTTP and HTTPS and verify the "hello" body and HTTP/1.0 protocol.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Sequence Diagram(s)

sequenceDiagram
  participant StreamingClient
  participant newStreamingTransport
  participant icyConn
  participant RadioStream
  StreamingClient->>newStreamingTransport: Perform GET request
  newStreamingTransport->>RadioStream: Dial TCP or TLS connection
  newStreamingTransport->>icyConn: Wrap established connection
  RadioStream-->>icyConn: Return ICY status and stream data
  icyConn-->>StreamingClient: Return HTTP/1.0 status and stream data
Loading
🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title is concise and accurately summarizes the main change: accepting ICY stream responses.
Linked Issues check ✅ Passed The changes normalize ICY responses and add HTTP/HTTPS coverage, matching the linked issue's stream playback failures.
Out of Scope Changes check ✅ Passed The transport and TLS adjustments are directly tied to handling ICY streams and HTTPS playback, with no clear unrelated additions.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Actionable comments posted: 1

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

Inline comments:
In `@internal/httpclient/client.go`:
- Around line 29-55: Update the transport setup error paths in the DialContext
and DialTLSContext callbacks: wrap TCP dial failures, net.SplitHostPort
failures, and TLS dial failures with contextual fmt.Errorf messages using %w,
while preserving the existing return behavior and successful connection flow.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 36a62696-fc28-4998-9c9d-f79d6ffc4a97

📥 Commits

Reviewing files that changed from the base of the PR and between 7404285 and a32d3a3.

📒 Files selected for processing (3)
  • internal/httpclient/client.go
  • internal/httpclient/client_test.go
  • internal/httpclient/icy.go

Comment thread internal/httpclient/client.go
@bjarneo
bjarneo merged commit 40fb54e into main Jul 25, 2026
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Issues with playing some radiostations

1 participant