From 778d88ff093752b07973ca10fecd8b76671832db Mon Sep 17 00:00:00 2001 From: Christian Bager Bach Houmann Date: Sun, 21 Jun 2026 20:03:41 +0200 Subject: [PATCH 1/5] refactor(store): keep vault file I/O out of the downloads store downloadedEpisodes.removeEpisode deleted the backing vault file via a global `app` reference behind the only production `@ts-ignore`, mixing file I/O into the pure state core. Make removeEpisode return the removed file's path and move the deletion to deleteEpisodeFile() in the download module (symmetric with createEpisodeFile, where the vault/app access already lives). Same separation #211 applied to the queue automation. The sole caller (spawnEpisodeContextMenu) deletes the returned path. deleteEpisodeFile now awaits the delete inside its own try/catch, so an async deletion failure is caught instead of becoming an unhandled rejection. Adds a unit test now that the store op is pure. --- src/downloadEpisode.ts | 20 ++++++++++++ src/store/downloads.ts | 31 ++++++++----------- src/store/index.test.ts | 30 ++++++++++++++++++ src/ui/PodcastView/spawnEpisodeContextMenu.ts | 9 ++++-- 4 files changed, 70 insertions(+), 20 deletions(-) diff --git a/src/downloadEpisode.ts b/src/downloadEpisode.ts index 15646b87..98591f93 100644 --- a/src/downloadEpisode.ts +++ b/src/downloadEpisode.ts @@ -254,6 +254,26 @@ async function createEpisodeFile({ downloadedEpisodes.addEpisode(episode, filePath, data.byteLength); } +/** + * Delete a downloaded episode's backing file from the vault. The download store + * owns the offline-set state and hands the path here for the actual file I/O, + * keeping vault side effects out of the store layer. Best-effort: a missing or + * already-removed file is a no-op, and failures are logged rather than thrown so + * removing a stale entry never breaks the calling UI flow. + */ +export async function deleteEpisodeFile(filePath: string): Promise { + if (!filePath) return; + + try { + const file = app.vault.getAbstractFileByPath(filePath); + if (file instanceof TFile) { + await app.vault.delete(file); + } + } catch (error) { + console.error(`Failed to delete downloaded file "${filePath}":`, error); + } +} + function resolveLocalEpisodeFilePath(episode: LocalEpisode): string | null { const downloadedEpisode = downloadedEpisodes.getEpisode(episode); const candidatePaths = [ diff --git a/src/store/downloads.ts b/src/store/downloads.ts index 14dbc8f4..d98f6eb5 100644 --- a/src/store/downloads.ts +++ b/src/store/downloads.ts @@ -1,5 +1,4 @@ import { get, writable } from "svelte/store"; -import { TFile } from "obsidian"; import type { Episode } from "src/types/Episode"; import type DownloadedEpisode from "src/types/DownloadedEpisode"; @@ -48,7 +47,16 @@ export const downloadedEpisodes = (() => { }, ); }, - removeEpisode: (episode: Episode, removeFile: boolean) => { + /** + * Drops an episode from the offline set and returns the vault path of the + * file it backed (or `undefined` if it wasn't tracked). This store is the + * pure state core, so it never touches the vault itself — the caller deletes + * the returned file via `deleteEpisodeFile` in the download module. Mirrors + * how #211 split the queue's automation side effect out of persistence. + */ + removeEpisode: (episode: Episode): string | undefined => { + let removedFilePath: string | undefined; + update((downloadedEpisodes) => { const podcastEpisodes = downloadedEpisodes[episode.podcastName] || []; const index = podcastEpisodes.findIndex( @@ -60,27 +68,14 @@ export const downloadedEpisodes = (() => { return downloadedEpisodes; } - const filePath = podcastEpisodes[index].filePath; + removedFilePath = podcastEpisodes[index].filePath; podcastEpisodes.splice(index, 1); - if (removeFile && filePath) { - try { - // @ts-ignore: app is not defined in the global scope anymore, but is still - // available. Need to fix this later - const file = app.vault.getAbstractFileByPath(filePath); - - if (file instanceof TFile) { - // @ts-ignore - app.vault.delete(file); - } - } catch (error) { - console.error(error); - } - } - downloadedEpisodes[episode.podcastName] = podcastEpisodes; return downloadedEpisodes; }); + + return removedFilePath; }, getEpisode: (episode: Episode) => { return get(store)[episode.podcastName]?.find( diff --git a/src/store/index.test.ts b/src/store/index.test.ts index 615b4dad..45e1c7b3 100644 --- a/src/store/index.test.ts +++ b/src/store/index.test.ts @@ -323,6 +323,36 @@ describe("localFiles store — syncWithDownloaded (issue #176)", () => { }); }); +describe("downloadedEpisodes store — removeEpisode is pure (no vault I/O)", () => { + beforeEach(() => { + downloadedEpisodes.set({}); + }); + + test("removes the entry and returns its backing file path", () => { + const downloaded = downloadedEpisode("Design Podcast", "Episode 1", { + filePath: "podcasts/design/ep1.mp3", + }); + downloadedEpisodes.addEpisode(downloaded, downloaded.filePath, 2048); + + const removedFilePath = downloadedEpisodes.removeEpisode(downloaded); + + expect(removedFilePath).toBe("podcasts/design/ep1.mp3"); + expect(downloadedEpisodes.isEpisodeDownloaded(downloaded)).toBe(false); + expect(get(downloadedEpisodes)["Design Podcast"]).toEqual([]); + }); + + test("returns undefined and leaves the store untouched when the episode is absent", () => { + const present = downloadedEpisode("Design Podcast", "Kept"); + downloadedEpisodes.addEpisode(present, present.filePath, 1024); + + const missing = downloadedEpisode("Design Podcast", "Never downloaded"); + const removedFilePath = downloadedEpisodes.removeEpisode(missing); + + expect(removedFilePath).toBeUndefined(); + expect(downloadedEpisodes.isEpisodeDownloaded(present)).toBe(true); + }); +}); + function ep(title: string, podcastName = "Pod"): Episode { return { title, diff --git a/src/ui/PodcastView/spawnEpisodeContextMenu.ts b/src/ui/PodcastView/spawnEpisodeContextMenu.ts index 1109d8b3..9bf1d86f 100644 --- a/src/ui/PodcastView/spawnEpisodeContextMenu.ts +++ b/src/ui/PodcastView/spawnEpisodeContextMenu.ts @@ -1,7 +1,9 @@ import { Menu, Notice } from "obsidian"; import createPodcastNote, { getPodcastNote, openPodcastNote } from "src/createPodcastNote"; import createFeedNote, { getFeedNote, openFeedNote } from "src/createFeedNote"; -import downloadEpisodeWithProgessNotice from "src/downloadEpisode"; +import downloadEpisodeWithProgessNotice, { + deleteEpisodeFile, +} from "src/downloadEpisode"; import { currentEpisode, downloadedEpisodes, favorites, playedEpisodes, playlists, plugin, queue, savedFeeds, viewState } from "src/store"; import type { Episode } from "src/types/Episode"; import type { PodcastFeed } from "src/types/PodcastFeed"; @@ -68,7 +70,10 @@ export default function spawnEpisodeContextMenu( .setTitle(isDownloaded ? "Remove file" : "Download") .onClick(() => { if (isDownloaded) { - downloadedEpisodes.removeEpisode(episode, true); + const removedFilePath = downloadedEpisodes.removeEpisode(episode); + if (removedFilePath) { + void deleteEpisodeFile(removedFilePath); + } } else { // The path template always yields a per-episode file via // safeDownloadBasename (#183), so no empty-path guard is needed — From 5bf70d52a6b8bcd008a30142a60bb9d4a0024a2a Mon Sep 17 00:00:00 2001 From: Christian Bager Bach Houmann Date: Sun, 21 Jun 2026 20:06:20 +0200 Subject: [PATCH 2/5] refactor(templates): dedupe the shared file-name tag registration MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit FilePathTemplateEngine, DownloadPathTemplateEngine, and the first five tags of TranscriptTemplateEngine registered a byte-identical block of {{title}}/{{podcast}}/{{date}}/{{currentdate}}/{{episodenumber}} tags. Extract addEpisodeFileNameTags(addTag, episode) plus a legalizedNameTag helper; FeedFilePathTemplateEngine reuses the latter for its own identical name closure. NoteTemplateEngine is intentionally untouched — its {{title}} is the raw episode title, not a file name. Behavior-preserving: -50 net LOC, all 50 TemplateEngine tests pass. --- src/TemplateEngine.ts | 132 +++++++++++++----------------------------- 1 file changed, 41 insertions(+), 91 deletions(-) diff --git a/src/TemplateEngine.ts b/src/TemplateEngine.ts index 27a3341d..fa94c92c 100644 --- a/src/TemplateEngine.ts +++ b/src/TemplateEngine.ts @@ -108,6 +108,43 @@ function resolveEpisodeNumber(episode: Episode): number | undefined { return episode.episodeNumber ?? parseEpisodeNumberFromTitle(episode.title); } +/** + * Build a tag that strips file-name-illegal characters from `rawValue` and, when + * the tag is used with an argument (e.g. `{{title:_}}`), collapses whitespace to + * that replacement. Shared by the file-name {{title}}/{{podcast}} tags. + */ +function legalizedNameTag(rawValue: string): TagValue { + return (whitespaceReplacement?: string) => { + const legal = replaceIllegalFileNameCharactersInString(rawValue); + return whitespaceReplacement + ? legal.replace(/\s+/g, whitespaceReplacement) + : legal; + }; +} + +/** + * Register the file-name-safe episode tags shared by every path/transcript + * template engine: {{title}} and {{podcast}} (illegal-character-stripped, with an + * optional whitespace-replacement arg), {{date}} (episode publish date), + * {{currentdate}}, and {{episodenumber}}. NoteTemplateEngine intentionally does + * NOT use this — there {{title}} is the raw episode title, not a file name. + */ +function addEpisodeFileNameTags(addTag: AddTagFn, episode: Episode): void { + addTag("title", legalizedNameTag(episode.title)); + addTag("podcast", legalizedNameTag(episode.podcastName)); + addTag("date", (format?: string) => + episode.episodeDate + ? formatDate(episode.episodeDate, format ?? "YYYY-MM-DD") + : "", + ); + addTag("currentdate", (format?: string) => + formatDate(new Date(), format ?? "YYYY-MM-DD"), + ); + addTag("episodenumber", (pad?: string) => + formatEpisodeNumber(resolveEpisodeNumber(episode), pad), + ); +} + function formatChapterTitle(title: string): string { return title.replace(/\s+/g, " ").trim(); } @@ -280,35 +317,7 @@ export function TimestampTemplateEngine( export function FilePathTemplateEngine(template: string, episode: Episode) { const [replacer, addTag] = useTemplateEngine(); - addTag("title", (whitespaceReplacement?: string) => { - const legalTitle = replaceIllegalFileNameCharactersInString(episode.title); - if (whitespaceReplacement) { - return legalTitle.replace(/\s+/g, whitespaceReplacement); - } - - return legalTitle; - }); - addTag("podcast", (whitespaceReplacement?: string) => { - const legalName = replaceIllegalFileNameCharactersInString( - episode.podcastName, - ); - if (whitespaceReplacement) { - return legalName.replace(/\s+/g, whitespaceReplacement); - } - - return legalName; - }); - addTag("date", (format?: string) => - episode.episodeDate - ? formatDate(episode.episodeDate, format ?? "YYYY-MM-DD") - : "", - ); - addTag("currentdate", (format?: string) => - formatDate(new Date(), format ?? "YYYY-MM-DD"), - ); - addTag("episodenumber", (pad?: string) => - formatEpisodeNumber(resolveEpisodeNumber(episode), pad), - ); + addEpisodeFileNameTags(addTag, episode); return replacer(template); } @@ -322,35 +331,7 @@ export function DownloadPathTemplateEngine(template: string, episode: Episode) { const [replacer, addTag] = useTemplateEngine(); - addTag("title", (whitespaceReplacement?: string) => { - const legalTitle = replaceIllegalFileNameCharactersInString(episode.title); - if (whitespaceReplacement) { - return legalTitle.replace(/\s+/g, whitespaceReplacement); - } - - return legalTitle; - }); - addTag("podcast", (whitespaceReplacement?: string) => { - const legalName = replaceIllegalFileNameCharactersInString( - episode.podcastName, - ); - if (whitespaceReplacement) { - return legalName.replace(/\s+/g, whitespaceReplacement); - } - - return legalName; - }); - addTag("date", (format?: string) => - episode.episodeDate - ? formatDate(episode.episodeDate, format ?? "YYYY-MM-DD") - : "", - ); - addTag("currentdate", (format?: string) => - formatDate(new Date(), format ?? "YYYY-MM-DD"), - ); - addTag("episodenumber", (pad?: string) => - formatEpisodeNumber(resolveEpisodeNumber(episode), pad), - ); + addEpisodeFileNameTags(addTag, episode); return replacer(templateWithoutExtension); } @@ -362,33 +343,7 @@ export function TranscriptTemplateEngine( ) { const [replacer, addTag] = useTemplateEngine(); - addTag("title", (whitespaceReplacement?: string) => { - const legalTitle = replaceIllegalFileNameCharactersInString(episode.title); - if (whitespaceReplacement) { - return legalTitle.replace(/\s+/g, whitespaceReplacement); - } - return legalTitle; - }); - addTag("podcast", (whitespaceReplacement?: string) => { - const legalName = replaceIllegalFileNameCharactersInString( - episode.podcastName, - ); - if (whitespaceReplacement) { - return legalName.replace(/\s+/g, whitespaceReplacement); - } - return legalName; - }); - addTag("date", (format?: string) => - episode.episodeDate - ? formatDate(episode.episodeDate, format ?? "YYYY-MM-DD") - : "", - ); - addTag("currentdate", (format?: string) => - formatDate(new Date(), format ?? "YYYY-MM-DD"), - ); - addTag("episodenumber", (pad?: string) => - formatEpisodeNumber(resolveEpisodeNumber(episode), pad), - ); + addEpisodeFileNameTags(addTag, episode); addTag("duration", (format?: string) => episode.duration !== undefined ? formatDuration(episode.duration, format) @@ -453,12 +408,7 @@ export function FeedNoteTemplateEngine(template: string, feed: PodcastFeed) { export function FeedFilePathTemplateEngine(template: string, feed: PodcastFeed) { const [replacer, addTag] = useTemplateEngine(); - const safeName = replaceIllegalFileNameCharactersInString(feed.title); - const nameTag = (whitespaceReplacement?: string) => - whitespaceReplacement - ? safeName.replace(/\s+/g, whitespaceReplacement) - : safeName; - + const nameTag = legalizedNameTag(feed.title); addTag("title", nameTag); addTag("podcast", nameTag); addTag("date", (format?: string) => From 7fe44a37b5600120d74e8e00e50f54c6e47a1c14 Mon Sep 17 00:00:00 2001 From: Christian Bager Bach Houmann Date: Sun, 21 Jun 2026 20:10:58 +0200 Subject: [PATCH 3/5] refactor(transcripts): extract pure WAV chunking from TranscriptionService MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The service mixed ~230 lines of pure audio work — chunk sizing, m4a->WAV decode/re-encode, and WAV header/PCM byte writing — into a class otherwise concerned with the queue, OpenAI client, diarization routing, and saving. Move it to a standalone src/services/audioChunker.ts of pure functions (createChunkFiles/getMimeType/...), shrinking the class 631->400 LOC. The chunker tests previously kept hand-maintained COPIES of getMimeType, shouldConvertToWav, createBinaryChunkFiles, and writeWavHeader, and reached createChunkFiles via an `as unknown as` cast on a live instance. They now live in audioChunker.test.ts and exercise the real exported functions, so they can no longer silently drift from production. No behavior change. --- src/services/TranscriptionService.test.ts | 233 +-------------------- src/services/TranscriptionService.ts | 239 +--------------------- src/services/audioChunker.test.ts | 139 +++++++++++++ src/services/audioChunker.ts | 234 +++++++++++++++++++++ 4 files changed, 381 insertions(+), 464 deletions(-) create mode 100644 src/services/audioChunker.test.ts create mode 100644 src/services/audioChunker.ts diff --git a/src/services/TranscriptionService.test.ts b/src/services/TranscriptionService.test.ts index 003bccbe..c00d74c0 100644 --- a/src/services/TranscriptionService.test.ts +++ b/src/services/TranscriptionService.test.ts @@ -83,51 +83,10 @@ describe("TranscriptionService", () => { }); }); - describe("getMimeType", () => { - test("returns correct mime types for audio formats", () => { - const getMimeType = (fileExtension: string): string => { - switch (fileExtension.toLowerCase()) { - case "mp3": - return "audio/mp3"; - case "m4a": - return "audio/mp4"; - case "ogg": - return "audio/ogg"; - case "wav": - return "audio/wav"; - case "flac": - return "audio/flac"; - case "webm": - return "audio/webm"; - default: - return "audio/mpeg"; - } - }; - - expect(getMimeType("mp3")).toBe("audio/mp3"); - expect(getMimeType("MP3")).toBe("audio/mp3"); - expect(getMimeType("m4a")).toBe("audio/mp4"); - expect(getMimeType("ogg")).toBe("audio/ogg"); - expect(getMimeType("wav")).toBe("audio/wav"); - expect(getMimeType("flac")).toBe("audio/flac"); - expect(getMimeType("webm")).toBe("audio/webm"); - expect(getMimeType("unknown")).toBe("audio/mpeg"); - }); - }); - - describe("shouldConvertToWav", () => { - test("returns true for m4a files", () => { - const shouldConvertToWav = (extension: string, mimeType: string): boolean => { - const normalizedExtension = extension.toLowerCase(); - return normalizedExtension === "m4a" || mimeType === "audio/mp4"; - }; - - expect(shouldConvertToWav("m4a", "audio/mp4")).toBe(true); - expect(shouldConvertToWav("M4A", "audio/mp4")).toBe(true); - expect(shouldConvertToWav("mp3", "audio/mp4")).toBe(true); - expect(shouldConvertToWav("mp3", "audio/mpeg")).toBe(false); - }); - }); + // NOTE: the audio chunking + WAV encoding tests (getMimeType, shouldConvertToWav, + // createBinaryChunkFiles, writeWavHeader, createChunkFiles) moved to + // audioChunker.test.ts, where they exercise the real exported functions instead + // of inline copies. This file keeps the service-orchestration tests. describe("getEpisodeKey", () => { test("generates unique key from podcast name and title", () => { @@ -139,146 +98,6 @@ describe("TranscriptionService", () => { }); }); - describe("createBinaryChunkFiles", () => { - const CHUNK_SIZE_BYTES = 20 * 1024 * 1024; - - function createBinaryChunkFiles( - buffer: ArrayBuffer, - basename: string, - extension: string, - mimeType: string, - ): File[] { - if (buffer.byteLength <= CHUNK_SIZE_BYTES) { - return [ - new File([buffer], `${basename}.${extension}`, { - type: mimeType, - }), - ]; - } - - const files: File[] = []; - for ( - let offset = 0, index = 0; - offset < buffer.byteLength; - offset += CHUNK_SIZE_BYTES, index++ - ) { - const chunk = buffer.slice(offset, offset + CHUNK_SIZE_BYTES); - files.push( - new File([chunk], `${basename}.part${index}.${extension}`, { - type: mimeType, - }), - ); - } - - return files; - } - - test("returns single file when buffer is smaller than chunk size", () => { - const smallBuffer = new ArrayBuffer(1024); - const files = createBinaryChunkFiles(smallBuffer, "test", "mp3", "audio/mpeg"); - - expect(files).toHaveLength(1); - expect(files[0].name).toBe("test.mp3"); - expect(files[0].type).toBe("audio/mpeg"); - expect(files[0].size).toBe(1024); - }); - - test("returns multiple files when buffer exceeds chunk size", () => { - const largeBuffer = new ArrayBuffer(CHUNK_SIZE_BYTES * 2 + 1024); - const files = createBinaryChunkFiles(largeBuffer, "test", "mp3", "audio/mpeg"); - - expect(files).toHaveLength(3); - expect(files[0].name).toBe("test.part0.mp3"); - expect(files[1].name).toBe("test.part1.mp3"); - expect(files[2].name).toBe("test.part2.mp3"); - }); - }); - - describe("writeWavHeader", () => { - const WAV_HEADER_SIZE = 44; - const PCM_BYTES_PER_SAMPLE = 2; - - function writeString(view: DataView, offset: number, str: string): void { - for (let i = 0; i < str.length; i++) { - view.setUint8(offset + i, str.charCodeAt(i)); - } - } - - function writeWavHeader( - view: DataView, - sampleRate: number, - numChannels: number, - sampleCount: number, - ): void { - const blockAlign = numChannels * PCM_BYTES_PER_SAMPLE; - const byteRate = sampleRate * blockAlign; - const dataSize = sampleCount * blockAlign; - writeString(view, 0, "RIFF"); - view.setUint32(4, 36 + dataSize, true); - writeString(view, 8, "WAVE"); - writeString(view, 12, "fmt "); - view.setUint32(16, 16, true); - view.setUint16(20, 1, true); - view.setUint16(22, numChannels, true); - view.setUint32(24, sampleRate, true); - view.setUint32(28, byteRate, true); - view.setUint16(32, blockAlign, true); - view.setUint16(34, PCM_BYTES_PER_SAMPLE * 8, true); - writeString(view, 36, "data"); - view.setUint32(40, dataSize, true); - } - - test("writes correct RIFF header", () => { - const buffer = new ArrayBuffer(WAV_HEADER_SIZE); - const view = new DataView(buffer); - - writeWavHeader(view, 44100, 2, 44100); - - const riff = String.fromCharCode( - view.getUint8(0), - view.getUint8(1), - view.getUint8(2), - view.getUint8(3), - ); - expect(riff).toBe("RIFF"); - - const wave = String.fromCharCode( - view.getUint8(8), - view.getUint8(9), - view.getUint8(10), - view.getUint8(11), - ); - expect(wave).toBe("WAVE"); - - const fmt = String.fromCharCode( - view.getUint8(12), - view.getUint8(13), - view.getUint8(14), - view.getUint8(15), - ); - expect(fmt).toBe("fmt "); - - const data = String.fromCharCode( - view.getUint8(36), - view.getUint8(37), - view.getUint8(38), - view.getUint8(39), - ); - expect(data).toBe("data"); - }); - - test("writes correct sample rate and channels", () => { - const buffer = new ArrayBuffer(WAV_HEADER_SIZE); - const view = new DataView(buffer); - - writeWavHeader(view, 44100, 2, 1000); - - expect(view.getUint16(22, true)).toBe(2); - expect(view.getUint32(24, true)).toBe(44100); - expect(view.getUint16(34, true)).toBe(16); - }); - }); - describe("TranscriptionService instantiation", () => { test("creates instance with plugin reference", () => { const mockPlugin = createMockPlugin(); @@ -304,48 +123,4 @@ describe("TranscriptionService", () => { }); }); - describe("createChunkFiles small-file fast path (#168 / PR #204 review)", () => { - type ChunkFn = (args: { - buffer: ArrayBuffer; - basename: string; - extension: string; - mimeType: string; - }) => Promise; - - function createChunkFiles(): ChunkFn { - const service = new TranscriptionService(createMockPlugin()); - return (service as unknown as { createChunkFiles: ChunkFn }) - .createChunkFiles.bind(service); - } - - test("sends a small m4a as a single original file instead of WAV-splitting it", async () => { - // A 1 KB m4a is well under the 20 MB chunk size, so it must not be - // converted to WAV (which would balloon it into many chunks and reset - // diarization speaker labels). It should be one intact .m4a file. - const files = await createChunkFiles()({ - buffer: new ArrayBuffer(1024), - basename: "episode", - extension: "m4a", - mimeType: "audio/mp4", - }); - - expect(files).toHaveLength(1); - expect(files[0].name).toBe("episode.m4a"); - expect(files[0].type).toBe("audio/mp4"); - expect(files[0].name).not.toContain(".wav"); - }); - - test("still sends a small mp3 as a single original file (unchanged)", async () => { - const files = await createChunkFiles()({ - buffer: new ArrayBuffer(2048), - basename: "episode", - extension: "mp3", - mimeType: "audio/mpeg", - }); - - expect(files).toHaveLength(1); - expect(files[0].name).toBe("episode.mp3"); - expect(files[0].size).toBe(2048); - }); - }); }); diff --git a/src/services/TranscriptionService.ts b/src/services/TranscriptionService.ts index ef5df2fa..023791db 100644 --- a/src/services/TranscriptionService.ts +++ b/src/services/TranscriptionService.ts @@ -6,6 +6,7 @@ import { TranscriptTemplateEngine } from "../TemplateEngine"; import { ensureFolderExists } from "../utility/ensureFolderExists"; import type { Episode } from "src/types/Episode"; import { getEpisodeTranscriptPath } from "src/utility/getEpisodeTranscriptPath"; +import { createChunkFiles, getMimeType } from "./audioChunker"; import { type DiarizationAudio, type DiarizationProviderId, @@ -61,9 +62,6 @@ export class TranscriptionService { private client: OpenAI | null = null; private cachedApiKey: string | null = null; private MAX_RETRIES = 3; - private readonly CHUNK_SIZE_BYTES = 20 * 1024 * 1024; - private readonly WAV_HEADER_SIZE = 44; - private readonly PCM_BYTES_PER_SAMPLE = 2; private readonly MAX_CONCURRENT_TRANSCRIPTIONS = 2; private readonly MAX_CONCURRENT_CHUNK_TRANSCRIPTIONS = 3; private pendingEpisodes: Episode[] = []; @@ -168,7 +166,7 @@ export class TranscriptionService { extension: fileExtension, basename, } = await getEpisodeAudioBuffer(episode); - const mimeType = this.getMimeType(fileExtension); + const mimeType = getMimeType(fileExtension); const transcriptBody = await this.buildTranscriptBody( { @@ -222,7 +220,7 @@ export class TranscriptionService { } updateNotice("Creating audio chunks..."); - const files = await this.createChunkFiles(audio); + const files = await createChunkFiles(audio); updateNotice("Starting transcription..."); const transcription = await this.transcribeChunks(files, updateNotice); return transcription.replace(/\.\s+/g, ".\n\n"); @@ -252,7 +250,7 @@ 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. updateNotice("Creating audio chunks..."); - const chunkFiles = await this.createChunkFiles(audio); + const chunkFiles = await createChunkFiles(audio); return diarizeWithOpenAI({ getClient: () => this.getClient(), chunkFiles, @@ -261,235 +259,6 @@ export class TranscriptionService { }); } - private async createChunkFiles({ - buffer, - basename, - extension, - mimeType, - }: { - buffer: ArrayBuffer; - basename: string; - extension: string; - mimeType: string; - }): Promise { - // A file that already fits in a single request needs neither conversion nor - // splitting: send the original (compressed) bytes. OpenAI accepts m4a/mp3/etc. - // directly under the upload limit, so this skips the m4a->WAV path below for - // small m4a episodes — that path only exists to SAFELY SPLIT an m4a (which - // can't be byte-split) and would otherwise balloon a small m4a into many - // uncompressed WAV chunks, multiplying requests and, for diarization, - // resetting speaker labels at artificial chunk boundaries (#168 / PR #204 review). - if (buffer.byteLength <= this.CHUNK_SIZE_BYTES) { - return [ - new File([buffer], `${basename}.${extension}`, { type: mimeType }), - ]; - } - - if (this.shouldConvertToWav(extension, mimeType)) { - const wavChunks = await this.convertToWavChunks(buffer, basename); - if (wavChunks.length > 0) { - return wavChunks; - } - } - - return this.createBinaryChunkFiles(buffer, basename, extension, mimeType); - } - - private shouldConvertToWav(extension: string, mimeType: string): boolean { - const normalizedExtension = extension.toLowerCase(); - return normalizedExtension === "m4a" || mimeType === "audio/mp4"; - } - - private createBinaryChunkFiles( - buffer: ArrayBuffer, - basename: string, - extension: string, - mimeType: string, - ): File[] { - if (buffer.byteLength <= this.CHUNK_SIZE_BYTES) { - return [ - new File([buffer], `${basename}.${extension}`, { - type: mimeType, - }), - ]; - } - - const files: File[] = []; - for ( - let offset = 0, index = 0; - offset < buffer.byteLength; - offset += this.CHUNK_SIZE_BYTES, index++ - ) { - const chunk = buffer.slice(offset, offset + this.CHUNK_SIZE_BYTES); - files.push( - new File([chunk], `${basename}.part${index}.${extension}`, { - type: mimeType, - }), - ); - } - - return files; - } - - private async convertToWavChunks( - buffer: ArrayBuffer, - basename: string, - ): Promise { - const audioContext = this.createAudioContext(); - if (!audioContext) return []; - - try { - const audioBuffer = await audioContext.decodeAudioData(buffer.slice(0)); - return this.renderWavChunks(audioBuffer, basename); - } catch (error) { - console.warn("Failed to convert audio buffer for transcription", error); - return []; - } finally { - try { - await audioContext.close(); - } catch (error) { - console.warn("Failed to close audio context", error); - } - } - } - - private createAudioContext(): AudioContext | null { - if (typeof window === "undefined") { - return null; - } - - const contextCtor = - window.AudioContext || - (window as typeof window & { webkitAudioContext?: typeof AudioContext }) - .webkitAudioContext; - if (!contextCtor) { - return null; - } - - return new contextCtor(); - } - - private renderWavChunks(audioBuffer: AudioBuffer, basename: string): File[] { - const numChannels = audioBuffer.numberOfChannels; - const bytesPerFrame = numChannels * this.PCM_BYTES_PER_SAMPLE; - const availableBytesPerChunk = this.CHUNK_SIZE_BYTES - this.WAV_HEADER_SIZE; - const maxSamplesPerChunk = Math.max( - 1, - Math.floor(availableBytesPerChunk / bytesPerFrame), - ); - const channelData = Array.from({ length: numChannels }, (_, channelIndex) => - audioBuffer.getChannelData(channelIndex), - ); - const files: File[] = []; - let chunkIndex = 0; - - for ( - let startSample = 0; - startSample < audioBuffer.length; - startSample += maxSamplesPerChunk - ) { - const endSample = Math.min( - audioBuffer.length, - startSample + maxSamplesPerChunk, - ); - const wavBuffer = this.renderWavBuffer( - channelData, - audioBuffer.sampleRate, - startSample, - endSample, - ); - files.push( - new File([wavBuffer], `${basename}.part${chunkIndex}.wav`, { - type: "audio/wav", - }), - ); - chunkIndex++; - } - - return files; - } - - private renderWavBuffer( - channelData: Float32Array[], - sampleRate: number, - startSample: number, - endSample: number, - ): ArrayBuffer { - const numChannels = channelData.length; - const sampleCount = Math.max(0, endSample - startSample); - const blockAlign = numChannels * this.PCM_BYTES_PER_SAMPLE; - const buffer = new ArrayBuffer( - this.WAV_HEADER_SIZE + sampleCount * blockAlign, - ); - const view = new DataView(buffer); - this.writeWavHeader(view, sampleRate, numChannels, sampleCount); - let offset = this.WAV_HEADER_SIZE; - - for (let i = 0; i < sampleCount; i++) { - for (let channel = 0; channel < numChannels; channel++) { - const sample = channelData[channel][startSample + i] ?? 0; - const clamped = Math.max(-1, Math.min(1, sample)); - const intSample = - clamped < 0 - ? clamped * 0x8000 - : clamped * 0x7fff; - view.setInt16(offset, Math.round(intSample), true); - offset += this.PCM_BYTES_PER_SAMPLE; - } - } - - return buffer; - } - - private writeWavHeader( - view: DataView, - sampleRate: number, - numChannels: number, - sampleCount: number, - ): void { - const blockAlign = numChannels * this.PCM_BYTES_PER_SAMPLE; - const byteRate = sampleRate * blockAlign; - const dataSize = sampleCount * blockAlign; - this.writeString(view, 0, "RIFF"); - view.setUint32(4, 36 + dataSize, true); - this.writeString(view, 8, "WAVE"); - this.writeString(view, 12, "fmt "); - view.setUint32(16, 16, true); - view.setUint16(20, 1, true); - view.setUint16(22, numChannels, true); - view.setUint32(24, sampleRate, true); - view.setUint32(28, byteRate, true); - view.setUint16(32, blockAlign, true); - view.setUint16(34, this.PCM_BYTES_PER_SAMPLE * 8, true); - this.writeString(view, 36, "data"); - view.setUint32(40, dataSize, true); - } - - private writeString(view: DataView, offset: number, str: string): void { - for (let i = 0; i < str.length; i++) { - view.setUint8(offset + i, str.charCodeAt(i)); - } - } - - private getMimeType(fileExtension: string): string { - switch (fileExtension.toLowerCase()) { - case "mp3": - return "audio/mp3"; - case "m4a": - return "audio/mp4"; - case "ogg": - return "audio/ogg"; - case "wav": - return "audio/wav"; - case "flac": - return "audio/flac"; - case "webm": - return "audio/webm"; - default: - return "audio/mpeg"; - } - } - private async transcribeChunks( files: File[], updateNotice: (message: string) => void, diff --git a/src/services/audioChunker.test.ts b/src/services/audioChunker.test.ts new file mode 100644 index 00000000..57e395de --- /dev/null +++ b/src/services/audioChunker.test.ts @@ -0,0 +1,139 @@ +import { describe, expect, test } from "vitest"; +import { + CHUNK_SIZE_BYTES, + createBinaryChunkFiles, + createChunkFiles, + getMimeType, + shouldConvertToWav, + writeWavHeader, +} from "./audioChunker"; + +describe("getMimeType", () => { + test("returns correct mime types for audio formats", () => { + expect(getMimeType("mp3")).toBe("audio/mp3"); + expect(getMimeType("MP3")).toBe("audio/mp3"); + expect(getMimeType("m4a")).toBe("audio/mp4"); + expect(getMimeType("ogg")).toBe("audio/ogg"); + expect(getMimeType("wav")).toBe("audio/wav"); + expect(getMimeType("flac")).toBe("audio/flac"); + expect(getMimeType("webm")).toBe("audio/webm"); + expect(getMimeType("unknown")).toBe("audio/mpeg"); + }); +}); + +describe("shouldConvertToWav", () => { + test("returns true for m4a files", () => { + expect(shouldConvertToWav("m4a", "audio/mp4")).toBe(true); + expect(shouldConvertToWav("M4A", "audio/mp4")).toBe(true); + expect(shouldConvertToWav("mp3", "audio/mp4")).toBe(true); + expect(shouldConvertToWav("mp3", "audio/mpeg")).toBe(false); + }); +}); + +describe("createBinaryChunkFiles", () => { + test("returns single file when buffer is smaller than chunk size", () => { + const smallBuffer = new ArrayBuffer(1024); + const files = createBinaryChunkFiles(smallBuffer, "test", "mp3", "audio/mpeg"); + + expect(files).toHaveLength(1); + expect(files[0].name).toBe("test.mp3"); + expect(files[0].type).toBe("audio/mpeg"); + expect(files[0].size).toBe(1024); + }); + + test("returns multiple files when buffer exceeds chunk size", () => { + const largeBuffer = new ArrayBuffer(CHUNK_SIZE_BYTES * 2 + 1024); + const files = createBinaryChunkFiles(largeBuffer, "test", "mp3", "audio/mpeg"); + + expect(files).toHaveLength(3); + expect(files[0].name).toBe("test.part0.mp3"); + expect(files[1].name).toBe("test.part1.mp3"); + expect(files[2].name).toBe("test.part2.mp3"); + }); +}); + +describe("writeWavHeader", () => { + const WAV_HEADER_SIZE = 44; + + test("writes correct RIFF header", () => { + const buffer = new ArrayBuffer(WAV_HEADER_SIZE); + const view = new DataView(buffer); + + writeWavHeader(view, 44100, 2, 44100); + + const riff = String.fromCharCode( + view.getUint8(0), + view.getUint8(1), + view.getUint8(2), + view.getUint8(3), + ); + expect(riff).toBe("RIFF"); + + const wave = String.fromCharCode( + view.getUint8(8), + view.getUint8(9), + view.getUint8(10), + view.getUint8(11), + ); + expect(wave).toBe("WAVE"); + + const fmt = String.fromCharCode( + view.getUint8(12), + view.getUint8(13), + view.getUint8(14), + view.getUint8(15), + ); + expect(fmt).toBe("fmt "); + + const data = String.fromCharCode( + view.getUint8(36), + view.getUint8(37), + view.getUint8(38), + view.getUint8(39), + ); + expect(data).toBe("data"); + }); + + test("writes correct sample rate and channels", () => { + const buffer = new ArrayBuffer(WAV_HEADER_SIZE); + const view = new DataView(buffer); + + writeWavHeader(view, 44100, 2, 1000); + + expect(view.getUint16(22, true)).toBe(2); + expect(view.getUint32(24, true)).toBe(44100); + expect(view.getUint16(34, true)).toBe(16); + }); +}); + +describe("createChunkFiles small-file fast path (#168 / PR #204 review)", () => { + test("sends a small m4a as a single original file instead of WAV-splitting it", async () => { + // A 1 KB m4a is well under the 20 MB chunk size, so it must not be + // converted to WAV (which would balloon it into many chunks and reset + // diarization speaker labels). It should be one intact .m4a file. + const files = await createChunkFiles({ + buffer: new ArrayBuffer(1024), + basename: "episode", + extension: "m4a", + mimeType: "audio/mp4", + }); + + expect(files).toHaveLength(1); + expect(files[0].name).toBe("episode.m4a"); + expect(files[0].type).toBe("audio/mp4"); + expect(files[0].name).not.toContain(".wav"); + }); + + test("still sends a small mp3 as a single original file (unchanged)", async () => { + const files = await createChunkFiles({ + buffer: new ArrayBuffer(2048), + basename: "episode", + extension: "mp3", + mimeType: "audio/mpeg", + }); + + expect(files).toHaveLength(1); + expect(files[0].name).toBe("episode.mp3"); + expect(files[0].size).toBe(2048); + }); +}); diff --git a/src/services/audioChunker.ts b/src/services/audioChunker.ts new file mode 100644 index 00000000..5b2d10e5 --- /dev/null +++ b/src/services/audioChunker.ts @@ -0,0 +1,234 @@ +/** + * Pure audio chunking + WAV encoding for transcription uploads. Splits an episode + * audio buffer into request-sized files, decoding and re-encoding m4a to WAV only + * when it must be split (m4a can't be byte-split). Kept free of any + * TranscriptionService/plugin state so it can be unit-tested directly — the + * service just orchestrates it. + */ + +// OpenAI's audio upload limit. 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; + +export function getMimeType(fileExtension: string): string { + switch (fileExtension.toLowerCase()) { + case "mp3": + return "audio/mp3"; + case "m4a": + return "audio/mp4"; + case "ogg": + return "audio/ogg"; + case "wav": + return "audio/wav"; + case "flac": + return "audio/flac"; + case "webm": + return "audio/webm"; + default: + return "audio/mpeg"; + } +} + +export function shouldConvertToWav(extension: string, mimeType: string): boolean { + const normalizedExtension = extension.toLowerCase(); + return normalizedExtension === "m4a" || mimeType === "audio/mp4"; +} + +export async function createChunkFiles({ + buffer, + basename, + extension, + mimeType, +}: { + buffer: ArrayBuffer; + basename: string; + extension: string; + mimeType: string; +}): Promise { + // A file that already fits in a single request needs neither conversion nor + // splitting: send the original (compressed) bytes. OpenAI accepts m4a/mp3/etc. + // directly under the upload limit, so this skips the m4a->WAV path below for + // small m4a episodes — that path only exists to SAFELY SPLIT an m4a (which + // can't be byte-split) and would otherwise balloon a small m4a into many + // uncompressed WAV chunks, multiplying requests and, for diarization, + // resetting speaker labels at artificial chunk boundaries (#168 / PR #204 review). + if (buffer.byteLength <= CHUNK_SIZE_BYTES) { + return [new File([buffer], `${basename}.${extension}`, { type: mimeType })]; + } + + if (shouldConvertToWav(extension, mimeType)) { + const wavChunks = await convertToWavChunks(buffer, basename); + if (wavChunks.length > 0) { + return wavChunks; + } + } + + return createBinaryChunkFiles(buffer, basename, extension, mimeType); +} + +export function createBinaryChunkFiles( + buffer: ArrayBuffer, + basename: string, + extension: string, + mimeType: string, +): File[] { + if (buffer.byteLength <= CHUNK_SIZE_BYTES) { + return [ + new File([buffer], `${basename}.${extension}`, { + type: mimeType, + }), + ]; + } + + const files: File[] = []; + for ( + let offset = 0, index = 0; + offset < buffer.byteLength; + offset += CHUNK_SIZE_BYTES, index++ + ) { + const chunk = buffer.slice(offset, offset + CHUNK_SIZE_BYTES); + files.push( + new File([chunk], `${basename}.part${index}.${extension}`, { + type: mimeType, + }), + ); + } + + return files; +} + +async function convertToWavChunks( + buffer: ArrayBuffer, + basename: string, +): Promise { + const audioContext = createAudioContext(); + if (!audioContext) return []; + + try { + const audioBuffer = await audioContext.decodeAudioData(buffer.slice(0)); + return renderWavChunks(audioBuffer, basename); + } catch (error) { + console.warn("Failed to convert audio buffer for transcription", error); + return []; + } finally { + try { + await audioContext.close(); + } catch (error) { + console.warn("Failed to close audio context", error); + } + } +} + +function createAudioContext(): AudioContext | null { + if (typeof window === "undefined") { + return null; + } + + const contextCtor = + window.AudioContext || + (window as typeof window & { webkitAudioContext?: typeof AudioContext }) + .webkitAudioContext; + if (!contextCtor) { + return null; + } + + return new contextCtor(); +} + +function renderWavChunks(audioBuffer: AudioBuffer, basename: string): File[] { + const numChannels = audioBuffer.numberOfChannels; + const bytesPerFrame = numChannels * PCM_BYTES_PER_SAMPLE; + const availableBytesPerChunk = CHUNK_SIZE_BYTES - WAV_HEADER_SIZE; + const maxSamplesPerChunk = Math.max( + 1, + Math.floor(availableBytesPerChunk / bytesPerFrame), + ); + const channelData = Array.from({ length: numChannels }, (_, channelIndex) => + audioBuffer.getChannelData(channelIndex), + ); + const files: File[] = []; + let chunkIndex = 0; + + for ( + let startSample = 0; + startSample < audioBuffer.length; + startSample += maxSamplesPerChunk + ) { + const endSample = Math.min( + audioBuffer.length, + startSample + maxSamplesPerChunk, + ); + const wavBuffer = renderWavBuffer( + channelData, + audioBuffer.sampleRate, + startSample, + endSample, + ); + files.push( + new File([wavBuffer], `${basename}.part${chunkIndex}.wav`, { + type: "audio/wav", + }), + ); + chunkIndex++; + } + + return files; +} + +function renderWavBuffer( + channelData: Float32Array[], + sampleRate: number, + startSample: number, + endSample: number, +): ArrayBuffer { + const numChannels = channelData.length; + const sampleCount = Math.max(0, endSample - startSample); + const blockAlign = numChannels * PCM_BYTES_PER_SAMPLE; + const buffer = new ArrayBuffer(WAV_HEADER_SIZE + sampleCount * blockAlign); + const view = new DataView(buffer); + writeWavHeader(view, sampleRate, numChannels, sampleCount); + let offset = WAV_HEADER_SIZE; + + for (let i = 0; i < sampleCount; i++) { + for (let channel = 0; channel < numChannels; channel++) { + const sample = channelData[channel][startSample + i] ?? 0; + const clamped = Math.max(-1, Math.min(1, sample)); + const intSample = clamped < 0 ? clamped * 0x8000 : clamped * 0x7fff; + view.setInt16(offset, Math.round(intSample), true); + offset += PCM_BYTES_PER_SAMPLE; + } + } + + return buffer; +} + +export function writeWavHeader( + view: DataView, + sampleRate: number, + numChannels: number, + sampleCount: number, +): void { + const blockAlign = numChannels * PCM_BYTES_PER_SAMPLE; + const byteRate = sampleRate * blockAlign; + const dataSize = sampleCount * blockAlign; + writeString(view, 0, "RIFF"); + view.setUint32(4, 36 + dataSize, true); + writeString(view, 8, "WAVE"); + writeString(view, 12, "fmt "); + view.setUint32(16, 16, true); + view.setUint16(20, 1, true); + view.setUint16(22, numChannels, true); + view.setUint32(24, sampleRate, true); + view.setUint32(28, byteRate, true); + view.setUint16(32, blockAlign, true); + view.setUint16(34, PCM_BYTES_PER_SAMPLE * 8, true); + writeString(view, 36, "data"); + view.setUint32(40, dataSize, true); +} + +function writeString(view: DataView, offset: number, str: string): void { + for (let i = 0; i < str.length; i++) { + view.setUint8(offset + i, str.charCodeAt(i)); + } +} From 29abba2e37587b5a54a743cc9c610c79ad7f7099 Mon Sep 17 00:00:00 2001 From: Christian Bager Bach Houmann Date: Sun, 21 Jun 2026 20:15:14 +0200 Subject: [PATCH 4/5] refactor(settings): factor out repeated settings-tab boilerplate MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The settings tab repeated three patterns: a 3-line `settingEl.style` column-layout triplet (7 copies), an `el.empty()` + MarkdownRenderer demo-render block (4 copies), and a hand-rolled OPML file picker that duplicated the existing pickFile() helper. Extract stackSettingVertically() and renderMarkdownPreview(), and route OPML import through pickFile(). Net -36 LOC. OPML import now also gets pickFile()'s niceties — it cleans up its orphaned element, handles cancel, and applies a sanity size cap — instead of leaking the input. No change to the happy-path flow. --- src/ui/settings/PodNotesSettingsTab.ts | 124 +++++++++---------------- 1 file changed, 44 insertions(+), 80 deletions(-) diff --git a/src/ui/settings/PodNotesSettingsTab.ts b/src/ui/settings/PodNotesSettingsTab.ts index 8bac9284..49d86d0e 100644 --- a/src/ui/settings/PodNotesSettingsTab.ts +++ b/src/ui/settings/PodNotesSettingsTab.ts @@ -53,6 +53,29 @@ import { type DiarizationProviderId, } from "src/services/diarization"; +/** + * Stack a Setting's control beneath its name, full width — the layout the + * template/path text areas need. Obsidian's Setting has no built-in vertical + * variant and the plugin ships no stylesheet (styles are injected per Svelte + * component), so this sets the few inline styles in one place instead of the + * seven hand-rolled copies this replaced. + */ +function stackSettingVertically(setting: Setting): void { + setting.settingEl.style.flexDirection = "column"; + setting.settingEl.style.alignItems = "unset"; + setting.settingEl.style.gap = "10px"; +} + +/** + * Render `markdown` into `el` as a small live preview, replacing any prior + * content. Shared by the path/template demo fields that echo what the configured + * template resolves to. + */ +function renderMarkdownPreview(markdown: string, el: HTMLElement): void { + el.empty(); + MarkdownRenderer.renderMarkdown(markdown, el, "", new Component()); +} + export class PodNotesSettingsTab extends PluginSettingTab { plugin: PodNotes; @@ -258,9 +281,7 @@ export class PodNotesSettingsTab extends PluginSettingTab { textArea.inputEl.style.width = "100%"; }); - timestampSetting.settingEl.style.flexDirection = "column"; - timestampSetting.settingEl.style.alignItems = "unset"; - timestampSetting.settingEl.style.gap = "10px"; + stackSettingVertically(timestampSetting); const timestampFormatDemoEl = container.createDiv(); @@ -268,13 +289,7 @@ export class PodNotesSettingsTab extends PluginSettingTab { if (!this.plugin.api.podcast) return; const demoVal = TimestampTemplateEngine(value); - timestampFormatDemoEl.empty(); - MarkdownRenderer.renderMarkdown( - demoVal, - timestampFormatDemoEl, - "", - new Component(), - ); + renderMarkdownPreview(demoVal, timestampFormatDemoEl); }; new Setting(container) @@ -314,20 +329,12 @@ export class PodNotesSettingsTab extends PluginSettingTab { this.plugin.saveSettings(); const demoVal = FilePathTemplateEngine(value, randomEpisode); - noteCreationFilePathDemoEl.empty(); - MarkdownRenderer.renderMarkdown( - demoVal, - noteCreationFilePathDemoEl, - "", - new Component(), - ); + renderMarkdownPreview(demoVal, noteCreationFilePathDemoEl); }); textComponent.inputEl.style.width = "100%"; }); - noteCreationFilePathSetting.settingEl.style.flexDirection = "column"; - noteCreationFilePathSetting.settingEl.style.alignItems = "unset"; - noteCreationFilePathSetting.settingEl.style.gap = "10px"; + stackSettingVertically(noteCreationFilePathSetting); const noteCreationFilePathDemoEl = container.createDiv(); @@ -361,9 +368,7 @@ export class PodNotesSettingsTab extends PluginSettingTab { ); }); - noteCreationSetting.settingEl.style.flexDirection = "column"; - noteCreationSetting.settingEl.style.alignItems = "unset"; - noteCreationSetting.settingEl.style.gap = "10px"; + stackSettingVertically(noteCreationSetting); } private addFeedNoteSettings(settingsContainer: HTMLDivElement) { @@ -397,21 +402,13 @@ export class PodNotesSettingsTab extends PluginSettingTab { textComponent.inputEl.style.width = "100%"; }); - feedNotePathSetting.settingEl.style.flexDirection = "column"; - feedNotePathSetting.settingEl.style.alignItems = "unset"; - feedNotePathSetting.settingEl.style.gap = "10px"; + stackSettingVertically(feedNotePathSetting); const feedNotePathDemoEl = container.createDiv(); const renderFeedPathDemo = (value: string) => { const demoVal = FeedFilePathTemplateEngine(value, randomFeed); - feedNotePathDemoEl.empty(); - MarkdownRenderer.renderMarkdown( - demoVal, - feedNotePathDemoEl, - "", - new Component(), - ); + renderMarkdownPreview(demoVal, feedNotePathDemoEl); }; renderFeedPathDemo(this.plugin.settings.feedNote.path); @@ -429,9 +426,7 @@ export class PodNotesSettingsTab extends PluginSettingTab { textArea.inputEl.style.height = "25vh"; }); - feedNoteTemplateSetting.settingEl.style.flexDirection = "column"; - feedNoteTemplateSetting.settingEl.style.alignItems = "unset"; - feedNoteTemplateSetting.settingEl.style.gap = "10px"; + stackSettingVertically(feedNoteTemplateSetting); } private addDownloadSettings(container: HTMLDivElement) { @@ -456,9 +451,7 @@ export class PodNotesSettingsTab extends PluginSettingTab { textComponent.inputEl.style.width = "100%"; }); - downloadPathSetting.settingEl.style.flexDirection = "column"; - downloadPathSetting.settingEl.style.alignItems = "unset"; - downloadPathSetting.settingEl.style.gap = "10px"; + stackSettingVertically(downloadPathSetting); const downloadFilePathDemoEl = container.createDiv(); const downloadFilePathWarningEl = container.createDiv(); @@ -469,13 +462,7 @@ export class PodNotesSettingsTab extends PluginSettingTab { // empty path resolves to ".mp3" at the vault root (#183). Warn inline. const refreshDownloadPathHints = (value: string) => { const demoVal = DownloadPathTemplateEngine(value, randomEpisode); - downloadFilePathDemoEl.empty(); - MarkdownRenderer.renderMarkdown( - `${demoVal}.mp3`, - downloadFilePathDemoEl, - "", - new Component(), - ); + renderMarkdownPreview(`${demoVal}.mp3`, downloadFilePathDemoEl); // Match only the forms DownloadPathTemplateEngine actually resolves — // {{title}} or {{title:...}}. A looser test (e.g. \s* after {{, or \b) @@ -541,38 +528,17 @@ export class PodNotesSettingsTab extends PluginSettingTab { .setDesc("Import podcasts from an OPML file.") .addButton((button) => button.setButtonText("Import").onClick(() => { - const fileInput = document.createElement("input"); - fileInput.type = "file"; - fileInput.accept = ".opml"; - fileInput.style.display = "none"; - document.body.appendChild(fileInput); - fileInput.click(); - - fileInput.onchange = async (e: Event) => { - const target = e.target as HTMLInputElement; - const file = target.files?.[0]; - - if (file) { - const reader = new FileReader(); - reader.onload = async (event) => { - const contents = event.target?.result as string; - if (contents) { - try { - await importOPML(contents); - } catch (e) { - console.error("Error importing OPML:", e); - new Notice( - `Error importing OPML: ${e instanceof Error ? e.message : "Unknown error"}`, - 10000, - ); - } - } - }; - reader.readAsText(file); - } else { - new Notice("No file selected"); + this.pickFile(".opml", async (contents) => { + try { + await importOPML(contents); + } catch (e) { + console.error("Error importing OPML:", e); + new Notice( + `Error importing OPML: ${e instanceof Error ? e.message : "Unknown error"}`, + 10000, + ); } - }; + }); }), ); @@ -876,9 +842,7 @@ export class PodNotesSettingsTab extends PluginSettingTab { text.inputEl.style.height = "25vh"; }); - transcriptTemplateSetting.settingEl.style.flexDirection = "column"; - transcriptTemplateSetting.settingEl.style.alignItems = "unset"; - transcriptTemplateSetting.settingEl.style.gap = "10px"; + stackSettingVertically(transcriptTemplateSetting); this.addDiarizationSettings(container); } From d85e7aaabfb0902fc3fcf62490778afe9830a186 Mon Sep 17 00:00:00 2001 From: Christian Bager Bach Houmann Date: Sun, 21 Jun 2026 20:28:11 +0200 Subject: [PATCH 5/5] refactor: tighten review findings (download-remove seam + OPML picker) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adversarial review (3 Codex reviewers) flagged two behavior/seam issues: - OPML import routed through pickFile() inherited the settings-import 5MB cap AND its "too large to be a PodNotes settings file" message. Make the cap and message per-caller (maxBytes/tooLargeMessage options) and give OPML its own copy — keeps the memory-safety cap, drops the wrong text. - M1's remove-download seam was shallow: a caller had to remember to delete the file after removeEpisode() or leak it. Add a composed removeDownloadedEpisode() that owns remove-then-delete and make deleteEpisodeFile module-private (symmetric with createEpisodeFile). The store stays pure; the context menu calls one function. Also trimmed the M1 doc comments. Happy-path behavior unchanged; gates green, 657 tests. --- src/downloadEpisode.ts | 20 +++++--- src/store/downloads.ts | 6 +-- src/ui/PodcastView/spawnEpisodeContextMenu.ts | 7 +-- src/ui/settings/PodNotesSettingsTab.ts | 47 ++++++++++++------- 4 files changed, 48 insertions(+), 32 deletions(-) diff --git a/src/downloadEpisode.ts b/src/downloadEpisode.ts index 98591f93..364aa085 100644 --- a/src/downloadEpisode.ts +++ b/src/downloadEpisode.ts @@ -255,13 +255,21 @@ async function createEpisodeFile({ } /** - * Delete a downloaded episode's backing file from the vault. The download store - * owns the offline-set state and hands the path here for the actual file I/O, - * keeping vault side effects out of the store layer. Best-effort: a missing or - * already-removed file is a no-op, and failures are logged rather than thrown so - * removing a stale entry never breaks the calling UI flow. + * Remove a downloaded episode: drop it from the offline set and delete its + * backing vault file. This composes the pure store removal with the file I/O so + * callers can't do one without the other (and leak files); the download store + * stays free of vault side effects. */ -export async function deleteEpisodeFile(filePath: string): Promise { +export async function removeDownloadedEpisode(episode: Episode): Promise { + const removedFilePath = downloadedEpisodes.removeEpisode(episode); + if (removedFilePath) { + await deleteEpisodeFile(removedFilePath); + } +} + +// Best-effort: a missing/already-removed file is a no-op, and failures are logged +// rather than thrown so removing a stale entry never breaks the calling UI flow. +async function deleteEpisodeFile(filePath: string): Promise { if (!filePath) return; try { diff --git a/src/store/downloads.ts b/src/store/downloads.ts index d98f6eb5..4a682a3f 100644 --- a/src/store/downloads.ts +++ b/src/store/downloads.ts @@ -49,10 +49,8 @@ export const downloadedEpisodes = (() => { }, /** * Drops an episode from the offline set and returns the vault path of the - * file it backed (or `undefined` if it wasn't tracked). This store is the - * pure state core, so it never touches the vault itself — the caller deletes - * the returned file via `deleteEpisodeFile` in the download module. Mirrors - * how #211 split the queue's automation side effect out of persistence. + * file it backed (or `undefined` if it wasn't tracked). Pure state only: the + * caller deletes the returned file (see `removeDownloadedEpisode`). */ removeEpisode: (episode: Episode): string | undefined => { let removedFilePath: string | undefined; diff --git a/src/ui/PodcastView/spawnEpisodeContextMenu.ts b/src/ui/PodcastView/spawnEpisodeContextMenu.ts index 9bf1d86f..3d1defdb 100644 --- a/src/ui/PodcastView/spawnEpisodeContextMenu.ts +++ b/src/ui/PodcastView/spawnEpisodeContextMenu.ts @@ -2,7 +2,7 @@ import { Menu, Notice } from "obsidian"; import createPodcastNote, { getPodcastNote, openPodcastNote } from "src/createPodcastNote"; import createFeedNote, { getFeedNote, openFeedNote } from "src/createFeedNote"; import downloadEpisodeWithProgessNotice, { - deleteEpisodeFile, + removeDownloadedEpisode, } from "src/downloadEpisode"; import { currentEpisode, downloadedEpisodes, favorites, playedEpisodes, playlists, plugin, queue, savedFeeds, viewState } from "src/store"; import type { Episode } from "src/types/Episode"; @@ -70,10 +70,7 @@ export default function spawnEpisodeContextMenu( .setTitle(isDownloaded ? "Remove file" : "Download") .onClick(() => { if (isDownloaded) { - const removedFilePath = downloadedEpisodes.removeEpisode(episode); - if (removedFilePath) { - void deleteEpisodeFile(removedFilePath); - } + void removeDownloadedEpisode(episode); } else { // The path template always yields a per-episode file via // safeDownloadBasename (#183), so no empty-path guard is needed — diff --git a/src/ui/settings/PodNotesSettingsTab.ts b/src/ui/settings/PodNotesSettingsTab.ts index 49d86d0e..bcaf3a87 100644 --- a/src/ui/settings/PodNotesSettingsTab.ts +++ b/src/ui/settings/PodNotesSettingsTab.ts @@ -528,17 +528,21 @@ export class PodNotesSettingsTab extends PluginSettingTab { .setDesc("Import podcasts from an OPML file.") .addButton((button) => button.setButtonText("Import").onClick(() => { - this.pickFile(".opml", async (contents) => { - try { - await importOPML(contents); - } catch (e) { - console.error("Error importing OPML:", e); - new Notice( - `Error importing OPML: ${e instanceof Error ? e.message : "Unknown error"}`, - 10000, - ); - } - }); + this.pickFile( + ".opml", + async (contents) => { + try { + await importOPML(contents); + } catch (e) { + console.error("Error importing OPML:", e); + new Notice( + `Error importing OPML: ${e instanceof Error ? e.message : "Unknown error"}`, + 10000, + ); + } + }, + { tooLargeMessage: "That file is too large to be an OPML file." }, + ); }), ); @@ -625,7 +629,19 @@ export class PodNotesSettingsTab extends PluginSettingTab { ); } - private pickFile(accept: string, onContents: (contents: string) => void): void { + private pickFile( + accept: string, + onContents: (contents: string) => void, + options: { maxBytes?: number; tooLargeMessage?: string } = {}, + ): void { + // Both pickers read small text files, so cap the size before reading the + // whole thing into memory. The cap and its message are per-caller so OPML + // import doesn't inherit the settings-file copy. + const maxBytes = options.maxBytes ?? 5 * 1024 * 1024; + const tooLargeMessage = + options.tooLargeMessage ?? + "That file is too large to be a PodNotes settings file."; + const fileInput = document.createElement("input"); fileInput.type = "file"; fileInput.accept = accept; @@ -646,11 +662,8 @@ export class PodNotesSettingsTab extends PluginSettingTab { return; } - // A settings file is small JSON; reject anything implausibly large - // before reading it fully into memory. - const MAX_BYTES = 5 * 1024 * 1024; - if (file.size > MAX_BYTES) { - new Notice("That file is too large to be a PodNotes settings file."); + if (file.size > maxBytes) { + new Notice(tooLargeMessage); return; }