refactor: tighten the seams left after #211 (store/templates/transcription/settings) - #212
Merged
Merged
Conversation
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.
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.
…rvice 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.
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 <input> element, handles cancel, and applies a sanity size cap — instead of leaking the input. No change to the happy-path flow.
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.
Deploying podnotes with
|
| Latest commit: |
d85e7aa
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://ffab15ca.podnotes.pages.dev |
| Branch Preview URL: | https://chhoumann-refactor-architect.podnotes.pages.dev |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
Contributor
|
🎉 This PR is included in version 2.17.0 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
A second, behavior-preserving architecture pass that finishes the seams the #211 god-module split left behind. No user-facing feature change; net −103 LOC across 10 files.
refactor:subject → no semantic-release version bump.Five focused, independently-gated steps:
refactor(store)— keep vault file I/O out of the downloads store.downloadedEpisodes.removeEpisodedeleted a vault file through a globalappbehind the only production@ts-ignore. The store is the pure state core, so it now just removes the entry and the file deletion lives in the download module — the same "separate the side effect from the state mutation" move refactor: declarative persistence + split store/main god-modules #211 applied to the queue.refactor(templates)— dedupe the shared file-name tag registration.FilePath/DownloadPath/Transcript(andFeedFilePath) engines repeated an identical{{title}}/{{podcast}}/{{date}}/{{currentdate}}/{{episodenumber}}block; extractedaddEpisodeFileNameTags+legalizedNameTag.NoteTemplateEngineis deliberately untouched (its{{title}}is the raw episode title, not a file name).refactor(transcripts)— extract pure WAV chunking intosrc/services/audioChunker.ts, shrinkingTranscriptionService631 → 400 LOC. The chunker tests previously kept hand-maintained copies of these functions (and reachedcreateChunkFilesvia anas unknown ascast); they now exercise the real exports and can no longer drift.refactor(settings)— factor out repeated settings-tab boilerplate:stackSettingVertically(replaces a 7×-copiedsettingEl.styletriplet) andrenderMarkdownPreview(replaces a 4×-copiedempty()+MarkdownRendererblock); OPML import now reuses the existingpickFilehelper.refactor— tighten review findings: after an adversarial review (3 reviewers on the opposite model), madepickFile's size cap/message per-caller so OPML import no longer shows a settings-file error, and added a composedremoveDownloadedEpisode()so the remove-then-delete invariant can't be half-applied (deleteEpisodeFileis now module-private).Behavioral impact
Behavior-preserving. The only observable deltas are improvements: OPML import shows a correct "too large to be an OPML file" message (it previously would have shown a settings-file message under the shared helper), and removing a downloaded episode is now leak-safe by construction.
Verification
Commands (Node 22):
npm run typecheck,npm run lint,npm run check:a11y,npm run build,npx vitest run— all green, 657 unit tests (58 files), after every step.Obsidian runtime verification (isolated per-worktree vault): plugin loads with zero console errors, 19 commands registered, player view opens, transcribe command degrades gracefully with no key/episode, settings tab renders with all 7 stacked layouts applied and the OPML Import button present.
Real-app e2e suite (
tests/e2e/podnotes-runtime.test.ts): the two segment-URI playback tests ("No PodNotes audio element found") flaked non-deterministically (9/11 then 10/11 across identical runs) — a known audio-element-mount race. This diff touches none of the player/audio/URI/segment code paths, and all deterministic unit coverage of that logic passes.Adversarial review (3 reviewers): verdict PASS, no high-severity findings. Two medium findings (OPML picker policy leak, shallow download-remove seam) and one low (comment trim) were actioned in the final commit; the rest were deliberate, documented tradeoffs (test-only chunker exports that eliminate the test copies; a pre-existing deprecated-
renderMarkdownpattern left as-is to stay behavior-preserving).I'm not merging this — leaving it for maintainer review.