diff --git a/docs/docs/api.md b/docs/docs/api.md index 415f6ebc..a3099ae9 100644 --- a/docs/docs/api.md +++ b/docs/docs/api.md @@ -102,9 +102,10 @@ ${transcript}`; }; ``` -## `getPodcastTimeFormatted(format: string, linkify?: boolean)` +## `getPodcastTimeFormatted(format: string, linkify?: boolean, offsetSeconds?: number)` This function will return the current playback time formatted according to the given (moment) format. If `linkify` is true, the time will be linked to the current episode at the given time. This is used by PodNotes to play from the recorded time. +`offsetSeconds` is optional (default 0), subtracted from the current playback time before formatting/linking, floored at 0, and also applies to the linkified timestamp. ## `getPodcastSegmentFormatted(format: string, startTime: number, endTime: number, linkify?: boolean)` This function returns a formatted `start-end` playback range. diff --git a/docs/docs/transcripts.md b/docs/docs/transcripts.md index b859ec7f..c89b5cf4 100644 --- a/docs/docs/transcripts.md +++ b/docs/docs/transcripts.md @@ -43,7 +43,7 @@ By default the transcription uses OpenAI's Whisper model, which produces plain t In the **Transcript settings** section, turn on **Speaker diarization** and choose a provider: -- **OpenAI** (`gpt-4o-transcribe-diarize`): reuses the OpenAI API key you already entered above, so there is nothing else to configure. Because OpenAI caps each request at 25 MB, a long episode is split into chunks that are diarized independently — so on long episodes the speaker labels can change across chunk boundaries (the same person may be labelled `A` in one chunk and `B` in the next). A typical-length episode fits in a single request and is fully consistent. +- **OpenAI** (`gpt-4o-transcribe-diarize`): reuses the OpenAI API key you already entered above, so there is nothing else to configure. Because each request is capped at ~20 MB (a conservative margin under OpenAI's 25 MB request cap), a long episode is split into chunks that are diarized independently — so on long episodes the speaker labels can change across chunk boundaries (the same person may be labelled `A` in one chunk and `B` in the next). A typical-length episode fits in a single request and is fully consistent. - **Deepgram**: sends the whole episode in one request, so speaker labels stay consistent across the entire episode. This requires a separate **Deepgram API key**, which you can create at [deepgram.com](https://deepgram.com) (new accounts include free credit). Your Deepgram key is stored separately from your OpenAI key and is only used for diarization. Diarization is off by default, so existing transcripts and the plain-Whisper workflow are unchanged unless you enable it. diff --git a/src/API/API.test.ts b/src/API/API.test.ts index cfdc1257..efcddf80 100644 --- a/src/API/API.test.ts +++ b/src/API/API.test.ts @@ -8,8 +8,10 @@ import { currentEpisode, currentTime, downloadedEpisodes, + duration, playbackRate, plugin, + volume, } from "src/store"; import type { Episode } from "src/types/Episode"; import type { LocalEpisode } from "src/types/LocalEpisode"; @@ -159,6 +161,106 @@ describe("API playback rate controls", () => { }); }); +describe("API skip controls (PB-02)", () => { + beforeEach(() => { + duration.set(0); + plugin.set({ + settings: { + defaultPlaybackRate: 1, + skipBackwardLength: 15, + skipForwardLength: 30, + }, + } as never); + }); + + test("skips backward and forward by the configured lengths", () => { + currentTime.set(100); + const api = new API(); + + api.skipBackward(); + expect(api.currentTime).toBe(85); + + api.skipForward(); + expect(api.currentTime).toBe(115); + }); + + test("never seeks before 0 when skipping backward past the start", () => { + currentTime.set(5); + const api = new API(); + + api.skipBackward(); + expect(api.currentTime).toBe(0); + }); + + test("falls back to a 15s default when a skip length is NaN/invalid", () => { + currentTime.set(100); + plugin.set({ + settings: { + defaultPlaybackRate: 1, + skipBackwardLength: Number.NaN, + skipForwardLength: undefined, + }, + } as never); + const api = new API(); + + api.skipBackward(); + expect(api.currentTime).toBe(85); + + api.skipForward(); + expect(api.currentTime).toBe(100); + }); + + test("clamps a forward skip just short of the end so it doesn't auto-advance", () => { + currentTime.set(100); + duration.set(110); + const api = new API(); + + api.skipForward(); + // 100 + 30 = 130, clamped to duration - 0.25. + expect(api.currentTime).toBe(109.75); + }); + + test("does not rewind on a forward skip when already within the last 0.25s (#213)", () => { + currentTime.set(9.9); + duration.set(10); + const api = new API(); + + api.skipForward(); + // The end-clamp (dur - 0.25 = 9.75) must never move a forward skip backward. + expect(api.currentTime).toBe(9.9); + }); + + test("leaves the forward target unclamped when duration is unknown", () => { + currentTime.set(100); + duration.set(0); + const api = new API(); + + api.skipForward(); + expect(api.currentTime).toBe(130); + }); +}); + +describe("API volume (AP-07)", () => { + test("clamps volume into [0, 1]", () => { + volume.set(0.5); + const api = new API(); + + api.volume = 2; + expect(api.volume).toBe(1); + + api.volume = -1; + expect(api.volume).toBe(0); + }); + + test("ignores a non-finite volume (keeps the current value) instead of poisoning it to NaN", () => { + volume.set(0.6); + const api = new API(); + + api.volume = Number.NaN; + expect(api.volume).toBe(0.6); + }); +}); + describe("API transcript access", () => { function setTranscriptVault(files: Record) { const createTFile = (path: string): TFile => diff --git a/src/API/API.ts b/src/API/API.ts index 537f9d98..0c382c1c 100644 --- a/src/API/API.ts +++ b/src/API/API.ts @@ -27,8 +27,25 @@ import { } from "src/utility/playbackRate"; import { getEpisodeTranscriptPath } from "src/utility/getEpisodeTranscriptPath"; -const clampVolume = (value: number): number => - Math.min(1, Math.max(0, value)); +// Default used when a persisted/configured skip length is missing or invalid +// (e.g. a cleared settings field serialized to null/NaN). Mirrors +// DEFAULT_SETTINGS.skip*Length. +const DEFAULT_SKIP_LENGTH = 15; + +const normalizeSkipLength = (length: number): number => + Number.isFinite(length) && length > 0 ? length : DEFAULT_SKIP_LENGTH; + +// A non-finite volume (e.g. api.volume = NaN) must never poison the player, which +// two-way binds the store onto the media element. Fall back to the current value +// (itself clamped) so a bad write is a no-op rather than corrupting playback. +const clampVolume = (value: number, fallback = 1): number => { + const safeFallback = Number.isFinite(fallback) + ? Math.min(1, Math.max(0, fallback)) + : 1; + return Number.isFinite(value) + ? Math.min(1, Math.max(0, value)) + : safeFallback; +}; export class API implements IAPI { public get podcast(): Episode { @@ -61,7 +78,7 @@ export class API implements IAPI { } public set volume(value: number) { - volumeStore.set(clampVolume(value)); + volumeStore.set(clampVolume(value, get(volumeStore))); } public get playbackRate(): number { @@ -187,13 +204,29 @@ export class API implements IAPI { } skipBackward(): void { - const skipBackLen = get(plugin).settings.skipBackwardLength; - this.currentTime -= skipBackLen; + const skipBackLen = normalizeSkipLength( + get(plugin).settings.skipBackwardLength, + ); + // Never seek before the start. A cleared settings field (NaN/null) falls + // back to the default rather than corrupting currentTime (PB-02). + this.currentTime = Math.max(0, this.currentTime - skipBackLen); } skipForward(): void { - const skipForwardLen = get(plugin).settings.skipForwardLength; - this.currentTime += skipForwardLen; + const skipForwardLen = normalizeSkipLength( + get(plugin).settings.skipForwardLength, + ); + const target = this.currentTime + skipForwardLen; + const dur = this.length; + // Clamp just short of the end so an over-skip lands at the end instead of + // firing 'ended' and auto-advancing the queue. Never let the clamp move the + // position BACKWARD: when already within the last 0.25s, a forward skip must + // not rewind, so keep at least the current time (PB-02 / Codex review #213). + // With an unknown/zero duration (metadata not loaded) leave it unclamped. + this.currentTime = + dur > 0 + ? Math.max(this.currentTime, Math.min(target, dur - 0.25)) + : target; } increasePlaybackRate(): void { diff --git a/src/TemplateEngine.test.ts b/src/TemplateEngine.test.ts index 62c3b3d3..8b95d2a2 100644 --- a/src/TemplateEngine.test.ts +++ b/src/TemplateEngine.test.ts @@ -74,6 +74,29 @@ const demoEpisode: Episode = { episodeDate: new Date("2024-01-01"), }; +describe("DownloadPathTemplateEngine extension stripping (#DL-04)", () => { + it("strips only a trailing template extension", () => { + expect( + DownloadPathTemplateEngine("Podcasts/{{title}}.mp3", demoEpisode), + ).toBe("Podcasts/Episode 1"); + }); + + it("does not corrupt a folder that contains the extension string earlier in the path", () => { + // getUrlExtension returns the trailing 'mp3', but the old positional + // `.replace('mp3', '')` would strip the FIRST 'mp3' (in the folder name), + // mangling 'mp3folder' -> 'folder' and leaving the real '.mp3' behind. + expect( + DownloadPathTemplateEngine("mp3folder/{{title}}.mp3", demoEpisode), + ).toBe("mp3folder/Episode 1"); + }); + + it("leaves a template without a trailing extension untouched", () => { + expect( + DownloadPathTemplateEngine("Podcasts/{{title}}", demoEpisode), + ).toBe("Podcasts/Episode 1"); + }); +}); + describe("TimestampTemplateEngine segment tags", () => { beforeEach(() => { currentEpisode.set(demoEpisode); @@ -116,6 +139,30 @@ describe("TimestampTemplateEngine segment tags", () => { }); }); +describe("empty tag arguments (NT-05/CH-09)", () => { + it("treats {{date:}} as the default date, not an unknown tag", () => { + plugin.set({ settings: { feedNote: { path: "" }, savedFeeds: {} } } as never); + // A bare trailing colon must use the tag default (same as {{date}}), + // not parse as tagId "date:" (invalid) and not pass "" as the format. + expect(NoteTemplateEngine("{{date:}}", demoEpisode)).toBe( + NoteTemplateEngine("{{date}}", demoEpisode), + ); + expect(NoteTemplateEngine("{{date:}}", demoEpisode)).toBe("2024-01-01"); + }); + + it("preserves a whitespace-only argument rather than collapsing it (no regression)", () => { + plugin.set({ settings: { feedNote: { path: "" }, savedFeeds: {} } } as never); + // A lone-space argument is passed through to the tag (formatDate(date, " ")), + // keeping the obscure space-prepend usage working — only an empty arg defaults. + expect(NoteTemplateEngine("{{date: }}", demoEpisode)).toBe(" "); + }); + + it("still honors a real date format argument", () => { + plugin.set({ settings: { feedNote: { path: "" }, savedFeeds: {} } } as never); + expect(NoteTemplateEngine("{{date:YYYY}}", demoEpisode)).toBe("2024"); + }); +}); + describe("NoteTemplateEngine feed-scoped tags (#163)", () => { it("keeps {{url}} and {{artwork}} pointing at the episode itself", () => { plugin.set({ settings: { feedNote: { path: "" }, savedFeeds: {} } } as never); diff --git a/src/TemplateEngine.ts b/src/TemplateEngine.ts index fa94c92c..973da4cb 100644 --- a/src/TemplateEngine.ts +++ b/src/TemplateEngine.ts @@ -28,7 +28,11 @@ interface Tags { type AddTagFn = (tag: Lowercase, value: TagValue) => void; type ReplacerFn = (template: string) => string; -const TEMPLATE_TAG_REGEX = /\{\{(.*?)(:\s*?.+?)?\}\}/g; +// The optional argument group uses `.*?` (zero-or-more), so a bare trailing +// colon ({{date:}}) is captured AS the argument group (params === ":") rather +// than being absorbed into the tag id — which previously yielded tagId "date:", +// an unknown tag, and a spurious "invalid tag" Notice (NT-05/CH-09). +const TEMPLATE_TAG_REGEX = /\{\{(.*?)(:\s*.*?)?\}\}/g; export interface NoteTemplateContext { chapters?: Chapter[]; @@ -79,15 +83,20 @@ function useTemplateEngine(): Readonly<[ReplacerFn, AddTagFn]> { } if (typeof tagValue === "function") { - if (params) { - // Pass everything after the leading colon as a single argument. - // No tag takes more than one argument, and splitting on "," would - // corrupt format strings that legitimately contain commas - // (e.g. {{currentDate:MMMM D, YYYY}}). - return tagValue(params.slice(1)); + // Everything after the leading colon is a single argument. A bare + // trailing colon ({{date:}}) means "use the tag default", same as a + // bare {{date}} — not an empty format string (NT-05/CH-09). Only an + // EMPTY argument is collapsed; a whitespace argument ({{description: }}) + // is preserved so the obscure space-prepend usage still works. + const arg = params ? params.slice(1) : ""; + if (arg === "") { + return tagValue(); } - return tagValue(); + // No tag takes more than one argument, and splitting on "," would + // corrupt format strings that legitimately contain commas + // (e.g. {{currentDate:MMMM D, YYYY}}). + return tagValue(arg); } return tagValue; @@ -323,10 +332,20 @@ export function FilePathTemplateEngine(template: string, episode: Episode) { } export function DownloadPathTemplateEngine(template: string, episode: Episode) { - // Removing the template extension, as this is added automatically depending on file type. + // Removing the template extension, as this is added automatically depending on + // file type. Anchor the strip at end-of-string: getUrlExtension returns the + // FIRST '.ext' followed by '?'/'#'/end, which need not be the trailing one, so + // a positional `.replace(ext, "")` could corrupt a folder name that happens to + // contain the same string earlier in the path (#DL-04). const templateExtension = getUrlExtension(template); const templateWithoutExtension = templateExtension - ? template.replace(templateExtension, "") + ? template.replace( + new RegExp( + `\\.${templateExtension.replace(/[.*+?^${}()|[\]\\]/g, "\\$&")}$`, + "i", + ), + "", + ) : template; const [replacer, addTag] = useTemplateEngine(); diff --git a/src/URIHandler.test.ts b/src/URIHandler.test.ts index e503db17..974f32ef 100644 --- a/src/URIHandler.test.ts +++ b/src/URIHandler.test.ts @@ -361,6 +361,41 @@ describe("podNotesURIHandler", () => { expect(mockGetEpisodes).not.toHaveBeenCalled(); }); + test("resolves a moved/renamed local file by name instead of routing to the feed parser (LF-08)", async () => { + const localEpisode: LocalEpisode = { + title: "Local Note", + streamUrl: "Old/Local Note.mp3", + url: "Old/Local Note.mp3", + description: "", + content: "", + podcastName: "local file", + filePath: "Old/Local Note.mp3", + }; + localFiles.set({ ...emptyLocalFiles, episodes: [localEpisode] }); + // The file was moved: the recorded path no longer resolves to a file, and + // the url has no http(s) scheme, so it must be treated as a vault path. + setApp(() => null); + + await podNotesURIHandler( + { + action: "podnotes", + url: "Old/Local Note.mp3", + episodeName: "Local Note", + time: "15", + }, + api as never, + ); + + expect(get(currentEpisode)).toMatchObject({ title: "Local Note" }); + expect(get(viewState)).toBe(ViewState.Player); + expect(get(requestedPlaybackTime)).toEqual({ + episodeKey: "local file::Local Note", + time: 15, + }); + // A schemeless path must never hit the feed parser. + expect(mockGetEpisodes).not.toHaveBeenCalled(); + }); + test("shows a notice and changes nothing when the episode cannot be found", async () => { mockGetEpisodes.mockResolvedValue([]); @@ -605,6 +640,22 @@ describe("podNotesURIHandler", () => { expect(mockGetEpisodes).not.toHaveBeenCalled(); }); + test("rejects a negative plain (non-segment) timestamp", async () => { + await podNotesURIHandler( + { + action: "podnotes", + url: testFeedUrl, + episodeName: testEpisode.title, + time: "-10", + }, + api as never, + ); + + expect(get(currentEpisode)).toBeUndefined(); + expect(get(viewState)).toBe(ViewState.PodcastGrid); + expect(mockGetEpisodes).not.toHaveBeenCalled(); + }); + test("rejects a negative segment start timestamp", async () => { await podNotesURIHandler( { diff --git a/src/URIHandler.ts b/src/URIHandler.ts index a6ed1460..3b5f32c1 100644 --- a/src/URIHandler.ts +++ b/src/URIHandler.ts @@ -93,6 +93,12 @@ export default async function podNotesURIHandler( new Notice("Timestamp must be a valid number"); return; } + // One guard ahead of both the segment and plain paths: a negative time is + // never valid (the segment path's own requestedTime < 0 check is now redundant). + if (requestedTime < 0) { + new Notice("Timestamp must be zero or greater"); + return; + } } const requestedEndTime = parseSegmentEndTime( @@ -161,6 +167,14 @@ export default async function podNotesURIHandler( episode = nameCandidates .map((name) => localFiles.getLocalEpisode(name)) .find((ep) => ep !== undefined); + } else if (!candidateValues(url).some((u) => /^https?:\/\//i.test(u))) { + // The probe found no file, yet `url` has no http(s) scheme, so it can only be + // a vault path whose file was moved/renamed. Resolve the episode by name from + // the local-files store rather than handing the bare path to FeedParser (which + // would fail and show a misleading "Could not load the podcast feed"). + episode = nameCandidates + .map((name) => localFiles.getLocalEpisode(name)) + .find((ep) => ep !== undefined); } else { try { // Fetch with the raw url (current-format links are correct as-is); only the title gets diff --git a/src/commands.ts b/src/commands.ts index beab7eed..4697d22d 100644 --- a/src/commands.ts +++ b/src/commands.ts @@ -13,7 +13,6 @@ import createFeedNote from "src/createFeedNote"; import { FeedSuggestModal, orderFeedsByCurrent } from "src/ui/FeedSuggestModal"; import downloadEpisodeWithNotice from "src/downloadEpisode"; import getUniversalPodcastLink from "src/getUniversalPodcastLink"; -import { requiredTranscriptionKeyPresent } from "src/services/diarization"; import { getEpisodeMediaType } from "src/utility/mediaType"; import type { IconType } from "src/types/IconType"; import type PodNotes from "src/main"; @@ -106,8 +105,12 @@ export function registerCommands(plugin: PodNotes): void { name: "Skip Backward", icon: "skip-back" as IconType, checkCallback: (checking) => { + // Skipping only seeks the position, so it is available whenever an + // episode is loaded — paused or playing — matching the always-active + // on-screen skip buttons (previously these commands required isPlaying, + // so a hotkey silently did nothing while paused). See PB-02. if (checking) { - return plugin.api.isPlaying && !!plugin.api.podcast; + return !!plugin.api.podcast; } plugin.api.skipBackward(); @@ -120,7 +123,7 @@ export function registerCommands(plugin: PodNotes): void { icon: "skip-forward" as IconType, checkCallback: (checking) => { if (checking) { - return plugin.api.isPlaying && !!plugin.api.podcast; + return !!plugin.api.podcast; } plugin.api.skipForward(); @@ -137,7 +140,13 @@ export function registerCommands(plugin: PodNotes): void { } const episode = plugin.api.podcast; - downloadEpisodeWithNotice(episode, plugin.settings.download.path); + // Settle the promise so a failed download surfaces in its own Notice + // without leaving an unhandled rejection (DL-01). The notice itself is + // the user-facing error; the log aids diagnosis. + void downloadEpisodeWithNotice( + episode, + plugin.settings.download.path, + ).catch((error) => console.error("PodNotes: download failed", error)); }, }); @@ -160,9 +169,19 @@ export function registerCommands(plugin: PodNotes): void { callback: () => { const id = plugin.manifest.id; + // Confirm success and handle failure: without a catch a failed + // disable/enable left the plugin off with no feedback, looking like a + // crash (MX-01/MX-05). plugin.app.plugins .disablePlugin(id) - .then(() => plugin.app.plugins.enablePlugin(id)); + .then(() => plugin.app.plugins.enablePlugin(id)) + .then(() => new Notice("PodNotes reloaded")) + .catch((error) => { + console.error("PodNotes reload failed", error); + new Notice( + "Failed to reload PodNotes. Re-enable it from Settings > Community plugins.", + ); + }); }, }); @@ -170,8 +189,19 @@ export function registerCommands(plugin: PodNotes): void { id: "capture-timestamp", name: "Capture Timestamp", icon: "clock" as IconType, + // Keep this an editorCallback (not editorCheckCallback): an unconditional + // editor command stays addable to the mobile editor toolbar / command + // picker even before an episode is loaded, whereas a checkCallback that + // returns false would filter it out (and an e2e test pins this). Surface a + // Notice when capture isn't possible instead of silently no-opping (TS-01). editorCallback: (editor) => { - plugin.captureTimestamp(editor); + if (plugin.captureTimestamp(editor)) { + return; + } + + new Notice( + "Play an episode and set a Capture timestamp format in settings to capture a timestamp.", + ); }, }); @@ -321,9 +351,12 @@ export function registerCommands(plugin: PodNotes): void { id: "podnotes-transcribe", name: "Transcribe current episode", checkCallback: (checking) => { + // Don't gate availability on the API key: keep the command offered + // whenever an audio episode is loaded so running it without a key shows + // the service's context-aware "set your API key" Notice instead of the + // command silently vanishing with no explanation (TR-02). const canTranscribe = !!plugin.api.podcast && - requiredTranscriptionKeyPresent(plugin.settings) && getEpisodeMediaType(plugin.api.podcast) === "audio"; if (checking) { diff --git a/src/downloadEpisode.test.ts b/src/downloadEpisode.test.ts index ee429029..0d098872 100644 --- a/src/downloadEpisode.test.ts +++ b/src/downloadEpisode.test.ts @@ -1,6 +1,6 @@ import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; import { get } from "svelte/store"; -import { requestUrl, TFile } from "obsidian"; +import { Notice, requestUrl, TFile } from "obsidian"; import downloadEpisodeWithNotice, { detectAudioFileExtension, downloadEpisode, @@ -71,11 +71,39 @@ afterEach(() => { (globalThis as { app?: unknown }).app = undefined; }); +// Build a minimal ISO-BMFF header: a 4-byte box size, the 'ftyp' box type at +// offset 4, and the 4-character major brand at offset 8 (#DL-06). +function ftyp(brand: string): ArrayBuffer { + const head = bytes(0x00, 0x00, 0x00, 0x18, 0x66, 0x74, 0x79, 0x70); + const brandBytes = new TextEncoder().encode(brand.padEnd(4, " ").slice(0, 4)); + const out = new Uint8Array(head.byteLength + brandBytes.byteLength); + out.set(new Uint8Array(head), 0); + out.set(brandBytes, head.byteLength); + return out.buffer; +} + describe("detectAudioFileExtension", () => { - it("matches exact signatures (ID3 -> mp3, RIFF -> wav, m4a)", () => { + it("matches exact signatures (ID3 -> mp3, RIFF -> wav)", () => { expect(detectAudioFileExtension(bytes(0x49, 0x44, 0x33, 0x04))).toBe("mp3"); expect(detectAudioFileExtension(bytes(0x52, 0x49, 0x46, 0x46))).toBe("wav"); - expect(detectAudioFileExtension(bytes(0x4d, 0x34, 0x41, 0x20))).toBe("m4a"); + }); + + it("detects real ISO-BMFF m4a/mp4 by the ftyp major brand at offset 8 (#DL-06)", () => { + // The major brand lives at offset 8, not offset 0 — the old offset-0 'M4A ' + // signature never matched a genuine m4a/mp4 file. + expect(detectAudioFileExtension(ftyp("M4A "))).toBe("m4a"); + expect(detectAudioFileExtension(ftyp("M4B "))).toBe("mp4"); + expect(detectAudioFileExtension(ftyp("M4V "))).toBe("m4v"); + expect(detectAudioFileExtension(ftyp("qt "))).toBe("mov"); + expect(detectAudioFileExtension(ftyp("mp42"))).toBe("mp4"); + expect(detectAudioFileExtension(ftyp("isom"))).toBe("mp4"); + // An unknown brand still resolves to a generic mp4 container. + expect(detectAudioFileExtension(ftyp("zzzz"))).toBe("mp4"); + }); + + it("does not treat a bare 'M4A ' (no ftyp box) as ISO-BMFF", () => { + // Four bytes with no ftyp box type at offset 4 are not an MP4 file. + expect(detectAudioFileExtension(bytes(0x4d, 0x34, 0x41, 0x20))).toBeNull(); }); it("applies the masked MPEG frame-sync signature", () => { @@ -274,6 +302,40 @@ describe("downloadEpisodeWithNotice (download command path)", () => { }); }); + it("saves an audio/mp4 download with a generic ISO-BMFF brand as m4a, not mp4 (Codex #213)", async () => { + const { createBinary } = setupVault(); + // Real ISO-BMFF: 4-byte box size, 'ftyp', then a generic 'mp42' major brand. + // detectAudioFileExtension returns "mp4" for this; for an audio download it + // must still be saved as m4a so it isn't treated as an ambiguous container. + const buffer = bytes( + 0x00, 0x00, 0x00, 0x20, 0x66, 0x74, 0x79, 0x70, 0x6d, 0x70, 0x34, 0x32, + ); + requestUrlMock.mockResolvedValue({ + status: 200, + headers: { "content-type": "audio/mp4" }, + arrayBuffer: buffer, + } as unknown as Awaited>); + const episode = makeEpisode({ + title: "Brandy MP4 Title", + streamUrl: "https://example.com/episode.mp4", + mediaType: "audio", + }); + const setTimeoutSpy = vi + .spyOn(globalThis, "setTimeout") + .mockImplementation(() => 0 as unknown as ReturnType); + + try { + await downloadEpisodeWithNotice(episode, "Podcasts/{{title}}"); + } finally { + setTimeoutSpy.mockRestore(); + } + + expect(createBinary).toHaveBeenCalledWith( + "Podcasts/Brandy MP4 Title.m4a", + buffer, + ); + }); + it("preserves audio/webm downloads as audio WebM files", async () => { const { createBinary } = setupVault(); const buffer = bytes(0x00, 0x00, 0x00, 0x18); @@ -309,6 +371,171 @@ describe("downloadEpisodeWithNotice (download command path)", () => { size: buffer.byteLength, }); }); + + it("dismisses the notice on a successful download (#DL-03)", async () => { + setupVault(); + requestUrlMock.mockResolvedValue({ + status: 200, + headers: { "content-type": "audio/mpeg" }, + arrayBuffer: bytes(0x49, 0x44, 0x33, 0x01), + } as unknown as Awaited>); + const hideSpy = vi.spyOn(Notice.prototype, "hide"); + vi.useFakeTimers(); + + try { + await downloadEpisodeWithNotice(makeEpisode(), "Podcasts/{{title}}"); + expect(hideSpy).not.toHaveBeenCalled(); + vi.runAllTimers(); + expect(hideSpy).toHaveBeenCalledTimes(1); + } finally { + vi.useRealTimers(); + hideSpy.mockRestore(); + } + }); + + it("still dismisses the notice when the download fails (#DL-03)", async () => { + setupVault(); + requestUrlMock.mockRejectedValue(new Error("network down")); + const hideSpy = vi.spyOn(Notice.prototype, "hide"); + vi.useFakeTimers(); + + try { + await expect( + downloadEpisodeWithNotice(makeEpisode(), "Podcasts/{{title}}"), + ).rejects.toThrow(/network down/); + // The dismissal lives in a finally, so a rejected download still hides the + // notice exactly once instead of leaving it open forever. + vi.runAllTimers(); + expect(hideSpy).toHaveBeenCalledTimes(1); + } finally { + vi.useRealTimers(); + hideSpy.mockRestore(); + } + }); + + it("still dismisses the notice on the non-playable rejection (#DL-03)", async () => { + setupVault(); + requestUrlMock.mockResolvedValue({ + status: 200, + headers: { "content-type": "text/html" }, + arrayBuffer: new TextEncoder().encode("nope").buffer, + } as unknown as Awaited>); + const hideSpy = vi.spyOn(Notice.prototype, "hide"); + vi.useFakeTimers(); + + try { + await expect( + downloadEpisodeWithNotice( + makeEpisode({ streamUrl: "https://example.com/ep.mp3" }), + "Podcasts/{{title}}", + ), + ).rejects.toThrow(/Not a playable media file/); + vi.runAllTimers(); + expect(hideSpy).toHaveBeenCalledTimes(1); + } finally { + vi.useRealTimers(); + hideSpy.mockRestore(); + } + }); + + it("rejects an HTML error page served at a .mp3 URL instead of saving it (#DL-07)", async () => { + const { createBinary } = setupVault(); + requestUrlMock.mockResolvedValue({ + status: 200, + headers: { "content-type": "text/html; charset=utf-8" }, + arrayBuffer: new TextEncoder().encode("login required") + .buffer, + } as unknown as Awaited>); + const setTimeoutSpy = vi + .spyOn(globalThis, "setTimeout") + .mockImplementation(() => 0 as unknown as ReturnType); + + try { + await expect( + downloadEpisodeWithNotice( + // URL ends in .mp3, so the extension heuristic would otherwise + // accept the HTML body as playable media. + makeEpisode({ streamUrl: "https://example.com/expired.mp3" }), + "Podcasts/{{title}}", + ), + ).rejects.toThrow(/Not a playable media file/); + } finally { + setTimeoutSpy.mockRestore(); + } + + expect(createBinary).not.toHaveBeenCalled(); + expect(get(downloadedEpisodes)["Pod"]).toBeUndefined(); + }); + + it("rejects an RSS/Atom feed served at a .mp3 URL (structured +xml types) (#213)", async () => { + const { createBinary } = setupVault(); + requestUrlMock.mockResolvedValue({ + status: 200, + headers: { "content-type": "application/rss+xml" }, + arrayBuffer: new TextEncoder().encode("").buffer, + } as unknown as Awaited>); + const setTimeoutSpy = vi + .spyOn(globalThis, "setTimeout") + .mockImplementation(() => 0 as unknown as ReturnType); + + try { + await expect( + downloadEpisodeWithNotice( + makeEpisode({ streamUrl: "https://example.com/expired.mp3" }), + "Podcasts/{{title}}", + ), + ).rejects.toThrow(/Not a playable media file/); + } finally { + setTimeoutSpy.mockRestore(); + } + + expect(createBinary).not.toHaveBeenCalled(); + }); + + it("rejects a JSON error body served at a .mp3 URL (#DL-07)", async () => { + const { createBinary } = setupVault(); + requestUrlMock.mockResolvedValue({ + status: 200, + headers: { "content-type": "application/json" }, + arrayBuffer: new TextEncoder().encode('{"error":"forbidden"}').buffer, + } as unknown as Awaited>); + + await expect( + downloadEpisodeWithNotice( + makeEpisode({ streamUrl: "https://example.com/ep.mp3" }), + "Podcasts/{{title}}", + ), + ).rejects.toThrow(/Not a playable media file/); + + expect(createBinary).not.toHaveBeenCalled(); + }); + + it("still saves a real audio download served as application/octet-stream (#DL-07)", async () => { + // octet-stream is genuinely ambiguous and is intentionally NOT rejected up + // front — real CDNs serve media this way, so it falls through to the + // extension/signature heuristic. + const { createBinary } = setupVault(); + const buffer = bytes(0x49, 0x44, 0x33, 0x01); + requestUrlMock.mockResolvedValue({ + status: 200, + headers: { "content-type": "application/octet-stream" }, + arrayBuffer: buffer, + } as unknown as Awaited>); + const setTimeoutSpy = vi + .spyOn(globalThis, "setTimeout") + .mockImplementation(() => 0 as unknown as ReturnType); + + try { + await downloadEpisodeWithNotice( + makeEpisode({ streamUrl: "https://example.com/ep.mp3" }), + "Podcasts/{{title}}", + ); + } finally { + setTimeoutSpy.mockRestore(); + } + + expect(createBinary).toHaveBeenCalledWith("Podcasts/My Title.mp3", buffer); + }); }); describe("downloadEpisode (API path)", () => { diff --git a/src/downloadEpisode.ts b/src/downloadEpisode.ts index 364aa085..fe4cc645 100644 --- a/src/downloadEpisode.ts +++ b/src/downloadEpisode.ts @@ -84,74 +84,81 @@ export default async function downloadEpisodeWithNotice( const SOME_LARGE_INT_SO_THE_BOX_DOESNT_AUTO_CLOSE = 999999999; const notice = new Notice(doc, SOME_LARGE_INT_SO_THE_BOX_DOESNT_AUTO_CLOSE); - update((bodyEl) => bodyEl.createEl("p", { text: "Starting download..." })); + // `finally` schedules the dismissal exactly once for every terminal state — + // success, write failure, download error, or the non-playable rejection below + // — so a failed download's notice can no longer linger forever (#DL-03). + try { + update((bodyEl) => bodyEl.createEl("p", { text: "Starting download..." })); - update((bodyEl) => { - bodyEl.createEl("p", { text: "Downloading..." }); - }); + update((bodyEl) => { + bodyEl.createEl("p", { text: "Downloading..." }); + }); - const { data, contentType } = await downloadFile(episode.streamUrl, { - onFinished: () => { - update((bodyEl) => bodyEl.createEl("p", { text: "Download complete!" })); - }, - onError: (error) => { - update((bodyEl) => - bodyEl.createEl("p", { - text: `Download failed: ${error.message}`, - }), - ); - }, - }); + const { data, contentType } = await downloadFile(episode.streamUrl, { + onFinished: () => { + update((bodyEl) => + bodyEl.createEl("p", { text: "Download complete!" }), + ); + }, + onError: (error) => { + update((bodyEl) => + bodyEl.createEl("p", { + text: `Download failed: ${error.message}`, + }), + ); + }, + }); - const inferredExtension = inferFileExtensionFromDownload( - episode, - data, - contentType, - ); + const inferredExtension = inferFileExtensionFromDownload( + episode, + data, + contentType, + ); - if ( - !downloadAppearsPlayable(contentType, inferredExtension, episode.mediaType) - ) { - update((bodyEl) => { - bodyEl.createEl("p", { - text: `Downloaded file is not an audio or video file. It is of type "${contentType}". File: ${data.byteLength} bytes.`, + if ( + !downloadAppearsPlayable(contentType, inferredExtension, episode.mediaType) + ) { + update((bodyEl) => { + bodyEl.createEl("p", { + text: `Downloaded file is not an audio or video file. It is of type "${contentType}". File: ${data.byteLength} bytes.`, + }); }); - }); - throw new Error("Not a playable media file"); - } - - const fileExtension = inferredExtension ?? "mp3"; + throw new Error("Not a playable media file"); + } - try { - update((bodyEl) => bodyEl.createEl("p", { text: "Creating file..." })); + const fileExtension = inferredExtension ?? "mp3"; - await createEpisodeFile({ - episode, - downloadPathTemplate, - data, - extension: fileExtension, - }); + try { + update((bodyEl) => bodyEl.createEl("p", { text: "Creating file..." })); - update((bodyEl) => - bodyEl.createEl("p", { - text: `Successfully downloaded "${episode.title}" from ${episode.podcastName}.`, - }), - ); - } catch (error: unknown) { - update((bodyEl) => { - bodyEl.createEl("p", { - text: `Failed to create file for downloaded episode "${episode.title}" from ${episode.podcastName}.`, + await createEpisodeFile({ + episode, + downloadPathTemplate, + data, + extension: fileExtension, }); - const errorMsgEl = bodyEl.createEl("p", { - text: getErrorMessage(error), + update((bodyEl) => + bodyEl.createEl("p", { + text: `Successfully downloaded "${episode.title}" from ${episode.podcastName}.`, + }), + ); + } catch (error: unknown) { + update((bodyEl) => { + bodyEl.createEl("p", { + text: `Failed to create file for downloaded episode "${episode.title}" from ${episode.podcastName}.`, + }); + + const errorMsgEl = bodyEl.createEl("p", { + text: getErrorMessage(error), + }); + errorMsgEl.style.fontStyle = "italic"; }); - errorMsgEl.style.fontStyle = "italic"; - }); + } + } finally { + setTimeout(() => notice.hide(), 10000); } - - setTimeout(() => notice.hide(), 10000); } function createNoticeDoc(title: string) { @@ -355,7 +362,20 @@ function inferFileExtensionFromDownload( const signatureExtension = detectAudioFileExtension(data); if (signatureExtension) { - return signatureExtension; + // A generic ISO-BMFF brand (mp42/isom/M4B...) resolves to "mp4"; for an + // audio download keep it as m4a so a podcast served as audio/mp4 isn't + // saved as *.mp4 and later treated as an ambiguous audio/video container + // (Codex review #213). The hint comes from the content type or, failing + // that, the episode's known media type. + const isAudioDownload = + getMediaTypeFromContentType(contentType) === "audio" || + episode.mediaType === "audio"; + return ( + normalizeAudioExtension( + signatureExtension, + isAudioDownload ? "audio" : undefined, + ) ?? signatureExtension + ); } if (contentTypeExtension) { @@ -369,11 +389,41 @@ function inferFileExtensionFromDownload( return null; } +/** + * Known non-media content types that can never be playable, even when the stream + * URL ends in `.mp3`/`.mp4`: an expired private feed or a CDN that answers a 200 + * with an HTML/JSON error page (#DL-07). Only KNOWN textual/document types are + * rejected here — `application/octet-stream` and other genuinely ambiguous binary + * types are intentionally NOT listed, because servers legitimately serve real + * media as octet-stream, so those still fall through to the extension/signature + * heuristic. + */ +function isKnownNonMediaContentType(contentType: string): boolean { + const normalizedType = contentType.split(";")[0].trim().toLowerCase(); + if (!normalizedType) return false; + + return ( + normalizedType.startsWith("text/") || + normalizedType === "application/json" || + normalizedType === "application/xml" || + // Structured XML/JSON families: rss+xml, atom+xml, xhtml+xml, problem+json, + // ld+json, etc. An expired/private feed often answers a media URL with one + // of these, and no real audio/video type uses a +xml/+json suffix (#DL-07 / + // Codex review #213). + normalizedType.endsWith("+xml") || + normalizedType.endsWith("+json") + ); +} + function downloadAppearsPlayable( contentType: string, extension: string | null, mediaTypeHint?: EpisodeMediaType, ): boolean { + if (isKnownNonMediaContentType(contentType)) { + return false; + } + const normalizedType = contentType.toLowerCase(); const contentMediaType = getMediaTypeFromContentType(normalizedType); @@ -397,6 +447,10 @@ function downloadAppearsAudio( extension: string | null, mediaTypeHint?: EpisodeMediaType, ): boolean { + if (isKnownNonMediaContentType(contentType)) { + return false; + } + const contentMediaType = getMediaTypeFromContentType(contentType); if (contentMediaType) { return ( @@ -689,6 +743,44 @@ interface AudioSignature { fileExtension: string; } +/** + * Map an ISO-BMFF (MP4/QuickTime) major brand to its file extension. Real m4a/mp4 + * files carry `ftyp` at offset 4 and the 4-byte major brand at offset 8 (not at + * offset 0), so the old offset-0 `M4A ` signature never matched (#DL-06). Known + * audio brands resolve to `m4a`; video brands to their own extension; everything + * else defaults to `mp4` (or `m4a` when the caller hints the media is audio). + */ +function detectIsoBmffExtension(arr: Uint8Array): string | null { + // Need 4 bytes of box size + 'ftyp' + the 4-byte major brand. + if (arr.length < 12) return null; + + const isFtyp = + arr[4] === 0x66 && arr[5] === 0x74 && arr[6] === 0x79 && arr[7] === 0x70; + if (!isFtyp) return null; + + const brand = String.fromCharCode(arr[8], arr[9], arr[10], arr[11]); + + if (brand.startsWith("M4A")) return "m4a"; + if (brand.startsWith("M4V")) return "m4v"; + if (brand === "qt ") return "mov"; + + // `M4B `/`M4P ` are audiobook/protected-audio brands; the rest are generic + // MP4 brands. All map to mp4 here — `inferFileExtensionFromDownload` / + // `normalizeAudioExtension` re-map mp4 -> m4a when the media hint is audio. + switch (brand) { + case "mp42": + case "isom": + case "iso2": + case "mp41": + case "dash": + case "M4B ": + case "M4P ": + return "mp4"; + default: + return "mp4"; + } +} + export function detectAudioFileExtension(data: ArrayBuffer): string | null { const audioSignatures: AudioSignature[] = [ { signature: [0xff, 0xe0], mask: [0xff, 0xe0], fileExtension: "mp3" }, @@ -696,7 +788,6 @@ export function detectAudioFileExtension(data: ArrayBuffer): string | null { { signature: [0x52, 0x49, 0x46, 0x46], fileExtension: "wav" }, { signature: [0x4f, 0x67, 0x67, 0x53], fileExtension: "ogg" }, { signature: [0x66, 0x4c, 0x61, 0x43], fileExtension: "flac" }, - { signature: [0x4d, 0x34, 0x41, 0x20], fileExtension: "m4a" }, { signature: [0x30, 0x26, 0xb2, 0x75, 0x8e, 0x66, 0xcf, 0x11], fileExtension: "wma", @@ -707,7 +798,10 @@ export function detectAudioFileExtension(data: ArrayBuffer): string | null { }, ]; + // The ftyp brand lives at offset 8, past every offset-0 signature, so read a + // header window long enough to cover both before zero-copy-viewing it. const maxSignatureLength = Math.max( + 12, ...audioSignatures.map((sig) => sig.signature.length), ); // Zero-copy view over just the header bytes — no Blob slice, no FileReader, @@ -718,6 +812,13 @@ export function detectAudioFileExtension(data: ArrayBuffer): string | null { Math.min(maxSignatureLength, data.byteLength), ); + // ISO-BMFF is special-cased first: its brand sits at offset 8, so it can't be + // expressed as an offset-0 signature like the entries above. + const isoBmffExtension = detectIsoBmffExtension(arr); + if (isoBmffExtension) { + return isoBmffExtension; + } + for (const { signature, mask, fileExtension } of audioSignatures) { if (signature.length > arr.length) { continue; diff --git a/src/getContextMenuHandler.ts b/src/getContextMenuHandler.ts index 0d452a87..42a8b215 100644 --- a/src/getContextMenuHandler.ts +++ b/src/getContextMenuHandler.ts @@ -1,5 +1,5 @@ import type { App, EventRef, Menu, TAbstractFile, WorkspaceLeaf } from "obsidian"; -import { TFile } from "obsidian"; +import { Notice, TFile } from "obsidian"; import { get } from "svelte/store"; import { VIEW_TYPE } from "./constants"; import { @@ -71,10 +71,27 @@ function addPlayLocalFileItem( // (see localFiles.syncWithDownloaded), so this single write // surfaces the file there too. Always refresh the entry: two // same-basename local files can differ by folder or media type. - downloadedEpisodes.addEpisode(localEpisode, file.path, file.stat.size); + // addEpisode reports a basename collision (a different file already + // tracked under this name) so we can warn here — the store stays pure + // (#LF-06). + const replacedFilePath = downloadedEpisodes.addEpisode( + localEpisode, + file.path, + file.stat.size, + ); + if (replacedFilePath) { + new Notice( + `A local file named "${file.basename}" was already tracked at "${replacedFilePath}". It has been replaced by "${file.path}".`, + ); + } - // Fixes where the episode won't play if it has been played. - if (get(playedEpisodes)[file.basename]?.finished) { + // Re-enable replay for a finished local file. Look it up with + // playedEpisodes.get (which resolves the composite "local + // file::" key and falls back to legacy bare-title), not + // the raw bare-basename map — markAsPlayed/setEpisodeTime store under + // the composite key, so the old bare-basename lookup never matched + // and a finished local file refused to replay (CM-10/LF-09). + if (playedEpisodes.get(localEpisode)?.finished) { playedEpisodes.markAsUnplayed(localEpisode); } diff --git a/src/getUniversalPodcastLink.test.ts b/src/getUniversalPodcastLink.test.ts new file mode 100644 index 00000000..960ff072 --- /dev/null +++ b/src/getUniversalPodcastLink.test.ts @@ -0,0 +1,252 @@ +import { afterEach, beforeEach, describe, expect, test, vi } from "vitest"; +import { requestUrl } from "obsidian"; + +import getUniversalPodcastLink from "./getUniversalPodcastLink"; +import { queryiTunesPodcasts } from "./iTunesAPIConsumer"; +import { savedFeeds } from "./store"; +import type { IAPI } from "./API/IAPI"; +import type { Episode } from "./types/Episode"; + +const noticeSpy = vi.fn(); + +vi.mock("obsidian", async (importOriginal) => { + const actual = await importOriginal(); + return { + ...actual, + requestUrl: vi.fn(), + // A spy class so we can assert which user-facing messages were shown. + Notice: class { + message?: string; + constructor(message?: string) { + this.message = message; + noticeSpy(message); + } + hide(): void {} + }, + }; +}); + +vi.mock("./iTunesAPIConsumer", () => ({ + queryiTunesPodcasts: vi.fn(), +})); + +const requestUrlMock = vi.mocked(requestUrl); +const queryiTunesMock = vi.mocked(queryiTunesPodcasts); + +const podcast: Episode = { + title: "Episode One", + streamUrl: "https://feeds.example.com/ep1.mp3", + url: "https://feeds.example.com/ep1", + description: "", + content: "", + podcastName: "Example Show", + feedUrl: "https://feeds.example.com/rss", +}; + +const api = { podcast } as unknown as IAPI; + +function noticeMessages(): (string | undefined)[] { + return noticeSpy.mock.calls.map((call) => call[0] as string | undefined); +} + +let writeTextMock: ReturnType; + +function setClipboard(impl?: () => Promise) { + writeTextMock = vi.fn(impl ?? (() => Promise.resolve())); + Object.defineProperty(globalThis, "navigator", { + value: { clipboard: { writeText: writeTextMock } }, + configurable: true, + writable: true, + }); +} + +function removeClipboard() { + Object.defineProperty(globalThis, "navigator", { + value: {}, + configurable: true, + writable: true, + }); +} + +beforeEach(() => { + vi.clearAllMocks(); + vi.spyOn(console, "error").mockImplementation(() => {}); + savedFeeds.set({}); + requestUrlMock.mockResolvedValue({ + status: 200, + json: { episodes: [{ episodeId: "ep-123", title: "Episode One" }] }, + } as never); + queryiTunesMock.mockResolvedValue([]); + setClipboard(); +}); + +afterEach(() => { + vi.restoreAllMocks(); + savedFeeds.set({}); +}); + +describe("getUniversalPodcastLink", () => { + test("uses the saved feed's collectionId and skips the iTunes re-query", async () => { + savedFeeds.set({ + "Example Show": { + title: "Example Show", + url: "https://feeds.example.com/rss", + artworkUrl: "", + collectionId: "555", + }, + }); + + await getUniversalPodcastLink(api); + + expect(queryiTunesMock).not.toHaveBeenCalled(); + expect(requestUrlMock).toHaveBeenCalledWith({ + url: "https://pod.link/555.json?limit=1000", + }); + expect(writeTextMock).toHaveBeenCalledWith( + "https://pod.link/555/episode/ep-123", + ); + expect(noticeMessages()).toContain( + "Universal episode link copied to clipboard.", + ); + }); + + test("matches a saved feed by normalized url when the key differs", async () => { + savedFeeds.set({ + "Different Key": { + title: "Different Key", + // Trailing slash + different case must still match the episode feedUrl. + url: "HTTPS://Feeds.Example.com/RSS/", + artworkUrl: "", + collectionId: "777", + }, + }); + + await getUniversalPodcastLink(api); + + expect(queryiTunesMock).not.toHaveBeenCalled(); + expect(writeTextMock).toHaveBeenCalledWith( + "https://pod.link/777/episode/ep-123", + ); + }); + + test("prefers a saved feed whose URL matches over the name-keyed entry (#213)", async () => { + savedFeeds.set({ + // Name-keyed entry that points at a DIFFERENT feed. + "Example Show": { + title: "Example Show", + url: "https://other.example.com/rss", + artworkUrl: "", + collectionId: "111", + }, + // A different key whose URL matches the playing episode's feedUrl. + Mirror: { + title: "Mirror", + url: "https://feeds.example.com/rss", + artworkUrl: "", + collectionId: "222", + }, + }); + + await getUniversalPodcastLink(api); + + expect(queryiTunesMock).not.toHaveBeenCalled(); + expect(writeTextMock).toHaveBeenCalledWith( + "https://pod.link/222/episode/ep-123", + ); + }); + + test("falls back to a tolerant iTunes match when no saved collectionId exists", async () => { + queryiTunesMock.mockResolvedValue([ + { + title: "Unrelated title casing", + url: "https://feeds.example.com/rss/", + artworkUrl: "", + collectionId: "999", + }, + ]); + + await getUniversalPodcastLink(api); + + expect(queryiTunesMock).toHaveBeenCalledWith("Example Show"); + expect(writeTextMock).toHaveBeenCalledWith( + "https://pod.link/999/episode/ep-123", + ); + }); + + test("prefers the feed-URL match over an earlier same-title iTunes result (#213)", async () => { + queryiTunesMock.mockResolvedValue([ + { + // A duplicate/rehosted show with the SAME title but a different feed, + // returned first by iTunes. + title: "Example Show", + url: "https://other.example.com/rss", + artworkUrl: "", + collectionId: "111", + }, + { + // The real one, matched by normalized feed URL (trailing slash/case). + title: "Example Show (mirror)", + url: "https://feeds.example.com/rss/", + artworkUrl: "", + collectionId: "222", + }, + ]); + + await getUniversalPodcastLink(api); + + expect(writeTextMock).toHaveBeenCalledWith( + "https://pod.link/222/episode/ep-123", + ); + }); + + test("shows an actionable notice when the podcast cannot be matched", async () => { + queryiTunesMock.mockResolvedValue([]); + + await getUniversalPodcastLink(api); + + expect(requestUrlMock).not.toHaveBeenCalled(); + expect(writeTextMock).not.toHaveBeenCalled(); + expect(noticeMessages()).toContain( + 'Could not find "Example Show" on Apple Podcasts to build a universal link.', + ); + }); + + test("does not announce success when the clipboard write rejects", async () => { + savedFeeds.set({ + "Example Show": { + title: "Example Show", + url: "https://feeds.example.com/rss", + artworkUrl: "", + collectionId: "555", + }, + }); + setClipboard(() => Promise.reject(new Error("denied"))); + + await getUniversalPodcastLink(api); + + expect(noticeMessages()).not.toContain( + "Universal episode link copied to clipboard.", + ); + expect(noticeMessages()).toContain( + "Could not copy to clipboard. Episode link: https://pod.link/555/episode/ep-123", + ); + }); + + test("surfaces the link when the clipboard API is unavailable", async () => { + savedFeeds.set({ + "Example Show": { + title: "Example Show", + url: "https://feeds.example.com/rss", + artworkUrl: "", + collectionId: "555", + }, + }); + removeClipboard(); + + await getUniversalPodcastLink(api); + + expect(noticeMessages()).toContain( + "Clipboard unavailable. Episode link: https://pod.link/555/episode/ep-123", + ); + }); +}); diff --git a/src/getUniversalPodcastLink.ts b/src/getUniversalPodcastLink.ts index ab6b727a..e3baedaa 100644 --- a/src/getUniversalPodcastLink.ts +++ b/src/getUniversalPodcastLink.ts @@ -1,30 +1,74 @@ import { requestUrl, Notice } from "obsidian"; +import { get } from "svelte/store"; import type { IAPI } from "./API/IAPI"; import { queryiTunesPodcasts } from "./iTunesAPIConsumer"; +import { savedFeeds } from "./store"; + +const normalizeFeedUrl = (url: string | undefined): string => + (url ?? "").trim().replace(/\/+$/, "").toLowerCase(); + +/** + * Resolve the iTunes collectionId for the loaded episode's podcast. Prefer the + * saved feed's collectionId (populated for iTunes-added feeds) so we skip the + * iTunes re-query and the brittle channel-title/url comparison entirely. Only + * when it is unavailable (custom-URL/OPML feeds) do we fall back to a tolerant + * iTunes lookup that normalizes the feed URL before comparing. + */ +async function resolveCollectionId( + podcastName: string, + feedUrl: string | undefined, +): Promise { + const feeds = get(savedFeeds); + const targetUrl = normalizeFeedUrl(feedUrl); + // Prefer a normalized feed-URL match above everything else (even the + // podcastName-keyed entry): duplicate or rehosted podcast titles mean a + // same-name entry can point at a different feed while another saved entry's + // URL actually matches the playing episode. Only when no URL match exists do + // we fall back to the name key, then a title match (Codex review #213). + const savedFeed = + (targetUrl + ? Object.values(feeds).find( + (feed) => normalizeFeedUrl(feed.url) === targetUrl, + ) + : undefined) ?? + feeds[podcastName] ?? + Object.values(feeds).find((feed) => feed.title === podcastName); + + if (savedFeed?.collectionId) { + return savedFeed.collectionId; + } + + const iTunesResponse = await queryiTunesPodcasts(podcastName); + const match = + (targetUrl + ? iTunesResponse.find((pod) => normalizeFeedUrl(pod.url) === targetUrl) + : undefined) ?? + iTunesResponse.find((pod) => pod.title === podcastName); + + return match?.collectionId; +} export default async function getUniversalPodcastLink(api: IAPI) { const { title, itunesTitle, podcastName, feedUrl } = api.podcast; - - try { - const iTunesResponse = await queryiTunesPodcasts( - api.podcast.podcastName - ); - const podcast = iTunesResponse.find( - (pod) => pod.title === podcastName && pod.url === feedUrl - ); - if (!podcast || !podcast.collectionId) { - throw new Error("Failed to get podcast from iTunes."); + let url: string; + try { + const collectionId = await resolveCollectionId(podcastName, feedUrl); + if (!collectionId) { + new Notice( + `Could not find "${podcastName}" on Apple Podcasts to build a universal link.`, + ); + return; } - const podLinkUrl = `https://pod.link/${podcast.collectionId}.json?limit=1000`; + const podLinkUrl = `https://pod.link/${collectionId}.json?limit=1000`; const res = await requestUrl({ url: podLinkUrl, }); if (res.status !== 200) { throw new Error( - `Failed to get response from pod.link: ${podLinkUrl}` + `Failed to get response from pod.link: ${podLinkUrl}`, ); } @@ -35,23 +79,35 @@ export default async function getUniversalPodcastLink(api: IAPI) { episodeId: string; title: string; [key: string]: string; - }) => episode.title === targetTitle + }) => episode.title === targetTitle, ); if (!ep) { - throw new Error( - `Failed to find episode "${targetTitle}" on pod.link. URL: ${podLinkUrl}` + new Notice( + `Could not find episode "${targetTitle}" on pod.link to build a universal link.`, ); + return; } - window.navigator.clipboard.writeText( - `https://pod.link/${podcast.collectionId}/episode/${ep.episodeId}` - ); - - new Notice("Universal episode link copied to clipboard."); + url = `https://pod.link/${collectionId}/episode/${ep.episodeId}`; } catch (error) { new Notice("Could not get podcast link."); console.error(error); return; } + + // Copy outside the lookup try/catch so a clipboard failure is reported on its + // own terms, never mislabeled as a lookup failure. navigator.clipboard is + // undefined on mobile / non-secure contexts, so fall back to surfacing the URL. + if (navigator.clipboard?.writeText) { + try { + await navigator.clipboard.writeText(url); + new Notice("Universal episode link copied to clipboard."); + } catch (error) { + console.error(error); + new Notice(`Could not copy to clipboard. Episode link: ${url}`); + } + } else { + new Notice(`Clipboard unavailable. Episode link: ${url}`); + } } diff --git a/src/main.activateView.test.ts b/src/main.activateView.test.ts index 0aed7476..2faf8ef1 100644 --- a/src/main.activateView.test.ts +++ b/src/main.activateView.test.ts @@ -277,7 +277,11 @@ describe("PodNotes onload wiring (#55)", () => { expect(showCmd?.checkCallback).toBeUndefined(); }); - it("registers Capture Timestamp as an editor callback so mobile toolbar can add it", async () => { + it("registers Capture Timestamp as an editor callback so the mobile toolbar can add it", async () => { + // Must stay an editorCallback (not editorCheckCallback): an unconditional + // editor command remains addable to the mobile editor toolbar / command + // picker before any episode is loaded; it surfaces a Notice when capture + // isn't possible rather than no-opping silently (TS-01 / Codex review #213). const { commands } = await loadPlugin(); const captureCmd = commands.find((c) => c.id === "capture-timestamp"); diff --git a/src/main.ts b/src/main.ts index 9ddd46da..c9487ca5 100644 --- a/src/main.ts +++ b/src/main.ts @@ -17,6 +17,7 @@ import { import { bindStoresToSettings } from "src/store/persistence"; import { registerCommands } from "src/commands"; import { + Notice, Platform, Plugin, type Editor, @@ -27,7 +28,9 @@ import type { IAPI } from "src/API/IAPI"; import { DEFAULT_SETTINGS, VIEW_TYPE } from "src/constants"; import { migrateDownloadPath, + migrateFeedNoteSettings, migrateNoteSettings, + migrateSkipLength, migrateTranscriptSettings, } from "src/settingsMigrations"; import { PodNotesSettingsTab } from "src/ui/settings/PodNotesSettingsTab"; @@ -316,16 +319,32 @@ export default class PodNotes extends Plugin implements IPodNotes { return false; } - const capture = TimestampTemplateEngine(this.settings.timestamp.template); - const textToAppend = capture.endsWith("\n") ? capture : `${capture}\n`; - const file = - getPodcastNote(this.api.podcast) ?? - (await createPodcastNoteFileIfNotExists(this.api.podcast)); - const content = await this.app.vault.read(file); - const separator = content.length > 0 && !content.endsWith("\n") ? "\n" : ""; - - await this.app.vault.modify(file, `${content}${separator}${textToAppend}`); - return true; + // This path runs from the media-session (headphone) handler, where there is + // no editor to surface a failure, so swallowing an error left a button + // press looking like it did nothing. Report it instead (NT-13). + try { + const capture = TimestampTemplateEngine(this.settings.timestamp.template); + const textToAppend = capture.endsWith("\n") ? capture : `${capture}\n`; + const file = + getPodcastNote(this.api.podcast) ?? + (await createPodcastNoteFileIfNotExists(this.api.podcast)); + const content = await this.app.vault.read(file); + const separator = + content.length > 0 && !content.endsWith("\n") ? "\n" : ""; + + await this.app.vault.modify( + file, + `${content}${separator}${textToAppend}`, + ); + return true; + } catch (error) { + console.error( + "PodNotes: failed to capture timestamp into episode note", + error, + ); + new Notice("Failed to capture timestamp into episode note"); + return false; + } } private async captureTimestampFromMediaSession(): Promise { @@ -423,6 +442,21 @@ export default class PodNotes extends Plugin implements IPodNotes { this.settings.episodeListLimit = sanitizeEpisodeListLimit( this.settings.episodeListLimit, ); + // Repair persisted skip lengths so a cleared field (NaN -> null in JSON) + // can't feed the skip arithmetic and corrupt the playback position (PB-02). + this.settings.skipBackwardLength = migrateSkipLength( + this.settings.skipBackwardLength, + DEFAULT_SETTINGS.skipBackwardLength, + ); + this.settings.skipForwardLength = migrateSkipLength( + this.settings.skipForwardLength, + DEFAULT_SETTINGS.skipForwardLength, + ); + // Self-heal a corrupt persisted default playback rate so the loaded store + // and the settings slider both read a clamped value (ST-02). + this.settings.defaultPlaybackRate = normalizePlaybackRate( + this.settings.defaultPlaybackRate, + ); // Upgrade the legacy empty episode-note default to the Bases-friendly // default, preserving any path/template the user configured (#160). Returns // a fresh object, so DEFAULT_SETTINGS.note is never mutated. @@ -433,6 +467,9 @@ export default class PodNotes extends Plugin implements IPodNotes { this.settings.transcript = migrateTranscriptSettings( loadedData?.transcript, ); + // Backfill a partial/legacy feedNote so a missing template can't crash + // createFeedNote's `template.replace(...)` (ST-08). + this.settings.feedNote = migrateFeedNoteSettings(loadedData?.feedNote); } async saveSettings() { diff --git a/src/opml.test.ts b/src/opml.test.ts new file mode 100644 index 00000000..e3ec161e --- /dev/null +++ b/src/opml.test.ts @@ -0,0 +1,192 @@ +import { get, writable } from "svelte/store"; +import { beforeEach, describe, expect, it, type Mock, vi } from "vitest"; +import type { PodcastFeed } from "./types/PodcastFeed"; + +// Mock the heavy collaborators so the tests exercise only opml.ts's own +// 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) => ({ + title: `Feed for ${url}`, + url, + artworkUrl: "", +})); + +vi.mock("./store", () => ({ + get savedFeeds() { + return savedFeeds; + }, +})); + +vi.mock("./parser/feedParser", () => ({ + default: class { + getFeed = getFeed; + }, +})); + +// The shared obsidian mock's Notice lacks setMessage, which TimerNotice calls +// on its progress ticker. Provide a minimal Notice with the methods opml.ts +// uses so the import flow can run to completion under jsdom. +vi.mock("obsidian", () => ({ + Notice: class { + constructor(public message?: unknown) {} + setMessage() {} + hide() {} + }, +})); + +import { exportOPML, importOPML } from "./opml"; + +beforeEach(() => { + savedFeeds.set({}); + getFeed.mockClear(); +}); + +/** Minimal Obsidian App stub capturing the single file vault.create writes. */ +function makeApp(): { + app: { vault: { create: Mock } }; + created: { path: string; data: string }[]; +} { + const created: { path: string; data: string }[] = []; + const create = vi.fn(async (path: string, data: string) => { + created.push({ path, data }); + }); + return { app: { vault: { create } }, created }; +} + +describe("exportOPML (IE-02)", () => { + it("uses a well-formed XML declaration (utf-8, not utf=8)", async () => { + const { app, created } = makeApp(); + await exportOPML(app as never, [], "out.opml"); + + expect(created[0]?.data).toContain('encoding="utf-8"'); + expect(created[0]?.data).not.toContain("utf=8"); + }); + + it('XML-escapes &, <, >, and " in titles and URLs', async () => { + const { app, created } = makeApp(); + const feeds: PodcastFeed[] = [ + { + title: 'Tom & Jerry <"news">', + url: "https://x.test/feed?a=1&b=2", + artworkUrl: "", + }, + ]; + + await exportOPML(app as never, feeds, "out.opml"); + const doc = created[0]?.data ?? ""; + + // Raw special characters must not survive into attribute values. + expect(doc).toContain("Tom & Jerry <"news">"); + expect(doc).toContain("https://x.test/feed?a=1&b=2"); + // `&` is escaped first, so entities are not double-escaped. + expect(doc).not.toContain("&amp;"); + }); + + it("round-trips an escaped export back through importOPML", async () => { + const { app, created } = makeApp(); + const feeds: PodcastFeed[] = [ + { + title: "Tom & Jerry", + url: "https://x.test/feed?a=1&b=2", + artworkUrl: "", + }, + ]; + + await exportOPML(app as never, feeds, "out.opml"); + await importOPML(created[0]?.data ?? ""); + + // The escaped attributes parse back to the original raw values. + expect(getFeed).toHaveBeenCalledTimes(1); + expect(getFeed).toHaveBeenCalledWith("https://x.test/feed?a=1&b=2"); + }); +}); + +describe("importOPML (IE-01)", () => { + it("throws on invalid XML (parsererror is detected)", async () => { + const consoleError = vi + .spyOn(console, "error") + .mockImplementation(() => {}); + + await importOPML(""); + + // importOPML catches and logs rather than rethrowing; assert it surfaced + // the invalid-XML error and never reached the feed fetch. + expect(consoleError).toHaveBeenCalledWith( + "Error importing OPML:", + expect.objectContaining({ message: "Invalid XML format" }), + ); + expect(getFeed).not.toHaveBeenCalled(); + + consoleError.mockRestore(); + }); + + it("does not false-positive on valid OPML", async () => { + const validOpml = + '' + + 't' + + '' + + '' + + ""; + + await importOPML(validOpml); + + expect(getFeed).toHaveBeenCalledTimes(1); + expect(getFeed).toHaveBeenCalledWith("https://a.test/feed"); + }); + + it("imports feeds whose xmlUrl attribute is lower/variant cased", async () => { + const opml = + '' + + "" + + '' + + '' + + '' + + '' + + ""; + + await importOPML(opml); + + const urls = getFeed.mock.calls.map((c) => c[0]).sort(); + expect(urls).toEqual([ + "https://camel.test/feed", + "https://lower.test/feed", + "https://mixed.test/feed", + "https://upper.test/feed", + ]); + }); + + it("skips outlines with an empty or missing url attribute", async () => { + const opml = + "" + + '' + + '' + + '' + + ""; + + await importOPML(opml); + + expect(getFeed).toHaveBeenCalledTimes(1); + expect(getFeed).toHaveBeenCalledWith("https://good.test/feed"); + }); + + it("does not re-fetch a feed whose url is already saved", async () => { + savedFeeds.set({ + Existing: { + title: "Existing", + url: "https://dup.test/feed", + artworkUrl: "", + }, + }); + + const opml = + "" + + '' + + ""; + + await importOPML(opml); + + expect(getFeed).not.toHaveBeenCalled(); + expect(Object.keys(get(savedFeeds))).toEqual(["Existing"]); + }); +}); diff --git a/src/opml.ts b/src/opml.ts index addfe991..aa0a7c5c 100644 --- a/src/opml.ts +++ b/src/opml.ts @@ -4,6 +4,27 @@ import { savedFeeds } from "./store"; import type { PodcastFeed } from "./types/PodcastFeed"; import { get } from "svelte/store"; +/** + * Read an attribute by name regardless of its casing. OPML in the wild is + * inconsistent about the `xmlUrl` attribute (xmlurl, XmlUrl, xmlURL, ...), so a + * fixed-case `getAttribute("xmlUrl")` silently drops otherwise-valid feeds. + * Returns the trimmed value, or null when absent/empty. + */ +function getAttributeCaseInsensitive( + node: Element, + name: string, +): string | null { + const target = name.toLowerCase(); + for (let i = 0; i < node.attributes.length; i++) { + const attr = node.attributes.item(i); + if (attr && attr.name.toLowerCase() === target) { + const value = attr.value?.trim(); + return value ? value : null; + } + } + return null; +} + function TimerNotice(heading: string, initialMessage: string) { let currentMessage = initialMessage; const startTime = Date.now(); @@ -60,17 +81,24 @@ async function importOPML(opml: string): Promise { const dp = new DOMParser(); const dom = dp.parseFromString(opml, "application/xml"); - if (dom.documentElement.nodeName === "parsererror") { + // DOMParser reports a malformed document by injecting a + // element rather than throwing. The previous root-nodeName check only + // worked in jsdom; Chromium (Obsidian's runtime) nests parsererror under + // the document body, so querySelector is the engine-agnostic detection. + if (dom.querySelector("parsererror")) { throw new Error("Invalid XML format"); } - const podcastEntryNodes = dom.querySelectorAll("outline[text][xmlUrl]"); + // Select on the always-lowercase tag and the camelCase-safe `text` attr; + // the xmlUrl attribute is read case-insensitively below so feeds written + // with a lowercase/variant casing (e.g. `xmlurl`) aren't silently dropped. + const podcastEntryNodes = dom.querySelectorAll("outline[text]"); const incompletePodcastsToAdd: Pick[] = []; for (let i = 0; i < podcastEntryNodes.length; i++) { const node = podcastEntryNodes.item(i); const text = node.getAttribute("text"); - const xmlUrl = node.getAttribute("xmlUrl"); + const xmlUrl = getAttributeCaseInsensitive(node, "xmlUrl"); if (!text || !xmlUrl) { continue; } @@ -168,13 +196,23 @@ async function exportOPML( feeds: PodcastFeed[], filePath = "PodNotes_Export.opml", ) { - const header = ``; + const header = ``; const opml = (child: string) => `${child}`; const head = (child: string) => `${child}`; const title = "PodNotes Feeds"; const body = (child: string) => `${child}`; + // Escape every interpolated attribute value so titles/URLs containing &, <, + // >, or " produce well-formed XML that round-trips back through importOPML. + // `&` must run first so the entities written by the later replaces are not + // themselves re-escaped. + const escAttr = (s: string) => + s + .replace(/&/g, "&") + .replace(//g, ">") + .replace(/"/g, """); const feedOutline = (feed: PodcastFeed) => - ``; + ``; const feedsOutline = (_feeds: PodcastFeed[]) => `${feeds.map(feedOutline).join("")}`; diff --git a/src/parser/feedParser.test.ts b/src/parser/feedParser.test.ts index 2c187318..3ec8e9ed 100644 --- a/src/parser/feedParser.test.ts +++ b/src/parser/feedParser.test.ts @@ -247,6 +247,35 @@ describe("FeedParser", () => { expect(feed.artworkUrl).toBe("https://example.com/itunes-artwork.jpg"); }); + test("does not adopt an item image as feed artwork when the channel has none (FP-02)", async () => { + const feedWithOnlyItemImage = ` + + + No Channel Image Podcast + https://example.com + + Episode 1 + + Mon, 01 Jan 2024 00:00:00 GMT + + + +`; + mockRequestWithTimeout.mockResolvedValueOnce({ + text: feedWithOnlyItemImage, + status: 200, + headers: {}, + arrayBuffer: new ArrayBuffer(0), + json: {}, + }); + + const parser = new FeedParser(); + const feed = await parser.getFeed("https://example.com/feed.xml"); + + // The item's image must NOT leak up as the feed artwork. + expect(feed.artworkUrl).toBe(""); + }); + test("throws error for invalid RSS feed without title", async () => { mockRequestWithTimeout.mockResolvedValueOnce({ text: invalidRssFeed, diff --git a/src/parser/feedParser.ts b/src/parser/feedParser.ts index e3ea0fb9..987effb0 100644 --- a/src/parser/feedParser.ts +++ b/src/parser/feedParser.ts @@ -30,7 +30,6 @@ export default class FeedParser { const body = await this.parseFeed(url); const titleEl = body.querySelector("title"); - const itunesImageEl = this.findImageElement(body); // A feed must have a title. is intentionally NOT required: it is the // human website (now surfaced as {{url}} in feed notes), but many valid @@ -40,6 +39,11 @@ export default class FeedParser { } const channel = body.querySelector("channel") ?? body; + // Resolve artwork from the channel's DIRECT children only. Searching the + // whole document (getElementsByTagName/querySelector) recurses into + // descendants, so a feed with no channel image but item images + // would wrongly adopt an episode's image as its artwork (FP-02). + const itunesImageEl = this.findChannelImageElement(channel); const title = titleEl.textContent || ""; const artworkUrl = @@ -82,6 +86,24 @@ export default class FeedParser { return doc.querySelector("image"); } + /** + * Resolve the feed's image from the channel's DIRECT children only, so a + * nested image never leaks up as the feed artwork (FP-02). Prefers + * then a generic . Unlike findImageElement + * (used for items, which can't nest images), this never recurses. + */ + private findChannelImageElement(scope: Element | Document): Element | null { + const directChildren = Array.from(scope.children); + const itunesImage = directChildren.find( + (el) => el.tagName.toLowerCase() === "itunes:image", + ); + if (itunesImage) return itunesImage; + + return ( + directChildren.find((el) => el.tagName.toLowerCase() === "image") ?? null + ); + } + /** * Resolve the feed's website URL from the channel's direct children. * Prefers a whose text content is an http(s) URL; RSS feeds commonly diff --git a/src/services/TranscriptionService.test.ts b/src/services/TranscriptionService.test.ts index c00d74c0..8b873fa7 100644 --- a/src/services/TranscriptionService.test.ts +++ b/src/services/TranscriptionService.test.ts @@ -1,8 +1,26 @@ import { describe, expect, test, vi, beforeEach } from "vitest"; +import { Notice } from "obsidian"; import { TranscriptionService } from "./TranscriptionService"; import type { Episode } from "src/types/Episode"; import type PodNotes from "src/main"; +// The shared obsidian mock's Notice has no setMessage; TimerNotice needs one. +(Notice.prototype as unknown as { setMessage: () => void }).setMessage = () => {}; + +const getEpisodeAudioBufferMock = vi.fn(); +const transcriptionsCreateMock = vi.fn(); + +vi.mock("../downloadEpisode", () => ({ + getEpisodeAudioBuffer: (...args: unknown[]) => + getEpisodeAudioBufferMock(...args), +})); + +vi.mock("openai", () => ({ + OpenAI: class { + audio = { transcriptions: { create: transcriptionsCreateMock } }; + }, +})); + const mockEpisode: Episode = { title: "Test Episode", streamUrl: "https://example.com/episode.mp3", @@ -64,6 +82,11 @@ function createMockPlugin(overrides: { describe("TranscriptionService", () => { beforeEach(() => { vi.clearAllMocks(); + getEpisodeAudioBufferMock.mockResolvedValue({ + buffer: new ArrayBuffer(1024), + extension: "mp3", + basename: "episode", + }); }); describe("formatTime", () => { @@ -123,4 +146,94 @@ describe("TranscriptionService", () => { }); }); + describe("empty Whisper transcript (TR-01)", () => { + // transcribeEpisode is private; drive it directly so the empty-body throw is + // observed as "no file written" (the throw is caught and surfaced as a + // "Transcription failed" notice, never as a saved transcript). + const runTranscribeEpisode = async (plugin: PodNotes) => { + const service = new TranscriptionService(plugin); + await ( + service as unknown as { + transcribeEpisode: (episode: Episode) => Promise; + } + ).transcribeEpisode(mockEpisode); + return service; + }; + + test("does not write a file when the transcript is empty", async () => { + transcriptionsCreateMock.mockResolvedValue({ text: "" }); + const plugin = createMockPlugin(); + + await runTranscribeEpisode(plugin); + + expect(plugin.app.vault.create).not.toHaveBeenCalled(); + }); + + test("does not write a file when the transcript is only whitespace", async () => { + // Multiple empty chunks join to " ", not "" — the trimmed-emptiness check + // must still treat this as failure. + transcriptionsCreateMock.mockResolvedValue({ text: " \n\t " }); + const plugin = createMockPlugin(); + + await runTranscribeEpisode(plugin); + + expect(plugin.app.vault.create).not.toHaveBeenCalled(); + }); + + test("writes a file when the transcript has content", async () => { + transcriptionsCreateMock.mockResolvedValue({ + text: "Hello world. This is a transcript.", + }); + const plugin = createMockPlugin(); + + await runTranscribeEpisode(plugin); + + expect(plugin.app.vault.create).toHaveBeenCalledTimes(1); + }); + }); + + describe("buildTranscriptBody (TR-01)", () => { + const buildBody = (plugin: PodNotes) => { + const service = new TranscriptionService(plugin); + return ( + service as unknown as { + buildTranscriptBody: ( + audio: { + buffer: ArrayBuffer; + mimeType: string; + extension: string; + basename: string; + }, + update: (message: string) => void, + ) => Promise; + } + ).buildTranscriptBody( + { + buffer: new ArrayBuffer(1024), + mimeType: "audio/mpeg", + extension: "mp3", + basename: "episode", + }, + () => {}, + ); + }; + + test("throws when the trimmed Whisper body is empty", async () => { + transcriptionsCreateMock.mockResolvedValue({ text: " \n " }); + + await expect(buildBody(createMockPlugin())).rejects.toThrow( + "Transcription returned no text.", + ); + }); + + test("returns the reflowed body when there is text", async () => { + transcriptionsCreateMock.mockResolvedValue({ + text: "One. Two.", + }); + + await expect(buildBody(createMockPlugin())).resolves.toBe( + "One.\n\nTwo.", + ); + }); + }); }); diff --git a/src/services/TranscriptionService.ts b/src/services/TranscriptionService.ts index 023791db..296a5672 100644 --- a/src/services/TranscriptionService.ts +++ b/src/services/TranscriptionService.ts @@ -198,8 +198,9 @@ export class TranscriptionService { * Produce the transcript body that fills the template's `{{transcript}}` tag. * Plain Whisper returns one run-on block, so it is reflowed after sentence * periods for readability; diarization returns speaker-labeled turns already - * separated into paragraphs, so it is rendered as-is. Diarization that yields - * no speech is treated as a failure rather than writing an empty transcript. + * separated into paragraphs, so it is rendered as-is. Either path yielding no + * speech is treated as a failure rather than writing an empty transcript (empty + * Whisper chunks join to whitespace, so the body is trimmed before the check). */ private async buildTranscriptBody( audio: DiarizationAudio, @@ -223,7 +224,12 @@ export class TranscriptionService { const files = await createChunkFiles(audio); updateNotice("Starting transcription..."); const transcription = await this.transcribeChunks(files, updateNotice); - return transcription.replace(/\.\s+/g, ".\n\n"); + // Empty chunks join to " " (not ""), so trim before deciding it is empty. + const body = transcription.trim().replace(/\.\s+/g, ".\n\n"); + if (body.length === 0) { + throw new Error("Transcription returned no text."); + } + return body; } /** Route the episode audio to the configured diarization provider (#168). */ @@ -247,8 +253,9 @@ export class TranscriptionService { }); } - // OpenAI diarization shares Whisper's 25 MB/request cap, so reuse the same - // chunking. Speaker labels can differ across chunks on a long episode. + // OpenAI diarization shares Whisper's ~20 MB chunk limit (a conservative + // margin under OpenAI's 25 MB request cap), so reuse the same chunking. + // Speaker labels can differ across chunks on a long episode. updateNotice("Creating audio chunks..."); const chunkFiles = await createChunkFiles(audio); return diarizeWithOpenAI({ diff --git a/src/services/audioChunker.ts b/src/services/audioChunker.ts index 5b2d10e5..3c75d799 100644 --- a/src/services/audioChunker.ts +++ b/src/services/audioChunker.ts @@ -6,7 +6,8 @@ * service just orchestrates it. */ -// OpenAI's audio upload limit. Buffers at or under this are sent as a single file. +// OpenAI's per-request limit is 25 MB; we cap at 20 MB for a safety margin. +// Buffers at or under this are sent as a single file. export const CHUNK_SIZE_BYTES = 20 * 1024 * 1024; const WAV_HEADER_SIZE = 44; const PCM_BYTES_PER_SAMPLE = 2; diff --git a/src/services/diarization/openaiProvider.ts b/src/services/diarization/openaiProvider.ts index 89949e7c..ef3e7bad 100644 --- a/src/services/diarization/openaiProvider.ts +++ b/src/services/diarization/openaiProvider.ts @@ -5,9 +5,10 @@ import { parseOpenAIDiarizedSegments } from "./segments"; /** * Diarize with OpenAI's `gpt-4o-transcribe-diarize` model (issue #168). * - * Reuses the same chunked `File[]` the Whisper path builds because the - * transcription endpoint caps a request at 25 MB. `chunking_strategy: "auto"` is - * required for audio longer than 30s and lets the model segment within a chunk. + * Reuses the same chunked `File[]` the Whisper path builds because the chunker + * caps each request at ~20 MB (a conservative margin under OpenAI's 25 MB request + * cap). `chunking_strategy: "auto"` is required for audio longer than 30s and lets + * the model segment within a chunk. * Speaker labels are assigned per request, so on a multi-chunk (long) episode * the labels can differ across chunk boundaries — the caller documents this; a * single-chunk episode (the common case) is fully consistent. @@ -59,7 +60,7 @@ export async function diarizeWithOpenAI(opts: { attempt++; if (attempt >= maxRetries) { console.error( - `OpenAI diarization failed for chunk ${index} after ${maxRetries} attempts:`, + `OpenAI diarization failed for chunk ${index + 1} after ${maxRetries} attempts:`, error, ); failedChunks++; diff --git a/src/services/diarization/types.ts b/src/services/diarization/types.ts index c81a7edd..535febee 100644 --- a/src/services/diarization/types.ts +++ b/src/services/diarization/types.ts @@ -7,9 +7,10 @@ * * - `openai`: the `gpt-4o-transcribe-diarize` model on the same * `/v1/audio/transcriptions` endpoint the plugin already uses, reusing the - * user's existing OpenAI key. Convenient, but its 25 MB/request limit means a - * long episode is split into chunks whose speaker labels are not guaranteed to - * line up across chunk boundaries (the labels are assigned per request). + * user's existing OpenAI key. Convenient, but the ~20 MB chunk limit (a + * conservative margin under OpenAI's 25 MB request cap) means a long episode is + * split into chunks whose speaker labels are not guaranteed to line up across + * chunk boundaries (the labels are assigned per request). * - `deepgram`: a single whole-file request to Deepgram's pre-recorded API, * which keeps speaker identity consistent across the entire episode but needs * its own API key. diff --git a/src/settingsMigrations.test.ts b/src/settingsMigrations.test.ts index 3f4b1c5f..96dab84e 100644 --- a/src/settingsMigrations.test.ts +++ b/src/settingsMigrations.test.ts @@ -3,7 +3,9 @@ import { DEFAULT_SETTINGS } from "./constants"; import { LEGACY_EMPTY_DOWNLOAD_PATH, migrateDownloadPath, + migrateFeedNoteSettings, migrateNoteSettings, + migrateSkipLength, migrateTranscriptSettings, } from "./settingsMigrations"; @@ -190,3 +192,57 @@ describe("migrateTranscriptSettings (#168)", () => { expect(migrateTranscriptSettings(once)).toEqual(DEFAULT_SETTINGS.transcript); }); }); + +describe("migrateFeedNoteSettings (ST-08)", () => { + const DEFAULT_FEED_NOTE = { + path: DEFAULT_SETTINGS.feedNote.path, + template: DEFAULT_SETTINGS.feedNote.template, + }; + + it("backfills a missing template on a partial feedNote", () => { + expect(migrateFeedNoteSettings({ path: "Podcasts/{{podcast}}.md" })).toEqual( + { + path: "Podcasts/{{podcast}}.md", + template: DEFAULT_SETTINGS.feedNote.template, + }, + ); + }); + + it("treats an absent/empty feedNote as all defaults", () => { + expect(migrateFeedNoteSettings(undefined)).toEqual(DEFAULT_FEED_NOTE); + expect(migrateFeedNoteSettings(null)).toEqual(DEFAULT_FEED_NOTE); + expect(migrateFeedNoteSettings({})).toEqual(DEFAULT_FEED_NOTE); + }); + + it("coalesces null/non-string fields so they never reach template.replace()", () => { + expect( + migrateFeedNoteSettings({ path: null, template: null }), + ).toEqual(DEFAULT_FEED_NOTE); + }); + + it("preserves a fully-configured feedNote verbatim", () => { + const custom = { path: "Feeds/{{podcast}}.md", template: "# {{title}}" }; + expect(migrateFeedNoteSettings(custom)).toEqual(custom); + }); +}); + +describe("migrateSkipLength (PB-02)", () => { + it("preserves a valid positive length", () => { + expect(migrateSkipLength(30, 15)).toBe(30); + expect(migrateSkipLength("45", 15)).toBe(45); + }); + + it("falls back to the default for NaN/null/zero/negative", () => { + // A cleared field serializes NaN -> null in data.json. + expect(migrateSkipLength(Number.NaN, 15)).toBe(15); + expect(migrateSkipLength(null, 15)).toBe(15); + expect(migrateSkipLength(undefined, 15)).toBe(15); + expect(migrateSkipLength(0, 15)).toBe(15); + expect(migrateSkipLength(-5, 15)).toBe(15); + expect(migrateSkipLength("abc", 15)).toBe(15); + }); + + it("floors fractional values", () => { + expect(migrateSkipLength(12.9, 15)).toBe(12); + }); +}); diff --git a/src/settingsMigrations.ts b/src/settingsMigrations.ts index 586fc1fe..6f6aa34e 100644 --- a/src/settingsMigrations.ts +++ b/src/settingsMigrations.ts @@ -154,3 +154,39 @@ function sanitizeDiarizationProvider(value: unknown): DiarizationProviderId { ? (value as DiarizationProviderId) : DEFAULT_SETTINGS.transcript.diarization.provider; } + +type StoredFeedNote = { path?: string | null; template?: string | null }; + +/** + * Backfill the feed-note settings so a partially-persisted `feedNote` is repaired + * on load (issue ST-08). `loadSettings` shallow-merges the persisted object over + * the default, so an old `data.json` holding only `{ path }` (or a hand-edited one + * with a `null` field) would leave `template` undefined and crash where + * createFeedNote calls `template.replace(...)`. This coalesces any missing/null/ + * non-string field back to the default while preserving a deliberately empty + * string (mirrors migrateTranscriptSettings). Pure, so it is unit-testable. + */ +export function migrateFeedNoteSettings( + stored: StoredFeedNote | null | undefined, +): IPodNotesSettings["feedNote"] { + const s = stored ?? {}; + return { + path: + typeof s.path === "string" ? s.path : DEFAULT_SETTINGS.feedNote.path, + template: + typeof s.template === "string" + ? s.template + : DEFAULT_SETTINGS.feedNote.template, + }; +} + +/** + * Repair a persisted skip length. A cleared settings field used to store NaN + * (Number.parseInt("")), which JSON serializes to null; either value would feed + * the skip arithmetic and corrupt the playback position. Coalesce any + * non-finite/non-positive value back to the default (issue PB-02). Pure. + */ +export function migrateSkipLength(value: unknown, fallback: number): number { + const n = typeof value === "number" ? value : Number(value); + return Number.isFinite(n) && n > 0 ? Math.floor(n) : fallback; +} diff --git a/src/settingsTransfer.test.ts b/src/settingsTransfer.test.ts index 551da22e..2a9dcf60 100644 --- a/src/settingsTransfer.test.ts +++ b/src/settingsTransfer.test.ts @@ -253,6 +253,92 @@ describe("parseImport", () => { expect(withoutKey.ok).toBe(true); if (withoutKey.ok) expect(withoutKey.meta.includesSecret).toBe(false); }); + + it("drops an empty/whitespace secret so it cannot clobber a saved key (IE-05)", () => { + const result = parseImport( + JSON.stringify({ + defaultVolume: 0.5, + openAIApiKey: "", + diarizationApiKey: " ", + }), + ); + + expect(result.ok).toBe(true); + if (result.ok) { + // An empty secret is treated as absent: not carried into the merge, and + // not counted as "includes a secret". + expect(result.settings).not.toHaveProperty("openAIApiKey"); + expect(result.settings).not.toHaveProperty("diarizationApiKey"); + expect(result.meta.includesSecret).toBe(false); + } + }); + + it("keeps a non-empty imported secret (IE-05)", () => { + const result = parseImport(JSON.stringify({ openAIApiKey: "sk-real" })); + expect(result.ok).toBe(true); + if (result.ok) { + expect(result.settings.openAIApiKey).toBe("sk-real"); + expect(result.meta.includesSecret).toBe(true); + } + }); + + it("drops a built-in playlist whose episodes is not an array (PL-10)", () => { + const result = parseImport( + JSON.stringify({ + defaultVolume: 0.5, + favorites: { name: "Favorites", icon: "star", episodes: "boom" }, + localFiles: { name: "Local Files", icon: "folder" }, + }), + ); + + expect(result.ok).toBe(true); + if (result.ok) { + // Malformed built-ins are dropped so the merge falls back to the default + // (which carries episodes: []), instead of feeding the UI a bad shape. + expect(result.settings).not.toHaveProperty("favorites"); + expect(result.settings).not.toHaveProperty("localFiles"); + } + }); + + it("keeps a well-formed built-in playlist (PL-10)", () => { + const favorites = { + name: "Favorites", + icon: "star", + shouldEpisodeRemoveAfterPlay: false, + shouldRepeat: false, + episodes: [], + }; + const result = parseImport(JSON.stringify({ favorites })); + + expect(result.ok).toBe(true); + if (result.ok) expect(result.settings.favorites).toEqual(favorites); + }); + + it("drops only the malformed entries from the playlists map (PL-10)", () => { + const good = { + name: "Good", + icon: "list", + shouldEpisodeRemoveAfterPlay: false, + shouldRepeat: false, + episodes: [], + }; + const result = parseImport( + JSON.stringify({ + playlists: { + Good: good, + NoEpisodes: { name: "NoEpisodes", icon: "list" }, + BadEpisodes: { name: "BadEpisodes", episodes: 42 }, + NotAnObject: "nope", + }, + }), + ); + + expect(result.ok).toBe(true); + if (result.ok) { + expect(Object.keys(result.settings.playlists ?? {})).toEqual(["Good"]); + expect(result.settings.playlists?.Good).toEqual(good); + } + }); }); describe("mergeImportedSettings", () => { @@ -295,6 +381,20 @@ describe("mergeImportedSettings", () => { expect(Object.keys(merged.savedFeeds)).toEqual(["A"]); }); + it("preserves a saved secret when the import omits it (IE-05)", () => { + const current = makeSettings({ + openAIApiKey: "sk-saved", + diarizationApiKey: "dg-saved", + }); + + // parseImport strips empty secrets, so a raw data.json with blank keys + // arrives here without them; the merge must keep the configured keys. + const merged = mergeImportedSettings(current, { defaultVolume: 0.5 }); + + expect(merged.openAIApiKey).toBe("sk-saved"); + expect(merged.diarizationApiKey).toBe("dg-saved"); + }); + it("keeps the current nested value when the import omits the field", () => { const current = makeSettings({ note: { path: "custom/{{title}}.md", template: "custom" }, diff --git a/src/settingsTransfer.ts b/src/settingsTransfer.ts index 22b91209..f11e8171 100644 --- a/src/settingsTransfer.ts +++ b/src/settingsTransfer.ts @@ -257,11 +257,34 @@ function sanitizeImportedSettings( // creation. Drop anything whose type does not match the default. if (!typeMatchesDefault(defaultValue, value)) continue; + // An empty/whitespace secret means "no key configured", so importing one + // must not clobber a key the user already has. Skipping it here keeps it + // absent from `imported`, so the merge `{ ...current, ...imported }` + // preserves the configured key and meta.includesSecret stays honest. + if ( + SECRET_KEYS.has(key as keyof IPodNotesSettings) && + (value as string).trim() === "" + ) { + continue; + } + if ((NESTED_KEYS as readonly string[]).includes(key)) { out[key] = sanitizeNestedObject( defaultValue as Record, value as Record, ); + } else if (key === "favorites" || key === "localFiles") { + // Built-in playlist objects must carry an `episodes` array; consumers + // (PlaylistCard, context menu, removeEpisodeFromPlaylists) iterate it + // without guarding. Drop the key when it's malformed so the merge falls + // back to the default rather than crashing the UI. + if (Array.isArray((value as { episodes?: unknown }).episodes)) { + out[key] = value; + } + } else if (key === "playlists") { + // Map of name -> Playlist; keep only entries whose episodes is an array + // so one malformed entry can't take down the whole import (or the grid). + out[key] = sanitizePlaylistMap(value as Record); } else { out[key] = value; } @@ -270,6 +293,22 @@ function sanitizeImportedSettings( return out as Partial; } +/** Drop any playlist entry that isn't a plain object with an `episodes` array. */ +function sanitizePlaylistMap( + value: Record, +): Record { + const out: Record = {}; + + for (const [name, playlist] of Object.entries(value)) { + if (DANGEROUS_KEYS.has(name)) continue; + if (!isPlainObject(playlist)) continue; + if (!Array.isArray(playlist.episodes)) continue; + out[name] = playlist; + } + + return out; +} + /** Keep only nested fields whose type matches the default; merge backfills the rest. */ function sanitizeNestedObject( defaults: Record, diff --git a/src/store/downloads.test.ts b/src/store/downloads.test.ts new file mode 100644 index 00000000..079b3b1c --- /dev/null +++ b/src/store/downloads.test.ts @@ -0,0 +1,61 @@ +import { get } from "svelte/store"; +import { beforeEach, describe, expect, it } from "vitest"; + +import type { Episode } from "src/types/Episode"; +import { downloadedEpisodes } from "./downloads"; + +function makeEpisode(overrides: Partial = {}): Episode { + return { + title: "Recording", + streamUrl: "", + url: "Folder A/Recording.mp3", + description: "", + content: "", + podcastName: "local file", + episodeDate: undefined, + artworkUrl: "", + ...overrides, + } as unknown as Episode; +} + +describe("downloadedEpisodes.addEpisode basename collision (#LF-06)", () => { + beforeEach(() => { + downloadedEpisodes.set({}); + }); + + it("returns the replaced path when an existing same-key entry has a different filePath", () => { + expect( + downloadedEpisodes.addEpisode(makeEpisode(), "Folder A/Recording.mp3", 10), + ).toBeUndefined(); + + // Same podcastName + title, DIFFERENT path -> basename collision. The store + // stays pure (no Notice); it signals the collision to the caller, which warns. + const replaced = downloadedEpisodes.addEpisode( + makeEpisode(), + "Folder B/Recording.mp3", + 20, + ); + expect(replaced).toBe("Folder A/Recording.mp3"); + + // Behavior is otherwise unchanged: the single entry is replaced in place, + // and the canonical podcastName+title key is preserved (not made unique). + const entries = get(downloadedEpisodes)["local file"]; + expect(entries).toHaveLength(1); + expect(entries?.[0]?.filePath).toBe("Folder B/Recording.mp3"); + }); + + it("returns undefined when re-adding the same episode at the same path", () => { + downloadedEpisodes.addEpisode(makeEpisode(), "Folder A/Recording.mp3", 10); + expect( + downloadedEpisodes.addEpisode(makeEpisode(), "Folder A/Recording.mp3", 11), + ).toBeUndefined(); + + expect(get(downloadedEpisodes)["local file"]).toHaveLength(1); + }); + + it("returns undefined for a brand-new episode", () => { + expect( + downloadedEpisodes.addEpisode(makeEpisode(), "Folder A/Recording.mp3", 10), + ).toBeUndefined(); + }); +}); diff --git a/src/store/downloads.ts b/src/store/downloads.ts index 4a682a3f..2bea6fd7 100644 --- a/src/store/downloads.ts +++ b/src/store/downloads.ts @@ -22,7 +22,19 @@ export const downloadedEpisodes = (() => { set, update, isEpisodeDownloaded, - addEpisode: (episode: Episode, filePath: string, size: number) => { + /** + * Adds or replaces a downloaded/local episode. Returns the file path that + * was REPLACED when a different file collapsed onto the same key (a basename + * collision), or `undefined` otherwise. The caller surfaces a Notice — this + * store stays pure (no UI/side effects), per the PR #211/#212 layering (#LF-06). + */ + addEpisode: ( + episode: Episode, + filePath: string, + size: number, + ): string | undefined => { + let replacedFilePath: string | undefined; + update( (downloadedEpisodes: { [podcastName: string]: DownloadedEpisode[]; @@ -33,6 +45,16 @@ export const downloadedEpisodes = (() => { (ep) => ep.title === episode.title, ); if (idx !== -1) { + // Entries are keyed by podcastName+title, so two distinct local + // files that share a basename in different folders collapse onto + // the same key and the second replaces the first. The key must stay + // basename-only (URIHandler/{{episodelink}} resolve local files by + // basename), so report the collision to the caller instead of + // overwriting silently (#LF-06). + const existingFilePath = podcastEpisodes[idx].filePath; + if (existingFilePath && existingFilePath !== filePath) { + replacedFilePath = existingFilePath; + } podcastEpisodes[idx] = { ...episode, filePath, size }; } else { podcastEpisodes.push({ @@ -46,6 +68,8 @@ export const downloadedEpisodes = (() => { return downloadedEpisodes; }, ); + + return replacedFilePath; }, /** * Drops an episode from the offline set and returns the vault path of the diff --git a/src/store/feeds.ts b/src/store/feeds.ts index 4463c214..a6a3a7b8 100644 --- a/src/store/feeds.ts +++ b/src/store/feeds.ts @@ -46,7 +46,11 @@ type FeedEpisodeSources = Map; function getEpisodeTimestamp(episode?: Episode): number { if (!episode?.episodeDate) return 0; - return Number(episode.episodeDate); + // An Invalid Date coerces to NaN, which makes every comparison false and + // produces an unstable/incorrect sort order. Collapse it to 0 (sorts as + // oldest), matching FeedCacheService.episodeTimestamp (FP-12). + const timestamp = Number(episode.episodeDate); + return Number.isFinite(timestamp) ? timestamp : 0; } function getLatestEpisodesForFeed( diff --git a/src/store/index.ts b/src/store/index.ts index c0916d37..287a8011 100644 --- a/src/store/index.ts +++ b/src/store/index.ts @@ -408,13 +408,17 @@ export const localFiles = (() => { getLocalEpisode: (title: string): LocalEpisode | undefined => { const { episodes } = get(store); - // Prefer a genuine manual local file so a same-titled downloaded episode - // (now mirrored into this playlist) can't shadow it in deep-links. + // Match case- and whitespace-insensitively so a deep link resolves the + // same way the normalized feed path does (LF-08). Still prefer a genuine + // manual local file so a same-titled downloaded episode (now mirrored + // into this playlist) can't shadow it. + const target = title.trim().toLowerCase(); + const matches = (episode: Episode) => + episode.title.trim().toLowerCase() === target; const ep = episodes.find( - (episode) => - episode.title === title && episode.podcastName === "local file", - ) ?? episodes.find((episode) => episode.title === title); + (episode) => matches(episode) && episode.podcastName === "local file", + ) ?? episodes.find(matches); return ep as LocalEpisode | undefined; }, diff --git a/src/ui/settings/PodNotesSettingsTab.ts b/src/ui/settings/PodNotesSettingsTab.ts index bcaf3a87..43ee70bf 100644 --- a/src/ui/settings/PodNotesSettingsTab.ts +++ b/src/ui/settings/PodNotesSettingsTab.ts @@ -242,7 +242,11 @@ export class PodNotesSettingsTab extends PluginSettingTab { textComponent .setValue(`${this.plugin.settings.skipBackwardLength}`) .onChange((value) => { - this.plugin.settings.skipBackwardLength = Number.parseInt(value); + // Ignore empty/invalid input instead of persisting NaN, which + // would corrupt the playback position when skipping (PB-02/ST-01). + const parsed = Number.parseInt(value, 10); + if (!Number.isFinite(parsed) || parsed <= 0) return; + this.plugin.settings.skipBackwardLength = parsed; this.plugin.saveSettings(); }) .setPlaceholder("seconds"); @@ -255,7 +259,9 @@ export class PodNotesSettingsTab extends PluginSettingTab { textComponent .setValue(`${this.plugin.settings.skipForwardLength}`) .onChange((value) => { - this.plugin.settings.skipForwardLength = Number.parseInt(value); + const parsed = Number.parseInt(value, 10); + if (!Number.isFinite(parsed) || parsed <= 0) return; + this.plugin.settings.skipForwardLength = parsed; this.plugin.saveSettings(); }) .setPlaceholder("seconds"); @@ -286,7 +292,15 @@ export class PodNotesSettingsTab extends PluginSettingTab { const timestampFormatDemoEl = container.createDiv(); const updateTimestampDemo = (value: string) => { - if (!this.plugin.api.podcast) return; + // Without a loaded episode there is no time to render; show a hint + // instead of leaving the preview blank so the control isn't mistaken + // for broken (TS-04). + if (!this.plugin.api.podcast) { + timestampFormatDemoEl.setText( + "Play an episode to preview the timestamp format.", + ); + return; + } const demoVal = TimestampTemplateEngine(value); renderMarkdownPreview(demoVal, timestampFormatDemoEl); @@ -734,11 +748,23 @@ export class PodNotesSettingsTab extends PluginSettingTab { return; } + // Warn about a collection only when the user CURRENTLY has data there AND + // the import carries that collection (so it will be replaced wholesale — + // including by an explicitly empty collection, which silently wiped feeds + // with no warning before). Keying off the imported data's contents alone + // missed exactly that data-loss case (SA-09). + const willReplace = ( + key: keyof IPodNotesSettings, + current: Record | undefined, + ): boolean => + Object.prototype.hasOwnProperty.call(result.settings, key) && + Object.keys(current ?? {}).length > 0; + const sections: string[] = []; - if (Object.keys(result.settings.savedFeeds ?? {}).length) { + if (willReplace("savedFeeds", this.plugin.settings.savedFeeds)) { sections.push("podcast feeds"); } - if (Object.keys(result.settings.playlists ?? {}).length) { + if (willReplace("playlists", this.plugin.settings.playlists)) { sections.push("playlists"); } sections.push(...describeSecrets(result.settings)); diff --git a/src/utility/buildEpisodeResumeLink.ts b/src/utility/buildEpisodeResumeLink.ts index 3139a4e9..add11840 100644 --- a/src/utility/buildEpisodeResumeLink.ts +++ b/src/utility/buildEpisodeResumeLink.ts @@ -23,7 +23,9 @@ export default function buildEpisodeResumeLink(episode: Episode): string { ? episode.filePath ?? downloadedPath() : episode.feedUrl ?? downloadedPath(); - if (!target) return ""; + // URIHandler rejects an empty episodeName, so without a title there is no + // working link to emit — degrade to "" rather than a dead link (UR-03). + if (!target || !episode.title) return ""; return encodePodnotesURI(episode.title, target).href; } diff --git a/src/utility/formatSeconds.test.ts b/src/utility/formatSeconds.test.ts index 3f15503f..1d6c0acf 100644 --- a/src/utility/formatSeconds.test.ts +++ b/src/utility/formatSeconds.test.ts @@ -24,4 +24,19 @@ describe("formatSeconds", () => { "00:00:00", ); }); + + test("substitutes each token once, never re-matching inserted digits (TS-09)", () => { + // 12h, 34m, 56s -> with 12-hour token h=12; a single pass must not let the + // '1'/'2' it inserts get reconsidered by a later token. + expect(formatSeconds(45296, "h:mm:ss A")).toBe("12:34:56 PM"); + // h -> "1", then \\h -> literal "h": proves the inserted "1" isn't re-scanned. + expect(formatSeconds(3661, "h\\h")).toBe("1h"); + }); + + test("honors backslash escaping for literal letters (TS-09)", () => { + // Without escaping, the old chained replaces corrupted any literal letter + // that collided with a token. `\\h` must render a literal 'h'. + expect(formatSeconds(3661, "mm\\m ss\\s")).toBe("01m 01s"); + expect(formatSeconds(3661, "\\H\\o\\u\\r")).toBe("Hour"); + }); }); diff --git a/src/utility/formatSeconds.ts b/src/utility/formatSeconds.ts index 9e7a2856..5e5d3ed1 100644 --- a/src/utility/formatSeconds.ts +++ b/src/utility/formatSeconds.ts @@ -6,6 +6,12 @@ * - m, mm: minutes (0-59, 00-59) * - s, ss: seconds (0-59, 00-59) * - A: AM/PM, a: am/pm + * + * Tokens are substituted in a SINGLE pass so a value inserted for one token can + * never be re-matched by a later token (the old chained `.replace()` calls + * corrupted any format containing literal letters that collide with tokens, e.g. + * "Hh" or words). Literal letters are escaped with a backslash, matching Moment's + * own escaping (e.g. `\\h` renders a literal "h"). See issue #?? (TS-09). */ export function formatSeconds(totalSeconds: number, format: string): string { // Clamp non-finite (NaN/Infinity) and negative inputs to 0 so the player never @@ -26,15 +32,25 @@ export function formatSeconds(totalSeconds: number, format: string): string { const pad = (n: number): string => n.toString().padStart(2, '0'); - return format - .replace(/HH/g, pad(hours)) - .replace(/H/g, hours.toString()) - .replace(/hh/g, pad(hours12)) - .replace(/h/g, hours12.toString()) - .replace(/mm/g, pad(minutes)) - .replace(/m/g, minutes.toString()) - .replace(/ss/g, pad(secs)) - .replace(/s/g, secs.toString()) - .replace(/A/g, isPM ? 'PM' : 'AM') - .replace(/a/g, isPM ? 'pm' : 'am'); + // Longest tokens first so the alternation prefers HH over H, etc. + const tokens: Record = { + HH: pad(hours), + hh: pad(hours12), + mm: pad(minutes), + ss: pad(secs), + H: hours.toString(), + h: hours12.toString(), + m: minutes.toString(), + s: secs.toString(), + A: isPM ? 'PM' : 'AM', + a: isPM ? 'pm' : 'am', + }; + + // `\\(.)` consumes an escaped literal first (emitting the escaped char as-is), + // otherwise the longest matching token is replaced. Everything else (`:`, `-`, + // spaces, unescaped non-token letters) passes through untouched. + return format.replace( + /\\(.)|HH|hh|mm|ss|H|h|m|s|A|a/g, + (match, escaped) => (escaped !== undefined ? escaped : tokens[match]), + ); } \ No newline at end of file diff --git a/src/utility/searchEpisodes.ts b/src/utility/searchEpisodes.ts index ba10a127..cdfbd99c 100644 --- a/src/utility/searchEpisodes.ts +++ b/src/utility/searchEpisodes.ts @@ -27,7 +27,10 @@ function getFuse(episodes: Episode[]): Fuse { export default function searchEpisodes(query: string, episodes: Episode[]): Episode[] { if (episodes.length === 0) return []; - if (query.length === 0) { + // Trim before the empty check so a whitespace-only query restores the full + // list rather than running a fuzzy search for spaces. This covers every call + // site (feed/playlist/latest/played searches) at once (PV-08). + if (query.trim().length === 0) { return episodes; }