From 3d09174c1645b9008c70e6f3dbae4a2c0a27eb70 Mon Sep 17 00:00:00 2001 From: debont80 Date: Tue, 30 Jun 2026 21:03:14 -0400 Subject: [PATCH] H4 + H2 formatter half: carry raw numbers across the progress boundary Closes H4 and the remaining formatter half of H2. - New @shared/format.ts is the single home for fmtBytes/fmtSpeed/fmtEta, imported by both main and renderer. main/lib/formatters.ts re-exports them. - DownloadProgress now carries raw speedBytesPerSec/etaSeconds instead of pre-formatted speed/eta strings. main's parseProgress emits the raw numbers. - The renderer stores the raw numbers on DownloadItem; QueueItem formats them for per-item display, and summarizeQueue sums/maxes them directly. The lossy string->number->string round-trip in queueStats (parseSpeed / parseEtaSeconds / a local formatSpeed / formatEta) is deleted, along with a duplicate fmtEta in store/downloads.ts. - Per-item and aggregate speed now use the same 1024-based scale; the aggregate previously used a 1000-based formatter (a latent inconsistency). Tests updated for the raw-number contract (parseProgress, summarizeQueue). typecheck + 248 tests + eslint + prettier green. Co-Authored-By: Claude Opus 4.8 --- CODE-AUDIT.md | 30 +++++++----- src/main/lib/formatters.ts | 42 +++++------------ src/renderer/src/components/QueueItem.tsx | 10 +++- src/renderer/src/store/downloads.ts | 47 ++++++++----------- src/renderer/src/store/queueStats.ts | 56 ++++------------------- src/shared/format.ts | 34 ++++++++++++++ src/shared/ipc.ts | 8 ++-- test/download.test.ts | 12 +++-- test/queueStats.test.ts | 18 +++++--- 9 files changed, 122 insertions(+), 135 deletions(-) create mode 100644 src/shared/format.ts diff --git a/CODE-AUDIT.md b/CODE-AUDIT.md index 433dd43..8a60b89 100644 --- a/CODE-AUDIT.md +++ b/CODE-AUDIT.md @@ -223,20 +223,26 @@ 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. *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] **H2 — Consolidate scattered utilities.** `youtubeId` ×2; 4 YouTube-URL-parsing variants; + byte/speed/eta/duration formatting across 4 modules. *URL half: 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. Formatter half (done with H4): `fmtBytes`/`fmtSpeed`/`fmtEta` + now have a single home in [@shared/format.ts](src/shared/format.ts) imported by both sides; the main + formatters module re-exports them and the renderer's three private copies (a byte formatter, an ETA + formatter, and a speed string→number re-parser in `queueStats`) are deleted. Per-item and aggregate + speed now use the same 1024-based scale (the old aggregate used 1000, a latent inconsistency). + Unit-tested in `test/clipboardLink.test.ts` + `test/download.test.ts` + `test/queueStats.test.ts`.* - [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 - formatted `speed`/`eta` strings; `queueStats.ts` re-parses them back to numbers. +- [x] **H4 — Carry raw numbers across the progress boundary.** `DownloadProgress` ships + formatted `speed`/`eta` strings; `queueStats.ts` re-parses them back to numbers. *Fixed: `DownloadProgress` + now carries `speedBytesPerSec`/`etaSeconds` as raw numbers (main's `parseProgress` no longer formats them); + the renderer stores the raw numbers on `DownloadItem`, `QueueItem` formats them for display via + [@shared/format](src/shared/format.ts), and `summarizeQueue` sums/maxes the numbers directly — the lossy + string→number→string round-trip (and `queueStats`' `parseSpeed`/`parseEtaSeconds`) is gone. Done together + with the formatter half of H2.* - [x] **H5 — History re-download silently changes quality.** `HistoryView.redownload` passes the stored `quality` (for format-picker downloads this is a *label* like "720p · mp4 · 184 MB") back as `quality` with no `formatId`; `buildArgs.videoFormat()` has no matching case and falls diff --git a/src/main/lib/formatters.ts b/src/main/lib/formatters.ts index ed69837..1a7277d 100644 --- a/src/main/lib/formatters.ts +++ b/src/main/lib/formatters.ts @@ -1,7 +1,11 @@ import type { DownloadProgress } from '@shared/ipc' +import { fmtBytes } from '@shared/format' -// Pure formatting helpers for yt-dlp progress output. Extracted from download.ts -// so they can be unit-tested without pulling in the electron import chain (L37). +// yt-dlp progress-line parsing. Extracted from download.ts so it can be +// unit-tested without pulling in the electron import chain (L37). The byte/ +// speed/ETA formatters now live in @shared/format (one home for both sides of +// the IPC boundary); re-exported here so existing importers keep working. +export { fmtBytes, fmtSpeed, fmtEta } from '@shared/format' function num(s?: string): number | undefined { if (!s || s === 'NA') return undefined @@ -9,33 +13,6 @@ function num(s?: string): number | undefined { return Number.isFinite(n) ? n : undefined } -export function fmtBytes(bytes: number): string { - if (bytes < 1024) return `${bytes} B` - const units = ['KB', 'MB', 'GB', 'TB'] - let v = bytes / 1024 - let i = 0 - while (v >= 1024 && i < units.length - 1) { - v /= 1024 - i++ - } - return `${v.toFixed(v >= 100 ? 0 : 1)} ${units[i]}` -} - -function fmtSpeed(bytesPerSec?: number): string | undefined { - if (bytesPerSec == null) return undefined - return `${fmtBytes(bytesPerSec)}/s` -} - -export function fmtEta(seconds?: number): string | undefined { - if (seconds == null) return undefined - const s = Math.max(0, Math.round(seconds)) - const h = Math.floor(s / 3600) - const m = Math.floor((s % 3600) / 60) - const r = s % 60 - if (h > 0) return `${h}:${String(m).padStart(2, '0')}:${String(r).padStart(2, '0')}` - return `${m}:${String(r).padStart(2, '0')}` -} - export function parseProgress(rest: string): DownloadProgress | null { const parts = rest.split('|') if (parts.length < 6) return null @@ -47,8 +24,11 @@ export function parseProgress(rest: string): DownloadProgress | null { return { status: status || 'downloading', progress, - speed: fmtSpeed(num(speed)), - eta: fmtEta(num(eta)), + // Carry raw numbers across the IPC boundary; the renderer formats them for + // display AND aggregates them (combined speed / longest ETA) without having + // to re-parse a formatted string (H4). + speedBytesPerSec: num(speed), + etaSeconds: num(eta), sizeLabel: totalBytes ? fmtBytes(totalBytes) : undefined, // No (estimated) total → the % can never advance; flag it so the renderer // shows an indeterminate bar rather than a stuck 0% (L137). diff --git a/src/renderer/src/components/QueueItem.tsx b/src/renderer/src/components/QueueItem.tsx index e5a650b..9acab9b 100644 --- a/src/renderer/src/components/QueueItem.tsx +++ b/src/renderer/src/components/QueueItem.tsx @@ -28,6 +28,7 @@ import { import { useDownloads, type DownloadItem } from '../store/downloads' import { thumbUrl } from '../thumb' import { fmtSchedule } from '../datetime' +import { fmtSpeed, fmtEta } from '@shared/format' import { MediaThumb } from './MediaThumb' import { Hint } from './Hint' import { StatusChip } from './ui/StatusChip' @@ -132,6 +133,11 @@ export const QueueItem = memo(function QueueItem({ } } + // Format the raw progress numbers for display (H4: main sends bytes/sec + secs, + // the renderer owns formatting via the one shared formatter). + const speedLabel = fmtSpeed(item.speedBytesPerSec) + const etaLabel = fmtEta(item.etaSeconds) + // L55: a probed-format quality label already carries the size ("720p · mp4 · // 184 MB"); skip sizeLabel when it's already embedded to avoid showing it twice. const sizeAlreadyInQuality = item.sizeLabel && item.quality.includes(item.sizeLabel) @@ -194,8 +200,8 @@ export const QueueItem = memo(function QueueItem({ ) : ( <> {item.sizeUnknown ? 'Downloading…' : pct(item.progress)} - {item.speed ? ` • ${item.speed}` : ''} - {item.eta ? ` • ${item.eta} left` : ''} + {speedLabel ? ` • ${speedLabel}` : ''} + {etaLabel ? ` • ${etaLabel} left` : ''} )} diff --git a/src/renderer/src/store/downloads.ts b/src/renderer/src/store/downloads.ts index cb64380..abe0eb0 100644 --- a/src/renderer/src/store/downloads.ts +++ b/src/renderer/src/store/downloads.ts @@ -39,8 +39,10 @@ export interface DownloadItem { status: DownloadStatus /** 0..1 */ progress: number - speed?: string - eta?: string + /** raw transfer rate in bytes/sec (formatted for display via @shared/format) */ + speedBytesPerSec?: number + /** raw time remaining in seconds (formatted for display via @shared/format) */ + etaSeconds?: number sizeLabel?: string /** yt-dlp reported no total size -- render the bar indeterminate, not 0% (L137) */ sizeUnknown?: boolean @@ -186,15 +188,6 @@ function titleFromUrl(url: string): string { return 'New download' } -function fmtEta(seconds: number): string { - const s = Math.max(0, Math.round(seconds)) - const h = Math.floor(s / 3600) - const m = Math.floor((s % 3600) / 60) - const r = s % 60 - if (h > 0) return `${h}:${String(m).padStart(2, '0')}:${String(r).padStart(2, '0')}` - return `${m}:${String(r).padStart(2, '0')}` -} - // --- Mock seed data so every visual state is visible in the UI preview ------ const seed: DownloadItem[] = PREVIEW ? [ @@ -208,8 +201,8 @@ const seed: DownloadItem[] = PREVIEW quality: '1080p', status: 'downloading', progress: 0.42, - speed: '4.7 MB/s', - eta: '1:12', + speedBytesPerSec: 4.7 * 1024 * 1024, + etaSeconds: 72, sizeLabel: '512 MB' }, { @@ -301,8 +294,8 @@ function startFakeTicker( channel: 'Sample Channel', durationLabel: '12:34', sizeLabel: '96.4 MB', - speed: done ? undefined : `${(2 + Math.random() * 5).toFixed(1)} MB/s`, - eta: done ? undefined : fmtEta((1 - next) * 90), + speedBytesPerSec: done ? undefined : (2 + Math.random() * 5) * 1024 * 1024, + etaSeconds: done ? undefined : (1 - next) * 90, status: done ? 'completed' : 'downloading', filePath: done ? `C:\\Users\\you\\Downloads\\${cur.title}.mp4` : undefined } @@ -407,8 +400,8 @@ export const useDownloads = create((set, get) => { status: 'queued', progress: 0, error: undefined, - speed: undefined, - eta: undefined + speedBytesPerSec: undefined, + etaSeconds: undefined } : i ) @@ -425,8 +418,8 @@ export const useDownloads = create((set, get) => { status: 'queued', progress: 0, error: undefined, - speed: undefined, - eta: undefined + speedBytesPerSec: undefined, + etaSeconds: undefined } : i ) @@ -443,7 +436,7 @@ export const useDownloads = create((set, get) => { set((s) => ({ items: s.items.map((i) => i.id === id && (i.status === 'downloading' || i.status === 'queued') - ? { ...i, status: 'paused', speed: undefined, eta: undefined } + ? { ...i, status: 'paused', speedBytesPerSec: undefined, etaSeconds: undefined } : i ) })) @@ -519,7 +512,7 @@ export const useDownloads = create((set, get) => { set((s) => ({ items: s.items.map((i) => i.id === id && (i.status === 'downloading' || i.status === 'queued') - ? { ...i, status: 'canceled', speed: undefined, eta: undefined } + ? { ...i, status: 'canceled', speedBytesPerSec: undefined, etaSeconds: undefined } : i ) })) @@ -584,8 +577,8 @@ export const useDownloads = create((set, get) => { // Main owns the latch and resends it on every tick, so reflecting // it directly resets cleanly on a retry's fresh spawn (SR7). finishing: ev.progress.finishing, - speed: ev.progress.speed, - eta: ev.progress.eta, + speedBytesPerSec: ev.progress.speedBytesPerSec, + etaSeconds: ev.progress.etaSeconds, sizeUnknown: ev.progress.sizeUnknown, sizeLabel: ev.progress.sizeLabel ?? i.sizeLabel } @@ -596,8 +589,8 @@ export const useDownloads = create((set, get) => { status: 'completed', progress: 1, finishing: false, - speed: undefined, - eta: undefined, + speedBytesPerSec: undefined, + etaSeconds: undefined, channel: i.channel === RESOLVING ? undefined : i.channel, filePath: ev.filePath ?? i.filePath } @@ -607,8 +600,8 @@ export const useDownloads = create((set, get) => { ...i, status: 'error', finishing: false, - speed: undefined, - eta: undefined, + speedBytesPerSec: undefined, + etaSeconds: undefined, channel: i.channel === RESOLVING ? undefined : i.channel, error: ev.error } diff --git a/src/renderer/src/store/queueStats.ts b/src/renderer/src/store/queueStats.ts index f317e7e..ef0d2ed 100644 --- a/src/renderer/src/store/queueStats.ts +++ b/src/renderer/src/store/queueStats.ts @@ -6,50 +6,7 @@ */ 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 -// difference is immaterial for a live display estimate. -function parseSpeed(s?: string): number { - if (!s) return 0 - const m = /([\d.]+)\s*([KMGT]?)i?B\/s/i.exec(s) - if (!m) return 0 - const n = parseFloat(m[1] ?? '') - if (!isFinite(n)) return 0 - const mult: Record = { '': 1, K: 1e3, M: 1e6, G: 1e9, T: 1e12 } - return n * (mult[(m[2] ?? '').toUpperCase()] ?? 1) -} - -function formatSpeed(bytesPerSec: number): string { - if (bytesPerSec <= 0) return '' - const units = ['B/s', 'KB/s', 'MB/s', 'GB/s'] - let v = bytesPerSec - let i = 0 - while (v >= 1000 && i < units.length - 1) { - v /= 1000 - i++ - } - // Match fmtBytes precision: one decimal below 100, none above (L165). - return `${v.toFixed(v >= 100 ? 0 : 1)} ${units[i]}` -} - -// Parse 'M:SS' / 'H:MM:SS' (or bare seconds) into seconds. -function parseEtaSeconds(s?: string): number { - if (!s) return 0 - const parts = s.split(':').map((p) => Number(p)) - if (parts.length === 0 || parts.some((p) => !isFinite(p))) return 0 - return parts.reduce((acc, p) => acc * 60 + p, 0) -} - -function formatEta(totalSeconds: number): string { - if (totalSeconds <= 0) return '' - const s = Math.round(totalSeconds) - const h = Math.floor(s / 3600) - const m = Math.floor((s % 3600) / 60) - const r = s % 60 - if (h > 0) return `${h}:${String(m).padStart(2, '0')}:${String(r).padStart(2, '0')}` - return `${m}:${String(r).padStart(2, '0')}` -} +import { fmtSpeed, fmtEta } from '@shared/format' export interface QueueSummary { downloading: number @@ -86,8 +43,10 @@ export function summarizeQueue(items: DownloadItem[]): QueueSummary { downloading++ progressSum += i.progress progressCount++ - speed += parseSpeed(i.speed) - maxEta = Math.max(maxEta, parseEtaSeconds(i.eta)) + // Raw numbers carried straight from main (H4) — sum the rates, take the + // longest ETA; no string round-trip. + speed += i.speedBytesPerSec ?? 0 + maxEta = Math.max(maxEta, i.etaSeconds ?? 0) } else if (i.status === 'queued') { queued++ progressCount++ @@ -101,8 +60,9 @@ export function summarizeQueue(items: DownloadItem[]): QueueSummary { failed, active: downloading > 0 || queued > 0, progress: progressCount > 0 ? progressSum / progressCount : 0, - speedLabel: formatSpeed(speed), - etaLabel: formatEta(maxEta) + // Empty label (not '0 B/s' / '0:00') when nothing is actively transferring. + speedLabel: speed > 0 ? (fmtSpeed(speed) ?? '') : '', + etaLabel: maxEta > 0 ? (fmtEta(maxEta) ?? '') : '' } } diff --git a/src/shared/format.ts b/src/shared/format.ts new file mode 100644 index 0000000..52d79f6 --- /dev/null +++ b/src/shared/format.ts @@ -0,0 +1,34 @@ +// Pure display formatters shared by main and renderer. Kept in @shared (not a +// main- or renderer-only lib) so byte/speed/ETA formatting has ONE definition on +// both sides of the IPC boundary — the renderer no longer re-parses formatted +// strings back into numbers to aggregate them (H2/H4). + +/** Format a byte count as a binary-scaled size, e.g. 1048576 → '1.0 MB'. */ +export function fmtBytes(bytes: number): string { + if (bytes < 1024) return `${bytes} B` + const units = ['KB', 'MB', 'GB', 'TB'] + let v = bytes / 1024 + let i = 0 + while (v >= 1024 && i < units.length - 1) { + v /= 1024 + i++ + } + return `${v.toFixed(v >= 100 ? 0 : 1)} ${units[i]}` +} + +/** Format a transfer rate, e.g. 4928307 → '4.7 MB/s'. Undefined for no rate. */ +export function fmtSpeed(bytesPerSec?: number): string | undefined { + if (bytesPerSec == null) return undefined + return `${fmtBytes(bytesPerSec)}/s` +} + +/** Format a duration in seconds as M:SS or H:MM:SS. Undefined for no value. */ +export function fmtEta(seconds?: number): string | undefined { + if (seconds == null) return undefined + const s = Math.max(0, Math.round(seconds)) + const h = Math.floor(s / 3600) + const m = Math.floor((s % 3600) / 60) + const r = s % 60 + if (h > 0) return `${h}:${String(m).padStart(2, '0')}:${String(r).padStart(2, '0')}` + return `${m}:${String(r).padStart(2, '0')}` +} diff --git a/src/shared/ipc.ts b/src/shared/ipc.ts index ce1fd55..00609b7 100644 --- a/src/shared/ipc.ts +++ b/src/shared/ipc.ts @@ -468,10 +468,10 @@ export interface DownloadProgress { status: string /** 0..1 (0 when total size is unknown) */ progress: number - /** human-readable, e.g. '4.7 MB/s' */ - speed?: string - /** human-readable, e.g. '1:12' */ - eta?: string + /** raw transfer rate in bytes/sec; the renderer formats + aggregates it (H4) */ + speedBytesPerSec?: number + /** raw time remaining in seconds; the renderer formats + aggregates it (H4) */ + etaSeconds?: number /** human-readable total size, e.g. '512 MB' */ sizeLabel?: string /** true when yt-dlp reports no total/estimated size (livestreams, some sites), diff --git a/test/download.test.ts b/test/download.test.ts index 20fae33..55b2038 100644 --- a/test/download.test.ts +++ b/test/download.test.ts @@ -66,7 +66,8 @@ describe('parseProgress', () => { const p = parseProgress(raw) expect(p).not.toBeNull() expect(p!.progress).toBeCloseTo(0.5) - expect(p!.eta).toBe('0:02') + expect(p!.speedBytesPerSec).toBe(262144) + expect(p!.etaSeconds).toBe(2) expect(p!.sizeLabel).toBe('1.0 MB') expect(p!.sizeUnknown).toBe(false) }) @@ -84,8 +85,8 @@ describe('parseProgress', () => { const p = parseProgress(raw) expect(p).not.toBeNull() expect(p!.sizeUnknown).toBe(true) - expect(p!.eta).toBeUndefined() - expect(p!.speed).toBeUndefined() + expect(p!.etaSeconds).toBeUndefined() + expect(p!.speedBytesPerSec).toBeUndefined() }) it('clamps progress to 1.0', () => { @@ -94,9 +95,10 @@ describe('parseProgress', () => { expect(p!.progress).toBe(1) }) - it('formats an ETA over 1 hour correctly (L166)', () => { + it('carries a raw ETA in seconds; the renderer formats it (L166)', () => { const raw = makeRaw(['downloading', '100', '1000', '1000', '10', '3661']) const p = parseProgress(raw) - expect(p!.eta).toBe('1:01:01') + expect(p!.etaSeconds).toBe(3661) + expect(fmtEta(p!.etaSeconds)).toBe('1:01:01') }) }) diff --git a/test/queueStats.test.ts b/test/queueStats.test.ts index 7dcf7ff..09a2291 100644 --- a/test/queueStats.test.ts +++ b/test/queueStats.test.ts @@ -48,19 +48,25 @@ describe('summarizeQueue', () => { expect(s.progress).toBeCloseTo(0.4, 5) }) - it('sums download speeds across MB/s and MiB/s strings', () => { + it('sums raw download rates into one formatted label', () => { + const MB = 1024 * 1024 const s = summarizeQueue([ - item({ status: 'downloading', progress: 0.1, speed: '4.0 MB/s' }), - item({ status: 'downloading', progress: 0.1, speed: '2000 KiB/s' }) + item({ status: 'downloading', progress: 0.1, speedBytesPerSec: 4 * MB }), + item({ status: 'downloading', progress: 0.1, speedBytesPerSec: 2 * MB }) ]) - // 4.0 MB/s + ~2.0 MB/s ≈ 6.0 MB/s + // 4 MB/s + 2 MB/s = 6 MB/s expect(s.speedLabel).toBe('6.0 MB/s') }) + it('leaves the speed label empty when no item reports a rate', () => { + const s = summarizeQueue([item({ status: 'downloading', progress: 0.1 })]) + expect(s.speedLabel).toBe('') + }) + it('reports the longest active ETA as the time left', () => { const s = summarizeQueue([ - item({ status: 'downloading', progress: 0.5, eta: '0:30' }), - item({ status: 'downloading', progress: 0.2, eta: '2:10' }) + item({ status: 'downloading', progress: 0.5, etaSeconds: 30 }), + item({ status: 'downloading', progress: 0.2, etaSeconds: 130 }) ]) expect(s.etaLabel).toBe('2:10') })