Repository navigation
Conversation
|
Please describe the context of the repair in the pull request; the "fix bandcamp" section is too generic to understand. |
|
copy pasted from the previous pull request: i noticed while im doing playlist test runs, bandcamp parsing (tracks, albums, playlist, possibly artist too) are broken (im hitting the following errors) this following pull request fixes it, toddy sent me a fix to improve on discord dms and i steered claude to fix the challenge because for some reason, node js fetch returns the following response on the last two commits are the follow ups of playlist author + thumbnail |
|
I'll check it out, but I noticed there's a workaround from one source to another, which isn't good. |
1Lucas1apk
left a comment
There was a problem hiding this comment.
Thanks for looking into this. The playlist implementation and the lowercase header normalization in makeRequest make sense, though while testing this locally I noticed a few architectural and edge-case issues across the changed files. Left some comments inline with the details.
| /** | ||
| * Browser-like User-Agent sent by default with HTTP requests | ||
| * | ||
| * Some platforms (e.g. Bandcamp) serve canonical redirects or challenge | ||
| * pages to requests identified as bots or unknown clients. Sending a | ||
| * browser-like User-Agent avoids those redirects and matches what | ||
| * `http1makeRequest` already sends by default. | ||
| * | ||
| * @public | ||
| */ | ||
| export const BROWSER_USER_AGENT = | ||
| 'Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/125.0.0.0 Safari/537.36' |
There was a problem hiding this comment.
Could we keep this User-Agent constant scoped to bandcamp.ts (and leave the internal default inside utils.ts) instead of exporting BROWSER_USER_AGENT from constants.ts?
We keep constants.ts focused on core audio, protocol, and gateway constants, while all other sources define their own User-Agent locally when a platform requires a specific header. Exporting a hardcoded Chrome string as a @public constant here doesn't really fit the structure of the project.
| TrackEncodeInput | ||
| } from '../typings/utils.types.ts' | ||
| import { encodeTrack, getVersion, http1makeRequest, logger } from '../utils.ts' | ||
| import { BANDCAMP_TRACK_PATTERN } from './bandcamp.ts' |
There was a problem hiding this comment.
I don't think http.ts should import BANDCAMP_TRACK_PATTERN from bandcamp.ts. Sources in NodeLink are meant to stay isolated from each other and are loaded dynamically based on the config, so importing bandcamp.ts directly here couples the generic HTTP source to Bandcamp and loads the module even when Bandcamp is disabled.
| // Bandcamp custom domains (and similar CDNs) redirect-loop | ||
| // non-browser user agents. Only force the NodeLink UA when it is | ||
| // explicitly configured, letting the helper's browser default | ||
| // apply otherwise. | ||
| const getData = await http1makeRequest(url, { | ||
| method: 'GET', | ||
| streamOnly: true, | ||
| headers: requestHeaders | ||
| headers: this.config.userAgent ? requestHeaders : {} | ||
| }) |
There was a problem hiding this comment.
Changing the fallback GET headers like this introduces a couple of issues. First, the initial HEAD request above still sends DEFAULT_HTTP_USER_AGENT, so custom Bandcamp domains like music.businesscasual.biz still get caught in the 5-redirect loop between the custom domain (303) and bandcamp.com (301), wasting around 3 seconds and logging a warning before falling back to GET.
Second, dropping requestHeaders on the fallback GET switches the User-Agent to a Mozilla/ browser string for every HTTP URL where HEAD fails. That breaks SHOUTcast streams (since they reject HEAD and return an HTML status page instead of the audio stream when they see Mozilla/), makes resolve() probe with a different User-Agent than loadStream() uses, and still fails on Bandcamp custom domains whenever sources.http.userAgent is configured.
There was a problem hiding this comment.
here's what claude's proposal. 2 seems like a terrible config bloat, 1 would just drop the custom domains which sucks
Custom domains need a decision
The reviewer’s two points describe a real conflict. The HTTP source can’t tell a Bandcamp custom domain from any other URL, and Bandcamp only serves one to a browser-like User-Agent. Forcing that User-Agent for everything breaks SHOUTcast, and not forcing it breaks custom domains. So a fix inside http.ts can’t satisfy both. The options I can see:
-
Drop custom-domain support from this PR (what this change does now). It’s the simplest and fits “sources stay isolated”, but those domains will no longer work.
-
Let the Bandcamp source claim custom domains through config, for example a
sources.bandcamp.customDomainslist. Its URL patterns would be built from that list, so the manager routes straight to Bandcamp. That avoids the redirect loop, the 3 seconds and the warning, works whateverhttp.userAgentis, and keepshttp.tsuntouched. The cost is a new config option, so users must list their domains. I haven’t built this, and it’s a design call for you and the maintainer. -
Error-driven retry in
http.ts: after a redirect-loop failure, retry once with a browser UA. It works without config, but it adds a Chrome UA string tohttp.tsand keeps the 3-second delay and the warning. I’d avoid it, given the reviewer’s earlier feedback.
There was a problem hiding this comment.
If this PR is just unreviewed AI back-and-forth, please close it.
Manually configuring custom domains in config files is completely out of the question, and dropping existing features is not a valid fix. We need solutions tested and understood by humans, not raw LLM outputs.
| // Bandcamp pages are HTML, which this source cannot play. Custom | ||
| // domains resolve through redirects to a `*.bandcamp.com` URL, | ||
| // so hand the final URL over to the Bandcamp source when loaded. | ||
| const sources = this.nodelink.sources | ||
| const finalUrl = (data as HttpRequestResult).finalUrl | ||
|
|
||
| if ( | ||
| sources?.sources.has('bandcamp') && | ||
| finalUrl && | ||
| BANDCAMP_TRACK_PATTERN.test(finalUrl) | ||
| ) { | ||
| logger( | ||
| 'debug', | ||
| 'HTTP Source', | ||
| `Delegating Bandcamp page to Bandcamp source: ${finalUrl}` | ||
| ) | ||
| return sources.resolve(finalUrl) | ||
| } |
There was a problem hiding this comment.
Following up on the import above, I don't think HttpSource should have Bandcamp-specific delegation logic hardcoded here (which also skips Bandcamp playlist URLs and double-counts metrics/hooks for both http and bandcamp).
If we want to support custom domains or shortlinks that redirect into a supported platform, we should handle that generically through SourcesManager and patternMap when finalUrl differs from the original URL instead of coupling http.ts to Bandcamp.
| message: result.message.replace( | ||
| 'Failed to fetch BandCamp page', | ||
| 'Failed to fetch track page' | ||
| ), |
There was a problem hiding this comment.
Rewriting the error message with .replace('Failed to fetch BandCamp page', 'Failed to fetch track page') is a bit fragile since it relies on the exact wording returned by fetchPageHtml. If fetchPageHtml just returns the underlying error or status code (or once we simplify the helper below), getTrackUrl can format its own message directly without string replacement.
| isStream: false, | ||
| title: this.normalizeText(item.title), | ||
| uri: item.url, | ||
| artworkUrl: this.createArtworkUrl(item.artId) |
There was a problem hiding this comment.
In Bandcamp's playlist payload, tracks can inherit their cover from the album when a track-specific artId isn't set. Falling back to item.artId ?? item.album?.artId (with album?: { artId?: number | string | null } | null typed on BandcampPlaylistTrack) would avoid missing track artwork here.
| type: 'playlist', | ||
| identifier: String(firstPage.itemId), | ||
| url: firstPage.itemUrl ?? url, | ||
| artworkUrl: this.createPlaylistArtworkUrl(firstPage.imageId), |
There was a problem hiding this comment.
firstPage.itemId is typed as optional in BandcampPlaylistTracklist, so calling String(firstPage.itemId) directly can turn identifier into the literal string "undefined" if itemId is missing. We should check firstPage.itemId != null before converting it.
| private async fetchPageHtml(url: string): Promise<BandcampPageResult> { | ||
| let lastFailure: BandcampPageResult = { | ||
| ok: false, | ||
| reason: 'http', | ||
| message: 'BandCamp page request was not attempted.' | ||
| } | ||
|
|
||
| for (let attempt = 1; attempt <= PAGE_FETCH_MAX_ATTEMPTS; attempt++) { | ||
| const { body, error, statusCode, headers } = await makeRequest(url, { | ||
| method: 'GET', | ||
| headers: { 'User-Agent': BROWSER_USER_AGENT } | ||
| }) | ||
|
|
||
| let retryable = false | ||
|
|
||
| if (error || statusCode !== 200) { | ||
| logger( | ||
| 'error', | ||
| 'Sources', | ||
| `Failed to fetch BandCamp page: ${error ?? statusCode}` | ||
| ) | ||
| lastFailure = { | ||
| ok: false, | ||
| reason: 'http', | ||
| message: `Failed to fetch BandCamp page: ${error ?? statusCode}`, | ||
| statusCode | ||
| } | ||
| retryable = | ||
| !error && | ||
| statusCode !== undefined && | ||
| (statusCode === 429 || statusCode >= 500) | ||
| } else { | ||
| const page = this.getResponseText({ body }) | ||
|
|
||
| if (page === null) { | ||
| return { | ||
| ok: false, | ||
| reason: 'unreadable', | ||
| message: 'BandCamp returned an unreadable page.', | ||
| statusCode | ||
| } | ||
| } | ||
|
|
||
| if (CHALLENGE_PAGE_MARKERS.some((marker) => page.includes(marker))) { | ||
| const servedBy = headers?.['x-served-by'] | ||
| logger( | ||
| 'warn', | ||
| 'Sources', | ||
| `BandCamp served a bot challenge for ${url} (attempt ${attempt}/${PAGE_FETCH_MAX_ATTEMPTS}, x-served-by: ${Array.isArray(servedBy) ? servedBy.join(', ') : (servedBy ?? 'unknown')}).` | ||
| ) | ||
| lastFailure = { | ||
| ok: false, | ||
| reason: 'challenge', | ||
| message: 'BandCamp served a bot challenge page.', | ||
| statusCode, | ||
| page | ||
| } | ||
| retryable = true | ||
| } else { | ||
| return { ok: true, page } | ||
| } | ||
| } | ||
|
|
||
| if (!retryable || attempt === PAGE_FETCH_MAX_ATTEMPTS) break | ||
|
|
||
| await new Promise((resolve) => | ||
| setTimeout(resolve, PAGE_FETCH_RETRY_DELAY_MS * attempt) | ||
| ) | ||
| } | ||
|
|
||
| return lastFailure | ||
| } |
There was a problem hiding this comment.
I tested HTTP/1.1 vs HTTP/2 locally against Bandcamp and found out why Node was hitting <title>Client Challenge</title> while Bun wasn't. Previously, passing 'User-Agent' into makeRequest collided with the lowercase 'user-agent' key in h2Headers, causing Node's http2 module to throw ERR_HTTP2_HEADER_SINGLE_VALUE, mark the host as failed, and silently fall back to http1makeRequest over HTTP/1.1 (whereas Bun's fetch path used HTTP/2 without duplicate headers). Bandcamp's Fastly setup deterministically serves that JS challenge whenever a browser User-Agent connects over HTTP/1.1, while HTTP/2 requests go straight through with 200 OK.
Since normalizing customHeaders to lowercase in utils.ts already keeps the request on HTTP/2, the challenge no longer triggers. That also means retrying with linear backoff here won't actually bypass a JS challenge if a request is ever forced onto HTTP/1.1 (for example when a proxy is configured); Fastly will serve the exact same challenge on all three attempts and just stall resolution for 3 seconds before failing.
I think we can simplify this quite a bit by removing the retry loop and BandcampPageResult / challenged plumbing, and just checking the response once so we return a clear error right away if the challenge markers appear.
| /** | ||
| * Builds the cover URL of a playlist from its `imageId`. Playlist images are | ||
| * addressed without the `a` prefix used for release art, and are served as | ||
| * 500px-wide 4:3 WebP. | ||
| * | ||
| * @param imageId Playlist image ID from the page data. | ||
| * @returns The cover URL, or `null` when the playlist has no image. | ||
| */ | ||
| private createPlaylistArtworkUrl( | ||
| imageId?: string | number | null | ||
| ): string | null { | ||
| if (imageId === undefined || imageId === null || imageId === '') { | ||
| return null | ||
| } | ||
|
|
||
| return `https://f4.bcbits.com/img/${String(imageId)}_4x3t_500.webp` | ||
| } |
There was a problem hiding this comment.
There is a bug here when a playlist doesn't have a custom cover uploaded and uses album art instead. In Bandcamp's playlist tracklist payload, there is an isArt boolean alongside imageId. When isArt is false, https://f4.bcbits.com/img/${imageId}_4x3t_500.webp works, but when isArt is true, imageId is actually an album artId and needs the a prefix (https://f4.bcbits.com/img/a${imageId}_10.jpg), otherwise f4.bcbits.com returns HTTP 404. Checking isArt so it delegates to createArtworkUrl(imageId) when true should fix that.
Also, createPlaylistArtworkUrl was inserted right between createArtworkUrl and its JSDoc block on lines 1078–1083, leaving two docblocks stacked together here and leaving createArtworkUrl below without its comment.
| } | ||
| } | ||
| } | ||
| } No newline at end of file |
There was a problem hiding this comment.
This file is missing a trailing newline at the end, which makes biome check fail. Also remember to run npm run build after making the updates so dist stays in sync.
i really need to make bandcamp custom domains work
1Lucas1apk
left a comment
There was a problem hiding this comment.
Reverting the direct import of bandcamp.ts is an improvement, but implementing findOtherSourceFor inside http.ts still violates source isolation and turns the HTTP stream provider into a secondary router.
Furthermore, custom Bandcamp domains do not redirect to *.bandcamp.com, so findOtherSourceFor returns null and the issue remains unresolved.
Please remove findOtherSourceFor and the redirect delegation from http.ts. If custom domains are to be supported, handle their URL pattern matching where it belongs: inside bandcamp.ts and SourcesManager.
| * | ||
| * @param url URL to look up. | ||
| * @returns The matching source name, or `null` when no other source claims it. | ||
| */ |
There was a problem hiding this comment.
This still breaks source isolation and project responsibilities. http.ts is an audio stream provider, not a source router. It shouldn't scan loaded sources or re-implement pattern matching, as that is solely the responsibility of SourcesManager through its patternMap.
Besides, this logic doesn't work for Bandcamp custom domains. Custom domains like music.businesscasual.biz don't redirect to *.bandcamp.com, so finalUrl !== url evaluates to false. Even if it did evaluate, BANDCAMP_TRACK_PATTERN strictly matches *.bandcamp.com, which means findOtherSourceFor returns null every time and the request still fails with an unsupported content-type error. This entire helper should be removed from http.ts.
|
|
||
| if (finalUrl && owner) { | ||
| logger( | ||
| 'debug', |
There was a problem hiding this comment.
Routing decisions shouldn't be handled here. If a URL returns text/html, http.ts simply cannot play it and should return the unsupported media error.
Bandcamp custom domains fail with redirect loops when requested with non-browser headers, but trying to catch and delegate that from inside http.ts only introduces cross-source workarounds that don't actually solve the custom domain issue. If Bandcamp custom domains are meant to be supported, that pattern matching belongs directly in bandcamp.ts.
| * clients it does not recognize as a browser. | ||
| */ | ||
| const USER_AGENT = | ||
| 'Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/125.0.0.0 Safari/537.36' |
There was a problem hiding this comment.
Moving this out of constants.ts makes sense, but now the full Chrome User-Agent string is duplicated as a raw literal in both src/utils.ts and src/sources/bandcamp.ts. We should keep shared internal defaults centralized in utils.ts rather than hardcoding identical strings in multiple files.
There was a problem hiding this comment.
want me to audit all the default user agent into a one global variable?
There was a problem hiding this comment.
This value is already a standard request value; there's no need to define it again twice in the project when it's already in the utils file. Please read the code; don't rely on a tool without understanding what's happening.
https://github.com/PerformanC/NodeLink/blob/dev/src%2Futils.ts#L1439
|
i dropped bandcamp custom domains for now but i'll bring it back once i come up with a solution, do not merge for now |
|
Until all requests are sorted, the process will not be merged. |
|
The correction has some good parts, but a lot of inconsistency, and it doesn't follow the standards we use in the codebase; each source is responsible for resolving its own information. The pull request has created 3 unnecessary duplications, redundancies, and complexity. Among other issues, whoever is doing this seems to be an AI agent that is quite hallucinating. |
No description provided.