Skip to content

Commit a79e529

Browse files
chhoumannchhoumann
authored
fix(opml): correct import progress math and saved-count reporting (#221)
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).
1 parent 83c34e7 commit a79e529

2 files changed

Lines changed: 168 additions & 14 deletions

File tree

‎src/opml.test.ts‎

Lines changed: 142 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -6,11 +6,17 @@ import type { PodcastFeed } from "./types/PodcastFeed";
66
// parse/validate/serialize logic: FeedParser would otherwise make real network
77
// requests for every imported feed, and savedFeeds pulls in the whole store.
88
const savedFeeds = writable<Record<string, PodcastFeed>>({});
9-
const getFeed = vi.fn(async (url: string) => ({
9+
const defaultGetFeed = async (url: string) => ({
1010
title: `Feed for ${url}`,
1111
url,
1212
artworkUrl: "",
13-
}));
13+
});
14+
const getFeed = vi.fn(defaultGetFeed);
15+
16+
// Every string the import flow renders into the progress Notice is recorded
17+
// here so tests can assert on what the user actually sees (e.g. no "NaN%", an
18+
// accurate saved count).
19+
const noticeMessages = vi.hoisted(() => [] as string[]);
1420

1521
vi.mock("./store", () => ({
1622
get savedFeeds() {
@@ -29,8 +35,12 @@ vi.mock("./parser/feedParser", () => ({
2935
// uses so the import flow can run to completion under jsdom.
3036
vi.mock("obsidian", () => ({
3137
Notice: class {
32-
constructor(public message?: unknown) {}
33-
setMessage() {}
38+
constructor(message?: unknown) {
39+
if (typeof message === "string") noticeMessages.push(message);
40+
}
41+
setMessage(message?: unknown) {
42+
if (typeof message === "string") noticeMessages.push(message);
43+
}
3444
hide() {}
3545
},
3646
}));
@@ -39,7 +49,9 @@ import { exportOPML, importOPML } from "./opml";
3949

4050
beforeEach(() => {
4151
savedFeeds.set({});
42-
getFeed.mockClear();
52+
getFeed.mockReset();
53+
getFeed.mockImplementation(defaultGetFeed);
54+
noticeMessages.length = 0;
4355
});
4456

4557
/** Minimal Obsidian App stub capturing the single file vault.create writes. */
@@ -189,4 +201,129 @@ describe("importOPML (IE-01)", () => {
189201
expect(getFeed).not.toHaveBeenCalled();
190202
expect(Object.keys(get(savedFeeds))).toEqual(["Existing"]);
191203
});
204+
205+
it("never renders 'NaN%' when every feed is already subscribed", async () => {
206+
savedFeeds.set({
207+
Existing: {
208+
title: "Existing",
209+
url: "https://dup.test/feed",
210+
artworkUrl: "",
211+
},
212+
});
213+
214+
const opml =
215+
"<opml><body>" +
216+
'<outline text="Dup" xmlUrl="https://dup.test/feed" />' +
217+
"</body></opml>";
218+
219+
await importOPML(opml);
220+
221+
// Nothing to fetch, so the 0/0 progress division must not surface as NaN.
222+
expect(getFeed).not.toHaveBeenCalled();
223+
expect(noticeMessages.some((m) => m.includes("NaN"))).toBe(false);
224+
expect(noticeMessages.some((m) => m.includes("Saved 0 new podcasts"))).toBe(
225+
true,
226+
);
227+
});
228+
229+
it("reports the saved count, not the fetched count, on duplicate titles", async () => {
230+
// Two distinct URLs that resolve to the same feed title. Both fetch fine,
231+
// but the title-keyed store can only hold one of them.
232+
getFeed.mockImplementation(async (url: string) => ({
233+
title: "Same Title",
234+
url,
235+
artworkUrl: "",
236+
}));
237+
238+
const opml =
239+
"<opml><body>" +
240+
'<outline text="A" xmlUrl="https://a.test/feed" />' +
241+
'<outline text="B" xmlUrl="https://b.test/feed" />' +
242+
"</body></opml>";
243+
244+
await importOPML(opml);
245+
246+
expect(getFeed).toHaveBeenCalledTimes(2);
247+
expect(Object.keys(get(savedFeeds))).toEqual(["Same Title"]);
248+
// Exactly one was written, so the summary must say "Saved 1", not "Saved 2".
249+
expect(noticeMessages.some((m) => m.includes("Saved 1 new podcasts"))).toBe(
250+
true,
251+
);
252+
expect(noticeMessages.some((m) => m.includes("Saved 2 new podcasts"))).toBe(
253+
false,
254+
);
255+
expect(
256+
noticeMessages.some((m) => m.includes("Skipped 1 with duplicate titles")),
257+
).toBe(true);
258+
});
259+
260+
it("counts a title-collision against an existing feed as not saved", async () => {
261+
// The existing feed has a different URL, so the new feed passes the
262+
// URL-dedup and is fetched, but its title collides and it is dropped.
263+
savedFeeds.set({
264+
"Same Title": {
265+
title: "Same Title",
266+
url: "https://old.test/feed",
267+
artworkUrl: "",
268+
},
269+
});
270+
getFeed.mockImplementation(async (url: string) => ({
271+
title: "Same Title",
272+
url,
273+
artworkUrl: "",
274+
}));
275+
276+
const opml =
277+
"<opml><body>" +
278+
'<outline text="New" xmlUrl="https://new.test/feed" />' +
279+
"</body></opml>";
280+
281+
await importOPML(opml);
282+
283+
expect(getFeed).toHaveBeenCalledTimes(1);
284+
expect(Object.keys(get(savedFeeds))).toEqual(["Same Title"]);
285+
// The original feed must be preserved, not overwritten.
286+
expect(get(savedFeeds)["Same Title"].url).toBe("https://old.test/feed");
287+
expect(noticeMessages.some((m) => m.includes("Saved 0 new podcasts"))).toBe(
288+
true,
289+
);
290+
expect(
291+
noticeMessages.some((m) => m.includes("Skipped 1 with duplicate titles")),
292+
).toBe(true);
293+
});
294+
295+
it("reports URL-skipped and title-dropped feeds in distinct counters", async () => {
296+
// One feed is already saved by URL (skipped before fetch); two new feeds
297+
// share a title (one saved, one title-dropped). Each bucket is counted on
298+
// its own axis so the summary stays honest.
299+
savedFeeds.set({
300+
Old: { title: "Old", url: "https://old.test/feed", artworkUrl: "" },
301+
});
302+
getFeed.mockImplementation(async (url: string) => ({
303+
title: "Shared",
304+
url,
305+
artworkUrl: "",
306+
}));
307+
308+
const opml =
309+
"<opml><body>" +
310+
'<outline text="Old" xmlUrl="https://old.test/feed" />' +
311+
'<outline text="A" xmlUrl="https://a.test/feed" />' +
312+
'<outline text="B" xmlUrl="https://b.test/feed" />' +
313+
"</body></opml>";
314+
315+
await importOPML(opml);
316+
317+
// Only the two new URLs are fetched; one is saved, the other title-dropped.
318+
expect(getFeed).toHaveBeenCalledTimes(2);
319+
expect(Object.keys(get(savedFeeds)).sort()).toEqual(["Old", "Shared"]);
320+
expect(
321+
noticeMessages.some(
322+
(m) =>
323+
m.includes("Saved 1 new podcasts") &&
324+
m.includes("Skipped 1 existing podcasts") &&
325+
m.includes("Skipped 1 with duplicate titles"),
326+
),
327+
).toBe(true);
328+
});
192329
});

‎src/opml.ts‎

Lines changed: 26 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -125,12 +125,17 @@ async function importOPML(opml: string): Promise<void> {
125125
let completedImports = 0;
126126

127127
const updateProgress = () => {
128-
const progress = (
129-
(completedImports / newPodcastsToAdd.length) *
130-
100
131-
).toFixed(1);
128+
const total = newPodcastsToAdd.length;
129+
// When every imported feed is already subscribed there is nothing to
130+
// fetch, so guard the 0/0 division that would otherwise render as
131+
// "NaN%" in the progress notice.
132+
if (total === 0) {
133+
notice.update("No new podcasts to import.");
134+
return;
135+
}
136+
const progress = ((completedImports / total) * 100).toFixed(1);
132137
notice.update(
133-
`Importing... ${completedImports}/${newPodcastsToAdd.length} podcasts completed (${progress}%)`,
138+
`Importing... ${completedImports}/${total} podcasts completed (${progress}%)`,
134139
);
135140
};
136141

@@ -158,19 +163,31 @@ async function importOPML(opml: string): Promise<void> {
158163
(pod): pod is PodcastFeed => pod !== null,
159164
);
160165

166+
// The store is keyed by title, so feeds whose title already exists (either
167+
// from an earlier import in this batch or a previously saved feed) are
168+
// dropped. Count what is actually written so the summary doesn't over-report.
169+
let savedCount = 0;
161170
savedFeeds.update((feeds) => {
162171
for (const pod of validPodcasts) {
163172
if (feeds[pod.title]) continue;
164173
feeds[pod.title] = structuredClone(pod);
174+
savedCount++;
165175
}
166176
return feeds;
167177
});
168178

169-
const skippedCount =
179+
// Feeds skipped before fetching because their URL was already subscribed.
180+
const skippedExisting =
170181
incompletePodcastsToAdd.length - newPodcastsToAdd.length;
171-
notice.update(
172-
`OPML import complete. Saved ${validPodcasts.length} new podcasts. Skipped ${skippedCount} existing podcasts.`,
173-
);
182+
// Feeds that fetched fine but collided with an existing/earlier title and
183+
// were therefore silently dropped by the title-keyed store above.
184+
const droppedDuplicateTitle = validPodcasts.length - savedCount;
185+
186+
let summary = `OPML import complete. Saved ${savedCount} new podcasts. Skipped ${skippedExisting} existing podcasts.`;
187+
if (droppedDuplicateTitle > 0) {
188+
summary += ` Skipped ${droppedDuplicateTitle} with duplicate titles.`;
189+
}
190+
notice.update(summary);
174191

175192
if (validPodcasts.length !== newPodcastsToAdd.length) {
176193
const failedImports = newPodcastsToAdd.length - validPodcasts.length;

0 commit comments

Comments
 (0)