From 6cb9c5ca472446f6d2e3b84fcf94aa12c1eb38ef Mon Sep 17 00:00:00 2001 From: debont80 Date: Sun, 5 Jul 2026 14:39:45 -0400 Subject: [PATCH] fix(downloads): re-resolve options on retry instead of replaying stale ones A queued item freezes its post-processing options at enqueue time, so retrying a failed download replayed whatever was in effect when it was first added -- a settings change made afterward (e.g. turning SponsorBlock-remove off, which forces a slow, failure-prone software re-encode) never took effect on retry. retry/retryAll now re-resolve a source-tied item's options from the current source profile / global settings (the same precedence enqueue uses) before requeuing it. Manual pastes with no source keep their explicit per-download options untouched, and this is deliberately distinct from the history re-download path, which keeps frozen options on purpose to reproduce an identical file. --- src/renderer/src/store/downloads.ts | 62 +++++++++++++++---------- src/renderer/src/store/retryOptions.ts | 51 +++++++++++++++++++++ test/retryOptions.test.ts | 63 ++++++++++++++++++++++++++ 3 files changed, 152 insertions(+), 24 deletions(-) create mode 100644 src/renderer/src/store/retryOptions.ts create mode 100644 test/retryOptions.test.ts diff --git a/src/renderer/src/store/downloads.ts b/src/renderer/src/store/downloads.ts index f5cbecc..87891ed 100644 --- a/src/renderer/src/store/downloads.ts +++ b/src/renderer/src/store/downloads.ts @@ -5,7 +5,10 @@ import { logError } from '../reportError' import { revealFile } from '../reveal' import { useSettings } from './settings' import { useHistory } from './history' +import { useSources } from './sources' +import { useProfiles } from './profiles' import { emit, on } from './coordinator' +import { pickRetryOptions } from './retryOptions' import { RESOLVING, buildItem } from './downloadItem' import { fromPersisted, persistableItems } from './queuePersist' import { seed } from './downloadSeed' @@ -27,6 +30,39 @@ export type { MediaKind, ChosenFormat, DownloadItem, DownloadStatus, AddOptions, // we've loaded it back. let queueHydrated = false +// Reset a failed/canceled item back to 'queued' for a fresh spawn, re-resolving +// its post-processing option group from the CURRENT source profile / global +// settings first (pickRetryOptions). Without this, a retry replays the options +// frozen when the item was queued, so a settings change made in between — e.g. +// turning SponsorBlock-remove off — never takes effect. Reading the sources / +// profiles stores here is a one-way lookup (they never import downloads, so no +// cycle); manual (non-source) items resolve to null and keep their explicit +// per-download options untouched. +function requeueForRetry(item: DownloadItem): DownloadItem { + const settings = useSettings.getState() + const fresh = pickRetryOptions( + item.mediaItemId, + useSources.getState().sources, + useProfiles.getState().profiles, + { + kind: settings.defaultKind, + videoQuality: settings.defaultVideoQuality, + audioQuality: settings.defaultAudioQuality, + options: settings.downloadOptions, + outputTemplate: settings.collectionOutputTemplate + } + ) + return { + ...item, + ...(fresh ?? {}), + status: 'queued', + progress: 0, + error: undefined, + speedBytesPerSec: undefined, + etaSeconds: undefined + } +} + // 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 @@ -213,36 +249,14 @@ export const useDownloads = create((set, get) => { retry: (id) => { set((s) => ({ - items: s.items.map((i) => - i.id === id - ? { - ...i, - status: 'queued', - progress: 0, - error: undefined, - speedBytesPerSec: undefined, - etaSeconds: undefined - } - : i - ) + items: s.items.map((i) => (i.id === id ? requeueForRetry(i) : i)) })) pump() }, retryAll: () => { set((s) => ({ - items: s.items.map((i) => - i.status === 'error' - ? { - ...i, - status: 'queued', - progress: 0, - error: undefined, - speedBytesPerSec: undefined, - etaSeconds: undefined - } - : i - ) + items: s.items.map((i) => (i.status === 'error' ? requeueForRetry(i) : i)) })) pump() }, diff --git a/src/renderer/src/store/retryOptions.ts b/src/renderer/src/store/retryOptions.ts new file mode 100644 index 0000000..5380a1a --- /dev/null +++ b/src/renderer/src/store/retryOptions.ts @@ -0,0 +1,51 @@ +import type { Source, MediaProfile } from '@shared/ipc' +import { resolveProfile, type ProfileDefaults } from '@shared/profileResolve' +import type { DownloadItem } from './downloadTypes' + +/** The per-download group a retry re-resolves from current config. */ +export type RetryOptionGroup = Pick< + DownloadItem, + 'options' | 'extraArgs' | 'collectionOutputTemplate' +> + +/** + * Re-resolve a failed download's post-processing option group from the CURRENT + * source profile / global settings, using the same precedence enqueue does + * (resolveProfile). This is what lets a retry pick up a settings change made + * after the item was queued — e.g. turning SponsorBlock-remove off — instead of + * replaying the options frozen at add time. + * + * Only downloads tied to a Library source are re-resolved: their options always + * derived from the source/global config in the first place. `mediaItemId` is + * `:`, so the source id is its prefix. Returned as a partial so + * the caller can spread it over the item. + * + * - no mediaItemId (a manual paste) → null: it carried the user's explicit + * per-download choices, so retry must not overwrite them. + * - source since deleted → null: nothing to resolve against; keep + * whatever options the item already had. + * - otherwise → the profile's group, or the globals when + * the source points at no (or a dangling) profile. + * + * Distinct from the history re-download path, which deliberately KEEPS an item's + * frozen options to reproduce an identical file (L87); a retry's goal is instead + * to get a failed download to succeed under the user's current configuration. + */ +export function pickRetryOptions( + mediaItemId: string | undefined, + sources: Source[], + profiles: MediaProfile[], + defaults: ProfileDefaults +): RetryOptionGroup | null { + const sourceId = mediaItemId?.split(':')[0] + if (!sourceId) return null + const src = sources.find((s) => s.id === sourceId) + if (!src) return null + const profile = src.profileId ? (profiles.find((p) => p.id === src.profileId) ?? null) : null + const resolved = resolveProfile(profile, defaults) + return { + options: resolved.options, + extraArgs: resolved.extraArgs || undefined, + collectionOutputTemplate: resolved.outputTemplate || undefined + } +} diff --git a/test/retryOptions.test.ts b/test/retryOptions.test.ts new file mode 100644 index 0000000..5774550 --- /dev/null +++ b/test/retryOptions.test.ts @@ -0,0 +1,63 @@ +import { describe, it, expect } from 'vitest' +import { pickRetryOptions } from '../src/renderer/src/store/retryOptions' +import { DEFAULT_DOWNLOAD_OPTIONS, type Source, type MediaProfile } from '../src/shared/ipc' +import type { ProfileDefaults } from '../src/shared/profileResolve' + +const source = (over: Partial = {}): Source => ({ + id: 'ac8e6608', + url: 'https://youtube.com/@NetworkChuck', + kind: 'channel', + title: 'NetworkChuck', + channel: 'NetworkChuck', + addedAt: 0, + itemCount: 0, + ...over +}) + +// Global defaults with SponsorBlock OFF — the state after the user toggled it. +const defaults: ProfileDefaults = { + kind: 'video', + videoQuality: '1080p', + audioQuality: 'Best', + options: { ...DEFAULT_DOWNLOAD_OPTIONS, sponsorBlock: false }, + outputTemplate: '' +} + +describe('pickRetryOptions', () => { + it('re-resolves a source-tied item to the current global options (SponsorBlock now off)', () => { + const group = pickRetryOptions('ac8e6608:qkUTB65OfAc', [source()], [], defaults) + expect(group?.options?.sponsorBlock).toBe(false) + expect(group?.extraArgs).toBeUndefined() + expect(group?.collectionOutputTemplate).toBeUndefined() + }) + + it('returns null for a manual paste (no mediaItemId) so explicit options are kept', () => { + expect(pickRetryOptions(undefined, [source()], [], defaults)).toBeNull() + }) + + it('returns null when the source has since been deleted', () => { + expect(pickRetryOptions('deleted99:vid', [source()], [], defaults)).toBeNull() + }) + + it("prefers the source's Media Profile options over the globals", () => { + const profile: MediaProfile = { + id: 'p1', + name: 'Archive', + options: { ...DEFAULT_DOWNLOAD_OPTIONS, sponsorBlock: true, sponsorBlockMode: 'remove' }, + extraArgs: '--no-mtime' + } + const group = pickRetryOptions( + 'ac8e6608:vid', + [source({ profileId: 'p1' })], + [profile], + defaults + ) + expect(group?.options?.sponsorBlock).toBe(true) + expect(group?.extraArgs).toBe('--no-mtime') + }) + + it('falls back to the globals when the source points at a dangling profile id', () => { + const group = pickRetryOptions('ac8e6608:vid', [source({ profileId: 'gone' })], [], defaults) + expect(group?.options?.sponsorBlock).toBe(false) + }) +})