From c274acca637e8c1ff9ea9b7110866653bd1f1e60 Mon Sep 17 00:00:00 2001 From: Christian Bager Bach Houmann Date: Mon, 29 Jun 2026 08:56:06 +0200 Subject: [PATCH] fix(opml): correct import progress math and saved-count reporting When every feed in an imported OPML is already subscribed, newPodcastsToAdd.length is 0, so updateProgress computed (0 / 0) * 100 = NaN and flashed "Importing... 0/0 podcasts completed (NaN%)". Guard the zero denominator and show "No new podcasts to import." instead. The save loop keys feeds by title and skips collisions, but the completion message reported validPodcasts.length (the number fetched), over-reporting "saved" when two imported feeds share a title or an imported feed's title matches an existing saved feed. Count the feeds actually written and report dropped duplicate-title collisions separately so the summary reflects what was stored. Resolves deepsec finding other-logic-bug (922f3003f7). --- src/opml.test.ts | 147 +++++++++++++++++++++++++++++++++++++++++++++-- src/opml.ts | 35 ++++++++--- 2 files changed, 168 insertions(+), 14 deletions(-) diff --git a/src/opml.test.ts b/src/opml.test.ts index e3ec161e..9876e665 100644 --- a/src/opml.test.ts +++ b/src/opml.test.ts @@ -6,11 +6,17 @@ import type { PodcastFeed } from "./types/PodcastFeed"; // parse/validate/serialize logic: FeedParser would otherwise make real network // requests for every imported feed, and savedFeeds pulls in the whole store. const savedFeeds = writable>({}); -const getFeed = vi.fn(async (url: string) => ({ +const defaultGetFeed = async (url: string) => ({ title: `Feed for ${url}`, url, artworkUrl: "", -})); +}); +const getFeed = vi.fn(defaultGetFeed); + +// Every string the import flow renders into the progress Notice is recorded +// here so tests can assert on what the user actually sees (e.g. no "NaN%", an +// accurate saved count). +const noticeMessages = vi.hoisted(() => [] as string[]); vi.mock("./store", () => ({ get savedFeeds() { @@ -29,8 +35,12 @@ vi.mock("./parser/feedParser", () => ({ // uses so the import flow can run to completion under jsdom. vi.mock("obsidian", () => ({ Notice: class { - constructor(public message?: unknown) {} - setMessage() {} + constructor(message?: unknown) { + if (typeof message === "string") noticeMessages.push(message); + } + setMessage(message?: unknown) { + if (typeof message === "string") noticeMessages.push(message); + } hide() {} }, })); @@ -39,7 +49,9 @@ import { exportOPML, importOPML } from "./opml"; beforeEach(() => { savedFeeds.set({}); - getFeed.mockClear(); + getFeed.mockReset(); + getFeed.mockImplementation(defaultGetFeed); + noticeMessages.length = 0; }); /** Minimal Obsidian App stub capturing the single file vault.create writes. */ @@ -189,4 +201,129 @@ describe("importOPML (IE-01)", () => { expect(getFeed).not.toHaveBeenCalled(); expect(Object.keys(get(savedFeeds))).toEqual(["Existing"]); }); + + it("never renders 'NaN%' when every feed is already subscribed", async () => { + savedFeeds.set({ + Existing: { + title: "Existing", + url: "https://dup.test/feed", + artworkUrl: "", + }, + }); + + const opml = + "" + + '' + + ""; + + await importOPML(opml); + + // Nothing to fetch, so the 0/0 progress division must not surface as NaN. + expect(getFeed).not.toHaveBeenCalled(); + expect(noticeMessages.some((m) => m.includes("NaN"))).toBe(false); + expect(noticeMessages.some((m) => m.includes("Saved 0 new podcasts"))).toBe( + true, + ); + }); + + it("reports the saved count, not the fetched count, on duplicate titles", async () => { + // Two distinct URLs that resolve to the same feed title. Both fetch fine, + // but the title-keyed store can only hold one of them. + getFeed.mockImplementation(async (url: string) => ({ + title: "Same Title", + url, + artworkUrl: "", + })); + + const opml = + "" + + '' + + '' + + ""; + + await importOPML(opml); + + expect(getFeed).toHaveBeenCalledTimes(2); + expect(Object.keys(get(savedFeeds))).toEqual(["Same Title"]); + // Exactly one was written, so the summary must say "Saved 1", not "Saved 2". + expect(noticeMessages.some((m) => m.includes("Saved 1 new podcasts"))).toBe( + true, + ); + expect(noticeMessages.some((m) => m.includes("Saved 2 new podcasts"))).toBe( + false, + ); + expect( + noticeMessages.some((m) => m.includes("Skipped 1 with duplicate titles")), + ).toBe(true); + }); + + it("counts a title-collision against an existing feed as not saved", async () => { + // The existing feed has a different URL, so the new feed passes the + // URL-dedup and is fetched, but its title collides and it is dropped. + savedFeeds.set({ + "Same Title": { + title: "Same Title", + url: "https://old.test/feed", + artworkUrl: "", + }, + }); + getFeed.mockImplementation(async (url: string) => ({ + title: "Same Title", + url, + artworkUrl: "", + })); + + const opml = + "" + + '' + + ""; + + await importOPML(opml); + + expect(getFeed).toHaveBeenCalledTimes(1); + expect(Object.keys(get(savedFeeds))).toEqual(["Same Title"]); + // The original feed must be preserved, not overwritten. + expect(get(savedFeeds)["Same Title"].url).toBe("https://old.test/feed"); + expect(noticeMessages.some((m) => m.includes("Saved 0 new podcasts"))).toBe( + true, + ); + expect( + noticeMessages.some((m) => m.includes("Skipped 1 with duplicate titles")), + ).toBe(true); + }); + + it("reports URL-skipped and title-dropped feeds in distinct counters", async () => { + // One feed is already saved by URL (skipped before fetch); two new feeds + // share a title (one saved, one title-dropped). Each bucket is counted on + // its own axis so the summary stays honest. + savedFeeds.set({ + Old: { title: "Old", url: "https://old.test/feed", artworkUrl: "" }, + }); + getFeed.mockImplementation(async (url: string) => ({ + title: "Shared", + url, + artworkUrl: "", + })); + + const opml = + "" + + '' + + '' + + '' + + ""; + + await importOPML(opml); + + // Only the two new URLs are fetched; one is saved, the other title-dropped. + expect(getFeed).toHaveBeenCalledTimes(2); + expect(Object.keys(get(savedFeeds)).sort()).toEqual(["Old", "Shared"]); + expect( + noticeMessages.some( + (m) => + m.includes("Saved 1 new podcasts") && + m.includes("Skipped 1 existing podcasts") && + m.includes("Skipped 1 with duplicate titles"), + ), + ).toBe(true); + }); }); diff --git a/src/opml.ts b/src/opml.ts index 998ab20e..e8e40705 100644 --- a/src/opml.ts +++ b/src/opml.ts @@ -125,12 +125,17 @@ async function importOPML(opml: string): Promise { let completedImports = 0; const updateProgress = () => { - const progress = ( - (completedImports / newPodcastsToAdd.length) * - 100 - ).toFixed(1); + const total = newPodcastsToAdd.length; + // When every imported feed is already subscribed there is nothing to + // fetch, so guard the 0/0 division that would otherwise render as + // "NaN%" in the progress notice. + if (total === 0) { + notice.update("No new podcasts to import."); + return; + } + const progress = ((completedImports / total) * 100).toFixed(1); notice.update( - `Importing... ${completedImports}/${newPodcastsToAdd.length} podcasts completed (${progress}%)`, + `Importing... ${completedImports}/${total} podcasts completed (${progress}%)`, ); }; @@ -158,19 +163,31 @@ async function importOPML(opml: string): Promise { (pod): pod is PodcastFeed => pod !== null, ); + // The store is keyed by title, so feeds whose title already exists (either + // from an earlier import in this batch or a previously saved feed) are + // dropped. Count what is actually written so the summary doesn't over-report. + let savedCount = 0; savedFeeds.update((feeds) => { for (const pod of validPodcasts) { if (feeds[pod.title]) continue; feeds[pod.title] = structuredClone(pod); + savedCount++; } return feeds; }); - const skippedCount = + // Feeds skipped before fetching because their URL was already subscribed. + const skippedExisting = incompletePodcastsToAdd.length - newPodcastsToAdd.length; - notice.update( - `OPML import complete. Saved ${validPodcasts.length} new podcasts. Skipped ${skippedCount} existing podcasts.`, - ); + // Feeds that fetched fine but collided with an existing/earlier title and + // were therefore silently dropped by the title-keyed store above. + const droppedDuplicateTitle = validPodcasts.length - savedCount; + + let summary = `OPML import complete. Saved ${savedCount} new podcasts. Skipped ${skippedExisting} existing podcasts.`; + if (droppedDuplicateTitle > 0) { + summary += ` Skipped ${droppedDuplicateTitle} with duplicate titles.`; + } + notice.update(summary); if (validPodcasts.length !== newPodcastsToAdd.length) { const failedImports = newPodcastsToAdd.length - validPodcasts.length;