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.
This commit is contained in:
@@ -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<DownloadState>((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()
|
||||
},
|
||||
|
||||
@@ -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
|
||||
* `<sourceId>:<videoId>`, 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
|
||||
}
|
||||
}
|
||||
@@ -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> = {}): 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)
|
||||
})
|
||||
})
|
||||
Reference in New Issue
Block a user