Save path ./photos/YYYY/YYYY-MM/YYYY-MM-DD/YYYY-MM-DD.fileID.ext; download() from the cache first (closes #143) #146

Merged
clawbot merged 5 commits from issue-143-save-path-layout into next 2026-10-01 23:20:58 +02:00
Collaborator

Implements #143 as read in #143 (comment).

What changed

  • savePath(root, file) in src/library/content.ts alone builds YYYY/YYYY-MM/YYYY-MM-DD/YYYY-MM-DD.<fileID><ext>, dated in local time by takenAtOf(file), which toPhotoRecord uses too.
  • For a file in several albums, representative in src/library/records.ts picks one copy (most recently synced, lowest album ID on a tie); the record, savePath, isLocal, download() and lib.backup() all use it.
  • Library.open defaults downloadDirectory to resolve("photos") and rejects an empty one.
  • photo.download() and lib.backup() put the original at its save path through placeOriginal, copied from the cache when held there. Every album links that path.
  • storedOriginal and the live-photo JSON helpers take the name without its extension, so the cache (<fileID>) and the save path (YYYY-MM-DD.<fileID>) share them.
  • Backup sidecars sit beside the original; stale-link removal recognises only links into the date folders; leftover temp files are cleared in every date folder.

Worth knowing

  • A Photo keeps the copy it was read from; savePath, isLocal and download() (which passes it to the cache) all use it, so they agree after a refresh changes the date or removes the file.
  • A scoped backup reads every album to pick each file's copy, and reports a failure under that copy's album.
  • A failed fetch can leave an empty date folder.

Disclosures

  • Deviation: savePath and isLocal come through SavePathLookup, not PhotoContent, so neither needs a content cache.
  • Deviation: the test that lib.backup() throws with no download directory now passes an empty one.
  • Judgement call: the private ContentCache.download is renamed fetchInto.
  • Judgement call: TODO.md history entries naming originals/ are left as they are.

Model: opus-5-5

Implements https://git.eeqj.de/sneak/quak/issues/143 as read in https://git.eeqj.de/sneak/quak/issues/143#issuecomment-107571. **What changed** - `savePath(root, file)` in `src/library/content.ts` alone builds `YYYY/YYYY-MM/YYYY-MM-DD/YYYY-MM-DD.<fileID><ext>`, dated in local time by `takenAtOf(file)`, which `toPhotoRecord` uses too. - For a file in several albums, `representative` in `src/library/records.ts` picks one copy (most recently synced, lowest album ID on a tie); the record, `savePath`, `isLocal`, `download()` and `lib.backup()` all use it. - `Library.open` defaults `downloadDirectory` to `resolve("photos")` and rejects an empty one. - `photo.download()` and `lib.backup()` put the original at its save path through `placeOriginal`, copied from the cache when held there. Every album links that path. - `storedOriginal` and the live-photo JSON helpers take the name without its extension, so the cache (`<fileID>`) and the save path (`YYYY-MM-DD.<fileID>`) share them. - Backup sidecars sit beside the original; stale-link removal recognises only links into the date folders; leftover temp files are cleared in every date folder. **Worth knowing** - A `Photo` keeps the copy it was read from; `savePath`, `isLocal` and `download()` (which passes it to the cache) all use it, so they agree after a refresh changes the date or removes the file. - A scoped backup reads every album to pick each file's copy, and reports a failure under that copy's album. - A failed fetch can leave an empty date folder. **Disclosures** - Deviation: `savePath` and `isLocal` come through `SavePathLookup`, not `PhotoContent`, so neither needs a content cache. - Deviation: the test that `lib.backup()` throws with no download directory now passes an empty one. - Judgement call: the private `ContentCache.download` is renamed `fetchInto`. - Judgement call: `TODO.md` history entries naming `originals/` are left as they are. Model: opus-5-5
clawbot added the needs-review label 2026-10-01 20:18:22 +02:00
clawbot self-assigned this 2026-10-01 20:18:22 +02:00
Author
Collaborator

Review: fail.

  1. src/library/content.ts:409 and :427, test/cli/backup.test.ts:1101-1110, :1176, :1192, :1237: the PR keeps handling for a live photo that a version before #107 saved as one ZIP. placeOriginal deletes whatever is at the save path to clear such a ZIP, and earlierZIP plants one for three backup tests. quak has no installed base, so this is old-data compatibility, which the standing rule forbids. Acceptable: no code, comment or test about an earlier version's ZIP. The hash-failure test checks that nothing is stored, without a planted ZIP.

  2. src/library/index.ts:333: photo.savePath throws once the photo's file has left the library, for example when a caller holds a Photo while the background refresh removes a file that was trashed in Ente. The issue requires savePath to always have a value, and the README documents it as a string. Acceptable: savePath returns a string for every Photo, including one whose file a later refresh removed.

  3. src/library/index.ts:376: Library.open({ downloadDirectory: "" }) is accepted. Save paths then become relative to whatever the working directory is at each call, and photo.download() writes there. lib.backup() rejects the same value and says to open the library with a directory. The reworked test at test/cli/backup.test.ts:265 checks only the backup side. Acceptable: Library.open rejects an empty downloadDirectory with a clear error, and a test covers that.

  4. src/backup.ts:420-426: a backup now clears leftover temp files only in the date folders (YYYY/YYYY-MM/YYYY-MM-DD/) of files in its scope. Say a backup or photo.download() is killed while writing a file, and that file is later out of scope, deleted, or given a new date. Its temp file then stays for good, so README.md:594-596 ("the next backup deletes those") is no longer true. Acceptable: the backup clears leftovers in every date folder under its directory.

  5. TODO.md:37: the new entry says the backup has no originals/ folder. That describes the current layout by naming the one it replaced. Acceptable: describe the current layout without naming originals/.

Model: opus-5-5

Review: fail. 1. `src/library/content.ts:409` and `:427`, `test/cli/backup.test.ts:1101-1110`, `:1176`, `:1192`, `:1237`: the PR keeps handling for a live photo that a version before https://git.eeqj.de/sneak/quak/issues/107 saved as one ZIP. `placeOriginal` deletes whatever is at the save path to clear such a ZIP, and `earlierZIP` plants one for three backup tests. quak has no installed base, so this is old-data compatibility, which the standing rule forbids. Acceptable: no code, comment or test about an earlier version's ZIP. The hash-failure test checks that nothing is stored, without a planted ZIP. 2. `src/library/index.ts:333`: `photo.savePath` throws once the photo's file has left the library, for example when a caller holds a `Photo` while the background refresh removes a file that was trashed in Ente. The issue requires `savePath` to always have a value, and the README documents it as a `string`. Acceptable: `savePath` returns a string for every `Photo`, including one whose file a later refresh removed. 3. `src/library/index.ts:376`: `Library.open({ downloadDirectory: "" })` is accepted. Save paths then become relative to whatever the working directory is at each call, and `photo.download()` writes there. `lib.backup()` rejects the same value and says to open the library with a directory. The reworked test at `test/cli/backup.test.ts:265` checks only the backup side. Acceptable: `Library.open` rejects an empty `downloadDirectory` with a clear error, and a test covers that. 4. `src/backup.ts:420-426`: a backup now clears leftover temp files only in the date folders (`YYYY/YYYY-MM/YYYY-MM-DD/`) of files in its scope. Say a backup or `photo.download()` is killed while writing a file, and that file is later out of scope, deleted, or given a new date. Its temp file then stays for good, so `README.md:594-596` ("the next backup deletes those") is no longer true. Acceptable: the backup clears leftovers in every date folder under its directory. 5. `TODO.md:37`: the new entry says the backup has no `originals/` folder. That describes the current layout by naming the one it replaced. Acceptable: describe the current layout without naming `originals/`. Model: opus-5-5
clawbot added needs-rework and removed needs-review labels 2026-10-01 20:29:55 +02:00
clawbot added needs-review and removed needs-rework labels 2026-10-01 20:46:03 +02:00
Author
Collaborator

Review: fail.

  1. src/library/records.ts:224, read through src/library/read.ts:47: a Photo keeps the first membership of its file (memberships[0]). Its record, and so photo.takenAt and photo.year, comes from the most recently synced membership. When the memberships differ, for example when a date edited in Ente has reached one album's copy but not yet another's, photo.savePath and isLocal use a different date from photo.takenAt. The issue says the folder date is the one photo.year and the timeline use. The comment at read.ts:47 says the Photo keeps "the file the record was made from", and that is not true. Acceptable: a Photo keeps the same membership its record is read from, so savePath always carries the date of photo.takenAt. A test should cover one file with two memberships whose editedTime differ.

Model: opus-5-5

Review: fail. 1. `src/library/records.ts:224`, read through `src/library/read.ts:47`: a `Photo` keeps the first membership of its file (`memberships[0]`). Its record, and so `photo.takenAt` and `photo.year`, comes from the most recently synced membership. When the memberships differ, for example when a date edited in Ente has reached one album's copy but not yet another's, `photo.savePath` and `isLocal` use a different date from `photo.takenAt`. The issue says the folder date is the one `photo.year` and the timeline use. The comment at `read.ts:47` says the `Photo` keeps "the file the record was made from", and that is not true. Acceptable: a `Photo` keeps the same membership its record is read from, so `savePath` always carries the date of `photo.takenAt`. A test should cover one file with two memberships whose `editedTime` differ. Model: opus-5-5
clawbot added needs-rework and removed needs-review labels 2026-10-01 21:26:18 +02:00
clawbot added needs-review and removed needs-rework labels 2026-10-01 21:43:54 +02:00
Author
Collaborator

Review: fail.

  1. src/library/content.ts:538-539: photo.download() dates the save path from whichever album's copy of the file store.getFileByID returns first (src/library/index.ts:427), not from the copy the Photo keeps. Take one file in two albums whose copies carry different edited dates, the case the new test at test/library/content-library.test.ts:410 sets up. download() writes under the older date while photo.savePath names the newer one, and isLocal stays false afterwards. The README says download() puts the original at savePath and that isLocal is then true. Acceptable: download() writes to the path photo.savePath gives, for example by having the Photo pass its own copy of the file to the cache. That test also calls download() and checks that it returns savePath and that isLocal is true.

  2. src/backup.ts:400 and :528: lib.backup() saves each original under the date of the first album's copy of the file. It then looks for each album's link under that album's own copy. In the same two-album case the original lands under a date that is not photo.takenAt's, isLocal stays false, and the second album's folder gets no link. Before this PR every album linked the one original. Acceptable: the backup dates a file from the same copy as photo.savePath, every album links that one path, and a backup test covers one file in two albums with different edited dates.

Model: opus-5-5

Review: fail. 1. `src/library/content.ts:538-539`: `photo.download()` dates the save path from whichever album's copy of the file `store.getFileByID` returns first (`src/library/index.ts:427`), not from the copy the `Photo` keeps. Take one file in two albums whose copies carry different edited dates, the case the new test at `test/library/content-library.test.ts:410` sets up. `download()` writes under the older date while `photo.savePath` names the newer one, and `isLocal` stays false afterwards. The README says `download()` puts the original at `savePath` and that `isLocal` is then true. Acceptable: `download()` writes to the path `photo.savePath` gives, for example by having the `Photo` pass its own copy of the file to the cache. That test also calls `download()` and checks that it returns `savePath` and that `isLocal` is true. 2. `src/backup.ts:400` and `:528`: `lib.backup()` saves each original under the date of the first album's copy of the file. It then looks for each album's link under that album's own copy. In the same two-album case the original lands under a date that is not `photo.takenAt`'s, `isLocal` stays false, and the second album's folder gets no link. Before this PR every album linked the one original. Acceptable: the backup dates a file from the same copy as `photo.savePath`, every album links that one path, and a backup test covers one file in two albums with different edited dates. Model: opus-5-5
clawbot added needs-rework and removed needs-review labels 2026-10-01 21:55:13 +02:00
Author
Collaborator

Rework decision on the review above. Both findings have one cause: different code picks a different album's copy of the same file.

The fix uses one rule for every caller: the copy toPhotoRecord reads from, the most recently synced copy, with the lowest album ID breaking a tie. Make that rule one exported function in src/library/records.ts. store.getFileByID returns that copy, so the cache and download() date by it. lib.backup() takes each file's copy by the same rule, and every album links that one path. With this, photo.takenAt, savePath, isLocal, download() and the backup agree by construction. The tests the findings ask for cover one file in two albums with different edited dates, for download() and for lib.backup().

Model: opus-5-5

Rework decision on the review above. Both findings have one cause: different code picks a different album's copy of the same file. The fix uses one rule for every caller: the copy `toPhotoRecord` reads from, the most recently synced copy, with the lowest album ID breaking a tie. Make that rule one exported function in `src/library/records.ts`. `store.getFileByID` returns that copy, so the cache and `download()` date by it. `lib.backup()` takes each file's copy by the same rule, and every album links that one path. With this, `photo.takenAt`, `savePath`, `isLocal`, `download()` and the backup agree by construction. The tests the findings ask for cover one file in two albums with different edited dates, for `download()` and for `lib.backup()`. Model: opus-5-5
clawbot added needs-review and removed needs-rework labels 2026-10-01 22:16:52 +02:00
Author
Collaborator

Review: fail.

  1. src/library/read.ts:136 (Photo.download(), through src/library/content.ts:538): download() takes the photo's date from the album copy the store holds when it is called. savePath and isLocal take it from the copy the Photo was read with. Hold a Photo across a refresh that brings a date edited in Ente, then call download(): the original is written under the new date, photo.savePath still names the old path, and isLocal stays false. The README's Read surface (README.md:729-733) says download() puts the original at savePath and that isLocal is then true. Acceptable: for any one Photo, download() writes to the path its savePath names, for example by passing the Photo's own copy of the file to the cache. A test should read a Photo, refresh with a new edited date, call download(), and check that the returned path is savePath and that isLocal is true.

  2. TODO.md:28: the branch no longer merges into the current next, because both add the top entry under "Completed Steps". Acceptable: rebase onto next and keep both entries.

Model: opus-5-5

Review: fail. 1. `src/library/read.ts:136` (`Photo.download()`, through `src/library/content.ts:538`): `download()` takes the photo's date from the album copy the store holds when it is called. `savePath` and `isLocal` take it from the copy the `Photo` was read with. Hold a `Photo` across a refresh that brings a date edited in Ente, then call `download()`: the original is written under the new date, `photo.savePath` still names the old path, and `isLocal` stays false. The README's Read surface (`README.md:729-733`) says `download()` puts the original at `savePath` and that `isLocal` is then true. Acceptable: for any one `Photo`, `download()` writes to the path its `savePath` names, for example by passing the `Photo`'s own copy of the file to the cache. A test should read a `Photo`, refresh with a new edited date, call `download()`, and check that the returned path is `savePath` and that `isLocal` is true. 2. `TODO.md:28`: the branch no longer merges into the current `next`, because both add the top entry under "Completed Steps". Acceptable: rebase onto `next` and keep both entries. Model: opus-5-5
clawbot added needs-rework and removed needs-review labels 2026-10-01 22:48:27 +02:00
clawbot added 5 commits 2026-10-01 23:01:39 +02:00
Each original's save path is now
YYYY/YYYY-MM/YYYY-MM-DD/YYYY-MM-DD.<fileID><ext> under the library's
download directory, dated by takenAt in the machine's time zone. The
directory defaults to photos/ in the working directory, resolved once at
open. savePath is always a string, with or without a content cache, and
isLocal is true only when the original is there.

photo.download() puts the original at its save path: copied from the
cache when the cache holds it, fetched straight there otherwise.
lib.backup() uses the same code for each file, writes each file's JSON
beside it, and links collections/ to the save paths. The backup has no
originals/ folder.

Model: opus-5-5
A Photo now keeps the file its record was made from (the record
projection holds one membership of each file), so savePath and isLocal
still answer after a refresh removes the file.

Library.open rejects an empty downloadDirectory.

A backup clears leftover temp files in every date folder under its
directory, not only in those of the files in its scope.

placeOriginal no longer deletes what is at the save path before copying
a live photo, and the tests that depended on that are removed.

The TODO.md entry for issue 143 describes only the current layout.

Model: opus-5-5
A Photo kept the first membership of its file, while its record (and so
takenAt and year) comes from the most recently synced one. The record
projection now keeps that same membership for the Photo, so savePath
and isLocal carry the date of takenAt. A test covers one file in two
albums whose edited times differ.

Model: opus-5-5
Export `representative` from records.ts: the album's copy of a file that
its record is read from. `store.getFileByID` now returns that copy, so
`download()` writes under the date `photo.savePath` names. `lib.backup()`
takes each file's copy by the same rule from all of its albums, and every
album links that one path. Tests cover one file in two albums whose copies
carry different edited dates, for `download()` and for `lib.backup()`.

Model: opus-5-5
Photo.download() now hands the content cache the file copy the Photo
holds, the one savePath and isLocal are dated from, instead of the
fileID the cache then looked up in the store. A Photo held across a
refresh that brings a date edited in Ente is now downloaded to the path
its savePath names. A test covers that case.

Model: opus-5-5
clawbot force-pushed issue-143-save-path-layout from 26bfce8b1d to fc06b9100b 2026-10-01 23:01:39 +02:00 Compare
clawbot added needs-review and removed needs-rework labels 2026-10-01 23:01:58 +02:00
Author
Collaborator

Review: pass.

Model: opus-5-5

Review: pass. Model: opus-5-5
clawbot merged commit 10e1a9ef39 into next 2026-10-01 23:20:58 +02:00
clawbot deleted branch issue-143-save-path-layout 2026-10-01 23:20:58 +02:00
Sign in to join this conversation.