fix(download): default per-episode download path and migrate empty default (#183) (#186)

download.path defaulted to "" (empty), so the Download command resolved every
episode to ".mp3" at the vault root — a dotfile Obsidian never indexes. The first
download wrote hidden junk and the second threw "File already exists", blocking
all further downloads until the user manually deleted the dotfile.

- constants: default download.path is now "PodNotes/{{podcast}}/{{title}}".
- settingsMigrations: on load, upgrade a persisted empty/absent path to the new
  default. Only the exact legacy "" (and null/undefined) is migrated, so a custom
  path is preserved. Needed because the whole settings object is persisted, so
  existing users already have download.path:"" overriding the new default.
- main: apply the migration in loadSettings and rebuild settings.download via
  spread so the shared DEFAULT_SETTINGS.download is never mutated.
- downloadEpisode: safeDownloadBasename guarantees a per-episode file name even
  for a misconfigured (empty/trailing-slash) template — never writes ".<ext>".
- settings UI: warn inline when the configured path can't resolve {{title}}.
- context menu: drop the now-stale empty-path guard and rely on the safety net,
  so the menu and the Download command behave consistently.
- docs + tests (migration, safe basename, default).

Author: chhoumann

docs/docs/commands.md

@@ -23,7 +23,7 @@ This will skip the current episode forward by the amount of seconds specified in
 ## Download Playing Episode
 This will download the currently playing episode.
 
-Downloads are stored in the location specified in settings.
+Downloads are stored in the location specified by the **Episode download path** setting. This path is a template and **must include a per-episode token** such as `{{title}}` (the default is `PodNotes/{{podcast}}/{{title}}`). A path without `{{title}}` makes every episode resolve to the same file, so downloads overwrite each other or fail. The file extension is added automatically — do not include one.
 
 ## Reload Podnotes
 This will reload PodNotes.

src/constants.ts

@@ -68,7 +68,11 @@ export const DEFAULT_SETTINGS: IPodNotesSettings = {
 	},
 
 	download: {
-		path: "",
+		// Must include a per-episode token ({{title}}). An empty or token-less path
+		// resolves every episode to the same name — e.g. "" -> ".mp3", a hidden
+		// dotfile at the vault root that Obsidian never indexes — so the first
+		// download writes junk and the second throws "File already exists" (#183).
+		path: "PodNotes/{{podcast}}/{{title}}",
 	},
 	downloadedEpisodes: {},
 	localFiles: {

src/downloadEpisode.test.ts

@@ -5,6 +5,7 @@ import {
 	detectAudioFileExtension,
 	downloadEpisode,
 	getEpisodeAudioBuffer,
+	safeDownloadBasename,
 } from "./downloadEpisode";
 import { downloadedEpisodes } from "./store";
 import type { Episode } from "./types/Episode";
@@ -128,6 +129,52 @@ describe("downloadEpisode (API path)", () => {
 		expect(requestUrlMock).not.toHaveBeenCalled();
 		expect(createBinary).not.toHaveBeenCalled();
 	});
+
+	it("never writes a '.<ext>' dotfile when the path template is empty (#183)", async () => {
+		const { createBinary } = setupVault();
+		const buffer = bytes(0x49, 0x44, 0x33, 0x01);
+		requestUrlMock.mockResolvedValue({
+			status: 200,
+			headers: { "content-type": "audio/mpeg" },
+			arrayBuffer: buffer,
+		} as unknown as Awaited<ReturnType<typeof requestUrl>>);
+
+		const path = await downloadEpisode(makeEpisode(), "");
+
+		// Falls back to a per-episode name instead of the un-indexable ".mp3".
+		expect(path).toBe("My Title.mp3");
+		expect(createBinary).toHaveBeenCalledWith("My Title.mp3", buffer);
+		const [writtenPath] = createBinary.mock.calls[0];
+		expect(writtenPath).not.toBe(".mp3");
+	});
+});
+
+describe("safeDownloadBasename (#183)", () => {
+	it("falls back to the title when the template resolves to an empty name", () => {
+		expect(safeDownloadBasename("", makeEpisode())).toBe("My Title");
+	});
+
+	it("replaces an empty trailing segment but keeps the folder", () => {
+		expect(safeDownloadBasename("Downloads/", makeEpisode())).toBe(
+			"Downloads/My Title",
+		);
+	});
+
+	it("drops a stray leading slash instead of writing an absolute path", () => {
+		expect(safeDownloadBasename("/{{title}}", makeEpisode())).toBe("My Title");
+	});
+
+	it("falls back to 'episode' when the title is empty/all-illegal", () => {
+		expect(safeDownloadBasename("", makeEpisode({ title: "??" }))).toBe(
+			"episode",
+		);
+	});
+
+	it("leaves a valid per-episode template untouched", () => {
+		expect(
+			safeDownloadBasename("podcast/{{podcast}}/{{title}}", makeEpisode()),
+		).toBe("podcast/Pod/My Title");
+	});
 });
 
 describe("getEpisodeAudioBuffer (issue #107)", () => {

src/downloadEpisode.ts

@@ -169,6 +169,36 @@ function createNoticeDoc(title: string) {
 	};
 }
 
+/**
+ * Resolves the on-disk basename for a downloaded episode, guaranteeing a
+ * per-episode file name even when the template is misconfigured.
+ *
+ * A template that resolves to an empty final segment — the legacy empty default
+ * `""`, or any path ending in `/` — would otherwise produce a hidden `".<ext>"`
+ * dotfile at the vault root that Obsidian never indexes, so the first download
+ * silently writes junk and the second throws "File already exists" (#183). In
+ * that case we fall back to the episode title (then a literal "episode" when the
+ * title is empty or all-illegal). Leading/interior empty segments are dropped so
+ * a stray slash can never yield an absolute-looking or double-slashed path.
+ */
+export function safeDownloadBasename(
+	downloadPathTemplate: string,
+	episode: Episode,
+): string {
+	const resolved = DownloadPathTemplateEngine(downloadPathTemplate, episode);
+	const segments = resolved.split("/");
+	const lastIndex = segments.length - 1;
+
+	if (segments[lastIndex].trim() === "") {
+		segments[lastIndex] =
+			replaceIllegalFileNameCharactersInString(episode.title) || "episode";
+	}
+
+	return segments
+		.filter((segment, index) => index === lastIndex || segment.trim() !== "")
+		.join("/");
+}
+
 async function createEpisodeFile({
 	episode,
 	downloadPathTemplate,
@@ -180,7 +210,7 @@ async function createEpisodeFile({
 	data: ArrayBuffer;
 	extension: string;
 }) {
-	const basename = DownloadPathTemplateEngine(downloadPathTemplate, episode);
+	const basename = safeDownloadBasename(downloadPathTemplate, episode);
 	const filePath = `${basename}.${extension}`;
 
 	// `createBinary` throws if a parent folder is missing, which previously left
@@ -291,7 +321,7 @@ export async function downloadEpisode(
 		return localFilePath;
 	}
 
-	const basename = DownloadPathTemplateEngine(downloadPathTemplate, episode);
+	const basename = safeDownloadBasename(downloadPathTemplate, episode);
 	const fileExtension = await getFileExtension(episode.streamUrl);
 	const filePath = `${basename}.${fileExtension}`;
 

src/main.ts

@@ -14,6 +14,7 @@ import { Plugin, type WorkspaceLeaf } from "obsidian";
 import { API } from "src/API/API";
 import type { IAPI } from "src/API/IAPI";
 import { DEFAULT_SETTINGS, VIEW_TYPE } from "src/constants";
+import { migrateDownloadPath } from "src/settingsMigrations";
 import { PodNotesSettingsTab } from "src/ui/settings/PodNotesSettingsTab";
 import { MainView } from "src/ui/PodcastView";
 import { QueueReorderModal } from "src/ui/QueueReorderModal";
@@ -411,6 +412,15 @@ export default class PodNotes extends Plugin implements IPodNotes {
 			...DEFAULT_SETTINGS.timestamp,
 			...(loadedData?.timestamp ?? {}),
 		};
+		// Build a fresh download object so we never mutate the shared
+		// DEFAULT_SETTINGS.download, then migrate the legacy empty default (#183).
+		this.settings.download = {
+			...DEFAULT_SETTINGS.download,
+			...(loadedData?.download ?? {}),
+		};
+		this.settings.download.path = migrateDownloadPath(
+			this.settings.download.path,
+		);
 	}
 
 	async saveSettings() {

src/settingsMigrations.test.ts

@@ -0,0 +1,49 @@
+import { describe, expect, it } from "vitest";
+import { DEFAULT_SETTINGS } from "./constants";
+import {
+	LEGACY_EMPTY_DOWNLOAD_PATH,
+	migrateDownloadPath,
+} from "./settingsMigrations";
+
+describe("download path default (#183)", () => {
+	it("is non-empty and contains a per-episode {{title}} token", () => {
+		expect(DEFAULT_SETTINGS.download.path).not.toBe("");
+		expect(DEFAULT_SETTINGS.download.path).toMatch(/\{\{\s*title\b/i);
+	});
+
+	it("groups by podcast", () => {
+		expect(DEFAULT_SETTINGS.download.path).toMatch(/\{\{\s*podcast\b/i);
+	});
+});
+
+describe("migrateDownloadPath (#183)", () => {
+	it("upgrades the legacy empty default to the current per-episode default", () => {
+		expect(migrateDownloadPath(LEGACY_EMPTY_DOWNLOAD_PATH)).toBe(
+			DEFAULT_SETTINGS.download.path,
+		);
+		expect(migrateDownloadPath("")).toBe(DEFAULT_SETTINGS.download.path);
+	});
+
+	it("treats an absent value (undefined/null) as the legacy default", () => {
+		expect(migrateDownloadPath(undefined)).toBe(
+			DEFAULT_SETTINGS.download.path,
+		);
+		// null is reachable via a corrupted/hand-edited data.json; mapping it to the
+		// default also keeps null out of DownloadPathTemplateEngine (would crash).
+		expect(migrateDownloadPath(null)).toBe(DEFAULT_SETTINGS.download.path);
+	});
+
+	it("preserves any non-empty custom path verbatim", () => {
+		expect(migrateDownloadPath("inputs/{{podcast}} - {{title}}")).toBe(
+			"inputs/{{podcast}} - {{title}}",
+		);
+		// Even an unusual (token-less) path the user chose is preserved — the
+		// migration only ever touches the exact legacy empty value.
+		expect(migrateDownloadPath("Downloads")).toBe("Downloads");
+	});
+
+	it("is idempotent on the current default", () => {
+		const once = migrateDownloadPath(DEFAULT_SETTINGS.download.path);
+		expect(migrateDownloadPath(once)).toBe(DEFAULT_SETTINGS.download.path);
+	});
+});

src/settingsMigrations.ts

@@ -0,0 +1,51 @@
+import { DEFAULT_SETTINGS } from "./constants";
+
+/**
+ * Settings migrations applied when settings are loaded from disk.
+ *
+ * These run in `PodNotes.loadSettings` after the persisted `data.json` is merged
+ * over `DEFAULT_SETTINGS`. Changing a default alone is NOT enough for existing
+ * users: the plugin persists the whole settings object, so anyone who has used
+ * PodNotes already has the old value written to `data.json`, which overrides the
+ * new default on load. A migration rewrites that stored value in memory so the
+ * change actually reaches existing users (it is re-persisted on the next save).
+ *
+ * This module is pure (no Obsidian/UI dependencies) so the risky rewrite logic is
+ * unit-testable.
+ */
+
+/**
+ * The download path used to default to "" (empty). With an empty template the
+ * Download command resolved every episode to ".mp3" at the vault root — a dotfile
+ * Obsidian never indexes, so the first download wrote junk and the second threw
+ * "File already exists" (issue #183). Users who never customized the path have
+ * this exact value persisted, so simply changing the default does not help them.
+ */
+export const LEGACY_EMPTY_DOWNLOAD_PATH = "";
+
+/**
+ * Upgrades the legacy empty download path to the current per-episode default.
+ *
+ * ONLY the exact legacy empty value (or an absent value) is migrated, so any
+ * non-empty path the user deliberately configured is preserved untouched — even
+ * an unusual one. An empty path is always broken (it can only ever write the
+ * ".mp3" dotfile), so replacing it is strictly an improvement.
+ *
+ * `null`/`undefined` are treated as absence (a missing key, or a corrupted /
+ * hand-edited data.json), not as a configured path: they map to the default both
+ * to apply the intended value and to keep a `null` from reaching
+ * DownloadPathTemplateEngine, where `null.replace(...)` would throw.
+ */
+export function migrateDownloadPath(
+	storedPath: string | null | undefined,
+): string {
+	if (
+		storedPath === undefined ||
+		storedPath === null ||
+		storedPath === LEGACY_EMPTY_DOWNLOAD_PATH
+	) {
+		return DEFAULT_SETTINGS.download.path;
+	}
+
+	return storedPath;
+}

src/ui/PodcastView/spawnEpisodeContextMenu.ts

@@ -68,13 +68,13 @@ export default function spawnEpisodeContextMenu(
 				if (isDownloaded) {
 					downloadedEpisodes.removeEpisode(episode, true);
 				} else {
-					const downloadPath = get(plugin).settings.download.path;
-					if (!downloadPath) {
-						new Notice(`Please set a download path in the settings.`);
-						return;
-					}
-
-					downloadEpisodeWithProgessNotice(episode, downloadPath);
+					// The path template always yields a per-episode file via
+					// safeDownloadBasename (#183), so no empty-path guard is needed —
+					// this matches the Download command in main.ts.
+					downloadEpisodeWithProgessNotice(
+						episode,
+						get(plugin).settings.download.path,
+					);
 				}
 			}));
 	}

src/ui/settings/PodNotesSettingsTab.ts

@@ -322,15 +322,7 @@ export class PodNotesSettingsTab extends PluginSettingTab {
 				textComponent.onChange((value) => {
 					this.plugin.settings.download.path = value;
 					this.plugin.saveSettings();
-
-					const demoVal = DownloadPathTemplateEngine(value, randomEpisode);
-					downloadFilePathDemoEl.empty();
-					MarkdownRenderer.renderMarkdown(
-						`${demoVal}.mp3`,
-						downloadFilePathDemoEl,
-						"",
-						new Component(),
-					);
+					refreshDownloadPathHints(value);
 				});
 				textComponent.inputEl.style.width = "100%";
 			});
@@ -340,6 +332,33 @@ export class PodNotesSettingsTab extends PluginSettingTab {
 		downloadPathSetting.settingEl.style.gap = "10px";
 
 		const downloadFilePathDemoEl = container.createDiv();
+		const downloadFilePathWarningEl = container.createDiv();
+		downloadFilePathWarningEl.style.color = "var(--text-error)";
+
+		// A download path without a per-episode token ({{title}}) resolves every
+		// episode to the same file, so downloads overwrite each other or fail; an
+		// empty path resolves to ".mp3" at the vault root (#183). Warn inline.
+		const refreshDownloadPathHints = (value: string) => {
+			const demoVal = DownloadPathTemplateEngine(value, randomEpisode);
+			downloadFilePathDemoEl.empty();
+			MarkdownRenderer.renderMarkdown(
+				`${demoVal}.mp3`,
+				downloadFilePathDemoEl,
+				"",
+				new Component(),
+			);
+
+			// Match only the forms DownloadPathTemplateEngine actually resolves —
+			// {{title}} or {{title:...}}. A looser test (e.g. \s* after {{, or \b)
+			// would wrongly stay silent for "{{ title }}" / "{{title-ish}}", which the
+			// engine leaves literal so every episode still collides.
+			downloadFilePathWarningEl.toggle(!/\{\{title(:[^}]*)?\}\}/i.test(value));
+			downloadFilePathWarningEl.setText(
+				"⚠ This path has no {{title}}, so multiple episodes can resolve to the same file — downloads will overwrite each other or fail. Add {{title}}, e.g. PodNotes/{{podcast}}/{{title}}.",
+			);
+		};
+
+		refreshDownloadPathHints(this.plugin.settings.download.path);
 	}
 
 	private addPerformanceSettings(container: HTMLDivElement) {