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; } 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; }