From d050e48af64ef2bac0dfc0882375e884a207db5a Mon Sep 17 00:00:00 2001 From: debont80 Date: Tue, 30 Jun 2026 20:54:09 -0400 Subject: [PATCH] H2 (URL half): one canonical youtubeId in urlHelpers MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Consolidates the scattered YouTube-URL parsing (the clean, renderer-contained half of H2): - Move the complete youtubeId (watch/shorts/embed/live/youtu.be) into lib/urlHelpers.ts as the single home. - thumb.ts, queueStats.ts (sameVideo), and store/downloads.ts (titleFromUrl) now import it; the three ad-hoc reimplementations are removed. - titleFromUrl gains correctness: it previously labeled ANY host with a ?v= param as "YouTube video (…)" and missed youtu.be/shorts; it's now host-correct and covers every YouTube shape. sameVideo now also dedupes /shorts/ID against /watch?v=ID. - Unit-tested in test/clipboardLink.test.ts (+4 cases, 247 pass). The formatter half of H2 is deferred to travel with H4: the renderer re-parses speed/eta strings only because raw numbers aren't carried across the IPC boundary (H4), and fmtBytes (1024) vs formatSpeed (1000) differ intentionally. typecheck + 247 tests + eslint + prettier green. Co-Authored-By: Claude Opus 4.8 --- CODE-AUDIT.md | 10 ++++++++- src/renderer/src/lib/urlHelpers.ts | 26 +++++++++++++++++++++++ src/renderer/src/store/downloads.ts | 9 ++++---- src/renderer/src/store/queueStats.ts | 13 +----------- src/renderer/src/thumb.ts | 25 +--------------------- test/clipboardLink.test.ts | 31 +++++++++++++++++++++++++++- 6 files changed, 72 insertions(+), 42 deletions(-) diff --git a/CODE-AUDIT.md b/CODE-AUDIT.md index dca9877..433dd43 100644 --- a/CODE-AUDIT.md +++ b/CODE-AUDIT.md @@ -224,7 +224,15 @@ token system + shared primitives (UI/SIMP — high value, larger effort) · i18n - [ ] **H1 — Decompose god files.** `SettingsView.tsx` (1104, ~11 cards, ~30 selector subs + ~20 useState), `DownloadBar.tsx` (858, ~17 state hooks), `store/downloads.ts` (614). - [ ] **H2 — Consolidate scattered utilities.** `youtubeId` ×2; 4 YouTube-URL-parsing variants; - byte/speed/eta/duration formatting across 4 modules. + byte/speed/eta/duration formatting across 4 modules. *URL half done: one canonical `youtubeId` now + lives in [urlHelpers.ts](src/renderer/src/lib/urlHelpers.ts) (the complete watch/shorts/embed/live/ + youtu.be parse); `thumb.ts`, `queueStats.ts` (`sameVideo`), and `store/downloads.ts` (`titleFromUrl`, + which also fixes an old bug where any host with a `?v=` param was mislabeled "YouTube video") all import + it — the three ad-hoc reimplementations are gone. Unit-tested in `test/clipboardLink.test.ts` (247 pass). + **Formatter half still open** and intentionally deferred to travel with **H4**: the renderer only + re-parses `speed`/`eta` strings back to numbers because raw numbers aren't carried across the IPC + boundary (that's H4), and `fmtBytes` (1024) vs `formatSpeed` (1000) differ on purpose — so a real + merge means solving H4 first, not just extracting a shared module.* - [x] **H3 — Fix `probe.ts → download.ts` dependency direction** (`fmtBytes` import); resolves with H2's shared formatter. - [ ] **H4 — Carry raw numbers across the progress boundary.** `DownloadProgress` ships diff --git a/src/renderer/src/lib/urlHelpers.ts b/src/renderer/src/lib/urlHelpers.ts index 16af175..b62da70 100644 --- a/src/renderer/src/lib/urlHelpers.ts +++ b/src/renderer/src/lib/urlHelpers.ts @@ -1,3 +1,29 @@ +/** + * Pull the video id out of any YouTube watch / shorts / embed / live / youtu.be + * URL. Returns null for non-YouTube URLs or anything unparseable, so callers can + * cheaply ask "is there a YouTube id derivable from this link?". The single home + * for this parse — thumbnails, duplicate detection, and the placeholder title all + * use it (H2). + */ +export function youtubeId(url: string | undefined): string | null { + if (!url) return null + let u: URL + try { + u = new URL(url) + } catch { + return null + } + const host = u.hostname.replace(/^www\./i, '').toLowerCase() + if (host === 'youtu.be') return u.pathname.slice(1).split('/')[0] || null + if (host === 'youtube.com' || host.endsWith('.youtube.com')) { + const v = u.searchParams.get('v') + if (v) return v + const m = u.pathname.match(/^\/(?:embed|shorts|v|live)\/([^/?#]+)/i) + if (m?.[1]) return m[1] + } + return null +} + /** A quick heuristic for "this clipboard text is a link worth offering". */ export function looksLikeUrl(text: string): boolean { const t = text.trim() diff --git a/src/renderer/src/store/downloads.ts b/src/renderer/src/store/downloads.ts index 6da1ce9..cb64380 100644 --- a/src/renderer/src/store/downloads.ts +++ b/src/renderer/src/store/downloads.ts @@ -11,6 +11,7 @@ import { useSettings } from './settings' import { useHistory } from './history' import { emit, on } from './coordinator' import { newId } from '../id' +import { youtubeId } from '../lib/urlHelpers' // Single-sourced from the IPC contract (L105) and re-exported so existing // `import { MediaKind } from '../store/downloads'` sites keep working. @@ -174,11 +175,11 @@ function buildItem(url: string, kind: MediaKind, quality: string, opts?: AddOpti } function titleFromUrl(url: string): string { + const id = youtubeId(url) + if (id) return `YouTube video (${id})` try { - const u = new URL(url) - const v = u.searchParams.get('v') - if (v) return `YouTube video (${v})` - if (u.hostname) return `${u.hostname} download` + const host = new URL(url).hostname + if (host) return `${host} download` } catch { /* not a URL */ } diff --git a/src/renderer/src/store/queueStats.ts b/src/renderer/src/store/queueStats.ts index 466f5e1..f317e7e 100644 --- a/src/renderer/src/store/queueStats.ts +++ b/src/renderer/src/store/queueStats.ts @@ -5,6 +5,7 @@ * unit-tested without constructing the Zustand store or its preview ticker. */ import type { DownloadItem } from './downloads' +import { youtubeId } from '../lib/urlHelpers' // Parse a human speed string ('4.7 MB/s', '512 KiB/s') into bytes/second. Tolerant // of the SI (KB) and binary (KiB) suffixes yt-dlp may emit; the 1000-vs-1024 @@ -107,18 +108,6 @@ export function summarizeQueue(items: DownloadItem[]): QueueSummary { // --- Duplicate detection ---------------------------------------------------- -function youtubeId(url: string): string | null { - try { - const u = new URL(url) - const host = u.hostname.replace(/^www\./, '') - if (host === 'youtu.be') return u.pathname.slice(1) || null - if (host.endsWith('youtube.com')) return u.searchParams.get('v') - } catch { - /* not a URL */ - } - return null -} - function normalizeUrl(url: string): string { try { const u = new URL(url) diff --git a/src/renderer/src/thumb.ts b/src/renderer/src/thumb.ts index 0fe52dd..34ab3b8 100644 --- a/src/renderer/src/thumb.ts +++ b/src/renderer/src/thumb.ts @@ -4,30 +4,7 @@ * video id with no extra network probe. Anything non-YouTube without a probed * thumbnail returns undefined, and the UI falls back to a kind icon. */ - -/** - * Pull the video id out of any YouTube watch / shorts / embed / live / youtu.be - * URL. Returns null for non-YouTube URLs or anything unparseable, so callers can - * cheaply ask "is there a thumbnail derivable from this link?". - */ -export function youtubeId(url: string | undefined): string | null { - if (!url) return null - let u: URL - try { - u = new URL(url) - } catch { - return null - } - const host = u.hostname.replace(/^www\./i, '').toLowerCase() - if (host === 'youtu.be') return u.pathname.slice(1).split('/')[0] || null - if (host === 'youtube.com' || host.endsWith('.youtube.com')) { - const v = u.searchParams.get('v') - if (v) return v - const m = u.pathname.match(/^\/(?:embed|shorts|v|live)\/([^/?#]+)/i) - if (m?.[1]) return m[1] - } - return null -} +import { youtubeId } from './lib/urlHelpers' /** * Resolve a preview thumbnail URL for a media item. Prefers an explicit thumbnail diff --git a/test/clipboardLink.test.ts b/test/clipboardLink.test.ts index 70fde08..5effde1 100644 --- a/test/clipboardLink.test.ts +++ b/test/clipboardLink.test.ts @@ -1,6 +1,35 @@ import { describe, it, expect } from 'vitest' // Import from the pure utility module directly — no Zustand store init needed (L40). -import { looksLikeUrl, looksLikeSingleVideo } from '../src/renderer/src/lib/urlHelpers' +import { looksLikeUrl, looksLikeSingleVideo, youtubeId } from '../src/renderer/src/lib/urlHelpers' + +describe('youtubeId', () => { + it('extracts the id from a watch URL', () => { + expect(youtubeId('https://www.youtube.com/watch?v=dQw4w9WgXcQ')).toBe('dQw4w9WgXcQ') + expect(youtubeId('https://youtube.com/watch?v=abc123&list=xyz')).toBe('abc123') + expect(youtubeId('https://music.youtube.com/watch?v=musicId')).toBe('musicId') + }) + + it('extracts the id from a youtu.be short link', () => { + expect(youtubeId('https://youtu.be/dQw4w9WgXcQ')).toBe('dQw4w9WgXcQ') + expect(youtubeId('https://youtu.be/abc123?t=42')).toBe('abc123') + }) + + it('extracts the id from shorts / embed / live / v paths', () => { + expect(youtubeId('https://www.youtube.com/shorts/shortId1')).toBe('shortId1') + expect(youtubeId('https://youtube.com/embed/embedId2')).toBe('embedId2') + expect(youtubeId('https://www.youtube.com/live/liveId3')).toBe('liveId3') + expect(youtubeId('https://youtube.com/v/vId4?fs=1')).toBe('vId4') + }) + + it('returns null for non-YouTube, unparseable, or empty input', () => { + expect(youtubeId('https://vimeo.com/12345')).toBeNull() + expect(youtubeId('https://notyoutube.com/watch?v=x')).toBeNull() + expect(youtubeId('https://www.youtube.com/@somechannel')).toBeNull() + expect(youtubeId('not a url')).toBeNull() + expect(youtubeId(undefined)).toBeNull() + expect(youtubeId('')).toBeNull() + }) +}) describe('looksLikeUrl', () => { it('accepts http(s) URLs', () => {