From 96800d5a17a9aae7c15c6dcacecd9792b9409439 Mon Sep 17 00:00:00 2001 From: debont80 Date: Wed, 1 Jul 2026 22:57:20 -0400 Subject: [PATCH] =?UTF-8?q?feat(audit):=20L51=20+=20L64=20=E2=80=94=20dail?= =?UTF-8?q?y-sync=20time=20picker=20+=20aria2c=20connection=20tuning?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two small self-contained features from the audit's deferred list: - L51: the Task Scheduler daily sync is no longer pinned to 09:00. New Settings.syncTime (24h HH:MM) with a native time input beside the Library "Daily sync" switch — persists on change, re-registers the task on blur (schtasks /Create /F overwrite is the time-change mechanism). The value reaches the schtasks /ST argv, so a shared isValidSyncTime guards it at both the settings write and in schedule.ts (unit-tested incl. injection-shaped input). Live-verified against the machine's real task. - L64: aria2c connection count (-x/-s) is user-tunable. New Settings.aria2cConnections (1-16, aria2c's own ceiling) exposed as a SpinButton in Settings -> Network while the aria2c toggle is on. ARIA2C_ARGS became the pure aria2cArgs(n?) with a defensive clamp; threaded through AccessOptions. The -k 1M split floor stays fixed — a free-text field would bypass the custom-command consent gate. typecheck + 309 tests + eslint + production build green. Co-Authored-By: Claude Opus 4.8 --- CODE-AUDIT.md | 25 +++++++++---- src/main/buildArgs.ts | 26 +++++++++++--- src/main/download.ts | 1 + src/main/ipc.ts | 5 ++- src/main/schedule.ts | 15 +++++--- src/main/settings.ts | 12 +++++++ src/preload/index.ts | 7 ++-- src/renderer/src/components/LibraryView.tsx | 28 ++++++++++++++- .../src/components/settings/NetworkCard.tsx | 31 +++++++++++++++- src/shared/ipc.ts | 15 ++++++++ test/buildArgs.test.ts | 20 +++++++++++ test/validation.test.ts | 35 ++++++++++++++++++- 12 files changed, 199 insertions(+), 21 deletions(-) diff --git a/CODE-AUDIT.md b/CODE-AUDIT.md index 15e95a0..abb11a1 100644 --- a/CODE-AUDIT.md +++ b/CODE-AUDIT.md @@ -797,9 +797,16 @@ token system + shared primitives (UI/SIMP — high value, larger effort) · i18n `\n`, which renders awkwardly in inline/Caption error spans. - [x] **L50 — "Cookies saved" shown for 0 cookies.** Closing the sign-in window without logging in still writes the file and reports `ok` with `cookieCount: 0`. -- [ ] **L51 — Scheduled daily sync time hardcoded to 09:00** (schedule.ts) — on/off toggle only, no time picker. - *Deferred (feature): a time picker is a small new feature (UI + a Task Scheduler time argument), not a - cleanup — worth its own change; 09:00 is a reasonable default meanwhile.* +- [x] **L51 — Scheduled daily sync time hardcoded to 09:00** (schedule.ts) — on/off toggle only, no time picker. + *Fixed. New `Settings.syncTime` (24h 'HH:MM', default '09:00'); a compact native time input appears next to + the Library's "Daily sync" switch when it's on. `setScheduledSync(enabled, time?)` threads the time into + `schtasks /ST`, guarded by a shared `isValidSyncTime` (in @shared/ipc, used by both main's settings + validation and schedule.ts — the value reaches the schtasks argv, so malformed input falls back to the + default rather than being forwarded; unit-tested incl. injection-shaped values). The picker persists via + settings on change and re-registers the task on blur (one `schtasks /Create /F` per commit, not per + keystroke — /F overwrite IS the time-change mechanism). Live-verified on this machine: re-registering the + real AeroFetchDailySync task with /ST 19:30 moved its start to 7:30 PM and back (the user's original 09:00 + registration restored byte-exact afterwards).* - [x] **L52 — Installer handoff is a fixed `setTimeout(app.quit, 1500)`** (updater.ts) — a race on slow machines. - [x] **L53 — `dialog.show*Dialog(win!, …)` non-null assertions** on a possibly-null window (chooseFolder, backup export/import) — can throw if the sender window is gone. @@ -818,9 +825,15 @@ token system + shared primitives (UI/SIMP — high value, larger effort) · i18n - [x] **L62 — "Best available" synthetic format hardcodes `ext:'mp4'`/`hasAudio:true`** (probe.ts) — mislabels when the chosen container is mkv/webm. - [x] **L63 — `audioQuality()` unknown label → silent `'0'` (best)** — the audio analog of H5. -- [ ] **L64 — `ARIA2C_ARGS` connection params hardcoded** (`-x16 -s16 -k1M`), no user control. - *Deferred (feature): exposing aria2c tuning is a new advanced setting, not a cleanup; the defaults are - sensible and aria2c itself is already opt-in.* +- [x] **L64 — `ARIA2C_ARGS` connection params hardcoded** (`-x16 -s16 -k1M`), no user control. + *Fixed. New `Settings.aria2cConnections` (1–16, default 16 — aria2c's own -x/-s ceiling), exposed as an + "aria2c connections" SpinButton in Settings → Network, shown only while the aria2c toggle is on (same + pattern as maxConcurrent). The const became a pure `aria2cArgs(connections?)` in buildArgs.ts that clamps/ + rounds defensively (settings validation clamps too — `applySettings` mirrors the maxConcurrent case); + `ARIA2C_ARGS` remains exported as `aria2cArgs()` so existing references stay valid. Threaded through + `AccessOptions.aria2cConnections` from buildCommand. The `-k 1M` split floor stays fixed — the meaningful + user knob is connection count; a free-text field would bypass the customCommandEnabled consent gate. + Unit-tested: custom count reaches `--downloader-args`, clamp floor/ceiling/rounding/non-finite.* - [x] **L65 — Pause uses `taskkill /F`** like cancel — a forced kill risks an unflushed `.part` tail vs a graceful stop. *Verified acceptable in the Batch 15 live pass — no code change needed. Live test (a real 360p download paused via the same `taskkill /T /F` the app issues, then resumed with identical args): the paused `.part` was left diff --git a/src/main/buildArgs.ts b/src/main/buildArgs.ts index 0020280..8ad30a5 100644 --- a/src/main/buildArgs.ts +++ b/src/main/buildArgs.ts @@ -104,6 +104,8 @@ export interface AccessOptions { rateLimit: string /** absolute path to aria2c.exe; omit to use yt-dlp's built-in downloader */ aria2cPath?: string + /** aria2c parallel connections (-x/-s), clamped to 1–16; omitted = 16 (L64) */ + aria2cConnections?: number /** read auth cookies from an installed browser's own cookie store */ cookiesFromBrowser?: CookieBrowser /** absolute path to a Netscape-format cookie file (the sign-in window's export) */ @@ -118,9 +120,23 @@ export interface AccessOptions { youtubePoToken?: string } -// Tuned for typical CDN-served video: 16 connections split across 16 segments, -// each at least 1MB so tiny files don't get split pointlessly. -export const ARIA2C_ARGS = 'aria2c:-x 16 -s 16 -k 1M' +/** + * The yt-dlp `--downloader-args` value for aria2c: N connections split across N + * segments, each at least 1MB so tiny files don't get split pointlessly. The + * connection count is user-tunable (Settings → Network, L64) and clamped here to + * aria2c's own -x/-s ceiling of 16 — settings validation clamps too, but this is + * pure-module defence for any other caller. Non-finite input → the default 16. + */ +export function aria2cArgs(connections?: number): string { + const n = + connections !== undefined && Number.isFinite(connections) + ? Math.min(16, Math.max(1, Math.round(connections))) + : 16 + return `aria2c:-x ${n} -s ${n} -k 1M` +} + +/** The default tuning (16 connections) — kept for tests/docs that reference it. */ +export const ARIA2C_ARGS = aria2cArgs() /** * Shell-like split of a raw "extra yt-dlp args" string (Phase C custom-command @@ -295,7 +311,9 @@ function accessArgs(a: AccessOptions): string[] { const args: string[] = [] if (a.proxy.trim()) args.push('--proxy', a.proxy.trim()) if (a.rateLimit.trim()) args.push('--limit-rate', a.rateLimit.trim()) - if (a.aria2cPath) args.push('--downloader', a.aria2cPath, '--downloader-args', ARIA2C_ARGS) + if (a.aria2cPath) { + args.push('--downloader', a.aria2cPath, '--downloader-args', aria2cArgs(a.aria2cConnections)) + } // cookiesFromBrowser and cookiesFile are mutually exclusive sources — the UI // only ever sets one (driven by Settings.cookieSource), but browser wins if // a caller somehow sets both. diff --git a/src/main/download.ts b/src/main/download.ts index 8f54875..8e63a48 100644 --- a/src/main/download.ts +++ b/src/main/download.ts @@ -214,6 +214,7 @@ export function buildCommand(opts: StartDownloadOptions, cookiesFile?: string): proxy: settings.proxy, rateLimit: settings.rateLimit, aria2cPath, + aria2cConnections: settings.aria2cConnections, // L136/M6: incognito attaches no cookies from the browser either. cookiesFromBrowser: !opts.incognito && settings.cookieSource === 'browser' ? settings.cookiesBrowser : undefined, diff --git a/src/main/ipc.ts b/src/main/ipc.ts index ea102c9..8c25099 100644 --- a/src/main/ipc.ts +++ b/src/main/ipc.ts @@ -270,7 +270,10 @@ export function registerIpcHandlers(getMainWindow: () => BrowserWindow | null): }) ) ipcMain.handle(IpcChannels.scheduledSyncGet, () => getScheduledSync()) - ipcMain.handle(IpcChannels.scheduledSyncSet, (_e, enabled: boolean) => setScheduledSync(enabled)) + // `time` (24h HH:MM) is validated inside setScheduledSync before touching schtasks (L51). + ipcMain.handle(IpcChannels.scheduledSyncSet, (_e, enabled: boolean, time?: string) => + setScheduledSync(enabled, time) + ) // Reflect overall queue progress on the Windows taskbar and window title (SR8). ipcMain.handle(IpcChannels.taskbarProgress, (_e, p: TaskbarProgress) => { diff --git a/src/main/schedule.ts b/src/main/schedule.ts index e352b13..645b69b 100644 --- a/src/main/schedule.ts +++ b/src/main/schedule.ts @@ -11,7 +11,7 @@ import { execFile } from 'child_process' import { getSystem32Path } from './binaries' -import type { ScheduledSyncStatus } from '@shared/ipc' +import { isValidSyncTime, DEFAULT_SETTINGS, type ScheduledSyncStatus } from '@shared/ipc' const TASK_NAME = 'AeroFetchDailySync' /** The argv flag the scheduled task passes so startup knows it's a sync launch. */ @@ -46,10 +46,17 @@ export async function getScheduledSync(): Promise { /** * Register or remove the daily-sync scheduled task. Creating runs AeroFetch with - * `--sync` every day at 09:00 (overwriting any prior task of the same name). + * `--sync` every day at the given 24h 'HH:MM' time (overwriting any prior task of + * the same name — which is also how a time change re-registers, L51). The time is + * renderer-supplied over IPC, so it's validated here before reaching the schtasks + * argv; anything malformed falls back to the default rather than failing the toggle. */ -export async function setScheduledSync(enabled: boolean): Promise { +export async function setScheduledSync( + enabled: boolean, + time?: string +): Promise { if (enabled) { + const startTime = isValidSyncTime(time) ? time : DEFAULT_SETTINGS.syncTime const tr = `"${process.execPath}" ${SYNC_FLAG}` const r = await schtasks([ '/Create', @@ -57,7 +64,7 @@ export async function setScheduledSync(enabled: boolean): Promise, partial: Partial): void { if (Number.isFinite(n)) s.set('maxConcurrent', Math.min(5, Math.max(1, Math.round(n)))) break } + case 'aria2cConnections': { + // aria2c caps -x/-s at 16; clamp like maxConcurrent rather than reject (L64). + const n = Number(value) + if (Number.isFinite(n)) s.set('aria2cConnections', Math.min(16, Math.max(1, Math.round(n)))) + break + } + case 'syncTime': + // Reaches `schtasks /ST` when the daily sync is (re)registered — only a + // well-formed 24h HH:MM is ever stored (L51). + if (isValidSyncTime(value)) s.set('syncTime', value) + break case 'clipboardWatch': case 'useAria2c': case 'restrictFilenames': diff --git a/src/preload/index.ts b/src/preload/index.ts index 1b05b7a..49e8f85 100644 --- a/src/preload/index.ts +++ b/src/preload/index.ts @@ -218,11 +218,12 @@ const api = { * match the main function `syncWatchedSources` (L92). */ syncWatchedSources: (): Promise => ipcRenderer.invoke(IpcChannels.sourcesSync), - /** Read / write the Windows daily-sync scheduled task. */ + /** Read / write the Windows daily-sync scheduled task. `time` is the 24h 'HH:MM' + * start time (L51); omitted = the default. Re-creating with a new time updates it. */ getScheduledSync: (): Promise => ipcRenderer.invoke(IpcChannels.scheduledSyncGet), - setScheduledSync: (enabled: boolean): Promise => - ipcRenderer.invoke(IpcChannels.scheduledSyncSet, enabled), + setScheduledSync: (enabled: boolean, time?: string): Promise => + ipcRenderer.invoke(IpcChannels.scheduledSyncSet, enabled, time), /** Subscribe to live indexing progress. Returns an unsubscribe function. */ onIndexProgress: (cb: (p: IndexProgress) => void): (() => void) => { diff --git a/src/renderer/src/components/LibraryView.tsx b/src/renderer/src/components/LibraryView.tsx index eb67d51..8ed38ac 100644 --- a/src/renderer/src/components/LibraryView.tsx +++ b/src/renderer/src/components/LibraryView.tsx @@ -237,6 +237,7 @@ export function LibraryView(): React.JSX.Element { const syncing = useSources((s) => s.syncing) const downloadItems = useDownloads((s) => s.items) const autoDownloadNew = useSettings((s) => s.autoDownloadNew) + const syncTime = useSettings((s) => s.syncTime) const updateSettings = useSettings((s) => s.update) const [url, setUrl] = useState('') @@ -294,11 +295,26 @@ export function LibraryView(): React.JSX.Element { async function toggleScheduled(next: boolean): Promise { setScheduled(next) // optimistic if (PREVIEW) return - const res = await window.api.setScheduledSync(next) + const res = await window.api.setScheduledSync(next, syncTime) setScheduled(res.enabled) if (res.error) setError(res.error) } + // The native time input emits a complete 'HH:MM' or '' (cleared/partial) — only + // persist real times, so the picker can never store a malformed value (L51). + function onSyncTimeChange(value: string): void { + if (!/^([01]\d|2[0-3]):[0-5]\d$/.test(value)) return + updateSettings({ syncTime: value }) + } + + // Re-register the task with the new time once editing settles (blur), so typing + // through the segments doesn't spawn schtasks per keystroke (L51). + async function onSyncTimeCommit(): Promise { + if (!scheduled || PREVIEW) return + const res = await window.api.setScheduledSync(true, useSettings.getState().syncTime) + if (res.error) setError(res.error) + } + // Reset the selection (and any batch note) whenever the expanded source changes. useEffect(() => { setSelected(new Set()) @@ -578,6 +594,16 @@ export function LibraryView(): React.JSX.Element { onChange={(_, d) => toggleScheduled(d.checked)} aria-label="Daily background sync" /> + {scheduled && ( + onSyncTimeChange(d.value)} + onBlur={onSyncTimeCommit} + aria-label="Daily sync time" + /> + )} diff --git a/src/renderer/src/components/settings/NetworkCard.tsx b/src/renderer/src/components/settings/NetworkCard.tsx index 2926cf2..643e13f 100644 --- a/src/renderer/src/components/settings/NetworkCard.tsx +++ b/src/renderer/src/components/settings/NetworkCard.tsx @@ -1,5 +1,14 @@ import { useState } from 'react' -import { Field, Input, Switch, Button, Card, Subtitle2, Spinner } from '@fluentui/react-components' +import { + Field, + Input, + Switch, + SpinButton, + Button, + Card, + Subtitle2, + Spinner +} from '@fluentui/react-components' import { GlobeRegular } from '@fluentui/react-icons' import { useSettings } from '../../store/settings' import { useSettingsStyles } from './settingsStyles' @@ -9,6 +18,7 @@ export function NetworkCard(): React.JSX.Element { const proxy = useSettings((s) => s.proxy) const rateLimit = useSettings((s) => s.rateLimit) const useAria2c = useSettings((s) => s.useAria2c) + const aria2cConnections = useSettings((s) => s.aria2cConnections) const youtubePlayerClient = useSettings((s) => s.youtubePlayerClient) const youtubePoToken = useSettings((s) => s.youtubePoToken) const update = useSettings((s) => s.update) @@ -53,6 +63,25 @@ export function NetworkCard(): React.JSX.Element { update({ useAria2c: d.checked })} /> + {useAria2c && ( + + { + const v = d.value ?? Number(d.displayValue) + if (typeof v === 'number' && Number.isFinite(v)) { + update({ aria2cConnections: Math.min(16, Math.max(1, Math.round(v))) }) + } + }} + /> + + )} + { const argv = build(opts(), dlo(), NO_ACCESS) expect(argv).not.toContain('--downloader-args') }) + + it('threads a custom aria2c connection count into --downloader-args (L64)', () => { + const argv = build(opts(), dlo(), { + ...NO_ACCESS, + aria2cPath: 'C:/fake/bin/aria2c.exe', + aria2cConnections: 4 + }) + expect(hasSeq(argv, '--downloader-args', 'aria2c:-x 4 -s 4 -k 1M')).toBe(true) + }) + + it('aria2cArgs clamps to aria2c’s 1–16 ceiling and defaults to 16 (L64)', () => { + expect(aria2cArgs()).toBe('aria2c:-x 16 -s 16 -k 1M') + expect(aria2cArgs(8)).toBe('aria2c:-x 8 -s 8 -k 1M') + expect(aria2cArgs(0)).toBe('aria2c:-x 1 -s 1 -k 1M') // below floor → 1 + expect(aria2cArgs(99)).toBe('aria2c:-x 16 -s 16 -k 1M') // above ceiling → 16 + expect(aria2cArgs(7.6)).toBe('aria2c:-x 8 -s 8 -k 1M') // rounds + expect(aria2cArgs(NaN)).toBe('aria2c:-x 16 -s 16 -k 1M') // non-finite → default + expect(ARIA2C_ARGS).toBe(aria2cArgs()) // the exported default stays in sync + }) }) // --- Cookies: browser store vs. sign-in window's exported file -------------- diff --git a/test/validation.test.ts b/test/validation.test.ts index 88a0e02..ce96b35 100644 --- a/test/validation.test.ts +++ b/test/validation.test.ts @@ -8,7 +8,40 @@ import { isValidSource, isValidMediaItem } from '../src/main/validation' -import type { HistoryEntry, ErrorLogEntry, Source, MediaItem } from '@shared/ipc' +import { + isValidSyncTime, + type HistoryEntry, + type ErrorLogEntry, + type Source, + type MediaItem +} from '@shared/ipc' + +// --- L51: daily-sync time (reaches `schtasks /ST`) --------------------------- + +describe('isValidSyncTime', () => { + it('accepts well-formed 24h HH:MM times', () => { + expect(isValidSyncTime('00:00')).toBe(true) + expect(isValidSyncTime('09:00')).toBe(true) + expect(isValidSyncTime('19:30')).toBe(true) + expect(isValidSyncTime('23:59')).toBe(true) + }) + + it('rejects out-of-range, partial, and 12h forms', () => { + expect(isValidSyncTime('24:00')).toBe(false) + expect(isValidSyncTime('12:60')).toBe(false) + expect(isValidSyncTime('9:00')).toBe(false) // must be zero-padded + expect(isValidSyncTime('09:0')).toBe(false) + expect(isValidSyncTime('9am')).toBe(false) + expect(isValidSyncTime('')).toBe(false) + }) + + it('rejects non-strings and injection-shaped values', () => { + expect(isValidSyncTime(undefined)).toBe(false) + expect(isValidSyncTime(900)).toBe(false) + expect(isValidSyncTime('09:00 /TN evil')).toBe(false) // nothing may trail the time + expect(isValidSyncTime('09:00\n')).toBe(false) + }) +}) // --- S4: filename template path-traversal -----------------------------------