Skip to content

Commit d85e7aa

Browse files
committed
refactor: tighten review findings (download-remove seam + OPML picker)
Adversarial review (3 Codex reviewers) flagged two behavior/seam issues: - OPML import routed through pickFile() inherited the settings-import 5MB cap AND its "too large to be a PodNotes settings file" message. Make the cap and message per-caller (maxBytes/tooLargeMessage options) and give OPML its own copy — keeps the memory-safety cap, drops the wrong text. - M1's remove-download seam was shallow: a caller had to remember to delete the file after removeEpisode() or leak it. Add a composed removeDownloadedEpisode() that owns remove-then-delete and make deleteEpisodeFile module-private (symmetric with createEpisodeFile). The store stays pure; the context menu calls one function. Also trimmed the M1 doc comments. Happy-path behavior unchanged; gates green, 657 tests.
1 parent 29abba2 commit d85e7aa

4 files changed

Lines changed: 48 additions & 32 deletions

File tree

‎src/downloadEpisode.ts‎

Lines changed: 14 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -255,13 +255,21 @@ async function createEpisodeFile({
255255
}
256256

257257
/**
258-
* Delete a downloaded episode's backing file from the vault. The download store
259-
* owns the offline-set state and hands the path here for the actual file I/O,
260-
* keeping vault side effects out of the store layer. Best-effort: a missing or
261-
* already-removed file is a no-op, and failures are logged rather than thrown so
262-
* removing a stale entry never breaks the calling UI flow.
258+
* Remove a downloaded episode: drop it from the offline set and delete its
259+
* backing vault file. This composes the pure store removal with the file I/O so
260+
* callers can't do one without the other (and leak files); the download store
261+
* stays free of vault side effects.
263262
*/
264-
export async function deleteEpisodeFile(filePath: string): Promise<void> {
263+
export async function removeDownloadedEpisode(episode: Episode): Promise<void> {
264+
const removedFilePath = downloadedEpisodes.removeEpisode(episode);
265+
if (removedFilePath) {
266+
await deleteEpisodeFile(removedFilePath);
267+
}
268+
}
269+
270+
// Best-effort: a missing/already-removed file is a no-op, and failures are logged
271+
// rather than thrown so removing a stale entry never breaks the calling UI flow.
272+
async function deleteEpisodeFile(filePath: string): Promise<void> {
265273
if (!filePath) return;
266274

267275
try {

‎src/store/downloads.ts‎

Lines changed: 2 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -49,10 +49,8 @@ export const downloadedEpisodes = (() => {
4949
},
5050
/**
5151
* Drops an episode from the offline set and returns the vault path of the
52-
* file it backed (or `undefined` if it wasn't tracked). This store is the
53-
* pure state core, so it never touches the vault itself — the caller deletes
54-
* the returned file via `deleteEpisodeFile` in the download module. Mirrors
55-
* how #211 split the queue's automation side effect out of persistence.
52+
* file it backed (or `undefined` if it wasn't tracked). Pure state only: the
53+
* caller deletes the returned file (see `removeDownloadedEpisode`).
5654
*/
5755
removeEpisode: (episode: Episode): string | undefined => {
5856
let removedFilePath: string | undefined;

‎src/ui/PodcastView/spawnEpisodeContextMenu.ts‎

Lines changed: 2 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,7 @@ import { Menu, Notice } from "obsidian";
22
import createPodcastNote, { getPodcastNote, openPodcastNote } from "src/createPodcastNote";
33
import createFeedNote, { getFeedNote, openFeedNote } from "src/createFeedNote";
44
import downloadEpisodeWithProgessNotice, {
5-
deleteEpisodeFile,
5+
removeDownloadedEpisode,
66
} from "src/downloadEpisode";
77
import { currentEpisode, downloadedEpisodes, favorites, playedEpisodes, playlists, plugin, queue, savedFeeds, viewState } from "src/store";
88
import type { Episode } from "src/types/Episode";
@@ -70,10 +70,7 @@ export default function spawnEpisodeContextMenu(
7070
.setTitle(isDownloaded ? "Remove file" : "Download")
7171
.onClick(() => {
7272
if (isDownloaded) {
73-
const removedFilePath = downloadedEpisodes.removeEpisode(episode);
74-
if (removedFilePath) {
75-
void deleteEpisodeFile(removedFilePath);
76-
}
73+
void removeDownloadedEpisode(episode);
7774
} else {
7875
// The path template always yields a per-episode file via
7976
// safeDownloadBasename (#183), so no empty-path guard is needed —

‎src/ui/settings/PodNotesSettingsTab.ts‎

Lines changed: 30 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -528,17 +528,21 @@ export class PodNotesSettingsTab extends PluginSettingTab {
528528
.setDesc("Import podcasts from an OPML file.")
529529
.addButton((button) =>
530530
button.setButtonText("Import").onClick(() => {
531-
this.pickFile(".opml", async (contents) => {
532-
try {
533-
await importOPML(contents);
534-
} catch (e) {
535-
console.error("Error importing OPML:", e);
536-
new Notice(
537-
`Error importing OPML: ${e instanceof Error ? e.message : "Unknown error"}`,
538-
10000,
539-
);
540-
}
541-
});
531+
this.pickFile(
532+
".opml",
533+
async (contents) => {
534+
try {
535+
await importOPML(contents);
536+
} catch (e) {
537+
console.error("Error importing OPML:", e);
538+
new Notice(
539+
`Error importing OPML: ${e instanceof Error ? e.message : "Unknown error"}`,
540+
10000,
541+
);
542+
}
543+
},
544+
{ tooLargeMessage: "That file is too large to be an OPML file." },
545+
);
542546
}),
543547
);
544548

@@ -625,7 +629,19 @@ export class PodNotesSettingsTab extends PluginSettingTab {
625629
);
626630
}
627631

628-
private pickFile(accept: string, onContents: (contents: string) => void): void {
632+
private pickFile(
633+
accept: string,
634+
onContents: (contents: string) => void,
635+
options: { maxBytes?: number; tooLargeMessage?: string } = {},
636+
): void {
637+
// Both pickers read small text files, so cap the size before reading the
638+
// whole thing into memory. The cap and its message are per-caller so OPML
639+
// import doesn't inherit the settings-file copy.
640+
const maxBytes = options.maxBytes ?? 5 * 1024 * 1024;
641+
const tooLargeMessage =
642+
options.tooLargeMessage ??
643+
"That file is too large to be a PodNotes settings file.";
644+
629645
const fileInput = document.createElement("input");
630646
fileInput.type = "file";
631647
fileInput.accept = accept;
@@ -646,11 +662,8 @@ export class PodNotesSettingsTab extends PluginSettingTab {
646662
return;
647663
}
648664

649-
// A settings file is small JSON; reject anything implausibly large
650-
// before reading it fully into memory.
651-
const MAX_BYTES = 5 * 1024 * 1024;
652-
if (file.size > MAX_BYTES) {
653-
new Notice("That file is too large to be a PodNotes settings file.");
665+
if (file.size > maxBytes) {
666+
new Notice(tooLargeMessage);
654667
return;
655668
}
656669

0 commit comments

Comments
 (0)