refactor: one canonical YouTube URL rule in server/lib/youtubeUrl.js (#6014) - #6122
Merged
Conversation
…6014) Music Video track import rejected `music.youtube.com`, `/shorts/`, `/live/`, and `/embed/` links with `400 YOUTUBE_URL_INVALID` even though yt-dlp handles them and both the brain ingest and the Google Takeout importer already accepted them. The cause was drift: the same "is this one YouTube video, and which one?" question was answered by four separate copies of the regexes — an older one in `trackYoutubeImport.js` (duplicated again in `routes/tracks.js`) that predated `music.`/shorts/live/embed support, a newer one in `youtubeIngest.js`, the id parser buried in `youtubeImport.js`, and a fifth mirror on the client. Extract the rule to `server/lib/youtubeUrl.js` (`YOUTUBE_VIDEO_URL_RE`, `youtubeVideoIdFromUrl` / `youtubeVideoId`, `isYoutubeVideoUrl`, `assertYoutubeVideoUrl`, `YOUTUBE_URL_INVALID_MESSAGE`) and point every server caller at it. The existing service-level names (`YOUTUBE_INGEST_URL_RE`, `assertYoutubeIngestUrl`, `YOUTUBE_URL_RE`, `assertYoutubeUrl`) stay as aliases, so nothing downstream had to change. This also removes the layering violation the duplication forced: the id parser lived in a service, so `server/lib/youtubeUrl.mirror.test.js` had to import from `server/services/` — libraries reaching into services. Track import now accepts every URL shape the other two pipelines do, and the rejection message names all of them. Claude-Session: https://claude.ai/code/session_01VjkWVTfzKyRuAv3HEsspwN
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Pasting a
music.youtube.com,/shorts/,/live/, or/embed/link into Music Video track import returned400 YOUTUBE_URL_INVALID("Not a recognized YouTube URL") — even though yt-dlp handles all of them and both the brain YouTube ingest and the Google Takeout importer already accepted them.The cause was copy-paste drift. The same question — is this one YouTube video, and which one? — was answered by four separate copies of the regexes:
services/trackYoutubeImport.js, duplicated verbatim inroutes/tracks.js, that predatedmusic./ shorts / live / embed support;services/youtubeIngest.js(YOUTUBE_INGEST_URL_RE);services/youtubeImport.js(the Takeout ZIP importer);client/src/lib/youtubeUrl.js.Because the id parser lived in a service,
server/lib/youtubeUrl.mirror.test.jshad to import fromserver/services/— a library reaching into services, inverting the dependency rule.This PR extracts the rule to
server/lib/youtubeUrl.js:YOUTUBE_VIDEO_URL_REwatch/shorts/live/embedacross thewww./m./music.hosts; still rejects playlists, channels, and/@handlefeeds so a batch paste can't kick off a 300-video download.youtubeVideoIdFromUrl(url)(aliasyoutubeVideoId)null.isYoutubeVideoUrl(url)assertYoutubeVideoUrl(url)400 YOUTUBE_URL_INVALID.YOUTUBE_URL_INVALID_MESSAGEEvery server caller now imports from there. The established service-level names (
YOUTUBE_INGEST_URL_RE,assertYoutubeIngestUrl,YOUTUBE_URL_RE,assertYoutubeUrl) remain as re-export aliases, so no downstream import had to change. Net effect for users: track import accepts every URL shape the other two pipelines do, and the rejection message names all of them.Behavior-neutral for the brain ingest and the Takeout importer — the regex and the id parser they get are byte-identical to what they declared before.
Files
server/lib/youtubeUrl.js, re-exported fromserver/lib/index.jsand catalogued inserver/lib/README.md.services/trackYoutubeImport.js,routes/tracks.js— drop the drifted regex, import the canonical one (this is the user-visible fix).ServerErrorwas left unused intrackYoutubeImport.jsand removed.services/youtubeIngest.js,services/youtubeImport.js,services/youtubeSync.js— import the shared rule/parser.server/lib/youtubeUrl.mirror.test.js— now comparesserver/lib/youtubeUrl.jsagainst the client mirror; noserver/services/imports remain.client/src/lib/youtubeUrl.js— doc comment only, repointed at the new authoritative module.Test plan
server/lib/youtubeUrl.mirror.test.js— unchanged case matrix, still asserts server↔client parity on both the predicate and the extracted id; now with the illegal cross-layer import gone.server/services/trackYoutubeImport.test.js— new regression cases assertingmusic.youtube.com,youtube.com/shorts,/live/, and/embed/are accepted (each 400'd before this change), thatassertYoutubeUrlhands back the video id, and that playlists / channels / feeds are still refused with/single-video YouTube URL/.server/routes/tracks.test.js— the route-level counterpart: thetrackYoutubeImportmock no longer stubsYOUTUBE_URL_RE(the route reads the real rule fromlib/), andPOST /api/tracks/import/youtubeis asserted to return202and reach the service for all four previously-rejected shapes.cd server && npm test).src/libsuite: 136 files / 2133 tests passed.Closes #6014
https://claude.ai/code/session_01VjkWVTfzKyRuAv3HEsspwN