Repository navigation
feat(web): keep downloads in a folder on disk - #231
mia-riezebos wants to merge 6 commits into
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configuration
You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Caution
Three reproduced concurrency failures can overwrite downloads, delete a file from a previously linked folder, or leave a completed download pointing at the wrong folder.
Reviewed changes across all nine files, covering folder-backed storage, recovery, permissions, and the Downloads UI.
- Folder storage: Readable file names, streaming copies, a portable manifest, and an IndexedDB-persisted directory handle.
- Download lifecycle: Automatic moves after completion, manual moves in both directions, manifest adoption, unlinking, and location-independent
file()access. - Downloads UI: Folder selection and access controls, location captions, move actions, and unsupported-browser handling.
- Verification: All 293 existing web unit tests pass and Svelte/TypeScript diagnostics are clean; three temporary regression tests reproduced the inline findings and were removed afterward. Real-picker and permission-restart behavior was not exercised.
gpt-6.1-sol | 𝕏
| await locked(fileLock(versionId), async () => { | ||
| if (to === "folder") { | ||
| const name = await freeName(folder, fileNameFor(record)); |
There was a problem hiding this comment.
[P1] Reserve destination names across Versions before copying
Two concurrent moves of different Versions with the same readable name can both select the same destination because these locks are per Version. I reproduced both records receiving the same fileName and both OPFS originals being deleted, leaving only one Version's bytes; the size check also passes when their sizes match.
Technical details
## Affected sites
- [store.ts:523-532](https://github.com/mia-cx/thalia/blob/a524c65347dd7370315d4488ba2c9b0700e1a6a9/apps/web/src/lib/downloads/store.ts#L523-L532): different Version locks do not serialize destination-name selection.
- [folder.ts:44-82](https://github.com/mia-cx/thalia/blob/a524c65347dd7370315d4488ba2c9b0700e1a6a9/apps/web/src/lib/downloads/folder.ts#L44-L82): freeName checks absence separately from getFileHandle(create:true), which can return the same existing file to both copies.
## Reproduction
Seed two completed OPFS records with distinct Version ids, identical titles/containers, and different same-sized bytes. With the existing FakeLocks fixture, await Promise.all of their move(id, "folder") calls. Both records end up with one identical filename rather than two unique destinations.
## Required outcome
Make destination-name selection and reservation atomic across Versions and tabs, so each copy owns its destination before writing or cleanup. Add concurrent same-name move coverage.
## API contracts
https://developer.mozilla.org/en-US/docs/Web/API/FileSystemDirectoryHandle/getFileHandle documents that create:true creates only when absent.
https://developer.mozilla.org/en-US/docs/Web/API/FileSystemFileHandle/createWritable documents that the default siloed mode permits multiple writers to one file.| function loadLinked(): Promise<void> { | ||
| loaded ??= folderSupported() | ||
| ? loadFolder() | ||
| .then((found) => { | ||
| handle = found; | ||
| }) | ||
| .catch(() => {}) | ||
| .then(publishFolder) | ||
| : Promise.resolve(); |
There was a problem hiding this comment.
[P1] Refresh linked-folder identity across tabs
The folder handle is cached for the lifetime of each page, but other tabs receive the new folder's records through the shared index without updating that handle. After changing folders in one tab, I reproduced Delete in another tab removing a retained file from the old folder instead of the intended download in the new folder when their readable names matched.
Technical details
## Affected sites
- [index.ts:37-69](https://github.com/mia-cx/thalia/blob/a524c65347dd7370315d4488ba2c9b0700e1a6a9/apps/web/src/lib/downloads/index.ts#L37-L69): loaded and handle remain cached after another tab updates IndexedDB.
- [store.ts:364-367](https://github.com/mia-cx/thalia/blob/a524c65347dd7370315d4488ba2c9b0700e1a6a9/apps/web/src/lib/downloads/store.ts#L364-L367): changed messages synchronize records only.
- [store.ts:877-883](https://github.com/mia-cx/thalia/blob/a524c65347dd7370315d4488ba2c9b0700e1a6a9/apps/web/src/lib/downloads/store.ts#L877-L883): deletion interprets shared fileName against the stale local folder. Reads, moves, and manifest writes likewise use that local handle.
## Reproduction
Using two stores with shared OPFS, FakeLocks, and channelPair, initially link both to X. Change the active store to Y through release/adopt, leaving X's files intact. Download a new Version into Y with a filename also present in X. Once the other store receives its record, remove the new Version there: X's retained file is deleted and Y's intended file remains.
## Required outcome
Coordinate folder selection/unlink state across tabs and ensure file operations interpret records against the matching folder identity. Updating the displayed folder name alone is insufficient; stale tabs must not read, delete, move, or mirror records through an old handle.| if (handle !== undefined && !(await handle.isSameEntry(picked))) { | ||
| // The old folder keeps its files and manifest; its rows leave the list. | ||
| handle = undefined; | ||
| await downloads.release(); | ||
| } | ||
| await saveFolder(picked); | ||
| handle = picked; | ||
| await publishFolder(); | ||
| await downloads.adopt(); |
There was a problem hiding this comment.
[P1] Coordinate folder changes with in-flight moves
Change and Unlink remain available while a copy is running, but move() captures its destination before copying and commits its location using the current linked folder afterward. I reproduced changing from X to Y mid-copy: the move succeeded and deleted the OPFS original, while Y's manifest referenced a file that existed only in X and downloads.file() rejected.
Technical details
## Affected sites
- [index.ts:143-151](https://github.com/mia-cx/thalia/blob/a524c65347dd7370315d4488ba2c9b0700e1a6a9/apps/web/src/lib/downloads/index.ts#L143-L151): release and folder replacement do not wait for active moves; unlink has the same missing coordination.
- [store.ts:516-532](https://github.com/mia-cx/thalia/blob/a524c65347dd7370315d4488ba2c9b0700e1a6a9/apps/web/src/lib/downloads/store.ts#L516-L532): copy uses a captured folder, then setFileName changes a folder-relative record and removes its OPFS source.
- [store.ts:242-248](https://github.com/mia-cx/thalia/blob/a524c65347dd7370315d4488ba2c9b0700e1a6a9/apps/web/src/lib/downloads/store.ts#L242-L248): the manifest destination is resolved again at commit time.
## Reproduction
Gate the first destination write for an OPFS-to-X move. While it is blocked, clear the linked handle, await release(), link Y, and await adopt(), matching link()'s ordering. Then release the copy gate. The moving record survives release because its fileName is still null; afterward it points into Y although the only bytes are in X.
## Required outcome
Serialize folder changes/unlinking with active moves, or otherwise prevent a move from committing a location against a different folder and deleting its recoverable source. Protect the operation across tabs as well as within one page, and cover switching folders during a gated copy.|
Pushed 2095bde to this PR: the folder test now uses in-app navigation and waits for the completed move, avoiding a reload that could interrupt index and manifest writes. Verification passed: 20 consecutive folder-test runs, all four download/offline browser tests together, three delayed-write trials, build, lint, and type checks. The full Task list (1/3 completed)
|
There was a problem hiding this comment.
Important
No new issues found in the incremental changes. The three previously reported concurrency findings remain unresolved, so the earlier request for changes still stands.
Reviewed changes since the previous Pullfrog review at a524c65, covering the index-write fix and browser-test updates.
- Skipped redundant writes: Manifest adoption now ignores Versions already listed locally, avoiding unnecessary index and manifest rewrites on restore.
- Added regression coverage: A new unit test verifies that restoring an already-populated folder index leaves the index untouched.
- Preserved active downloads: The browser test navigates within the app while the automatic move finishes and waits for its controls before checking storage.
- Updated recovery navigation: The manifest-recovery test uses
page.goto()instead of a raw reload before checking the restored Downloads page. - Verified web checks: All 294 web unit tests passed, including 34 download tests; Svelte/TypeScript diagnostics reported zero errors and warnings. Browser integration and real-picker/permission-restart behavior were not exercised in this run.
gpt-6.1-sol | 𝕏
2c5980f to
7eb0f81
Compare
7b2393d to
0ea0530
Compare
There was a problem hiding this comment.
Important
No new issues found in the rebase integration. The three previously reported concurrency findings remain unresolved, so the earlier request for changes still stands.
Reviewed changes since the prior review at 2c5980f, focusing on the folder feature's integration with the updated offline-player base.
- Preserved subtitle access: Kept saved-subtitle metadata and OPFS reads while resolving media files from their current storage location.
- Adapted deletion: Removed folder-backed media from the linked folder and saved subtitle files from browser storage.
- Retained playback UI: Combined folder-location captions with offline codec-support messages and preserved the base's Play action.
- Verified integration: All 307 web unit tests passed; Svelte/TypeScript checks reported zero errors and warnings. Browser integration and real-picker/permission-restart behavior were not exercised.
gpt-6.1-sol | 𝕏
7eb0f81 to
cfc4d76
Compare
There was a problem hiding this comment.
Important
No new issues found in this rebase integration. The three previously reported concurrency issues remain unresolved, so the earlier request for changes still stands.
Reviewed changes since 7eb0f81, covering integration with the updated offline-progress base. A range-diff confirmed all five folder-feature patches are unchanged.
- Integrated offline progress: Retained
downloads.file()as the playback file resolver while the updated base records progress using the saved caller and Version duration. - Checked inherited test changes: Reviewed the base's fresh-page navigation and offline playback progress test changes for interaction with folder downloads.
- Verified web checks: Passed all 307 web unit tests and Svelte/TypeScript diagnostics with zero errors or warnings. Browser integration and real-picker/permission-restart behavior were not exercised.
gpt-6.1-sol | 𝕏
|
The failed CI job has one test failure: Task list (1/3 completed)
|
There was a problem hiding this comment.
Important
No new issues found in this incremental review. The three previously reported concurrency findings remain unresolved, so the earlier request for changes still stands.
Reviewed changes since cfc4d76, covering saved subtitles moving with folder-backed downloads and their offline playback integration.
- Moved saved subtitles: Copied media and saved subtitles together in both directions, committed their location after all copies succeeded, and removed completed destination copies if a later copy failed.
- Resolved folder-backed files: Routed saved-subtitle reads and deletion through the download's current location, using readable sidecar names beside the media.
- Protected existing sidecars: Included subtitle destinations when choosing an available media filename, with a unit test covering a user's existing subtitle and the move back to browser storage.
- Extended offline playback coverage: Added browser-test assertions for recovered folder downloads playing with their saved forced subtitle, then moving both files back to browser storage.
- Verified web checks: Passed all 308 web unit tests and Svelte/TypeScript diagnostics with zero errors or warnings. Browser integration, the real picker, and permission restoration after a browser restart were not exercised in this run.
gpt-6.1-sol | 𝕏
dfc96f0 to
830a0de
Compare
9086388 to
d050798
Compare
830a0de to
510da17
Compare
There was a problem hiding this comment.
Important
No new issues found in this rebase integration. The three previously reported concurrency issues remain unresolved, so the earlier request for changes still stands.
Reviewed changes since dfc96f0, covering the updated base and its interaction with folder downloads. Range-diff confirmed all six folder-feature patches are unchanged.
- Integrated browser-test waits: Inherited the base's asynchronous predicate handling and wait for offline artwork cleanup; checked compatibility with the folder browser test.
- Preserved playback integration: Confirmed media and saved subtitles still resolve through
downloads.file(), with offline progress recording unchanged. - Verified web checks: Passed all 308 web unit tests and Svelte/TypeScript diagnostics with zero errors or warnings after restoring missing dependencies. Browser integration, the real picker, and permission restoration after restart were not exercised.
gpt-6.1-sol | 𝕏
On Chromium desktop browsers, the Downloads page can link a folder. Finished downloads move there under readable names, such as "Arrival (2016).mkv", and each row can move its file between the folder and browser storage. - Downloads still land in browser storage first, where they resume in place, then copy into the folder once finished. A copy is checked byte for byte before the original goes; a failed copy keeps it. - The folder's handle is kept in IndexedDB; when the browser drops access, the folder row offers Allow access. - .thalia/downloads.json in the folder lists its downloads, so a later session, or one without this browser's list, finds them again. - Browsers without showDirectoryPicker don't offer the folder. - downloads.file(versionId) reads a download's file wherever it is. Closes #182 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
… test Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Opening Downloads read the folder's manifest and rewrote the index even when every download it lists was already there, so a write ran on after the page showed its list. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Page.reload answers before the new document commits, so the "In disk" wait could pass in the old page. The navigation sent next then raced the reload, and under load it never answered and hung the test. Navigating answers only once the new page commits. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The offline player reads a download through downloads.file(name), the media by Version id and a saved subtitle by its name. Subtitles stayed in browser storage when the media moved to the folder, so a folder found again after the browser's data was cleared had none. They now move with the media and sit beside it under its name, such as Arrival (2016).en.forced.srt, where media players find them too, and file() reads either from wherever the download is kept. A name is free only when the media and every subtitle beside it are, so no file of the user's is overwritten. A failed copy removes what it copied and keeps the originals. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
d050798 to
550c7f9
Compare
510da17 to
60f6018
Compare
There was a problem hiding this comment.
Important
No new issues found in this rebase integration. The three previously reported concurrency findings remain unresolved, so the earlier request for changes still stands.
Reviewed changes since 510da17, covering the inherited browser-test changes and their interaction with folder downloads.
- Confirmed unchanged feature patches: Range-diff matched all six folder-feature commits to their previously reviewed versions; folder storage and playback behavior are unchanged.
- Checked menu synchronization: Reviewed the inherited subtitle-test helper's wait for menu focus after opening, before keyboard selection. The folder playback test uses an automatically selected forced subtitle and is unaffected.
- Checked assertion formatting: Verified the inherited subtitle assertion changes preserve their expectations.
Tests were not rerun in this review; the delta changes browser-test synchronization and formatting only. Real-picker and permission-restoration behavior remain unverified.
gpt-6.1-sol | 𝕏

Downloads only lived in the browser's private storage, where the user can't see them, back them up, or free space outside the app. Chromium desktop browsers can write to a real folder. This is #182 of PRD #173, stacked on #227.
On Chromium desktop, the Downloads page can link a folder. Finished downloads move there under readable names, such as
Arrival (2016).mkvorSeverance · S1, E2 · Good News About Hell.mkv, and each row can move its file between the folder and browser storage..thalia/downloads.jsonin the folder lists its downloads with their saved details, mirrored on every index write. When a folder is linked or allowed, Thalia adds the downloads it lists whose files are there whole. So a later session, or one whose browser data was cleared, finds them again.Needs accessand an Allow access button. Its rows hide their actions until access comes back, and they still open offline from their saved details.showDirectoryPicker(Safari, Firefox, Chrome on Android), the folder row isn't shown.downloads.file(name): the media by Version id, a saved subtitle (feat(web): show text and ASS subtitles in offline playback #233) by its name, from wherever the download is kept. Saved subtitles move with the media and sit beside it under its name, such asArrival (2016).en.forced.srt, so media players find them too and a folder found again after the browser's data is cleared still has them. A name counts as free only when the media and every subtitle beside it are, so no file of the user's is overwritten.Closes #182
Mia's call
.thalia/downloads.json), and moves cover finished downloads only, copying first and removing the source only after verification.Screenshots
Headless Chromium can't drive the real picker, so these use an OPFS folder named
diskas a stand-in (henceIn disk). The "Needs access" state is forced with a stubbed permission check.Tests
apps/server/src/api/download-folder-browser.test.ts(new, headless Chromium, stand-in folder as above): links the folder, downloads through the button, and checks the file is in the folder asArrival (2016).mkvwith the right size, its stored forced subtitle beside it asArrival (2016).en.forced.srt, and both gone from OPFS. It then deletes the browser's own index and stops the server; after a reload the download is listed again from the folder's manifest, its page opens offline, and Play on the Downloads page plays it from the folder with its subtitle showing. Move to browser storage brings both files back into OPFS. Without the subtitle move it fails on the folder listing. 10 of 10 passed with 12 cores busy. WithshowDirectoryPickerremoved, the folder row doesn't render. Before the fix, 2 of 13 runs failed: one read the page mid-reload, and I didn't keep the other's output. The test now waits instead of reading once, and the 8 runs since all passed, 3 of them alongside the other three download browser tests.lib/downloads/store.test.ts: a finished download lands in the folder under the Item's name and the manifest lists it; moving back restores the OPFS file and empties the manifest. A later session lists the folder's downloads from the manifest and skips one whose file is gone;file()reads it; unlinking drops the rows and leaves the file. A saved subtitle moves into the folder beside its media and back,file()reads it in both places, and a user's ownArrival (2016).en.srtalready there is left alone: the download takesArrival (2016) (2).mkvinstead.bun test: 308 pass.bun run check: clean.bun run lint: 0 errors, the 9 CSS warnings already on main.Not covered automatically: the real picker, a real folder on disk, and the permission prompt after a browser restart. Those are in the manual checks.
Made with claude-opus-5.5 using Claude Code in T3 Code.
🤖 Generated with Claude Code