ffmpeg setting, nag toggle, download persistence setting - #15
Conversation
for download settings.
There was a problem hiding this comment.
Code Review
This pull request introduces the ability to configure a custom FFmpeg directory, adds a setting to toggle confirmation before clearing completed downloads, and persists extra download options as defaults for future tasks. The review feedback highlights three key issues: first, initializing backend settings only when mounting the settings page can leave them uninitialized if the user bypasses that page; second, the asynchronous watcher for the FFmpeg directory lacks error handling; and third, passive watchers on extra options can inadvertently overwrite global defaults when switching between pending items, which should be resolved by using active event-driven updates instead.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| appVersion.value = await getVersion(); | ||
| await applyBinaryPathResolveMode(); | ||
| await applyYoutubeExtractorArgs(); | ||
| await applyFfmpegDir(); | ||
| refreshAll(); | ||
| }); |
There was a problem hiding this comment.
Issue: Currently, applyFfmpegDir() (and other apply functions like applyYoutubeExtractorArgs()) are only called when the Settings.vue component is mounted. If a user launches the app and immediately starts a download or uses tools without visiting the Settings page, the backend's static FFMPEG_DIR (and YOUTUBE_EXTRACTOR_ARGS) will remain uninitialized (empty), meaning their saved custom settings won't take effect.
Recommendation: Move these initialization calls to a global startup location (such as App.vue's onMounted or main.ts after Pinia store hydration). This ensures the backend is always synchronized with the persisted frontend settings on startup, regardless of which page the user visits first.
| watch( | ||
| () => settingStore.ffmpegDir, | ||
| async () => { | ||
| await applyFfmpegDir(); | ||
| }, | ||
| ); |
There was a problem hiding this comment.
Issue: The watcher for settingStore.ffmpegDir calls applyFfmpegDir() asynchronously without any error handling. If the backend invocation fails (e.g., due to a poisoned lock or invalid path), it will result in an unhandled promise rejection.
Recommendation: Wrap the call in a try-catch block and gracefully handle the error, such as showing an error message to the user using window.$message.error.
watch(
() => settingStore.ffmpegDir,
async () => {
try {
await applyFfmpegDir();
} catch (e) {
window.$message.error(t("common.error", { e }));
}
},
);
| watch(embedSubs, (v) => (settingStore.embedSubs = v)); | ||
| watch(embedThumbnail, (v) => (settingStore.embedThumbnail = v)); | ||
| watch(embedMetadata, (v) => (settingStore.embedMetadata = v)); | ||
| watch(embedChapters, (v) => (settingStore.embedChapters = v)); | ||
| watch(sponsorblockRemove, (v) => (settingStore.sponsorblockRemove = v)); | ||
| watch(extractAudio, (v) => (settingStore.extractAudio = v)); | ||
| watch(audioConvertFormat, (v) => (settingStore.audioConvertFormat = v)); | ||
| watch(noMerge, (v) => (settingStore.noMerge = v)); | ||
| watch(recodeFormat, (v) => (settingStore.recodeFormat = v)); | ||
| watch(limitRate, (v) => (settingStore.limitRate = v)); | ||
| watch(ffmpegArgs, (v) => (settingStore.ffmpegArgs = v)); |
There was a problem hiding this comment.
Issue: Watching the model values (like embedSubs, embedThumbnail, etc.) to directly update settingStore causes the global defaults to be overwritten whenever the user switches between existing pending items. For example, if Item A has embedSubs = true and Item B has embedSubs = false, simply clicking on Item B to view it will trigger these watchers and overwrite the global default to false. This makes the default settings for new tasks highly unpredictable and confusing.
Recommendation: Remove these passive watchers from the script. Instead, update the global settingStore only when the user actively interacts with the UI components. You can achieve this by listening to the @update:value or @update:checked events on the Naive UI components in the template.
For example, in the template:
<n-checkbox
v-model:checked="embedSubs"
@update:checked="(val) => settingStore.embedSubs = val"
size="small"
>
{{ $t("detail.embedSubs") }}
</n-checkbox>This ensures settingStore is only updated on active user interaction, while programmatic updates (like switching active items) will safely update the local model without affecting the global defaults.
// 记住额外选项的选择,作为下次新任务的默认值(时间裁剪范围为单次任务专属,不记忆)
// 建议移除这些被动监听器,改在模板中使用 @update:checked / @update:value 触发更新,以避免切换任务时意外覆写全局默认值。
|
Hi @imsyy , sorry to bother you. When you have some free time, could you please take a look at this PR? No rush at all, thank you for your time and great work on this project! |
Added setting for ffmpeg location, toggle for clear nag, and persistence for download settings.