From 1c4f7ce4949f92d62abde7a4d850503dd604e7b6 Mon Sep 17 00:00:00 2001 From: Christian Bager Bach Houmann Date: Mon, 29 Jun 2026 09:17:18 +0200 Subject: [PATCH 1/2] fix(feed): parse the feed once per getEpisodes call getEpisodes fetched and parsed the feed twice on a cold call: once via getFeed (to populate channel metadata) and again directly to read the items. This doubled the network round-trips and XML parses on every cold path, e.g. the URIHandler resume-link flow that constructs a FeedParser with no cached feed. Fetch and parse the document a single time and reuse it for both the channel metadata and the episode items. The metadata extraction moves to a private extractFeed(body, url) helper that getFeed and getEpisodes both call, so behavior (metadata population, the "Invalid RSS feed" guard, this.feed caching) is unchanged - only the redundant second fetch is removed. Resolves deepsec finding other-redundant-fetch. --- src/parser/feedParser.test.ts | 260 ++++++++-------------------------- src/parser/feedParser.ts | 19 ++- 2 files changed, 70 insertions(+), 209 deletions(-) diff --git a/src/parser/feedParser.test.ts b/src/parser/feedParser.test.ts index 3ec8e9ed..6c4d6962 100644 --- a/src/parser/feedParser.test.ts +++ b/src/parser/feedParser.test.ts @@ -10,6 +10,18 @@ import { requestWithTimeout } from "src/utility/networkRequest"; const mockRequestWithTimeout = vi.mocked(requestWithTimeout); +// Build the shape requestWithTimeout resolves to. Keeps the per-test mock setup +// to a single line and makes the number of expected fetches obvious at a glance. +function feedResponse(text: string) { + return { + text, + status: 200, + headers: {}, + arrayBuffer: new ArrayBuffer(0), + json: {}, + }; +} + const sampleRssFeed = ` @@ -359,23 +371,10 @@ describe("FeedParser", () => { }); describe("getEpisodes", () => { - test("parses all valid episodes from feed", async () => { - // getEpisodes now calls getFeed first, then parseFeed again - mockRequestWithTimeout - .mockResolvedValueOnce({ - text: sampleRssFeed, - status: 200, - headers: {}, - arrayBuffer: new ArrayBuffer(0), - json: {}, - }) - .mockResolvedValueOnce({ - text: sampleRssFeed, - status: 200, - headers: {}, - arrayBuffer: new ArrayBuffer(0), - json: {}, - }); + test("parses all valid episodes from a single feed fetch", async () => { + mockRequestWithTimeout.mockResolvedValueOnce( + feedResponse(sampleRssFeed), + ); const parser = new FeedParser(); const episodes = await parser.getEpisodes("https://example.com/feed.xml"); @@ -383,24 +382,16 @@ describe("FeedParser", () => { expect(episodes).toHaveLength(2); expect(episodes[0].title).toBe("Episode 1"); expect(episodes[1].title).toBe("Episode 2"); + // A cold getEpisodes call must fetch + parse the feed exactly ONCE. It + // previously fetched twice (here, then again inside getFeed), doubling + // the load on the feed host. + expect(mockRequestWithTimeout).toHaveBeenCalledTimes(1); }); test("parses episode properties correctly and populates feed metadata", async () => { - mockRequestWithTimeout - .mockResolvedValueOnce({ - text: sampleRssFeed, - status: 200, - headers: {}, - arrayBuffer: new ArrayBuffer(0), - json: {}, - }) - .mockResolvedValueOnce({ - text: sampleRssFeed, - status: 200, - headers: {}, - arrayBuffer: new ArrayBuffer(0), - json: {}, - }); + mockRequestWithTimeout.mockResolvedValueOnce( + feedResponse(sampleRssFeed), + ); const parser = new FeedParser(); const episodes = await parser.getEpisodes("https://example.com/feed.xml"); @@ -418,21 +409,9 @@ describe("FeedParser", () => { }); test("parses Podcasting 2.0 chapter URLs from episodes (#47)", async () => { - mockRequestWithTimeout - .mockResolvedValueOnce({ - text: rssFeedWithPodcastChapters, - status: 200, - headers: {}, - arrayBuffer: new ArrayBuffer(0), - json: {}, - }) - .mockResolvedValueOnce({ - text: rssFeedWithPodcastChapters, - status: 200, - headers: {}, - arrayBuffer: new ArrayBuffer(0), - json: {}, - }); + mockRequestWithTimeout.mockResolvedValueOnce( + feedResponse(rssFeedWithPodcastChapters), + ); const parser = new FeedParser(); const episodes = await parser.getEpisodes("https://example.com/feed.xml"); @@ -456,21 +435,7 @@ describe("FeedParser", () => { `; - mockRequestWithTimeout - .mockResolvedValueOnce({ - text: videoFeed, - status: 200, - headers: {}, - arrayBuffer: new ArrayBuffer(0), - json: {}, - }) - .mockResolvedValueOnce({ - text: videoFeed, - status: 200, - headers: {}, - arrayBuffer: new ArrayBuffer(0), - json: {}, - }); + mockRequestWithTimeout.mockResolvedValueOnce(feedResponse(videoFeed)); const parser = new FeedParser(); const episodes = await parser.getEpisodes("https://example.com/feed.xml"); @@ -495,21 +460,9 @@ describe("FeedParser", () => { `; - mockRequestWithTimeout - .mockResolvedValueOnce({ - text: audioMp4Feed, - status: 200, - headers: {}, - arrayBuffer: new ArrayBuffer(0), - json: {}, - }) - .mockResolvedValueOnce({ - text: audioMp4Feed, - status: 200, - headers: {}, - arrayBuffer: new ArrayBuffer(0), - json: {}, - }); + mockRequestWithTimeout.mockResolvedValueOnce( + feedResponse(audioMp4Feed), + ); const parser = new FeedParser(); const episodes = await parser.getEpisodes("https://example.com/feed.xml"); @@ -544,21 +497,9 @@ describe("FeedParser", () => { `; - mockRequestWithTimeout - .mockResolvedValueOnce({ - text: untypedAmbiguousFeed, - status: 200, - headers: {}, - arrayBuffer: new ArrayBuffer(0), - json: {}, - }) - .mockResolvedValueOnce({ - text: untypedAmbiguousFeed, - status: 200, - headers: {}, - arrayBuffer: new ArrayBuffer(0), - json: {}, - }); + mockRequestWithTimeout.mockResolvedValueOnce( + feedResponse(untypedAmbiguousFeed), + ); const parser = new FeedParser(); const episodes = await parser.getEpisodes("https://example.com/feed.xml"); @@ -571,21 +512,9 @@ describe("FeedParser", () => { }); test("filters out invalid episodes missing required fields", async () => { - mockRequestWithTimeout - .mockResolvedValueOnce({ - text: rssFeedWithInvalidItem, - status: 200, - headers: {}, - arrayBuffer: new ArrayBuffer(0), - json: {}, - }) - .mockResolvedValueOnce({ - text: rssFeedWithInvalidItem, - status: 200, - headers: {}, - arrayBuffer: new ArrayBuffer(0), - json: {}, - }); + mockRequestWithTimeout.mockResolvedValueOnce( + feedResponse(rssFeedWithInvalidItem), + ); const parser = new FeedParser(); const episodes = await parser.getEpisodes("https://example.com/feed.xml"); @@ -601,37 +530,23 @@ describe("FeedParser", () => { artworkUrl: "https://example.com/feed-artwork.jpg", }; - mockRequestWithTimeout.mockResolvedValueOnce({ - text: sampleRssFeed, - status: 200, - headers: {}, - arrayBuffer: new ArrayBuffer(0), - json: {}, - }); + mockRequestWithTimeout.mockResolvedValueOnce( + feedResponse(sampleRssFeed), + ); - // When constructed with a feed, it skips calling getFeed + // When constructed with a feed, it skips re-deriving feed metadata. const parser = new FeedParser(mockFeed); const episodes = await parser.getEpisodes("https://example.com/feed.xml"); expect(episodes[1].artworkUrl).toBe("https://example.com/feed-artwork.jpg"); + // A pre-populated, matching feed still fetches the items exactly once. + expect(mockRequestWithTimeout).toHaveBeenCalledTimes(1); }); test("uses episode artwork from itunes:image when available", async () => { - mockRequestWithTimeout - .mockResolvedValueOnce({ - text: sampleRssFeedWithItunesImage, - status: 200, - headers: {}, - arrayBuffer: new ArrayBuffer(0), - json: {}, - }) - .mockResolvedValueOnce({ - text: sampleRssFeedWithItunesImage, - status: 200, - headers: {}, - arrayBuffer: new ArrayBuffer(0), - json: {}, - }); + mockRequestWithTimeout.mockResolvedValueOnce( + feedResponse(sampleRssFeedWithItunesImage), + ); const parser = new FeedParser(); const episodes = await parser.getEpisodes("https://example.com/feed.xml"); @@ -642,21 +557,9 @@ describe("FeedParser", () => { describe("episode number and duration (#34, #88)", () => { test("parses / with a title fallback", async () => { - mockRequestWithTimeout - .mockResolvedValueOnce({ - text: rssFeedWithEpisodeNumberAndDuration, - status: 200, - headers: {}, - arrayBuffer: new ArrayBuffer(0), - json: {}, - }) - .mockResolvedValueOnce({ - text: rssFeedWithEpisodeNumberAndDuration, - status: 200, - headers: {}, - arrayBuffer: new ArrayBuffer(0), - json: {}, - }); + mockRequestWithTimeout.mockResolvedValueOnce( + feedResponse(rssFeedWithEpisodeNumberAndDuration), + ); const parser = new FeedParser(); const episodes = await parser.getEpisodes("https://example.com/feed.xml"); @@ -704,21 +607,7 @@ describe("FeedParser", () => { `; - mockRequestWithTimeout - .mockResolvedValueOnce({ - text: emptyFeed, - status: 200, - headers: {}, - arrayBuffer: new ArrayBuffer(0), - json: {}, - }) - .mockResolvedValueOnce({ - text: emptyFeed, - status: 200, - headers: {}, - arrayBuffer: new ArrayBuffer(0), - json: {}, - }); + mockRequestWithTimeout.mockResolvedValueOnce(feedResponse(emptyFeed)); const parser = new FeedParser(); const episodes = await parser.getEpisodes("https://example.com/feed.xml"); @@ -740,21 +629,7 @@ describe("FeedParser", () => { `; - mockRequestWithTimeout - .mockResolvedValueOnce({ - text: minimalFeed, - status: 200, - headers: {}, - arrayBuffer: new ArrayBuffer(0), - json: {}, - }) - .mockResolvedValueOnce({ - text: minimalFeed, - status: 200, - headers: {}, - arrayBuffer: new ArrayBuffer(0), - json: {}, - }); + mockRequestWithTimeout.mockResolvedValueOnce(feedResponse(minimalFeed)); const parser = new FeedParser(); const episodes = await parser.getEpisodes("https://example.com/feed.xml"); @@ -781,21 +656,7 @@ describe("FeedParser", () => { `; - mockRequestWithTimeout - .mockResolvedValueOnce({ - text: cdataFeed, - status: 200, - headers: {}, - arrayBuffer: new ArrayBuffer(0), - json: {}, - }) - .mockResolvedValueOnce({ - text: cdataFeed, - status: 200, - headers: {}, - arrayBuffer: new ArrayBuffer(0), - json: {}, - }); + mockRequestWithTimeout.mockResolvedValueOnce(feedResponse(cdataFeed)); const parser = new FeedParser(); const episodes = await parser.getEpisodes("https://example.com/feed.xml"); @@ -805,20 +666,8 @@ describe("FeedParser", () => { test("getFeed sets internal feed state for subsequent calls", async () => { mockRequestWithTimeout - .mockResolvedValueOnce({ - text: sampleRssFeed, - status: 200, - headers: {}, - arrayBuffer: new ArrayBuffer(0), - json: {}, - }) - .mockResolvedValueOnce({ - text: sampleRssFeed, - status: 200, - headers: {}, - arrayBuffer: new ArrayBuffer(0), - json: {}, - }); + .mockResolvedValueOnce(feedResponse(sampleRssFeed)) + .mockResolvedValueOnce(feedResponse(sampleRssFeed)); const parser = new FeedParser(); await parser.getFeed("https://example.com/feed.xml"); @@ -827,6 +676,9 @@ describe("FeedParser", () => { // Episodes should have feed metadata populated expect(episodes[0].podcastName).toBe("Test Podcast"); expect(episodes[0].feedUrl).toBe("https://example.com/feed.xml"); + // One fetch for getFeed, one for getEpisodes: getEpisodes reuses the + // already-cached feed metadata and does NOT re-fetch it. + expect(mockRequestWithTimeout).toHaveBeenCalledTimes(2); }); }); }); diff --git a/src/parser/feedParser.ts b/src/parser/feedParser.ts index 987effb0..24a0c28e 100644 --- a/src/parser/feedParser.ts +++ b/src/parser/feedParser.ts @@ -16,19 +16,29 @@ export default class FeedParser { } public async getEpisodes(url: string): Promise { - // Ensure feed metadata is loaded and cached + // Fetch and parse the feed exactly ONCE, then reuse that single document + // for both the channel metadata and the episode items. Previously this + // parsed the feed here AND again inside getFeed, doubling the network + // round-trips and XML parses on every cold call (e.g. the URIHandler + // resume-link path that constructs a FeedParser with no cached feed). + const body = await this.parseFeed(url); + + // Ensure feed metadata is loaded and cached; parseItem reads this.feed for + // the podcast name and the per-episode artwork fallback. if (!this.feed || this.feed.url !== url) { - await this.getFeed(url); + this.feed = this.extractFeed(body, url); } - const body = await this.parseFeed(url); - return this.parsePage(body); } public async getFeed(url: string): Promise { const body = await this.parseFeed(url); + this.feed = this.extractFeed(body, url); + return this.feed; + } + private extractFeed(body: Document, url: string): PodcastFeed { const titleEl = body.querySelector("title"); // A feed must have a title. is intentionally NOT required: it is the @@ -73,7 +83,6 @@ export default class FeedParser { ]); if (author) feed.author = author; - this.feed = feed; return feed; } From c57fae3001ed1f86e60eb69bcd2d1e3881f2464e Mon Sep 17 00:00:00 2001 From: Christian Bager Bach Houmann Date: Mon, 29 Jun 2026 09:17:18 +0200 Subject: [PATCH 2/2] fix(search): rebuild the Fuse cache on content change, not just length The Fuse search index was cached in a WeakMap keyed by the episodes array and reused whenever the cached size matched the array length. Length is a weak fingerprint: the same array reference mutated in place at the same length (an entry swapped or edited) would return the stale index built from the old contents. Validate the cache with a content signature (a JSON-framed list of each episode's title and streamUrl) instead of length, so any content or ordering change rebuilds the index while an unchanged list keeps reusing it across keystrokes (preserving the #149 optimization). Adds the first unit tests for searchEpisodes, including a Fuse-construction spy proving the index is reused when unchanged and rebuilt when the content changes. Resolves deepsec finding other-stale-cache. --- src/utility/searchEpisodes.test.ts | 116 +++++++++++++++++++++++++++++ src/utility/searchEpisodes.ts | 25 ++++++- 2 files changed, 138 insertions(+), 3 deletions(-) create mode 100644 src/utility/searchEpisodes.test.ts diff --git a/src/utility/searchEpisodes.test.ts b/src/utility/searchEpisodes.test.ts new file mode 100644 index 00000000..9b7f8043 --- /dev/null +++ b/src/utility/searchEpisodes.test.ts @@ -0,0 +1,116 @@ +import { beforeEach, describe, expect, test, vi } from "vitest"; +import type { Episode } from "src/types/Episode"; + +// Wrap the real Fuse so we can count how many times an index is built. This lets +// the cache tests assert that an unchanged list reuses its index (the #149 +// optimization) while a changed list rebuilds it - without altering Fuse's real +// search behaviour. +const { fuseConstructorSpy } = vi.hoisted(() => ({ + fuseConstructorSpy: vi.fn(), +})); + +vi.mock("fuse.js", async (importOriginal) => { + const actual = await importOriginal(); + const RealFuse = actual.default; + + class CountingFuse extends RealFuse { + constructor(...args: ConstructorParameters>) { + fuseConstructorSpy(); + super(...args); + } + } + + return { ...actual, default: CountingFuse }; +}); + +import searchEpisodes from "./searchEpisodes"; + +function makeEpisode( + title: string, + streamUrl = `https://example.com/${title}.mp3`, +): Episode { + return { + title, + streamUrl, + url: streamUrl, + description: "", + content: "", + podcastName: "Test Podcast", + }; +} + +function titlesOf(episodes: Episode[]): string[] { + return episodes.map((episode) => episode.title); +} + +describe("searchEpisodes", () => { + beforeEach(() => { + fuseConstructorSpy.mockClear(); + }); + + test("returns episodes whose title fuzzy-matches the query", () => { + const episodes = [makeEpisode("Alpha"), makeEpisode("Beta")]; + + expect(titlesOf(searchEpisodes("Alpha", episodes))).toEqual(["Alpha"]); + }); + + test("returns an empty array for an empty list without building an index", () => { + expect(searchEpisodes("anything", [])).toEqual([]); + expect(fuseConstructorSpy).not.toHaveBeenCalled(); + }); + + test("restores the full list for a whitespace-only query", () => { + const episodes = [makeEpisode("Alpha"), makeEpisode("Beta")]; + + expect(searchEpisodes(" ", episodes)).toBe(episodes); + // A whitespace query short-circuits, so no index is built. + expect(fuseConstructorSpy).not.toHaveBeenCalled(); + }); + + test("reuses the cached index for an unchanged list across searches", () => { + const episodes = [makeEpisode("Alpha"), makeEpisode("Beta")]; + + searchEpisodes("Alpha", episodes); + searchEpisodes("Beta", episodes); + + // The same array reference with unchanged content builds the index once. + expect(fuseConstructorSpy).toHaveBeenCalledTimes(1); + }); + + test("returns fresh results when the list is mutated in place at the same length", () => { + const episodes = [makeEpisode("Alpha"), makeEpisode("Beta")]; + + // Populate the cache against the original contents. + expect(titlesOf(searchEpisodes("Alpha", episodes))).toEqual(["Alpha"]); + + // Replace both entries in place: same array reference, same length, new + // content. A length-only cache check would return the stale index here. + episodes[0] = makeEpisode("Gamma"); + episodes[1] = makeEpisode("Delta"); + + // The old title is gone... + expect(searchEpisodes("Alpha", episodes)).toEqual([]); + // ...and the new content is searchable. + expect(titlesOf(searchEpisodes("Gamma", episodes))).toEqual(["Gamma"]); + // The index was rebuilt because the content signature changed. + expect(fuseConstructorSpy).toHaveBeenCalledTimes(2); + }); + + test("rebuilds the index on a same-title swap to a different identity", () => { + const episodes = [makeEpisode("Same", "https://example.com/first.mp3")]; + + searchEpisodes("Same", episodes); + + // Same title, same length, but a different underlying episode (new + // streamUrl). A length-only OR title-only signature would keep serving the + // stale index here; folding streamUrl into the signature detects the swap. + // Asserting the rebuild (not just the result) is what makes this test + // meaningful: fuse.js reads the live array, so the streamUrl result is + // correct either way - only the rebuild count distinguishes the fix. + episodes[0] = makeEpisode("Same", "https://example.com/second.mp3"); + const results = searchEpisodes("Same", episodes); + + expect(results[0].streamUrl).toBe("https://example.com/second.mp3"); + expect(fuseConstructorSpy).toHaveBeenCalledTimes(2); + }); +}); diff --git a/src/utility/searchEpisodes.ts b/src/utility/searchEpisodes.ts index cdfbd99c..cfa1851c 100644 --- a/src/utility/searchEpisodes.ts +++ b/src/utility/searchEpisodes.ts @@ -9,17 +9,36 @@ const fuseOptions = { keys: ["title"], }; -const fuseCache = new WeakMap; size: number }>(); +const fuseCache = new WeakMap< + Episode[], + { fuse: Fuse; signature: string } +>(); + +// Fingerprint the searchable content of the list. The Fuse index is keyed by the +// array reference, but the same reference can be mutated in place (an entry +// swapped or its title edited) while keeping the same length, so length alone is +// too weak a validity check - it would hand back an index built from the old +// contents. The signature captures each episode's title (the only indexed field) +// and streamUrl (its stable identity, so a same-title swap is still detected) in +// order, so any content or ordering change rebuilds the index while an unchanged +// list keeps reusing it across keystrokes. JSON framing keeps it collision-free +// regardless of what the titles and URLs contain. +function contentSignature(episodes: Episode[]): string { + return JSON.stringify( + episodes.map((episode) => [episode.title, episode.streamUrl]), + ); +} function getFuse(episodes: Episode[]): Fuse { + const signature = contentSignature(episodes); const cached = fuseCache.get(episodes); - if (cached && cached.size === episodes.length) { + if (cached && cached.signature === signature) { return cached.fuse; } const newFuse = new Fuse(episodes, fuseOptions); - fuseCache.set(episodes, { fuse: newFuse, size: episodes.length }); + fuseCache.set(episodes, { fuse: newFuse, signature }); return newFuse; }