From c55e131fb094ef2d6447e670ec6673f7c45d668a Mon Sep 17 00:00:00 2001 From: debont80 Date: Tue, 30 Jun 2026 15:36:13 -0400 Subject: [PATCH] Fix M3: extract shared recordCompletion() for download-done side-effects The completion side-effect (add to history unless private + markDownloaded on the source MediaItem) was duplicated in applyEvent('done') and the preview fake-ticker, and had already drifted (the preview copy omitted formatId/ formatHasAudio). Extracted one recordCompletion(item) helper that both call. Also tightened a latent bug: the old applyEvent ran markDownloaded for any 'done' event with a mediaItemId, even when the item was canceled (the reducer keeps canceled items as 'canceled'). recordCompletion now runs only when status === 'completed', so a canceled collection item is no longer wrongly marked downloaded (and will be retried on the next re-sync). typecheck + 242 tests + eslint green. Co-Authored-By: Claude Opus 4.8 --- CODE-AUDIT.md | 2 +- src/renderer/src/store/downloads.ts | 69 ++++++++++++----------------- 2 files changed, 30 insertions(+), 41 deletions(-) diff --git a/CODE-AUDIT.md b/CODE-AUDIT.md index 7f06c66..48c7d30 100644 --- a/CODE-AUDIT.md +++ b/CODE-AUDIT.md @@ -223,7 +223,7 @@ token system + shared primitives (UI/SIMP — high value, larger effort) · i18n `history.ts`/`errorlog.ts`/`templates.ts` reimplement it inline. - [x] **M2 — Remove dead code: `getDefaultFolder` / `download:default-folder`** (channel + preload + handler + mock, zero callers). -- [ ] **M3 — Remove duplicated completion side-effect in `store/downloads.ts`** (history write + +- [x] **M3 — Remove duplicated completion side-effect in `store/downloads.ts`** (history write + `markDownloaded` in both `applyEvent('done')` and the preview ticker). Subsumed by C2. - [ ] **M4 — Decide queue persistence explicitly.** Scheduler/queue is renderer-memory only; `saved`/scheduled items don't survive a quit. Persist or document as a non-goal in code. diff --git a/src/renderer/src/store/downloads.ts b/src/renderer/src/store/downloads.ts index 700e282..fb5f6a9 100644 --- a/src/renderer/src/store/downloads.ts +++ b/src/renderer/src/store/downloads.ts @@ -252,6 +252,32 @@ const seed: DownloadItem[] = PREVIEW ] : [] +// Side-effects to run exactly once when a download completes: record it to +// history (unless private) and mark its source MediaItem downloaded. Shared by +// the real `applyEvent('done')` path and the preview ticker so the two can't +// drift (M3). +function recordCompletion(item: DownloadItem): void { + if (!item.incognito) { + useHistory.getState().add({ + id: item.id, + title: item.title, + channel: item.channel, + url: item.url, + kind: item.kind, + quality: item.quality, + formatId: item.formatId, + formatHasAudio: item.formatHasAudio, + filePath: item.filePath, + sizeLabel: item.sizeLabel, + thumbnail: item.thumbnail, + completedAt: Date.now() + }) + } + if (item.mediaItemId) { + useSources.getState().markDownloaded(item.mediaItemId, item.filePath) + } +} + // UI-preview only: animate fake progress so the queue feels alive without yt-dlp. function startFakeTicker( id: string, @@ -286,25 +312,7 @@ function startFakeTicker( if (done) { clearInterval(timer) const finished = get().items.find((i) => i.id === id) - if (finished && !finished.incognito) { - useHistory.getState().add({ - id: finished.id, - title: finished.title, - channel: finished.channel, - url: finished.url, - kind: finished.kind, - quality: finished.quality, - filePath: finished.filePath, - sizeLabel: finished.sizeLabel, - thumbnail: finished.thumbnail, - completedAt: Date.now() - }) - } - // Mirror the real applyEvent path so collection completions mark their - // source item downloaded in the preview too. - if (finished?.mediaItemId) { - useSources.getState().markDownloaded(finished.mediaItemId, finished.filePath) - } + if (finished) recordCompletion(finished) get().pump() // a slot just freed -- promote the next queued item } }, 650) @@ -609,29 +617,10 @@ export const useDownloads = create((set, get) => { } }) })) - // Record completed downloads to history (unless this was a private download). + // Record completion (history + source markDownloaded) via the shared helper. if (ev.type === 'done') { const item = get().items.find((i) => i.id === ev.id) - if (item && item.status === 'completed' && !item.incognito) { - useHistory.getState().add({ - id: item.id, - title: item.title, - channel: item.channel, - url: item.url, - kind: item.kind, - quality: item.quality, - formatId: item.formatId, - formatHasAudio: item.formatHasAudio, - filePath: item.filePath, - sizeLabel: item.sizeLabel, - thumbnail: item.thumbnail, - completedAt: Date.now() - }) - } - // Persist media-manager completion so an incremental re-sync skips it. - if (item?.mediaItemId) { - useSources.getState().markDownloaded(item.mediaItemId, item.filePath) - } + if (item && item.status === 'completed') recordCompletion(item) } // A finished item frees a slot -- promote whatever is queued next. if (ev.type === 'done' || ev.type === 'error') pump()