Harden audit findings: correctness, type-safety, Windows conventions & polish
Audit-pass over CODE-AUDIT.md (~48 items closed this pass; all verified — typecheck + 234 tests + eslint + prettier green). Correctness / bugs: - B3: match the release checksum to the asset's filename line (no wrong-hash verify) - B4: newline-safe metadata probe (one --print with a unit-separator delimiter) - B5 / L88: guard the meta event against canceled items; progress no longer promotes a queued item outside pump() - B7: cookie-login promise always resolves (handles destroy-without-close) - L146: trim parser rejects >2 colon-group times; M36: Library selection counts only actionable rows - L11 / L50 / L156 / L57 / L159 / L15 / L3: live queue count, empty-cookie message, schedule picker min, dead-code/comment cleanup Type safety: - Enable noUncheckedIndexedAccess + noFallthroughCasesInSwitch (15 real edge cases fixed) Resilience / Windows / metadata: - R5: settings write failure handled (no unhandled IPC rejection; reconciles to truth) - W1 / W5 / W6: min window size, seeded folder picker, parented sign-in window; L147 dead macOS branches removed - CL1: shared stdout markers; package/builder metadata (license, homepage, repository, copyright, tsbuildinfo glob) Copy / docs / tests: - M37 / SR9 dev-jargon cleanup in hints; M8 / M25 / M26 / L66 / L80 / L81 reconciled - New unit tests for L35 (isValidMediaItem) and L36 (compareVersions) This commit also checkpoints the previously-uncommitted feat/tray-background-clipboard work it builds on: background running + auto-download, library clipboard detection, tray, binary management & library scale, credential encryption at rest, the shared jsonStore and ui/ primitives, and the eslint/prettier tooling. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This commit is contained in:
+107
-32
@@ -1,5 +1,6 @@
|
||||
import { spawn, execFile, type ChildProcess } from 'child_process'
|
||||
import { existsSync } from 'fs'
|
||||
import { existsSync, unlinkSync } from 'fs'
|
||||
import { tmpdir } from 'os'
|
||||
import { join } from 'path'
|
||||
import { BrowserWindow, Notification, type WebContents } from 'electron'
|
||||
import {
|
||||
@@ -8,11 +9,12 @@ import {
|
||||
getAria2cPath,
|
||||
getFfmpegPath,
|
||||
getFfprobePath,
|
||||
getSystem32Path
|
||||
getSystem32Path,
|
||||
getAppIconImage
|
||||
} from './binaries'
|
||||
import { getSettings, getDownloadArchivePath, getDefaultMediaDir } from './settings'
|
||||
import { ensureManagedYtdlp } from './ytdlp'
|
||||
import { getCookiesFilePath } from './cookies'
|
||||
import { materializeCookies, hasStoredCookies } from './cookies'
|
||||
import { listTemplates } from './templates'
|
||||
import { assertHttpUrl } from './url'
|
||||
import { isSafeOutputDir } from './validation'
|
||||
@@ -20,7 +22,9 @@ import {
|
||||
buildArgs,
|
||||
selectExtraArgs,
|
||||
formatCommandLine,
|
||||
collectionOutputTemplate
|
||||
collectionOutputTemplate,
|
||||
PROGRESS_MARKER,
|
||||
FILEPATH_MARKER
|
||||
} from './buildArgs'
|
||||
import { cleanError } from './log'
|
||||
import { addErrorLog } from './errorlog'
|
||||
@@ -43,6 +47,14 @@ interface ActiveDownload {
|
||||
|
||||
const active = new Map<string, ActiveDownload>()
|
||||
|
||||
// Backstop for B1: if a spawned yt-dlp goes completely silent — no stdout or
|
||||
// stderr — for this long, treat it as wedged, kill it, and error the item so the
|
||||
// concurrency slot frees instead of leaking forever. Generous on purpose so a
|
||||
// long, output-less post-processing step (e.g. a large ffmpeg merge) isn't
|
||||
// mistaken for a stall; --socket-timeout (buildArgs) handles the common
|
||||
// dead-connection case at the network layer.
|
||||
const STALL_TIMEOUT_MS = 5 * 60_000
|
||||
|
||||
/**
|
||||
* Whether any yt-dlp download is currently running. Used by the window's close
|
||||
* handler to keep the app alive in the tray (instead of quitting and killing the
|
||||
@@ -98,7 +110,10 @@ function parseProgress(rest: string): DownloadProgress | null {
|
||||
progress,
|
||||
speed: fmtSpeed(num(speed)),
|
||||
eta: fmtEta(num(eta)),
|
||||
sizeLabel: totalBytes ? fmtBytes(totalBytes) : undefined
|
||||
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).
|
||||
sizeUnknown: !totalBytes
|
||||
}
|
||||
}
|
||||
|
||||
@@ -109,7 +124,7 @@ function send(wc: WebContents, ev: DownloadEvent): void {
|
||||
/** Native OS notification on completion/failure, gated by Settings.notifyOnComplete. */
|
||||
function notify(wc: WebContents, title: string, body: string): void {
|
||||
if (!getSettings().notifyOnComplete || !Notification.isSupported()) return
|
||||
const n = new Notification({ title, body })
|
||||
const n = new Notification({ title, body, icon: getAppIconImage() })
|
||||
n.on('click', () => {
|
||||
if (wc.isDestroyed()) return
|
||||
const win = BrowserWindow.fromWebContents(wc)
|
||||
@@ -135,6 +150,12 @@ function logFailure(opts: StartDownloadOptions, title: string | undefined, error
|
||||
|
||||
// --- Best-effort metadata probe (runs alongside the download) ---------------
|
||||
|
||||
// Unit Separator (0x1F): a control char that can't appear in a title/uploader,
|
||||
// so it safely delimits the three fields in ONE --print template. Splitting on
|
||||
// newlines instead (B4) would mis-assign channel/duration whenever a title
|
||||
// itself contains a newline, since each --print field is emitted on its own line.
|
||||
const META_SEP = '\u001f'
|
||||
|
||||
function probeMeta(ytdlp: string, url: string): Promise<DownloadMeta | null> {
|
||||
return new Promise((resolve) => {
|
||||
execFile(
|
||||
@@ -144,20 +165,15 @@ function probeMeta(ytdlp: string, url: string): Promise<DownloadMeta | null> {
|
||||
'--no-warnings',
|
||||
'--skip-download',
|
||||
'--print',
|
||||
'title',
|
||||
'--print',
|
||||
'uploader',
|
||||
'--print',
|
||||
'duration_string',
|
||||
`%(title)s${META_SEP}%(uploader)s${META_SEP}%(duration_string)s`,
|
||||
'--',
|
||||
url
|
||||
],
|
||||
{ windowsHide: true, maxBuffer: 4 * 1024 * 1024, timeout: 30_000 },
|
||||
(err, stdout) => {
|
||||
if (err) return resolve(null)
|
||||
const [title, uploader, duration] = stdout.split('\n').map((l) => l.trim())
|
||||
const clean = (v?: string): string | undefined =>
|
||||
v && v !== 'NA' ? v : undefined
|
||||
const [title, uploader, duration] = stdout.split(META_SEP).map((l) => l.trim())
|
||||
const clean = (v?: string): string | undefined => (v && v !== 'NA' ? v : undefined)
|
||||
resolve({
|
||||
title: clean(title),
|
||||
channel: clean(uploader),
|
||||
@@ -175,6 +191,9 @@ function probeMeta(ytdlp: string, url: string): Promise<DownloadMeta | null> {
|
||||
// (see selectExtraArgs / audit F2). The gate is enforced here in main, not just
|
||||
// in the renderer UI, so the renderer can't be trusted to apply it.
|
||||
function resolveExtraArgs(opts: StartDownloadOptions, settings: Settings): string[] {
|
||||
// Gate the file read: listTemplates() parses templates.json on every call, so
|
||||
// skip it entirely when the feature is off (PERF2).
|
||||
if (!settings.customCommandEnabled) return []
|
||||
return selectExtraArgs({
|
||||
customCommandEnabled: settings.customCommandEnabled,
|
||||
perDownloadExtraArgs: opts.extraArgs,
|
||||
@@ -184,8 +203,10 @@ function resolveExtraArgs(opts: StartDownloadOptions, settings: Settings): strin
|
||||
})
|
||||
}
|
||||
|
||||
/** Resolve settings + per-download overrides into the full yt-dlp argv. */
|
||||
export function buildCommand(opts: StartDownloadOptions): string[] {
|
||||
/** Resolve settings + per-download overrides into the full yt-dlp argv.
|
||||
* `cookiesFile` is the transient decrypted jar the caller materialises for the
|
||||
* 'login' cookie source (H7); the 'browser' source uses --cookies-from-browser. */
|
||||
export function buildCommand(opts: StartDownloadOptions, cookiesFile?: string): string[] {
|
||||
const settings = getSettings()
|
||||
// Output dir resolution: a per-download override wins, then the user's explicit
|
||||
// per-kind folder (Settings → Video/Audio folder), and finally — when that's
|
||||
@@ -210,12 +231,6 @@ export function buildCommand(opts: StartDownloadOptions): string[] {
|
||||
// Silently fall back to yt-dlp's own downloader if aria2c.exe wasn't dropped
|
||||
// into resources/bin — the toggle shouldn't turn into a hard error.
|
||||
const aria2cPath = settings.useAria2c && existsSync(getAria2cPath()) ? getAria2cPath() : undefined
|
||||
// Same idea: 'login' cookies only apply once the sign-in window has actually
|
||||
// exported a file; otherwise the download proceeds cookie-less rather than failing.
|
||||
const cookiesFile =
|
||||
settings.cookieSource === 'login' && existsSync(getCookiesFilePath())
|
||||
? getCookiesFilePath()
|
||||
: undefined
|
||||
const access = {
|
||||
proxy: settings.proxy,
|
||||
rateLimit: settings.rateLimit,
|
||||
@@ -250,10 +265,7 @@ export function previewCommand(opts: StartDownloadOptions): CommandPreviewResult
|
||||
|
||||
// --- Public API -------------------------------------------------------------
|
||||
|
||||
export function startDownload(
|
||||
wc: WebContents,
|
||||
opts: StartDownloadOptions
|
||||
): StartDownloadResult {
|
||||
export function startDownload(wc: WebContents, opts: StartDownloadOptions): StartDownloadResult {
|
||||
// Self-heal the managed copy from the bundled seed before spawning, so a
|
||||
// never-seeded or deleted yt-dlp.exe doesn't fail an otherwise-fine download.
|
||||
ensureManagedYtdlp()
|
||||
@@ -301,10 +313,30 @@ export function startDownload(
|
||||
return { ok: false, error: 'Max concurrent downloads reached. Wait for a slot to free up.' }
|
||||
}
|
||||
|
||||
// Decrypt the stored cookie jar to a short-lived per-download temp file (H7).
|
||||
// yt-dlp reads --cookies once at startup; we delete it the moment the download
|
||||
// settles, so the plaintext never lingers at rest.
|
||||
let cookiesFile: string | undefined
|
||||
if (getSettings().cookieSource === 'login' && hasStoredCookies()) {
|
||||
const tmp = join(tmpdir(), `aerofetch-cookies-${opts.id}.txt`)
|
||||
if (materializeCookies(tmp)) cookiesFile = tmp
|
||||
}
|
||||
function cleanupCookies(): void {
|
||||
if (cookiesFile) {
|
||||
try {
|
||||
unlinkSync(cookiesFile)
|
||||
} catch {
|
||||
/* best-effort */
|
||||
}
|
||||
cookiesFile = undefined
|
||||
}
|
||||
}
|
||||
|
||||
let child: ChildProcess
|
||||
try {
|
||||
child = spawn(ytdlp, buildCommand(opts), { windowsHide: true })
|
||||
child = spawn(ytdlp, buildCommand(opts, cookiesFile), { windowsHide: true })
|
||||
} catch (e) {
|
||||
cleanupCookies()
|
||||
return { ok: false, error: (e as Error).message }
|
||||
}
|
||||
|
||||
@@ -331,31 +363,72 @@ export function startDownload(
|
||||
let stdoutBuf = ''
|
||||
let stderrTail = ''
|
||||
let filePath: string | undefined
|
||||
// Latched once the first download stream reports 'finished'; flags later
|
||||
// progress as the merge/post-processing "finishing" phase (SR7).
|
||||
let finishing = false
|
||||
// 'error' and 'close' can both fire for one process; only act on the first.
|
||||
let settled = false
|
||||
|
||||
// Idle watchdog (B1): reset on any output; if it ever fires, the child has been
|
||||
// silent for STALL_TIMEOUT_MS, so kill + error it and free the slot.
|
||||
let stallTimer: ReturnType<typeof setTimeout> | null = null
|
||||
function clearWatchdog(): void {
|
||||
if (stallTimer) {
|
||||
clearTimeout(stallTimer)
|
||||
stallTimer = null
|
||||
}
|
||||
}
|
||||
function bumpWatchdog(): void {
|
||||
clearWatchdog()
|
||||
stallTimer = setTimeout(() => {
|
||||
if (settled) return
|
||||
settled = true
|
||||
clearWatchdog()
|
||||
cleanupCookies()
|
||||
active.delete(opts.id)
|
||||
killTree(rec)
|
||||
const msg = `Download stalled — no activity for ${Math.round(STALL_TIMEOUT_MS / 60_000)} min. Stopped; retry to resume.`
|
||||
send(wc, { type: 'error', id: opts.id, error: msg })
|
||||
logFailure(opts, resolvedTitle, msg)
|
||||
notify(wc, resolvedTitle ?? 'Download failed', msg)
|
||||
}, STALL_TIMEOUT_MS)
|
||||
}
|
||||
bumpWatchdog()
|
||||
|
||||
child.stdout?.on('data', (chunk: Buffer) => {
|
||||
bumpWatchdog()
|
||||
stdoutBuf += chunk.toString()
|
||||
let nl: number
|
||||
while ((nl = stdoutBuf.indexOf('\n')) >= 0) {
|
||||
const line = stdoutBuf.slice(0, nl).replace(/\r$/, '')
|
||||
stdoutBuf = stdoutBuf.slice(nl + 1)
|
||||
if (line.startsWith('prog|')) {
|
||||
const p = parseProgress(line.slice('prog|'.length))
|
||||
if (p) send(wc, { type: 'progress', id: opts.id, progress: p })
|
||||
} else if (line.startsWith('path|')) {
|
||||
filePath = line.slice('path|'.length).trim()
|
||||
if (line.startsWith(PROGRESS_MARKER)) {
|
||||
const p = parseProgress(line.slice(PROGRESS_MARKER.length))
|
||||
if (p) {
|
||||
// SR7: yt-dlp reports a 'finished' status when each download stream
|
||||
// completes. A video+audio download has two streams, so the bar would
|
||||
// otherwise fill 0→100% twice. Latch on the first 'finished' and flag
|
||||
// every later tick as "finishing" so the renderer shows an indeterminate
|
||||
// merge state instead of a visible restart.
|
||||
if (p.status === 'finished') finishing = true
|
||||
send(wc, { type: 'progress', id: opts.id, progress: { ...p, finishing } })
|
||||
}
|
||||
} else if (line.startsWith(FILEPATH_MARKER)) {
|
||||
filePath = line.slice(FILEPATH_MARKER.length).trim()
|
||||
}
|
||||
}
|
||||
})
|
||||
|
||||
child.stderr?.on('data', (chunk: Buffer) => {
|
||||
bumpWatchdog()
|
||||
stderrTail = (stderrTail + chunk.toString()).slice(-4000)
|
||||
})
|
||||
|
||||
child.on('error', (err) => {
|
||||
if (settled) return
|
||||
settled = true
|
||||
clearWatchdog()
|
||||
cleanupCookies()
|
||||
active.delete(opts.id)
|
||||
// A paused download was killed on purpose — stay silent, like a cancel.
|
||||
if (!rec.canceled && !rec.paused) {
|
||||
@@ -368,6 +441,8 @@ export function startDownload(
|
||||
child.on('close', (code) => {
|
||||
if (settled) return
|
||||
settled = true
|
||||
clearWatchdog()
|
||||
cleanupCookies()
|
||||
active.delete(opts.id)
|
||||
// Canceled: renderer already showed 'canceled'. Paused: renderer showed
|
||||
// 'paused' and keeps the .part for a later resume. Either way, no event.
|
||||
|
||||
Reference in New Issue
Block a user