Skip to content

Commit 5aee276

Browse files
authored
fix(download): stream to a temp file then move into place (#220)
Stream each chunk to a dot-prefixed sibling temp the vault watchers don't see, then rename it into place once complete and size-verified - so watcher plugins (Waypoint, Dataview, Obsidian Git) don't get a per-chunk modify storm on the growing media file and OOM-crash the app on mobile. Fixes the crash reported on #113.
1 parent 1535e17 commit 5aee276

5 files changed

Lines changed: 455 additions & 26 deletions

File tree

Lines changed: 97 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,97 @@
1+
---
2+
name: verify-in-obsidian
3+
description: >-
4+
Verify an Obsidian plugin change/fix actually works by driving the REAL Obsidian app in
5+
a dev vault via the `obsidian` CLI (eval / dev:console / dev:errors / dev:screenshot /
6+
command / plugin:reload) and asserting on persisted state — instead of trusting unit
7+
tests with mocks (jsdom can't load real Obsidian). Use when asked to verify/QA a fix,
8+
confirm a feature works end-to-end, check that something persists, reproduce a
9+
user-reported bug in the real app, or validate before merging — especially stateful UI
10+
(settings, modals, drag-reorder, lists). FIRST read the repo's AGENTS.md/CLAUDE.md for
11+
the vault name, plugin id, dev commands, and any QA matrix.
12+
when_to_use: >-
13+
"verify this works", "QA the fix", "does it persist / save", "reproduce the bug in the
14+
real app", "check end-to-end before merge", "drive the GUI", "test in the dev vault".
15+
---
16+
17+
# Verify Obsidian plugin changes in the real app
18+
19+
Vitest/jsdom can't load real Obsidian, so unit tests run against mocks/stubs — they **pass
20+
while the real flow is broken**. For anything stateful (settings, modals, drag-reorder,
21+
persistence), "the unit test passes" is necessary but **not sufficient**. Verify the real
22+
app and assert on what actually persisted.
23+
24+
## Step 0 — read the repo's own guide first
25+
`AGENTS.md` / `CLAUDE.md` hold the project specifics: **vault name, plugin id, dev-vault
26+
path, build/test commands, and the QA matrix.** Use those, not guesses. Many Obsidian repos
27+
already mandate CLI-verifiable development — follow it.
28+
29+
## The `obsidian` CLI (the house tool for this)
30+
Anything you can do in Obsidian you can do from the CLI, including dev tools.
31+
32+
- **`vault=<name>` is a PREFIX argument**, always first: `obsidian vault=dev <command> …`.
33+
The suffix form (`obsidian <command> vault=dev`) can resolve to the wrong vault — don't use it.
34+
- Useful commands: `plugin:reload id=<plugin>`, `command id=<commandId>`, `eval code=…`,
35+
`dev:debug on|off`, `dev:console`, `dev:errors`, `dev:screenshot`, `dev:dom`, `dev:css`,
36+
`dev:cdp`, `dev:mobile`, `devtools`, `tabs`, `workspace` (the repo's guide lists the full set).
37+
- `dev:console` / `dev:errors` are only reliable while debugger capture is attached
38+
(`obsidian vault=dev dev:debug on`).
39+
- For non-trivial `eval` code, pass it via a **heredoc/file to `code=…`** to avoid
40+
shell-quoting corruption.
41+
42+
## The loop (evidence-first)
43+
1. **Rebuild** the bundle (watch: `bun run dev`; one-off per the repo's build script). Note the
44+
plugin `main.js` is often **symlinked** into the dev vault, so a rebuild updates it in place.
45+
2. **Reload the plugin**`obsidian vault=dev plugin:reload id=<plugin>`. A rebuilt `main.js`
46+
does NOT auto-reload in a running Obsidian. **This is the #1 false-negative**: never conclude
47+
a fix "doesn't work" without confirming the running app loaded the fresh build.
48+
3. **Reproduce / seed** deterministic state (edit `data.json` or use the plugin's data API),
49+
then reload so it takes effect.
50+
4. **Trigger** the behavior — `obsidian vault=dev command id=<…>`, or drive the UI via `eval`
51+
(see gotchas). Test BOTH hotkey and direct-command paths when relevant.
52+
5. **Gather evidence**`dev:debug on``dev:console clear`/`dev:errors clear` → act →
53+
`dev:console limit=200`, `dev:errors`, `dev:screenshot`, and `tabs`/`workspace ids` for layout.
54+
6. **Assert on persisted state** — read the on-disk `data.json` (source of truth), not just the
55+
UI. Confirm it's plain JSON with no framework artifacts (e.g. Svelte `$state` Proxy leakage).
56+
7. **Add/adjust a regression test** around a CLI-native seam so it's caught without manual
57+
steps. Structure code so Obsidian deps are injected behind interfaces; unit tests then swap
58+
in adapters / the obsidian stub for the pure logic.
59+
8. **Clean up** — remove seeded data + temp files; `dev:debug off`.
60+
61+
## Evidence-first triage (for bug reports)
62+
- **Don't assume the reported bug still exists** — it may have been fixed by unrelated changes.
63+
Confirm current behavior in the dev vault before touching code.
64+
- **Reproduce under real user conditions**, not synthetic ones: the actual plugin settings,
65+
workspace/tab layout, and platform — and test BOTH the hotkey and direct-command
66+
(`command id=…`) paths.
67+
- **Capture evidence before AND after** the action (`dev:console`, `dev:errors`,
68+
`dev:screenshot`, `tabs`, `workspace`). For pane/tab layout, `workspace … ids` is the
69+
authoritative evidence; `tabs` is a quick summary.
70+
- **If you can't reproduce** after solid evidence-gathering, report the exact setup you tested
71+
and ask for a fresh issue with versions, config, and repro artifacts — don't guess-fix.
72+
73+
## Driving UI via `eval` — gotchas (apply to raw `eval` and obsidian-e2e alike)
74+
- `eval` may **return before a long async body finishes**. Don't chain `await sleep(...)`
75+
inside one big eval and trust the return. Instead: **fast evals** (one DOM action + immediate
76+
return), and **sequence timing outside** (shell/Node `sleep` between calls).
77+
- **Return JSON-serializable values** (a string), never a live DOM node/object.
78+
- **Selectors:** reuse components' `aria-label`s — they're stable and intent-named.
79+
- **Stacked modals:** query all matches, act on the **last** (topmost). Count `.modal-container`.
80+
- **State pollutes across runs:** close leftovers first (click all `.modal-close-button`,
81+
close settings).
82+
- **Read results reliably:** from a final UI-stable eval, or have the eval write to a vault
83+
file and read it from disk.
84+
85+
## Committed regression tests (`obsidian-e2e`, if available)
86+
For flows worth guarding, port the check into the repo's e2e suite:
87+
`createObsidianClient({ vault })`, a plugin handle, `acquireVaultRunLock`/`publishMarker` in
88+
`beforeAll`, and `restoreData()` + lock release in `afterAll`. The handle exposes
89+
`reload`, `data().patch(...)`, `exec(commandId, args)`, and `dev.eval(...)`.
90+
**These are local-only** (CI has no Obsidian) — keep a CI-runnable unit/component test as the
91+
primary guard and use the e2e test as the integration safety net.
92+
93+
## The two lessons worth internalizing
94+
1. **Stale loaded code** is the most common false negative — reload the plugin before
95+
concluding anything about a fix.
96+
2. **Verify the persisted state, not the UI** — and add the regression test against a
97+
CLI-native seam, because mock-based unit tests will happily pass on a broken real flow.

src/download/streaming.test.ts

Lines changed: 100 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,11 @@ import { requestUrl } from "obsidian";
33
import { plugin } from "../store";
44
import type PodNotes from "../main";
55
import {
6+
isPartialPath,
7+
moveIntoPlace,
8+
partialPathFor,
69
probeAndFetchFirstChunk,
10+
sweepStalePartials,
711
writeStreamedFile,
812
type RangeProbe,
913
} from "./streaming";
@@ -183,3 +187,99 @@ describe("writeStreamedFile", () => {
183187
).rejects.toThrow(/Range request failed/);
184188
});
185189
});
190+
191+
describe("partialPathFor / isPartialPath", () => {
192+
it("builds a dot-prefixed sibling temp in the same folder", () => {
193+
const tmp = partialPathFor("Podcasts/Show/Ep 1.mp3");
194+
// dir + ".<token>.<name>.podnotes-partial" (token first so it survives the cap)
195+
expect(tmp).toMatch(/^Podcasts\/Show\/\..*\.Ep 1\.mp3\.podnotes-partial$/);
196+
expect(isPartialPath(tmp)).toBe(true);
197+
});
198+
199+
it("handles a vault-root (no folder) path", () => {
200+
const tmp = partialPathFor("Ep 1.mp3");
201+
expect(tmp).toMatch(/^\..*\.Ep 1\.mp3\.podnotes-partial$/);
202+
expect(tmp).not.toContain("/");
203+
expect(isPartialPath(tmp)).toBe(true);
204+
});
205+
206+
it("is unique per call so concurrent downloads to one final path never clash", () => {
207+
const a = partialPathFor("Podcasts/Ep.mp3");
208+
const b = partialPathFor("Podcasts/Ep.mp3");
209+
expect(a).not.toBe(b);
210+
});
211+
212+
it("keeps the temp name within the filesystem limit for a maxed-out title (#22)", () => {
213+
// The final name segment is already capped to ~255; the dot prefix + suffix
214+
// must not push the temp past ENAMETOOLONG on the new write path.
215+
const longFinal = `Podcasts/${"X".repeat(400)}.mp3`;
216+
const tmp = partialPathFor(longFinal);
217+
const lastSegment = tmp.split("/").pop() ?? "";
218+
expect(lastSegment.length).toBeLessThanOrEqual(255);
219+
expect(isPartialPath(tmp)).toBe(true);
220+
// The unique token (front of the name) must survive the tail truncation.
221+
expect(partialPathFor(longFinal)).not.toBe(tmp);
222+
});
223+
224+
it("rejects non-partial paths", () => {
225+
expect(isPartialPath("Podcasts/Ep.mp3")).toBe(false);
226+
expect(isPartialPath("Podcasts/.Ep.mp3")).toBe(false); // dotfile, wrong suffix
227+
expect(isPartialPath("Podcasts/Ep.mp3.podnotes-partial")).toBe(false); // no dot prefix
228+
});
229+
});
230+
231+
describe("moveIntoPlace", () => {
232+
it("moves the temp into place with a single rename (buffers nothing)", async () => {
233+
const rename = vi.fn(async () => {});
234+
plugin.set({
235+
app: { vault: { adapter: { writeBinary: vi.fn(), rename } } },
236+
} as unknown as PodNotes);
237+
238+
await moveIntoPlace("dir/.tok.ep.mp3.podnotes-partial", "dir/ep.mp3");
239+
240+
expect(rename).toHaveBeenCalledWith(
241+
"dir/.tok.ep.mp3.podnotes-partial",
242+
"dir/ep.mp3",
243+
);
244+
});
245+
});
246+
247+
describe("sweepStalePartials", () => {
248+
function setupSweepAdapter(files: string[]) {
249+
const remove = vi.fn(async () => {});
250+
const list = vi.fn(async () => ({ files, folders: [] as string[] }));
251+
plugin.set({
252+
app: { vault: { adapter: { writeBinary: vi.fn(), list, remove } } },
253+
} as unknown as PodNotes);
254+
return { remove, list };
255+
}
256+
257+
it("removes orphaned partials but never an active one or a real file", async () => {
258+
const s = setupSweepAdapter([
259+
"Podcasts/.Ep.dead.podnotes-partial", // orphan -> remove
260+
"Podcasts/.Ep.live.podnotes-partial", // in flight -> keep
261+
"Podcasts/Ep.mp3", // real file -> keep
262+
]);
263+
264+
await sweepStalePartials(
265+
"Podcasts",
266+
(p) => p === "Podcasts/.Ep.live.podnotes-partial",
267+
);
268+
269+
expect(s.remove).toHaveBeenCalledTimes(1);
270+
expect(s.remove).toHaveBeenCalledWith("Podcasts/.Ep.dead.podnotes-partial");
271+
});
272+
273+
it("never throws when listing fails (best-effort)", async () => {
274+
const list = vi.fn(async () => {
275+
throw new Error("list failed");
276+
});
277+
plugin.set({
278+
app: { vault: { adapter: { writeBinary: vi.fn(), list, remove: vi.fn() } } },
279+
} as unknown as PodNotes);
280+
281+
await expect(
282+
sweepStalePartials("Podcasts", () => false),
283+
).resolves.toBeUndefined();
284+
});
285+
});

src/download/streaming.ts

Lines changed: 94 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -1,19 +1,35 @@
1-
import { requestUrl } from "obsidian";
1+
import { type DataAdapter, requestUrl } from "obsidian";
22
import { get } from "svelte/store";
33
import { plugin } from "../store";
44
import { encodeUrlForRequest } from "../utility/encodeUrlForRequest";
5+
import { enforceMaxPathLength } from "../utility/enforceMaxPathLength";
56

67
// ---- Streaming download (issue #113) ---------------------------------------
78
// Mobile WebViews have a tight per-process memory budget. Buffering a whole
89
// episode (hundreds of MB) via requestUrl().arrayBuffer — plus the native->JS
910
// bridge copy — used to OOM-kill Obsidian on iOS. Instead we pull the file in
1011
// bounded HTTP Range chunks and append each straight to disk, so peak heap
1112
// stays at roughly one chunk regardless of episode size.
13+
//
14+
// ---- Temp-then-move (Waypoint et al. crash) --------------------------------
15+
// Appending chunks directly to the final vault path makes the growing, half-
16+
// written media file visible to the vault's file watcher: a single download
17+
// fires ~12 create/modify events. Watcher plugins (Waypoint, Dataview, Obsidian
18+
// Git, MOC/index generators) react to each one and re-scan the partial file,
19+
// and that synchronous re-scan storm — racing the chunked writer — OOM-crashes
20+
// the app on mobile. So we stream every chunk to a dot-prefixed sibling temp
21+
// (which Obsidian keeps out of the file index entirely, so it fires zero events
22+
// while it grows), then move the finished, size-verified file into place as a
23+
// single rename. Watchers then see exactly one create of an already-complete
24+
// file — the same shape the pre-#113 atomic createBinary path produced.
1225

1326
export const DOWNLOAD_CHUNK_SIZE = 4 * 1024 * 1024; // 4 MiB per range request
1427

15-
export interface BinaryAppendAdapter {
16-
writeBinary(path: string, data: ArrayBuffer): Promise<void>;
28+
// Obsidian's DataAdapter — writeBinary/rename/remove/list, all used below — is
29+
// fully typed and public. Only `appendBinary` exists at runtime on the desktop and
30+
// mobile Capacitor adapters without appearing in the public typings, so we extend
31+
// the real interface with that single bolt-on and keep one cast (appendableAdapter).
32+
export interface BinaryAppendAdapter extends DataAdapter {
1733
appendBinary?(path: string, data: ArrayBuffer): Promise<void>;
1834
}
1935

@@ -97,18 +113,20 @@ export async function probeAndFetchFirstChunk(
97113
}
98114

99115
// Write the already-fetched first chunk, then pull the remaining bytes in
100-
// bounded Range requests, appending each straight to disk. Peak memory is one
101-
// chunk, not the whole file. Returns the total number of bytes written.
116+
// bounded Range requests, appending each straight to `destPath`. Peak memory is
117+
// one chunk, not the whole file. Returns the total number of bytes written.
118+
// `destPath` is the temp path (see partialPathFor); the caller moves the result
119+
// into the final vault path once it is complete and size-verified.
102120
export async function writeStreamedFile(
103121
url: string,
104-
filePath: string,
122+
destPath: string,
105123
probe: RangeProbe,
106124
onProgress?: (written: number, total: number | null) => void,
107125
chunkSize: number = DOWNLOAD_CHUNK_SIZE,
108126
): Promise<number> {
109127
const adapter = appendableAdapter();
110128

111-
await adapter.writeBinary(filePath, probe.firstChunk);
129+
await adapter.writeBinary(destPath, probe.firstChunk);
112130
let written = probe.firstChunk.byteLength;
113131
onProgress?.(written, probe.totalSize);
114132

@@ -144,7 +162,7 @@ export async function writeStreamedFile(
144162
const chunk = response.arrayBuffer;
145163
if (chunk.byteLength === 0) break;
146164

147-
await adapter.appendBinary(filePath, chunk);
165+
await adapter.appendBinary(destPath, chunk);
148166
written += chunk.byteLength;
149167
onProgress?.(written, probe.totalSize);
150168

@@ -154,3 +172,71 @@ export async function writeStreamedFile(
154172

155173
return written;
156174
}
175+
176+
const PARTIAL_SUFFIX = ".podnotes-partial";
177+
178+
// A monotonic counter makes every temp name unique within a session even when two
179+
// downloads resolve to the same final path (distinct episodes can collide on one
180+
// path — #107/#183) and start in the same millisecond, so concurrent downloads
181+
// never share a temp file.
182+
let partialCounter = 0;
183+
184+
// The sibling temp path a download streams to before being moved into place. It is
185+
// dot-prefixed so Obsidian keeps it out of the file index (no watcher events while
186+
// it grows), lives in the same folder as the final file so the move is a cheap
187+
// in-place rename, and carries a unique token so concurrent downloads can't clash.
188+
// The token comes first so it survives the length cap (#22), which trims the tail:
189+
// the final name's own segment is already at the byte limit, and prepending a dot +
190+
// appending the suffix would otherwise push the temp past ENAMETOOLONG. The embedded
191+
// final name is only there to make the temp recognisable while debugging.
192+
export function partialPathFor(filePath: string): string {
193+
const slash = filePath.lastIndexOf("/");
194+
const dir = slash === -1 ? "" : filePath.slice(0, slash + 1);
195+
const name = slash === -1 ? filePath : filePath.slice(slash + 1);
196+
const token = `${Date.now().toString(36)}-${(partialCounter++).toString(36)}`;
197+
return enforceMaxPathLength(`${dir}.${token}.${name}${PARTIAL_SUFFIX}`, PARTIAL_SUFFIX);
198+
}
199+
200+
export function isPartialPath(path: string): boolean {
201+
const name = path.slice(path.lastIndexOf("/") + 1);
202+
return name.startsWith(".") && name.endsWith(PARTIAL_SUFFIX);
203+
}
204+
205+
// Move the completed temp file to its final vault path with a single rename, so
206+
// watchers see one create of an already-complete file. rename is a standard
207+
// DataAdapter op on every platform and, because the temp is a sibling of the final
208+
// file, an in-place metadata move that buffers zero bytes — preserving #113's
209+
// memory win. (We never finalize by reading the temp back into memory and
210+
// re-writing it: that whole-file buffer is exactly the #113 OOM this path avoids.)
211+
export async function moveIntoPlace(
212+
tmpPath: string,
213+
filePath: string,
214+
): Promise<void> {
215+
await appendableAdapter().rename(tmpPath, filePath);
216+
}
217+
218+
// Remove temp partials orphaned in `folder` by a previous download that was hard-
219+
// killed (OOM, force-quit) before it could move its file into place or clean up —
220+
// otherwise hidden partials accumulate and can replicate via sync. `isActive`
221+
// guards this and any concurrent download's live temp from being swept (temp names
222+
// are unique per attempt, so a live temp would otherwise look like an orphan).
223+
// Best-effort: a listing failure must never block a download.
224+
export async function sweepStalePartials(
225+
folder: string,
226+
isActive: (path: string) => boolean,
227+
): Promise<void> {
228+
const adapter = appendableAdapter();
229+
try {
230+
const listing = await adapter.list(folder);
231+
for (const entry of listing.files) {
232+
if (isPartialPath(entry) && !isActive(entry)) {
233+
await adapter.remove(entry);
234+
}
235+
}
236+
} catch (error) {
237+
console.error(
238+
`Failed to sweep stale download temp files in "${folder}":`,
239+
error,
240+
);
241+
}
242+
}

0 commit comments

Comments
 (0)