From 4d1c33a081a7e0782dc7a36a287dee9e21545969 Mon Sep 17 00:00:00 2001 From: debont80 Date: Wed, 1 Jul 2026 17:41:48 -0400 Subject: [PATCH] =?UTF-8?q?feat(audit):=20Batch=2015=20=E2=80=94=20downloa?= =?UTF-8?q?d-engine=20lifecycle=20(safe=20part;=20rest=20live-verify)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This is the batch the run reserves for a WATCHED LIVE SESSION, so only the safe, non-file-deleting item is implemented here; the rest are staged with per-item live-verify rationale in CODE-AUDIT.md. Landed: - L157: removeSource cancels its in-flight downloads. removeSource emits a `sourceRemoved` event on the coordinator bus (C2); the downloads store cancels every active (downloading/queued) item whose mediaItemId starts with `${sourceId}:` — MediaItem ids are `${sourceId}:${videoId}` (indexerCore), so the prefix reliably identifies the source's downloads. Reuses the proven cancel() path (process kill + pump). Type/lint/test-green; process-kill worth a live confirm. Resolved by prior work: - UX11: M4 persists the queue, so a scheduled item fires on next launch once due — no longer "never fires if quit". - UX20: L68 resets notifiedBackground on window show, so every close-to-tray re-fires the "still running in the tray" notice, not just the first. Deferred to the watched live pass (each needs a real download to verify): R4 (delete .part on cancel — file-deletion data-safety), L65 (graceful Ctrl-C pause), L139 (spawn↔auto-update interlock — timing race), B2 (index AbortSignal + Cancel), B6 (app-update cancel — ties to CL4's deferred updater), L143 (in-window quit-anyway). Verified: typecheck (node+web) + 279 tests + eslint + production build green; touched files prettier-clean. Co-Authored-By: Claude Opus 4.8 --- CODE-AUDIT.md | 76 +++++++++++++++++++++++++-- src/renderer/src/store/coordinator.ts | 3 ++ src/renderer/src/store/downloads.ts | 17 ++++++ src/renderer/src/store/sources.ts | 4 ++ 4 files changed, 95 insertions(+), 5 deletions(-) diff --git a/CODE-AUDIT.md b/CODE-AUDIT.md index e873ebb..36cb924 100644 --- a/CODE-AUDIT.md +++ b/CODE-AUDIT.md @@ -226,6 +226,34 @@ software-rendering workaround — external driver dependency), and **L104** (His L161 layout rework + live verification; the 500-cap bounds it). All green: typecheck (node+web) + 279 tests + eslint + production build; touched files prettier-clean. +**Session 2026-07-01 pass 12 (Batch 15 — download-engine lifecycle):** the batch the run's policy reserves for +a **watched live session** — so only the safe, non-file-deleting item was implemented, and the rest are staged +with per-item live-verify rationale. **Landed: L157** — `removeSource` now emits a `sourceRemoved` bus event +(C2) and the downloads store cancels every active item whose `mediaItemId` prefix (`${sourceId}:`, per +indexerCore) matches, reusing the proven `cancel()` path (implemented + type/lint/test-green; process-kill +worth a live confirm). **Resolved by prior work: UX11** (M4 persists the queue, so a scheduled item fires on +next launch once due — no longer "never fires if quit"), **UX20** (L68 resets `notifiedBackground` on window +show, so every close-to-tray re-fires the "still running" notice, not just the first). **Deferred to the +watched live pass with rationale — each needs a real download to verify:** **R4** (delete `.part` on cancel — +file-deletion data-safety), **L65** (graceful Ctrl-C pause vs `taskkill /F`), **L139** (spawn↔auto-update +interlock — timing race), **B2** (index `AbortSignal` + Cancel), **B6** (app-update cancel — ties to CL4's +deferred updater), **L143** (in-window quit-anyway). All green: typecheck (node+web) + 279 tests + eslint + +production build; touched files prettier-clean. + +--- + +## Run wrap-up (2026-07-01 — Batches 9–15 unattended) + +All seven remaining audit batches were worked as stacked branches (`audit/batch-9-surface-primitives` → +`audit/batch-15-download-lifecycle`), each: implement → self peer-review → verify (typecheck node+web · tests · +eslint · prod build · prettier) → push. The contained, safe, testable items landed; three categories were +**honestly deferred with documented rationale** rather than forced unattended: (1) large architectural +refactors the release gate already puts post-1.0 (CC9 zod, CC10/CC12/CC14 persistence/reorg, CL4 updater), +(2) subjective-visual items needing an eyeball (UI3/UI12/UI16/UX7/UX18/UI21/UX26 …), and (3) the download-engine +lifecycle changes that require a live download to verify (Batch 15's R4/L65/L139/B2/B6/L143). **Two items need +maintainer awareness:** SR4 flips `clipboardWatch` off-by-default for new installs (reversible), and Batch 15's +deferred set + L157's cancel-on-remove want a watched live smoke-test before merge. + **Deliberately deferred** (need live-app/visual verification or are larger refactors the audit itself defers to 1.x): the wire-or-cut features (M5/M6/UX1), the god-file/store refactors (C1, C2, H1), the SIMP* helpers + shared-UI-token work, the CC* consolidations, @@ -794,6 +822,11 @@ token system + shared primitives (UI/SIMP — high value, larger effort) · i18n *Deferred (feature): exposing aria2c tuning is a new advanced setting, not a cleanup; the defaults are sensible and aria2c itself is already opt-in.* - [ ] **L65 — Pause uses `taskkill /F`** like cancel — a forced kill risks an unflushed `.part` tail vs a graceful stop. + *Deferred to the watched Batch 15 live pass: a graceful stop on Windows means signalling the child + (GenerateConsoleCtrlEvent/Ctrl-C) so yt-dlp flushes, which is fiddly and only verifiable by pausing a real + download and checking the `.part` resumes cleanly. The forced kill works today (resume re-spawns and yt-dlp + continues the `.part`), so the practical loss is at most the last buffer — low impact, but the graceful path + needs live verification.* - [x] **L66 — Sidebar caption flips** "yt-dlp frontend" → "v" once the version loads (text shift on boot). - [x] **L67 — Onboarding has no Skip and can't be revisited** (no "show tips again"). *Fixed the revisit half (with UX22): Settings → About has a "Show welcome tips again" button that re-opens the onboarding @@ -1112,6 +1145,11 @@ token system + shared primitives (UI/SIMP — high value, larger effort) · i18n - [ ] **L139 — Auto-update vs concurrent spawn race.** `runStartupYtdlpAutoUpdate` can overwrite `yt-dlp.exe` on launch while a deep-link/queued download spawns the same binary — a Windows file-in-use/corrupt-spawn window. + *Deferred to the watched Batch 15 live pass: the fix (a spawn↔update interlock — hold spawns while the + update replaces the exe, and skip the update if a download is mid-spawn) is a timing-sensitive change to the + launch/spawn path that can only be validated by reproducing the race with a real launch-time update + a + queued download. The interlock flag itself is simple, but confirming it closes the window needs the live + session.* - [x] **L140 — Retry-during-teardown race.** `retry` reuses the same id; if the previous process's `close` handler hasn't removed it from `active` yet, `startDownload` returns "already running" → markError. *Fixed in [download.ts](src/main/download.ts): `cancelDownload`/`pauseDownload` now drop the item from @@ -1136,6 +1174,9 @@ token system + shared primitives (UI/SIMP — high value, larger effort) · i18n - [ ] **L143 — No in-window "quit anyway."** With a download active (or tray mode), closing only hides; quitting requires the tray menu. If the tray ever fails to create, the only exit is Task Manager (mitigated today by the embedded fallback tray icon). + *Deferred to the watched Batch 15 live pass: adding an in-window force-quit affordance (and deciding when to + show it) is a window-lifecycle change best verified against the real tray/close behavior at several window + states. Already mitigated by the embedded fallback tray icon, so it's low-urgency.* - [x] **L144 — Duplicate guard checks the queue only, not history.** Re-adding a URL downloaded earlier (already cleared from the queue) gives no "already downloaded" hint (roadmap notes this as a possible extension). *Fixed: the DownloadBar dup guard ([useDownloadBar.ts](src/renderer/src/components/downloadBar/useDownloadBar.ts)) @@ -1188,8 +1229,14 @@ token system + shared primitives (UI/SIMP — high value, larger effort) · i18n `aria-label` is gone (aria-checked conveys it).* - [x] **L156 — `datetime-local` schedule has no `min`.** Past times are selectable, then silently download now (extends L57). Add `min={now}`. -- [ ] **L157 — `removeSource` doesn't cancel in-flight downloads.** Removing a source while its videos are +- [x] **L157 — `removeSource` doesn't cancel in-flight downloads.** Removing a source while its videos are downloading lets them finish into the removed source's folders; `markDownloaded` then no-ops on the gone item. + *Fixed: `removeSource` now emits a `sourceRemoved` event on the coordinator bus (C2); the downloads store + subscribes and cancels every active (downloading/queued) item whose `mediaItemId` starts with `${sourceId}:` + — MediaItem ids are `${sourceId}:${videoId}` (indexerCore), so the prefix reliably identifies the source's + downloads. Reuses the proven `cancel()` path (process kill + pump). **Live-smoke pending** (per the + mark-done convention): implemented + type/lint/test-green, but the actual process-kill-on-remove is worth a + watched confirm in the Batch 15 live pass.* - [x] **L158 — Drag highlight flickers.** `onDragLeave={() => setDragActive(false)}` fires when the cursor crosses child elements (no enter/leave depth counter), so the dashed-outline blinks during a drag. *Fixed: [useDownloadBar.ts](src/renderer/src/components/downloadBar/useDownloadBar.ts) now tracks a @@ -1587,9 +1634,13 @@ root cause is already filed. a non-portal bottom-center stack that any store/component can raise (now used for UX6's file-action errors). Together this closes **UI25**.* - [x] **UX10 — Re-download silently changes quality** (H5) — a confusing "I asked for 720p, got Best." *Resolved by H5 (re-download stores `formatId`/`formatHasAudio` and strips compound quality labels).* -- [ ] **UX11 — Scheduled downloads silently never fire if the app is quit** (M4). The hint is easy to +- [x] **UX11 — Scheduled downloads silently never fire if the app is quit** (M4). The hint is easy to miss; the user returns to find nothing happened. **Fix:** warn at schedule time that it requires the - app running (or persist + relaunch). + app running (or persist + relaunch). *Resolved by M4 (the "persist + relaunch" option the fix names): the + queue is now persisted, so a `saved`+`scheduledFor` item survives a quit and the App promoter tick fires it + on the next launch once its time has passed (the store normalizes/promotes due items). So a scheduled + download no longer "never fires" if quit — it fires when due, or on the next launch if that time already + passed. (Live-smoke of the quit→relaunch cycle noted in M4.)* - [x] **UX12 — Terminal is a dead-end when custom commands are off** (L18). The nav item is always visible; clicking it shows a gate pointing to a setting on another screen. **Fix:** hide/disable the nav item, or let the gate enable the setting inline. *Fixed (both fixes): the nav item is already hidden @@ -1630,9 +1681,13 @@ root cause is already filed. - [x] **UX19 — Window size/position isn't remembered.** [index.ts](src/main/index.ts) opens a fixed 920×700 every launch (no bounds persistence). **Fix:** persist + restore window bounds. *Fixed with W2 — [windowState.ts](src/main/windowState.ts) persists + restores bounds/maximized with an off-screen guard.* -- [ ] **UX20 — "App is still running" surprise.** Closing with a download active hides to tray and +- [x] **UX20 — "App is still running" surprise.** Closing with a download active hides to tray and notifies once per process (L68); later closes give no hint, so the window "won't close" reads as a - bug. **Fix:** a persistent tray hint / first-close explainer. + bug. **Fix:** a persistent tray hint / first-close explainer. *Resolved by L68 (the "first-close explainer" + it now is on *every* close): the once-per-process guard was the whole bug — `notifiedBackground` now resets + when the window is shown again, so each close-to-tray with a download active (or tray mode) re-fires the + "AeroFetch is still running in the tray" notification, not just the first. So a later close no longer looks + like the window is stuck. (The tray behavior itself is worth a watched confirm in the Batch 15 live pass.)* - [x] **UX21 — No keyboard-shortcut discoverability.** Ctrl+K (palette), Enter, Ctrl+Enter are undocumented; there's no "?"/shortcuts screen. **Fix:** a shortcuts hint or help affordance. *Fixed (discoverability): the first-run Onboarding now includes a tip — "Press Ctrl+K anytime to jump to any @@ -2100,6 +2155,9 @@ earlier passes are listed so this is complete; and the categories that came back sequential probes (each up to the 180 s `probeFlat` timeout); the UI shows "indexing…" with the Index button disabled and **no cancel**. A large channel locks the add-source flow for minutes with no abort. **Fix:** thread an `AbortSignal`/cancel token and a Cancel button. + *Deferred to the watched Batch 15 live pass: threading an `AbortSignal` through the main-process index walk + (and killing the in-flight probe child) plus a Cancel button is a real feature whose abort behavior needs a + live large-channel index to verify it actually stops the walk mid-probe.* - [x] **B3 — `extractSha256` ignores the filename (wrong-hash risk).** It returns the **first** 64-hex token in the file ([updater.ts](src/main/updater.ts)); a combined multi-file checksum uploaded as `.sha256` would verify the installer against the wrong line's hash. Safe only by the one-hash- @@ -2113,6 +2171,10 @@ earlier passes are listed so this is complete; and the categories that came back canceled item's title. Harmless today, but the asymmetry is a latent bug. **Fix:** guard `meta` like the others. - [ ] **B6 — App-update download has no cancel.** Once `downloadAppUpdate` starts there's no user abort — only completion or the idle timeout stops it; the progress UI offers no Cancel. **Fix:** expose cancel (abort the request). + *Deferred to the watched Batch 15 live pass (ties to CL4): it touches the security-critical updater + installer-download path (the same block CL4 defers) — exposing a cancel means an IPC that calls the + request's `abort()` + teardown, verifiable only by cancelling a real ~300 MB update download and confirming + the partial is cleaned up. Done with the CL4/updater watched work.* - [x] **B7 — Cookie-login promise can never resolve if the window is destroyed without `close`.** `openCookieLoginWindow` resolves only via the `close`→`exportAndResolve` path; a `destroy()` (or a `closed` without `close`) would leave `pendingResolvers` pending forever. Not triggered by current code, @@ -2167,6 +2229,10 @@ cosmetics. `R` IDs. deletes the partial; canceled items vanish from the UI while `.part`/fragment files accumulate in the output folder. (App crash mid-download likewise — the queue isn't persisted to resume, M4.) **Fix:** clean up the partial on cancel, or surface/offer-resume for orphans. + *Deferred to the watched Batch 15 live pass (data-safety): the fix means capturing yt-dlp's destination path + during the run and **deleting** `.part`/`.part-Frag*` on cancel — file deletion whose correctness + can't be verified without a real download+cancel. Doing it unattended risks deleting the wrong file if the + destination capture is even slightly off, so it must be verified live before shipping.* - [x] **R5 — Settings write failure is unhandled.** `setSettings` calls `store.set(...)` with no try/catch; on disk-full/read-only `electron-store` throws → the IPC rejects → the renderer's `setSettings().catch(() => {})` swallows it while the optimistic UI keeps the change (the disk-full form of M34). **Fix:** catch and report. diff --git a/src/renderer/src/store/coordinator.ts b/src/renderer/src/store/coordinator.ts index 95e78f2..429db20 100644 --- a/src/renderer/src/store/coordinator.ts +++ b/src/renderer/src/store/coordinator.ts @@ -13,12 +13,15 @@ import type { AddEntry } from './downloads' * * sources --enqueueDownloads--> downloads (queue a channel's media items) * downloads --downloadCompleted--> sources (mark the source item downloaded) + * sources --sourceRemoved------> downloads (cancel that source's in-flight downloads) */ export interface StoreEvents { /** sources → downloads: enqueue a batch of media items as downloads */ enqueueDownloads: AddEntry[] /** downloads → sources: a download tied to a source MediaItem finished */ downloadCompleted: { mediaItemId: string; filePath?: string } + /** sources → downloads: a source was removed; cancel its in-flight downloads (L157) */ + sourceRemoved: string } type Handler = (payload: StoreEvents[K]) => void diff --git a/src/renderer/src/store/downloads.ts b/src/renderer/src/store/downloads.ts index f44e369..c2ea243 100644 --- a/src/renderer/src/store/downloads.ts +++ b/src/renderer/src/store/downloads.ts @@ -462,6 +462,23 @@ export const useDownloads = create((set, get) => { // no direct import from sources, so the two stores stay acyclic. on('enqueueDownloads', (entries) => useDownloads.getState().addMany(entries)) +// A removed source cancels its in-flight downloads (L157): MediaItem ids are +// `${sourceId}:${videoId}` (indexerCore), so a download's mediaItemId prefix +// identifies its source. Only active (downloading/queued) items are canceled; +// finished/failed ones are left alone. cancel() owns the process kill + pump. +on('sourceRemoved', (sourceId) => { + const st = useDownloads.getState() + const prefix = `${sourceId}:` + for (const it of st.items) { + if ( + (it.status === 'downloading' || it.status === 'queued') && + it.mediaItemId?.startsWith(prefix) + ) { + st.cancel(it.id) + } + } +}) + // M4: mirror the durable queue to main whenever it changes, so saved/scheduled and // other pending items survive a quit. Skipped in preview (no IPC bridge). A // progress-only update doesn't churn this — toPersisted drops progress/speed/eta, so diff --git a/src/renderer/src/store/sources.ts b/src/renderer/src/store/sources.ts index 0f42f81..ce8b74f 100644 --- a/src/renderer/src/store/sources.ts +++ b/src/renderer/src/store/sources.ts @@ -178,6 +178,10 @@ export const useSources = create((set, get) => ({ sources: s.sources.filter((x) => x.id !== id), selectedSourceId: s.selectedSourceId === id ? null : s.selectedSourceId })) + // Cancel any of this source's downloads still in flight, so removing a source + // stops its videos from finishing into now-orphaned folders (L157). Routed via + // the event bus (C2) so sources doesn't import downloads. + emit('sourceRemoved', id) if (!PREVIEW) window.api.removeSource(id).catch(logError('removeSource')) },